*Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision
2018-01-18 16:25 ` Jacob Keller@ 2018-01-18 21:13 ` Johannes Schindelin
2018-01-18 21:21 ` Jacob Keller
2018-01-18 21:24 ` Philip Oakley
2018-01-22 21:25 ` Junio C Hamano2 siblings, 1 reply; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-18 21:13 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, Junio C Hamano
Hi Jake,
On Thu, 18 Jan 2018, Jacob Keller wrote:
> On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > This commit implements the commands to label, and to reset to, given
> > revisions. The syntax is:
> >
> > label <name>
> > reset <name>
> >
> > As a convenience shortcut, also to improve readability of the generated
> > todo list, a third command is introduced: bud. It simply resets to the
> > "onto" revision, i.e. the commit onto which we currently rebase.
> >
>
> The code looks good, but I'm a little wary of adding bud which
> hard-codes a specific label. I suppose it does grant a bit of
> readability to the resulting script... ? It doesn't seem that
> important compared to use using "reset onto"? At least when
> documenting this it should be made clear that the "onto" label is
> special.
Indeed, `bud` helped me more in the earlier times of the Git garden shears
when I had to *introduce* branch structure into the long list of Git for
Windows' patches.
If there are no voices in favor of `bud` other than mine, I will remove
support for it in the next iteration.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision
2018-01-18 21:13 ` Johannes Schindelin@ 2018-01-18 21:21 ` Jacob Keller0 siblings, 0 replies; 412+ messages in thread
From: Jacob Keller @ 2018-01-18 21:21 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git mailing list, Junio C Hamano
On Thu, Jan 18, 2018 at 1:13 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Jake,
>
> On Thu, 18 Jan 2018, Jacob Keller wrote:
>
>> On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>> > This commit implements the commands to label, and to reset to, given
>> > revisions. The syntax is:
>> >
>> > label <name>
>> > reset <name>
>> >
>> > As a convenience shortcut, also to improve readability of the generated
>> > todo list, a third command is introduced: bud. It simply resets to the
>> > "onto" revision, i.e. the commit onto which we currently rebase.
>> >
>>
>> The code looks good, but I'm a little wary of adding bud which
>> hard-codes a specific label. I suppose it does grant a bit of
>> readability to the resulting script... ? It doesn't seem that
>> important compared to use using "reset onto"? At least when
>> documenting this it should be made clear that the "onto" label is
>> special.
>
> Indeed, `bud` helped me more in the earlier times of the Git garden shears
> when I had to *introduce* branch structure into the long list of Git for
> Windows' patches.
>
> If there are no voices in favor of `bud` other than mine, I will remove
> support for it in the next iteration.
>
> Ciao,
> Dscho
After I saw more examples, I don't mind it. I do think it's not
strictly necessary, but it seemed to help me with the readability. My
only main concern is that it's limited to a special label.
Thanks,
Jake
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision
2018-01-18 16:25 ` Jacob Keller
2018-01-18 21:13 ` Johannes Schindelin@ 2018-01-18 21:24 ` Philip Oakley
2018-01-18 21:28 ` Jacob Keller
2018-01-29 20:28 ` Johannes Schindelin
2018-01-22 21:25 ` Junio C Hamano2 siblings, 2 replies; 412+ messages in thread
From: Philip Oakley @ 2018-01-18 21:24 UTC (permalink / raw)
To: Jacob Keller, Johannes Schindelin; +Cc: Git mailing list, Junio C Hamano
From: "Jacob Keller" <jacob.keller@gmail.com>
> On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> This commit implements the commands to label, and to reset to, given
>> revisions. The syntax is:
>>
>> label <name>
>> reset <name>
>>
>> As a convenience shortcut, also to improve readability of the generated
>> todo list, a third command is introduced: bud. It simply resets to the
>> "onto" revision, i.e. the commit onto which we currently rebase.
>>
>
> The code looks good, but I'm a little wary of adding bud which
> hard-codes a specific label. I suppose it does grant a bit of
> readability to the resulting script... ? It doesn't seem that
> important compared to use using "reset onto"? At least when
> documenting this it should be made clear that the "onto" label is
> special.
>
> Thanks,
> Jake.
I'd agree.
The special 'onto' label should be fully documented, and the commit message
should indicate which patch actually defines it (and all its corner cases
and fall backs if --onto isn't explicitly given..)
Likewise the choice of 'bud' should be explained with some nice phraseology
indicating that we are growing the new flowering from the bud, otherwise the
word is a bit too short and sudden for easy explanation.
Philip
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision
2018-01-18 21:24 ` Philip Oakley@ 2018-01-18 21:28 ` Jacob Keller
2018-01-29 20:28 ` Johannes Schindelin1 sibling, 0 replies; 412+ messages in thread
From: Jacob Keller @ 2018-01-18 21:28 UTC (permalink / raw)
To: Philip Oakley; +Cc: Johannes Schindelin, Git mailing list, Junio C Hamano
On Thu, Jan 18, 2018 at 1:24 PM, Philip Oakley <philipoakley@iee.org> wrote:
> From: "Jacob Keller" <jacob.keller@gmail.com>
>
>> On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>>>
>>> This commit implements the commands to label, and to reset to, given
>>> revisions. The syntax is:
>>>
>>> label <name>
>>> reset <name>
>>>
>>> As a convenience shortcut, also to improve readability of the generated
>>> todo list, a third command is introduced: bud. It simply resets to the
>>> "onto" revision, i.e. the commit onto which we currently rebase.
>>>
>>
>> The code looks good, but I'm a little wary of adding bud which
>> hard-codes a specific label. I suppose it does grant a bit of
>> readability to the resulting script... ? It doesn't seem that
>> important compared to use using "reset onto"? At least when
>> documenting this it should be made clear that the "onto" label is
>> special.
>>
>> Thanks,
>> Jake.
>
>
> I'd agree.
>
> The special 'onto' label should be fully documented, and the commit message
> should indicate which patch actually defines it (and all its corner cases
> and fall backs if --onto isn't explicitly given..)
I don't think it actually relates to "--onto" but rather to simply
using "label onto" in your sequencer script allows bud to work, and
simply shortens the overall work necessary. It's equivalent to "reset
onto" if I understand.
>
> Likewise the choice of 'bud' should be explained with some nice phraseology
> indicating that we are growing the new flowering from the bud, otherwise the
> word is a bit too short and sudden for easy explanation.
>
> Philip
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision
2018-01-18 21:24 ` Philip Oakley
2018-01-18 21:28 ` Jacob Keller@ 2018-01-29 20:28 ` Johannes Schindelin1 sibling, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 20:28 UTC (permalink / raw)
To: Philip Oakley; +Cc: Jacob Keller, Git mailing list, Junio C Hamano
Hi Philip,
On Thu, 18 Jan 2018, Philip Oakley wrote:
> From: "Jacob Keller" <jacob.keller@gmail.com>
> > On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
> > <johannes.schindelin@gmx.de> wrote:
> > > This commit implements the commands to label, and to reset to, given
> > > revisions. The syntax is:
> > >
> > > label <name>
> > > reset <name>
> > >
> > > As a convenience shortcut, also to improve readability of the generated
> > > todo list, a third command is introduced: bud. It simply resets to the
> > > "onto" revision, i.e. the commit onto which we currently rebase.
> > >
> >
> > The code looks good, but I'm a little wary of adding bud which
> > hard-codes a specific label. I suppose it does grant a bit of
> > readability to the resulting script... ? It doesn't seem that
> > important compared to use using "reset onto"? At least when
> > documenting this it should be made clear that the "onto" label is
> > special.
> >
> > Thanks,
> > Jake.
>
> I'd agree.
>
> The special 'onto' label should be fully documented, and the commit message
> should indicate which patch actually defines it (and all its corner cases and
> fall backs if --onto isn't explicitly given..)
I hoped that the example todo lists would clarify that 'onto' is just the
name of the first label:
label onto
is *literally* how all of those todo lists start. And that is why there
are no possible concerns about any missing `--onto` argument: that
argument is irrelevant for that label.
Maybe it is just a bad name. Maybe `START` would be a better label.
What do you think?
> Likewise the choice of 'bud' should be explained with some nice
> phraseology indicating that we are growing the new flowering from the
> bud, otherwise the word is a bit too short and sudden for easy
> explanation.
I dropped the `bud` command.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision
2018-01-18 16:25 ` Jacob Keller
2018-01-18 21:13 ` Johannes Schindelin
2018-01-18 21:24 ` Philip Oakley@ 2018-01-22 21:25 ` Junio C Hamano
2018-01-29 22:00 ` Johannes Schindelin2 siblings, 1 reply; 412+ messages in thread
From: Junio C Hamano @ 2018-01-22 21:25 UTC (permalink / raw)
To: Jacob Keller; +Cc: Johannes Schindelin, Git mailing list
Jacob Keller <jacob.keller@gmail.com> writes:
> The code looks good, but I'm a little wary of adding bud which
> hard-codes a specific label. I suppose it does grant a bit of
> readability to the resulting script... ? It doesn't seem that
> important compared to use using "reset onto"? At least when
> documenting this it should be made clear that the "onto" label is
> special.
I do not think we would mind "bud" too much in the end result, but
the change in 1/8 is made harder to read than necessary with it. It
is the only thing that needs "a single-letter command name may now
not have any argument after it" change to the parser among the three
things being added here, and it also needs to be added to the list
of special commands without arguments.
It would have been easier to reason about if addition of "bud" was
in its own patch done after label and reset are added. And if done
as a separate step, perhaps it would have been easier to realize
that it would be a more future-proof solution for handling the
"special" ness of BUD to add a new "unsigned flags" word to
todo_command_info[] structure and using a bit that says "this does
not take an arg" than to hardcode "noop and bud are the commands
without args" in the code. That hardcode was good enough when there
was only one thing in that special case. Now it has two.
In a similar way, the code to special case label and reset just like
exec may also want to become more table driven, perhaps using
another bit in the same new flags word to say "this does not refer
to commit". I think that can become [v2 1/N] while addition of "bud"
can be [v2 2/N] (after all, "bud" just does the same do_reset() with
hardcoded argument, so "label/reset" must come first).
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision
2018-01-22 21:25 ` Junio C Hamano@ 2018-01-29 22:00 ` Johannes Schindelin0 siblings, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 22:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list
Hi Junio,
On Mon, 22 Jan 2018, Junio C Hamano wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
> > The code looks good, but I'm a little wary of adding bud which
> > hard-codes a specific label. I suppose it does grant a bit of
> > readability to the resulting script... ? It doesn't seem that
> > important compared to use using "reset onto"? At least when
> > documenting this it should be made clear that the "onto" label is
> > special.
>
> I do not think we would mind "bud" too much in the end result, but
> the change in 1/8 is made harder to read than necessary with it. It
> is the only thing that needs "a single-letter command name may now
> not have any argument after it" change to the parser among the three
> things being added here, and it also needs to be added to the list
> of special commands without arguments.
>
> It would have been easier to reason about if addition of "bud" was
> in its own patch done after label and reset are added. And if done
> as a separate step, perhaps it would have been easier to realize
> that it would be a more future-proof solution for handling the
> "special" ness of BUD to add a new "unsigned flags" word to
> todo_command_info[] structure and using a bit that says "this does
> not take an arg" than to hardcode "noop and bud are the commands
> without args" in the code. That hardcode was good enough when there
> was only one thing in that special case. Now it has two.
I dropped the `bud` command. It did come in handy when I truly recreated
branch structure from a way-too-long topic branch, but that is probably a
rare use case, and not worth spending so much air time on.
> In a similar way, the code to special case label and reset just like
> exec may also want to become more table driven, perhaps using
> another bit in the same new flags word to say "this does not refer
> to commit". I think that can become [v2 1/N] while addition of "bud"
> can be [v2 2/N] (after all, "bud" just does the same do_reset() with
> hardcoded argument, so "label/reset" must come first).
The downside of such a table-driven approach is readability, of course. It
becomes *less* readable because all of a sudden you have to jump back and
forth between the parsing code and the table (and then you also have to
keep the table header in sight).
I have had enough problems with such table-driven approaches in the past,
even in Git's own source code. Exhibit A: t0027. I do not wish upon my
worst enemies having to investigate problems in that script, for in those
tables despair awaits ye who dare enter.
So I respectfully decline to go into that direction in the sequencer.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision
2018-01-19 8:59 ` Eric Sunshine@ 2018-01-24 22:01 ` Junio C Hamano
2018-01-29 20:55 ` Johannes Schindelin
2018-01-29 20:50 ` Johannes Schindelin1 sibling, 1 reply; 412+ messages in thread
From: Junio C Hamano @ 2018-01-24 22:01 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Johannes Schindelin, Git List, Jacob Keller
Eric Sunshine <sunshine@sunshineco.com> writes:
>> +static int do_reset(const char *name, int len)
>> +{
>> + [...]
>> + if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
>> + return -1;
>> +
>> + for (i = 0; i < len; i++)
>> + if (isspace(name[i]))
>> + len = i;
>
> What is the purpose of this loop? I could imagine that it's trying to
> strip all whitespace from the end of 'name', however, to do that it
> would iterate backward, not forward. (Or perhaps it's trying to
> truncate at the first space, but then it would need to invert the
> condition or use 'break'.) Am I missing something obvious?
I must be missing the same thing. Given that the callers of
do_reset(), other than the "bug" thing that passes the hard coded
"onto", uses item->arg/item->arg_len which includes everything after
the insn word on the line in the todo list, I do suspect that the
intention is to stop at the first whitespace char to avoid creating
a ref with whitespace in it, i.e. it is a bug that can be fixed with
s/len = i/break/.
The code probably should further check the resulting string with
check_ref_format() to detect strange chars and char sequences that
make the resulting refname invalid. For example, you would not want
to allow a label with two consecutive periods in it.
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision
2018-01-24 22:01 ` Junio C Hamano@ 2018-01-29 20:55 ` Johannes Schindelin0 siblings, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 20:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, Git List, Jacob Keller
Hi Junio,
On Wed, 24 Jan 2018, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> >> +static int do_reset(const char *name, int len)
> >> +{
> >> + [...]
> >> + if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> >> + return -1;
> >> +
> >> + for (i = 0; i < len; i++)
> >> + if (isspace(name[i]))
> >> + len = i;
> >
> > What is the purpose of this loop? I could imagine that it's trying to
> > strip all whitespace from the end of 'name', however, to do that it
> > would iterate backward, not forward. (Or perhaps it's trying to
> > truncate at the first space, but then it would need to invert the
> > condition or use 'break'.) Am I missing something obvious?
>
> I must be missing the same thing. Given that the callers of
> do_reset(), other than the "bug" thing that passes the hard coded
> "onto", uses item->arg/item->arg_len which includes everything after
> the insn word on the line in the todo list, I do suspect that the
> intention is to stop at the first whitespace char to avoid creating
> a ref with whitespace in it, i.e. it is a bug that can be fixed with
> s/len = i/break/.
>
> The code probably should further check the resulting string with
> check_ref_format() to detect strange chars and char sequences that
> make the resulting refname invalid. For example, you would not want
> to allow a label with two consecutive periods in it.
The code already checks that by creating a ref.
No need to go crazy and validate ref names every time we parse the todo
list (which is quite often), eh?
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision
2018-01-19 8:59 ` Eric Sunshine
2018-01-24 22:01 ` Junio C Hamano@ 2018-01-29 20:50 ` Johannes Schindelin
2018-01-30 7:12 ` Eric Sunshine1 sibling, 1 reply; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 20:50 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Junio C Hamano, Jacob Keller
Hi Eric,
On Fri, 19 Jan 2018, Eric Sunshine wrote:
> On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > [...]
> > +static int do_reset(const char *name, int len)
> > +{
> > + [...]
> > + if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> > + return -1;
> > +
> > + for (i = 0; i < len; i++)
> > + if (isspace(name[i]))
> > + len = i;
>
> What is the purpose of this loop? I could imagine that it's trying to
> strip all whitespace from the end of 'name', however, to do that it
> would iterate backward, not forward. (Or perhaps it's trying to
> truncate at the first space, but then it would need to invert the
> condition or use 'break'.) Am I missing something obvious?
Yes, you are missing something obvious. The idea of the `reset` command is
that it not only has a label, but also the oneline of the original commit:
reset branch-point sequencer: prepare for cleanup
In this instance, `branch-point` is the label. And for convenience of the
person editing, it also has the oneline. This came in *extremely* handy
when editing the commit topology in Git for Windows, i.e. when introducing
topic branches or flattening them.
In the Git garden shears, I separated the two arguments via `#`:
reset branch-point # sequencer: prepare for cleanup
I guess that is actually more readable, so I will introduce that into this
patch series, too.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision
2018-01-29 20:50 ` Johannes Schindelin@ 2018-01-30 7:12 ` Eric Sunshine0 siblings, 0 replies; 412+ messages in thread
From: Eric Sunshine @ 2018-01-30 7:12 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano, Jacob Keller
On Mon, Jan 29, 2018 at 3:50 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Fri, 19 Jan 2018, Eric Sunshine wrote:
>> On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>> > +static int do_reset(const char *name, int len)
>> > +{
>> > + for (i = 0; i < len; i++)
>> > + if (isspace(name[i]))
>> > + len = i;
>>
>> What is the purpose of this loop? I could imagine that it's trying to
>> strip all whitespace from the end of 'name', however, to do that it
>> would iterate backward, not forward. (Or perhaps it's trying to
>> truncate at the first space, but then it would need to invert the
>> condition or use 'break'.) Am I missing something obvious?
>
> Yes, you are missing something obvious. The idea of the `reset` command is
> that it not only has a label, but also the oneline of the original commit:
>
> reset branch-point sequencer: prepare for cleanup
>
> In this instance, `branch-point` is the label. And for convenience of the
> person editing, it also has the oneline.
No, that's not what I was missing. What I was missing was that
assigning 'i' to 'len' also causes the loop to terminate. It's
embarrassing how long I had to stare at this loop to see that, and I
suspect that's what fooled a couple other reviewers, as well, since
idiomatic loops don't normally muck with the termination condition in
quite that fashion (and is why I suggested that a 'break' might be
missing).
Had the loop been a bit more idiomatic:
for (i = 0; i < len; i++)
if (isspace(name[i]))
break;
len = i;
then the question would never have arisen. Anyhow, it's a minor point
in the greater scheme of the patch series.
> In the Git garden shears, I separated the two arguments via `#`:
>
> reset branch-point # sequencer: prepare for cleanup
>
> I guess that is actually more readable, so I will introduce that into this
> patch series, too.
Given my termination-condition blindness, the extra "#" would not have
helped me understand the loop any better.
Having now played with the feature a tiny bit, I don't have a strong
opinion about the "#" other than to note that it seems inconsistent
with other commands which don't use "#" as a separator.
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision
2018-01-19 12:24 ` [PATCH 1/8] sequencer: introduce new commands to resettherevision Phillip Wood
@ 2018-01-19 18:55 ` Phillip Wood
2018-01-19 18:59 ` Jacob Keller
2018-01-29 21:23 ` Johannes Schindelin1 sibling, 1 reply; 412+ messages in thread
From: Phillip Wood @ 2018-01-19 18:55 UTC (permalink / raw)
To: Johannes Schindelin, git
Cc: Junio C Hamano, Jacob Keller, Philip Oakley, Eric Sunshine
On 19/01/18 12:24, Phillip Wood wrote:
>
> On 18/01/18 15:35, Johannes Schindelin wrote:
>>
>> Internally, the `label <name>` command creates the ref
>> `refs/rewritten/<name>`. This makes it possible to work with the labeled
>> revisions interactively, or in a scripted fashion (e.g. via the todo
>> list command `exec`).
>
> If a user has two work trees and runs a rebase in each with the same
> label name, they'll clobber each other. I'd suggest storing them under
> refs/rewritten/<branch-name or detached HEAD SHA> instead. If the user
> tries to rebase a second worktree with the same detached HEAD as an
> existing rebase then refuse to start.
>
Ah this isn't a concern after all as patch 5 makes refs/rewritten local
to the worktree. Perhaps you could move that part of patch 5 here or add
a note to the commit message that it will become worktree local later in
the series
Best Wishes
Phillip
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision
2018-01-19 18:55 ` Phillip Wood@ 2018-01-19 18:59 ` Jacob Keller
2018-01-29 21:25 ` Johannes Schindelin0 siblings, 1 reply; 412+ messages in thread
From: Jacob Keller @ 2018-01-19 18:59 UTC (permalink / raw)
To: Phillip Wood
Cc: Johannes Schindelin, Git mailing list, Junio C Hamano,
Philip Oakley, Eric Sunshine
On Fri, Jan 19, 2018 at 10:55 AM, Phillip Wood
<phillip.wood@talktalk.net> wrote:
> On 19/01/18 12:24, Phillip Wood wrote:
>>
>> On 18/01/18 15:35, Johannes Schindelin wrote:
>>>
>>> Internally, the `label <name>` command creates the ref
>>> `refs/rewritten/<name>`. This makes it possible to work with the labeled
>>> revisions interactively, or in a scripted fashion (e.g. via the todo
>>> list command `exec`).
>>
>> If a user has two work trees and runs a rebase in each with the same
>> label name, they'll clobber each other. I'd suggest storing them under
>> refs/rewritten/<branch-name or detached HEAD SHA> instead. If the user
>> tries to rebase a second worktree with the same detached HEAD as an
>> existing rebase then refuse to start.
>>
>
> Ah this isn't a concern after all as patch 5 makes refs/rewritten local
> to the worktree. Perhaps you could move that part of patch 5 here or add
> a note to the commit message that it will become worktree local later in
> the series
>
> Best Wishes
>
> Phillip
I'd rather it be included here as well.
Thanks,
Jake
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision
2018-01-19 18:59 ` Jacob Keller@ 2018-01-29 21:25 ` Johannes Schindelin
2018-01-29 21:29 ` Johannes Schindelin0 siblings, 1 reply; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 21:25 UTC (permalink / raw)
To: Jacob Keller
Cc: Phillip Wood, Git mailing list, Junio C Hamano, Philip Oakley,
Eric Sunshine
Hi Jake,
On Fri, 19 Jan 2018, Jacob Keller wrote:
> On Fri, Jan 19, 2018 at 10:55 AM, Phillip Wood
> <phillip.wood@talktalk.net> wrote:
> > On 19/01/18 12:24, Phillip Wood wrote:
> >>
> >> On 18/01/18 15:35, Johannes Schindelin wrote:
> >>>
> >>> Internally, the `label <name>` command creates the ref
> >>> `refs/rewritten/<name>`. This makes it possible to work with the labeled
> >>> revisions interactively, or in a scripted fashion (e.g. via the todo
> >>> list command `exec`).
> >>
> >> If a user has two work trees and runs a rebase in each with the same
> >> label name, they'll clobber each other. I'd suggest storing them under
> >> refs/rewritten/<branch-name or detached HEAD SHA> instead. If the user
> >> tries to rebase a second worktree with the same detached HEAD as an
> >> existing rebase then refuse to start.
> >>
> >
> > Ah this isn't a concern after all as patch 5 makes refs/rewritten local
> > to the worktree. Perhaps you could move that part of patch 5 here or add
> > a note to the commit message that it will become worktree local later in
> > the series
> >
> > Best Wishes
> >
> > Phillip
>
> I'd rather it be included here as well.
But it would have been really easy to overlook in here. I really want this
to be a separate commit, also to have a chance to get this done
*differently* if somebody comes up with a splendid idea how to do that
(because hard-coding feels quite dirty).
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision
2018-01-29 21:25 ` Johannes Schindelin@ 2018-01-29 21:29 ` Johannes Schindelin0 siblings, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 21:29 UTC (permalink / raw)
To: Jacob Keller
Cc: Phillip Wood, Git mailing list, Junio C Hamano, Philip Oakley,
Eric Sunshine
Hi,
On Mon, 29 Jan 2018, Johannes Schindelin wrote:
> On Fri, 19 Jan 2018, Jacob Keller wrote:
>
> > On Fri, Jan 19, 2018 at 10:55 AM, Phillip Wood
> > <phillip.wood@talktalk.net> wrote:
> > > On 19/01/18 12:24, Phillip Wood wrote:
> > >>
> > >> On 18/01/18 15:35, Johannes Schindelin wrote:
> > >>>
> > >>> Internally, the `label <name>` command creates the ref
> > >>> `refs/rewritten/<name>`. This makes it possible to work with the labeled
> > >>> revisions interactively, or in a scripted fashion (e.g. via the todo
> > >>> list command `exec`).
> > >>
> > >> If a user has two work trees and runs a rebase in each with the same
> > >> label name, they'll clobber each other. I'd suggest storing them under
> > >> refs/rewritten/<branch-name or detached HEAD SHA> instead. If the user
> > >> tries to rebase a second worktree with the same detached HEAD as an
> > >> existing rebase then refuse to start.
> > >>
> > >
> > > Ah this isn't a concern after all as patch 5 makes refs/rewritten local
> > > to the worktree. Perhaps you could move that part of patch 5 here or add
> > > a note to the commit message that it will become worktree local later in
> > > the series
> > >
> > > Best Wishes
> > >
> > > Phillip
> >
> > I'd rather it be included here as well.
>
> But it would have been really easy to overlook in here. I really want this
> to be a separate commit, also to have a chance to get this done
> *differently* if somebody comes up with a splendid idea how to do that
> (because hard-coding feels quite dirty).
BTW there is an additional good reason why the patch to make
refs/rewritten/* worktree-local is so far away: that is the first time in
the patch series when we can test this really effectively; at that stage
we can easily just add to t3430 because all the building blocks for
`rebase -i --recreate-merges` are in place.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision
2018-01-19 12:24 ` [PATCH 1/8] sequencer: introduce new commands to resettherevision Phillip Wood
2018-01-19 18:55 ` Phillip Wood@ 2018-01-29 21:23 ` Johannes Schindelin1 sibling, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 21:23 UTC (permalink / raw)
To: phillip.wood
Cc: git, Junio C Hamano, Jacob Keller, Philip Oakley, Eric Sunshine
Hi Phillip,
On Fri, 19 Jan 2018, Phillip Wood wrote:
>
> On 18/01/18 15:35, Johannes Schindelin wrote:
>
> > This idea was developed in Git for Windows' Git garden shears (that
> > are used to maintain the "thicket of branches" on top of upstream
> > Git), and this patch is part of the effort to make it available to a
> > wider audience, as well as to make the entire process more robust (by
> > implementing it in a safe and portable language rather than a Unix
> > shell script).
> >
> > This commit implements the commands to label, and to reset to, given
> > revisions. The syntax is:
> >
> > label <name>
> > reset <name>
>
> If I've understood the code below correctly then reset will clobber
> untracked files, this is the opposite behaviour to what happens when
> tries to checkout <onto> at the start of a rebase - then it will fail if
> untracked files would be overwritten.
This would be completely unintentional, I will verify that untracked files
are not clobbered.
However, in practice this should not happen because the intended use case
is for revisions to be labeled *before* checking them out at a later
stage. Therefore, the files that would be clobbered would already have
been tracked in the revision when it was labeled, and I do not quite see
how those files could become untracked without playing sloppy exec games
in between.
> > Internally, the `label <name>` command creates the ref
> > `refs/rewritten/<name>`. This makes it possible to work with the labeled
> > revisions interactively, or in a scripted fashion (e.g. via the todo
> > list command `exec`).
>
> If a user has two work trees and runs a rebase in each with the same
> label name, they'll clobber each other. I'd suggest storing them under
> refs/rewritten/<branch-name or detached HEAD SHA> instead. If the user
> tries to rebase a second worktree with the same detached HEAD as an
> existing rebase then refuse to start.
That is why a later patch marks those refs/rewritten/ refs as
worktree-local.
> > +static int do_label(const char *name, int len)
> > +{
> > + struct ref_store *refs = get_main_ref_store();
> > + struct ref_transaction *transaction;
> > + struct strbuf ref_name = STRBUF_INIT, err = STRBUF_INIT;
> > + struct strbuf msg = STRBUF_INIT;
> > + int ret = 0;
> > + struct object_id head_oid;
> > +
> > + strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
> > + strbuf_addf(&msg, "label '%.*s'", len, name);
>
> The other reflog messages below have a (rebase -i) prefix
Good point. I changed it to "rebase -i (label)".
> > + transaction = ref_store_transaction_begin(refs, &err);
> > + if (!transaction ||
> > + get_oid("HEAD", &head_oid) ||
> > + ref_transaction_update(transaction, ref_name.buf, &head_oid, NULL,
> > + 0, msg.buf, &err) < 0 ||
> > + ref_transaction_commit(transaction, &err)) {
> > + error("%s", err.buf);
>
> if get_oid() fails then err is empty so there wont be an message after
> the 'error: '
Yep, that would be nasty. Fixed.
> > +static int do_reset(const char *name, int len)
> > +{
> > + struct strbuf ref_name = STRBUF_INIT;
> > + struct object_id oid;
> > + struct lock_file lock = LOCK_INIT;
> > + struct tree_desc desc;
> > + struct tree *tree;
> > + struct unpack_trees_options opts;
> > + int ret = 0, i;
> > +
> > + if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> > + return -1;
> > +
> > + for (i = 0; i < len; i++)
> > + if (isspace(name[i]))
> > + len = i;
>
> If name starts with any white space then I think this effectively
> truncates name to a bunch of white space which doesn't sound right. I'm
> not sure how this is being called, but it might be better to clean up
> name when the to-do list is parsed instead.
The left-trimming of the name was already performed as part of the todo
list parsing.
And we are not really right-trimming here. We are splitting a line of the
form
reset <label> <oneline>
In fact, after reflecting about it, I changed the code so that it would
now even read:
reset <label> # <oneline>
So the code really is doing the intended thing here.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 2/8] sequencer: introduce the `merge` command
2018-01-18 15:35 ` [PATCH 2/8] sequencer: introduce the `merge` command Johannes Schindelin
@ 2018-01-18 16:31 ` Jacob Keller
2018-01-18 21:22 ` Johannes Schindelin
2018-01-19 9:54 ` Eric Sunshine
` (2 subsequent siblings)3 siblings, 1 reply; 412+ messages in thread
From: Jacob Keller @ 2018-01-18 16:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git mailing list, Junio C Hamano
On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> 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 <commit> <rev> <oneline>
>
> The <commit> parameter in this instance is the *original* merge commit,
> whose author and message will be used for the to-be-created merge
> commit.
>
> The <rev> 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 <commit>
> parameter (in which case the text after the <rev> 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 <rev> 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
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 2/8] sequencer: introduce the `merge` command
2018-01-18 16:31 ` Jacob Keller@ 2018-01-18 21:22 ` Johannes Schindelin
2018-01-18 21:26 ` Jacob Keller0 siblings, 1 reply; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-18 21:22 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, Junio C Hamano
Hi Jake,
On Thu, 18 Jan 2018, Jacob Keller wrote:
> On Thu, Jan 18, 2018 at 7:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> 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 <commit> <rev> <oneline>
> >
> > The <commit> parameter in this instance is the *original* merge commit,
> > whose author and message will be used for the to-be-created merge
> > commit.
> >
> > The <rev> 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 <commit>
> > parameter (in which case the text after the <rev> 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 <rev> 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
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 2/8] sequencer: introduce the `merge` command
2018-01-18 21:22 ` Johannes Schindelin@ 2018-01-18 21:26 ` Jacob Keller0 siblings, 0 replies; 412+ messages in thread
From: Jacob Keller @ 2018-01-18 21:26 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git mailing list, Junio C Hamano
On Thu, Jan 18, 2018 at 1:22 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>> Would it be possible to open the editor with the supplied text when
>> there's no commit? The text after <rev> 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
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 2/8] sequencer: introduce the `merge` command
2018-01-19 14:45 ` Phillip Wood@ 2018-01-20 9:18 ` Jacob Keller
2018-01-29 21:41 ` Johannes Schindelin0 siblings, 1 reply; 412+ messages in thread
From: Jacob Keller @ 2018-01-20 9:18 UTC (permalink / raw)
To: Phillip Wood
Cc: Johannes Schindelin, Git mailing list, Junio C Hamano, Eric Sunshine
On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood <phillip.wood@talktalk.net> 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 <commit> <rev> <oneline>
>
> 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 <commit> <rev> ...' instead so it's clear which argument specifies
> the message and which the remote head to merge. It would also allow for
> 'merge -c <commit> <rev> ...' in the future for rewording an existing
> merge message and also avoid the slightly odd 'merge - <rev> ...'. 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 <commit> ... to take message from commit
merge -c <commit> ... to take the message from commit and open editor to edit
merge -m "<message>" ... 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.
>
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 2/8] sequencer: introduce the `merge` command
2018-01-20 9:18 ` Jacob Keller@ 2018-01-29 21:41 ` Johannes Schindelin
2018-01-31 13:48 ` Johannes Schindelin0 siblings, 1 reply; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 21:41 UTC (permalink / raw)
To: Jacob Keller
Cc: Phillip Wood, Git mailing list, Junio C Hamano, Eric Sunshine
Hi Jake & Phillip,
On Sat, 20 Jan 2018, Jacob Keller wrote:
> On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood <phillip.wood@talktalk.net> 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 <commit> <rev> <oneline>
> >
> > 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 <commit> <rev> ...' instead so it's clear which argument specifies
> > the message and which the remote head to merge. It would also allow for
> > 'merge -c <commit> <rev> ...' in the future for rewording an existing
> > merge message and also avoid the slightly odd 'merge - <rev> ...'. 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 <commit> ... 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 <commit> ... to take the message from commit and open editor to edit
> merge -m "<message>" ... 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
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 2/8] sequencer: introduce the `merge` command
2018-01-29 21:41 ` Johannes Schindelin@ 2018-01-31 13:48 ` Johannes Schindelin
2018-01-31 17:58 ` Phillip Wood
2018-02-01 6:40 ` Jacob Keller0 siblings, 2 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-31 13:48 UTC (permalink / raw)
To: Jacob Keller
Cc: Phillip Wood, Git mailing list, Junio C Hamano, Eric Sunshine
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 <phillip.wood@talktalk.net> wrote:
> > > On 18/01/18 15:35, Johannes Schindelin wrote:
> > >>
> > >> This patch adds the `merge` command, with the following syntax:
> > >>
> > >> merge <commit> <rev> <oneline>
> > >
> > > 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 <commit> <rev> ...' instead so it's clear which
> > > argument specifies the message and which the remote head to merge.
> > > It would also allow for 'merge -c <commit> <rev> ...' in the future
> > > for rewording an existing merge message and also avoid the slightly
> > > odd 'merge - <rev> ...'. 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 <commit> ... 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 <commit> ... to take the message from commit and open editor to edit
> > merge -m "<message>" ... 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 <original> <to-merge> <oneline>
and for octopus:
merge <original> "<to-merge> <to-merge2>..." <oneline>...
I think with the -C syntax, it would become something like
merge -C <original> <to-merge> # <oneline>
and
merge -C <original> <to-merge> <to-merge2>...
# Merging: <oneline>
# Merging: <oneline2>
# ...
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
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 2/8] sequencer: introduce the `merge` command
2018-01-31 13:48 ` Johannes Schindelin@ 2018-01-31 17:58 ` Phillip Wood
2018-02-01 6:40 ` Jacob Keller1 sibling, 0 replies; 412+ messages in thread
From: Phillip Wood @ 2018-01-31 17:58 UTC (permalink / raw)
To: Johannes Schindelin, Jacob Keller
Cc: Phillip Wood, Git mailing list, Junio C Hamano, Eric Sunshine
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 <phillip.wood@talktalk.net> wrote:
>>>> On 18/01/18 15:35, Johannes Schindelin wrote:
>>>>>
>>>>> This patch adds the `merge` command, with the following syntax:
>>>>>
>>>>> merge <commit> <rev> <oneline>
>>>>
>>>> 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 <commit> <rev> ...' instead so it's clear which
>>>> argument specifies the message and which the remote head to merge.
>>>> It would also allow for 'merge -c <commit> <rev> ...' in the future
>>>> for rewording an existing merge message and also avoid the slightly
>>>> odd 'merge - <rev> ...'. 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 <commit> ... 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 <commit> ... to take the message from commit and open editor to edit
>>> merge -m "<message>" ... 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 <original> <to-merge> <oneline>
>
> and for octopus:
>
> merge <original> "<to-merge> <to-merge2>..." <oneline>...
>
> I think with the -C syntax, it would become something like
>
> merge -C <original> <to-merge> # <oneline>
>
> and
>
> merge -C <original> <to-merge> <to-merge2>...
> # Merging: <oneline>
> # Merging: <oneline2>
> # ...
>
> 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 <original> <to-merge> : <oneline>
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
>
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 2/8] sequencer: introduce the `merge` command
2018-01-31 13:48 ` Johannes Schindelin
2018-01-31 17:58 ` Phillip Wood@ 2018-02-01 6:40 ` Jacob Keller1 sibling, 0 replies; 412+ messages in thread
From: Jacob Keller @ 2018-02-01 6:40 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Phillip Wood, Git mailing list, Junio C Hamano, Eric Sunshine
On Wed, Jan 31, 2018 at 5:48 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> 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 <phillip.wood@talktalk.net> wrote:
>> > > On 18/01/18 15:35, Johannes Schindelin wrote:
>> > >>
>> > >> This patch adds the `merge` command, with the following syntax:
>> > >>
>> > >> merge <commit> <rev> <oneline>
>> > >
>> > > 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 <commit> <rev> ...' instead so it's clear which
>> > > argument specifies the message and which the remote head to merge.
>> > > It would also allow for 'merge -c <commit> <rev> ...' in the future
>> > > for rewording an existing merge message and also avoid the slightly
>> > > odd 'merge - <rev> ...'. 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 <commit> ... 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 <commit> ... to take the message from commit and open editor to edit
>> > merge -m "<message>" ... 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 <original> <to-merge> <oneline>
>
> and for octopus:
>
> merge <original> "<to-merge> <to-merge2>..." <oneline>...
>
> I think with the -C syntax, it would become something like
>
> merge -C <original> <to-merge> # <oneline>
>
I like this, especially given you added the "#" for one of the other
new commands as well, (reset I think?)
> and
>
> merge -C <original> <to-merge> <to-merge2>...
> # Merging: <oneline>
> # Merging: <oneline2>
> # ...
>
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
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 3/8] sequencer: fast-forward merge commits, if possible
2018-01-19 14:53 ` Phillip Wood@ 2018-01-23 19:12 ` Junio C Hamano
2018-01-24 10:32 ` Phillip Wood
2018-01-29 21:47 ` Johannes Schindelin1 sibling, 1 reply; 412+ messages in thread
From: Junio C Hamano @ 2018-01-23 19:12 UTC (permalink / raw)
To: Phillip Wood; +Cc: Johannes Schindelin, git, Jacob Keller
Phillip Wood <phillip.wood@talktalk.net> writes:
> On 18/01/18 15:35, Johannes Schindelin wrote:
>>
>> Just like with regular `pick` commands, if we are trying to recreate a
>> merge commit, we now test whether the parents of said commit match HEAD
>> and the commits to be merged, and fast-forward if possible.
>>
>> This is not only faster, but also avoids unnecessary proliferation of
>> new objects.
>
> I might have missed something but shouldn't this be checking opts->allow_ff?
Because the whole point of this mechanism is to recreate the
topology faithfully to the original, even if the original was a
redundant merge (which has a side parent that is an ancestor or a
descendant of the first parent), we should just point at the
original merge when the condition allows it, regardless of
opts->allow_ff.
I think it is a different matter if an insn to create a new merge
(i.e. "merge - <parent> <message>", not "merge <commit> <parent>")
should honor opts->allow_ff; because it is not about recreating an
existing history but is a way to create what did not exist before,
I think it is sensible if allow_ff option is honored.
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 3/8] sequencer: fast-forward merge commits, if possible
2018-01-23 19:12 ` Junio C Hamano@ 2018-01-24 10:32 ` Phillip Wood
2018-01-24 18:51 ` Junio C Hamano0 siblings, 1 reply; 412+ messages in thread
From: Phillip Wood @ 2018-01-24 10:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, Jacob Keller
On 23/01/18 19:12, Junio C Hamano wrote:
> Phillip Wood <phillip.wood@talktalk.net> writes:
>
>> On 18/01/18 15:35, Johannes Schindelin wrote:
>>>
>>> Just like with regular `pick` commands, if we are trying to recreate a
>>> merge commit, we now test whether the parents of said commit match HEAD
>>> and the commits to be merged, and fast-forward if possible.
>>>
>>> This is not only faster, but also avoids unnecessary proliferation of
>>> new objects.
>>
>> I might have missed something but shouldn't this be checking opts->allow_ff?
>
> Because the whole point of this mechanism is to recreate the
> topology faithfully to the original, even if the original was a
> redundant merge (which has a side parent that is an ancestor or a
> descendant of the first parent), we should just point at the
> original merge when the condition allows it, regardless of
> opts->allow_ff.
I agree that the merge should be recreated, but I was thinking of
something slightly different. Currently the sequencer uses
opts->allow_ff to control whether a new commit with the same contents
should be created even if the existing one could be reused. So I was
querying whether we should recreate the commit when the user run 'git
rebase --recreate-merges --no-ff' rather than just reusing it. As merges
also have another meaning for fast-forward the terminology gets confusing.
> I think it is a different matter if an insn to create a new merge
> (i.e. "merge - <parent> <message>", not "merge <commit> <parent>")
> should honor opts->allow_ff; because it is not about recreating an
> existing history but is a way to create what did not exist before,
> I think it is sensible if allow_ff option is honored.
This is the merge sense of 'fast-forward' not the existing sequencer
sense, without thinking about it more I'm not sure if one command line
option for rebase is sufficient to cover both uses.
Best Wishes
Phillip
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 3/8] sequencer: fast-forward merge commits, if possible
2018-01-24 10:32 ` Phillip Wood@ 2018-01-24 18:51 ` Junio C Hamano0 siblings, 0 replies; 412+ messages in thread
From: Junio C Hamano @ 2018-01-24 18:51 UTC (permalink / raw)
To: Phillip Wood; +Cc: Johannes Schindelin, git, Jacob Keller
Phillip Wood <phillip.wood@talktalk.net> writes:
> I agree that the merge should be recreated, but I was thinking of
> something slightly different. Currently the sequencer uses
> opts->allow_ff to control whether a new commit with the same contents
> should be created even if the existing one could be reused.
Ahh, OK. I misunderstood what you meant. Yes, what you said makes
sense to me.
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 3/8] sequencer: fast-forward merge commits, if possible
2018-01-19 14:53 ` Phillip Wood
2018-01-23 19:12 ` Junio C Hamano@ 2018-01-29 21:47 ` Johannes Schindelin1 sibling, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 21:47 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Junio C Hamano, Jacob Keller
Hi Phillip,
On Fri, 19 Jan 2018, Phillip Wood wrote:
> On 18/01/18 15:35, Johannes Schindelin wrote:
> >
> > Just like with regular `pick` commands, if we are trying to recreate a
> > merge commit, we now test whether the parents of said commit match HEAD
> > and the commits to be merged, and fast-forward if possible.
> >
> > This is not only faster, but also avoids unnecessary proliferation of
> > new objects.
>
> I might have missed something but shouldn't this be checking
> opts->allow_ff?
Good point. This is the type of review for which I was hoping.
> Another possible optimization is that if the parent branches have only
> reworded commits or some commits that have been squashed but no other
> changes then their trees will be the same as in the original merge
> commit and so could be reused without calling merge_recursive().
True. It is also a bit involved to check this condition, and I am not sure
that it is worth the effort for my use case.
So I would invite you to work on this after this patch series settles, if
you are interested.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 4/8] rebase-helper --make-script: introduce a flag to recreate merges
2018-01-19 10:34 ` Eric Sunshine@ 2018-01-23 20:13 ` Junio C Hamano
2018-01-29 21:07 ` Johannes Schindelin
2018-01-29 21:05 ` Johannes Schindelin1 sibling, 1 reply; 412+ messages in thread
From: Junio C Hamano @ 2018-01-23 20:13 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Johannes Schindelin, Git List, Jacob Keller
Eric Sunshine <sunshine@sunshineco.com> writes:
>> + is_octopus = to_merge && to_merge->next;
>> +
>> + if (is_octopus)
>> + BUG("Octopus merges not yet supported");
>
> Is this a situation which the end-user can trigger by specifying a
> merge with more than two parents? If so, shouldn't this be just a
> normal error message rather than a (developer) bug message? Or, am I
> misunderstanding?
BUG() is "we wrote code carefully so that this should not trigger;
we do not _expect_ the code to reach here". This one is expected to
trigger, and I agree with you that it should be die(), if the series
is meant to be released to the general public in the current form
(i.e. until the limitation is lifted so that it can handle an
octopus).
If the callers are made more careful to check if there is an octopus
involved and reject the request early, then seeing an octopus in
this location in a loop will become a BUG().
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 4/8] rebase-helper --make-script: introduce a flag to recreate merges
2018-01-23 20:13 ` Junio C Hamano@ 2018-01-29 21:07 ` Johannes Schindelin0 siblings, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 21:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, Git List, Jacob Keller
Hi Junio,
On Tue, 23 Jan 2018, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> >> + is_octopus = to_merge && to_merge->next;
> >> +
> >> + if (is_octopus)
> >> + BUG("Octopus merges not yet supported");
> >
> > Is this a situation which the end-user can trigger by specifying a
> > merge with more than two parents? If so, shouldn't this be just a
> > normal error message rather than a (developer) bug message? Or, am I
> > misunderstanding?
>
> BUG() is "we wrote code carefully so that this should not trigger;
> we do not _expect_ the code to reach here". This one is expected to
> trigger, and I agree with you that it should be die(), if the series
> is meant to be released to the general public in the current form
> (i.e. until the limitation is lifted so that it can handle an
> octopus).
>
> If the callers are made more careful to check if there is an octopus
> involved and reject the request early, then seeing an octopus in
> this location in a loop will become a BUG().
This has occupied both of you for way too long.
It is *not interesting*. What *is* interesting is for example the
discussion about the "cousin commits". And maybe both of you gentle
persons can spend your brain cycles splendidly by trying to come up with a
better term. Or by trying to beat out obvious or not-so-obvious bugs in
the code.
Seriously, I am not interested in a discussion about BUG() vs die() as
long as there may be real bugs hiding.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 4/8] rebase-helper --make-script: introduce a flag to recreate merges
2018-01-19 10:34 ` Eric Sunshine
2018-01-23 20:13 ` Junio C Hamano@ 2018-01-29 21:05 ` Johannes Schindelin1 sibling, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 21:05 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Junio C Hamano, Jacob Keller
Hi Eric,
On Fri, 19 Jan 2018, Eric Sunshine wrote:
> On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>
> > structure (similar in spirit to --preserve-merges, but with a
> > substantially less-broken design).
> > [...]
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -2785,6 +2787,335 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
> > +static const char *label_oid(struct object_id *oid, const char *label,
> > + struct label_state *state)
> > +{
> > + [...]
> > + } else if (((len = strlen(label)) == GIT_SHA1_RAWSZ &&
> > + !get_oid_hex(label, &dummy)) ||
> > + hashmap_get_from_hash(&state->labels,
> > + strihash(label), label)) {
> > + /*
> > + * If the label already exists, or if the label is a valid full
> > + * OID, we append a dash and a number to make it unique.
> > + */
> > + [...]
> > + for (i = 2; ; i++) {
>
> Why '2'? Is there some non-obvious significance to this value?
I personally found it irritating to have labels "sequencer",
"sequencer-1". It sounds *wrong* to have a "-1". Because it is the second
label referring to the term "sequencer". So if there are two labels that
both want to be named "sequencer", the first one wins, and the second one
will be called "sequencer-2".
Hence the 2.
> > +static int make_script_with_merges(struct pretty_print_context *pp,
> > + struct rev_info *revs, FILE *out,
> > + unsigned flags)
> > +{
> > + [...]
> > + is_octopus = to_merge && to_merge->next;
> > +
> > + if (is_octopus)
> > + BUG("Octopus merges not yet supported");
>
> Is this a situation which the end-user can trigger by specifying a
> merge with more than two parents? If so, shouldn't this be just a
> normal error message rather than a (developer) bug message? Or, am I
> misunderstanding?
You are misunderstanding.
This is just a place-holder here. The patches to introduce support for
octopus merges are already written. They are lined up after this here
patch series, is all.
As such, please do not occupy your mind on the specifics or even the
upper-case of the "Octopus". This line is here only as a hint for the
reviewer that this is not yet implemented. And BUG(...) was chosen because
that way, we are not even tempted to waste the time of translators.
Speaking of wasting time... let's move on to further interesting code
reviews.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-07 6:16 ` Sergey Organov@ 2018-02-07 7:26 ` Jacob Keller
2018-02-07 9:47 ` Sergey Organov
2018-02-07 7:27 ` Johannes Sixt
2018-02-07 17:36 ` Johannes Schindelin2 siblings, 1 reply; 412+ messages in thread
From: Jacob Keller @ 2018-02-07 7:26 UTC (permalink / raw)
To: Sergey Organov
Cc: Johannes Schindelin, Git mailing list, Junio C Hamano, Johannes Sixt
On Tue, Feb 6, 2018 at 10:16 PM, Sergey Organov <sorganov@gmail.com> wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> [...]
>
>> +--recreate-merges::
>> + Recreate merge commits instead of flattening the history by replaying
>> + merges. Merge conflict resolutions or manual amendments to merge
>> + commits are not preserved.
>
> I wonder why you guys still hold on replaying "merge-the-operation"
> instead of replaying "merge-the-result"? The latter, the merge commit
> itself, no matter how exactly it was created in the first place, is the
> most valuable thing git keeps about the merge, and you silently drop it
> entirely! OTOH, git keeps almost no information about
> "merge-the-operation", so it's virtually impossible to reliably replay
> the operation automatically, and yet you try to.
>
I'm not sure I follow what you mean here?
You mean that you'd want this to actually attempt to re-create the
original merge including conflict resolutions by taking the contents
of the result?
How do you handle if that result has conflicts? What UX do you present
to the user to handle such conflicts? I don't think the normal 3-way
conflicts would even be possible in this case?
Thanks,
Jake
> IMHO that was severe mistake in the original --preserve-merges, and you
> bring with you to this new --recreate-merges... It's sad. Even more sad
> as solution is already known for years:
>
> bc00341838a8faddcd101da9e746902994eef38a
> Author: Johannes Sixt <j6t@kdbg.org>
> Date: Sun Jun 16 15:50:42 2013 +0200
>
> rebase -p --first-parent: redo merge by cherry-picking first-parent change
>
> and it works like a charm.
>
> -- Sergey
>
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-07 7:26 ` Jacob Keller@ 2018-02-07 9:47 ` Sergey Organov0 siblings, 0 replies; 412+ messages in thread
From: Sergey Organov @ 2018-02-07 9:47 UTC (permalink / raw)
To: Jacob Keller
Cc: Johannes Schindelin, Git mailing list, Junio C Hamano, Johannes Sixt
Jacob Keller <jacob.keller@gmail.com> writes:
> On Tue, Feb 6, 2018 at 10:16 PM, Sergey Organov <sorganov@gmail.com> wrote:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>> [...]
>>
>>> +--recreate-merges::
>>> + Recreate merge commits instead of flattening the history by replaying
>>> + merges. Merge conflict resolutions or manual amendments to merge
>>> + commits are not preserved.
>>
>> I wonder why you guys still hold on replaying "merge-the-operation"
>> instead of replaying "merge-the-result"? The latter, the merge commit
>> itself, no matter how exactly it was created in the first place, is the
>> most valuable thing git keeps about the merge, and you silently drop it
>> entirely! OTOH, git keeps almost no information about
>> "merge-the-operation", so it's virtually impossible to reliably replay
>> the operation automatically, and yet you try to.
>>
>
> I'm not sure I follow what you mean here?
>
> You mean that you'd want this to actually attempt to re-create the
> original merge including conflict resolutions by taking the contents
> of the result?
I mean just cherry-pick the merge the same way all other commits are
essentially cherry-picked during rebase. That's what Johannes Sixt did
in his patch I was reffering to.
> How do you handle if that result has conflicts? What UX do you present
> to the user to handle such conflicts? I don't think the normal 3-way
> conflicts would even be possible in this case?
No problem here. It goes exactly the same way as for non-merge commits
that are being rebased. You can try it right now using
$ git cherry-pick -m1 <merge_commit>
that will induce conflicts.
The (somewhat tricky) functional difference is only in recording correct
additional parents to the final commit, but that part is hidden from the
user.
-- Sergey
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-07 6:16 ` Sergey Organov
2018-02-07 7:26 ` Jacob Keller
2018-02-07 7:27 ` Johannes Sixt@ 2018-02-07 17:36 ` Johannes Schindelin
2018-02-07 22:58 ` Øyvind Rønningstad
2018-02-09 6:11 ` Sergey Organov2 siblings, 2 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-02-07 17:36 UTC (permalink / raw)
To: Sergey Organov; +Cc: git, Junio C Hamano, Jacob Keller, Johannes Sixt
Hi,
On Wed, 7 Feb 2018, Sergey Organov wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> [...]
>
> > +--recreate-merges::
> > + Recreate merge commits instead of flattening the history by replaying
> > + merges. Merge conflict resolutions or manual amendments to merge
> > + commits are not preserved.
>
> I wonder why you guys still hold on replaying "merge-the-operation"
> instead of replaying "merge-the-result"?
This misses the point of rebasing: you want to replay the changes.
> The latter, the merge commit itself, no matter how exactly it was
> created in the first place, is the most valuable thing git keeps about
> the merge, and you silently drop it entirely!
You miss another very crucial point. I don't blame you, as you certainly
have not used the Git garden shears for years.
Let me explain the scenario which comes up plenty of times in my work with
Git for Windows. We have a thicket of some 70 branches on top of git.git's
latest release. These branches often include fixup! and squash! commits
and even more complicated constructs that rebase cannot handle at all at
the moment, such as reorder-before! and reorder-after! (for commits that
really need to go into a different branch).
Even if you do not have such a complicated setup, it is quite possible
that you need to include a commit in your development that needs to be
dropped before contributing your work. Think e.g. removing the `-O2` flag
when compiling with GCC because GDB gets utterly confused with executables
compiled with `-O2` while single-stepping. This could be an initial commit
called `TO-DROP` or some such.
And guess what happens if you drop that `pick` line in your todo list and
then the `merge` command simply tries to re-create the original merge
commit's changes?
Exactly. The merge will become an evil merge, and will introduce that very
much not-wanted and therefore-dropped changes.
> OTOH, git keeps almost no information about "merge-the-operation", so
> it's virtually impossible to reliably replay the operation
> automatically, and yet you try to.
That is true. However, the intended use case is not to allow you to
recreate funny merges. Its use case is to allow you to recreate merges.
At a later stage, I might introduce support to detect `-s ours` merges,
because they are easy to detect. But even then, it will be an opt-in.
> IMHO that was severe mistake in the original --preserve-merges, and you
> bring with you to this new --recreate-merges... It's sad.
Please refrain from drawing this discussion into an emotional direction.
That is definitely not helpful.
> Even more sad as solution is already known for years:
>
> bc00341838a8faddcd101da9e746902994eef38a
> Author: Johannes Sixt <j6t@kdbg.org>
> Date: Sun Jun 16 15:50:42 2013 +0200
>
> rebase -p --first-parent: redo merge by cherry-picking first-parent change
>
> and it works like a charm.
It might work for you, as you probably used --preserve-merges, and dealt
with the fact that you could neither drop nor reorder commits.
So --preserve-merges --first-parent is probably what you were looking for.
Instead, --recreate-merges is all about allowing the same level of freedom
as with regular interactive rebases, but recreating the original commit
topology (and allowing to change it, too).
Therefore, I think that it would be even harmful to allow
--recreate-merges --first-parent *because it would cause evil merges*!
And I totally could see myself being vexed again about options that worked
perfectly well (just like --preserve-merges) being completely messed up by
allowing it to be combined with options *that they cannot work with* (just
like --preserve-merges --interactive, a *huge* mistake causing so many
annoying "bug" reports: I *never intended it that way because I knew it
would not work as users expect*).
So no, I do not think that --recreate-merges --first-parent is a good idea
at all. Unless you try to do that non-interactively only, *and disallow it
in interactive mode*. Because the entire point of the interactive rebase
is to allow reordering and dropping commits, in --recreate-merges even
moving, introducing and dropping merge commits. The --first-parent option
flies in the face of this idea.
Ciao,
Johannes
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-07 17:36 ` Johannes Schindelin@ 2018-02-07 22:58 ` Øyvind Rønningstad
2018-02-07 23:31 ` Junio C Hamano
2018-02-09 6:11 ` Sergey Organov1 sibling, 1 reply; 412+ messages in thread
From: Øyvind Rønningstad @ 2018-02-07 22:58 UTC (permalink / raw)
To: Johannes Schindelin, Sergey Organov
Cc: git, Junio C Hamano, Jacob Keller, Johannes Sixt
edit: Sending again, hopefully without HTML :). Sorry for spamming.
Hi, I think --recreate-merges is a very exciting feature.
I've also been puzzled by why we can't just pick merge commits directly
including
conflict resolutions, so allow me to join the discussion.
On Wed, Feb 7, 2018 at 6:36 PM, Johannes Schindelin <Johannes.Schindeli
n@gmx.de> wrote:
>
> Hi,
>
> [...]
>
> And guess what happens if you drop that `pick` line in your todo list
and
> then the `merge` command simply tries to re-create the original merge
> commit's changes?
>
> Exactly. The merge will become an evil merge, and will introduce that
very
> much not-wanted and therefore-dropped changes.
I think I understand. Evil merges happen when we change the branch
that is not the mainline..? Is there any reason why the following
wouldn't work?
Imagine rebase is about to pick a merge commit, and we have edited at
least one
commit in each branch to be merged.
1. apply patch mainline_orig..merge_orig
2. apply patch branch1_orig..branch1
...
N. apply patch branchN_orig..branchN
N+1. Commit merge
I do see complications, like the fact that steps 2-N can be done in any
order, with
possibly quite different results. Moving commits from one branch to
another might
not work very well. And what to do when you remove branches or create
new ones?
These problems might be prohibitive, but picking merge commits seems
like
something that should be possible to do.
>
> [...]
>
> So --preserve-merges --first-parent is probably what you were looking
for.
I want this as well :). I don't quite see the risk if it's not used
with --interactive.
> [...]
>
> So no, I do not think that --recreate-merges --first-parent is a good
idea
> at all. Unless you try to do that non-interactively only, *and
disallow it
> in interactive mode*. Because the entire point of the interactive
rebase
> is to allow reordering and dropping commits, in --recreate-merges
even
> moving, introducing and dropping merge commits. The --first-parent
option
> flies in the face of this idea.
FWIW I'd be totally fine with disallowing it in --interactive. It would
be incredibly useful
e.g. with pull --rebase in merge-based workflows.
BTW what is the difference between --recreate-merges and --preserve-
merges when
--interactive is not present? I apologize if you have explained this
somewhere
else in the patch series.
>
> Ciao,
> Johannes
Thanks,
Øyvind
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-07 22:58 ` Øyvind Rønningstad@ 2018-02-07 23:31 ` Junio C Hamano
2018-02-08 12:34 ` Johannes Schindelin0 siblings, 1 reply; 412+ messages in thread
From: Junio C Hamano @ 2018-02-07 23:31 UTC (permalink / raw)
To: Øyvind Rønningstad
Cc: Johannes Schindelin, Sergey Organov, git, Jacob Keller, Johannes Sixt
Øyvind Rønningstad <ronningstad@gmail.com> writes:
>> So no, I do not think that --recreate-merges --first-parent is a good
> idea
>> at all. Unless you try to do that non-interactively only, *and
> disallow it
>> in interactive mode*.
Correct. If the original side branch has commits A, B and C, you
are rebuilding the topic to have only A and C but not B and then
recreate the merge of that rebuilt topic, then you absolutely do not
want "cherry-pick -m1" of the original merge when recreating the
merge, as that would resurrect the effect of having B. The same
argument applies if you rebuilt the topic with A and C and then a
new commit D. "cherry-pick -m1" of the original would do a wrong
thing.
When there is no such fixing up, "cherry-pick -m1" is the right
thing to do, though, so it probably makes sense to pick merges that
way when the side topic being merged consists of the same commits as
the original. I do not think that the code structure in the topic
as posted makes it impossible (or unnecessarily hard) to give an
enhancement like that in the future as a follow-up series.
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-07 23:31 ` Junio C Hamano@ 2018-02-08 12:34 ` Johannes Schindelin
2018-02-14 5:41 ` Sergey Organov0 siblings, 1 reply; 412+ messages in thread
From: Johannes Schindelin @ 2018-02-08 12:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Øyvind Rønningstad, Sergey Organov, git, Jacob Keller,
Johannes Sixt
[-- Attachment #1: Type: text/plain, Size: 4062 bytes --]
Hi Junio,
On Wed, 7 Feb 2018, Junio C Hamano wrote:
> Øyvind Rønningstad <ronningstad@gmail.com> writes:
>
> >> So no, I do not think that --recreate-merges --first-parent is a good
> > idea
> >> at all. Unless you try to do that non-interactively only, *and
> > disallow it
> >> in interactive mode*.
>
> Correct. If the original side branch has commits A, B and C, you
> are rebuilding the topic to have only A and C but not B and then
> recreate the merge of that rebuilt topic, then you absolutely do not
> want "cherry-pick -m1" of the original merge when recreating the
> merge, as that would resurrect the effect of having B. The same
> argument applies if you rebuilt the topic with A and C and then a
> new commit D. "cherry-pick -m1" of the original would do a wrong
> thing.
>
> When there is no such fixing up, "cherry-pick -m1" is the right
> thing to do, though, so it probably makes sense to pick merges that
> way when the side topic being merged consists of the same commits as
> the original.
Please note that there are a lot of conditions of "fixing up". A lot more
than just dropping, reordering or adding `pick`s.
> I do not think that the code structure in the topic as posted makes it
> impossible (or unnecessarily hard) to give an enhancement like that in
> the future as a follow-up series.
Just to give you one concrete example: when I recently rebased some
patches (no reording or dropping involved here!) and one of the picks
failed with merge conflicts, I realized that that particular commit
introduced incorrect formatting and fixed that right away (verifying that
no other commits introduced incorrect formatting, of course).
With your new cute idea to magically cherry-pick -m1, this change would
have been magically dropped from the subsequent merge commits!
And let me pick a bit on the statement "I do not think that ... makes it
impossible (or unnecessarily hard) ...": I absolutely agree. I absolutely
agree that it is not impossible or unnecessarily hard to introduce
features *that are confusing the users because they are inconsistent with
the expectations how such a command should operate*.
So the question is not so much whether we can introduce a feature that
makes no sense. Of course we can, we are decent software developers.
The question is: will this be confusing, inconsistent behavior that
violates the Principle of Least Surprise?
In that respect, introducing conditional code that would `cherry-pick -m1`
when the todo list is unchanged so far (and only then) is an absolute no,
no, no. It would be all three: confusing, inconsistent and violating the
Principle of Least Surprise.
So how about introducing support for `--recreate-merges --first-parent`
and allowing to combine it with `--interactive`? Also violating all three.
I can see how you *could* argue that `--recreate-merges --first-parent` is
a Good Thing. I really can. It would even recreate evil merges.
But in interactive mode?
Nope. It would cause all kind of pain, not the least on me, because I know
how many people ask me about `--preserve-merges --interactive` and its
confusing and inconsistent behavior that violates the Principle of Least
Surprise.
So as long as y'all don't go anywhere near "oh, let's just introduce
--recreate-merges --first-parent and *then also support it in
--interactive because we can even if it hurts the user experience", I
agree that it could be a good follow-up patch series.
Taking a step back, I have to wonder, though, why we stumble over our feet
trying to cross bridges that are one farther than the one we currently
have to cross.
By now, it should be clear why the default mode of --recreate-merges
*cannot* be --first-parent.
And before --recreate-merges is said and done, we should maybe work on
getting it said and done, rather than musing about what comes next? I, for
one, would really like to focus my time on getting *this* patch series
reviewed and included.
Thanks,
Johannes
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-08 12:34 ` Johannes Schindelin@ 2018-02-14 5:41 ` Sergey Organov0 siblings, 0 replies; 412+ messages in thread
From: Sergey Organov @ 2018-02-14 5:41 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Øyvind Rønningstad, git, Jacob Keller,
Johannes Sixt
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
[...]
> Just to give you one concrete example: when I recently rebased some
> patches (no reording or dropping involved here!) and one of the picks
> failed with merge conflicts, I realized that that particular commit
> introduced incorrect formatting and fixed that right away (verifying that
> no other commits introduced incorrect formatting, of course).
>
> With your new cute idea to magically cherry-pick -m1, this change would
> have been magically dropped from the subsequent merge commits!
You put it as if the problem you describe is unsolvable short of getting
back to your favorite blind re-merge. Do you really believe it?
I thought it's obvious that I originally meant "cherry-pick -m1" to be
an explanation facility, a proof of concept, not the final answer to all
the problems of history editing. It's a nice base for actually
approaching these problems though, unlike blind re-merge currently being
used, the latter having no potential.
The fact that bare naked "cherry-pick -m1" doesn't do what is often[1]
required in such cases neither voids the general idea of reproducing
merge-the-result, nor does it make current re-merge approach less
broken.
[1] Please take into consideration that it's _not always_ the case that
one needs a change made to a side-branch to actually propagate to the
main-line over the merge (think "merge -x ours", or something similar
but not that simple), and then it's rather the cute idea to blindly
re-merge that will wreak havoc, as in a lot of other cases.
-- Sergey
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-07 17:36 ` Johannes Schindelin
2018-02-07 22:58 ` Øyvind Rønningstad@ 2018-02-09 6:11 ` Sergey Organov
2018-02-09 7:13 ` Johannes Sixt1 sibling, 1 reply; 412+ messages in thread
From: Sergey Organov @ 2018-02-09 6:11 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Jacob Keller, Johannes Sixt
Hi,
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Wed, 7 Feb 2018, Sergey Organov wrote:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> > +--recreate-merges::
>> > + Recreate merge commits instead of flattening the history by replaying
>> > + merges. Merge conflict resolutions or manual amendments to merge
>> > + commits are not preserved.
>>
>> I wonder why you guys still hold on replaying "merge-the-operation"
>> instead of replaying "merge-the-result"?
>
> This misses the point of rebasing: you want to replay the changes.
What this comment has to do with the statement to which it's supposed to
be a reply? Sounds like topic change to me. Please clarify if it isn't.
>
>> The latter, the merge commit itself, no matter how exactly it was
>> created in the first place, is the most valuable thing git keeps about
>> the merge, and you silently drop it entirely!
>
> You miss another very crucial point.
What was the first crucial point I miss? Do you rather agree that the
point you are replying to with this is very crucial one as well?
> I don't blame you, as you certainly have not used the Git garden
> shears for years.
Thanks a lot!
> Let me explain the scenario which comes up plenty of times in my work with
> Git for Windows. We have a thicket of some 70 branches on top of git.git's
> latest release. These branches often include fixup! and squash! commits
> and even more complicated constructs that rebase cannot handle at all at
> the moment, such as reorder-before! and reorder-after! (for commits that
> really need to go into a different branch).
I sympathize, but a solution that breaks even in simple cases can't be
used reliably to solve more complex problems, sorry. Being so deep
into your problems, I think you maybe just aren't seeing forest for the
trees [1].
> Even if you do not have such a complicated setup, it is quite possible
> that you need to include a commit in your development that needs to be
> dropped before contributing your work. Think e.g. removing the `-O2` flag
> when compiling with GCC because GDB gets utterly confused with executables
> compiled with `-O2` while single-stepping. This could be an initial commit
> called `TO-DROP` or some such.
>
> And guess what happens if you drop that `pick` line in your todo list and
> then the `merge` command simply tries to re-create the original merge
> commit's changes?
>
> Exactly. The merge will become an evil merge, and will introduce that very
> much not-wanted and therefore-dropped changes.
Okay, Houston, we've had a problem here.
I'm sure you'll be able to come-up with suitable solution once you start
to think about it positively, but automatic unguided silent re-merge is
still not the right answer, for the same reason of distortion of user
changes.
As for "evil merges"... I don't want to get too far from original
subject to even start discussing this.
>> OTOH, git keeps almost no information about "merge-the-operation", so
>> it's virtually impossible to reliably replay the operation
>> automatically, and yet you try to.
>
> That is true. However, the intended use case is not to allow you to
> recreate funny merges. Its use case is to allow you to recreate
> merges.
Then it at least should behave accordingly, e.g., stop after every such
occurrence, for user assistance. As an example, see what rerere does
when it fires, even though it's much more reliable than this blind
re-merge.
But the actual problem here is that almost any merge but those made with
pure "git merge", no options, no conflicts, no edits, no nothing,
becomes "funny" and is being destroyed, sometimes in a weird way, by
silently creating something different instead of original.
> At a later stage, I might introduce support to detect `-s ours` merges,
> because they are easy to detect. But even then, it will be an opt-in.
So you are going to fix one particular case that is "easy to detect"
(and fix). Does it mean you do realize it's a problem, but fail to see that
it's _fundamental_ problem with current approach?
I think you start from the wrong end. I think that any merge should be
made reproducible first (with possible guidance from the user when
required, as usual), and then advanced features for complex history
tweaking should come, not the other way around.
I feel that any solution that fails to exactly reproduce original
history, unless it is to be actually changed, is flawed, and we will
continue to hit our heads on sharp corners like "merge -s ours", most of
which will be not that simple to detect and fix, for an unforeseeable
future.
>
>> IMHO that was severe mistake in the original --preserve-merges, and you
>> bring with you to this new --recreate-merges... It's sad.
>
> Please refrain from drawing this discussion into an emotional direction.
> That is definitely not helpful.
I don't actually blame anybody for that original implementation in the
first place. It was made based on the actual needs, and that's perfectly
fine with me, however quick-and-dirty it was. Keeping it quick-and-dirty
for years isn't that fine though.
And I do get somewhat emotional seeing this, all right. Look what you
did. We had one pet that strikes badly (with corresponding warning label
put on it in the manual), and now we have another one that would strike
as bad! I'm even afraid that I'm unfortunately not that young anymore to
get /properly/ emotional about it.
>> Even more sad as solution is already known for years:
>>
>> bc00341838a8faddcd101da9e746902994eef38a
>> Author: Johannes Sixt <j6t@kdbg.org>
>> Date: Sun Jun 16 15:50:42 2013 +0200
>>
>> rebase -p --first-parent: redo merge by cherry-picking first-parent change
>>
>> and it works like a charm.
>
> It might work for you, as you probably used --preserve-merges, and dealt
> with the fact that you could neither drop nor reorder commits.
>
> So --preserve-merges --first-parent is probably what you were looking
> for.
No. What I'm looking for is for my history to be kept as intact as
possible during rebase, unless I explicitly ask to change it, be it
--preserve-merges, or --recreate-merges, interactive or not. Is it too
much to ask for?
And no, I don't think --preserve-merges --first-parent is the right
answer either, in general, even though it did suit most of my purposes
indeed (in fact I simply patched --preserve-merges in my local git).
> Instead, --recreate-merges is all about allowing the same level of
> freedom as with regular interactive rebases, but recreating the
> original commit topology (and allowing to change it, too).
That's a very good thing and a very nice job as a whole, sure! If it
weren't I'd not even bother to raise this topic. But provided you
realize what problem "--preserve-merges --first-parent" would solve for
non-interactive use, you should realize that you have exactly the same
problem unsolved with the new --recreate-merges.
> Therefore, I think that it would be even harmful to allow
> --recreate-merges --first-parent *because it would cause evil merges*!
Once again, for me it seems you are thinking about it from the wrong
end, and this indeed won't work, but for different reasons than you
think.
[And please, stop frightening us with those "evil merge" thingy!]
> And I totally could see myself being vexed again about options that worked
> perfectly well (just like --preserve-merges) being completely messed up by
> allowing it to be combined with options *that they cannot work with* (just
> like --preserve-merges --interactive, a *huge* mistake causing so many
> annoying "bug" reports: I *never intended it that way because I knew it
> would not work as users expect*).
IMHO it's a minor problem. At least there was a warning there in the
manual, and nobody actually claimed reliable support for the feature.
> So no, I do not think that --recreate-merges --first-parent is a good idea
> at all. Unless you try to do that non-interactively only, *and disallow it
> in interactive mode*. Because the entire point of the interactive rebase
> is to allow reordering and dropping commits, in --recreate-merges even
> moving, introducing and dropping merge commits. The --first-parent option
> flies in the face of this idea.
Forget about --first-parent, please! The patch that introduced it was
only meant to show the general way of replaying a merge. Actual
suggestion is to implement proper support right with --preserve-merges,
or call it --recreate-merges, or --keep-topology, or
--no-flatten-history, whatever [2].
-- Sergey
[1] Actually I wonder how, with such a complex setup, you seem to never
step onto the problem I regularly have? Don't you ever have sub-topic
branches on top of those 70? How do you ensure none of the merges you
are rebasing are "funny", as you call them?
[2] Proper implementation of this will likely need some tweaks to the
todo list, such as "rm" command for a commit to be removed, explicit
distinguish between "merge-redo" and "merge-pick" (or even "merge-dwim")
in the list, or something like that. But to start actually thinking
about implementation, we need to agree that silent dropping user changes
is a huge problem, be it "evil merge" or something else that sounds even
more "dangerous".
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-09 6:11 ` Sergey Organov@ 2018-02-09 7:13 ` Johannes Sixt
2018-02-11 10:16 ` Jacob Keller
2018-02-12 7:38 ` Sergey Organov0 siblings, 2 replies; 412+ messages in thread
From: Johannes Sixt @ 2018-02-09 7:13 UTC (permalink / raw)
To: Sergey Organov; +Cc: Johannes Schindelin, git, Junio C Hamano, Jacob Keller
Am 09.02.2018 um 07:11 schrieb Sergey Organov:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Let me explain the scenario which comes up plenty of times in my work with
>> Git for Windows. We have a thicket of some 70 branches on top of git.git's
>> latest release. These branches often include fixup! and squash! commits
>> and even more complicated constructs that rebase cannot handle at all at
>> the moment, such as reorder-before! and reorder-after! (for commits that
>> really need to go into a different branch).
>
> I sympathize, but a solution that breaks even in simple cases can't be
> used reliably to solve more complex problems, sorry. Being so deep
> into your problems, I think you maybe just aren't seeing forest for the
> trees [1].
Hold your horses! Dscho has a point here. --preserve-merges
--first-parent works only as long as you don't tamper with the side
branches. If you make changes in the side branches during the same
rebase operation, this --first-parent mode would undo that change. (And,
yes, its result would be called an "evil merge", and that scary name
_should_ frighten you!)
-- Hannes
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-09 7:13 ` Johannes Sixt@ 2018-02-11 10:16 ` Jacob Keller
2018-02-12 7:38 ` Sergey Organov1 sibling, 0 replies; 412+ messages in thread
From: Jacob Keller @ 2018-02-11 10:16 UTC (permalink / raw)
To: Johannes Sixt
Cc: Sergey Organov, Johannes Schindelin, Git mailing list, Junio C Hamano
On Thu, Feb 8, 2018 at 11:13 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 09.02.2018 um 07:11 schrieb Sergey Organov:
>>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>>
>>> Let me explain the scenario which comes up plenty of times in my work
>>> with
>>> Git for Windows. We have a thicket of some 70 branches on top of
>>> git.git's
>>> latest release. These branches often include fixup! and squash! commits
>>> and even more complicated constructs that rebase cannot handle at all at
>>> the moment, such as reorder-before! and reorder-after! (for commits that
>>> really need to go into a different branch).
>>
>>
>> I sympathize, but a solution that breaks even in simple cases can't be
>> used reliably to solve more complex problems, sorry. Being so deep
>> into your problems, I think you maybe just aren't seeing forest for the
>> trees [1].
>
>
> Hold your horses! Dscho has a point here. --preserve-merges --first-parent
> works only as long as you don't tamper with the side branches. If you make
> changes in the side branches during the same rebase operation, this
> --first-parent mode would undo that change. (And, yes, its result would be
> called an "evil merge", and that scary name _should_ frighten you!)
>
> -- Hannes
This is the reason I agree with Johannes, in regards to why
recreate-merges approach is correct.
Yes, an ideal system would be one which correctly, automatically
re-creates the merge *as if* a human had re-merged the two newly
re-created side branches, and preserves any changes in the result of
the merge, such as cases we call "evil merges" which includes
necessary changes to resolve conflicts properly.
However, I would state that such a system, in order to cause the least
surprise to a user must be correct against arbitrary removal, reorder,
and addition of new commits on both the main and topic side branches
for which we are re-creating the merges.
This is problematic, because something like how --preserve-merges
--first-parent does not work under this case.
As a user of the tool, I may be biased because I already read and
understand how recreate-merges is expected to work, but it makes sense
to me that the re-creation of the merge merely re-does the merge and
any modfications in the original would have to be carried over.
I don't know what process we could use to essentially move the changes
from the original merge into the new copy. What ever solution we have
would need to have a coherent user interface and be presentable in
some manner.
One way to think about the contents we're wanting to keep, rather than
the full tree result of the merge which we had before, what we
actually want to keep in some sense is the resulting "diff" as shown
by something like the condensed --combined output. This is obviously
not really a diff that we can apply.
And even if we could apply it, since the merge is occurring, we can't
exactly use 3-way merge conflict in order to actually apply the old
changes into the new merged setup? Could something like rerere logic
work here to track what was done and then re-apply it to the new merge
we create? And once we apply it, we need to be able to handle any
conflicts that occur because of deleting, adding, or re-ordering
commits on the branches we're merging... so in some sense we could
have "conflicts of conflicts" which is a scenario that I don't yet
have a good handle on how this would be presented to the user.
Thanks,
Jake
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-09 7:13 ` Johannes Sixt
2018-02-11 10:16 ` Jacob Keller@ 2018-02-12 7:38 ` Sergey Organov1 sibling, 0 replies; 412+ messages in thread
From: Sergey Organov @ 2018-02-12 7:38 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Junio C Hamano, Jacob Keller
Johannes Sixt <j6t@kdbg.org> writes:
> Am 09.02.2018 um 07:11 schrieb Sergey Organov:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>> Let me explain the scenario which comes up plenty of times in my work with
>>> Git for Windows. We have a thicket of some 70 branches on top of git.git's
>>> latest release. These branches often include fixup! and squash! commits
>>> and even more complicated constructs that rebase cannot handle at all at
>>> the moment, such as reorder-before! and reorder-after! (for commits that
>>> really need to go into a different branch).
>>
>> I sympathize, but a solution that breaks even in simple cases can't be
>> used reliably to solve more complex problems, sorry. Being so deep
>> into your problems, I think you maybe just aren't seeing forest for the
>> trees [1].
>
> Hold your horses! Dscho has a point here. --preserve-merges
> --first-parent works only as long as you don't tamper with the side
> branches. If you make changes in the side branches during the same
> rebase operation, this --first-parent mode would undo that change.
He has a point indeed, but it must not be used as an excuse to silently
damage user data, as if there are no other options!
Simple --first-parent won't always fit, it's obvious. I used
--first-parent patch as mere illustration of concept, it's rather
"rebase [-i] --keep-the-f*g-shape" itself that should behave. There
should be no need for actual --first-parent that only fits
no-manual-editing use-cases.
Look at it as if it's a scale where --first-parent is on one side, and
"blind re-merge" is on the other. The right answer(s) lie somewhere
in-between, but I think they are much closer to --first-parent than they
are to "blind re-merge".
> (And, yes, its result would be called an "evil merge", and that scary
> name _should_ frighten you!)
(It won't always be "evil merge", and it still doesn't frighten even if
it will, provided git stops making them more evil then they actually
deserve, and it isn't an excuse to silently distort user data anyway!)
-- Sergey
[1] The "--first-parent" here would rather keep that change from
propagation to the main-line, not undo it, and sometimes it's even the
right thing to do ("-x ours" for the original merge being one example).
Frequently though it is needed on main-line indeed, and there should be
a way to tell git to propagate the change to the main-line, but even
then automatic blind unattended re-merge is wrong answer and I'm sure
git can be made to do better than that.
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-01-18 15:35 ` [PATCH 5/8] rebase: introduce the --recreate-merges option Johannes Schindelin
` (2 preceding siblings ...)
2018-02-07 6:16 ` Sergey Organov@ 2018-02-09 6:50 ` Sergey Organov
2018-02-10 23:06 ` Johannes Schindelin3 siblings, 1 reply; 412+ messages in thread
From: Sergey Organov @ 2018-02-09 6:50 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Jacob Keller
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
[...]
> With this patch, the goodness of the Git garden shears comes to `git
> rebase -i` itself. Passing the `--recreate-merges` option will generate
> a todo list that can be understood readily, and where it is obvious
> how to reorder commits. New branches can be introduced by inserting
> `label` commands and calling `merge - <label> <oneline>`. And once this
> mode has become stable and universally accepted, we can deprecate the
> design mistake that was `--preserve-merges`.
This doesn't explain why you introduced this new --recreate-merges. Why
didn't you rather fix --preserve-merges to generate and use new todo
list format?
It doesn't seem likely that todo list created by one Git version is to
be ever used by another, right? Is there some hidden reason here? Some
tools outside of Git that use old todo list format, maybe?
Then, if new option indeed required, please look at the resulting manual:
--recreate-merges::
Recreate merge commits instead of flattening the history by replaying
merges. Merge conflict resolutions or manual amendments to merge
commits are not preserved.
-p::
--preserve-merges::
Recreate merge commits instead of flattening the history by replaying
commits a merge commit introduces. Merge conflict resolutions or manual
amendments to merge commits are not preserved.
Don't you think more explanations are needed there in the manual on
why do we have 2 separate options with almost the same yet subtly
different description? Is this subtle difference even important? How?
I also have trouble making sense of "Recreate merge commits instead of
flattening the history by replaying merges." Is it "<Recreate merge
commits by replaying merges> instead of <flattening the history>" or is it
rather "<Recreate merge commits> instead of <flattening the history by
replaying merges>?
-- Sergey
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-09 6:50 ` Sergey Organov@ 2018-02-10 23:06 ` Johannes Schindelin
2018-02-12 4:58 ` Sergey Organov
2018-02-12 5:22 ` Sergey Organov0 siblings, 2 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-02-10 23:06 UTC (permalink / raw)
To: Sergey Organov; +Cc: git, Junio C Hamano, Jacob Keller
Hi Sergey,
On Fri, 9 Feb 2018, Sergey Organov wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> [...]
>
> > With this patch, the goodness of the Git garden shears comes to `git
> > rebase -i` itself. Passing the `--recreate-merges` option will generate
> > a todo list that can be understood readily, and where it is obvious
> > how to reorder commits. New branches can be introduced by inserting
> > `label` commands and calling `merge - <label> <oneline>`. And once this
> > mode has become stable and universally accepted, we can deprecate the
> > design mistake that was `--preserve-merges`.
>
> This doesn't explain why you introduced this new --recreate-merges. Why
> didn't you rather fix --preserve-merges to generate and use new todo
> list format?
Because that would of course break existing users of --preserve-merges.
So why not --preserve-merges=v2? Because that would force me to maintain
--preserve-merges forever. And I don't want to.
> It doesn't seem likely that todo list created by one Git version is to
> be ever used by another, right?
No. But by scripts based on `git rebase -p`.
> Is there some hidden reason here? Some tools outside of Git that use old
> todo list format, maybe?
Exactly.
I did mention such a tool: the Git garden shears:
https://github.com/git-for-windows/build-extra/blob/master/shears.sh
Have a look at it. It will inform the discussion.
> Then, if new option indeed required, please look at the resulting manual:
>
> --recreate-merges::
> Recreate merge commits instead of flattening the history by replaying
> merges. Merge conflict resolutions or manual amendments to merge
> commits are not preserved.
>
> -p::
> --preserve-merges::
> Recreate merge commits instead of flattening the history by replaying
> commits a merge commit introduces. Merge conflict resolutions or manual
> amendments to merge commits are not preserved.
As I stated in the cover letter, there are more patches lined up after
this patch series.
Have a look at https://github.com/git/git/pull/447, especially the latest
commit in there which is an early version of the deprecation I intend to
bring about.
Also, please refrain from saying things like... "Don't you think ..."
If you don't like the wording, I wold much more appreciate it if a better
alternative was suggested.
> Don't you think more explanations are needed there in the manual on
> why do we have 2 separate options with almost the same yet subtly
> different description? Is this subtle difference even important? How?
>
> I also have trouble making sense of "Recreate merge commits instead of
> flattening the history by replaying merges." Is it "<Recreate merge
> commits by replaying merges> instead of <flattening the history>" or is it
> rather "<Recreate merge commits> instead of <flattening the history by
> replaying merges>?
The documentation of the --recreate-merges option is not meant to explain
the difference to --preserve-merges. It is meant to explain the difference
to regular `git rebase -i`, which flattens the commit history into a
single branch without merge commits (in fact, all merge commits are simply
ignored).
And I would rather not start to describe the difference between
--recreate-merges and --preserve-merges because I want to deprecate the
latter, and describing the difference as I get the sense is your wish
would simply mean more work because it would have to be added and then
removed again.
If you still think it would be a good idea to describe the difference
between --recreate-merges and --preserve-merges, then please provide a
suggestion, preferably in the form of a patch, that adds appropriate
paragraphs to *both* options' documentation, so that your proposal can be
discussed properly.
Ciao,
Johannes
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-12 4:58 ` Sergey Organov@ 2018-02-12 20:21 ` Johannes Schindelin
2018-02-13 6:44 ` Sergey Organov0 siblings, 1 reply; 412+ messages in thread
From: Johannes Schindelin @ 2018-02-12 20:21 UTC (permalink / raw)
To: Sergey Organov; +Cc: git, Junio C Hamano, Jacob Keller
Hi Sergey,
On Mon, 12 Feb 2018, Sergey Organov wrote:
> Thanks for explanations, and could you please answer this one:
>
> [...]
>
> >> I also have trouble making sense of "Recreate merge commits instead of
> >> flattening the history by replaying merges." Is it "<Recreate merge
> >> commits by replaying merges> instead of <flattening the history>" or is it
> >> rather "<Recreate merge commits> instead of <flattening the history by
> >> replaying merges>?
I thought I had answered that one.
Flattening the history is what happens in regular rebase (i.e. without
--recreate-merges and without --preserve-merges).
The idea to recreate merges is of course to *not* flatten the history.
Maybe there should have been a comma after "history" to clarify what the
sentence means.
The wording is poor either way, but you are also not a native speaker so
we have to rely on, say, Eric to help us out here.
Ciao,
Johannes
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-12 20:21 ` Johannes Schindelin@ 2018-02-13 6:44 ` Sergey Organov
2018-02-15 1:08 ` Johannes Schindelin0 siblings, 1 reply; 412+ messages in thread
From: Sergey Organov @ 2018-02-13 6:44 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Jacob Keller
Hi Johannes,
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Sergey,
>
> On Mon, 12 Feb 2018, Sergey Organov wrote:
>
>> Thanks for explanations, and could you please answer this one:
>>
>> [...]
>>
>> >> I also have trouble making sense of "Recreate merge commits instead of
>> >> flattening the history by replaying merges." Is it "<Recreate merge
>> >> commits by replaying merges> instead of <flattening the history>" or is it
>> >> rather "<Recreate merge commits> instead of <flattening the history by
>> >> replaying merges>?
>
> I thought I had answered that one.
No, not really, but now you did, please see below.
>
> Flattening the history is what happens in regular rebase (i.e. without
> --recreate-merges and without --preserve-merges).
>
> The idea to recreate merges is of course to *not* flatten the history.
Sure. Never supposed it is.
> Maybe there should have been a comma after "history" to clarify what the
> sentence means.
That's the actual answer to my question, but it in turn raises another
one: why did you change wording of --preserve-merges description for
this new option?
> The wording is poor either way, but you are also not a native speaker so
> we have to rely on, say, Eric to help us out here.
Likely, but why didn't you keep original wording from --preserve-merges?
Do you feel it's somehow poor either?
Anyway, please also refer to wording suggestion in the another (lengthy)
answer in this thread.
-- Sergey
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-15 4:28 ` Sergey Organov@ 2018-02-15 16:51 ` Johannes Schindelin0 siblings, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-02-15 16:51 UTC (permalink / raw)
To: Sergey Organov; +Cc: git, Junio C Hamano, Jacob Keller
Hi,
On Thu, 15 Feb 2018, Sergey Organov wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Tue, 13 Feb 2018, Sergey Organov wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >>
> >> > The wording is poor either way, but you are also not a native speaker so
> >> > we have to rely on, say, Eric to help us out here.
> >>
> >> Likely, but why didn't you keep original wording from --preserve-merges?
> >> Do you feel it's somehow poor either?
> >
> > Yes, I felt it is poor, especially when --recreate-merges is present, that
> > is indeed why I changed it.
>
> So, how about this (yeah, I noticed the option now got arguments, but
> please, tweak this to the new implementation yourself):
>
> --recreate-merges::
> Recreate merge commits instead of flattening the history. Merge
> conflict resolutions or manual amendments to merge commits are
> not preserved.
>
> -p::
> --preserve-merges::
> (deprecated) This option is similar to --recreate-merges. It has
> no proper support for interactive mode and thus is deprecated.
> Use '--recreate-merges' instead.
I still don't like either.
I want something different there: descriptions that are a bit more
self-contained, and only describe the differences to -i or
--preserve-merges in a second paragraph.
Don't worry about it, though, I don't think you or me are capable of a
good explanation. I will ask some native speakers I trust.
Ciao,
Johannes
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-10 23:06 ` Johannes Schindelin
2018-02-12 4:58 ` Sergey Organov@ 2018-02-12 5:22 ` Sergey Organov
2018-02-12 20:39 ` Johannes Schindelin1 sibling, 1 reply; 412+ messages in thread
From: Sergey Organov @ 2018-02-12 5:22 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Jacob Keller
Hi Johannes,
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Sergey,
>
> On Fri, 9 Feb 2018, Sergey Organov wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>> [...]
>>
>> > With this patch, the goodness of the Git garden shears comes to `git
>> > rebase -i` itself. Passing the `--recreate-merges` option will generate
>> > a todo list that can be understood readily, and where it is obvious
>> > how to reorder commits. New branches can be introduced by inserting
>> > `label` commands and calling `merge - <label> <oneline>`. And once this
>> > mode has become stable and universally accepted, we can deprecate the
>> > design mistake that was `--preserve-merges`.
>>
>> This doesn't explain why you introduced this new --recreate-merges. Why
>> didn't you rather fix --preserve-merges to generate and use new todo
>> list format?
>
> Because that would of course break existing users of
> --preserve-merges.
How exactly? Doesn't "--recreate-merges" produce the same result as
"--preserve-merges" if run non-interactively?
> So why not --preserve-merges=v2? Because that would force me to maintain
> --preserve-merges forever. And I don't want to.
>
>> It doesn't seem likely that todo list created by one Git version is to
>> be ever used by another, right?
>
> No. But by scripts based on `git rebase -p`.
>
>> Is there some hidden reason here? Some tools outside of Git that use old
>> todo list format, maybe?
>
> Exactly.
>
> I did mention such a tool: the Git garden shears:
>
> https://github.com/git-for-windows/build-extra/blob/master/shears.sh
>
> Have a look at it. It will inform the discussion.
I've searched for "-p" in the script, but didn't find positives for
either "-p" or "--preserve-merges". How it would break if it doesn't use
them? What am I missing?
>
>> Then, if new option indeed required, please look at the resulting manual:
>>
>> --recreate-merges::
>> Recreate merge commits instead of flattening the history by replaying
>> merges. Merge conflict resolutions or manual amendments to merge
>> commits are not preserved.
>>
>> -p::
>> --preserve-merges::
>> Recreate merge commits instead of flattening the history by replaying
>> commits a merge commit introduces. Merge conflict resolutions or manual
>> amendments to merge commits are not preserved.
>
> As I stated in the cover letter, there are more patches lined up after
> this patch series.
Good, but I thought this one should better be self-consistent anyway.
What if those that come later aren't included?
>
> Have a look at https://github.com/git/git/pull/447, especially the latest
> commit in there which is an early version of the deprecation I intend to
> bring about.
You shouldn't want a deprecation at all should you have re-used
--preserve-merges in the first place, and I still don't see why you
haven't.
>
> Also, please refrain from saying things like... "Don't you think ..."
>
> If you don't like the wording, I wold much more appreciate it if a better
> alternative was suggested.
Sorry, but how can I suggest one if I don't understand what you are
doing here in the first place? That's why I ask you.
>
>> Don't you think more explanations are needed there in the manual on
>> why do we have 2 separate options with almost the same yet subtly
>> different description? Is this subtle difference even important? How?
>>
>> I also have trouble making sense of "Recreate merge commits instead of
>> flattening the history by replaying merges." Is it "<Recreate merge
>> commits by replaying merges> instead of <flattening the history>" or is it
>> rather "<Recreate merge commits> instead of <flattening the history by
>> replaying merges>?
>
> The documentation of the --recreate-merges option is not meant to explain
> the difference to --preserve-merges. It is meant to explain the difference
> to regular `git rebase -i`, which flattens the commit history into a
> single branch without merge commits (in fact, all merge commits are simply
> ignored).
Yeah, that's obvious, but the point is that resulting manual is ended
up being confusing.
> And I would rather not start to describe the difference between
> --recreate-merges and --preserve-merges because I want to deprecate the
> latter, and describing the difference as I get the sense is your wish
> would simply mean more work because it would have to be added and then
> removed again.
I suspect you actually didn't need those new option in the first place,
and that's the core reason of these troubles.
-- Sergey
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-12 5:22 ` Sergey Organov@ 2018-02-12 20:39 ` Johannes Schindelin
2018-02-13 4:39 ` Jacob Keller
2018-02-13 6:43 ` Sergey Organov0 siblings, 2 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-02-12 20:39 UTC (permalink / raw)
To: Sergey Organov; +Cc: git, Junio C Hamano, Jacob Keller
Hi Sergey,
On Mon, 12 Feb 2018, Sergey Organov wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > On Fri, 9 Feb 2018, Sergey Organov wrote:
> >
> >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >>
> >> [...]
> >>
> >> > With this patch, the goodness of the Git garden shears comes to `git
> >> > rebase -i` itself. Passing the `--recreate-merges` option will generate
> >> > a todo list that can be understood readily, and where it is obvious
> >> > how to reorder commits. New branches can be introduced by inserting
> >> > `label` commands and calling `merge - <label> <oneline>`. And once this
> >> > mode has become stable and universally accepted, we can deprecate the
> >> > design mistake that was `--preserve-merges`.
> >>
> >> This doesn't explain why you introduced this new --recreate-merges. Why
> >> didn't you rather fix --preserve-merges to generate and use new todo
> >> list format?
> >
> > Because that would of course break existing users of
> > --preserve-merges.
>
> How exactly?
Power users of interactive rebase use scripting to augment Git's
functionality. One particularly powerful trick is to override
GIT_SEQUENCER_EDITOR with an invocation of such a script, to perform
automated edits. Such a script breaks when we change the format of the
content to edit. If we change the format of the todo list generated in
--preserve-merges mode, that is exactly what happens. We break existing
users.
BTW it seems that you did not really read my previous reply carefully
because I referenced such a use case: the Git garden shears. They do
override the sequencer editor, and while they do not exactly edit the todo
list (they simply through the generated one away), they generate a new
todo list and would break if that format changes. Of course, the shears do
not use the --preserve-merges mode, but from just reading about the way
how the Git garden shears work, it is quite obvious how similar users of
--preserve-merges are likely to exist?
> Doesn't "--recreate-merges" produce the same result as
> "--preserve-merges" if run non-interactively?
The final result of a rebase where you do not edit the todo list? Should
be identical, indeed.
But that is the most boring, most uninteresting, and least important use
case. So we might just as well forget about it when we focus on keeping
Git's usage stable.
> > So why not --preserve-merges=v2? Because that would force me to
> > maintain --preserve-merges forever. And I don't want to.
> >
> >> It doesn't seem likely that todo list created by one Git version is
> >> to be ever used by another, right?
> >
> > No. But by scripts based on `git rebase -p`.
> >
> >> Is there some hidden reason here? Some tools outside of Git that use
> >> old todo list format, maybe?
> >
> > Exactly.
> >
> > I did mention such a tool: the Git garden shears:
> >
> > https://github.com/git-for-windows/build-extra/blob/master/shears.sh
> >
> > Have a look at it. It will inform the discussion.
>
> I've searched for "-p" in the script, but didn't find positives for
> either "-p" or "--preserve-merges". How it would break if it doesn't use
> them? What am I missing?
*This* particular script does not use -p.
But it is not *this* particular script that I do not want to break! It is
*all* scripts that use interactive rebase! Don't you also care about not
breaking existing users?
> >> Then, if new option indeed required, please look at the resulting manual:
> >>
> >> --recreate-merges::
> >> Recreate merge commits instead of flattening the history by replaying
> >> merges. Merge conflict resolutions or manual amendments to merge
> >> commits are not preserved.
> >>
> >> -p::
> >> --preserve-merges::
> >> Recreate merge commits instead of flattening the history by replaying
> >> commits a merge commit introduces. Merge conflict resolutions or manual
> >> amendments to merge commits are not preserved.
> >
> > As I stated in the cover letter, there are more patches lined up after
> > this patch series.
>
> Good, but I thought this one should better be self-consistent anyway.
> What if those that come later aren't included?
Right, let's just rip apart the partial progress because the latter
patches might not make it in?
I cannot work on that basis, and I also do not want to work on that basis.
If you do not like how the documentation is worded, fine, suggest a better
alternative.
> > Have a look at https://github.com/git/git/pull/447, especially the
> > latest commit in there which is an early version of the deprecation I
> > intend to bring about.
>
> You shouldn't want a deprecation at all should you have re-used
> --preserve-merges in the first place, and I still don't see why you
> haven't.
Keep repeating it, and it won't become truer.
If you break formats, you break scripts. Git has *so* many users, there
are very likely some who script *every* part of it.
We simply cannot do that.
What we can is deprecate designs which we learned on the way were not only
incomplete from the get-go, but bad overall and hard (or impossible) to
fix. Like --preserve-merges.
Or for that matter like the design you proposed, to use --first-parent for
--recreate-merges. Or to use --first-parent for some --recreate-merges,
surprising users in very bad ways when it is not used (or when it is
used). I get the impression that you still think it would be a good idea,
even if it should be obvious that it is not.
> > Also, please refrain from saying things like... "Don't you think ..."
> >
> > If you don't like the wording, I wold much more appreciate it if a better
> > alternative was suggested.
>
> Sorry, but how can I suggest one if I don't understand what you are
> doing here in the first place? That's why I ask you.
There are ways to put the person you ask on trial. And there are ways to
genuinely show interest and seek education.
I am a really poor example how to communicate properly, of course, so
don't try to learn from me. I am trying myself to learn better ways to
express what I mean clearly, and to express it in a direct yet kind
manner.
> >> Don't you think more explanations are needed there in the manual on
> >> why do we have 2 separate options with almost the same yet subtly
> >> different description? Is this subtle difference even important? How?
> >>
> >> I also have trouble making sense of "Recreate merge commits instead of
> >> flattening the history by replaying merges." Is it "<Recreate merge
> >> commits by replaying merges> instead of <flattening the history>" or is it
> >> rather "<Recreate merge commits> instead of <flattening the history by
> >> replaying merges>?
> >
> > The documentation of the --recreate-merges option is not meant to explain
> > the difference to --preserve-merges. It is meant to explain the difference
> > to regular `git rebase -i`, which flattens the commit history into a
> > single branch without merge commits (in fact, all merge commits are simply
> > ignored).
>
> Yeah, that's obvious, but the point is that resulting manual is ended
> up being confusing.
Again, just saying something is bad, is bad. Saying something leaves room
for improvement and then suggesting how to improve it, is good.
> > And I would rather not start to describe the difference between
> > --recreate-merges and --preserve-merges because I want to deprecate the
> > latter, and describing the difference as I get the sense is your wish
> > would simply mean more work because it would have to be added and then
> > removed again.
>
> I suspect you actually didn't need those new option in the first place,
> and that's the core reason of these troubles.
Are you suspecting that I, myself, do not use --recreate-merges?
If so, please read the cover letter again, in particular the part where I
describe how this entire series of patch series arose from the Git garden
shears, which I invented myself to help with maintaining Git for Windows,
and which I use for five years now. This should help disperse that
suspicion rather quickly: the intent of --recreate-merges is to allow me
to simplify the shears by quite a bit, and maybe eventually even get rid
of the script altogether (if I ever manage to convince myself that the
concept of a merging-rebase should be official enough to enter core Git).
I am a heavy user of --recreate-merges, even if it does not really exist
yet. I have five years of experience with it, which is the reason why I am
so confident about its design, and why I can tell you a lot about typical
use cases and common pitfalls, and where the original design had to be
adjusted.
Ciao,
Johannes
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-12 20:39 ` Johannes Schindelin@ 2018-02-13 4:39 ` Jacob Keller
2018-02-13 7:15 ` Sergey Organov
2018-02-13 6:43 ` Sergey Organov1 sibling, 1 reply; 412+ messages in thread
From: Jacob Keller @ 2018-02-13 4:39 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Sergey Organov, Git mailing list, Junio C Hamano
On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Sergey,
>
> On Mon, 12 Feb 2018, Sergey Organov wrote:
>> > Have a look at https://github.com/git/git/pull/447, especially the
>> > latest commit in there which is an early version of the deprecation I
>> > intend to bring about.
>>
>> You shouldn't want a deprecation at all should you have re-used
>> --preserve-merges in the first place, and I still don't see why you
>> haven't.
>
> Keep repeating it, and it won't become truer.
>
> If you break formats, you break scripts. Git has *so* many users, there
> are very likely some who script *every* part of it.
>
> We simply cannot do that.
>
> What we can is deprecate designs which we learned on the way were not only
> incomplete from the get-go, but bad overall and hard (or impossible) to
> fix. Like --preserve-merges.
>
> Or for that matter like the design you proposed, to use --first-parent for
> --recreate-merges. Or to use --first-parent for some --recreate-merges,
> surprising users in very bad ways when it is not used (or when it is
> used). I get the impression that you still think it would be a good idea,
> even if it should be obvious that it is not.
If we consider the addition of new todo list elements as "user
breaking", then yes this change would be user-script breaking.
Since we did not originally spell out that todo-list items are subject
to enhancement by addition of operations in the future, scripts are
likely not designed to allow addition of new elements.
Thus, adding recreate-merges, and deprecating preserve-merges, seems
to me to be the correct action to take here.
One could argue that users should have expected new todo list elements
to be added in the future and thus design their scripts to cope with
such a thing. If you can convincingly argue this, then I don't
necessarily see it as a complete user breaking change to fix
preserve-merges in order to allow it to handle re-ordering properly..
I think I lean towards agreeing with Johannes, and that adding
recreate-merges and removing preserve-merges is the better solution.
Thanks,
Jake
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-13 4:39 ` Jacob Keller@ 2018-02-13 7:15 ` Sergey Organov
2018-02-14 1:35 ` Jacob Keller0 siblings, 1 reply; 412+ messages in thread
From: Sergey Organov @ 2018-02-13 7:15 UTC (permalink / raw)
To: Jacob Keller; +Cc: Johannes Schindelin, Git mailing list, Junio C Hamano
Hi Jake,
Jacob Keller <jacob.keller@gmail.com> writes:
> On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Hi Sergey,
>>
>> On Mon, 12 Feb 2018, Sergey Organov wrote:
>>> > Have a look at https://github.com/git/git/pull/447, especially the
>>> > latest commit in there which is an early version of the deprecation I
>>> > intend to bring about.
>>>
>>> You shouldn't want a deprecation at all should you have re-used
>>> --preserve-merges in the first place, and I still don't see why you
>>> haven't.
>>
>> Keep repeating it, and it won't become truer.
>>
>> If you break formats, you break scripts. Git has *so* many users, there
>> are very likely some who script *every* part of it.
>>
>> We simply cannot do that.
>>
>> What we can is deprecate designs which we learned on the way were not only
>> incomplete from the get-go, but bad overall and hard (or impossible) to
>> fix. Like --preserve-merges.
>>
>> Or for that matter like the design you proposed, to use --first-parent for
>> --recreate-merges. Or to use --first-parent for some --recreate-merges,
>> surprising users in very bad ways when it is not used (or when it is
>> used). I get the impression that you still think it would be a good idea,
>> even if it should be obvious that it is not.
>
> If we consider the addition of new todo list elements as "user
> breaking", then yes this change would be user-script breaking.
It _is_ user script breaking, provided such script exists. Has anybody
actually seen one? Not that it's wrong to be extra-cautious about it,
just curios. Note that to be actually affected, such a script must
invoke "git rebase -p" _command_ and then tweak its todo output to
produce outcome.
> Since we did not originally spell out that todo-list items are subject
> to enhancement by addition of operations in the future, scripts are
> likely not designed to allow addition of new elements.
Out of curiosity, are you going to spell it now, for the new todo
format?
> Thus, adding recreate-merges, and deprecating preserve-merges, seems
> to me to be the correct action to take here.
Yes, sure, provided there is actual breakage, or at least informed
suspicion there is one.
> One could argue that users should have expected new todo list elements
> to be added in the future and thus design their scripts to cope with
> such a thing. If you can convincingly argue this, then I don't
> necessarily see it as a complete user breaking change to fix
> preserve-merges in order to allow it to handle re-ordering properly..
I'd not argue this way myself. If there are out-of-git-tree non-human
users that accept and tweak todo _generated_ by current "git rebase -p"
_command_, I also vote for a new option.
> I think I lean towards agreeing with Johannes, and that adding
> recreate-merges and removing preserve-merges is the better solution.
On these grounds it is, no objections.
-- Sergey
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-13 7:15 ` Sergey Organov@ 2018-02-14 1:35 ` Jacob Keller
2018-02-15 1:14 ` Johannes Schindelin0 siblings, 1 reply; 412+ messages in thread
From: Jacob Keller @ 2018-02-14 1:35 UTC (permalink / raw)
To: Sergey Organov; +Cc: Johannes Schindelin, Git mailing list, Junio C Hamano
On Mon, Feb 12, 2018 at 11:15 PM, Sergey Organov <sorganov@gmail.com> wrote:
> Hi Jake,
>
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>> Hi Sergey,
>>>
>>> On Mon, 12 Feb 2018, Sergey Organov wrote:
>>>> > Have a look at https://github.com/git/git/pull/447, especially the
>>>> > latest commit in there which is an early version of the deprecation I
>>>> > intend to bring about.
>>>>
>>>> You shouldn't want a deprecation at all should you have re-used
>>>> --preserve-merges in the first place, and I still don't see why you
>>>> haven't.
>>>
>>> Keep repeating it, and it won't become truer.
>>>
>>> If you break formats, you break scripts. Git has *so* many users, there
>>> are very likely some who script *every* part of it.
>>>
>>> We simply cannot do that.
>>>
>>> What we can is deprecate designs which we learned on the way were not only
>>> incomplete from the get-go, but bad overall and hard (or impossible) to
>>> fix. Like --preserve-merges.
>>>
>>> Or for that matter like the design you proposed, to use --first-parent for
>>> --recreate-merges. Or to use --first-parent for some --recreate-merges,
>>> surprising users in very bad ways when it is not used (or when it is
>>> used). I get the impression that you still think it would be a good idea,
>>> even if it should be obvious that it is not.
>>
>> If we consider the addition of new todo list elements as "user
>> breaking", then yes this change would be user-script breaking.
>
> It _is_ user script breaking, provided such script exists. Has anybody
> actually seen one? Not that it's wrong to be extra-cautious about it,
> just curios. Note that to be actually affected, such a script must
> invoke "git rebase -p" _command_ and then tweak its todo output to
> produce outcome.
>
>> Since we did not originally spell out that todo-list items are subject
>> to enhancement by addition of operations in the future, scripts are
>> likely not designed to allow addition of new elements.
>
> Out of curiosity, are you going to spell it now, for the new todo
> format?
>
>> Thus, adding recreate-merges, and deprecating preserve-merges, seems
>> to me to be the correct action to take here.
>
> Yes, sure, provided there is actual breakage, or at least informed
> suspicion there is one.
>
>> One could argue that users should have expected new todo list elements
>> to be added in the future and thus design their scripts to cope with
>> such a thing. If you can convincingly argue this, then I don't
>> necessarily see it as a complete user breaking change to fix
>> preserve-merges in order to allow it to handle re-ordering properly..
>
> I'd not argue this way myself. If there are out-of-git-tree non-human
> users that accept and tweak todo _generated_ by current "git rebase -p"
> _command_, I also vote for a new option.
>
To be fair, I have not seen anything that actually reads the todo list
and tweaks it in such a manner. The closest example is the git garden
shears script, which simply replaces the todo list.
It's certainly *possible* that such a script would exist though,
Thanks,
Jake
>> I think I lean towards agreeing with Johannes, and that adding
>> recreate-merges and removing preserve-merges is the better solution.
>
> On these grounds it is, no objections.
>
> -- Sergey
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-14 1:35 ` Jacob Keller@ 2018-02-15 1:14 ` Johannes Schindelin
2018-02-15 4:35 ` Sergey Organov0 siblings, 1 reply; 412+ messages in thread
From: Johannes Schindelin @ 2018-02-15 1:14 UTC (permalink / raw)
To: Jacob Keller; +Cc: Sergey Organov, Git mailing list, Junio C Hamano
Hi Jake,
On Tue, 13 Feb 2018, Jacob Keller wrote:
> On Mon, Feb 12, 2018 at 11:15 PM, Sergey Organov <sorganov@gmail.com> wrote:
> >
> > Jacob Keller <jacob.keller@gmail.com> writes:
> >
> >> On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
> >> <Johannes.Schindelin@gmx.de> wrote:
> >>>
> >>> On Mon, 12 Feb 2018, Sergey Organov wrote:
> >>>> > Have a look at https://github.com/git/git/pull/447, especially the
> >>>> > latest commit in there which is an early version of the deprecation I
> >>>> > intend to bring about.
> >>>>
> >>>> You shouldn't want a deprecation at all should you have re-used
> >>>> --preserve-merges in the first place, and I still don't see why you
> >>>> haven't.
> >>>
> >>> Keep repeating it, and it won't become truer.
> >>>
> >>> If you break formats, you break scripts. Git has *so* many users, there
> >>> are very likely some who script *every* part of it.
> >>>
> >>> We simply cannot do that.
> >>>
> >>> What we can is deprecate designs which we learned on the way were not only
> >>> incomplete from the get-go, but bad overall and hard (or impossible) to
> >>> fix. Like --preserve-merges.
> >>>
> >>> Or for that matter like the design you proposed, to use --first-parent for
> >>> --recreate-merges. Or to use --first-parent for some --recreate-merges,
> >>> surprising users in very bad ways when it is not used (or when it is
> >>> used). I get the impression that you still think it would be a good idea,
> >>> even if it should be obvious that it is not.
> >>
> >> If we consider the addition of new todo list elements as "user
> >> breaking", then yes this change would be user-script breaking.
> >
> > It _is_ user script breaking, provided such script exists. Has anybody
> > actually seen one? Not that it's wrong to be extra-cautious about it,
> > just curios. Note that to be actually affected, such a script must
> > invoke "git rebase -p" _command_ and then tweak its todo output to
> > produce outcome.
> >
> >> Since we did not originally spell out that todo-list items are subject
> >> to enhancement by addition of operations in the future, scripts are
> >> likely not designed to allow addition of new elements.
> >
> > Out of curiosity, are you going to spell it now, for the new todo
> > format?
> >
> >> Thus, adding recreate-merges, and deprecating preserve-merges, seems
> >> to me to be the correct action to take here.
> >
> > Yes, sure, provided there is actual breakage, or at least informed
> > suspicion there is one.
> >
> >> One could argue that users should have expected new todo list elements
> >> to be added in the future and thus design their scripts to cope with
> >> such a thing. If you can convincingly argue this, then I don't
> >> necessarily see it as a complete user breaking change to fix
> >> preserve-merges in order to allow it to handle re-ordering properly..
> >
> > I'd not argue this way myself. If there are out-of-git-tree non-human
> > users that accept and tweak todo _generated_ by current "git rebase -p"
> > _command_, I also vote for a new option.
> >
>
> To be fair, I have not seen anything that actually reads the todo list
> and tweaks it in such a manner. The closest example is the git garden
> shears script, which simply replaces the todo list.
>
> It's certainly *possible* that such a script would exist though,
We actually know of such scripts.
Remember how rewriting parts of rebase -i in C broke somebody's script
because the todo list was not re-read after a successful `exec`?
Guess three times why that script was broken? Precisely: it modified the
todo list!
To see the fix (and the explanation) in all its glory, just have a look at
54fd3243dae (rebase -i: reread the todo list if `exec` touched it,
2017-04-26).
And even if we did not know about any user. What does that mean? Does it
mean that there is no such user? Or does it not rather mean that our
imagination is rather limited, but we *still* should practice safe
software development and use the totally appropriate vehicle of
deprecating, rather than replacing, functionality?
Obviously, the latter option is what I favor, that's why I suggested it in
the first place.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-15 1:14 ` Johannes Schindelin@ 2018-02-15 4:35 ` Sergey Organov
2018-02-15 16:50 ` Johannes Schindelin0 siblings, 1 reply; 412+ messages in thread
From: Sergey Organov @ 2018-02-15 4:35 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jacob Keller, Git mailing list, Junio C Hamano
Hi Johannes,
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
[...]
>> > I'd not argue this way myself. If there are out-of-git-tree non-human
>> > users that accept and tweak todo _generated_ by current "git rebase -p"
>> > _command_, I also vote for a new option.
>> >
>>
>> To be fair, I have not seen anything that actually reads the todo list
>> and tweaks it in such a manner. The closest example is the git garden
>> shears script, which simply replaces the todo list.
>>
>> It's certainly *possible* that such a script would exist though,
>
> We actually know of such scripts.
Please consider to explain this in the description of the change. I
believe readers deserve an explanation of why you decided to invent new
option instead of fixing the old one, even if it were only a suspicion,
more so if it is confidence.
-- Sergey
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-15 4:35 ` Sergey Organov@ 2018-02-15 16:50 ` Johannes Schindelin0 siblings, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-02-15 16:50 UTC (permalink / raw)
To: Sergey Organov; +Cc: Jacob Keller, Git mailing list, Junio C Hamano
Hi,
On Thu, 15 Feb 2018, Sergey Organov wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> [...]
>
> >> > I'd not argue this way myself. If there are out-of-git-tree non-human
> >> > users that accept and tweak todo _generated_ by current "git rebase -p"
> >> > _command_, I also vote for a new option.
> >> >
> >>
> >> To be fair, I have not seen anything that actually reads the todo list
> >> and tweaks it in such a manner. The closest example is the git garden
> >> shears script, which simply replaces the todo list.
> >>
> >> It's certainly *possible* that such a script would exist though,
> >
> > We actually know of such scripts.
>
> Please consider to explain this in the description of the change. I
> believe readers deserve an explanation of why you decided to invent new
> option instead of fixing the old one, even if it were only a suspicion,
> more so if it is confidence.
I considered.
And since even the absence of this use case would *still* not be a
convincing case against keeping --preserve-merges backwards-compatible, I
will not mention it.
Just saying that --preserve-merges is not changed, in order to keep
backwards-compatibility, is plenty enough.
It probably already convinced the Git maintainer, who is very careful
about backwards-compatibility, and rightfully so.
Ciao,
Johannes
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-12 20:39 ` Johannes Schindelin
2018-02-13 4:39 ` Jacob Keller@ 2018-02-13 6:43 ` Sergey Organov
2018-02-15 1:40 ` Johannes Schindelin1 sibling, 1 reply; 412+ messages in thread
From: Sergey Organov @ 2018-02-13 6:43 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Jacob Keller
Hi Johannes,
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Sergey,
>
> On Mon, 12 Feb 2018, Sergey Organov wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> >
>> > On Fri, 9 Feb 2018, Sergey Organov wrote:
>> >
>> >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> >>
>> >> [...]
>> >>
>> >> > With this patch, the goodness of the Git garden shears comes to `git
>> >> > rebase -i` itself. Passing the `--recreate-merges` option will generate
>> >> > a todo list that can be understood readily, and where it is obvious
>> >> > how to reorder commits. New branches can be introduced by inserting
>> >> > `label` commands and calling `merge - <label> <oneline>`. And once this
>> >> > mode has become stable and universally accepted, we can deprecate the
>> >> > design mistake that was `--preserve-merges`.
>> >>
>> >> This doesn't explain why you introduced this new --recreate-merges. Why
>> >> didn't you rather fix --preserve-merges to generate and use new todo
>> >> list format?
>> >
>> > Because that would of course break existing users of
>> > --preserve-merges.
>>
>> How exactly?
>
> Power users of interactive rebase use scripting to augment Git's
> functionality. One particularly powerful trick is to override
> GIT_SEQUENCER_EDITOR with an invocation of such a script, to perform
> automated edits. Such a script breaks when we change the format of the
> content to edit. If we change the format of the todo list generated in
> --preserve-merges mode, that is exactly what happens. We break existing
> users.
I didn't say a word against "--preserve-merges mode", whatever it is,
only about re-using "--preserve-merges" command-line option to "git
rebase", the git user interface. I'm sure you see the difference? Unless
there are out-of-git scripts that do use "git rebase --preserve-merges"
and simultaneously do rely on the todo list format this exact command
generates, there should be no breakage of existing users caused by
changing todo list format generated by "git rebase --preserve-merges".
Old broken "--preserve-merges mode" could be then kept in the
implementation for ages, unused by the new fixed "git rebase
--preserve-merge", for the sake of compatibility.
> BTW it seems that you did not really read my previous reply carefully
> because I referenced such a use case: the Git garden shears.
I thought I did. You confirm below that this script doesn't use "git
rebase --preserve-merges" in the first place, nor will it break if "git
rebase --preserve-merges" starts to generate new todo format, yet you
expected I'd readily see how it's relevant? No, I'm not that clever, nor
am I a mind-reader.
> They do override the sequencer editor, and while they do not exactly
> edit the todo list (they simply through the generated one away), they
> generate a new todo list and would break if that format changes. Of
> course, the shears do not use the --preserve-merges mode,
> but from just reading about the way how the Git garden shears work, it
> is quite obvious how similar users of --preserve-merges are likely to
> exist?
Maybe, I dunno. If even "garden shears" won't break, then what will? Do
you know an example?
Anyway, as it seems it's too late already for such a change, let me stop
this and assume there are indeed such scripts that will break and that
it's indeed a good idea to introduce new option. Case closed. The manual
should still be fixed though, I think.
>> Doesn't "--recreate-merges" produce the same result as
>> "--preserve-merges" if run non-interactively?
>
> The final result of a rebase where you do not edit the todo list? Should
> be identical, indeed.
That's good to hear.
> But that is the most boring, most uninteresting, and least important use
> case.
For you. Do you suddenly stop caring about compatibility?
> So we might just as well forget about it when we focus on keeping
> Git's usage stable.
Why? It's good it behaves the same, so --preserve-merges could indeed be
deprecated, as you apparently intend.
>> > So why not --preserve-merges=v2? Because that would force me to
>> > maintain --preserve-merges forever. And I don't want to.
>> >
>> >> It doesn't seem likely that todo list created by one Git version is
>> >> to be ever used by another, right?
>> >
>> > No. But by scripts based on `git rebase -p`.
>> >
>> >> Is there some hidden reason here? Some tools outside of Git that use
>> >> old todo list format, maybe?
>> >
>> > Exactly.
>> >
>> > I did mention such a tool: the Git garden shears:
>> >
>> > https://github.com/git-for-windows/build-extra/blob/master/shears.sh
>> >
>> > Have a look at it. It will inform the discussion.
>>
>> I've searched for "-p" in the script, but didn't find positives for
>> either "-p" or "--preserve-merges". How it would break if it doesn't use
>> them? What am I missing?
>
> *This* particular script does not use -p.
>
> But it is not *this* particular script that I do not want to break!
I thought that was an example of a tool that would break. Well, it won't
break. Good.
> It is *all* scripts that use interactive rebase!
I'm really interested, and here I *do* ask for education. What are
those? As I now only ask this out of curiosity, and don't argue
--recreate-merges anymore, are you finally willing to reveal the
information?
> Don't you also care about not breaking existing users?
I do care. I just suspected they are very unlikely to exist, and I do
want to be educated in this matter indeed, as they could be rather
interesting.
[Please notice violation of your own standard of not using "Don't
you...", not that I care myself.]
>> >> Then, if new option indeed required, please look at the resulting manual:
>> >>
>> >> --recreate-merges::
>> >> Recreate merge commits instead of flattening the history by replaying
>> >> merges. Merge conflict resolutions or manual amendments to merge
>> >> commits are not preserved.
>> >>
>> >> -p::
>> >> --preserve-merges::
>> >> Recreate merge commits instead of flattening the history by replaying
>> >> commits a merge commit introduces. Merge conflict resolutions or manual
>> >> amendments to merge commits are not preserved.
>> >
>> > As I stated in the cover letter, there are more patches lined up after
>> > this patch series.
>>
>> Good, but I thought this one should better be self-consistent anyway.
>> What if those that come later aren't included?
>
> Right, let's just rip apart the partial progress because the latter
> patches might not make it in?
No, let's fix it instead.
>
> I cannot work on that basis, and I also do not want to work on that basis.
>
> If you do not like how the documentation is worded, fine, suggest a better
> alternative.
I suggested to re-use --preserve-merges command-line option to "git
rebase", unless there are actual users that would break. But as you
believe that's wrong idea, then it could be something like this in the
manual:
--recreate-merges::
Recreate merge commits instead of flattening the history. Merge
conflict resolutions or manual amendments to merge commits are
not preserved.
-p::
--preserve-merges::
This option is similar to --recreate-merges, but doesn't
support interactive mode properly. This option is deprecated,
use --recreate-merges instead.
>
>> > Have a look at https://github.com/git/git/pull/447, especially the
>> > latest commit in there which is an early version of the deprecation I
>> > intend to bring about.
>>
>> You shouldn't want a deprecation at all should you have re-used
>> --preserve-merges in the first place, and I still don't see why you
>> haven't.
>
> Keep repeating it, and it won't become truer.
It is just my point that I repeat, and you gave no evidence it is false,
so I assume it's true, unless proved otherwise.
[...]
> Or for that matter like the design you proposed, to use --first-parent for
> --recreate-merges. Or to use --first-parent for some --recreate-merges,
> surprising users in very bad ways when it is not used (or when it is
> used). I get the impression that you still think it would be a good idea,
> even if it should be obvious that it is not.
What you describe here is bad idea indeed, but it has little to do with
what I actually have in mind and what you apparently don't want to even
try to understand.
>> > Also, please refrain from saying things like... "Don't you think ..."
>> >
>> > If you don't like the wording, I wold much more appreciate it if a better
>> > alternative was suggested.
>>
>> Sorry, but how can I suggest one if I don't understand what you are
>> doing here in the first place? That's why I ask you.
>
> There are ways to put the person you ask on trial. And there are ways to
> genuinely show interest and seek education.
I didn't seek education, nor did I intend any trial. I asked for
clarification of the patch to the manual page that you wrote in a way
that made resulting manual page confusing for me. Confusing manual is
often indication of some additional problem(s) elsewhere, that's what
I've learned for sure, from multiple occasions, so I did reveal my
doubts.
> I am a really poor example how to communicate properly, of course, so
> don't try to learn from me. I am trying myself to learn better ways to
> express what I mean clearly, and to express it in a direct yet kind
> manner.
>
>> >> Don't you think more explanations are needed there in the manual on
>> >> why do we have 2 separate options with almost the same yet subtly
>> >> different description? Is this subtle difference even important? How?
>> >>
>> >> I also have trouble making sense of "Recreate merge commits instead of
>> >> flattening the history by replaying merges." Is it "<Recreate merge
>> >> commits by replaying merges> instead of <flattening the history>" or is it
>> >> rather "<Recreate merge commits> instead of <flattening the history by
>> >> replaying merges>?
>> >
>> > The documentation of the --recreate-merges option is not meant to explain
>> > the difference to --preserve-merges. It is meant to explain the difference
>> > to regular `git rebase -i`, which flattens the commit history into a
>> > single branch without merge commits (in fact, all merge commits are simply
>> > ignored).
>>
>> Yeah, that's obvious, but the point is that resulting manual is ended
>> up being confusing.
>
> Again, just saying something is bad, is bad. Saying something leaves room
> for improvement and then suggesting how to improve it, is good.
Please see wording suggestion above.
>> > And I would rather not start to describe the difference between
>> > --recreate-merges and --preserve-merges because I want to deprecate the
>> > latter, and describing the difference as I get the sense is your wish
>> > would simply mean more work because it would have to be added and then
>> > removed again.
>>
>> I suspect you actually didn't need those new option in the first place,
>> and that's the core reason of these troubles.
>
> Are you suspecting that I, myself, do not use --recreate-merges?
I suspect that if you've had rather changed --preserve-merges, you'd
happily use it and no --recreate-merges were ever necessary. You did
what you did, and it seems to be too late to ask for changing it back,
exactly due to heavy use of this new option.
> If so, please read the cover letter again, in particular the part where I
> describe how this entire series of patch series arose from the Git garden
> shears, which I invented myself to help with maintaining Git for Windows,
> and which I use for five years now. This should help disperse that
> suspicion rather quickly: the intent of --recreate-merges is to allow me
> to simplify the shears by quite a bit, and maybe eventually even get rid
> of the script altogether (if I ever manage to convince myself that the
> concept of a merging-rebase should be official enough to enter core Git).
>
> I am a heavy user of --recreate-merges, even if it does not really exist
> yet. I have five years of experience with it, which is the reason why I am
> so confident about its design, and why I can tell you a lot about typical
> use cases and common pitfalls, and where the original design had to be
> adjusted.
I fail to see how anything of the above would change should
--recreate-merges be still called --preserve-merges, but I do see why
you don't want that to happen now, so please only consider fixing of the
manual page.
-- Sergey
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
2018-02-13 6:43 ` Sergey Organov@ 2018-02-15 1:40 ` Johannes Schindelin0 siblings, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-02-15 1:40 UTC (permalink / raw)
To: Sergey Organov; +Cc: git, Junio C Hamano, Jacob Keller
Hi Sergey,
On Tue, 13 Feb 2018, Sergey Organov wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > On Mon, 12 Feb 2018, Sergey Organov wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> >
> >> > On Fri, 9 Feb 2018, Sergey Organov wrote:
> >> >
> >> >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >> >>
> >> >> [...]
> >> >>
> >> >> > With this patch, the goodness of the Git garden shears comes to `git
> >> >> > rebase -i` itself. Passing the `--recreate-merges` option will generate
> >> >> > a todo list that can be understood readily, and where it is obvious
> >> >> > how to reorder commits. New branches can be introduced by inserting
> >> >> > `label` commands and calling `merge - <label> <oneline>`. And once this
> >> >> > mode has become stable and universally accepted, we can deprecate the
> >> >> > design mistake that was `--preserve-merges`.
> >> >>
> >> >> This doesn't explain why you introduced this new --recreate-merges. Why
> >> >> didn't you rather fix --preserve-merges to generate and use new todo
> >> >> list format?
> >> >
> >> > Because that would of course break existing users of
> >> > --preserve-merges.
> >>
> >> How exactly?
> >
> > Power users of interactive rebase use scripting to augment Git's
> > functionality. One particularly powerful trick is to override
> > GIT_SEQUENCER_EDITOR with an invocation of such a script, to perform
> > automated edits. Such a script breaks when we change the format of the
> > content to edit. If we change the format of the todo list generated in
> > --preserve-merges mode, that is exactly what happens. We break existing
> > users.
>
> I didn't say a word against "--preserve-merges mode", whatever it is,
> only about re-using "--preserve-merges" command-line option to "git
> rebase", the git user interface.
*I* said something against --preserve-merges. You did not even need to. I
know fully well its limitations.
I also said something agains the suggestion to replace the functionality
of a previously well-defined (although misdesigned) feature.
I do not know how often I have to repeat that your suggestion would break
backwards-compatibility?
> I'm sure you see the difference?
Yes, of course I do, and you do not even have to suggest otherwise by
asking such a question.
I already demonstrated plenty of times that I do understand what you wish
for, and that I see serious problems with it.
> Unless there are out-of-git scripts that do use "git rebase
> --preserve-merges" and simultaneously do rely on the todo list format
> this exact command generates, there should be no breakage of existing
> users caused by changing todo list format generated by "git rebase
> --preserve-merges".
So. Just because you cannot imagine that anybody uses rebase in such a
powerful way means you are willing to break their setups?
Git is used by millions of users. Many of them are power users. It would
be quite naive to assume that nobody uses rebase -p in a scripted manner
that modifies the todo list.
Changing the behavior of --preserve-merges would be simply irresponsible,
and that's why we won't do it.
Even if that was not so, there is yet another really good reason not to
reuse the name --preserve-merges: The name itself suggests that this mode
is about preserving all merges in the specified commit range. That was its
original intention, too, as I never designed it to be user with rebase -i.
If the todo list of rebase -p is not modified (preserving the entire
commit topology as well as possible), it works quite well.
The new mode is not so much about preserving, though. It is about
interactively modifying the todo list, to change the order of the commits,
even to change the branch topology. That means that we do not necessarily
preserve the merges. We recreate them. So you see, I did try to be careful
about the naming, too. I thought about this.
> Old broken "--preserve-merges mode" could be then kept in the
> implementation for ages, unused by the new fixed "git rebase
> --preserve-merge", for the sake of compatibility.
This sentence contradicts itself. Either you keep the code unused, or you
keep it used for backwards-compatibility.
> > BTW it seems that you did not really read my previous reply carefully
> > because I referenced such a use case: the Git garden shears.
>
> I thought I did. You confirm below that this script doesn't use "git
> rebase --preserve-merges" in the first place, nor will it break if "git
> rebase --preserve-merges" starts to generate new todo format, yet you
> expected I'd readily see how it's relevant? No, I'm not that clever, nor
> am I a mind-reader.
You caught me. I am not a user of --preserve-merges. Not anymore.
Does that mean that by extension nobody is a user of that feature?
Certainly not.
And does my example of (ab-)using interactive rebase by scripting on top
of it maybe suggest that others do the same? Maybe even with
--preserve-merges? Most likely. Git is used by many, many users. It would
be foolish to make any assumption about how Git is used by others.
> > They do override the sequencer editor, and while they do not exactly
> > edit the todo list (they simply through the generated one away), they
> > generate a new todo list and would break if that format changes. Of
> > course, the shears do not use the --preserve-merges mode, but from
> > just reading about the way how the Git garden shears work, it is quite
> > obvious how similar users of --preserve-merges are likely to exist?
>
> Maybe, I dunno. If even "garden shears" won't break, then what will? Do
> you know an example?
You are not seriously suggesting that we should assume that there is no
such Git user, just because neither you nor I personally know such a user?
Seriously?
> Anyway, as it seems it's too late already for such a change, let me stop
> this and assume there are indeed such scripts that will break and that
> it's indeed a good idea to introduce new option. Case closed. The manual
> should still be fixed though, I think.
Finally I got through. Yes, we cannot break backwards-compatibility.
> >> Doesn't "--recreate-merges" produce the same result as
> >> "--preserve-merges" if run non-interactively?
> >
> > The final result of a rebase where you do not edit the todo list? Should
> > be identical, indeed.
>
> That's good to hear.
>
> > But that is the most boring, most uninteresting, and least important use
> > case.
>
> For you. Do you suddenly stop caring about compatibility?
What does "fun" and "interesting" have to do with compatibility?
Yes, to me, this case is boring. And yes, I took pains to make it work
(for compatibility).
And yes, I did not stop after that. After the boring case, I still wanted
to think things through, to come up with a design that would not be too
limited to be useful. With a design that is consistent.
If you find holes in the consistency or usability, please do call them
out.
But please stop suggesting to break backwards-compatibility, or to
introduce features that are inconsistent and/or can produce "surprising"
results (as --first-parent would, and multiple contributors had to argue
in concert, pointing out how it is not extensible to the general case, and
is hence consistent).
> > It is *all* scripts that use interactive rebase!
>
> I'm really interested, and here I *do* ask for education. What are
> those? As I now only ask this out of curiosity, and don't argue
> --recreate-merges anymore, are you finally willing to reveal the
> information?
If you are interested, why don't you go about asking people for their
power scripts.
In the context of this patch series, I am not interested in such a
collection. What I had to do was to convince myself that they could not
exist, in which case I could just do away with backwards-compatibility. In
the alternative, the safe play is to go the deprecation route.
A mere "highly unlikely" made up from thin air does not convince me,
though. So deprecation route it is.
> > Don't you also care about not breaking existing users?
>
> I do care. I just suspected they are very unlikely to exist, and I do
> want to be educated in this matter indeed, as they could be rather
> interesting.
Okay, "very unlikely". Not "highly unlikely". Still, it is an unconvincing
argument that suffers very seriously from lack of any robust evidence.
> [Please notice violation of your own standard of not using "Don't
> you...", not that I care myself.]
True. My apologies.
And as the rest of the mail seems to reiterate the idea that the
--recreate-merges code should override --preserve-merges (breaking
backwards-compatibility), despite my repeated efforts to educate you why
this would be a bad idea, I guess the best course of action to avoid
telling you "Don't you ..." is to just stop here.
Ciao,
Johannes
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 8/8] rebase -i: introduce --recreate-merges=no-rebase-cousins
2018-01-18 22:00 ` Philip Oakley@ 2018-01-29 20:42 ` Johannes Schindelin0 siblings, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 20:42 UTC (permalink / raw)
To: Philip Oakley; +Cc: git, Junio C Hamano, Jacob Keller
Hi Philip,
On Thu, 18 Jan 2018, Philip Oakley wrote:
> From: "Johannes Schindelin" <johannes.schindelin@gmx.de>
> > This one is a bit tricky to explain, so let's try with a diagram:
> >
> > C
> > / \
> > A - B - E - F
> > \ /
> > D
> >
> > To illustrate what this new mode is all about, let's consider what
> > happens upon `git rebase -i --recreate-merges B`, in particular to
> > the commit `D`. In the default mode, the new branch structure is:
> >
> > --- C' --
> > / \
> > A - B ------ E' - F'
> > \ /
> > D'
> >
> > This is not really preserving the branch topology from before! The
> > reason is that the commit `D` does not have `B` as ancestor, and
> > therefore it gets rebased onto `B`.
> >
> > However, when recreating branch structure, there are legitimate use
> > cases where one might want to preserve the branch points of commits that
> > do not descend from the <upstream> commit that was passed to the rebase
> > command, e.g. when a branch from core Git's `next` was merged into Git
> > for Windows' master we will not want to rebase those commits on top of a
> > Windows-specific commit. In the example above, the desired outcome would
> > look like this:
> >
> > --- C' --
> > / \
> > A - B ------ E' - F'
> > \ /
> > -- D' --
>
> I'm not understanding this. I see that D properly starts from A, but
> don't see why it is now D'. Surely it's unchanged.
It is not necessarily unchanged, because this is an *interactive* rebase.
If you mark `D` for `reword`, for example, it may be changed.
I use the label D' in the mathematical sense, to indicate that D' is
derived from D. It may even be identical to D, but the point is that it is
in the todo list of the interactive rebase, so it can be changed. As
opposed to, say, A and B. Those cannot be changed in this interactive
rebase.
> Maybe it's the arc/node confusion. Maybe even spell out that the rebased
> commits from the command are B..HEAD, but that includes D, which may not
> be what folk had expected. (not even sure if the reflog comes into
> determining merge-bases here..)
>
> I do think an exact definition is needed (e.g. via --ancestry-path or
> its equivalent?).
I don't find "ancestry path" any more intuitive a term than the
mathematically correct "uncomparable".
If you have a better way to explain this (without devolving into
mathematical terminology), please let's hear it.
Don't get me wrong, as a mathematician I am comfortable with very precise
descriptions involving plenty of Greek symbols.
But this documentation, and these commit messages do not target myself. I
know perfectly well what I am talking about here. The target audience are
software developers who may not have a background in mathematics, who do
not even want to fully understand what the heck constitutes a Directed
Acyclic Graph.
So what we need here is plain English. And I had thought that the analogy
with the family tree would be intuitive enough for even math haters to
understand easily and quickly...
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 9/8] [DO NOT APPLY, but squash?] git-rebase--interactive: clarify arguments
2018-01-18 21:36 ` Johannes Schindelin
2018-01-18 21:58 ` Stefan Beller@ 2018-01-19 20:30 ` Junio C Hamano
2018-01-20 9:14 ` Jacob Keller1 sibling, 1 reply; 412+ messages in thread
From: Junio C Hamano @ 2018-01-19 20:30 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Stefan Beller, git, jacob.keller
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Good idea! I would rather do it as an introductory patch (that only
> converts the existing list).
>
> As to `merge`: it is a bit more complicated ;-)
>
> m, merge <original-merge-commit> ( <label> | "<label>..." ) [<oneline>]
> create a merge commit using the original merge commit's
> message (or the oneline, if "-" is given). Use a quoted
> list of commits to be merged for octopus merges.
Is it just the message that is being reused?
Aren't the trees of the original commit and its parents participate
in creating the tree of the recreated merge? One way to preserve an
originally evil merge is to notice how it was made by taking the
difference between the result of mechanical merge of original merge
parents and the original merge result, and carry it forward when
recreating the merge across new parents. Just being curious.
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 9/8] [DO NOT APPLY, but squash?] git-rebase--interactive: clarify arguments
2018-01-19 20:30 ` Junio C Hamano@ 2018-01-20 9:14 ` Jacob Keller
2018-01-29 17:02 ` Johannes Schindelin0 siblings, 1 reply; 412+ messages in thread
From: Jacob Keller @ 2018-01-20 9:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Stefan Beller, Git mailing list
On Fri, Jan 19, 2018 at 12:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Good idea! I would rather do it as an introductory patch (that only
>> converts the existing list).
>>
>> As to `merge`: it is a bit more complicated ;-)
>>
>> m, merge <original-merge-commit> ( <label> | "<label>..." ) [<oneline>]
>> create a merge commit using the original merge commit's
>> message (or the oneline, if "-" is given). Use a quoted
>> list of commits to be merged for octopus merges.
>
> Is it just the message that is being reused?
>
> Aren't the trees of the original commit and its parents participate
> in creating the tree of the recreated merge? One way to preserve an
> originally evil merge is to notice how it was made by taking the
> difference between the result of mechanical merge of original merge
> parents and the original merge result, and carry it forward when
> recreating the merge across new parents. Just being curious.
>
It looks like currently that only the commit is kept, with no attempt
to recreate evil merges.
Thanks,
Jake
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 9/8] [DO NOT APPLY, but squash?] git-rebase--interactive: clarify arguments
2018-01-20 9:14 ` Jacob Keller@ 2018-01-29 17:02 ` Johannes Schindelin0 siblings, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 17:02 UTC (permalink / raw)
To: Jacob Keller; +Cc: Junio C Hamano, Stefan Beller, Git mailing list
Hi,
On Sat, 20 Jan 2018, Jacob Keller wrote:
> On Fri, Jan 19, 2018 at 12:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> Good idea! I would rather do it as an introductory patch (that only
> >> converts the existing list).
> >>
> >> As to `merge`: it is a bit more complicated ;-)
> >>
> >> m, merge <original-merge-commit> ( <label> | "<label>..." ) [<oneline>]
> >> create a merge commit using the original merge commit's
> >> message (or the oneline, if "-" is given). Use a quoted
> >> list of commits to be merged for octopus merges.
> >
> > Is it just the message that is being reused?
> >
> > Aren't the trees of the original commit and its parents participate
> > in creating the tree of the recreated merge? One way to preserve an
> > originally evil merge is to notice how it was made by taking the
> > difference between the result of mechanical merge of original merge
> > parents and the original merge result, and carry it forward when
> > recreating the merge across new parents. Just being curious.
> >
>
> It looks like currently that only the commit is kept, with no attempt
> to recreate evil merges.
Yep. I even documented that somewhere ;-)
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 10/8] [DO NOT APPLY, but improve?] rebase--interactive: introduce "stop" command
2018-01-18 22:00 ` Johannes Schindelin@ 2018-01-18 22:09 ` Stefan Beller0 siblings, 0 replies; 412+ messages in thread
From: Stefan Beller @ 2018-01-18 22:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Jacob Keller
On Thu, Jan 18, 2018 at 2:00 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>> + TODO_STOP,
>
> I see that your original idea was "stop", but then you probably realized
> that there would be no good abbreviation for that, and changed your mind.
>
> Personally, I would have called it `break`...
I was looking at a synonym list of stop to find a word that contained a letter
which was not already taken. 'break' would allow for 'a', or 'k', assuming 'bud'
takes 'b' (or can that go to 'u'? Are there people out there with muscle memory
on these letters already?)
Any word (of stop, break, stay, control) sounds good to me, though 'break' might
be the clearest.
>
>> @@ -2407,6 +2415,8 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>> /* `current` will be incremented below */
>> todo_list->current = -1;
>> }
>> + } else if (item->command == TODO_STOP) {
>> + todo_list->current = -1;
>
> That is incorrect, it will most likely write an unexpected `done` file.
>
> Did you mean `return 0` instead?
I guess. I did not compile or test the patch, I was merely writing down enough
to convey the idea, hopefully.
While talking about this idea of exploding the number of keywords,
maybe we can also have 'abort', which does the same as deleting all lines
(every time I want to abort I still get shivers if I just drop all
patches instead
of aborting, so maybe typing 'abort-and-restore' as the first thing in the file
would convey a safer feeling to users?)
Thanks for taking these additional considerations into mind while I don't
review the actual patches,
Stefan
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 0/8] rebase -i: offer to recreate merge commits
2018-01-18 15:35 [PATCH 0/8] rebase -i: offer to recreate merge commits Johannes Schindelin
` (9 preceding siblings ...)
2018-01-18 18:36 ` [PATCH 9, 10/8] interactive rebase feedback Stefan Beller
@ 2018-01-19 20:25 ` Junio C Hamano
2018-01-29 21:53 ` Johannes Schindelin
2018-01-23 20:29 ` Junio C Hamano
2018-01-29 22:54 ` [PATCH v2 00/10] " Johannes Schindelin
12 siblings, 1 reply; 412+ messages in thread
From: Junio C Hamano @ 2018-01-19 20:25 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jacob Keller
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Think of --recreate-merges as "--preserve-merges done right". It
> introduces new verbs for the todo list, `label`, `reset` and `merge`.
> For a commit topology like this:
>
> A - B - C
> \ /
> D
>
> the generated todo list would look like this:
>
> # branch D
> pick 0123 A
> label branch-point
> pick 1234 D
> label D
>
> reset branch-point
> pick 2345 B
> merge 3456 D C
Yup. I've seen this design talked about on list in the past, and
I've always felt that this is "sequencer done right".
At the first glance, it may feel somewhat unsatisfying that "merge"
has to say effects of which commits should be reflected in the
result and which commot to take the log message from, i.e.
(recreated)D is merged to form the resulting tree, and 3456=C is
used for the log, to recreate C in the above example, while "pick"
always uses the same commit for both, i.e. recreated B inherits both
the changes and log message from the original B=2345 (or depending
on the readers' point of view, "merge" is allowed to use two
different commits, while "pick" is always limited to the same one).
But I think this distinction is probably fundamental and I am not
opposed to it at all. The result of "pick" has only one parent, and
the parent is determined only by the previous actions and not by
anything on the "pick" line in the todo list. But the result of
"merge" has to record all the other parents, and only the first
parent is determined implicitly by the previous actions. We need to
tell the "merge" command about "3456=C" in order to recreate the
effect of original merge commit (i.e. changes between B and C) as
well as its log message, and we also need to tell it about label "D"
that it is the "other parent" that need to be recorded.
Obviously "merge" command syntax should allow recreating an octopus,
so whenever I said "two" in the above, I meant "N". The original
merge commit is needed so that the effect to replay (roughly: a
patch going to the original merge result from its first parent) can
be learned from the existing history, and all the other "N-1"
parents needs to be given (and they must have been already created
in the todo list) so that the resulting recreated merge can be
recorded with them as parents (in addition to the first parent that
is implicitly given as the result of all the previous steps).
One interesting (and probably useful) thing to notice is that if A
were not rebased in the above sample picture, and only B were the
one that was tweaked, then a recreated C may use the same original D
as its side parent, and the mechanism outlined above naturally can
support it by allowing an un-rewritten commit to be given as a side
parent when "merge" is redoing C.
I probably won't have time to actually look at the code for a few
days, but I am reasonably excited about the topic ;-)
Thanks.
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 0/8] rebase -i: offer to recreate merge commits
2018-01-19 20:25 ` [PATCH 0/8] rebase -i: offer to recreate merge commits Junio C Hamano
@ 2018-01-29 21:53 ` Johannes Schindelin0 siblings, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 21:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jacob Keller
Hi Junio,
On Fri, 19 Jan 2018, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > Think of --recreate-merges as "--preserve-merges done right". It
> > introduces new verbs for the todo list, `label`, `reset` and `merge`.
> > For a commit topology like this:
> >
> > A - B - C
> > \ /
> > D
> >
> > the generated todo list would look like this:
> >
> > # branch D
> > pick 0123 A
> > label branch-point
> > pick 1234 D
> > label D
> >
> > reset branch-point
> > pick 2345 B
> > merge 3456 D C
>
> Yup. I've seen this design talked about on list in the past, and
> I've always felt that this is "sequencer done right".
>
> At the first glance, it may feel somewhat unsatisfying that "merge"
> has to say effects of which commits should be reflected in the
> result and which commot to take the log message from, i.e.
> (recreated)D is merged to form the resulting tree, and 3456=C is
> used for the log, to recreate C in the above example, while "pick"
> always uses the same commit for both, i.e. recreated B inherits both
> the changes and log message from the original B=2345 (or depending
> on the readers' point of view, "merge" is allowed to use two
> different commits, while "pick" is always limited to the same one).
>
> But I think this distinction is probably fundamental and I am not
> opposed to it at all. The result of "pick" has only one parent, and
> the parent is determined only by the previous actions and not by
> anything on the "pick" line in the todo list. But the result of
> "merge" has to record all the other parents, and only the first
> parent is determined implicitly by the previous actions. We need to
> tell the "merge" command about "3456=C" in order to recreate the
> effect of original merge commit (i.e. changes between B and C) as
> well as its log message, and we also need to tell it about label "D"
> that it is the "other parent" that need to be recorded.
Yes, this was the hard lesson of the failed preserve-merges design.
> Obviously "merge" command syntax should allow recreating an octopus,
> so whenever I said "two" in the above, I meant "N". The original
> merge commit is needed so that the effect to replay (roughly: a
> patch going to the original merge result from its first parent) can
> be learned from the existing history, and all the other "N-1"
> parents needs to be given (and they must have been already created
> in the todo list) so that the resulting recreated merge can be
> recorded with them as parents (in addition to the first parent that
> is implicitly given as the result of all the previous steps).
I have two more patch series lined up after this one, the first one
implements --root via the sequencer, and the second one indeed extends
`merge` to handle octopus commits.
> One interesting (and probably useful) thing to notice is that if A
> were not rebased in the above sample picture, and only B were the
> one that was tweaked, then a recreated C may use the same original D
> as its side parent, and the mechanism outlined above naturally can
> support it by allowing an un-rewritten commit to be given as a side
> parent when "merge" is redoing C.
I think that you will get a kick out of reading the commit message of the
last commit, as it does talk about the problematic C: it *would* be
rebased by default.
In the next iteration I will actually switch around the default from
rebase-cousins to no-rebase-cousins for that reason.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH 0/8] rebase -i: offer to recreate merge commits
2018-01-23 20:29 ` Junio C Hamano@ 2018-01-29 22:53 ` Johannes Schindelin0 siblings, 0 replies; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-29 22:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jacob Keller
Hi Junio,
On Tue, 23 Jan 2018, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > My original attempt was --preserve-merges, but that design was so
> > limited that I did not even enable it in interactive mode.
> > ...
> > There are more patches in the pipeline, based on this patch series, but
> > left for later in the interest of reviewable patch series: one mini
> > series to use the sequencer even for `git rebase -i --root`, and another
> > one to add support for octopus merges to --recreate-merges.
>
> I left comments on a handful of them, but I do not think any of them
> spotted a grave design issue to be a show stopper. Overall, the
> series was quite a pleasant read, even with those minor nits and
> rooms for improvements.
I objected to a couple obviously problematic suggestions, and I
implemented all others (even those to which I did not respond
specifically).
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH v2 00/10] rebase -i: offer to recreate merge commits
2018-01-29 22:54 ` [PATCH v2 00/10] " Johannes Schindelin
` (9 preceding siblings ...)
2018-01-29 22:55 ` [PATCH v2 10/10] rebase -i: introduce --recreate-merges=[no-]rebase-cousins Johannes Schindelin
@ 2018-01-30 18:47 ` Stefan Beller
2018-01-31 13:08 ` Johannes Schindelin
2018-01-30 21:36 ` Junio C Hamano
2018-02-11 0:09 ` [PATCH v3 00/12] " Johannes Schindelin
12 siblings, 1 reply; 412+ messages in thread
From: Stefan Beller @ 2018-01-30 18:47 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Jacob Keller, Philip Oakley, Eric Sunshine,
Phillip Wood
On Mon, Jan 29, 2018 at 2:54 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Once upon a time, I dreamt of an interactive rebase that would not
> flatten branch structure, but instead recreate the commit topology
> faithfully.
>
> My original attempt was --preserve-merges, but that design was so
> limited that I did not even enable it in interactive mode.
>
> Subsequently, it *was* enabled in interactive mode, with the predictable
> consequences: as the --preserve-merges design does not allow for
> specifying the parents of merge commits explicitly, all the new commits'
> parents are defined *implicitly* by the previous commit history, and
> hence it is *not possible to even reorder commits*.
>
> This design flaw cannot be fixed. Not without a complete re-design, at
> least. This patch series offers such a re-design.
>
> Think of --recreate-merges as "--preserve-merges done right". It
> introduces new verbs for the todo list, `label`, `reset` and `merge`.
> For a commit topology like this:
>
> A - B - C
> \ /
> D
>
> the generated todo list would look like this:
>
> # branch D
> pick 0123 A
> label branch-point
> pick 1234 D
> label D
>
> reset branch-point
> pick 2345 B
> merge 3456 D C
>
> There are more patches in the pipeline, based on this patch series, but
> left for later in the interest of reviewable patch series: one mini
> series to use the sequencer even for `git rebase -i --root`, and another
> one to add support for octopus merges to --recreate-merges.
>
> Changes since v1:
>
> - reintroduced "sequencer: make refs generated by the `label` command
> worktree-local" (which was squashed into "sequencer: handle autosquash
> and post-rewrite for merge commands" by accident)
>
> - got rid of the universally-hated `bud` command
Sorry if you got the impression for that. Maybe I was imprecise.
I had no strong opinion one way or another, I merely pointed out the
collision in abbreviation letters with the potential new 'break', IIRC.
'bud' was a special case for resetting to a specific revision
(and labeling it?)
Maybe we can have default labels, such that there is no need to reset
to the first revision manually, but can just use these defaults in the merge.
(I haven't thought about this in the big picture, just food for thought)
>
> - as per Stefan's suggestion, the help blurb at the end of the todo list
> now lists the syntax
>
> - the no-rebase-cousins mode was made the default; This not only reflects
> the experience won from those years of using the Git garden shears, but
> was also deemed the better default in the discussion on the PR at
> https://github.com/git/git/pull/447
>
> - I tried to clarify the role of the `onto` label in the commit message of
> `rebase-helper --make-script: introduce a flag to recreate merges`
>
> - fixed punctuation at the end of error(...) messages, and incorrect
> upper-case at the start
>
> - changed the generated todo lists to separate the label and the oneline in
> the `reset` command with a `#`, for readability
>
> - dropped redundant paragraph in the commit message that talked about
> support for octopus merges
>
> - avoided empty error message when HEAD could not be read during do_label()
>
> - merge commits are fast-forwarded only unless --force-rebase was passed
>
> - do_merge() now errors out a lot earlier when HEAD could not be parsed
>
> - the one-letter variables to hold either abbreviated or full todo list
> instructions in make_script_recreating_merges() were renamed to clearer
> names
>
> - The description of rebase's --recreate-merge option has been reworded;
> Hopefully it is a lot more clear now.
>
>
> Johannes Schindelin (9):
> sequencer: introduce new commands to reset the revision
> sequencer: introduce the `merge` command
> sequencer: fast-forward merge commits, if possible
> rebase-helper --make-script: introduce a flag to recreate merges
> rebase: introduce the --recreate-merges option
> sequencer: make refs generated by the `label` command worktree-local
> sequencer: handle autosquash and post-rewrite for merge commands
> pull: accept --rebase=recreate to recreate the branch topology
> rebase -i: introduce --recreate-merges=[no-]rebase-cousins
>
> Stefan Beller (1):
> git-rebase--interactive: clarify arguments
No need to honor me with authorship, as I just wrote
that patch in a quick hurry to express the idea.
But this is fine, too.
The interdiff looks good to me, I'll review the patches now.
Thanks,
Stefan
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH v2 00/10] rebase -i: offer to recreate merge commits
2018-01-30 21:36 ` Junio C Hamano@ 2018-01-31 13:29 ` Johannes Schindelin
2018-02-01 6:37 ` Jacob Keller0 siblings, 1 reply; 412+ messages in thread
From: Johannes Schindelin @ 2018-01-31 13:29 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jacob Keller, Stefan Beller, Philip Oakley, Eric Sunshine,
Phillip Wood
Hi Junio,
On Tue, 30 Jan 2018, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > Changes since v1:
> >
> > - reintroduced "sequencer: make refs generated by the `label` command
> > worktree-local" (which was squashed into "sequencer: handle autosquash
> > and post-rewrite for merge commands" by accident)
>
> Good.
>
> > - got rid of the universally-hated `bud` command
>
> Universally is a bit too strong a word, unless you want to hint that
> you are specifically ignoring my input ;-).
In the interest of comic effect, I exaggerated a little.
> > - the no-rebase-cousins mode was made the default
>
> Although I lack first-hand experience with this implementation, this
> design decision matches my instinct.
Excellent.
> May comment on individual patches separately, later.
I think I may want to introduce a bigger change, still. I forgot who
exactly came up with the suggestion to use `merge -C <original-commit>
<to-merge>` (I think it was Jake), and I reacted too forcefully in
rejecting it.
This design had been my original design in the Git garden shears, and I
did not like it because it felt clunky and it also broke the style of
<command> <commit>.
But the longer I think about this, the more I come to the conclusion that
I was wrong, and that the -C way is the way that leaves the door open to
the pretty elegant `-c <commit>` (imitating `git commit`'s option to
borrow the commit message from elsewhere but still allowing to edit it).
And it also leaves open the door to just write `merge <to-merge>` and have
the sequencer come up with a default merge message that the user can then
edit.
I'll probably refrain from implementing support for -m because I do not
want to implement the dequoting.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread

*Re: [PATCH v2 00/10] rebase -i: offer to recreate merge commits
2018-01-31 13:29 ` Johannes Schindelin@ 2018-02-01 6:37 ` Jacob Keller0 siblings, 0 replies; 412+ messages in thread
From: Jacob Keller @ 2018-02-01 6:37 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Git mailing list, Stefan Beller, Philip Oakley,
Eric Sunshine, Phillip Wood
On Wed, Jan 31, 2018 at 5:29 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> I think I may want to introduce a bigger change, still. I forgot who
> exactly came up with the suggestion to use `merge -C <original-commit>
> <to-merge>` (I think it was Jake), and I reacted too forcefully in
> rejecting it.
>
I believe someone else suggested it, but I replied that I liked it.
> This design had been my original design in the Git garden shears, and I
> did not like it because it felt clunky and it also broke the style of
> <command> <commit>.
>
I agree it's a bit weird it breaks the style of "<command> <commit>",
but on some level merge does this anyways as it's the first one to
take more than one argument.
> But the longer I think about this, the more I come to the conclusion that
> I was wrong, and that the -C way is the way that leaves the door open to
> the pretty elegant `-c <commit>` (imitating `git commit`'s option to
> borrow the commit message from elsewhere but still allowing to edit it).
>
The other reason I liked this, is that it matches merge syntax on the
command line, so users don't need to learn a special new syntax for
the todo file.
> And it also leaves open the door to just write `merge <to-merge>` and have
> the sequencer come up with a default merge message that the user can then
> edit.
I like that we could completely forgo the -C and -c in order to allow
the normal default merge commit message that is auto generated as
well.
>
> I'll probably refrain from implementing support for -m because I do not
> want to implement the dequoting.
>
Yea, I don't think that is necessary either.
Thanks,
Jake
> Ciao,
> Dscho
^permalinkrawreply [flat|nested] 412+ messages in thread