Re: [PATCH v7 1/3] worktree: add top-level worktree.c

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 16, 2015, 20:42 UTC
Message-ID: <CAPig+cQ9QpGUZCaZXG3avChVdpXGoKXbKAc9vYUCFWMSUE-B4g@mail.gmail.com>
In-Reply-To: <xmqqwpvsx5ie.fsf@gitster.mtv.corp.google.com>

On Mon, Sep 14, 2015 at 1:41 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> Mike Rappazzo <rappazzo@gmail.com> writes:
>> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> +struct worktree_list *get_worktree_list()
>>>
>>> Can we be more concise and call this get_worktrees()?
>>
>> I prefer 'get_worktree_list' because I also added the 'get_worktree'
>> function, and I wanted to differentiate
>> the function names.
>
> I'd say that plural can be differentiating enough; it probably is a
> matter of taste.  How often do external callers want to call
> get_worktree() and not get_worktrees()?

The shorter name, get_worktrees(), also has the minor benefit of concision, similar to the way we use short variable names (i, j, n, p, s) to help reveal and (often) make code structure obvious at a glance; whereas long, noisy, wordy names tend to obscure code structure.

The "_list" suffix doesn't add any value over the shorter pluralizing "s"; in fact, it may be (very, very slightly) detrimental in implying too strongly that the return value must be a linked list.

Show 24 quoted lines

>>>> +struct worktree {
>>>> +       char *path;
>>>> +       char *git_dir;
>>>> +       char *head_ref;
>>>> +       unsigned char head_sha1[20];
>>>> +       int is_detached;
>>>> +       int is_bare;
>>>> +};
>>>> +
>>>> +struct worktree_list {
>>>> +       struct worktree *worktree;
>>>> +       struct worktree_list *next;
>>>> +};
>>>
>>> I don't care too strongly, but an alternate approach (which I probably
>>> would have taken) would be to have get_worktrees() simply return an
>>> array of 'struct worktree' objects, hence no need for the additional
>>> 'struct worktree_list'.
>
> I do not think we are using this to hold thousands of worktree
> objects in core.  Adding "struct worktree *next" pointer to the
> worktree object itself would probably be sufficient for the need of
> codepaths that want to enumerate and iterate over them and that
> would be another way to lose the extra structure.

I was more concerned with the inherent (and, in this case, unnecessary) complexity of a linked list. Being able to drop the extra 'worktree_list' structure was just an added benefit of moving to the simpler array approach.