| Message ID | 951de6bb1992773cda60791c4b7a09867b5e0f19.1631546362.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rebase: dereference tags | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Aborting a rebase stated with 'git rebase <upstream> <tag-object>' > should checkout the commit pointed to by <tag-object>. Instead it gives I am not sure if "should checkout the commit pointed to by." is a good description. It does not seem to be sufficiently justified. Did we auto-peel in scripted version of "git rebase" and is this a regression when the command was rewritten in C? If that is not the case, this topic is perhaps slightly below borderline "meh" to me. The optional "first switch to this <branch> before doing anything" command-line argument in git rebase [--onto <there>] <upstream> [<branch>] was meant to give a branch, and because we treat detached HEAD as almost first-class citizen when dealing with branch-ish things, we allowed git rebase master my-topic^0 to try rebasing my-topic on detached HEAD without losing the original. In other words, you had to be explicit that you meant the commit object, not a ref that points at it, to trigger this "rebase detached" feature. The same thing for tags. git rebase master v12.3^0 would be a proper request to rebase the history leading to that commit. Without the peeling, it appears the user is asking to update the ref that can be uniquely identified with "v12.3", but we do not want to rebase a tag. It would have been a different story if we had a problem when a tag is given to "--onto <there>", but I do not think this topic is about that case. Having said that, even if we decide that we shouldn't accept the tag object and require peeled form to avoid mistakes (instead of silently peeling the tag ourselves), I do agree that > error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD' > is a bad error message for this. It should be something like error: cannot rebase a tag perhaps. But if we auto-peeled in an old version, I do not mind this series (but let's drop pointless "clean-up" that is not, like what was pointed out by Réne). In such a case, the first paragraph should say, instead of "should checkout", that "we used to do X, but commit Y broke us and now we die with an error message". Thanks.
On Mon, Sep 13, 2021 at 8:47 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Aborting a rebase stated with 'git rebase <upstream> <tag-object>' s/stated/started/ > should checkout the commit pointed to by <tag-object>. Instead it gives > > error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': > trying to write non-commit object > 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD' > > This is because when we parse the command line arguments although we > check that the tag points to a commit we remember the oid of the tag > and try and checkout that object rather than the commit it points > to. Fix this by using lookup_commit_reference_by_name() when parsing > the command line. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > --- > builtin/rebase.c | 14 ++++++++------ > t/t3407-rebase-abort.sh | 18 ++++++++++++++---- > 2 files changed, 22 insertions(+), 10 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 74663208468..2b70a196f9a 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1903,13 +1903,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > die_if_checked_out(buf.buf, 1); > options.head_name = xstrdup(buf.buf); > /* If not is it a valid ref (branch or commit)? */ > - } else if (!get_oid(branch_name, &options.orig_head) && > - lookup_commit_reference(the_repository, > - &options.orig_head)) > + } else { > + struct commit *commit = > + lookup_commit_reference_by_name(branch_name); > + if (!commit) > + die(_("no such branch/commit '%s'"), > + branch_name); > + oidcpy(&options.orig_head, &commit->object.oid); > options.head_name = NULL; > - else > - die(_("no such branch/commit '%s'"), > - branch_name); > + } > } else if (argc == 0) { > /* Do not need to switch branches, we are already on it. */ > options.head_name = > diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh > index 162112ba5ea..ebbaed147a6 100755 > --- a/t/t3407-rebase-abort.sh > +++ b/t/t3407-rebase-abort.sh > @@ -11,18 +11,18 @@ test_expect_success setup ' > test_commit a a a && > git branch to-rebase && > > - test_commit b a b && > - test_commit c a c && > + test_commit --annotate b a b && > + test_commit --annotate c a c && > > git checkout to-rebase && > test_commit "merge should fail on this" a d d && > - test_commit "merge should fail on this, too" a e pre-rebase > + test_commit --annotate "merge should fail on this, too" a e pre-rebase > ' > > # Check that HEAD is equal to "pre-rebase" and the current branch is > # "to-rebase" > check_head() { > - test_cmp_rev HEAD pre-rebase && > + test_cmp_rev HEAD pre-rebase^{commit} && > test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase > } > > @@ -67,6 +67,16 @@ testrebase() { > test_path_is_missing "$state_dir" > ' > > + test_expect_success "rebase$type --abort when checking out a tag" ' > + test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" && > + git reset --hard a -- && > + test_must_fail git rebase$type --onto b c pre-rebase && > + test_cmp_rev HEAD b^{commit} && > + git rebase --abort && > + test_cmp_rev HEAD pre-rebase^{commit} && > + ! git symbolic-ref HEAD > + ' > + > test_expect_success "rebase$type --abort does not update reflog" ' > # Clean up the state from the previous one > git reset --hard pre-rebase && > -- > gitgitgadget
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Aborting a rebase stated with 'git rebase <upstream> <tag-object>' > should checkout the commit pointed to by <tag-object>. Instead it gives > > error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': > trying to write non-commit object > 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD' > > This is because when we parse the command line arguments although we > check that the tag points to a commit we remember the oid of the tag > and try and checkout that object rather than the commit it points > to. Fix this by using lookup_commit_reference_by_name() when parsing > the command line. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> [...] > diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh > index 162112ba5ea..ebbaed147a6 100755 > --- a/t/t3407-rebase-abort.sh > +++ b/t/t3407-rebase-abort.sh > @@ -11,18 +11,18 @@ test_expect_success setup ' > test_commit a a a && > git branch to-rebase && > > - test_commit b a b && > - test_commit c a c && > + test_commit --annotate b a b && > + test_commit --annotate c a c && > > git checkout to-rebase && > test_commit "merge should fail on this" a d d && > - test_commit "merge should fail on this, too" a e pre-rebase > + test_commit --annotate "merge should fail on this, too" a e pre-rebase > ' These two do not seem to belong to this particular commit? > > # Check that HEAD is equal to "pre-rebase" and the current branch is > # "to-rebase" > check_head() { > - test_cmp_rev HEAD pre-rebase && > + test_cmp_rev HEAD pre-rebase^{commit} && > test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase > } -- Sergey Organov
Hi Sergey On 14/09/2021 10:48, Sergey Organov wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> Aborting a rebase stated with 'git rebase <upstream> <tag-object>' >> should checkout the commit pointed to by <tag-object>. Instead it gives >> >> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': >> trying to write non-commit object >> 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD' >> >> This is because when we parse the command line arguments although we >> check that the tag points to a commit we remember the oid of the tag >> and try and checkout that object rather than the commit it points >> to. Fix this by using lookup_commit_reference_by_name() when parsing >> the command line. >> >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > > [...] > >> diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh >> index 162112ba5ea..ebbaed147a6 100755 >> --- a/t/t3407-rebase-abort.sh >> +++ b/t/t3407-rebase-abort.sh >> @@ -11,18 +11,18 @@ test_expect_success setup ' >> test_commit a a a && >> git branch to-rebase && >> >> - test_commit b a b && >> - test_commit c a c && >> + test_commit --annotate b a b && >> + test_commit --annotate c a c && >> >> git checkout to-rebase && >> test_commit "merge should fail on this" a d d && >> - test_commit "merge should fail on this, too" a e pre-rebase >> + test_commit --annotate "merge should fail on this, too" a e pre-rebase >> ' > > These two do not seem to belong to this particular commit? They do, the annotated tag is used in the new test added in this commit which tests that we peel tags. Best Wishes Phillip >> # Check that HEAD is equal to "pre-rebase" and the current branch is >> # "to-rebase" >> check_head() { >> - test_cmp_rev HEAD pre-rebase && >> + test_cmp_rev HEAD pre-rebase^{commit} && >> test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase >> } > > > -- Sergey Organov >
Hi Junio On 13/09/2021 23:58, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> Aborting a rebase stated with 'git rebase <upstream> <tag-object>' >> should checkout the commit pointed to by <tag-object>. Instead it gives > > I am not sure if "should checkout the commit pointed to by." is a > good description. It does not seem to be sufficiently justified. My logic was that as we handle commits here it would make sense to handle tags as well - I discovered that this did not work when I happened to use an annotated tag as the <branch> argument to rebase the commits pointed to by the tag and was surprised it did not work when we happily accept tags for <upstream> and --onto. > Did we auto-peel in scripted version of "git rebase" and is this a > regression when the command was rewritten in C? As far as I can tell we have never peeled tags here > If that is not the case, this topic is perhaps slightly below > borderline "meh" to me. The optional "first switch to this <branch> > before doing anything" command-line argument in > > git rebase [--onto <there>] <upstream> [<branch>] > > was meant to give a branch, and because we treat detached HEAD as > almost first-class citizen when dealing with branch-ish things, we > allowed > > git rebase master my-topic^0 > > to try rebasing my-topic on detached HEAD without losing the > original. In other words, you had to be explicit that you meant the > commit object, not a ref that points at it, to trigger this "rebase > detached" feature. The same thing for tags. > > git rebase master v12.3^0 > > would be a proper request to rebase the history leading to that > commit. Without the peeling, it appears the user is asking to > update the ref that can be uniquely identified with "v12.3", but we > do not want to rebase a tag. I wrote this patch as I felt it was an artificial distinction to require that <branch> is a branch-ish thing rather than a commit-ish thing. Rebase already peels <upstream> and --onto so it feels inconsistent not to do it for <branch>. I guess the counter argument to that is users may be confused and start complaining that the tag itself is not rebased. > It would have been a different story if we had a problem when a tag > is given to "--onto <there>", but I do not think this topic is about > that case. No "--onto <tag>" works fine. We also accept a tag object for upstream without requiring the user to peel it for us. > Having said that, even if we decide that we shouldn't accept the tag > object and require peeled form to avoid mistakes (instead of > silently peeling the tag ourselves), I do agree that > >> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD' >> > > is a bad error message for this. It should be something like > > error: cannot rebase a tag > > perhaps. We could do that if we're worried that users would be confused by the tag not being rebased if we started automatically peeling <branch>. (I'm kind of leaning in that direction at the moment having read your email) Best Wishes Phillip > But if we auto-peeled in an old version, I do not mind this series > (but let's drop pointless "clean-up" that is not, like what was > pointed out by Réne). In such a case, the first paragraph should > say, instead of "should checkout", that "we used to do X, but commit > Y broke us and now we die with an error message". > > Thanks. >
On 14/09/2021 11:17, Phillip Wood wrote: > Hi Junio > > On 13/09/2021 23:58, Junio C Hamano wrote: >> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> From: Phillip Wood <phillip.wood@dunelm.org.uk> >>> >>> Aborting a rebase stated with 'git rebase <upstream> <tag-object>' >>> should checkout the commit pointed to by <tag-object>. Instead it gives >> >> I am not sure if "should checkout the commit pointed to by." is a >> good description. It does not seem to be sufficiently justified. > > My logic was that as we handle commits here it would make sense to > handle tags as well - I discovered that this did not work when I > happened to use an annotated tag as the <branch> argument to rebase the > commits pointed to by the tag and was surprised it did not work when we > happily accept tags for <upstream> and --onto. > >> Did we auto-peel in scripted version of "git rebase" and is this a >> regression when the command was rewritten in C? > > As far as I can tell we have never peeled tags here That's a bit misleading. We have never peeled a tag given as <branch> when we parse it. In the scripted version we just passed the tag oid along to rev-list, checkout and reset and they peeled it. So I think this is actually a regression in the builtin rebase. I'll update the commit message to reflect that unless we feel that allowing a tag for <branch> is a mistake and we should be erroring out to avoid the possible confusion of the tag not being rebased, only the commits it points to. Sorry for the confusion Phillip >> If that is not the case, this topic is perhaps slightly below >> borderline "meh" to me. The optional "first switch to this <branch> >> before doing anything" command-line argument in >> >> git rebase [--onto <there>] <upstream> [<branch>] >> >> was meant to give a branch, and because we treat detached HEAD as >> almost first-class citizen when dealing with branch-ish things, we >> allowed >> >> git rebase master my-topic^0 >> >> to try rebasing my-topic on detached HEAD without losing the >> original. In other words, you had to be explicit that you meant the >> commit object, not a ref that points at it, to trigger this "rebase >> detached" feature. The same thing for tags. >> >> git rebase master v12.3^0 >> >> would be a proper request to rebase the history leading to that >> commit. Without the peeling, it appears the user is asking to >> update the ref that can be uniquely identified with "v12.3", but we >> do not want to rebase a tag. > > I wrote this patch as I felt it was an artificial distinction to require > that <branch> is a branch-ish thing rather than a commit-ish thing. > Rebase already peels <upstream> and --onto so it feels inconsistent not > to do it for <branch>. I guess the counter argument to that is users may > be confused and start complaining that the tag itself is not rebased. > >> It would have been a different story if we had a problem when a tag >> is given to "--onto <there>", but I do not think this topic is about >> that case. > > No "--onto <tag>" works fine. We also accept a tag object for upstream > without requiring the user to peel it for us. > >> Having said that, even if we decide that we shouldn't accept the tag >> object and require peeled form to avoid mistakes (instead of >> silently peeling the tag ourselves), I do agree that >> >>> error: update_ref failed for ref 'HEAD': cannot update ref >>> 'HEAD': trying to write non-commit object >>> 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD' >>> >> >> is a bad error message for this. It should be something like >> >> error: cannot rebase a tag >> >> perhaps. > > We could do that if we're worried that users would be confused by the > tag not being rebased if we started automatically peeling <branch>. (I'm > kind of leaning in that direction at the moment having read your email) > > Best Wishes > > Phillip > >> But if we auto-peeled in an old version, I do not mind this series >> (but let's drop pointless "clean-up" that is not, like what was >> pointed out by Réne). In such a case, the first paragraph should >> say, instead of "should checkout", that "we used to do X, but commit >> Y broke us and now we die with an error message". >> >> Thanks. >>
Phillip Wood <phillip.wood123@gmail.com> writes: >>> Did we auto-peel in scripted version of "git rebase" and is this a >>> regression when the command was rewritten in C? >> As far as I can tell we have never peeled tags here > > That's a bit misleading. We have never peeled a tag given as <branch> > when we parse it. In the scripted version we just passed the tag oid > along to rev-list, checkout and reset and they peeled it. So I think > this is actually a regression in the builtin rebase. I'll update the > commit message to reflect that unless we feel that allowing a tag for > <branch> is a mistake and we should be erroring out to avoid the > possible confusion of the tag not being rebased, only the commits it > points to. OK, so this is a regression fix. That makes the change much simpler to sell. I'd expect that the description would be along the lines of something like this, perhaps. A rebase started with 'git rebase <A> <B>' is conceptually to first checkout <B> and run 'git rebase <A>' starting from that state. 'git rebase --abort' in the middle of such a rebase should take us back to the state we checked out <B>. This used to work, even when <B> is a tag that points at a commit, until Git X.Y.Z when the command was reimplemented in C. The command now complains that the tag object itself cannot be checked out, which may be technically correct but is not what the user asked to do. Fix this old regression by doing .... Thanks for digging (and fixing, of course).
diff --git a/builtin/rebase.c b/builtin/rebase.c index 74663208468..2b70a196f9a 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1903,13 +1903,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) die_if_checked_out(buf.buf, 1); options.head_name = xstrdup(buf.buf); /* If not is it a valid ref (branch or commit)? */ - } else if (!get_oid(branch_name, &options.orig_head) && - lookup_commit_reference(the_repository, - &options.orig_head)) + } else { + struct commit *commit = + lookup_commit_reference_by_name(branch_name); + if (!commit) + die(_("no such branch/commit '%s'"), + branch_name); + oidcpy(&options.orig_head, &commit->object.oid); options.head_name = NULL; - else - die(_("no such branch/commit '%s'"), - branch_name); + } } else if (argc == 0) { /* Do not need to switch branches, we are already on it. */ options.head_name = diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh index 162112ba5ea..ebbaed147a6 100755 --- a/t/t3407-rebase-abort.sh +++ b/t/t3407-rebase-abort.sh @@ -11,18 +11,18 @@ test_expect_success setup ' test_commit a a a && git branch to-rebase && - test_commit b a b && - test_commit c a c && + test_commit --annotate b a b && + test_commit --annotate c a c && git checkout to-rebase && test_commit "merge should fail on this" a d d && - test_commit "merge should fail on this, too" a e pre-rebase + test_commit --annotate "merge should fail on this, too" a e pre-rebase ' # Check that HEAD is equal to "pre-rebase" and the current branch is # "to-rebase" check_head() { - test_cmp_rev HEAD pre-rebase && + test_cmp_rev HEAD pre-rebase^{commit} && test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase } @@ -67,6 +67,16 @@ testrebase() { test_path_is_missing "$state_dir" ' + test_expect_success "rebase$type --abort when checking out a tag" ' + test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" && + git reset --hard a -- && + test_must_fail git rebase$type --onto b c pre-rebase && + test_cmp_rev HEAD b^{commit} && + git rebase --abort && + test_cmp_rev HEAD pre-rebase^{commit} && + ! git symbolic-ref HEAD + ' + test_expect_success "rebase$type --abort does not update reflog" ' # Clean up the state from the previous one git reset --hard pre-rebase &&