Re: [PATCH v3 1/2] rebase: skip branch symref aliases

Phillip Wood <[email protected]> writes:

>> Thanks for re-rolling I'm pretty sure the logic is sound now but I'm a 
>> bit confused by a couple of things - see my comments below.
>> ...
>> It would be nice to have a comment here explaining what we're doing. 
>> Also I don't think we need to copy the refname so it would be more 
>> efficient to use refs_resolve_ref_unsafe().
>
> Looking at this again we cannot use refs_resolve_ref_unsafe() because 
> the result would be overwritten by the call to refs_resolve_refdup() in 
> branch_checked_out().

Makes sense.  Thanks for raising a possible alternative and then
clarifying that it is not quite workable.

>>> +        /*
>>> +         * If the branch is the current HEAD, then it will be
>>> +         * updated by the default rebase behavior.
>>> +         */
>>> +        if (head_ref && !strcmp(head_ref, decoration->name)) {
>>> +            free(resolved_ref);
>>>               decoration = decoration->next;
>>>               continue;
>>>           }
>> 
>> Then we check to see if the decoration matches HEAD which we used to do 
>> above - I'm not clear why we have moved this check.
>
> Should we be using "resolved_ref" instead of "decoration->name"? That 
> would explain why this was moved and would makes sense as we resolve 
> symrefs when reading HEAD. When HEAD points outside "refs/heads/" we'd 
> then skip updating any symrefs under "refs/heads/" that pointed to the 
> same ref as HEAD.

Yeah, decoration is very much end-user facing and if we can make
behavioural decision based on a more stable resolved_ref that would
make it easier to reason about.

But stepping back a bit, is having a HEAD that is a symref and
points outside "refs/heads/" an invalid state?  Why are we catering
to such a configuration to begin with?