On Sun, Feb 5, 2023 at 9:41 AM Tao Klerks via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Tao Klerks <tao@klerks.biz>
>
> When "git pull" is called without a conflict-handling instruction or
> configuration, it displays a hint proposing "pull.rebase" and "pull.ff"
> config options for future handling.
>
> The hint offers three permanent settings, "merge", rebase", and "ff". The
> proposed command for "rebase" is "git config pull.rebase true".
>
> Unfortunately, this rebase configuration can easily lead to non-expert users
> accidentally rebasing not their own commits, instead others' commits, if the
> new commits they have locally before the "pull" include a merge of another
> branch, eg "main".
>
> Since 2018 in git version "2.18", it has supported a new rebase flag
> "--rebase-merges", with corresponding pull.rebase config option "merges".
> This new option is ideal for rebasing local work on "pull", as it will
> not "mangle"/flatten any local merge commits but rather recreate them.
>
> Change the pull conflict hint text to propose "pull.rebase merges" instead
> of "pull.rebase true", and "git pull --rebase=merges" instead of
> "git pull --rebase".
>
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
>     pull: conflict hint pull.rebase suggestion should offer "merges" vs
>     "true"
>
>     Hint change as proposed in
>     https://lore.kernel.org/git/xmqqa61uo3q0.fsf@gitster.g/
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1474%2FTaoK%2Ftao-fetch-rebase-hint-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1474/TaoK/tao-fetch-rebase-hint-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1474
>
>  builtin/pull.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 1ab4de0005d..535364fbb07 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -967,13 +967,13 @@ static void show_advice_pull_non_ff(void)
>                  "your next pull:\n"
>                  "\n"
>                  "  git config pull.rebase false  # merge\n"
> -                "  git config pull.rebase true   # rebase\n"
> +                "  git config pull.rebase merges # rebase\n"
>                  "  git config pull.ff only       # fast-forward only\n"
>                  "\n"
>                  "You can replace \"git config\" with \"git config --global\" to set a default\n"
> -                "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
> -                "or --ff-only on the command line to override the configured default per\n"
> -                "invocation.\n"));
> +                "preference for all repositories. You can also pass --rebase=merges,\n"
> +                "--no-rebase, or --ff-only on the command line to override the configured\n"
> +                "default per invocation.\n"));

Hi Tao, thank you for sharing your experiences with non-experts using
`git pull`. I am always curious to see how people who are learning Git
react to it, and I am very interested in making Git as straightforward
as possible.

I'm afraid I have several objections to this patch...

- The proposed wording is likely to further confuse novices. It's
asking the user to choose between the reconciliation strategies of
merging and rebasing, but then says to use the unintuitive combination
"rebase=merges" which sounds like it's going to make a merge commit at
the end of the branch anyway.

- The proposed wording makes it sound like there's something wrong
with doing a regular rebase, but that's not usually the case because
in practice a regular rebase is almost always equivalent to
rebase=merges. A regular rebase may even be what the user really
wants: For example, the user might choose to merge when pulling and
then change their mind and decide that they really wanted to rebase.
Repeating the pull with the regular -r or --rebase flag fixes the
mistake.

- `git pull -ri` (or its longer form `git pull --rebase=interactive`)
is generally more useful than `git pull --rebase=merges`, but once
rebase=merges has been specified, there's no way to specify
rebase=interactive also. Recommending rebase=merges steers people away
from rebase=interactive, hiding useful functionality from the user.

Now, this is not to say that there's no room for improvement. I like
the rebase=merges option and I wish everyone knew about it because
there are situations where it really is the best option. I suggest
leaving the existing text alone, but adding an additional paragraph,
something like:

Note that --rebase or pull.rebase=true will drop existing merge
commits and rebase all of the commits from all of the merged branches.
If you want to rebase but preserve existing merge commits, use
--rebase=merges or pull.rebase=merges instead.

-Alex

On Thu, Feb 16, 2023 at 4:22 AM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> - The proposed wording is likely to further confuse novices. It's
> asking the user to choose between the reconciliation strategies of
> merging and rebasing, but then says to use the unintuitive combination
> "rebase=merges"

My thesis, which you clearly disagree with, is that for this type of
situation, "rebase=merges" is not an "unintuitive combination", but
rather is "a plain and simple rebase". It is truly unfortunate that
git's history has led us to a place where this command is so awkwardly
named, I agree with that at least.

If there's an appetite for it, I would love to contribute to a
multi-year adventure to change git's behavior, little by little, until
the behavior of "rebase=merges" is the default, and the old behavior
becomes a different option like
"rebase=copy-merged-commits-to-flatten"

> which sounds like it's going to make a merge commit at
> the end of the branch anyway.

I can't quite tell whether you're referring to the naming of the
option (which I agree, sucks), or saying that it sounds *to you* like
it will make a merge commit. It will not make a merge commit unless
*you* previously made a merge commit. It will rebase your merge
commits, only if there are any that should be rebased.

