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

Phillip Wood [off-list ref] writes:

quoted

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.

quoted

quoted

+        /*
+         * 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?