| Message ID | 20220418012716.2683624-1-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rebase: use correct base for --keep-base when a branch is given | expand |
Alex Henrie <alexhenrie24@gmail.com> writes: > - /* Make sure the branch to rebase onto is valid. */ So, we used to ... > - if (keep_base) { > - strbuf_reset(&buf); > - strbuf_addstr(&buf, options.upstream_name); > - strbuf_addstr(&buf, "..."); > - options.onto_name = xstrdup(buf.buf); ... store "<upstream>..." in onto_name ... > - } else if (!options.onto_name) > - options.onto_name = options.upstream_name; ... or used <upstream> as onto_name, and then ... > - if (strstr(options.onto_name, "...")) { ... immediately used the "..." as a cue to compute the merge base and ... > - if (get_oid_mb(options.onto_name, &merge_base) < 0) { > - if (keep_base) > - die(_("'%s': need exactly one merge base with branch"), > - options.upstream_name); > - else > - die(_("'%s': need exactly one merge base"), > - options.onto_name); > - } > - options.onto = lookup_commit_or_die(&merge_base, > - options.onto_name); ... used the "<upstream>..." as a label, and the merge base as the "onto" commit. > - } else { > - options.onto = > - lookup_commit_reference_by_name(options.onto_name); > - if (!options.onto) > - die(_("Does not point to a valid commit '%s'"), > - options.onto_name); > - } > - But that was done before we even examined the optinal "this one, not the current branch, is to be rebased" argument. So it all works by assuming the current branch is what is being rebase, which clearly is wrong. It is surprising that a basic and trivial bug was with us forever. I wonder if the addition of --keep-base was faulty and the feature was broken from the beginning, or there was an unrelated change that broke the interaction between --keep-base and branch-to-be-rebased? ... goes and looks ... Between 640f9cd5 (Merge branch 'dl/rebase-i-keep-base', 2019-09-30) which was where "--keep-base" was introduced, and today's codebase, there are many unrelated changes to builtin/rebase.c but this part is essentially unchanged. Before the option was introduced, it didn't matter if the computation of "onto" came before or after the next part that is not shown in this patch, because "..." could have come only from the end-user input and there the end-user must have given the branch to be rebased on the right hand side of "..." if that is what they meant. So, the feature was broken from the moment the "--keep-base" option was introduced. Now we got rid of all the above, so we need to be careful that we do not depend on options.onto_name and options.onto in the part that you did not touch, from here to the precontext of the next hunk. And I looked for onto and onto_name, these strings do not appear from this point until the next hunk where we see an added line, so we are not breaking that part of the code by removing this block of code from here and moving it down. > /* > * If the branch to rebase is given, that is the branch we will rebase > * branch_name -- branch/commit being rebased, or > @@ -1659,6 +1632,34 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > } else > BUG("unexpected number of arguments left to parse"); > > + /* Make sure the branch to rebase onto is valid. */ And now we do what the removed block wanted to do here, but we do so in a different context. We know know which branch gets rebased in the branch_name variable, so we do not use "<upstream>..." notation to imply "the current branch" on the RHS; we can be explicit. > + if (keep_base) { > + strbuf_reset(&buf); > + strbuf_addstr(&buf, options.upstream_name); > + strbuf_addstr(&buf, "..."); > + strbuf_addstr(&buf, branch_name); ... and that is what happens here, which is good. The rest of this new block is a literal copy from the old code removed above, which is as expected. > + options.onto_name = xstrdup(buf.buf); > + } else if (!options.onto_name) > + options.onto_name = options.upstream_name; > + if (strstr(options.onto_name, "...")) { > + if (get_oid_mb(options.onto_name, &merge_base) < 0) { > + if (keep_base) > + die(_("'%s': need exactly one merge base with branch"), > + options.upstream_name); > + else > + die(_("'%s': need exactly one merge base"), > + options.onto_name); > + } > + options.onto = lookup_commit_or_die(&merge_base, > + options.onto_name); > + } else { > + options.onto = > + lookup_commit_reference_by_name(options.onto_name); > + if (!options.onto) > + die(_("Does not point to a valid commit '%s'"), > + options.onto_name); > + } > + > if (options.fork_point > 0) { > struct commit *head = > lookup_commit_reference(the_repository, > diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh > index 3716a42e81..d1db528e25 100755 > --- a/t/t3416-rebase-onto-threedots.sh > +++ b/t/t3416-rebase-onto-threedots.sh > @@ -129,6 +129,22 @@ test_expect_success 'rebase --keep-base main from topic' ' > test_cmp expect actual > ' > > +test_expect_success 'rebase --keep-base main topic from main' ' > + git reset --hard && Clearing whatever cruft the last test left is good, but ... > + git checkout topic && > + git reset --hard G && > + git checkout main && it would be more efficient to just do git checkout main && git branch -f topic G && no? Instead of rewriting the working tree three times, you only need to do so once here.
On Wed, Apr 20, 2022 at 2:46 PM Junio C Hamano <gitster@pobox.com> wrote: > > It is surprising that a basic and trivial bug was with us forever. I was also surprised. Like you, I dug through the mailing list archives and my impression was that the code review was mostly concerned about the case where there are multiple merge bases. No one seemed to have read the man page closely enough to notice that 'git rebase --keep-base <upstream> <branch>' cannot be equivalent to 'git rebase --onto <upstream>... <upstream>' because the latter doesn't have <branch> at all. Seeing that discrepancy is what prompted me to try out what actually happens when the branch is specified on the command line, and then I realized that it was more than just a documentation error. > > +test_expect_success 'rebase --keep-base main topic from main' ' > > + git reset --hard && > > Clearing whatever cruft the last test left is good, but ... > > > + git checkout topic && > > + git reset --hard G && > > + git checkout main && > > it would be more efficient to just do > > git checkout main && > git branch -f topic G && > > no? Instead of rewriting the working tree three times, you only > need to do so once here. Sure. I'll make that change. -Alex
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 9da4647061..262fb01aec 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -215,9 +215,10 @@ leave out at most one of A and B, in which case it defaults to HEAD. --keep-base:: Set the starting point at which to create the new commits to the - merge base of <upstream> <branch>. Running + merge base of <upstream> and <branch>. Running 'git rebase --keep-base <upstream> <branch>' is equivalent to - running 'git rebase --onto <upstream>... <upstream>'. + running + 'git rebase --onto <upstream>...<branch> <upstream> <branch>'. + This option is useful in the case where one is developing a feature on top of an upstream branch. While the feature is being worked on, the diff --git a/builtin/rebase.c b/builtin/rebase.c index 27fde7bf28..7f3bffc0a2 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1583,33 +1583,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.upstream_arg = "--root"; } - /* Make sure the branch to rebase onto is valid. */ - if (keep_base) { - strbuf_reset(&buf); - strbuf_addstr(&buf, options.upstream_name); - strbuf_addstr(&buf, "..."); - options.onto_name = xstrdup(buf.buf); - } else if (!options.onto_name) - options.onto_name = options.upstream_name; - if (strstr(options.onto_name, "...")) { - if (get_oid_mb(options.onto_name, &merge_base) < 0) { - if (keep_base) - die(_("'%s': need exactly one merge base with branch"), - options.upstream_name); - else - die(_("'%s': need exactly one merge base"), - options.onto_name); - } - options.onto = lookup_commit_or_die(&merge_base, - options.onto_name); - } else { - options.onto = - lookup_commit_reference_by_name(options.onto_name); - if (!options.onto) - die(_("Does not point to a valid commit '%s'"), - options.onto_name); - } - /* * If the branch to rebase is given, that is the branch we will rebase * branch_name -- branch/commit being rebased, or @@ -1659,6 +1632,34 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } else BUG("unexpected number of arguments left to parse"); + /* Make sure the branch to rebase onto is valid. */ + if (keep_base) { + strbuf_reset(&buf); + strbuf_addstr(&buf, options.upstream_name); + strbuf_addstr(&buf, "..."); + strbuf_addstr(&buf, branch_name); + options.onto_name = xstrdup(buf.buf); + } else if (!options.onto_name) + options.onto_name = options.upstream_name; + if (strstr(options.onto_name, "...")) { + if (get_oid_mb(options.onto_name, &merge_base) < 0) { + if (keep_base) + die(_("'%s': need exactly one merge base with branch"), + options.upstream_name); + else + die(_("'%s': need exactly one merge base"), + options.onto_name); + } + options.onto = lookup_commit_or_die(&merge_base, + options.onto_name); + } else { + options.onto = + lookup_commit_reference_by_name(options.onto_name); + if (!options.onto) + die(_("Does not point to a valid commit '%s'"), + options.onto_name); + } + if (options.fork_point > 0) { struct commit *head = lookup_commit_reference(the_repository, diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh index 3716a42e81..d1db528e25 100755 --- a/t/t3416-rebase-onto-threedots.sh +++ b/t/t3416-rebase-onto-threedots.sh @@ -129,6 +129,22 @@ test_expect_success 'rebase --keep-base main from topic' ' test_cmp expect actual ' +test_expect_success 'rebase --keep-base main topic from main' ' + git reset --hard && + git checkout topic && + git reset --hard G && + git checkout main && + + git rebase --keep-base main topic && + git rev-parse C >base.expect && + git merge-base main HEAD >base.actual && + test_cmp base.expect base.actual && + + git rev-parse HEAD~2 >actual && + git rev-parse C^0 >expect && + test_cmp expect actual +' + test_expect_success 'rebase --keep-base main from side' ' git reset --hard && git checkout side && @@ -153,6 +169,23 @@ test_expect_success 'rebase -i --keep-base main from topic' ' test_cmp expect actual ' +test_expect_success 'rebase -i --keep-base main topic from main' ' + git reset --hard && + git checkout topic && + git reset --hard G && + git checkout main && + + set_fake_editor && + EXPECT_COUNT=2 git rebase -i --keep-base main topic && + git rev-parse C >base.expect && + git merge-base main HEAD >base.actual && + test_cmp base.expect base.actual && + + git rev-parse HEAD~2 >actual && + git rev-parse C^0 >expect && + test_cmp expect actual +' + test_expect_success 'rebase -i --keep-base main from side' ' git reset --hard && git checkout side &&
--keep-base rebases onto the merge base of the given upstream and the current HEAD regardless of whether a branch is given. This is contrary to the documentation and to the option's intended purpose. Instead, rebase onto the merge base of the given upstream and the given branch. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- Documentation/git-rebase.txt | 5 +-- builtin/rebase.c | 55 ++++++++++++++++---------------- t/t3416-rebase-onto-threedots.sh | 33 +++++++++++++++++++ 3 files changed, 64 insertions(+), 29 deletions(-)