If your concern is that we shouldn't be showing anyone the
"consistently reasonable rebase option" because it's confusingly named
wrt the "rebase option that experts understand and has a shorter
name", then let's figure out how to rename it. In the meantime, let's
help avoid people shooting themselves in the foot. A hint pointing at
the cool loaded gun lying on the mantelpiece is *not* helping users
avoid shooting themselves in the foot.

>
> - The proposed wording makes it sound like there's something wrong
> with doing a regular rebase, but that's not usually the case because
> in practice a regular rebase is almost always equivalent to
> rebase=merges.

The new proposed option will do the right thing in (almost?) all
cases. The previous option will make a horrible mess of things in some
(or, depending on the workflow, many!) cases.

In all the cases where the behavior is equivalent, that's great. In
almost all the cases where the behavior is different, the proposed new
behavior is superior (to anyone who needs that hint).

There is only one case that I know of in which the proposed new
behavior could be considered marginally worse:

* I have a local branch "feature-1", with some local work on it
* This is a shared branch, others are working on it also, so
"origin/feature-1" has a few other commits on it also (diverged)
* I create a derived branch, "feature-1-sub". I do some work on it
* I pull with rebase (--rebase or --rebase=merges, makes no
difference), so my original local feature-1 changes are rebased on top
of others' changes.
* I do some more work on (my local, rebased) "feature-1"
* I merge "feature-1-sub" into "feature-1"
** -> I now have duplicate commits in my history: the original local
"feature-1" work is in the history of "feature-1-sub", and it was/is
separately rebased in "feature-1"
* I "git pull --rebase" or  "git pull --rebase=merges":
** A "simple rebase" will automatically squash the duplicated commit(s)
** A "rebase with rebase-merges" will retain/recreate the merge
commit, and thereby retain the existence of a duplicated commit in the
commit graph.
(test script available upon request, I didn't want to spam the list with it)

While I agree "--rebase=merges" is not clearly superior, or could even
be worse in this one contrived case, I would argue that this is far
less harmful than the current "pathological case" of "--rebase", which
will happen far more easily and which I will outline again:

* There is a "main" branch with lots of commit activity from lots of
contributors
* There is a "feature-1" branch with a few contributors collaborating
on something that will be merged into "main" when ready
* These contributors are not experts - they don't coordinate on
rebasing "feature-1" every time they need to incorporate changes from
"main" - instead, they merge "main" into "feature-1" when that's
needed
* One of those contributors is tasked with doing the merge from main,
resolving conflicts, spot-checking, etc - the last merge from main was
10 days ago, 100 commits.
* They have a merge commit ready, tested
* They try to push "feature-1", but one of the other contributors has
added some work/commits, so the error tells them to "git pull" first
* They "git pull", get an error and follow the wrong advice, or they
followed the wrong advice in the preceding days/weeks - they end up
doing a "git pull --rebase" without knowing what that means for their
recent merge commit, and/or without even realizing that's how they
previously configured things
* Their "feature-1" branch now has 100 duplicated commits by arbitrary
"main" developers, but they haven't even necessarily noticed - they
may well be using a GUI that just congratulates them on a successful
pull
* They now push, successfully.
* Unless anyone looks at the commit graph of "feature-1" carefully,
no-one necessarily notices anything is wrong; the changes from "main"
are in "feature-1", as expected; it's just the lines connecting them
that are wrong
* Two weeks later, the team needs to merge in "main" again - and they
start to get all sorts of weird conflicts
** "Oh man, git sucks - I thought it was supposed to merge *better*
than that other stuff, but it's finding conflicts that have nothing to
do with us, all over the place!"
** "Hmm, this is weird - let's see what the git expert says"
** "Oh man, 'feature-1' needs to be rebuilt, and everyone working on
it needs to figure out how to rebase their work / their branch(es)
onto the new state"
** etc

> A regular rebase may even be what the user really
> wants: For example, the user might choose to merge when pulling and
> then change their mind and decide that they really wanted to rebase.
> Repeating the pull with the regular -r or --rebase flag fixes the
> mistake.

I don't understand the relevance of this example: No-one is suggesting
to forbid "merge-flattening rebases" - only avoiding the suggestion to
use them as the default for people who don't know what they are doing
(that's who the hints are *for*!)

The way you suggest this example, it feels like you think this might
be intuitive/predictable: "I chose the wrong thing, so I flip the
choice and I get the other outcome" - that's not true at all, because
if you flip the choice the other way (--rebase first, and then merge),
you get a completely different outcome! (especially if what you
accidentally rebased contained a merge of course - but even without
merges in the rebased history, doing a merge later does not yield
nearly the same outcome).

>
> - `git pull -ri` (or its longer form `git pull --rebase=interactive`)
> is generally more useful than `git pull --rebase=merges`, but once
> rebase=merges has been specified, there's no way to specify
> rebase=interactive also. Recommending rebase=merges steers people away
> from rebase=interactive, hiding useful functionality from the user.

I don't understand your argument here... Are you saying that users
reading "You can also pass --rebase" would have been more likely to
end up running "--rebase=interactive" than users reading "You can also
pass --rebase=merges"? I believe this to be a grave misreading of user
behavior, but I have no credentials to back up this belief.

People consistently, and unhesitantly, copy-paste the suggestions
offered to them. If you believe users should be running
"--rebase=interactive", then the new wording is no worse than the old.

Now, as to whether users should in fact typically be running
"--rebase=interactive" when doing a "git pull" - is there an option to
"preserve merges" in this interaction? For users who *do not ever
merge* your suggestion sounds... possibly-overbearing, but not wrong.
For users who *do* merge, it is plain wrong as far as I know.

>
> Now, this is not to say that there's no room for improvement. I like
> the rebase=merges option and I wish everyone knew about it because
> there are situations where it really is the best option. I suggest
> leaving the existing text alone, but adding an additional paragraph,
> something like:
>
> Note that --rebase or pull.rebase=true will drop existing merge
> commits and rebase all of the commits from all of the merged branches.
> If you want to rebase but preserve existing merge commits, use
> --rebase=merges or pull.rebase=merges instead.

My primary motivation with this pull request is to reduce the
incidences, out there in the world, of people copy-pasting "git config
pull.rebase true" into their command-line, and causing themselves
major headaches days or weeks later. The "--rebase=interactive" part
is secondary (to my concerns), because it's much less copy-pastable.

Your proposal does nothing for my concern, unfortunately - it leaves a
message that, overall, offers three copy-pastable options, two of
which are safe-enough, and one of which has substantial chances of
plunging you into a world of pain that you cannot comprehend. It is
plain wrong. We need to change it.

I am very happy to add the paragraph you proposed instead of changing
"--rebase" to "--rebase=interactive", but I would like to see a much
better suggestion as to how to address the harm of "git config
pull.rebase true".


Thanks for your feedback, and my apologies for the insistent response
- I'm having a hard time figuring out how to express just how *bad*
the existing copy-pastable suggestion in this hint is (in this day and
age), for users who merge - users who, I believe, make up the
significant majority of "corporate" developers at the very least, and
I suspect even the significant majority of git users out in the world.

I'm adding Johannes Schindelin to the thread in case he has the cycles
to weigh in - as the original author of what I would call "the better
way" (5 years ago now!), I'm sure he's more aware than most of its
limitations, and of any reasons why we *wouldn't* want to make the
change(s) I've suggested here.

Thanks,
Tao

On Thu, Feb 16, 2023 at 5:31 AM Tao Klerks <tao@klerks.biz> wrote:
>
> If there's an appetite for it, I would love to contribute to a
> multi-year adventure to change git's behavior, little by little, until
> the behavior of "rebase=merges" is the default, and the old behavior
> becomes a different option like
> "rebase=copy-merged-commits-to-flatten"

I know you had a lot to say in your last email, but I'd like to focus
on this point. I would be OK with the proposed patch if it were part
of a larger effort to make --rebase-merges the default behavior of
`git rebase`. That seems like an achievable goal, and I don't think it
would take multiple years, maybe one year at the most. The process
would look something like this:

1. Add a --no-rebase-merges option to `git rebase`.

2. Add a rebase.merges config option.

3. Add a warning to `git rebase` that appears if rebase.merges is
unset and neither --rebase-merges nor --no-rebase-merges is given. The
warning would advise the user that the default behavior of `git
rebase` will change in a future release and suggest setting
rebase.merges=no-rebase-cousins to get the new behavior now.

4. Change the `git pull` advice to recommend --rebase=merges and
pull.rebase=merges.

5. Wait a couple of releases.

6. Change the default behavior of `git rebase` to `git rebase
--rebase-merges` and the default behavior of `git pull --rebase` to
`git pull --rebase=merges`. At the same time, remove the warning from
`git rebase`. The old `git pull` behavior would still be available as
`git pull --rebase=true`.

7. Change the `git pull` advice to recommend the short and simple
--rebase option again (leaving the recommendation of
pull.rebase=merges for the config option).

Does that sound reasonable? I think I could lend a hand with steps 1-3.

-Alex

On Fri, Feb 17, 2023 at 4:15 AM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> I would be OK with the proposed patch if it were part
> of a larger effort to make --rebase-merges the default behavior of
> `git rebase`.

Heh, what would it take to convince you there is such an effort? :) -
sparse and minor as my contributions are, I certainly believe that is
a "natural" effort that I will do what I can to support.

> That seems like an achievable goal, and I don't think it
> would take multiple years, maybe one year at the most.

My estimate is based on the observation that there are still, several
years after --rebase-merges was introduced, git GUIs that don't handle
it right - eg Jetbrains IDEA:
https://youtrack.jetbrains.com/issue/IDEA-232160/Rebase-merges-is-not-properly-supported

This kind of functionality change should be slow, not because it's a
huge amount of work, but more because it takes time for the entire
ecosystem to adapt. Git releases basically-monthly, but many of the
systems that users use git with release far less often; similarly,
it's helpful to users who use a mix of current and older systems (I'm
looking at you, CentOS 7) for the introduction and recommendation of a
behavior change to come *long* before its defaulting.

