Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Jacob Keller]

On Wed, Jan 31, 2018 at 5:48 AM, Johannes Schindelin
 wrote:
> Hi Jake & Phillip,
>
> On Mon, 29 Jan 2018, Johannes Schindelin wrote:
>
>> On Sat, 20 Jan 2018, Jacob Keller wrote:
>>
>> > On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood  
>> > wrote:
>> > > On 18/01/18 15:35, Johannes Schindelin wrote:
>> > >>
>> > >> This patch adds the `merge` command, with the following syntax:
>> > >>
>> > >>   merge   
>> > >
>> > > I'm concerned that this will be confusing for users. All of the other
>> > > rebase commands replay the changes in the commit hash immediately
>> > > following the command name. This command instead uses the first
>> > > commit to specify the message which is different to both 'git merge'
>> > > and the existing rebase commands. I wonder if it would be clearer to
>> > > have 'merge -C   ...' instead so it's clear which
>> > > argument specifies the message and which the remote head to merge.
>> > > It would also allow for 'merge -c   ...' in the future
>> > > for rewording an existing merge message and also avoid the slightly
>> > > odd 'merge -  ...'. Where it's creating new merges I'm not sure
>> > > it's a good idea to encourage people to only have oneline commit
>> > > messages by making it harder to edit them, perhaps it could take
>> > > another argument to mean open the editor or not, though as Jake said
>> > > I guess it's not that common.
>> >
>> > I actually like the idea of re-using commit message options like -C,
>> > -c,  and -m, so we could do:
>> >
>> > merge -C  ... to take message from commit
>>
>> That is exactly how the Git garden shears do it.
>>
>> I found it not very readable. That is why I wanted to get away from it in
>> --recreate-merges.
>
> I made up my mind. Even if it is not very readable, it is still better
> than the `merge A B` where the order of A and B magically determines their
> respective roles.
>
>> > merge -c  ...  to take the message from commit and open editor to 
>> > edit
>> > merge -m "" ... to take the message from the quoted test
>> > merge ... to merge and open commit editor with default message
>
> I will probably implement -c, but not -m, and will handle the absence of
> the -C and -c options to construct a default merge message which can then
> be edited.
>
> The -m option just opens such a can of worms with dequoting, that's why I
> do not want to do that.
>

I agree, I don't see a need for "-m".

> BTW I am still trying to figure out how to present the oneline of the
> commit to merge (which is sometimes really helpful because the label might
> be less than meaningful) while *still* allowing for octopus merges.
>
> So far, what I have is this:
>
> merge   
>
> and for octopus:
>
> merge  " ..." ...
>
> I think with the -C syntax, it would become something like
>
> merge -C   # 
>

I like this, especially given you added the "#" for one of the other
new commands as well, (reset I think?)

> and
>
> merge -C   ...
> # Merging: 
> # Merging: 
> # ...
>

I really like this, since you can show each oneline for all the
to-merges for an octopus.

> The only qualm I have about this is that `#` really *is* a valid ref name.
> (Seriously, it is...). So that would mean that I'd have to disallow `#`
> as a label specifically.
>
> Thoughts?
>

I think it's fine to disallow # as a label.

Thanks,
Jake

> Ciao,
> Dscho

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Phillip Wood]

On 31/01/18 13:48, Johannes Schindelin wrote:
> Hi Jake & Phillip,
> 
> On Mon, 29 Jan 2018, Johannes Schindelin wrote:
> 
>> On Sat, 20 Jan 2018, Jacob Keller wrote:
>>
>>> On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood  
>>> wrote:
 On 18/01/18 15:35, Johannes Schindelin wrote:
>
> This patch adds the `merge` command, with the following syntax:
>
>   merge   

 I'm concerned that this will be confusing for users. All of the other
 rebase commands replay the changes in the commit hash immediately
 following the command name. This command instead uses the first
 commit to specify the message which is different to both 'git merge'
 and the existing rebase commands. I wonder if it would be clearer to
 have 'merge -C   ...' instead so it's clear which
 argument specifies the message and which the remote head to merge.
 It would also allow for 'merge -c   ...' in the future
 for rewording an existing merge message and also avoid the slightly
 odd 'merge -  ...'. Where it's creating new merges I'm not sure
 it's a good idea to encourage people to only have oneline commit
 messages by making it harder to edit them, perhaps it could take
 another argument to mean open the editor or not, though as Jake said
 I guess it's not that common.
