| Message ID | pull.1474.git.1675614276549.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pull: conflict hint pull.rebase suggestion should offer "merges" vs "true" | expand |
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.
On Sun, Feb 19, 2023 at 10:01 PM Tao Klerks <tao@klerks.biz> wrote: > > 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 :) ? When we teach new folks about git, and get to rebasing, there is a simple and easy rule to tell users: don't mix merges and rebases. (There's a minor exception there in that merges with the upstream branch are fine and rebasing can let you get rid of those otherwise ugly-and-frequent back-merges that users sometimes make.) Obviously, your users are ignoring that advice, and feeling pain. To be fair, the "RECOVERING FROM UPSTREAM REBASE" section of the rebase manual isn't that prominent, and perhaps your users didn't have more seasoned developers sharing this don't-mix-merges-and-rebases advice with them. (It seemed to me to be shared pretty widely and commonly, but perhaps we are relying on education from others too much and education is never uniform if not coming from the tool itself.) I understand you want to make it easier for users to avoid accidentally getting into this state. That's a valid concern and desire. I think we should improve the situation. However, on what timetable and at what cost to others? You're advocating we start advertising an alternate option, one which has some caveats and gotchas that are not going to be so easy to explain to users -- neither to new users, nor to folks who have been using Git for years. We could just bite the bullet and start explaining, but these caveats and gotchas are completely incidental to the implementation, and are in no-wise fundamental to the desired operation. I believe that switching to this new option is going to generate an awful lot of questions and surprises by users. It seems to me to be a really sad state of affairs to be recommending an option with known defects when (IMO) the solution is known. Can't we fix it first, then recommend it? Granted, this is a trade-off. You have users experiencing real pain. You want a solution now. I want to not recommend features with known implementation shortcomings and known solutions, until those solutions are implemented, and I know that will take a while. What to do here is a judgement call, and I was merely giving my opinion on the call to make. Other folks on the list might see things differently than I do.
On Sun, Feb 5, 2023 at 9:41 AM Tao Klerks via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Tao Klerks <tao@klerks.biz> > > 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". On Mon, Feb 20, 2023 at 10:21 AM Elijah Newren <newren@gmail.com> wrote: > > When we teach new folks about git, and get to rebasing, there is a > simple and easy rule to tell users: don't mix merges and rebases. > (There's a minor exception there in that merges with the upstream > branch are fine and rebasing can let you get rid of those otherwise > ugly-and-frequent back-merges that users sometimes make.) The "minor exception" is merging a topic branch into main, right? And the "ugly-and-frequent back-merges" are the merges from main into a topic branch? Tao, the primary motivation behind the `git pull` warning was to help prevent users from merging main into a topic branch when that's not what they really want to do. The fact that novices sometimes do that has been a point of pain for many people, including Linus Torvalds: See "Don't merge upstream code at random points" at [1] and "github creates absolutely useless garbage merges" at [2]. If you're seeing users merge main into topic branches without a good reason, that does sound like more of an education problem than a bad-defaults problem. We might still want to change the default to better support the more unusual cases, but if you're going for a quick win, it would be faster to teach users the wisdom of not mixing rebase and merge in the first place. -Alex [1] https://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html [2] https://lore.kernel.org/lkml/CAHk-=wjbtip559HcMG9VQLGPmkurh5Kc50y5BceL8Q8=aL0H3Q@mail.gmail.com/
On Mon, Feb 20, 2023 at 5:56 PM Elijah Newren <newren@gmail.com> wrote: > > 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/. Ah, thank you! I think you had mentioned this before, and I somehow lost track of this. At first I want to summarize this concern as "any strategy that treats a merge rebase as a *pair* of cherry-picks risks encountering nested/overlapping merge conflicts", but I must be understanding too superficially, as you then mention arbitrary conflict nesting (and I assume this is not about octopus merges). > 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. Understood - thanks for the update, and of course for all your continued work on this. Is it fair to say that, for the simple situations that Phillip's cherry-pick strategy *does* kick in for, the outcome should be exactly the same as the outcome of the replay strategy?
On Mon, Feb 20, 2023 at 6:21 PM Elijah Newren <newren@gmail.com> wrote: > > > When we teach new folks about git, and get to rebasing, there is a > simple and easy rule to tell users: don't mix merges and rebases. > (There's a minor exception there in that merges with the upstream > branch are fine and rebasing can let you get rid of those otherwise > ugly-and-frequent back-merges that users sometimes make.) Who is "we" here? When I search for the exact text "don't mix merges and rebases" in google, the only hit I get is this very email thread. Without the quotes, I get a similar-looking page title, but I don't understand whether the author's thesis is the same thing you're getting at - I don't think so: https://dev.to/jessekphillips/rebase-and-merge-don-t-mix-4aj > > Obviously, your users are ignoring that advice, and feeling pain. "Ignoring" is a strong (and in my opinion, strange) term to use here. They are *not seeing* that advice, and I think you can reasonably assume that many, or most, users will not see almost any of the advice you can possibly offer. As software designers I believe we all strive to set things up so you need to learn as little as possible to use something usefully, and safely. > To > be fair, the "RECOVERING FROM UPSTREAM REBASE" section of the rebase > manual isn't that prominent, and perhaps your users didn't have more > seasoned developers sharing this don't-mix-merges-and-rebases advice > with them. (It seemed to me to be shared pretty widely and commonly, > but perhaps we are relying on education from others too much and > education is never uniform if not coming from the tool itself.) My equivalent of this is "never rebase a shared branch unless you and your team know what you're doing". For most users I interact with, that translates to "don't use rebase at all unless you have a git veteran standing at your shoulder / on a screenshare with you". This advice is, again, not necessarily something that users even *see* before they start needing to use git. Maybe they should? Should we add a child lock on the executable, "this is likely to do very surprising things, please sign here that you have studied graph theory and really understand how this stuff works before you get to use it"? > I > understand you want to make it easier for users to avoid accidentally > getting into this state. That's a valid concern and desire. I think > we should improve the situation. > > However, on what timetable and at what cost to others? > > You're advocating we start advertising an alternate option, one which > has some caveats and gotchas that are not going to be so easy to > explain to users -- neither to new users, nor to folks who have been > using Git for years. What I'm trying to understand is whether or how these caveats and gotchas are *any worse than the status quo / than the current "pull.rebase=true" behavior*. I haven't understood any clear concrete ways in which this is true yet: * "pull.rebase=merges" throws away your merge conflict resolutions - so does "pull.rebase=merges", right?? * You might find it surprising that a same-merge-point branch gets rebased with the default "-rno-rebase-cousins" behavior... but "pull.rebase=true" will also do that! * You might be disappointed at the fact that an interactive --rebase-merges rebase fills your screen with stuff - but a "flattening" rebase does that too. * You might be disappointed at the fact that --rebase-merges takes a long time when fast-forwarding over the merge of a large amount of history - but a "flattening" rebase does that too. I'm not advocating for experienced users being by any means required to use this functionality in its less-than-perfect state - but I *am* arguing that foisting that less-than-ideal state on people who copy-paste a suggested command from a pull conflict hint is far better than allowing them to accidentally "flatten the history" of upstream-branch commits. > We could just bite the bullet and start > explaining, but these caveats and gotchas are completely incidental to > the implementation, and are in no-wise fundamental to the desired > operation. We already *do* explain, right? We've already retired --preserve-merges! > I believe that switching to this new option is going to > generate an awful lot of questions and surprises by users. It seems > to me to be a really sad state of affairs to be recommending an option > with known defects when (IMO) the solution is known. Can't we fix it > first, then recommend it? I guess maybe I'm misunderstanding your concern: * My main aim is to stop users shooting themselves in the foot * If making --preserve-merges the overall default for "git rebase" is the only or best way as Alex has proposed, great, let's do that. I personally would like to have the "use --rebase-merges by default when using "git rebase" option that Alex is proposing even if we don't do this, but even that is secondary to me compared with the "stop offering a rebase behavior that will cause users to duplicate upstream history unknowingly, at a blocking prompt where you're forcing them to pick *something*" objective that kicked this off. * If there is another, better way of removing this foot-gun, I'm happy to explore another direction Do you have a suggestion? Or would you advocate for *eventually* replacing the foot-gun with a more childsafe tool, when that tool is as sophisticated as we think it should eventually become? > > Granted, this is a trade-off. You have users experiencing real pain. > You want a solution now. I want to not recommend features with known > implementation shortcomings and known solutions, until those solutions > are implemented, and I know that will take a while. What to do here > is a judgement call, and I was merely giving my opinion on the call to > make. Other folks on the list might see things differently than I do. I think there are many options, and I'd love to understand which one you advocate for in the immediate term, with respect to the specific issue I noted: * Replace the pull conflict hint only, as I initially proposed * Engage on an "asap" replacement of default "git rebase" behavior to "--rebase-merges" by default * Change the pull conflict hint in some other way (that removes the copy-paste footgun) * Do nothing, accepting that we will revise all this in some future, and it's been like this for so long, what's wrong with a few more people hitting the classic issues? * Some other proposal for short-term relief of this very specific problem? I should note here, that for "my" users, setting the new config option Alex proposes in "rebase: add a config option for --rebase-merges" by default, in all their repos, is sufficient for me to ensure people will stop hurting themselves, and that's something I can easily do if/when the patch is accepted - but the main reason I hang out here is to try to advocate for users *like* mine, people who use git because it's the best or only game in town, rather than people like me who think it's so friggin awesome and are fascinated to learn all its arcane mysteries. In my environment, that's easily a 10:1 ratio. I suspect that's a reasonable reflection on the universe of git users generally.
On Mon, Feb 20, 2023 at 7:33 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > > Tao, the primary motivation behind the `git pull` warning was to help > prevent users from merging main into a topic branch when that's not > what they really want to do. The fact that novices sometimes do that > has been a point of pain for many people, including Linus Torvalds: > See "Don't merge upstream code at random points" at [1] and "github > creates absolutely useless garbage merges" at [2]. > > If you're seeing users merge main into topic branches without a good > reason, that does sound like more of an education problem than a > bad-defaults problem. I would disagree on two points: 1. The need for merging in the upstream varies project by project, user by user, etc. If you are working on a part of a system where you can reasonably assume the ground will not shift under your feet, awesome, lucky you! Many users are not so fortunate, and need to regularly ensure that their changes still make sense in the ever-changing upstream context. Regularly (whatever that means to you) merging in the upstream is the simplest way of achieving that. If you're working on your own or as part of a team that's happy to handle coordinated rebasing, then rebasing is a potentially-more-satisfying way of achieving the same end - either way, assuming that their changes will make sense in the upstream context is simply not a luxury many users can afford over any period of time. Now, you note that Linus advocates for merging specific points, because he doesn't respect you merging "random crap" from a branch called "linus" - that's fine, but many projects strive to keep a specific trunk branch "evergreen" in order to minimize late conflicts are maximize coordination - there's a pretty cool site about it: https://trunkbaseddevelopment.com/ - this is not really different to Linus' advice except that the goal is to make there *never* be "random crap" on the upstream. 2. The fact that the commit history of non-expert git users (those who should not be using rebase, especially in teams) are so often... spidery... is why the "Squash" option of pull requests / merge requests is so popular in centralized workflows (GitHub, GitLab, BitBucket, etc). If your project follows a "merge down, squash up" strategy with a well-CI-guarded evergreen trunk on a central server, there's simply no reason to *require* your users to become rebasing experts - you can let them use simple merge-based workflows, keep your trunk clean by squashing away their complex commit graphs, let them merge down whenever they need or want to, etc. > We might still want to change the default to > better support the more unusual cases, but Do we have any analysis/understanding of how common workflows like that of the git or linux projects are, vs github-style fork-based projects, vs straight-up single central server projects? I'm not sure what you mean by "unusual", but I don't think "avoid rebase unless you really know what you're doing, merge down at will, we will squash your contribution in the pull/merge request at the end anyway" is an unusual flow at all nowadays. > if you're going for a quick > win, it would be faster to teach users the wisdom of not mixing rebase > and merge in the first place. > "teach [...] wisdom" is a good one! No, seriously - of course I'm going to do the best I can to prevent my users from falling into the traps surrounding them - but my point here is that *we simply shouldn't have pointless traps*. Offering a command that can cause significant "harm" (time loss, frustration, etc), silently... just doesn't seem like a good idea. > [1] https://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html > [2] https://lore.kernel.org/lkml/CAHk-=wjbtip559HcMG9VQLGPmkurh5Kc50y5BceL8Q8=aL0H3Q@mail.gmail.com/
On Tue, Feb 21, 2023 at 8:40 AM Tao Klerks <tao@klerks.biz> wrote: > > 2. The fact that the commit history of non-expert git users (those who > should not be using rebase, especially in teams) are so often... > spidery... is why the "Squash" option of pull requests / merge > requests is so popular in centralized workflows (GitHub, GitLab, > BitBucket, etc). > > If your project follows a "merge down, squash up" strategy with a > well-CI-guarded evergreen trunk on a central server, there's simply no > reason to *require* your users to become rebasing experts - you can > let them use simple merge-based workflows, keep your trunk clean by > squashing away their complex commit graphs, let them merge down > whenever they need or want to, etc. The advantage to that workflow is that you don't have to teach users how to rebase. (Whether the actual process of merging or rebasing is easier, assuming that the user knows how to do both, is debatable and likely depends a lot on the particular situation.) The disadvantage is that even merge requests that seem like they only need one commit often turn into multiple commits, and squashing all of those commits together indiscriminately both makes it harder for the reviewer to follow the progression of steps the developer took and decreases the usefulness of tools like `git blame` and `git bisect`. For example, the patch series that I sent to add a rebase.merges option will be 3 or 4 commits in the end, and other developers have good reasons to ask me to keep those commits separate instead of squashing them all into a single patch. On top of that, if your developers get the impression that all projects on GitHub/GitLab/whatever use the same workflow, they are likely to cause headaches when they present spidery merge requests to other projects. If you are OK with those tradeoffs then that's fine, Git will support you. My point is simply that every workflow has its advantages and disadvantages, and there's no workflow that solves every problem. > Do we have any analysis/understanding of how common workflows like > that of the git or linux projects are, vs github-style fork-based > projects, vs straight-up single central server projects? I don't have any statistics (although I would love to see them if they exist), but I do know that all of these workflows are common enough that `git pull` can't assume what the user wants. The warning exists to try to prevent the user from shooting themself in the foot. > I'm not sure what you mean by "unusual", but I don't think "avoid > rebase unless you really know what you're doing, merge down at will, > we will squash your contribution in the pull/merge request at the end > anyway" is an unusual flow at all nowadays. The unusual cases are the ones where you mix merge and rebase on your own topic branch. Your developers did that accidentally (despite `git pull` trying to warn them) and suffered because of it, because it isn't well supported right now. I think we all agree that it should be better supported, we just disagree on how to get there. -Alex
Tao Klerks <tao@klerks.biz> writes: > 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/ > ! Being the provoker of all the fuss then, as well as the author of basic original method, I agree Buga has summarized and described all the ideas in existence at that time extremely well. > > (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. As far as I'm aware, it's not the case. The originally described method indeed misbehaved, but this simple mistake has been quickly fixed, and the description by Buga you've referenced already discusses updated version. > 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? Even if there are in fact such corner cases, we could make ourselves very cautious and stop even after non-conflicting rebase, if we detect that U1' and U2' don't match, and let user decide if the result is acceptable (similar to what rerere does on successful application of replayed resolutions). I also agree (in particular with Buga) that from the POV of user experience the method suggested by Phillip should be superior, as it emphasizes the natural dominance of the "current branch", as opposed to originally described symmetric method that is more suitable for formal analysis than for actual convenient implementation. Yet creating U1' and U2' from the original method could be useful for the purpose of checking for possible problems with automatic rebase that the user may need to be aware of. The biggest problem here, as I see it, is designing UI that'd make sense in the case of conflicts in multiple stages of the suggested algorithms, but I think we can simplify it for now by stopping and suggesting blind re-merge in case of any conflict but that on rebasing of changes to the first parent. Even this would be a huge step forward compared to silent drop of merge commits and blindly re-merging of updated parents. > > Are there *any* circumstances where the new cherry-picking behavior > introduced here wouldn't be the right thing to have happen? None that I'm aware off, but I admit I'm not familiar with later Elijah work on the subject, so I could be mistaken. I only got a sketchy look at what Elijah did, and it looks like advanced material to me. I'd incline to rather get solid implementation of basics first, probably using Phillip method, then consider advanced methods if practice reveals demands for further improvements. I'm afraid that there is no ideal general solution for the problem of rebasing merge commits, so we need to limit ourselves and get a practical one that has already been described. Overall, I'd love to finally have reliable Git behavior when rebasing merge commits, even though I've already got a habit to perform all the merges in 2 steps: auto-merge resolving textual conflicts only (if any), followed by a fixup for semantics conflicts (if any). Thanks, -- Sergey Organov
On Tue, Feb 21, 2023 at 7:01 AM Tao Klerks <tao@klerks.biz> wrote: > > On Mon, Feb 20, 2023 at 6:21 PM Elijah Newren <newren@gmail.com> wrote: > > [...] > > Obviously, your users are ignoring that advice, and feeling pain. > > "Ignoring" is a strong (and in my opinion, strange) term to use here. > They are *not seeing* that advice, and I think you can reasonably > assume that many, or most, users will not see almost any of the advice > you can possibly offer. As software designers I believe we all strive > to set things up so you need to learn as little as possible to use > something usefully, and safely. Yep, fair enough. > > We could just bite the bullet and start > > explaining, but these caveats and gotchas are completely incidental to > > the implementation, and are in no-wise fundamental to the desired > > operation. > > We already *do* explain, right? We've already retired --preserve-merges! But we didn't suggest either `--preserve-merges` or `--rebase-merges` to general users. The only ones who used it went looking for it. That's fundamentally different than recommending it to all new and many existing Git users. > > Granted, this is a trade-off. You have users experiencing real pain. > > You want a solution now. I want to not recommend features with known > > implementation shortcomings and known solutions, until those solutions > > are implemented, and I know that will take a while. What to do here > > is a judgement call, and I was merely giving my opinion on the call to > > make. Other folks on the list might see things differently than I do. > > I think there are many options, and I'd love to understand which one > you advocate for in the immediate term, with respect to the specific > issue I noted: > > * Replace the pull conflict hint only, as I initially proposed > * Engage on an "asap" replacement of default "git rebase" behavior to > "--rebase-merges" by default > * Change the pull conflict hint in some other way (that removes the > copy-paste footgun) > * Do nothing, accepting that we will revise all this in some future, > and it's been like this for so long, what's wrong with a few more > people hitting the classic issues? > * Some other proposal for short-term relief of this very specific problem? My personal opinion is that we should avoid long term problems for users and maintainers of rebasing merges by pushing it too early to too many folks. For short term relief, I would suggest some mixture of the following are worth looking into: * Attempt to improve the message shown to users, perhaps referring them to somewhere in the docs that point out advantages and disadvantages of each choice. * Possibly make the message shown to users be "smart" rather than hardcoded. For example, you could check the local-only portion of history; if there are no merge commits, or if the upstream branch is "origin/main" and the only merges within the local-only history are 'Merge branch "origin/main"...' then suggesting a regular rebase is fine. If there are other merges, then adapt the wording (and perhaps in that special case, actually bringing up rebasing merges is okay, though it'd still be nice if the docs with advantages/disadvantages pointed out its shortcomings). This might be an expensive check, but if only users who haven't configured pull.<whatever> have to pay for it, then perhaps it's a useful thing to spend cycles on. > I should note here, that for "my" users, setting the new config option > Alex proposes in "rebase: add a config option for --rebase-merges" by > default, in all their repos, is sufficient for me to ensure people > will stop hurting themselves, and that's something I can easily do > if/when the patch is accepted - but the main reason I hang out here is > to try to advocate for users *like* mine, people who use git because > it's the best or only game in town, rather than people like me who > think it's so friggin awesome and are fascinated to learn all its > arcane mysteries. In my environment, that's easily a 10:1 ratio. I > suspect that's a reasonable reflection on the universe of git users > generally. Yes, I understand. It's frustrating when something you need isn't there and we only have a suboptimal approximation. I want to make things better and have put a lot of time into it; some things that already went into merge-ort were designed around this problem space and I'm planning to do more here. But, also, remember that I'm only one voice among many. Others may disagree with me and agree with you on pushing this earlier.
On Wed, Feb 22, 2023 at 6:27 AM Sergey Organov <sorganov@gmail.com> wrote: > > Tao Klerks <tao@klerks.biz> writes: > > > 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: [...] > I also agree (in particular with Buga) that from the POV of user > experience the method suggested by Phillip should be superior, as it > emphasizes the natural dominance of the "current branch", as opposed to > originally described symmetric method that is more suitable for formal > analysis than for actual convenient implementation. Yet creating U1' and > U2' from the original method could be useful for the purpose of checking > for possible problems with automatic rebase that the user may need to be > aware of. > > The biggest problem here, as I see it, is designing UI that'd make sense > in the case of conflicts in multiple stages of the suggested algorithms, > but I think we can simplify it for now by stopping and suggesting blind > re-merge in case of any conflict but that on rebasing of changes to the > first parent. Even this would be a huge step forward compared to silent > drop of merge commits and blindly re-merging of updated parents. I'm not so sure it's a huge step forward. Or even a step forward. Dscho actually implemented the old proposals and tried them out, as mentioned in the threads I linked to. The results on balance were significantly worse to him than just throwing away the previous merge resolution information and redoing the merge from scratch. He really wanted a better solution, but the previous proposals didn't provide it. This newer approximation, while more careful about only attempting to run in specific cases and having some good ideas to improve the user experience, still builds on the problematic foundations in those old suggestions (namely, cherry-picking merges relative to either of their parents). I think it isn't careful enough about the subset of cases where those problematic foundations can work right. > > Are there *any* circumstances where the new cherry-picking behavior > > introduced here wouldn't be the right thing to have happen? Yes, I can think of one fairly readily: Do an interactive rebase and drop one or more commits on the side-branch being merged. This cherry-picking of merges would reinstate those dropped changes via silently squashing them into the merge commit itself, making for a rather evil merge. > None that I'm aware off, but I admit I'm not familiar with later Elijah > work on the subject, so I could be mistaken. I only got a sketchy look > at what Elijah did, and it looks like advanced material to me. I'd > incline to rather get solid implementation of basics first, probably > using Phillip method, then consider advanced methods if practice reveals > demands for further improvements. That'd be fine if there's another solution that can provide a "solid implementation of the basics"; I've not seen another proposal that can yet. > I'm afraid that there is no ideal general solution for the problem of > rebasing merge commits. Sometimes problems aren't generically solvable. However, sometimes the problem is solvable, but folks so far have only provided solutions built on a faulty basis, or that were only designed for special cases, or that only look at a subset of the problem space. I think rebasing merges falls into the latter category, and that the prior proposals were just off the mark. Granted, I haven't implemented my proposal yet and I might discover more issues when I do, but I'm optimistic. It just really needs some good uninterrupted time, and my Git time comes in highly interrupted occasional spurts these days (and with new short-term priorities being inserted based on other things that come up on the mailing list and from elsewhere to boot). But I'll get to it one way or another.
Elijah Newren <newren@gmail.com> writes: > On Wed, Feb 22, 2023 at 6:27 AM Sergey Organov <sorganov@gmail.com> wrote: >> >> Tao Klerks <tao@klerks.biz> writes: >> >> > 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: > > [...] > >> I also agree (in particular with Buga) that from the POV of user >> experience the method suggested by Phillip should be superior, as it >> emphasizes the natural dominance of the "current branch", as opposed to >> originally described symmetric method that is more suitable for formal >> analysis than for actual convenient implementation. Yet creating U1' and >> U2' from the original method could be useful for the purpose of checking >> for possible problems with automatic rebase that the user may need to be >> aware of. >> >> The biggest problem here, as I see it, is designing UI that'd make sense >> in the case of conflicts in multiple stages of the suggested algorithms, >> but I think we can simplify it for now by stopping and suggesting blind >> re-merge in case of any conflict but that on rebasing of changes to the >> first parent. Even this would be a huge step forward compared to silent >> drop of merge commits and blindly re-merging of updated parents. > > I'm not so sure it's a huge step forward. Or even a step forward. Git currently throws away my precious merges! Silently! How it's not a step forward to stop doing this?! Sorry for getting that heated :) Git is well-known for being extremely careful with user content, and there is the only case where it fails miserably: rebasing merges. The above method will simply fix this long-standing deficiency that is even more dangerous as users do trust Git so much. I can only tell that I, for example, will definitely benefit a lot once it is implemented, as currently a rebase containing merge is at roughly the same level of risk as "cvs update" was in the old days: run and keep your fingers crossed. Well, with Git it's unless you are careful to do 2-step merge-fixup thingy every time you merge, that is basically just poor man attempt at fighting long-standing Git weakness. > Dscho actually implemented the old proposals and tried them out, as > mentioned in the threads I linked to. The results on balance were > significantly worse to him than just throwing away the previous merge > resolution information and redoing the merge from scratch. He really > wanted a better solution, but the previous proposals didn't provide > it. OTOH, Buga has sketched the proposals, confirmed problem with my original one (that Dscho predicted), then sketched the update I came up with, and showed it does work in common cases as expected. That said, I'm almost sure that for any method of rebasing and/or merging of whatever, one motivated enough will be able to find corner cases where the method fails, yet we do have both merges and rebases in Git, and rebasing of merges falls to the same category. We need them. Merges need to be properly rebased, not silently replaced with (different) merges, unless user asks for re-merge explicitly. To me Dscho (or anybody else) finding rough cases is an expected outcome, and is not a convincing argument against the feature. As for Dscho results specifically, I've got an impression that he never needed rebasing of merges in the first place, and re-merging always suited him just fine, so it'd be rather a surprise if rebasing of merges suddenly started to work better for his needs and workflows once he has implemented it. That said, when better method(s) of rebasing of merges will be found, I'm sure they'll be adopted, but for now I do believe we need something reliable that has been checked to actually work for common cases, as blind re-merging simply does not, and I still suspect the best choice for the time being is Phillip's incremental method. Overall, I'm still in desperate need for my precious merge-the-commits be rebased, and not replaced with Git idea of how merge commit would look if [current version of] 'git-merge' algorithm merged my branch in [using Git current default settings]. It's my dream that Git finally stops silently substituting a result of 'git-merge' (just-a-helper-operation intended to simplify creation of merge commits) for actual merge-the-commit that is part of my content. Best regards, -- Sergey Organov
On Fri, Feb 24, 2023 at 2:06 PM Sergey Organov <sorganov@gmail.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > On Wed, Feb 22, 2023 at 6:27 AM Sergey Organov <sorganov@gmail.com> wrote: > >> I also agree (in particular with Buga) that from the POV of user > >> experience the method suggested by Phillip should be superior. > >> [...] > >> Even this would be a huge step forward compared to silent > >> drop of merge commits and blindly re-merging of updated parents. > > > > I'm not so sure it's a huge step forward. Or even a step forward. > > Git currently throws away my precious merges! Silently! How it's not a > step forward to stop doing this?! Sorry for getting that heated :) I totally agree with you that we have a big problem. No need to convince me on that. :-) But having a big problem does not imply we have to implement and ship the first proposal that comes along to change things. Or second, or third. Such proposals might actually make things even worse. You correctly point out that we do not need to require perfection, but we can and should require that the proposed solutions not only make some things better but that they make things better overall. And in order to convincingly persuade others to adopt various proposals, we should be aware of what the advantages and shortcomings are...at least the ones that have already been discovered and publicized, and be able to talk about those shortcomings candidly. > As for Dscho results specifically, I've got an impression that he never > needed rebasing of merges in the first place, and re-merging always > suited him just fine, so it'd be rather a surprise if rebasing of merges > suddenly started to work better for his needs and workflows once he has > implemented it. Are you serious? You're claiming the author of --preserve-merges; and the author of --rebase-merges; and someone who actually implemented the ideas you, Buga, and Phillip were all discussing to improve rebasing of merges[1]; and who maintains a project (Git for Windows) that has countless branches with hundreds of commits and myriad merge points and needs to rebase the whole lot as Git is updated...is someone who doesn't actually care about rebasing of merges? I thought you had tried to read up on this subject and were commenting in good faith, but I'm starting to have my doubts. Please, go read at least [1] to see Johannes comments about how the prior proposals don't work beyond simple cases. He didn't discard those ideas because he didn't care about the useful information in merge commits, he discarded them because in practice those ideas resulted in behavior that was *even worse* than the current big problems. [1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.1804130002090.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/ [...] > for now I do believe we need something > reliable that has been checked to actually work for common cases, as > blind re-merging simply does not. I agree with you. The word "reliable" is particularly key, and IMO rules out any suggestion that involves applying the diff between a merge commit and either of its parents. Not only do I think it's the wrong solution theoretically, I also think they have empirically been shown to provide problems that many will consider to be as bad or worse than our current poison. I obviously don't have veto power or anything close to it, but in my opinion any solution based on those ideas do not meet the threshold bar for inclusion in Git and I'll raise my voice against them. Solutions based on other ideas are fair game. Heck, I've proposed one and I know of simpler variants to my proposal. Other solutions may exist too. But can we stop pushing already discredited proposals and instead reach for something that has a more solid foundation?
Elijah Newren <newren@gmail.com> writes: > On Fri, Feb 24, 2023 at 2:06 PM Sergey Organov <sorganov@gmail.com> wrote: >> >> Elijah Newren <newren@gmail.com> writes: [...] > Please, go read at least [1] to see Johannes comments about how the > prior proposals don't work beyond simple cases. It's exactly handling of simple cases that we need most. We can get fancy afterwards, if feasible. Thanks, -- Sergey Organov
On Sat, Feb 25, 2023 at 7:15 AM Sergey Organov <sorganov@gmail.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > On Fri, Feb 24, 2023 at 2:06 PM Sergey Organov <sorganov@gmail.com> wrote: > >> > >> Elijah Newren <newren@gmail.com> writes: > > [...] > > > Please, go read at least [1] to see Johannes comments about how the > > prior proposals don't work beyond simple cases. > > It's exactly handling of simple cases that we need most. We can get > fancy afterwards, if feasible. If we can handle just the simple cases without making common cases significantly worse, that'd be a potential path forward. Any proposal involving the diff between a merge commit and either of its parents (or an equivalent such as a three-way merge involving the merge commit and one of its parents) doesn't achieve that, IMO.
Elijah Newren <newren@gmail.com> writes: > On Sat, Feb 25, 2023 at 7:15 AM Sergey Organov <sorganov@gmail.com> wrote: >> >> Elijah Newren <newren@gmail.com> writes: >> >> > On Fri, Feb 24, 2023 at 2:06 PM Sergey Organov <sorganov@gmail.com> wrote: >> >> >> >> Elijah Newren <newren@gmail.com> writes: >> >> [...] >> >> > Please, go read at least [1] to see Johannes comments about how the >> > prior proposals don't work beyond simple cases. >> >> It's exactly handling of simple cases that we need most. We can get >> fancy afterwards, if feasible. > > If we can handle just the simple cases without making common cases > significantly worse, that'd be a potential path forward. Any proposal > involving the diff between a merge commit and either of its parents > (or an equivalent such as a three-way merge involving the merge commit > and one of its parents) doesn't achieve that, IMO. Except the method discussed does achieve exactly that according to the evidence gathered at the time of debates, and here is confirmation (from Johannes himself) from the reference you provided: "This strategy, while it performed well in my initial tests (and in Buga's initial tests, too), *does* involve more than one 3-way merge, and therefore it risks something very, very nasty: *nested* merge conflicts." So, overall, the method performs well in general, and we just need to avoid driving ourselves into nested merge conflicts, as resolving them is beyond capabilities of most human beings. Setting this back into perspective, in comparison to blind re-merge, that fails to keep user changes even when no conflicts at all exist, and even when it's applied at the same place in the history, the discussed method is a *huge* step forward, especially if re-merge is kept as a fallback strategy. P.S. BTW, where this hate for using of diffs with respect to parents come from, I wonder, provided we do use them all the time anyway? Thanks, -- Sergey Organov
On Sun, Feb 26, 2023 at 1:29 AM Sergey Organov <sorganov@gmail.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > On Sat, Feb 25, 2023 at 7:15 AM Sergey Organov <sorganov@gmail.com> wrote: > >> > >> Elijah Newren <newren@gmail.com> writes: > >> > >> > On Fri, Feb 24, 2023 at 2:06 PM Sergey Organov <sorganov@gmail.com> wrote: > >> >> > >> >> Elijah Newren <newren@gmail.com> writes: > >> > >> [...] > >> > >> > Please, go read at least [1] to see Johannes comments about how the > >> > prior proposals don't work beyond simple cases. > >> > >> It's exactly handling of simple cases that we need most. We can get > >> fancy afterwards, if feasible. > > > > If we can handle just the simple cases without making common cases > > significantly worse, that'd be a potential path forward. Any proposal > > involving the diff between a merge commit and either of its parents > > (or an equivalent such as a three-way merge involving the merge commit > > and one of its parents) doesn't achieve that, IMO. > > Except the method discussed does achieve exactly that according to the > evidence gathered at the time of debates, and here is confirmation (from > Johannes himself) from the reference you provided: I'm glad you read it. :-) > "This strategy, while it performed well in my initial tests (and in > Buga's initial tests, too), *does* involve more than one 3-way merge, > and therefore it risks something very, very nasty: *nested* merge > conflicts." > > So, overall, the method performs well in general, Jumping from "performed well on initial tests" to "performs well in general" seems to me to be quite a large and unwarranted logical leap. > and we just need to > avoid driving ourselves into nested merge conflicts I'm glad you're discussing a disadvantage and how to address it, but I don't understand how you can jump to the implication that this is the only one. > Setting this back into perspective, in comparison to blind re-merge, > that fails to keep user changes even when no conflicts at all exist, and > even when it's applied at the same place in the history, the discussed > method is a *huge* step forward, especially if re-merge is kept as a > fallback strategy. The use of superlatives and asterisks doesn't change my opinion; I'm still skeptical that the given strategy is overall a step forward, let alone a large one. (I do agree we have a huge problem and thus that a huge step forward theoretically could be taken, I just don't see this as it.) > P.S. BTW, where this hate for using of diffs with respect to parents > come from, I wonder, provided we do use them all the time anyway? I have no hate for such diffs; I just firmly believe they are inappropriate as a solution for the particular problem space being discussed. But I've stated that more than enough, and no one is producing patches on this topic right now, so I'll drop out of this thread. I still believe in my proposed solution, and I'll implement it as I get time for it.
Elijah Newren <newren@gmail.com> writes: > On Sun, Feb 26, 2023 at 1:29 AM Sergey Organov <sorganov@gmail.com> wrote: >> >> Elijah Newren <newren@gmail.com> writes: >> >> > On Sat, Feb 25, 2023 at 7:15 AM Sergey Organov <sorganov@gmail.com> wrote: >> >> >> >> Elijah Newren <newren@gmail.com> writes: >> >> >> >> > On Fri, Feb 24, 2023 at 2:06 PM Sergey Organov <sorganov@gmail.com> wrote: >> >> >> >> >> >> Elijah Newren <newren@gmail.com> writes: >> >> >> >> [...] >> >> >> >> > Please, go read at least [1] to see Johannes comments about how the >> >> > prior proposals don't work beyond simple cases. >> >> >> >> It's exactly handling of simple cases that we need most. We can get >> >> fancy afterwards, if feasible. >> > >> > If we can handle just the simple cases without making common cases >> > significantly worse, that'd be a potential path forward. Any proposal >> > involving the diff between a merge commit and either of its parents >> > (or an equivalent such as a three-way merge involving the merge commit >> > and one of its parents) doesn't achieve that, IMO. >> >> Except the method discussed does achieve exactly that according to the >> evidence gathered at the time of debates, and here is confirmation (from >> Johannes himself) from the reference you provided: > > I'm glad you read it. :-) In fact I didn't read it, I rather re-read it ;-) (I'm in the CC list there, so it should not have been a surprise I did read it then.) > >> "This strategy, while it performed well in my initial tests (and in >> Buga's initial tests, too), *does* involve more than one 3-way merge, >> and therefore it risks something very, very nasty: *nested* merge >> conflicts." >> >> So, overall, the method performs well in general, > > Jumping from "performed well on initial tests" to "performs well in > general" seems to me to be quite a large and unwarranted logical leap. There were quite a few tests performed and methods were polished before Johannes has been persuaded to give the feature a try, some of the tests being complex enough, and both methods did perform rather well. That is what he calls "initial tests" there I believe, and then he found the case that is very important to him, but lead him to nested merge conflicts, that is indeed quite bad. > >> and we just need to avoid driving ourselves into nested merge >> conflicts > > I'm glad you're discussing a disadvantage and how to address it, but I > don't understand how you can jump to the implication that this is the > only one. Well, it was you who gave me the reference to comment on, and that was the only disadvantage I was able to find being discussed there. I also don't recall any other objections back then when the problem has been discussed a lot. >> Setting this back into perspective, in comparison to blind re-merge, >> that fails to keep user changes even when no conflicts at all exist, and >> even when it's applied at the same place in the history, the discussed >> method is a *huge* step forward, especially if re-merge is kept as a >> fallback strategy. > > The use of superlatives and asterisks doesn't change my opinion; I'm > still skeptical that the given strategy is overall a step forward, let > alone a large one. You just repeat saying the same thing, without any further arguments? OK, thank you for your opinion anyway. > (I do agree we have a huge problem and thus that a huge step forward > theoretically could be taken, I just don't see this as it.) It works. Really. > >> P.S. BTW, where this hate for using of diffs with respect to parents >> come from, I wonder, provided we do use them all the time anyway? > > I have no hate for such diffs; I just firmly believe they are > inappropriate as a solution for the particular problem space being > discussed. From my POV particular problem space (rebasing commits) already uses the diffs, and only them, that's why I can't figure how you end up coming to such conclusion. > > But I've stated that more than enough, and no one is producing patches > on this topic right now, so I'll drop out of this thread. OK, I participate only in hope that there will be somebody who actually cares enough to implement it. Maybe it will be me, maybe not, and I already got it that neither you nor the original author of git-rebase are interested. > I still believe in my proposed solution, and I'll implement it as I > get time for it. Sure it'd be nice. Fortunately there is nothing mutually exclusive here. Thanks, -- Sergey Organov
Note: I'm not talking about rebasing merges anymore in this thread, but I thought there was a useful how-we're-communicating subthread that's worth addressing to see if we can make that part work better... On Mon, Feb 27, 2023 at 9:17 AM Sergey Organov <sorganov@gmail.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > On Sun, Feb 26, 2023 at 1:29 AM Sergey Organov <sorganov@gmail.com> wrote: > >> > >> Elijah Newren <newren@gmail.com> writes: > >> > >> > On Sat, Feb 25, 2023 at 7:15 AM Sergey Organov <sorganov@gmail.com> wrote: > >> >> > >> >> Elijah Newren <newren@gmail.com> writes: > >> >> > >> >> > On Fri, Feb 24, 2023 at 2:06 PM Sergey Organov <sorganov@gmail.com> wrote: > >> >> >> > >> >> >> Elijah Newren <newren@gmail.com> writes: > >> >> > >> >> [...] > >> >> > >> >> > Please, go read at least [1] to see Johannes comments about how the > >> >> > prior proposals don't work beyond simple cases. [...] > >> Except the method discussed does achieve exactly that according to the > >> evidence gathered at the time of debates, and here is confirmation (from > >> Johannes himself) from the reference you provided: > > > > I'm glad you read it. :-) > > In fact I didn't read it, I rather re-read it ;-) > > (I'm in the CC list there, so it should not have been a surprise I did > read it then.) I knew you were on the CC, I just didn't believe at the time that you could have read that email and still claimed that "As for Dscho results specifically, I've got an impression that he never needed rebasing of merges in the first place", so I assumed you had skipped that email or only lightly skimmed it. I'm still quite surprised, but clearly my assumption that you read the email was wrong. Sorry about that. > >> Setting this back into perspective, in comparison to blind re-merge, > >> that fails to keep user changes even when no conflicts at all exist, and > >> even when it's applied at the same place in the history, the discussed > >> method is a *huge* step forward, especially if re-merge is kept as a > >> fallback strategy. > > > > The use of superlatives and asterisks doesn't change my opinion; I'm > > still skeptical that the given strategy is overall a step forward, let > > alone a large one. > > You just repeat saying the same thing, without any further arguments? > OK, thank you for your opinion anyway. That exactly mirrors how I've felt about your emails in this thread. You are right that I'm not going into detail either...but why would I? * I'm not trying to convince you to implement these ideas or change your own implementation, especially since: * You've previously said you aren't even planning on working on this[1]. Of course, you can easily ask why I would think you might provide details when I was not providing any. Well, from my viewpoint: * You did say you were hoping someone else would work on this problem[1,2,& end of this email], and I've expressed interest in working on the problem space. * If you want someone to work on your ideas, using your particular favored approach, for free, then you need to convince them that your approach is worth investing in * I have read the old proposals, in detail, and stated I don't believe in them. * You didn't try to address my concerns beyond simply reasserting that the ideas in the original proposals were good, and seemed to be more interested in discrediting or minimizing my concerns (e.g. asking why I "hated diffs from merge to either parent") than in learning about or addressing them. I think your intentions are good (you're trying to solve a big problem in Git), I'm just a little worried about the execution (e.g. brushing aside my concerns so folks won't pay attention to them while actively recruiting eager contributors, with the plan to send them down what I believe is a dead-end path). If it was just you going down this path, I wouldn't be so concerned. Personally, I pursue a *lot* of my own bad ideas, and learn in the process that they were bad. Also, if you showed some willingness to entertain that I might be right that the old proposals are bad, and could communicate that to new contributors and let them decide, I would have dropped out of the thread sooner and just let you do your thing. The way you've responded in this thread doesn't seem unique to our interactions; it reminds me of an interaction you had with Junio at [3]. He suggested there were some code issues. You could have asked what they were and maybe learned how to improve things. Or maybe you could have learned that he just had a specific misunderstanding which could have been corrected if you asked some questions to find out what he was thinking. Instead, you simply asserted that things were fine and dismissed his concerns. I think it was a lost opportunity. Now, it's fully possible here that I've misunderstood your purpose; if so, I apologize. The above was the understanding I was working off of; maybe knowing that will help you understand my responses. [1] stated in final paragraph of https://lore.kernel.org/git/87zgkh9buq.fsf@osv.gnss.ru/ [2] hinted at in final paragraph of https://lore.kernel.org/git/87bklilnvp.fsf@osv.gnss.ru/ [3] https://lore.kernel.org/git/87wna3jwx8.fsf@osv.gnss.ru/ > > (I do agree we have a huge problem and thus that a huge step forward > > theoretically could be taken, I just don't see this as it.) > > It works. Really. > > > But I've stated that more than enough, and no one is producing patches > > on this topic right now, so I'll drop out of this thread. > > OK, I participate only in hope that there will be somebody who actually > cares enough to implement it. Maybe it will be me, maybe not, and I > already got it that neither you nor the original author of git-rebase > are interested. Correct, I'm not interested in implementing it that particular way, though I will be implementing it in what I feel is the right way. Anyway, I hope something I said above helps in some way. Even if not, I wish you the best of luck on your efforts.
On Thu, Feb 16, 2023 at 7:33 AM Tao Klerks <tao@klerks.biz> wrote: > On Thu, Feb 16, 2023 at 4:22 AM Alex Henrie <alexhenrie24@gmail.com> wrote: > > 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. That's because the whole approach to the pull.rebase configuration is wrong. I explained why multiple times in countless discussions and git developers did not listen. What we need is a pull.mode configuration that is *orthogonal* to pull.rebase, then everything just works. For example, you could have this configuration. git config pull.mode merge git config pull.rebase merges Then doing `git pull --rebase` would do a merges rebase. This is not possible with the current approach, which I objected to. Then there's no problem with telling the users to do pull.mode=rebase (or whatever), since that doesn't override pull.rebase=merges. I programmed and explained this precise interaction with rebase=merges more than two years ago [1], but nobody listened. For an example of how such configuration would look like, see the patches I just sent [2]. Cheers. [1] https://lore.kernel.org/git/20201218211026.1937168-14-felipe.contreras@gmail.com/ [2] https://lore.kernel.org/git/20230228140236.4175835-1-felipe.contreras@gmail.com/T/#t
On Tue, Feb 28, 2023 at 7:13 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > On Thu, Feb 16, 2023 at 7:33 AM Tao Klerks <tao@klerks.biz> wrote: > > On Thu, Feb 16, 2023 at 4:22 AM Alex Henrie <alexhenrie24@gmail.com> wrote: > > > > 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. > > That's because the whole approach to the pull.rebase configuration is > wrong. I explained why multiple times in countless discussions and git > developers did not listen. > > What we need is a pull.mode configuration that is *orthogonal* to > pull.rebase, then everything just works. > > For example, you could have this configuration. > > git config pull.mode merge > git config pull.rebase merges > > Then doing `git pull --rebase` would do a merges rebase. > > This is not possible with the current approach, which I objected to. > > Then there's no problem with telling the users to do pull.mode=rebase > (or whatever), since that doesn't override pull.rebase=merges. > > I programmed and explained this precise interaction with rebase=merges > more than two years ago [1], but nobody listened. For an example of > how such configuration would look like, see the patches I just sent > [2]. > > Cheers. > > [1] https://lore.kernel.org/git/20201218211026.1937168-14-felipe.contreras@gmail.com/ > [2] https://lore.kernel.org/git/20230228140236.4175835-1-felipe.contreras@gmail.com/T/#t I think everybody agrees that the current situation is kind of a mess. For better or for worse, there was no consensus to get away from the current set of config options and introduce something more straightforward, so we keep making incremental evolutionary changes within the current framework. I didn't include it in my list of things to do, but your email made me realize that the problem with `git pull` not being able to do an interactive rebase that rebases merge commits is also something that needs to be fixed before flipping on "rebase merges" by default. If we add that to the list, I guess it would be step 2.25. It's going to be confusing if `git pull -r` rebases merges but `git pull -ri` does not. One way to solve that would be to make -ri/--rebase=interactive on the command line work with pull.rebase=merges instead of overriding it, and add a separate --(no-)rebase-merges command line option for if the user really does want to override it. On top of that, either before or at the same time as when the default behavior of `git pull --rebase` changes to rebase merges, the behavior of branch.autoSetupRebase needs to change to match. -Alex
On Tue, Feb 28, 2023 at 2:04 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > I think everybody agrees that the current situation is kind of a mess. > For better or for worse, there was no consensus to get away from the > current set of config options and introduce something more > straightforward, so we keep making incremental evolutionary changes > within the current framework. Yes, but my proposed incremental evolutionary changes would have solved the problems by now if the maintainer club bothered to look at them. Instead, they implemented a solution from one of their club, which in my opinion is an incremental change that made the situation *worse*, not better. This issue with pull.rebase=merges is one that I had *already* explained to them, and they ignored it. And it's not the only one. When you ignore the opinions of certain kinds of people, and only listen to certain kinds of people, you end up with worse code. Cheers.
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")); } int cmd_pull(int argc, const char **argv, const char *prefix)