> The process
> would look something like this:
>
> 1. Add a --no-rebase-merges option to `git rebase`.
>
> 2. Add a rebase.merges config option.

Yes and yes! I alluded to this in
https://lore.kernel.org/git/CAPMMpoj6E-85a59EaHD2aR_oKA=_u78qRV+wp8mqXkR39KctmA@mail.gmail.com/
but didn't feel I'd likely to make a solid change along these lines.

>
> 3. Add a warning to `git rebase` that appears if rebase.merges is
> unset and neither --rebase-merges nor --no-rebase-merges is given. The
> warning would advise the user that the default behavior of `git
> rebase` will change in a future release and suggest setting
> rebase.merges=no-rebase-cousins to get the new behavior now.
>

Makes sense to me!

> 4. Change the `git pull` advice to recommend --rebase=merges and
> pull.rebase=merges.
>

I'm not sure why this would be step 4 - I would (and did try to) make
it step 1 :)

> 5. Wait a couple of releases.
>

As I noted above, I believe it should be far more than a couple.

> 6. Change the default behavior of `git rebase` to `git rebase
> --rebase-merges` and the default behavior of `git pull --rebase` to
> `git pull --rebase=merges`. At the same time, remove the warning from
> `git rebase`. The old `git pull` behavior would still be available as
> `git pull --rebase=true`.
>

