New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rebase: invent a better way to recreate commit topology (think: --preserve-merges
done right)
#447
Conversation
83d7862
to
3661c96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be unimportant nitpick in comparison to the functionality itself, but for what it's worth, alongside "this is what we have" and "this is what we (sometimes) don't want", the commit message might benefit from the additional/final "this is what we`re now making possible" diagram, too, concluding the text itself and making the point clear (if not already).
For example, I myself am not sure whether D' is going to stay with A as ancestor, which seems be desired here (basically making the whole process no-op if "todo" script is left unchanged)...?
That said, wouldn't this mode actually be a better default?
In rebasing from HEAD (F) to B it kind of seems unexpected that commit D gets rebased in the first place, as that one isn't found in "F to B" traversal, and even less expected that topology might/will change, and by default.
Sorry if I'm missing something obvious, might be lacking additional knowledge (or some fundamental one, even) .
Thanks, Buga
p.s. Not sure if I did this correctly, I (thought I) was commenting on commit d4d755d437c437a8bddb78cbcb38cb8bbf84cd95, "rebase -i: introduce --recreate-merges=no-rebase-cousins".
e0d8c0d
to
199c030
Compare
@boogisha first of all: thank you for your interest and your comments.
Good point. Here is the updated commit: ce0dc5c
Yes, I would agree. Alas, backwards-compatibility prohibits us from making it the default. We could, of course, introduce a config setting later to opt-in to recreate merges by default. Even better: it does not need to be me who makes that patch ;-)
In this example, we rebase "from" B "onto" B. That means that all commits that are reachable from F but not from B get rebased. Including D. If you were to perform a traditional rebase,
In short: it is totally expected that commits C, D, E and F are rebased. What the The idea is that running |
04033bb
to
b5b2815
Compare
I haven't dug into this all the way, but it caught my eye, as I've run into the problems that --preserve-merges has, and I've seen your shears.sh script before. I definitely want to see this feature in the main tree and built into the sequencer, as it's done here. Thanks for continuing to push this concept! |
Will contribute it directly after v2.16.0 is out! |
@dscho Thanks, updated commit ce0dc5ca4cfe80ba722864278115f3e87bcebaf6 now looks clear, even to me :) (and sorry for a bit delayed reply)
This one confuses me a bit, though - if Unless you mean in comparison to existing default behavior of
Yeah, it came to me a bit after I posted the message that commits for rebasing are actually picked by (1) "reachable from HEAD but not reachable from B", instead of (2) "found inside HEAD to B traversal only". I'm just not using rebase much with merge commits (one reason being its current fragility), thus slipped the difference (with no merge commits, (1) and (2) make no difference, picking the same commits). Thanks for clarifying.
This I understand (and like), and thus I find "no-rebase-cousins" to be a more appropriate default mode for That said, I would even argue |
I thought he meant that you can't make "recreate-merges" be the default for rebase interactive mode. I certainly think that we could make no-rebase-cousins the default for recreate-merges. |
Oh, I misunderstood! I thought you wanted to make I can certainly make the |
All clarified now, thanks both! :)
Yes, and it seems to make the most sense (to me at least) - having unchanged |
b5b2815
to
06b23ae
Compare
@dscho With all due respect to the great work you did so we actually have this feature implemented, and understanding that your impression might be different, but being heavily involved in the discussion / thinking / testing that led to it, too, I'm kind of left with a bitter aftertaste that d41a29ceb61ff445efd0f97a836f381de57c5a41 commit message doesn`t do justice to Sergey Organov. I have a long list of reasons to support this claim (and I'm willing to discuss it, as I might have understood him better from the beginning), but not to waste your time, I'm proposing a slightly updated commit message instead, might be serving the purpose better to give credit where credit is due, something like this:
|
@dscho Regarding the code itself, not sure if I'm doing something wrong, but I'm getting a consistent/reproducible crash on Ubuntu 16.04, built Git from this pull request (hopefully correctly), Repo setup script:
In this situation, I do
Now, no matter if I first do
Let me know if you need more data... and please note I'm a very novice Linux user ;) |
0a0f2e0
to
14115b9
Compare
I am sorry, that is not my intention. It was my impression that his approach was not viable, and that Phillip's approach is vastly superior (even if Sergey apparently did not bother to weigh its pros and cons). But I do not want you to be bitter. So I changed the commit message (copy-edited yours).
I will try to find some time to reproduce this here. Thank you so much for being so thorough and helpful! It has been a pleasure working with you so far, and I think the result is already so much better than what I came up with on my own, alone. |
I cut out some time I wanted to spend on exercise, and exercised my brain muscle instead: dscho@b3aad3a (this test still needs a lot of love, of course, but I think I'll get there, as soon as I refactored the Tomorrow, though. Or day after tomorrow if I am still sick. |
No worries, nothing to be sorry about, we all have different perspectives. But, thank you, really, for everything you did, and you're doing, it means (and shows) a lot.
Hehe, wanting to write this before I even read your reply, I'll just say now that the feeling is mutual :) I'm glad if I can be of help, and if I get to learn something in the process, even better.
Take your time. I suffered a nasty stomach flu a few days ago, so I`m still recovering myself, too. |
We do. But we also have different priorities, it seems, and we also have different ideas how to form consensus. I now regret editing in Sergey into the commit message, because his messages are the reason I don't want to read this mail thread anymore. He ignores everything I say (except the parts that can be contorted into seemingly agreeing with him), he does not answer any question, let alone consider that his strategy might be awful. And then he ridicules me for still trying to convince him. I so want to throw out this mail thread from my memory. It makes me sick. |
Eh, I guess this has to do with recent replies to that mailing list topic...? :( I did get a bunch of e-mails from you and Sergey lately, but didn't have time to check them out yet (and I won't be able to do so for a while, at least), so I'm not really sure what's happening, but I wanted to reply here as it seems it got a bit out of control - I'm sorry that you feel like that, it shouldn't be the way all this works, especially for the people that are actually doing the most of the work... and I'm sorry if anything I did took part in the feeling :( But all this said, Sergey does have his part in raising the issue and pushing for its solution, coming up with the initial idea, even, so I think the commit message mention is fair enough, and the right thing to do - which you did, and at least shouldn't be a thing to regret over, unrelated to the feelings between the two of you, not to be confused, even if it seems to spill over and color the thread itself. It's just pretty unfortunate that you seem not to (get along with | understand) each other too much, discussions needlessly getting overheated, eventually causing bad feelings... and for no good reason, I`m afraid :/ I can only suggest to avoid further direct communication, not making it any worse, and possibly have me look into the current state of it (soon), hopefully being able to come to some middle ground, and for the better of everyone. What me personally makes a bit upset is that I was able to hint what both of you are talking about so far (not sure if that is going to be the case with the latest replies once I get to them, though, but I hope so), where you both seem to aim for the best of it, but eventually just get to annoy each other so much that the main purpose of your very discussion falls out of sight, lost in the noise. But I don't mind it much as I really find (interactive) rebase to be one of Git`s greatest possibilities, thus trying to keep myself motivated to have the new merge rebasing logic as good as possible, helping in possibly the only way I can at the moment, discussing it through, as much as my humble knowledge allows me to. Please don't feel bad about all this, but also feel free to follow your inner senses. I might prefer to see some things discussed further, or changed, even, but I would also totally understand if you would like to get over with all this already, nothing to blame you for - and I guess some changes will be possible after the fact as well, if needed. No matter what you decide upon, might be after letting it settle a bit, thanks again, for everything, and heads up! Please :) You're doing a great job, and without me telling you that. All this should be fun and enjoying, and if it slipped off path, let's try making it so again ;) |
That's certainly how this feels.
Hannes Sixt came up with the original idea. So I think Hannes deserves the credit. And Phillip deserves the credit for putting the derailed train wreck back into a productive direction. I cannot mention them all. But if I should mention what made me implement the changes, it was Phillip's idea. I will change the commit message accordingly, to set the record straight. If you want, I can give Sergey credit for annoying me so much that I took a break from this project for almost two weeks.
For the moment, I do. There is nothing you or I can do about it. I guess this comes back to the difference between computer scientists, programmers and software engineers: computer scientists come up with theories that look good on paper, programmers write code, engineers use programming to create solutions. While I certainly fall prey to the appeal of nice theories (and could talk all night about them over a good drink or three), I was always interested in solutions (and consequently, I am annoyed when others stand in the way of solutions).
Let's see. For the moment, I am struggling with the problem that Phillip's strategy -- even if it is simple in theory -- does not map well into the code present in At least I am in the process again of focusing on the solution. That should turn the fun back on for me. |
@dscho I tried using --recreate-merges on a directory in which I'm using contrib/subtree. I.e. I added the directory using git add it using:
I was hoping --recreate-merges would work as --preserve-merges doesn't, but turns out --recreate-merges didn't work either. I definitely could have made a mistake, are you expecting --recreate-merges to work in this case? |
@winksaville this part of the Note, however, that the |
@dscho, I'm not sure if a different merge strategy or not, I'm to inexperienced, but here is the "graph" of the subtree:
So 91fa3bf5 is a squash of 54d92f93 and is just dangling which is probably unnatural, but I would hope it would be "easy" to recreate. Let me know what else you might need or give me some hints on what I need to do to have --recreate-merges work in this scenario. |
In the context of the new --rebase-merges mode, which was designed specifically to allow for changing the existing branch topology liberally, a user may want to extract commits into a completely fresh branch that starts with a newly-created root commit. This is now possible by inserting the command `reset [new root]` before `pick`ing the commit that wants to become a root commit. Example: reset [new root] pick 012345 a commit that is about to become a root commit pick 234567 this commit will have the previous one as parent This does not conflict with other uses of the `reset` command because `[new root]` is not (part of) a valid ref name: both the opening bracket as well as the space are illegal in ref names. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This is consistent with `git commit` which, like `git merge`, supports passing the commit message via `-m <msg>` and, unlike `git merge` before this patch, via `-F <file>`. It is useful to allow this for scripted use, or for the upcoming patch to allow (re-)creating octopus merges in `git rebase --rebase-merges`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This commit adds a lengthy test case to t3430 that reflects some challenging use cases for the --rebase-merges option. In particular, it sets up a scenario which demonstrates that "evil merges" happen in practice, and they are necessarily introducing those extra changes. It then sets up three "upstream" branches with competing changes that are designed to conflict with the changes to rebase. The purpose of this added test case is two-fold: 1. to document what we expect --rebase-merges to accomplish, and even more to document what we do *not* expect it to be able to do. 2. to explore what kinds of merge conflicts --rebase-merges can produce (spoiler: we can end up with some bad ones, with unintuitively-nested merge conflicts). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This one is a bit tricky to explain, so let's try with a diagram: C / \ A - B - E - F \ / D To illustrate what this new mode is all about, let's consider what happens upon `git rebase -i --rebase-merges B`, in particular to the commit `D`. So far, the new branch structure would be: --- C' -- / \ A - B ------ E' - F' \ / D' This is not really preserving the branch topology from before! The reason is that the commit `D` does not have `B` as ancestor, and therefore it gets rebased onto `B`. This is unintuitive behavior. Even worse, when recreating branch structure, most use cases would appear to want cousins *not* to be rebased onto the new base commit. For example, Git for Windows (the heaviest user of the Git garden shears, which served as the blueprint for --rebase-merges) frequently merges branches from `next` early, and these branches certainly do *not* want to be rebased. In the example above, the desired outcome would look like this: --- C' -- / \ A - B ------ E' - F' \ / -- D' -- Let's introduce the term "cousins" for such commits ("D" in the example), and let's not rebase them by default, introducing the new "rebase-cousins" mode for use cases where they should be rebased. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When a user provides a todo list containing something like reset [new root] merge my-branch let's do the same as if pulling into an orphan branch: simply fast-forward. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Previously, we introduced the `merge` command for use in todo lists, to allow to recreate and modify branch topology. For ease of implementation, and to make review easier, the initial implementation only supported merge commits with exactly two parents. This patch adds support for octopus merges, making use of the just-introduced `-F <file>` option for the `git merge` command: to keep things simple, we simply spawn a new Git command, also opening the door to enhance `rebase --rebase-merges` to optionally use a merge strategy different from `recursive` for regular merges: this feature would use the same code path as octopus merges and simply spawn a `git merge`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When encountering nested conflicts, it can really be challenging to make sense of what goes where. The semi-realistic example that was added to t3430-rebase-merges.sh, for example, shows this nested conflict: int hi(void) { printf("Hello, world!\n"); } <<<<<<< intermediate merge <<<<<<< HEAD /* main event loop */ void event_loop(void) { /* TODO: place holder for now */ ======= ======= } >>>>>>> <HASH>... merge head #1 /* caller */ void caller(void) { hi(); >>>>>>> <HASH>... original merge } This is really confusing, in particular because the nested merge conflict is not contained in one arm of the outer merge conflict, but they seem to be interleaved. With this patch, the first 3-way merge produces conflict markers that are one character longer than the second 3-way merge's conflict markers, and it becomes a *little* more readable: int hi(void) { printf("Hello, world!\n"); } <<<<<<< intermediate merge <<<<<<<< HEAD /* main event loop */ void event_loop(void) { /* TODO: place holder for now */ ======== ======= } >>>>>>> <HASH>... merge head #1 /* caller */ void caller(void) { hi(); >>>>>>>> <HASH>... original merge } It still does not immediately make a whole lot of sense, and instead requires some brain-twisting and inspection of the intermediate state to understand. So what is going on? Well, after the intermediate merge, the event loop was added (via upstream, onto which we rebased), but with conflict markers, because the second parent had added the caller() function in the same place in the original merge. Since the rebased second parent also has the event loop added (through upstream, onto which it was rebased), the conflict markers added in the first 3-way merge *cause* the conflict in the second 3-way merge. And the last conflict marker, which looked as if it was concluding the outer conflict, is actually part of the "inner" (i.e. nested) conflict and just happens to not cause any further conflicts in the outer merge. Granted, it would be slightly more obvious if the conflict markers causing conflicts were wrapped in their own little conflict markers: int hi(void) { printf("Hello, world!\n"); } <<<<<<< intermediate merge <<<<<<<< HEAD ======= >>>>>>> <HASH>... merge head #1 /* main event loop */ void event_loop(void) { /* TODO: place holder for now */ <<<<<<< intermediate merge ======== ======= } >>>>>>> <HASH>... merge head #1 /* caller */ void caller(void) { hi(); >>>>>>>> <HASH>... original merge } At least now it is obvious that the extra `<<<<<<<< HEAD` before the event loop, and the extra `========` after it, *caused* the "outer" merge conflict. But xdl_merge() does not wrap the merge conflicts this way because there are only three unconflicting lines between the conflicting lines, and xdl_merge() tries to optimize for a minimal total number of lines (including the added conflict markers). In practice, the functions would be longer, and xdl_merge() *would* wrap only the nested conflict markers in outer conflict markers. It is still not something you would want to encounter in your every-day work, but presenting it this way is better than what we had before. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The --rebase-merges mode is probably not half as intuitive to use as its inventor hopes, so let's document it some. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Reported by Wink Saville: when rebasing with no-rebase-cousins, we will want to refrain from rebasing all of them, even when they are root commits. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Now that we support octopus merges in the `--rebase-merges` mode, we should give users who actually read the manuals a chance to know about this fact. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When rebasing a regular merge between two parent commits, we have that problem that we have to perform *two* 3-way merges, because we want to merge in the changes (like amendments, merge conflict resolutions, etc) from the original merge commit, too. When the first of these 3-way merges already had conflicts, then we run the chance of ending up with nested conflicts in the second 3-way merge. So let's see in that case whether we gain something by merging the original merge commit with the other parent first, and if that resulted in a clean merge, proceed to merge the first parent (in this case, we cannot end up with nested merge conflicts). This simplifies the realistic example of a nested merge conflict to a non-nested merge conflict. Before: int hi(void) { printf("Hello, world!\n"); } <<<<<<< intermediate merge <<<<<<<< HEAD /* main event loop */ void event_loop(void) { /* TODO: place holder for now */ ======== ======= } >>>>>>> <HASH>... merge head #1 /* caller */ void caller(void) { hi(); >>>>>>>> <HASH>... original merge } With this patch, this becomes much simpler: int hi(void) { printf("Hello, world!\n"); } /* main event loop */ void event_loop(void) { /* TODO: place holder for now */ } <<<<<<<< HEAD ======== /* caller */ void caller(void) { hi(); } >>>>>>>> <HASH>... intermediate merge Note: this needs to be refactored and stuff and things. It may even be necessary to dive deeper into the code and implement a "W merge" that avoids the problem where (one part of) one file would benefit from merging the second parent before the first, while another (part of the same) file would benefit from the reverse order. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The design of --preserve-merges was never meant to allow any interactive rebase, as demonstrated by the inability to reorder commits, to change merge commits' ancestry or to introduce new merge commits. The --rebase-merges mode we just introduced has a design that fixes those issues, and therefore we can now safely start to deprecate the --preserve-merges. While at it, explain a little better in the man page what the `--rebase-merges` mode is all about. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This made it into |
This made it into |
5 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…-with-gpg ci(windows): avoid using external gpg by mistake
The Git for Windows project uses the "Git garden shears" (a Unix shell script, piggy-backing on the interactive rebase) to rebase a thicket of branches, maintaining the branch structure.
To this end, it invents a couple of new commands for the todo list to
In contrast to
--preserve-merges
, this design makes the topology clear in the todo list and allows for reordering commits or even for changing the branch topology (introducing new branches, reordering commits from several branches into a single one, etcThe
shears.sh
script uses some ugly tricks to "add" those commands, causing issues with the stability, testability and performance.This Pull Request (which is of course not intended to be merged by the Git project, why use Pull Requests when you can force everybody to send patches through a lossy medium like a mailing list) has the patches to teach core Git's
rebase -i
proper to perform the same trick.The PR was opened mainly to leverage the Travis CI configuration to get this tested more thoroughly than a mere patch review ever could.
Funnily enough, those patches are already maintained in a thicket of branches, which are of course maintained using the patched
rebase -i
itself (using--recreate-merges=no-rebase-cousins
, to be precise). So there has been some interactive testing already :-)