>>>
>>> I actually like the idea of re-using commit message options like -C,
>>> -c,  and -m, so we could do:
>>>
>>> merge -C  ... to take message from commit
>>
>> That is exactly how the Git garden shears do it.
>>
>> I found it not very readable. That is why I wanted to get away from it in
>> --recreate-merges.
> 
> I made up my mind. Even if it is not very readable, it is still better
> than the `merge A B` where the order of A and B magically determines their
> respective roles.
> 
>>> merge -c  ...  to take the message from commit and open editor to 
>>> edit
>>> merge -m "" ... to take the message from the quoted test
>>> merge ... to merge and open commit editor with default message
> 
> I will probably implement -c, but not -m, and will handle the absence of
> the -C and -c options to construct a default merge message which can then
> be edited.

That sounds like a good plan (-c can always be added later), I'm really
pleased you changed your mind on this, having the -C may be a bit ugly
but I think it is valuable to have some way of distinguishing the
message commit from the merge heads.

> The -m option just opens such a can of worms with dequoting, that's why I
> do not want to do that.
> 
> BTW I am still trying to figure out how to present the oneline of the
> commit to merge (which is sometimes really helpful because the label might
> be less than meaningful) while *still* allowing for octopus merges.
> 
> So far, what I have is this:
> 
>   merge   
> 
> and for octopus:
> 
>   merge  " ..." ...
> 
> I think with the -C syntax, it would become something like
> 
>   merge -C   # 
> 
> and
> 
>   merge -C   ...
>   # Merging: 
>   # Merging: 
>   # ...
> 
> The only qualm I have about this is that `#` really *is* a valid ref name.
> (Seriously, it is...). So that would mean that I'd have to disallow `#`
> as a label specificially.
> 
> Thoughts?

As ':' is not a valid ref if you want a separator you could have

merge -C   : 

personally I'm not sure what value having a separator adds in this case.
I think in the octopus case have a separate comment line for the subject
of each merge head is a good idea - maybe the two head merge could just
have the subject of the remote head in a comment below. I wonder if
having the subject of the commit that is going to be used for the
message may be a useful prompt in some cases but that's just making
things more complicated.

Best Wishes

Phillip

> Ciao,
> Dscho
> 

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Jake & Phillip,

On Mon, 29 Jan 2018, Johannes Schindelin wrote:

> On Sat, 20 Jan 2018, Jacob Keller wrote:
> 
> > On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood  
> > wrote:
> > > On 18/01/18 15:35, Johannes Schindelin wrote:
> > >>
> > >> This patch adds the `merge` command, with the following syntax:
> > >>
> > >>   merge   
> > >
> > > I'm concerned that this will be confusing for users. All of the other
> > > rebase commands replay the changes in the commit hash immediately
> > > following the command name. This command instead uses the first
> > > commit to specify the message which is different to both 'git merge'
> > > and the existing rebase commands. I wonder if it would be clearer to
> > > have 'merge -C   ...' instead so it's clear which
> > > argument specifies the message and which the remote head to merge.
> > > It would also allow for 'merge -c   ...' in the future
> > > for rewording an existing merge message and also avoid the slightly
> > > odd 'merge -  ...'. Where it's creating new merges I'm not sure
> > > it's a good idea to encourage people to only have oneline commit
> > > messages by making it harder to edit them, perhaps it could take
> > > another argument to mean open the editor or not, though as Jake said
> > > I guess it's not that common.
> > 
> > I actually like the idea of re-using commit message options like -C,
> > -c,  and -m, so we could do:
> > 
> > merge -C  ... to take message from commit
> 
> That is exactly how the Git garden shears do it.
> 
> I found it not very readable. That is why I wanted to get away from it in
> --recreate-merges.

I made up my mind. Even if it is not very readable, it is still better
than the `merge A B` where the order of A and B magically determines their
respective roles.