Makes sense to me!

> 7. Change the `git pull` advice to recommend the short and simple
> --rebase option again (leaving the recommendation of
> pull.rebase=merges for the config option).
>
> Does that sound reasonable? I think I could lend a hand with steps 1-3.
>

I'm sold, except insofar as I think the right approach is to move step
4 to be the first :)

Alex Henrie <alexhenrie24@gmail.com> writes:

> 1. Add a --no-rebase-merges option to `git rebase`.
>
> 2. Add a rebase.merges config option.
>
> 3. Add a warning to `git rebase` that appears if rebase.merges is
> unset and neither --rebase-merges nor --no-rebase-merges is given. The
> warning would advise the user that the default behavior of `git
> rebase` will change in a future release and suggest setting
> rebase.merges=no-rebase-cousins to get the new behavior now.
>
> 4. Change the `git pull` advice to recommend --rebase=merges and
> pull.rebase=merges.
>
> 5. Wait a couple of releases.

The above sounds like a standard "flip the default" dance executed
in the usual order.  I am not sure about the remainder but that is
not because I find anything wrong in it, but because I haven't
thought things through that far into the future ;-).

On Fri, Feb 17, 2023 at 4:15 AM Tao Klerks <tao@klerks.biz> wrote:
>
> On Fri, Feb 17, 2023 at 4:15 AM Alex Henrie <alexhenrie24@gmail.com> wrote:
> >
> > I would be OK with the proposed patch if it were part
> > of a larger effort to make --rebase-merges the default behavior of
> > `git rebase`.
>
> Heh, what would it take to convince you there is such an effort? :)

Doing steps 1-3 :)

> > 4. Change the `git pull` advice to recommend --rebase=merges and
> > pull.rebase=merges.
>
> I'm not sure why this would be step 4 - I would (and did try to) make
> it step 1 :)

The unintuitive syntax --rebase=merges makes a little more sense if
there is a warning in `git rebase` about it being a temporary
necessity to support a planned behavior change, and we're explicitly
committing to not expect users to use that syntax forever. It might be
a good idea to add a similar note to the `git pull` warning too.

-Alex

On Thu, Feb 16, 2023 at 8:02 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> On Thu, Feb 16, 2023 at 5:31 AM Tao Klerks <tao@klerks.biz> wrote:
> >
> > If there's an appetite for it, I would love to contribute to a
> > multi-year adventure to change git's behavior, little by little, until
> > the behavior of "rebase=merges" is the default, and the old behavior
> > becomes a different option like
> > "rebase=copy-merged-commits-to-flatten"
>
> I know you had a lot to say in your last email, but I'd like to focus
> on this point. I would be OK with the proposed patch if it were part
> of a larger effort to make --rebase-merges the default behavior of
> `git rebase`. That seems like an achievable goal, and I don't think it
> would take multiple years, maybe one year at the most. The process
> would look something like this:
>
> 1. Add a --no-rebase-merges option to `git rebase`.
>
> 2. Add a rebase.merges config option.
>
> 3. Add a warning to `git rebase` that appears if rebase.merges is
> unset and neither --rebase-merges nor --no-rebase-merges is given. The
> warning would advise the user that the default behavior of `git
> rebase` will change in a future release and suggest setting
> rebase.merges=no-rebase-cousins to get the new behavior now.
>
> 4. Change the `git pull` advice to recommend --rebase=merges and
> pull.rebase=merges.
>
> 5. Wait a couple of releases.
>
> 6. Change the default behavior of `git rebase` to `git rebase
> --rebase-merges` and the default behavior of `git pull --rebase` to
> `git pull --rebase=merges`. At the same time, remove the warning from
> `git rebase`. The old `git pull` behavior would still be available as
> `git pull --rebase=true`.
>
> 7. Change the `git pull` advice to recommend the short and simple
> --rebase option again (leaving the recommendation of
> pull.rebase=merges for the config option).
>
> Does that sound reasonable? I think I could lend a hand with steps 1-3.

