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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 14, 2021, 10:17 UTC
Message-ID: <5ef402a4-3477-6227-e08c-041ed373313e@gmail.com>
In-Reply-To: <xmqq5yv4ccc0.fsf@gitster.g>

Hi Junio

On 13/09/2021 23:58, Junio C Hamano wrote:

Show 9 quoted lines

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

Show 23 quoted lines

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

Show 12 quoted lines

> 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

Show 8 quoted lines

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