Re: [PATCH v2 11/11] rebase: dereference tags

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 13, 2021, 22:58 UTC
Message-ID: <xmqq5yv4ccc0.fsf@gitster.g>
In-Reply-To: <951de6bb1992773cda60791c4b7a09867b5e0f19.1631546362.git.gitgitgadget@gmail.com>

"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.