One concern I have is that "--rebase-merges" itself has negative user
surprises in store.  In particular, "--rebase-merges", despite its
name, does not rebase merges.  It uses the existing author & commit
message info, but otherwise just discards the existing merge and
creates a new one.  Any information it contained about fixing
conflicts, or making adjustments to make the two branches work
together, is summarily and silently discarded.

My personal opinion would be adding such a capability should be step
2.5 in your list, though I suspect that would make Tao unhappy (it's a
non-trivial amount of work, unlike the other steps in your list).

On 18/02/2023 03:17, Elijah Newren wrote:
> On Thu, Feb 16, 2023 at 8:02 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
>>
>> On Thu, Feb 16, 2023 at 5:31 AM Tao Klerks <tao@klerks.biz> wrote:
>>>
>>> If there's an appetite for it, I would love to contribute to a
>>> multi-year adventure to change git's behavior, little by little, until
>>> the behavior of "rebase=merges" is the default, and the old behavior
>>> becomes a different option like
>>> "rebase=copy-merged-commits-to-flatten"
>>
>> I know you had a lot to say in your last email, but I'd like to focus
>> on this point. I would be OK with the proposed patch if it were part
>> of a larger effort to make --rebase-merges the default behavior of
>> `git rebase`. That seems like an achievable goal, and I don't think it
>> would take multiple years, maybe one year at the most. The process
>> would look something like this:
>>
>> 1. Add a --no-rebase-merges option to `git rebase`.
>>
>> 2. Add a rebase.merges config option.
>>
>> 3. Add a warning to `git rebase` that appears if rebase.merges is
>> unset and neither --rebase-merges nor --no-rebase-merges is given. The
>> warning would advise the user that the default behavior of `git
>> rebase` will change in a future release and suggest setting
>> rebase.merges=no-rebase-cousins to get the new behavior now.
>>
>> 4. Change the `git pull` advice to recommend --rebase=merges and
>> pull.rebase=merges.
>>
>> 5. Wait a couple of releases.
>>
>> 6. Change the default behavior of `git rebase` to `git rebase
>> --rebase-merges` and the default behavior of `git pull --rebase` to
>> `git pull --rebase=merges`. At the same time, remove the warning from
>> `git rebase`. The old `git pull` behavior would still be available as
>> `git pull --rebase=true`.
>>
>> 7. Change the `git pull` advice to recommend the short and simple
>> --rebase option again (leaving the recommendation of
>> pull.rebase=merges for the config option).
>>
>> Does that sound reasonable? I think I could lend a hand with steps 1-3.
> > One concern I have is that "--rebase-merges" itself has negative user
> surprises in store.  In particular, "--rebase-merges", despite its
> name, does not rebase merges.  It uses the existing author & commit
> message info, but otherwise just discards the existing merge and
> creates a new one.  Any information it contained about fixing
> conflicts, or making adjustments to make the two branches work
> together, is summarily and silently discarded.

That's a good point. Another potentially surprising behavior is that when I'm rebasing an integration branch with -rno-rebase-cousins then if one of the topic branches merged into the integration branch happens to share the same base as the integration branch itself the topic branch gets rebased as well. -rno-rebase-cousins is also slower that it needs to be because it creates a todo list that contains all the commits on the topic branches merged into the integration branch rather than just the merges. The commits on the topic branches are fast-forwarded rather than rewritten so long as they don't share the same base as the integration branch but it noticeably slower than using a todo list with just the merge commands.

> My personal opinion would be adding such a capability should be step
> 2.5 in your list, though I suspect that would make Tao unhappy (it's a
> non-trivial amount of work, unlike the other steps in your list).

I've got a couple of patches[1] that cherry-pick the merge if only one of the parents has changed. I've never tried upstreaming them as it is only a partial solution to the problem of rebasing merges but that approach should work well with "git pull --rebase=merges" as only the upstream side will have changed (when rebasing my git integration branch with that patch the merges are cherry-picked). They might make a useful starting point if anyone wants to try and improve the rebasing of merges.

Best Wishes

Phillip

[1] https://github.com/phillipwood/git/commits/rebase-cherry-pick-merges

On Sat, Feb 18, 2023 at 4:17 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Thu, Feb 16, 2023 at 8:02 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
> >
> > On Thu, Feb 16, 2023 at 5:31 AM Tao Klerks <tao@klerks.biz> wrote:
> > >
> > > If there's an appetite for it, I would love to contribute to a
> > > multi-year adventure to change git's behavior, little by little, until
> > > the behavior of "rebase=merges" is the default, and the old behavior
> > > becomes a different option like
> > > "rebase=copy-merged-commits-to-flatten"
> >
> > I know you had a lot to say in your last email, but I'd like to focus
> > on this point. I would be OK with the proposed patch if it were part
> > of a larger effort to make --rebase-merges the default behavior of
> > `git rebase`. That seems like an achievable goal, and I don't think it
> > would take multiple years, maybe one year at the most. The process
> > would look something like this:
> >
<SNIP>
> >
> > Does that sound reasonable? I think I could lend a hand with steps 1-3.
>
> One concern I have is that "--rebase-merges" itself has negative user
> surprises in store.  In particular, "--rebase-merges", despite its
> name, does not rebase merges.  It uses the existing author & commit
> message info, but otherwise just discards the existing merge and
> creates a new one.  Any information it contained about fixing
> conflicts, or making adjustments to make the two branches work
> together, is summarily and silently discarded.
>
> My personal opinion would be adding such a capability should be step
> 2.5 in your list, though I suspect that would make Tao unhappy (it's a
> non-trivial amount of work, unlike the other steps in your list).

