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

[Eric Sunshine]

On Wed, Sep 16, 2015 at 4:49 PM, Mike Rappazzo  wrote:
> On Wed, Sep 16, 2015 at 4:32 PM, Eric Sunshine  
> wrote:
>> On Mon, Sep 14, 2015 at 8:20 AM, Mike Rappazzo  wrote:
>>> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine  
>>> wrote:
 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.
>>
>> The benefit is a reduction in complexity, which is an important goal.
>> Linked lists are inherently more complex than arrays, requiring extra
>> effort, and especially extra care, to manage the head, each "next"
>> pointer (and possibly a tail pointer). Arrays are used often in Git,
>> thus are familiar in this code-base, and Git has decent support for
>> making their management relatively painless. Your suggestions of
>> changing into "an array-backed list structure" or "adding list_head
>> pointer" increase complexity, rather than reduce it.
>>
>> Aside from the complexity issue, arrays allow random access, whereas
>> linked lists allow only sequential access. While this limitation may
>> not impact your current, planned use of get_worktrees(), it places a
>> potentially unnecessary restriction on future clients.
>>
>> And, as noted, client iteration is at least as convenient with arrays,
>> if not moreso, due to the reduction in noise ("p++" rather than "p =
>> p->next").
>>
>> struct worktree *p;
>> for (p = get_worktrees(); p->path; p++)
>> blarp(p);
>>
>> There are cases where linked lists are an obvious win, but this
>> doesn't seem to be such a case. On the other hand, there are obvious
>> benefits to making this an array, such as reduced complexity and
>> removal of the sequential-access-only restriction.
>
> What you are suggesting makes sense, but I feel it falls short when it
> comes to the "sentinel".  Would the last element in the list be a
> dummy worktree?  I would sooner add a NULL to the end.  Would that be
> an acceptable implementation?

The sentinel would indeed be a dummy worktree structure, which may be
very slightly ugly, however, it's dead simple, both in implementation
and from client viewpoint, whereas other approaches increase
complexity.

Making the last entry a NULL means get_worktrees() would have to
return an array of pointers rather than an array of structures, which
is more syntactically noisy, and complex since it's harder to reason
about pointer-to-pointer. In my mind, at least, the simplicity of the
array of structures approach (even with the slight ugliness of the
dummy sentinel) outweighs the complexity of the array-of-pointers
approach.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Eric Sunshine  writes:

> Making the last entry a NULL means get_worktrees() would have to
> return an array of pointers rather than an array of structures, which
> is more syntactically noisy, and complex since it's harder to reason
> about pointer-to-pointer. In my mind, at least, the simplicity of the
> array of structures approach (even with the slight ugliness of the
> dummy sentinel) outweighs the complexity of the array-of-pointers
> approach.

Hmph, I think the bog-standard way in our code is an array of
pointers.  An array of structures have a few downsides:

 - You have to copy a lot when you do realloc(3);

 - You have to move a lot when you insert a new element;

 - Individual structure cannot be pointed at by a pointer sensibly.
   Passing [4] to a function that expects "struct worktree *"
   is unsafe unless you are sure nobody is mucking with the worktree[]
   array.

For the read-only operation "worktree list", the last one may not
matter much because you would build all the elements before doing
anything else, but once you want to run this inside a library and
maintain the in-core forest of worktrees that are in sync with what
your running process does (i.e. create a new or destroy an existing
worktree), it may become problematic.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mike Rappazzo]

On Wed, Sep 16, 2015 at 4:32 PM, Eric Sunshine  wrote:
> On Mon, Sep 14, 2015 at 8:20 AM, Mike Rappazzo  wrote:
>> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine  
>> wrote:
 +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.
