Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi,

On Thu, 15 Feb 2018, Sergey Organov wrote:

> Johannes Schindelin  writes:
> 
> > On Tue, 13 Feb 2018, Sergey Organov wrote:
> >
> >> Johannes Schindelin  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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi,

On Thu, 15 Feb 2018, Sergey Organov wrote:

> Johannes Schindelin  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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Hi Johannes,
Johannes Schindelin  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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Johannes Schindelin  writes:

> Hi,
>
> On Tue, 13 Feb 2018, Sergey Organov wrote:
>
>> Johannes Schindelin  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.


-- Sergey

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi Sergey,

On Tue, 13 Feb 2018, Sergey Organov wrote:

> Johannes Schindelin  writes:
> >
> > On Mon, 12 Feb 2018, Sergey Organov wrote:
> >
> >> Johannes Schindelin  writes:
> >> >
> >> > On Fri, 9 Feb 2018, Sergey Organov wrote:
> >> >
> >> >> Johannes Schindelin  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 -  `. 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 

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi Jake,

On Tue, 13 Feb 2018, Jacob Keller wrote:

> On Mon, Feb 12, 2018 at 11:15 PM, Sergey Organov  wrote:
> >
> > Jacob Keller  writes:
> >
> >> On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
> >>  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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi,

On Tue, 13 Feb 2018, Sergey Organov wrote:

> Johannes Schindelin  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.

Ciao,
Johannes

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Johannes Schindelin  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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Jacob Keller]

On Mon, Feb 12, 2018 at 11:15 PM, Sergey Organov  wrote:
> Hi Jake,
>
> Jacob Keller  writes:
>
>> On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
>>  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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Hi Jake,

Jacob Keller  writes:

> On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
>  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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Hi Johannes,

Johannes Schindelin  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 "> >> commits by replaying merges> instead of " or is it
>> >> rather " instead of > >> 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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Hi Johannes,

Johannes Schindelin  writes:
> Hi Sergey,
>
> On Mon, 12 Feb 2018, Sergey Organov wrote:
>
>> Johannes Schindelin  writes:
>> >
>> > On Fri, 9 Feb 2018, Sergey Organov wrote:
>> >
>> >> Johannes Schindelin  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 -  `. 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 

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Jacob Keller]

On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
 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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi Sergey,

On Mon, 12 Feb 2018, Sergey Organov wrote:

> Johannes Schindelin  writes:
> >
> > On Fri, 9 Feb 2018, Sergey Organov wrote:
> >
> >> Johannes Schindelin  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 -  `. 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 

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

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 " >> commits by replaying merges> instead of " or is it
> >> rather " instead of  >> 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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Johannes Sixt  writes:

> Am 09.02.2018 um 07:11 schrieb Sergey Organov:
>> Johannes Schindelin  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.

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Hi Johannes,

Johannes Schindelin  writes:
> Hi Sergey,
>
> On Fri, 9 Feb 2018, Sergey Organov wrote:
>
>> Johannes Schindelin  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 -  `. 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 "> commits by replaying merges> instead of " or is it
>> rather " instead of > 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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Hi Johannes,

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 "> commits by replaying merges> instead of " or is it
>> rather " instead of > replaying merges>?

-- Sergey

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Jacob Keller]

On Thu, Feb 8, 2018 at 11:13 PM, Johannes Sixt  wrote:
> Am 09.02.2018 um 07:11 schrieb Sergey Organov:
>>
>> Johannes Schindelin  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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi Sergey,

On Fri, 9 Feb 2018, Sergey Organov wrote:

> Johannes Schindelin  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 -  `. 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 " commits by replaying merges> instead of " or is it
> rather " instead of  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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi Junio,

On Tue, 23 Jan 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> > index 8a861c1e0d6..1d061373288 100644
> > --- a/Documentation/git-rebase.txt
> > +++ b/Documentation/git-rebase.txt
> > @@ -368,6 +368,11 @@ The commit list format can be changed by setting the 
> > configuration option
> >  rebase.instructionFormat.  A customized instruction format will 
> > automatically
> >  have the long commit hash prepended to the format.
> >  
> > +--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.
> > +
> 
> It is sensible to postpone tackling "evil merges" in this initial
> iteration of the series, and "manual amendments ... not preserved"
> is a reasonable thing to document.  But do we want to say a bit more
> about conflicting merges?  "conflict resolutions ... not preserved"
> sounds as if it does not stop and instead record the result with
> conflict markers without even letting rerere to kick in, which
> certainly is not the impression you wanted to give to the readers.
> 
> I am imagining that it will stop and give control back to the end
> user just like a conflicted "pick" would, and allow "rebase
> --continue" to record resolution from the working tree, and just
> like conflicted "pick", it would allow rerere() to help end users
> recall previous resolution.

This is my current version:

--recreate-merges[=(rebase-cousins|no-rebase-cousins)]::
Recreate merge commits instead of flattening the history by replaying
merges. Merge conflict resolutions or manual amendments to merge
commits are not recreated automatically, but have to be recreated
manually.

Ciao,
Dscho

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Sixt]

Am 09.02.2018 um 07:11 schrieb Sergey Organov:

Johannes Schindelin  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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Johannes Schindelin  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 -  `. 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 " instead of " or is it
rather " instead of ?