I apologize for my ignorance here, but I'm not sure how this "does not
rebase merges" concern overlaps with the "pull.rebase" context I'm
most specifically concerned about.

I would have assumed that when merge commits are "dropped", as results
from the current "pull.rebase=true" option in the pull conflict
advice, any merge resolution information is *also* dropped - so there
is no loss to the user here in advising the use of
"pull.rebase=merges" instead.

Is your concern about the "pull.rebase=merges" advice change, or more
about the broader "let's encourage users to more explicitly choose
between traditional merge-dropping rebase and rebase-merges" change
Alex is advocating for as a precondition to "my" change :) ?

On Sat, Feb 18, 2023 at 5:39 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 18/02/2023 03:17, Elijah Newren wrote:
> >
> > One concern I have is that "--rebase-merges" itself has negative user
> > surprises in store.  In particular, "--rebase-merges", despite its
> > name, does not rebase merges.  It uses the existing author & commit
> > message info, but otherwise just discards the existing merge and
> > creates a new one.  Any information it contained about fixing
> > conflicts, or making adjustments to make the two branches work
> > together, is summarily and silently discarded.
>
> That's a good point. Another potentially surprising behavior is that
> when I'm rebasing an integration branch with -rno-rebase-cousins then if
> one of the topic branches merged into the integration branch happens to
> share the same base as the integration branch itself the topic branch
> gets rebased as well.

I've been trying to understand how this behavior is (potentially)
surprising - I imagine it's been discussed elsewhere but I'm having a
hard time understanding, sorry.

The situation you described is a boundary condition between two others, right?
* The topic branch could be branched from the integration branch
(potentially *after* some other change were made to the integration
branch, but not in this case) - in which case rebasing is what you
would expect
* The topic branch could be branched from the main branch (potentially
*before* the integration branch branched, but not in this case) - in
which case not rebasing is what you would expect.

If topic branched from main (at around the same time as integration),
it might be surprising that it rebases; if it branched from
integration (before that had any changes), then it is expected.

> -rno-rebase-cousins is also slower that it needs
> to be because it creates a todo list that contains all the commits on
> the topic branches merged into the integration branch rather than just
> the merges. The commits on the topic branches are fast-forwarded rather
> than rewritten so long as they don't share the same base as the
> integration branch but it noticeably slower than using a todo list with
> just the merge commands.

