Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

> On Wed, Sep 16, 2015 at 5:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, Sep 14, 2015 at 1:44 PM, Mike Rappazzo <rappazzo@gmail.com> wrote:
>>> On Sat, Sep 12, 2015 at 11:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> The original code in branch.c, which this patch removes, did not need
>>>> to make a special case of HEAD, so it's not immediately clear why this
>>>> replacement code does so. This is the sort of issue which argues in
>>>> favor of mutating the existing code (slowly) over the course of
>>>> several patches into the final form, rather than having the final form
>>>> come into existence out of thin air. When the changes are made
>>>> incrementally, it is easier for reviewers to understand why such
>>>> modifications are needed, which (hopefully) should lead to fewer
>>>> questions such as this one.
>>>
>>> I reversed the the check here; it is intended to check if the symref
>>> is _not_ the head, since the head
>>> ref has already been parsed.  This is used in notes.c to find
>>> "NOTES_MERGE_REF".
>>
>> I'm probably being dense, but I still don't understand why the code
>> now needs a special case for HEAD, whereas the original didn't. But,
>> my denseness my be indicative of this change not being well-described
>> (or described at all) by the commit message. Hopefully, when this is
>> refactored into finer changes, the purpose will become clear.
>
> The special case for HEAD is because the HEAD is already included in
> the worktree struct.  This block is intended to save from re-parsing.
> If you think the code would be easier to read, the HEAD check could be
> removed, and the ref will just be parsed always.