> > merge -c  ...  to take the message from commit and open editor to 
> > edit
> > merge -m "" ... to take the message from the quoted test
> > merge ... to merge and open commit editor with default message

I will probably implement -c, but not -m, and will handle the absence of
the -C and -c options to construct a default merge message which can then
be edited.

The -m option just opens such a can of worms with dequoting, that's why I
do not want to do that.

BTW I am still trying to figure out how to present the oneline of the
commit to merge (which is sometimes really helpful because the label might
be less than meaningful) while *still* allowing for octopus merges.

So far, what I have is this:

merge   

and for octopus:

merge  " ..." ...

I think with the -C syntax, it would become something like

merge -C   # 

and

merge -C   ...
# Merging: 
# Merging: 
# ...

The only qualm I have about this is that `#` really *is* a valid ref name.
(Seriously, it is...). So that would mean that I'd have to disallow `#`
as a label specificially.

Thoughts?

Ciao,
Dscho

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Junio,

On Mon, 22 Jan 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> > +   item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> > +   item->arg_len = (int)(eol - item->arg);
> > +
> > saved = *end_of_object_name;
> > +   if (item->command == TODO_MERGE && *bol == '-' &&
> > +   bol + 1 == end_of_object_name) {
> > +   item->commit = NULL;
> > +   return 0;
> > +   }
> > +
> > *end_of_object_name = '\0';
> > status = get_oid(bol, _oid);
> > *end_of_object_name = saved;
> >  
> > -   item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> > -   item->arg_len = (int)(eol - item->arg);
> > -
> 
> Assigning to "saved" before the added "if we are doing merge and see
> '-', do this special thing" is not only unnecessary, but makes the
> logic in the non-special case harder to read.  The four things
> "saved = *eol; *eol = 0; do_thing_using(bol); *eol = saved;" is a
> single logical unit; keep them together.

True. This was a sloppily resolved merge conflict in one of the many
rewrites, I guess.

> > +   if (*p)
> > +   len = strlen(p);
> > +   else {
> > +   strbuf_addf(, "Merge branch '%.*s'",
> > +   merge_arg_len, arg);
> > +   p = buf.buf;
> > +   len = buf.len;
> > +   }
> 
> So... "arg" received by this function can be a single non-whitespace
> token, which is taken as the name of the branch being merged (in
> this else clause).  Or it can also be followed by a single liner
> message for the merge commit.  Presumably, this is for creating a
> new merge (i.e. "commit==NULL" case), and preparing a proper log
> message in the todo list is unrealistic, so this would be a
> reasonable compromise.  Those users who want to write proper log
> message could presumably follow such "merge" insn with a "x git
> commit --amend" or something, I presume, if they really wanted to.

Precisely.

> > +   if (write_message(p, len, git_path_merge_msg(), 0) < 0) {
> > +   error_errno(_("Could not write '%s'"),
> > +   git_path_merge_msg());
> > +   strbuf_release();
> > +   rollback_lock_file();
> > +   return -1;
> > +   }
> > +   strbuf_release();
> > +   }
> 
> OK.  Now we have prepared the MERGE_MSG file and are ready to commit.
> 
> > +   head_commit = lookup_commit_reference_by_name("HEAD");
> > +   if (!head_commit) {
> > +   rollback_lock_file();
> > +   return error(_("Cannot merge without a current revision"));
> > +   }
> 
> Hmph, I would have expected to see this a lot earlier, before
> dealing with the log message.  Leftover MERGE_MSG file after an
> error will cause unexpected fallout to the end-user experience
> (including what is shown by the shell prompt scripts), but if we do
> this before the MERGE_MSG thing, we do not have to worry about
> error codepath having to remove it.

Fixed.

> > +   strbuf_addf(_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(_name, 0, strlen("refs/rewritten/"), "", 0);
> > +   merge_commit = lookup_commit_reference_by_name(ref_name.buf);
> > +   }
> 
> OK, so "abc" in the example in the log message is looked up first as
> a label and then we take a fallback to interpret as an object name.

Yes. And auto-generated labels are guaranteed not to be full hex hashes
for that reason.

> Hopefully allowed names in "label" would be documented clearly in
> later steps (I am guessing that "a name that can be used as a branch
> name can be used as a label name and vice versa" or something like
> that).

