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

[Phillip Wood <phillip.wood123@gmail.com>]

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