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