Well, I thought that it would suffice to say that these labels are
available as refs/rewritten/. It kind of goes without saying that
those need to be valid ref names, then?

> > +   if (!merge_commit) {
> > +   error(_("could not resolve '%s'"), ref_name.buf);
> > +   strbuf_release(_name);
> > +   rollback_lock_file();
> > +   return -1;
> > +   }
> > +   write_message(oid_to_hex(_commit->object.oid), GIT_SHA1_HEXSZ,
> > + git_path_merge_head(), 0);
> > +   write_message("no-ff", 5, git_path_merge_mode(), 0);
> 
> These two calls gave me a "Huh?" moment; write_message() sounds like
> it is allowed to be later updated with frills suitable for *_MSG
> files we place in .git/ directory (iow, it is in principle OK if
> commented out instructions common to these files are added to the
> output by the function), but these want exact bytes passed in the
> result, for which wrapper.c::write_file() is more appropriate.

I agree that 

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Jake & Phillip,

On Sat, 20 Jan 2018, Jacob Keller wrote:

> On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood  
> wrote:
> > On 18/01/18 15:35, Johannes Schindelin wrote:
> >>
> >> This patch is part of the effort to reimplement `--preserve-merges` with
> >> a substantially improved design, a design that has been developed in the
> >> Git for Windows project to maintain the dozens of Windows-specific patch
> >> series on top of upstream Git.
> >>
> >> The previous patch implemented the `label`, `bud` and `reset` commands
> >> to label commits and to reset to a labeled commits. This patch adds the
> >> `merge` command, with the following syntax:
> >>
> >>   merge   
> >
> > I'm concerned that this will be confusing for users. All of the other
> > rebase commands replay the changes in the commit hash immediately
> > following the command name. This command instead uses the first commit
> > to specify the message which is different to both 'git merge' and the
> > existing rebase commands. I wonder if it would be clearer to have 'merge
> > -C   ...' instead so it's clear which argument specifies
> > the message and which the remote head to merge. It would also allow for
> > 'merge -c   ...' in the future for rewording an existing
> > merge message and also avoid the slightly odd 'merge -  ...'. Where
> > it's creating new merges I'm not sure it's a good idea to encourage
> > people to only have oneline commit messages by making it harder to edit
> > them, perhaps it could take another argument to mean open the editor or
> > not, though as Jake said I guess it's not that common.
> 
> I actually like the idea of re-using commit message options like -C,
> -c,  and -m, so we could do:
> 
> merge -C  ... to take message from commit

That is exactly how the Git garden shears do it.

I found it not very readable. That is why I wanted to get away from it in
--recreate-merges.

> merge -c  ...  to take the message from commit and open editor to edit
> merge -m "" ... to take the message from the quoted test
> merge ... to merge and open commit editor with default message
> 
> This also, I think, allows us to not need to put the oneline on the
> end, meaning we wouldn't have to quote the parent commit arguments
> since we could use option semantics?

The oneline is there primarily to give you, the reader, a clue when
reading and editing the todo list.

Reusing it for the `merge -` command was only an afterthought.

> > One thought that just struck me - if a merge or reset command specifies
> > an invalid label is it rescheduled so that it's still in the to-do list
> > when the user edits it after rebase stops?

It is not rescheduled, because the command already failed, so we know it
is bad.

You have to go edit the todo list, possibly after copying the faulty
command from `git status`' output.

> > In the future it might be nice if the label, reset and merge commands
> > were validated when the to-do list is parsed so that the user gets
> > immediate feedback if they try to create a label that is not a valid
> > ref name or that they have a typo in a name given to reset or merge
> > rather than the rebase stopping later.

There are too many possible errors to make this fool-proof. What if the
ref name is valid, but there was no `label` command yet?  What if there
*has* been a `label` command but it is now stuck in the `done` file (which
we do not parse, ever)? What if the user specified two label commands with
the same label?

It sounds like an exercise in futility to try to catch these things in the
parser.

And keep in mind that the parser is used

- when shortening the commit names
- when checking the todo list for accidentally dropped picks
- when skipping unnecessary picks
- when rearranging fixup!/squash! commands
- when adding `exec` commands specified via `-x`