This seems improvable, but no worse than a plain legacy rebase (as
Alex's new patch would have it, "rebase-merges=drop"), right? Insofar
as we're discussing why it might make sense to avoid promoting this
over a plain rebase, I don't understand the concern.


>
> > My personal opinion would be adding such a capability should be step
> > 2.5 in your list, though I suspect that would make Tao unhappy (it's a
> > non-trivial amount of work, unlike the other steps in your list).
>
> I've got a couple of patches[1] that cherry-pick the merge if only one
> of the parents has changed. I've never tried upstreaming them as it is
> only a partial solution to the problem of rebasing merges but that
> approach should work well with "git pull --rebase=merges" as only the
> upstream side will have changed (when rebasing my git integration branch
> with that patch the merges are cherry-picked). They might make a useful
> starting point if anyone wants to try and improve the rebasing of merges.
>

This is awesome!

It feels like the first step towards the general strategy that was (I
believe) best described by Buga at
https://public-inbox.org/git/a0cc88d2-bfed-ce7b-1b3f-3c447d2b32da@gmail.com/
!

(unless I'm missing something, the result of this is exactly the same
as the result of that strategy, in these "simple" cases where it kicks
in)

The one concern I have with this is that, *if I understand correctly*,
it sometimes throws away the existing merge information, and sometimes
doesn't, and there's no easy way to know which it is at runtime. Would
adding a warning on stderr when a both-parents merge is encountered
(and any merge resolutions or related changes are still discarded) be
enough to make this shippable?

Are there *any* circumstances where the new cherry-picking behavior
introduced here wouldn't be the right thing to have happen?

Hi Tao

On 20/02/2023 08:03, Tao Klerks wrote:
> On Sat, Feb 18, 2023 at 5:39 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> On 18/02/2023 03:17, Elijah Newren wrote:
>>>
>>> One concern I have is that "--rebase-merges" itself has negative user
>>> surprises in store.  In particular, "--rebase-merges", despite its
>>> name, does not rebase merges.  It uses the existing author & commit
>>> message info, but otherwise just discards the existing merge and
>>> creates a new one.  Any information it contained about fixing
>>> conflicts, or making adjustments to make the two branches work
>>> together, is summarily and silently discarded.
>>
>> That's a good point. Another potentially surprising behavior is that
>> when I'm rebasing an integration branch with -rno-rebase-cousins then if
>> one of the topic branches merged into the integration branch happens to
>> share the same base as the integration branch itself the topic branch
>> gets rebased as well.
> > I've been trying to understand how this behavior is (potentially)
> surprising - I imagine it's been discussed elsewhere but I'm having a
> hard time understanding, sorry.
> > The situation you described is a boundary condition between two others, right?
> * The topic branch could be branched from the integration branch
> (potentially *after* some other change were made to the integration
> branch, but not in this case) - in which case rebasing is what you
> would expect
> * The topic branch could be branched from the main branch (potentially
> *before* the integration branch branched, but not in this case) - in
> which case not rebasing is what you would expect.
> > If topic branched from main (at around the same time as integration),
> it might be surprising that it rebases;

Yes that's what I was referring to, on the one hand it isn't surprising at all because both the topic and integration branch have the same base but on the other hand using no-rebase-cousins is supposed to stop the topic branches being rebased.

> if it branched from
> integration (before that had any changes), then it is expected.

Yes

>> -rno-rebase-cousins is also slower that it needs
>> to be because it creates a todo list that contains all the commits on
>> the topic branches merged into the integration branch rather than just
>> the merges. The commits on the topic branches are fast-forwarded rather
>> than rewritten so long as they don't share the same base as the
>> integration branch but it noticeably slower than using a todo list with
>> just the merge commands.
> > This seems improvable, but no worse than a plain legacy rebase (as
> Alex's new patch would have it, "rebase-merges=drop"), right? Insofar
> as we're discussing why it might make sense to avoid promoting this
> over a plain rebase, I don't understand the concern.

My concern is to have a good understanding of the issues around --rebase-merges before we start promoting it over a plain rebase. It is not a reason not to make the change but it does show --rebase-merges would benefit from some additional polish.

>>> My personal opinion would be adding such a capability should be step
>>> 2.5 in your list, though I suspect that would make Tao unhappy (it's a
>>> non-trivial amount of work, unlike the other steps in your list).
>>
>> I've got a couple of patches[1] that cherry-pick the merge if only one
>> of the parents has changed. I've never tried upstreaming them as it is
>> only a partial solution to the problem of rebasing merges but that
>> approach should work well with "git pull --rebase=merges" as only the
>> upstream side will have changed (when rebasing my git integration branch
>> with that patch the merges are cherry-picked). They might make a useful
>> starting point if anyone wants to try and improve the rebasing of merges.
>>
> > This is awesome!
> > It feels like the first step towards the general strategy that was (I
> believe) best described by Buga at
> https://public-inbox.org/git/a0cc88d2-bfed-ce7b-1b3f-3c447d2b32da@gmail.com/
> !
> > (unless I'm missing something, the result of this is exactly the same
> as the result of that strategy, in these "simple" cases where it kicks
> in)

Yes

> The one concern I have with this is that, *if I understand correctly*,
> it sometimes throws away the existing merge information, and sometimes
> doesn't, and there's no easy way to know which it is at runtime.

Right, there are two ways the existing merge can be thrown away.

 (i) The existing merge has conflicts when being cherry picked
     and so we redo the merge (that is a choice, we could present
     the user with the conflicts from the cherry-pick). It is
     possible that the merge succeeds where the cherry-pick failed
     but most of the time we'd stop because if the cherry-pick has
     conflicts the merge will probably have conflicts as well.

(ii) More than one parent has changed and so we redo the merge

> Would
> adding a warning on stderr when a both-parents merge is encountered
> (and any merge resolutions or related changes are still discarded) be
> enough to make this shippable?

I'm not sure. It works well enough for what I use it for (which is essentially "git pull --rebase") but sometimes cherry-picking and sometimes remerging does make it more complicated for users. If we printed a warning what is the user going to do? An experienced user can use the reflog to get back to the original state and redo the rebase with some break statements added in to let them fix up the merges. A less experienced user is going to think git lost their work.

> Are there *any* circumstances where the new cherry-picking behavior
> introduced here wouldn't be the right thing to have happen?

Not that I can think of

Best Wishes

Phillip

Hi Phillip,

On Sat, Feb 18, 2023 at 8:39 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 18/02/2023 03:17, Elijah Newren wrote:
> >
> > One concern I have is that "--rebase-merges" itself has negative user
> > surprises in store.  In particular, "--rebase-merges", despite its
> > name, does not rebase merges.  It uses the existing author & commit
> > message info, but otherwise just discards the existing merge and
> > creates a new one.  Any information it contained about fixing
> > conflicts, or making adjustments to make the two branches work
> > together, is summarily and silently discarded.
>
> That's a good point. Another potentially surprising behavior is that
> when I'm rebasing an integration branch with -rno-rebase-cousins then if
> one of the topic branches merged into the integration branch happens to
> share the same base as the integration branch itself the topic branch
> gets rebased as well. -rno-rebase-cousins is also slower that it needs
> to be because it creates a todo list that contains all the commits on
> the topic branches merged into the integration branch rather than just
> the merges. The commits on the topic branches are fast-forwarded rather
> than rewritten so long as they don't share the same base as the
> integration branch but it noticeably slower than using a todo list with
> just the merge commands.

Yeah, modifying rebase to accept a general range expression (instead
of assuming upstream..HEAD) would really help.  Then, to get just the
parts you are interested in, you could use a range with extra commit
exclusions and additional qualifiers like --ancestry-path=<commit> and
--first-parent.  In fact, you could also list multiple branches (none
of which necessarily fully contains any of the others) to replay
multiple branches at a time.  (See [2] for where I discuss this
more, though focusing on the --ancestry-path=<commit> part of it.).

But, it'd also fundamentally break existing workflows, so it might
have to be a new command, perhaps `git replay`.  However, there's
multiple other improvements needed in rebase (such as not wasting time
updating the working tree or index or reflog for every commit, or
wasting time writing N control files when we could move to 1 control
file, and allowing working on branches that aren't checked out) that I
think would likely also break compatibility, so maybe another command
is a good idea anyway[3].

[2] https://lore.kernel.org/git/CABPp-BHmj+QCBFDrH77iNfEU41V=UDu7nhBYkAbCsbXhshJzzw@mail.gmail.com/
[3] https://github.com/newren/git/blob/e84f5f3585fd770ed21f398d2ae5f96e90a51b1e/replay-design-notes.txt

> > My personal opinion would be adding such a capability should be step
> > 2.5 in your list, though I suspect that would make Tao unhappy (it's a
> > non-trivial amount of work, unlike the other steps in your list).
>
> I've got a couple of patches[1] that cherry-pick the merge if only one
> of the parents has changed. I've never tried upstreaming them as it is
> only a partial solution to the problem of rebasing merges but that
> approach should work well with "git pull --rebase=merges" as only the
> upstream side will have changed (when rebasing my git integration branch
> with that patch the merges are cherry-picked). They might make a useful
> starting point if anyone wants to try and improve the rebasing of merges.

I've actually put quite a bit of time into this problem.  I have
outlined what I think is a full solution to the rebasing of merges
problem space at [4], which expands on my earlier discussion with
Johannes on-list over at [5] (which in turn was a follow-up to
previous discussions that you, Johannes, and several others had years
ago).  If you're interested and have any thoughts on my plans for this
problem space, I'd love to hear it.  You tend to have very strong
insights on everything xdiff, sequencer, and rebasing related.  My
"replay" branch contains a partial implementation, but it's not really
usable for anything rebase-merges-related yet, so you'd mostly have to
go with my writeups.

A warning, though, that I won't be able to respond to feedback on this
topic very soon.  I will definitely get back to working on it, but
it's been much more challenging with more limited git time these days.
Unfortunately, the current economic environment reduces the number of
ways possible to extend the amount of time available for working on
Git, but one way or another I'll eventually get back to this problem
and implement my ideas, unless someone beats me to it.

[4] https://github.com/newren/git/blob/e84f5f3585fd770ed21f398d2ae5f96e90a51b1e/replay-design-notes.txt#L264-L341
[5] https://lore.kernel.org/git/CABPp-BHWVO5VRhr1-Ou60F1wjKzJZ1e_dC01Mmzs+qB9kGayww@mail.gmail.com/

On Mon, Feb 20, 2023 at 12:03 AM Tao Klerks <tao@klerks.biz> wrote:
>
> On Sat, Feb 18, 2023 at 5:39 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >
> > On 18/02/2023 03:17, Elijah Newren wrote:
> > >
[...]
> > > My personal opinion would be adding such a capability should be step
> > > 2.5 in your list, though I suspect that would make Tao unhappy (it's a
> > > non-trivial amount of work, unlike the other steps in your list).
> >
> > I've got a couple of patches[1] that cherry-pick the merge if only one
> > of the parents has changed. I've never tried upstreaming them as it is
> > only a partial solution to the problem of rebasing merges but that
> > approach should work well with "git pull --rebase=merges" as only the
> > upstream side will have changed (when rebasing my git integration branch
> > with that patch the merges are cherry-picked). They might make a useful
> > starting point if anyone wants to try and improve the rebasing of merges.
> >
>
> This is awesome!
>
> It feels like the first step towards the general strategy that was (I
> believe) best described by Buga at
> https://public-inbox.org/git/a0cc88d2-bfed-ce7b-1b3f-3c447d2b32da@gmail.com/
> !

The strategies described by Buga and others in that mega-thread were
suboptimal solutions, in my opinion.  Johannes went and implemented
some and found them wanting; see the thread over at
https://lore.kernel.org/git/nycvar.QRO.7.76.6.1804130002090.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/.
There were follow-ups with an improved strategy in the thread over at
https://lore.kernel.org/git/CABPp-BHWVO5VRhr1-Ou60F1wjKzJZ1e_dC01Mmzs+qB9kGayww@mail.gmail.com/
(Note that this route has also independently been discovered and
implemented in jj and found to work well, though it does handle
conflicts much differently).  And I've since improved the strategy
further at https://github.com/newren/git/blob/e84f5f3585fd770ed21f398d2ae5f96e90a51b1e/replay-design-notes.txt#L264-L341.
However, note that this isn't a case of merely performing the proper
series of merges, it needs some specialized logic and some new
capabilities at the xdiff level.