>
> The benefit is a reduction in complexity, which is an important goal.
> Linked lists are inherently more complex than arrays, requiring extra
> effort, and especially extra care, to manage the head, each "next"
> pointer (and possibly a tail pointer). Arrays are used often in Git,
> thus are familiar in this code-base, and Git has decent support for
> making their management relatively painless. Your suggestions of
> changing into "an array-backed list structure" or "adding list_head
> pointer" increase complexity, rather than reduce it.
>
> Aside from the complexity issue, arrays allow random access, whereas
> linked lists allow only sequential access. While this limitation may
> not impact your current, planned use of get_worktrees(), it places a
> potentially unnecessary restriction on future clients.
>
> And, as noted, client iteration is at least as convenient with arrays,
> if not moreso, due to the reduction in noise ("p++" rather than "p =
> p->next").
>
> struct worktree *p;
> for (p = get_worktrees(); p->path; p++)
> blarp(p);
>
> There are cases where linked lists are an obvious win, but this
> doesn't seem to be such a case. On the other hand, there are obvious
> benefits to making this an array, such as reduced complexity and
> removal of the sequential-access-only restriction.

What you are suggesting makes sense, but I feel it falls short when it
comes to the "sentinel".  Would the last element in the list be a
dummy worktree?  I would sooner add a NULL to the end.  Would that be
an acceptable implementation?

I have re-coded it to put the next pointer in the worktree structure
as Junio has suggested, but I am open to changing it to use the array
approach.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Mon, Sep 14, 2015 at 8:20 AM, Mike Rappazzo  wrote:
> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine  
> wrote:
>>> +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.

The benefit is a reduction in complexity, which is an important goal.
Linked lists are inherently more complex than arrays, requiring extra
effort, and especially extra care, to manage the head, each "next"
pointer (and possibly a tail pointer). Arrays are used often in Git,
thus are familiar in this code-base, and Git has decent support for
making their management relatively painless. Your suggestions of
changing into "an array-backed list structure" or "adding list_head
pointer" increase complexity, rather than reduce it.

Aside from the complexity issue, arrays allow random access, whereas
linked lists allow only sequential access. While this limitation may
not impact your current, planned use of get_worktrees(), it places a
potentially unnecessary restriction on future clients.

And, as noted, client iteration is at least as convenient with arrays,
if not moreso, due to the reduction in noise ("p++" rather than "p =
p->next").

struct worktree *p;
for (p = get_worktrees(); p->path; p++)
blarp(p);

There are cases where linked lists are an obvious win, but this
doesn't seem to be such a case. On the other hand, there are obvious
benefits to making this an array, such as reduced complexity and
removal of the sequential-access-only restriction.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Mon, Sep 14, 2015 at 1:41 PM, Junio C Hamano  wrote:
> Mike Rappazzo  writes:
>> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine  
>> 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.

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

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

[Mike Rappazzo]

On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine  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 000..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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Mike Rappazzo  writes:

> On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine  
> 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()?

>>> diff --git a/worktree.h b/worktree.h
>>> new file mode 100644
>>> index 000..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'.

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

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

[Eric Sunshine]

On Sat, Sep 12, 2015 at 10:39 PM, Eric Sunshine  wrote:
> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo  wrote:
>> +   }
>> +   } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
>> +   if (starts_with(ref->buf, "ref:")) {
>> +   strbuf_remove(ref, 0, strlen("ref:"));
>> +   strbuf_trim(ref);
>> +   } else if (is_detached) {
>> +   *is_detached = 1;
>
> I find the placement of the detached detection logic here a bit
> strange. The only case for which it might trigger is the so-called
> "main worktree", yet having it in this general purpose parse function
> seems to imply misleadingly that any worktree could be detached. Also,
> given the current world order[1], wouldn't missing "ref:" indicate an
> error for any worktree other than the main one? Consequently, this
> detection probably ought to be done only for the main worktree
> (outside of this function, probably).

Eh, ignore this bit. My brain was conflating 'bare' and 'detached'.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo  wrote:
> Including functions to get the list of all worktrees, and to get
> a specific worktree (primary or linked).

Some comments below in addition to those by Junio...

> Signed-off-by: Michael Rappazzo 
> ---
> diff --git a/worktree.c b/worktree.c
> new file mode 100644
> index 000..33d2e57
> --- /dev/null
> +++ b/worktree.c
> @@ -0,0 +1,157 @@
> +#include "worktree.h"
> +#include "cache.h"
> +#include "git-compat-util.h"
> +#include "refs.h"
> +#include "strbuf.h"
> +
> +void worktree_release(struct worktree *worktree)
> +{
> +   if (worktree) {
> +   free(worktree->path);
> +   free(worktree->git_dir);
> +   if (!worktree->is_detached) {
> +   /* could be headless */
> +   free(worktree->head_ref);

It's safe to invoke free() on NULL, so as long as worktree->head_ref
holds NULL in the detached case, then there's no need for the
conditional at all (or the comment).

> +   }
> +   free(worktree);
> +   }
> +}
> +
> +void worktree_list_release(struct worktree_list *worktree_list)
> +{
> +   while (worktree_list) {
> +   struct worktree_list *next = worktree_list->next;
> +   worktree_release(worktree_list->worktree);
> +   free (worktree_list);
> +   worktree_list = next;
> +   }
> +}
> +
> +/*
> + * read 'path_to_ref' into 'ref'.  Also set is_detached to 1 if the ref is 
> detatched
> + *
> + * return 1 if the ref is not a proper ref, 0 otherwise (success)
> + */
> +int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)