I am not sure that I would come out in favor of trying to catch ref name
errors during parsing time if I wanted to balance bang vs buck.

Ciao,
Dscho

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Junio C Hamano]

Johannes Schindelin  writes:

>   end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> + item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> + item->arg_len = (int)(eol - item->arg);
> +
>   saved = *end_of_object_name;
> + if (item->command == TODO_MERGE && *bol == '-' &&
> + bol + 1 == end_of_object_name) {
> + item->commit = NULL;
> + return 0;
> + }
> +
>   *end_of_object_name = '\0';
>   status = get_oid(bol, _oid);
>   *end_of_object_name = saved;
>  
> - item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> - item->arg_len = (int)(eol - item->arg);
> -

Assigning to "saved" before the added "if we are doing merge and see
'-', do this special thing" is not only unnecessary, but makes the
logic in the non-special case harder to read.  The four things
"saved = *eol; *eol = 0; do_thing_using(bol); *eol = saved;" is a
single logical unit; keep them together.

This hunk may have been the most expedient way to coax "-" into the
location where a commit object name is expected; it looks ugly, but
for the limited purpose of this series it should do.

> @@ -2069,6 +2077,132 @@ static int do_reset(const char *name, int len)
>   return ret;
>  }
>  
> +static int do_merge(struct commit *commit, const char *arg, int arg_len,
> + struct replay_opts *opts)
> +{
> + int merge_arg_len;
> + struct strbuf ref_name = STRBUF_INIT;
> + struct commit *head_commit, *merge_commit, *i;
> + struct commit_list *common, *j, *reversed = NULL;
> + struct merge_options o;
> + int ret;
> + static struct lock_file lock;
> +
> + for (merge_arg_len = 0; merge_arg_len < arg_len; merge_arg_len++)
> + if (isspace(arg[merge_arg_len]))
> + break;

Mental note: this scans for a whitespace, and tab is accepted
instead of SP, which presumably is to allow human typed string.

> + if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0)
> + return -1;
> +
> + if (commit) {
> + const char *message = get_commit_buffer(commit, NULL);
> + const char *body;
> + int len;
> +
> + if (!message) {
> + rollback_lock_file();
> + return error(_("could not get commit message of '%s'"),
> +  oid_to_hex(>object.oid));
> + }
> + write_author_script(message);
> + find_commit_subject(message, );
> + 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();
> + return -1;
> + }
> + unuse_commit_buffer(commit, message);
> + } else {
> + const char *p = arg + merge_arg_len;
> + struct strbuf buf = STRBUF_INIT;
> + int len;
> +
> + strbuf_addf(, "author %s", git_author_info(0));
> + write_author_script(buf.buf);
> + strbuf_reset();
> +
> + p += strspn(p, " \t");

... and this matches the above mental note.  It allows consecutive
whitespaces as a separator, which is sensible behaviour.

> + if (*p)
> + len = strlen(p);
> + else {
> + strbuf_addf(, "Merge branch '%.*s'",
> + merge_arg_len, arg);
> + p = buf.buf;
> + len = buf.len;
> + }

So... "arg" received by this function can be a single non-whitespace
token, which is taken as the name of the branch being merged (in
this else clause).  Or it can also be followed by a single liner
message for the merge commit.  Presumably, this is for creating a
new merge (i.e. "commit==NULL" case), and preparing a proper log
message in the todo list is unrealistic, so this would be a
reasonable compromise.  Those users who want to write proper log
message could presumably follow such "merge" insn with a "x git
commit --amend" or something, I presume, if they really wanted to.

> + if (write_message(p, len, git_path_merge_msg(), 0) < 0) {
> + error_errno(_("Could not write '%s'"),
> + git_path_merge_msg());
> + strbuf_release();
> + rollback_lock_file();
> + return -1;
> + }
> + strbuf_release();
> + }

OK.  Now we have prepared the MERGE_MSG file and are ready to commit.

