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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 16, 2015 at 5:09 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Mon, Sep 14, 2015 at 1:44 PM, Mike Rappazzo <rappazzo@xxxxxxxxx> wrote:
>> On Sat, Sep 12, 2015 at 11:19 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>>> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@xxxxxxxxx> wrote:
>>>> +       while (!matched && worktree_list) {
>>>> +               if (strcmp("HEAD", symref)) {
>>>> +                       strbuf_reset(&path);
>>>> +                       strbuf_reset(&sb);
>>>> +                       strbuf_addf(&path, "%s/%s", worktree_list->worktree->git_dir, symref);
>>>> +
>>>> +                       if (_parse_ref(path.buf, &sb, NULL)) {
>>>> +                               continue;
>>>> +                       }
>>>> +
>>>> +                       if (!strcmp(sb.buf, target))
>>>> +                               matched = worktree_list->worktree;
>>>
>>> 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.
>
> Thanks.

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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]