Mentioned already by Junio in his review of patch 2: Make this
function static and drop the leading underscore from the name.

> +{
> +   if (!strbuf_readlink(ref, path_to_ref, 0)) {
> +   if (!starts_with(ref->buf, "refs/") || 
> check_refname_format(ref->buf, 0)) {
> +   /* invalid ref - something is awry with this repo */
> +   return 1;

In this codebase, it's common to return -1 on error. You could also
probably drop the unnecessary braces (here and a few other places).

> +   }
> +   } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
> +   if (starts_with(ref->buf, "ref:")) {
> +   strbuf_remove(ref, 0, strlen("ref:"));
> +   strbuf_trim(ref);
> +   } else if (is_detached) {
> +   *is_detached = 1;

The code allows the caller to pass in NULL for is_detached if not
interested in this information, however, the comment block documenting
the function doesn't mention this.

Also, don't you need to clear *is_detached when not detached since you
don't know what garbage value *is_detached holds upon entry?

I find the placement of the detached detection logic here a bit
strange. The only case for which it might trigger is the so-called
"main worktree", yet having it in this general purpose parse function
seems to imply misleadingly that any worktree could be detached. Also,
given the current world order[1], wouldn't missing "ref:" indicate an
error for any worktree other than the main one? Consequently, this
detection probably ought to be done only for the main worktree
(outside of this function, probably).

[1]: Though, this might not hold true in a future world order
involving merging of notes, if I correctly understood that discussion,
since notes merging wouldn't require a "worktree", per se.

> +   }
> +   }
> +   return 0;
> +}
> +
> +struct worktree *get_worktree(const char *id)
> +{
> +   struct worktree *worktree = NULL;
> +   struct strbuf path = STRBUF_INIT;
> +   struct strbuf worktree_path = STRBUF_INIT;
> +   struct strbuf git_dir = STRBUF_INIT;
> +   struct strbuf head_ref = STRBUF_INIT;
> +   int is_bare = 0;
> +   int is_detached = 0;
> +
> +   if (id) {
> +   strbuf_addf(_dir, "%s/worktrees/%s", 
> absolute_path(get_git_common_dir()), id);
> +   strbuf_addf(, "%s/gitdir", git_dir.buf);
> +   if (strbuf_read_file(_path, path.buf, 0) <= 0) {
> +   /* invalid git_dir file */
> +   goto done;
> +   }
> +   strbuf_rtrim(_path);
> +   if (!strbuf_strip_suffix(_path, "/.git")) {
> +   strbuf_reset(_path);
> +   strbuf_addstr(_path, absolute_path("."));
> +   strbuf_strip_suffix(_path, "/.");
> +   }
> +
> +   strbuf_reset();
> +   strbuf_addf(, "%s/worktrees/%s/HEAD", 
> get_git_common_dir(), id);
> +   } else {
> +   strbuf_addf(_dir, "%s", 
> absolute_path(get_git_common_dir()));
> +   strbuf_addf(_path, "%s", 
> absolute_path(get_git_common_dir()));
> +  

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

[Mike Rappazzo]

On Thu, Sep 10, 2015 at 4:04 PM, Junio C Hamano  wrote:
> Michael Rappazzo  writes:
>
>> Including functions to get the list of all worktrees, and to get
>> a specific worktree (primary or linked).
>
> Was this meant as a continuation of the sentence started on the
> Subject line, or is s/Including/Include/ necessary?

I think I was continuing the subject line.  I will make it nicer.

>
>> + worktree_list = next;
>> + }
>> +}
>> +
>> +/*
>> + * read 'path_to_ref' into 'ref'.  Also set is_detached to 1 if the ref is 
>> detatched
>> + *
>> + * return 1 if the ref is not a proper ref, 0 otherwise (success)
>
> These lines are overly long.
>
>> + */
>> +int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
>> +{
>> + if (!strbuf_readlink(ref, path_to_ref, 0)) {
>> + if (!starts_with(ref->buf, "refs/") || 
>> check_refname_format(ref->buf, 0)) {
>
> An overly long line.  Perhaps
>
> if (!starts_with(ref->buf, "refs/") ||
> check_refname_format(ref->buf, 0)) {
>
> (I see many more "overly long line" after this point, which I won't mention).


Is the limit 80 characters?

>
>> + /* invalid ref - something is awry with this repo */
>> + return 1;
>> + }
>> + } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
>> + if (starts_with(ref->buf, "ref:")) {
>> + strbuf_remove(ref, 0, strlen("ref:"));
>> + strbuf_trim(ref);
>
> We don't do the same "starts_with() and format is sane" check?

The code from this snippet was mostly ripped from branch.c (see the
second commit).
I will add the sanity check.

>
>
> An idiomatic way to extend a singly-linked list at the end in our
> codebase is to use a pointer to the pointer that has the element at
> the end, instead of to use a pointer that points at the element at
> the end or NULL (i.e. the pointer the above code calls current_entry
> is "struct worktree_list **end_of_list").  Would it make the above
> simpler if you followed that pattern?
>

I think I follow what you are saying here.  I will explore this route.
I am also unhappy about
having to separately maintain a point to the head of the list when
using it.  Would it be
"normal" to add a head pointer to the list structure?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Michael Rappazzo  writes:

> Including functions to get the list of all worktrees, and to get
> a specific worktree (primary or linked).

Was this meant as a continuation of the sentence started on the
Subject line, or is s/Including/Include/ necessary?

> diff --git a/worktree.c b/worktree.c
> new file mode 100644
> index 000..33d2e57
> --- /dev/null
> +++ b/worktree.c
> @@ -0,0 +1,157 @@
> +#include "worktree.h"
> +#include "cache.h"
> +#include "git-compat-util.h"

The first header to be included must be "git-compat-util.h" or
"cache.h", the latter of which is so well known to include the
former as the first thing (so inclusion of "git-compat-util.h"
becomes unnecessary).

After that, include your own header as necessary.

> +#include "refs.h"
> +#include "strbuf.h"
> +
> +void worktree_release(struct worktree *worktree)
> +{
> + if (worktree) {
> + free(worktree->path);
> + free(worktree->git_dir);
> + if (!worktree->is_detached) {
> + /* could be headless */
> + free(worktree->head_ref);
> + }

Unnecessary brace {} pair?  It is OK to keep them if later patches
in the series will make this a multi-statement block, though.

> + free(worktree);
> + }
> +}
> +
> +void worktree_list_release(struct worktree_list *worktree_list)
> +{
> + while (worktree_list) {
> + struct worktree_list *next = worktree_list->next;
> + worktree_release(worktree_list->worktree);
> + free (worktree_list);

No SP between function name and the parenthesis that introduces its
arguments.

> + worktree_list = next;
> + }
> +}
> +
> +/*
> + * read 'path_to_ref' into 'ref'.  Also set is_detached to 1 if the ref is 
> detatched
> + *
> + * return 1 if the ref is not a proper ref, 0 otherwise (success)

These lines are overly long.

> + */
> +int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
> +{
> + if (!strbuf_readlink(ref, path_to_ref, 0)) {
> + if (!starts_with(ref->buf, "refs/") || 
> check_refname_format(ref->buf, 0)) {

An overly long line.  Perhaps 

if (!starts_with(ref->buf, "refs/") ||
check_refname_format(ref->buf, 0)) {

(I see many more "overly long line" after this point, which I won't mention).

> + /* invalid ref - something is awry with this repo */
> + return 1;
> + }
> + } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
> + if (starts_with(ref->buf, "ref:")) {
> + strbuf_remove(ref, 0, strlen("ref:"));
> + strbuf_trim(ref);

We don't do the same "starts_with() and format is sane" check?

> + } else if (is_detached) {
> + *is_detached = 1;
> + }
> + }

> + return 0;
> +}
> +

> +struct worktree_list *get_worktree_list()

struct worktree_list *get_worktree_list(void)

> +{
> + struct worktree_list *list = NULL;
> + struct worktree_list *current_entry = NULL;
> + struct worktree *current_worktree = NULL;
> + struct strbuf path = STRBUF_INIT;
> + DIR *dir;
> + struct dirent *d;
> +
> + current_worktree = get_worktree(NULL);
> + if (current_worktree) {
> + list = malloc(sizeof(struct worktree_list));

Always use x*alloc() family of functions.

> + list->worktree = current_worktree;
> + list->next = NULL;
> + current_entry = list;
> + }

If the above if() is not taken, current_entry is left as NULL

> + strbuf_addf(, "%s/worktrees", get_git_common_dir());
> + dir = opendir(path.buf);
> + strbuf_release();
> + if (dir) {
> + while ((d = readdir(dir)) != NULL) {
> + if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> + continue;
> +
> + current_worktree = get_worktree(d->d_name);
> + if (current_worktree) {
> + current_entry->next = malloc(sizeof(struct 
> worktree_list));

But this line assumes that current_entry is never NULL.

> + current_entry = current_entry->next;
> + current_entry->worktree = current_worktree;
> + current_entry->next = NULL;
> + }
> + }
> + closedir(dir);
> + }

An idiomatic way to extend a singly-linked list at the end in our
codebase is to use a pointer to the pointer that has the element at
the end, instead of to use a pointer that points at the element at
the end or NULL (i.e. the pointer the above code calls current_entry
is "struct worktree_list **end_of_list").  Would it make the above
simpler if you followed that pattern?

> +
> +done:
> +
> + return list;
> +}
> +
> diff --git a/worktree.h b/worktree.h
> 

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

[Michael Rappazzo]

Including functions to get the list of all worktrees, and to get
a specific worktree (primary or linked).

Signed-off-by: Michael Rappazzo 
---
 Makefile   |   1 +
 worktree.c | 157 +
 worktree.h |  48 +++
 3 files changed, 206 insertions(+)
 create mode 100644 worktree.c
 create mode 100644 worktree.h

diff --git a/Makefile b/Makefile
index e326fa0..0131fed 100644
--- a/Makefile
+++ b/Makefile
@@ -807,6 +807,7 @@ LIB_OBJS += version.o
 LIB_OBJS += versioncmp.o
 LIB_OBJS += walker.o
 LIB_OBJS += wildmatch.o
+LIB_OBJS += worktree.o
 LIB_OBJS += wrapper.o
 LIB_OBJS += write_or_die.o
 LIB_OBJS += ws.o
diff --git a/worktree.c b/worktree.c
new file mode 100644
index 000..33d2e57
--- /dev/null
+++ b/worktree.c
@@ -0,0 +1,157 @@
+#include "worktree.h"
+#include "cache.h"
+#include "git-compat-util.h"
+#include "refs.h"
+#include "strbuf.h"
+
+void worktree_release(struct worktree *worktree)
+{
+   if (worktree) {
+   free(worktree->path);
+   free(worktree->git_dir);
+   if (!worktree->is_detached) {
+   /* could be headless */
+   free(worktree->head_ref);
+   }
+   free(worktree);
+   }
+}
+
+void worktree_list_release(struct worktree_list *worktree_list)
+{
+   while (worktree_list) {
+   struct worktree_list *next = worktree_list->next;
+   worktree_release(worktree_list->worktree);
+   free (worktree_list);
+   worktree_list = next;
+   }
+}
+
+/*
+ * read 'path_to_ref' into 'ref'.  Also set is_detached to 1 if the ref is 
detatched
+ *
+ * return 1 if the ref is not a proper ref, 0 otherwise (success)
+ */
+int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
+{
+   if (!strbuf_readlink(ref, path_to_ref, 0)) {
+   if (!starts_with(ref->buf, "refs/") || 
check_refname_format(ref->buf, 0)) {
+   /* invalid ref - something is awry with this repo */
+   return 1;
+   }
+   } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
+   if (starts_with(ref->buf, "ref:")) {
+   strbuf_remove(ref, 0, strlen("ref:"));
+   strbuf_trim(ref);
+   } else if (is_detached) {
+   *is_detached = 1;
+   }
+   }
+   return 0;
+}
+
+struct worktree *get_worktree(const char *id)
+{
+   struct worktree *worktree = NULL;
+   struct strbuf path = STRBUF_INIT;
+   struct strbuf worktree_path = STRBUF_INIT;
+   struct strbuf git_dir = STRBUF_INIT;
+   struct strbuf head_ref = STRBUF_INIT;
+   int is_bare = 0;
+   int is_detached = 0;
+
+   if (id) {
+   strbuf_addf(_dir, "%s/worktrees/%s", 
absolute_path(get_git_common_dir()), id);
+   strbuf_addf(, "%s/gitdir", git_dir.buf);
+   if (strbuf_read_file(_path, path.buf, 0) <= 0) {
+   /* invalid git_dir file */
+   goto done;
+   }
+   strbuf_rtrim(_path);
+   if (!strbuf_strip_suffix(_path, "/.git")) {
+   strbuf_reset(_path);
+   strbuf_addstr(_path, absolute_path("."));
+   strbuf_strip_suffix(_path, "/.");
+   }
+
+   strbuf_reset();
+   strbuf_addf(, "%s/worktrees/%s/HEAD", 
get_git_common_dir(), id);
+   } else {
+   strbuf_addf(_dir, "%s", 
absolute_path(get_git_common_dir()));
+   strbuf_addf(_path, "%s", 
absolute_path(get_git_common_dir()));
+   is_bare = !strbuf_strip_suffix(_path, "/.git");
+   if (is_bare)
+   strbuf_strip_suffix(_path, "/.");
+
+   strbuf_addf(, "%s/HEAD", get_git_common_dir());
+   }
+
+   /*
+* $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR 
so for linked worktrees,
+* `resolve_ref_unsafe()` won't work (it uses git_path). Parse the ref 
ourselves.
+*/
+   if (_parse_ref(path.buf, _ref, _detached))
+   goto done;
+
+   worktree = malloc(sizeof(struct worktree));
+   worktree->path = strbuf_detach(_path, NULL);
+   worktree->git_dir = strbuf_detach(_dir, NULL);
+   worktree->is_bare = is_bare;
+   worktree->head_ref = NULL;
+   worktree->is_detached = is_detached;
+   if (strlen(head_ref.buf) > 0) {
+   if (!is_detached) {
+   resolve_ref_unsafe(head_ref.buf, 0, 
worktree->head_sha1, NULL);
+   worktree->head_ref = strbuf_detach(_ref, NULL);
+   } else {
+   get_sha1_hex(head_ref.buf, worktree->head_sha1);
+   }
+   }
+done:
+   strbuf_release();
+   strbuf_release(_dir);
+