> + head_commit = lookup_commit_reference_by_name("HEAD");
> + if (!head_commit) {
> + rollback_lock_file();
> + return error(_("Cannot 

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Jacob Keller]

On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood  wrote:
> On 18/01/18 15:35, Johannes Schindelin wrote:
>>
>> This patch is part of the effort to reimplement `--preserve-merges` with
>> a substantially improved design, a design that has been developed in the
>> Git for Windows project to maintain the dozens of Windows-specific patch
>> series on top of upstream Git.
>>
>> The previous patch implemented the `label`, `bud` and `reset` commands
>> to label commits and to reset to a labeled commits. This patch adds the
>> `merge` command, with the following syntax:
>>
>>   merge   
>
> I'm concerned that this will be confusing for users. All of the other
> rebase commands replay the changes in the commit hash immediately
> following the command name. This command instead uses the first commit
> to specify the message which is different to both 'git merge' and the
> existing rebase commands. I wonder if it would be clearer to have 'merge
> -C   ...' instead so it's clear which argument specifies
> the message and which the remote head to merge. It would also allow for
> 'merge -c   ...' in the future for rewording an existing
> merge message and also avoid the slightly odd 'merge -  ...'. Where
> it's creating new merges I'm not sure it's a good idea to encourage
> people to only have oneline commit messages by making it harder to edit
> them, perhaps it could take another argument to mean open the editor or
> not, though as Jake said I guess it's not that common.

I actually like the idea of re-using commit message options like -C,
-c,  and -m, so we could do:

merge -C  ... to take message from commit
merge -c  ...  to take the message from commit and open editor to edit
merge -m "" ... to take the message from the quoted test
merge ... to merge and open commit editor with default message

This also, I think, allows us to not need to put the oneline on the
end, meaning we wouldn't have to quote the parent commit arguments
since we could use option semantics?

>
> One thought that just struck me - if a merge or reset command specifies
> an invalid label is it rescheduled so that it's still in the to-do list
> when the user edits it after rebase stops?
>
> In the future it might be nice if the label, reset and merge commands
> were validated when the to-do list is parsed so that the user gets
> immediate feedback if they try to create a label that is not a valid ref
> name or that they have a typo in a name given to reset or merge rather
> than the rebase stopping later.
>

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Phillip Wood]

On 18/01/18 15:35, Johannes Schindelin wrote:
> 
> This patch is part of the effort to reimplement `--preserve-merges` with
> a substantially improved design, a design that has been developed in the
> Git for Windows project to maintain the dozens of Windows-specific patch
> series on top of upstream Git.
> 
> The previous patch implemented the `label`, `bud` and `reset` commands
> to label commits and to reset to a labeled commits. This patch adds the
> `merge` command, with the following syntax:
> 
>   merge   

I'm concerned that this will be confusing for users. All of the other
rebase commands replay the changes in the commit hash immediately
following the command name. This command instead uses the first commit
to specify the message which is different to both 'git merge' and the
existing rebase commands. I wonder if it would be clearer to have 'merge
-C   ...' instead so it's clear which argument specifies
the message and which the remote head to merge. It would also allow for
'merge -c   ...' in the future for rewording an existing
merge message and also avoid the slightly odd 'merge -  ...'. Where
it's creating new merges I'm not sure it's a good idea to encourage
people to only have oneline commit messages by making it harder to edit
them, perhaps it could take another argument to mean open the editor or
not, though as Jake said I guess it's not that common.

One thought that just struck me - if a merge or reset command specifies
an invalid label is it rescheduled so that it's still in the to-do list
when the user edits it after rebase stops?

In the future it might be nice if the label, reset and merge commands
were validated when the to-do list is parsed so that the user gets
immediate feedback if they try to create a label that is not a valid ref
name or that they have a typo in a name given to reset or merge rather
than the rebase stopping later.

> The  parameter in this instance is the *original* merge commit,
> whose author and message will be used for the to-be-created merge
> commit.
> 
> The  parameter refers to the (possibly rewritten) revision to
> merge. Let's see an example of a todo list:
> 
>   label onto
> 
>   # Branch abc
>   bud
>   pick deadbeef Hello, world!
>   label abc
> 
>   bud
>   pick cafecafe And now for something completely different
>   merge baaabaaa abc Merge the branch 'abc' into master
> 
> To support creating *new* merges, i.e. without copying the commit
> message from an existing commit, use the special value `-` as 
> parameter (in which case the text after the  parameter is used as
> commit message):
> 
>   merge - abc This will be the actual commit message of the merge
> 
> This comes in handy when splitting a branch into two or more branches.
> 
> Note: this patch only adds support for recursive merges, to keep things
> simple. Support for octopus merges will be added later in this patch
> series, support for merges using strategies other than the recursive
> merge is left for future contributions.
> 
> The design of the `merge` command as introduced by this patch only
> supports creating new merge commits with exactly two parents, i.e. it
> adds no support for octopus merges.
> 
> We will introduce support for octopus merges in a later commit.
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  git-rebase--interactive.sh |   1 +
>  sequencer.c| 146 
> +++--
>  2 files changed, 143 insertions(+), 4 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 3d2cd19d65a..5bf1ea3781f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -165,6 +165,7 @@ d, drop = remove commit
>  l, label = label current HEAD with a name
>  t, reset = reset HEAD to a label
>  b, bud = reset HEAD to the revision labeled 'onto'
> +m, merge = create a merge commit using a given commit's message
>  
>  These lines can be re-ordered; they are executed from top to bottom.
>  " | git stripspace --comment-lines >>"$todo"
> diff --git a/sequencer.c b/sequencer.c
> index 91cc55a002f..567cfcbbe8b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -779,6 +779,7 @@ enum todo_command {
>   TODO_LABEL,
>   TODO_RESET,
>   TODO_BUD,
> + TODO_MERGE,
>   /* commands that do nothing but are counted for reporting progress */
>   TODO_NOOP,
>   TODO_DROP,
> @@ -800,6 +801,7 @@ static struct {
>   { 'l', "label" },
>   { 't', "reset" },
>   { 'b', "bud" },
> + { 'm', "merge" },
>   { 0,   "noop" },
>   { 'd', "drop" },
>   { 0,   NULL }
> @@ -1304,14 +1306,20 @@ static int parse_insn_line(struct todo_item *item, 
> const char *bol, char *eol)
>   }
>  
>   end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> + item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> + item->arg_len = (int)(eol - item->arg);
> +
>   

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Eric Sunshine]

On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
 wrote:
> [...]
> Note: this patch only adds support for recursive merges, to keep things
> simple. Support for octopus merges will be added later in this patch
> series, support for merges using strategies other than the recursive
> merge is left for future contributions.

The above paragraph...

> The design of the `merge` command as introduced by this patch only
> supports creating new merge commits with exactly two parents, i.e. it
> adds no support for octopus merges.
>
> We will introduce support for octopus merges in a later commit.

and these two sentences say the same thing. I suppose one or the other
was meant to be dropped(?).

> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -2069,6 +2077,132 @@ static int do_reset(const char *name, int len)
> +static int do_merge(struct commit *commit, const char *arg, int arg_len,
> +   struct replay_opts *opts)
> +{
> +   [...]
> +   if (write_message(body, len, git_path_merge_msg(), 0) < 0) {
> +   error_errno(_("Could not write '%s'"),

s/Could/could/

> +   if (write_message(p, len, git_path_merge_msg(), 0) < 0) {
> +   error_errno(_("Could not write '%s'"),

Ditto.

> +   if (!head_commit) {
> +   rollback_lock_file();
> +   return error(_("Cannot merge without a current revision"));

s/Cannot/cannot/

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Jacob Keller]

On Thu, Jan 18, 2018 at 1:22 PM, Johannes Schindelin
 wrote:
>> Would it be possible to open the editor with the supplied text when
>> there's no commit?  The text after  must be oneline only..
>
> I actually want to avoid that because my main use case is fire-and-forget,
> i.e. I want to edit only the todo list and then (barring any merge
> conflicts) I do not want to edit anything anymore.
>

Agreed, for the case where we copy a commit message, I do not want the
editor either.

> But I guess we could special-case the thing where `-` is specified as
> "merge commit message provider" and an empty oneline is provided?
>

It's for when there is a new merge, for when we are creating a new one
using "-", yes.

>> It's difficult to reword merges because of the nature of rebase
>> interactive, you can't just re-run the rebase command and use
>> "reword".
>>
>> I suppose you could cheat by putting in an "edit" command that let you
>> create an empty commit with a message...
>
> Or you could "cheat" by adding `exec git commit --amend`...
>
> Seriously again, I have no good idea how to provide an equivalent to the
> `reword` verb that would work on merge commits...
>

Given that there is a work around, and I doubt it's that common, I'm
not sure we need one, plus i have no idea what verb to use

We could allow reword on its own to simply reword the top commit?

That being said, since there's a simple-ish workaruond using "stop",
or "exec git commit --amend" I don't see this as being important
enough to worry about now.

Thanks,
Jake

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Jake,

On Thu, 18 Jan 2018, Jacob Keller wrote:

> On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
>  wrote:
> > This patch is part of the effort to reimplement `--preserve-merges` with
> > a substantially improved design, a design that has been developed in the
> > Git for Windows project to maintain the dozens of Windows-specific patch
> > series on top of upstream Git.
> >
> > The previous patch implemented the `label`, `bud` and `reset` commands
> > to label commits and to reset to a labeled commits. This patch adds the
> > `merge` command, with the following syntax:
> >
> > merge   
> >
> > The  parameter in this instance is the *original* merge commit,
> > whose author and message will be used for the to-be-created merge
> > commit.
> >
> > The  parameter refers to the (possibly rewritten) revision to
> > merge. Let's see an example of a todo list:
> >
> > label onto
> >
> > # Branch abc
> > bud
> > pick deadbeef Hello, world!
> > label abc
> >
> > bud
> > pick cafecafe And now for something completely different
> > merge baaabaaa abc Merge the branch 'abc' into master
> >
> > To support creating *new* merges, i.e. without copying the commit
> > message from an existing commit, use the special value `-` as 
> > parameter (in which case the text after the  parameter is used as
> > commit message):
> >
> > merge - abc This will be the actual commit message of the merge
> >
> > This comes in handy when splitting a branch into two or more branches.
> >
> 
> Would it be possible to open the editor with the supplied text when
> there's no commit?  The text after  must be oneline only..

I actually want to avoid that because my main use case is fire-and-forget,
i.e. I want to edit only the todo list and then (barring any merge
conflicts) I do not want to edit anything anymore.

But I guess we could special-case the thing where `-` is specified as
"merge commit message provider" and an empty oneline is provided?

> It's difficult to reword merges because of the nature of rebase
> interactive, you can't just re-run the rebase command and use
> "reword".
> 
> I suppose you could cheat by putting in an "edit" command that let you
> create an empty commit with a message...

Or you could "cheat" by adding `exec git commit --amend`...

Seriously again, I have no good idea how to provide an equivalent to the
`reword` verb that would work on merge commits...

Anyone?

Ciao,
Dscho

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Jacob Keller]

On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
 wrote:
> This patch is part of the effort to reimplement `--preserve-merges` with
> a substantially improved design, a design that has been developed in the
> Git for Windows project to maintain the dozens of Windows-specific patch
> series on top of upstream Git.
>
> The previous patch implemented the `label`, `bud` and `reset` commands
> to label commits and to reset to a labeled commits. This patch adds the
> `merge` command, with the following syntax:
>
> merge   
>
> The  parameter in this instance is the *original* merge commit,
> whose author and message will be used for the to-be-created merge
> commit.
>
> The  parameter refers to the (possibly rewritten) revision to
> merge. Let's see an example of a todo list:
>
> label onto
>
> # Branch abc
> bud
> pick deadbeef Hello, world!
> label abc
>
> bud
> pick cafecafe And now for something completely different
> merge baaabaaa abc Merge the branch 'abc' into master
>
> To support creating *new* merges, i.e. without copying the commit
> message from an existing commit, use the special value `-` as 
> parameter (in which case the text after the  parameter is used as
> commit message):
>
> merge - abc This will be the actual commit message of the merge
>
> This comes in handy when splitting a branch into two or more branches.
>

Would it be possible to open the editor with the supplied text when
there's no commit? The text after  must be oneline only..

It's difficult to reword merges because of the nature of rebase
interactive, you can't just re-run the rebase command and use
"reword".

I suppose you could cheat by putting in an "edit" command that let you
create an empty commit with a message...

Thanks,
Jake