On Thu, Sep 10, 2015 at 4:04 PM, Junio C Hamano <[email protected]> wrote:
> Michael Rappazzo <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