-- Sergey

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Hi,

Johannes Schindelin  writes:
> On Wed, 7 Feb 2018, Sergey Organov wrote:
>> Johannes Schindelin  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 

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi Junio,

On Wed, 7 Feb 2018, Junio C Hamano wrote:

> Øyvind Rønningstad  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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Junio C Hamano]

Øyvind Rønningstad  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.

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Øyvind Rønningstad]

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  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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi,

On Wed, 7 Feb 2018, Sergey Organov wrote:

> Johannes Schindelin  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 
> 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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Jacob Keller  writes:

> On Tue, Feb 6, 2018 at 10:16 PM, Sergey Organov  wrote:
>> Johannes Schindelin  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 

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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Sixt]

Am 07.02.2018 um 07:16 schrieb Sergey Organov:

Johannes Schindelin  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.


Very well put. I share your concerns.

-- Hannes


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 
 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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Jacob Keller]

On Tue, Feb 6, 2018 at 10:16 PM, Sergey Organov  wrote:
> Johannes Schindelin  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 
> 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
>

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Johannes Schindelin  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.

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 
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

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi Eric,

On Fri, 19 Jan 2018, Eric Sunshine wrote:

> On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
>  wrote:
> > [...]
> > 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 -  `. And once this
> > mode has become stable and universally accepted, we can deprecate the
> > design mistake that was `--preserve-merges`.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > @@ -900,6 +900,7 @@ fi
> >  if test t != "$preserve_merges"
> >  then
> > git rebase--helper --make-script ${keep_empty:+--keep-empty} \
> > +   ${recreate_merges:+--recreate-merges} \
> 
> If the user specifies both --preserve-merges and --recreate-merges, it
> looks like --preserve-merges will take precedence.
> 
> Should git-rebase.sh have a mutual-exclusion check and error out if
> both are specified?

Maybe. I welcome you to contribute such a patch once recreate-merges made
it into the code base.

In other words: this would be premature optimization. We're not at that
stage yet.

Ciao,
Dscho

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Junio C Hamano]

Johannes Schindelin  writes:

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 8a861c1e0d6..1d061373288 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -368,6 +368,11 @@ The commit list format can be changed by setting the 
> configuration option
>  rebase.instructionFormat.  A customized instruction format will automatically
>  have the long commit hash prepended to the format.
>  
> +--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.
> +

It is sensible to postpone tackling "evil merges" in this initial
iteration of the series, and "manual amendments ... not preserved"
is a reasonable thing to document.  But do we want to say a bit more
about conflicting merges?  "conflict resolutions ... not preserved"
sounds as if it does not stop and instead record the result with
conflict markers without even letting rerere to kick in, which
certainly is not the impression you wanted to give to the readers.

I am imagining that it will stop and give control back to the end
user just like a conflicted "pick" would, and allow "rebase
--continue" to record resolution from the working tree, and just
like conflicted "pick", it would allow rerere() to help end users
recall previous resolution.

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Eric Sunshine]

On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
 wrote:
> [...]
> 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 -  `. And once this
> mode has become stable and universally accepted, we can deprecate the
> design mistake that was `--preserve-merges`.
>
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> @@ -900,6 +900,7 @@ fi
>  if test t != "$preserve_merges"
>  then
> git rebase--helper --make-script ${keep_empty:+--keep-empty} \
> +   ${recreate_merges:+--recreate-merges} \

If the user specifies both --preserve-merges and --recreate-merges, it
looks like --preserve-merges will take precedence.

Should git-rebase.sh have a mutual-exclusion check and error out if
both are specified?

> $revisions ${restrict_revision+^$restrict_revision} >"$todo" 
> ||
> die "$(gettext "Could not generate todo list")"
> diff --git a/git-rebase.sh b/git-rebase.sh
> @@ -262,6 +264,10 @@ do
> +   --recreate-merges)
> +   recreate_merges=t
> +   test -z "$interactive_rebase" && interactive_rebase=implied
> +   ;;
> --preserve-merges)
> preserve_merges=t
> test -z "$interactive_rebase" && interactive_rebase=implied