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

On Thu, Sep 10, 2015 at 4:04 PM, Junio C Hamano [off-list ref] wrote:

Michael Rappazzo [off-list ref] writes:

quoted

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.

quoted

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

quoted

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

quoted

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