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

> Michael Rappazzo <rappazzo@gmail.com> 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?

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

>
>
> 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?
>