Re: [PATCH v6 05/15] sequencer: introduce the `merge` command
Hi Phillip,
On Sat, 14 Apr 2018, Johannes Schindelin wrote:
> On Fri, 13 Apr 2018, Phillip Wood wrote:
>
> > On 13/04/18 11:12, Phillip Wood wrote:
> > > @@ -3030,7 +3029,8 @@ static int pick_commits(struct todo_list
> > > *todo_list, struct replay_opts *opts)
> > > return error(_("unknown command %d"), item->command);
> > >
> > > if (res < 0 && (item->command == TODO_LABEL ||
> > > - item->command == TODO_RESET)) {
> > > + item->command == TODO_RESET ||
> > > + item->command == TODO_MERGE)) {
> >
> > Unfortunately it's not as simple as that - we only want to reschedule if
> > merge_recursive() fails, not if run_git_commit() does.
>
> Correct. How about introducing a flag `reschedule` that is passed to
> do_label(), do_reset() and do_merge()?
>
> Seeing as do_reset() and do_merge() already have a replay_opts parameter,
> we could add a field `needs_rescheduling` and pass the replay_opts also to
> do_label().
Nevermind, we already use the trick in do_pick_commit() that -1 means:
reschedule, 0 means: success, and 1 means: merge conflicts (don't bother
rescheduling).
It just had slipped my mind; I use the same convention in do_merge() now.
Thank you so much for your review and suggestions. I *think* I
incorporated it all.
Ciao,
Dscho
Re: [PATCH v6 05/15] sequencer: introduce the `merge` command
On 14/04/18 01:51, Johannes Schindelin wrote:
> Hi Phillip,
>
> On Fri, 13 Apr 2018, Phillip Wood wrote:
>
>> On 13/04/18 11:12, Phillip Wood wrote:
>>> @@ -3030,7 +3029,8 @@ static int pick_commits(struct todo_list *todo_list,
>>> struct replay_opts *opts)
>>> return error(_("unknown command %d"), item->command);
>>>
>>> if (res < 0 && (item->command == TODO_LABEL ||
>>> - item->command == TODO_RESET)) {
>>> + item->command == TODO_RESET ||
>>> + item->command == TODO_MERGE)) {
>>
>> Unfortunately it's not as simple as that - we only want to reschedule if
>> merge_recursive() fails, not if run_git_commit() does.
>
> Correct. How about introducing a flag `reschedule` that is passed to
> do_label(), do_reset() and do_merge()?
That would work (I was thinking about using return codes but having a
parameter is a better idea). Do you want me to re-roll the fixups or are
you happy to make the changes in your next version?
>
> Seeing as do_reset() and do_merge() already have a replay_opts parameter,
> we could add a field `needs_rescheduling` and pass the replay_opts also to
> do_label().
I'm slightly wary of putting state in an options structure but maybe it
doesn't matter.
Best Wishes
Phillip
> Ciao,
> Dscho
>
Re: [PATCH v6 05/15] sequencer: introduce the `merge` command
Hi Phillip,
On Fri, 13 Apr 2018, Phillip Wood wrote:
> On 13/04/18 11:12, Phillip Wood wrote:
> > @@ -3030,7 +3029,8 @@ static int pick_commits(struct todo_list *todo_list,
> > struct replay_opts *opts)
> > return error(_("unknown command %d"), item->command);
> >
> > if (res < 0 && (item->command == TODO_LABEL ||
> > - item->command == TODO_RESET)) {
> > + item->command == TODO_RESET ||
> > + item->command == TODO_MERGE)) {
>
> Unfortunately it's not as simple as that - we only want to reschedule if
> merge_recursive() fails, not if run_git_commit() does.
Correct. How about introducing a flag `reschedule` that is passed to
do_label(), do_reset() and do_merge()?
Seeing as do_reset() and do_merge() already have a replay_opts parameter,
we could add a field `needs_rescheduling` and pass the replay_opts also to
do_label().
Ciao,
Dscho
Re: [PATCH v6 05/15] sequencer: introduce the `merge` command
On 13/04/18 11:12, Phillip Wood wrote:
> On 10/04/18 13:29, Johannes Schindelin wrote:
>> +static int do_merge(struct commit *commit, const char *arg, int arg_len,
>> +int flags, struct replay_opts *opts)
>> +{
>> +int run_commit_flags = (flags & TODO_EDIT_MERGE_MSG) ?
>> +EDIT_MSG | VERIFY_MSG : 0;
>> +struct strbuf ref_name = STRBUF_INIT;
>> +struct commit *head_commit, *merge_commit, *i;
>> +struct commit_list *bases, *j, *reversed = NULL;
>> +struct merge_options o;
>> +int merge_arg_len, oneline_offset, ret;
>> +static struct lock_file lock;
>> +const char *p;
>> +
>> +oneline_offset = arg_len;
>> +merge_arg_len = strcspn(arg, " \t\n");
>> +p = arg + merge_arg_len;
>> +p += strspn(p, " \t\n");
>> +if (*p == '#' && (!p[1] || isspace(p[1]))) {
>> +p += 1 + strspn(p + 1, " \t\n");
>> +oneline_offset = p - arg;
>> +} else if (p - arg < arg_len)
>> +BUG("octopus merges are not supported yet: '%s'", p);
>> +
>> +strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg);
>> +merge_commit = lookup_commit_reference_by_name(ref_name.buf);
>> +if (!merge_commit) {
>> +/* fall back to non-rewritten ref or commit */
>> +strbuf_splice(&ref_name, 0, strlen("refs/rewritten/"), "", 0);
>> +merge_commit = lookup_commit_reference_by_name(ref_name.buf);
>> +}
>> +if (!merge_commit) {
>> +error(_("could not resolve '%s'"), ref_name.buf);
>> +strbuf_release(&ref_name);
>> +return -1;
>> +}
>> +
>> +if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
>> +return -1;
>> +
>> +head_commit = lookup_commit_reference_by_name("HEAD");
>> +if (!head_commit) {
>> +rollback_lock_file(&lock);
>> +return error(_("cannot merge without a current revision"));
>> +}
>> +
>> +if (commit) {
>> +const char *message = get_commit_buffer(commit, NULL);
>> +const char *body;
>> +int len;
>> +
>> +if (!message) {
>> +rollback_lock_file(&lock);
>> +return error(_("could not get commit message of '%s'"),
>> + oid_to_hex(&commit->object.oid));
>> +}
>> +write_author_script(message);
>> +find_commit_subject(message, &body);
>> +len = strlen(body);
>> +if (write_message(body, len, git_path_merge_msg(), 0) < 0) {
>> +error_errno(_("could not write '%s'"),
>> +git_path_merge_msg());
>> +unuse_commit_buffer(commit, message);
>> +rollback_lock_file(&lock);
>> +return -1;
>> +}
>> +unuse_commit_buffer(commit, message);
>> +} else {
>> +struct strbuf buf = STRBUF_INIT;
>> +int len;
>> +
>> +strbuf_addf(&buf, "author %s", git_author_info(0));
>> +write_author_script(buf.buf);
>> +strbuf_reset(&buf);
>> +
>> +if (oneline_offset < arg_len) {
>> +p = arg + oneline_offset;
>> +len = arg_len - oneline_offset;
>> +} else {
>> +strbuf_addf(&buf, "Merge branch '%.*s'",
>> +merge_arg_len, arg);
>> +p = buf.buf;
>> +len = buf.len;
>> +}
>> +
>> +if (write_message(p, len, git_path_merge_msg(), 0) < 0) {
>> +error_errno(_("could not write '%s'"),
>> +git_path_merge_msg());
>> +strbuf_release(&buf);
>> +rollback_lock_file(&lock);
>> +return -1;
>> +}
>> +strbuf_release(&buf);
>> +}
>> +
>> +write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
>> + git_path_merge_head(), 0);
>> +write_message("no-ff", 5, git_path_merge_mode(), 0);
>> +
>> +bases = get_merge_bases(head_commit, merge_commit);
>> +for (j = bases; j; j = j->next)
>> +commit_list_insert(j->item, &reversed);
>> +free_commit_list(bases);
>> +
>> +read_cache();
>> +init_merge_options(&o);
>> +o.branch1 = "HEAD";
>> +o.branch2 = ref_name.buf;
>> +o.buffer_output = 2;
>> +
>> +ret = merge_recursive(&o, head_commit, merge_commit, reversed, &i);
>> +if (!ret)
>> +rerere(opts->allow_rerere_auto);
>> +if (ret <= 0)
>> +fputs(o.obuf.buf, stdout);
>> +strbuf_release(&o.obuf);
>> +if (ret < 0) {
>> +strbuf_release(&ref_name);
>> +rollback_lock_file(&lock);
>> +return error(_("conflicts while merging '%.*s'"),
>> + merge_arg_len, arg);
>> +}
>
> If there are conflicts
Re: [PATCH v6 05/15] sequencer: introduce the `merge` command
On 10/04/18 13:29, Johannes Schindelin wrote:
> +static int do_merge(struct commit *commit, const char *arg, int arg_len,
> + int flags, struct replay_opts *opts)
> +{
> + int run_commit_flags = (flags & TODO_EDIT_MERGE_MSG) ?
> + EDIT_MSG | VERIFY_MSG : 0;
> + struct strbuf ref_name = STRBUF_INIT;
> + struct commit *head_commit, *merge_commit, *i;
> + struct commit_list *bases, *j, *reversed = NULL;
> + struct merge_options o;
> + int merge_arg_len, oneline_offset, ret;
> + static struct lock_file lock;
> + const char *p;
> +
> + oneline_offset = arg_len;
> + merge_arg_len = strcspn(arg, " \t\n");
> + p = arg + merge_arg_len;
> + p += strspn(p, " \t\n");
> + if (*p == '#' && (!p[1] || isspace(p[1]))) {
> + p += 1 + strspn(p + 1, " \t\n");
> + oneline_offset = p - arg;
> + } else if (p - arg < arg_len)
> + BUG("octopus merges are not supported yet: '%s'", p);
> +
> + strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg);
> + merge_commit = lookup_commit_reference_by_name(ref_name.buf);
> + if (!merge_commit) {
> + /* fall back to non-rewritten ref or commit */
> + strbuf_splice(&ref_name, 0, strlen("refs/rewritten/"), "", 0);
> + merge_commit = lookup_commit_reference_by_name(ref_name.buf);
> + }
> + if (!merge_commit) {
> + error(_("could not resolve '%s'"), ref_name.buf);
> + strbuf_release(&ref_name);
> + return -1;
> + }
> +
> + if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> + return -1;
> +
> + head_commit = lookup_commit_reference_by_name("HEAD");
> + if (!head_commit) {
> + rollback_lock_file(&lock);
> + return error(_("cannot merge without a current revision"));
> + }
> +
> + if (commit) {
> + const char *message = get_commit_buffer(commit, NULL);
> + const char *body;
> + int len;
> +
> + if (!message) {
> + rollback_lock_file(&lock);
> + return error(_("could not get commit message of '%s'"),
> + oid_to_hex(&commit->object.oid));
> + }
> + write_author_script(message);
> + find_commit_subject(message, &body);
> + len = strlen(body);
> + if (write_message(body, len, git_path_merge_msg(), 0) < 0) {
> + error_errno(_("could not write '%s'"),
> + git_path_merge_msg());
> + unuse_commit_buffer(commit, message);
> + rollback_lock_file(&lock);
> + return -1;
> + }
> + unuse_commit_buffer(commit, message);
> + } else {
> + struct strbuf buf = STRBUF_INIT;
> + int len;
> +
> + strbuf_addf(&buf, "author %s", git_author_info(0));
> + write_author_script(buf.buf);
> + strbuf_reset(&buf);
> +
> + if (oneline_offset < arg_len) {
> + p = arg + oneline_offset;
> + len = arg_len - oneline_offset;
> + } else {
> + strbuf_addf(&buf, "Merge branch '%.*s'",
> + merge_arg_len, arg);
> + p = buf.buf;
> + len = buf.len;
> + }
> +
> + if (write_message(p, len, git_path_merge_msg(), 0) < 0) {
> + error_errno(_("could not write '%s'"),
> + git_path_merge_msg());
> + strbuf_release(&buf);
> + rollback_lock_file(&lock);
> + return -1;
> + }
> + strbuf_release(&buf);
> + }
> +
> + write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
> + git_path_merge_head(), 0);
> + write_message("no-ff", 5, git_path_merge_mode(), 0);
> +
> + bases = get_merge_bases(head_commit, merge_commit);
> + for (j = bases; j; j = j->next)
> + commit_list_insert(j->item, &reversed);
> + free_commit_list(bases);
> +
> + read_cache();
> + init_merge_options(&o);
> + o.branch1 = "HEAD";
> + o.branch2 = ref_name.buf;
> + o.buffer_output = 2;
> +
> + ret = merge_recursive(&o, head_commit, merge_commit, reversed, &i);
> + if (!ret)
> + rerere(opts->allow_rerere_auto);
> + if (ret <= 0)
> + fputs(o.obuf.buf, stdout);
> + strbuf_release(&o.obuf);
> + if (ret < 0) {
> + strbuf_release(&ref_name);
> + rollback_lock_file(&lock);
> + return error(_("conflicts while merging '%.*s'"),
> + merge_arg_len, arg);
> + }
If there are conflicts then ret == 0 rather than -1
> +
> + if (active_cache_c
