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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 11, 2015, 23:10 UTC
Message-ID: <CAPig+cQ4BUbS0ZQ=NoDO9JZmWYRzX0vhoi-W9ahg3yaWM+pKQw@mail.gmail.com>
In-Reply-To: <xmqqbnd8zks9.fsf@gitster.mtv.corp.google.com>

On Fri, Sep 11, 2015 at 5:52 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 28 quoted lines

> Mike Rappazzo <rappazzo@gmail.com> writes:
>> On Fri, Sep 11, 2015 at 12:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Michael Rappazzo <rappazzo@gmail.com> writes:
>>>> The code formerly in branch.c was largely the basis of the
>>>> get_worktree_list implementation is now moved to worktree.c,
>>>> and the find_shared_symref implementation has been refactored
>>>> to use get_worktree_list
>>>
>>> Copying the bulk of the function in 1/3 and then removing the
>>> original here made it somewhat hard to compare what got changed in
>>> the implementation.
>>>
>>> I _think_ the code structure in the end result is more or less
>>> right, though.
>>
>> Should I squash these first two commits together in the next series?
>
> Mashing the two into one patch would not help anybody, I would
> suspect.
>
> I didn't try this myself, but if I were doing this and if I were
> aiming for perfection, then I would probably try to split it even
> finer.  Refactor the original while both the callers and the helpers
> are still inside branch.c (hence the early steps in the series will
> not benefit worktree.c at all---worktree.c may not even exist in
> them yet), move refactored helpers to worktree.[ch] (and at this
> step you may not even have get_worktree() etc. yet), and then use
> that refactored helper while writing new functions in worktree.c.

Thanks for bringing this up. I haven't found a sufficient block of time yet to review these patches, however, I had the same thought upon a quick initial read, and is how I was hoping to see the patch series constructed based upon my earlier two reviews suggesting refactoring the existing branch.c functions into a new get_worktrees(). There are at least a couple important reasons for taking this approach.

First, it keeps the "blame" trail intact, the full context of which can be helpful to someone trying to understand why the code is the way it is. The current approach of having get_worktree_list() materialize out of thin air (even though it may have been patterned after existing code) doesn't give the same benefit.

Second, it's easier for reviewers to understand and check the code for correctness if it mutates to the final form in small increments than it is to have get_worktrees() come into existence fully matured.

A final minor comment: Since all three branch.c functions, die_if_checked_out(), find_shared_symref(), and find_linked_symref(), ought to be moved to top-level worktree.c, I'd probably have patch 1 do the relocation (with no functional changes), and subsequent patches refactor the moved code into a general purpose get_worktrees(). The final patch would then implement "git worktrees list".