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

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

 



On Thu, Sep 10, 2015 at 4:04 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Michael Rappazzo <rappazzo@xxxxxxxxx> 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 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]