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

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

 



On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> I realize that this is modeled closely after existing code in
> branch.c, but, with the exception of parsing the ref file and
> constructing a worktree structure, the main worktree case (id == NULL)
> is entirely disjoint from the linked worktree case (id != NULL). This
> suggests strongly that get_worktree() should be split into two
> functions, one for the main worktree and one for linked worktrees,
> which would make the code easier to understand. You might call the
> functions get_main_worktree() and get_linked_worktree(id) (or perhaps
> drop "linked" from the latter name).

I originally wrote it like that, but I felt that the code looked like
it was mostly duplicated in the functions.
I can give it a relook.

>> +
>> +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.

>> diff --git a/worktree.h b/worktree.h
>> new file mode 100644
>> index 0000000..2bc0ab8
>> --- /dev/null
>> +++ b/worktree.h
>> @@ -0,0 +1,48 @@
>> +#ifndef WORKTREE_H
>> +#define WORKTREE_H
>> +
>> +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'. The slight complication with this approach,
> though, is that get_worktrees() either also needs to return the length
> of the array, or the array should end with some sort of end-of-array
> sentinel. An obvious sentinel would be path==NULL or git_dir==NULL or
> all of the above.
>
> Client iteration is just about the same with the array approach as
> with the linked-list approach.
>

I can't see what benefit this would provide.  I would sooner change
the returned list into
an array-backed list struct.  Alternatively, I think adding a
list_head pointer to this structure
could benefit client code.
--
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]