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@xxxxxxxxx> writes:
From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
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.
