Re: [PATCH v7 1/3] worktree: add top-level worktree.c
flat view
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:06:30
Michael Rappazzo [off-list ref] 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?
quoted hunk
diff --git a/worktree.c b/worktree.c new file mode 100644 index 0000000..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.
quoted hunk
+#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.
quoted hunk
+ 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.
quoted hunk
+ 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 hunk
+ */ +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).
quoted hunk
+ /* 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?
quoted hunk
+ } else if (is_detached) { + *is_detached = 1; + } + }
quoted hunk
+ return 0; +} +
+struct worktree_list *get_worktree_list()
struct worktree_list *get_worktree_list(void)
quoted hunk
+{ + 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.
quoted hunk
+ list->worktree = current_worktree; + list->next = NULL; + current_entry = list; + }
If the above if() is not taken, current_entry is left as NULL
quoted hunk
+ strbuf_addf(&path, "%s/worktrees", get_git_common_dir()); + dir = opendir(path.buf); + strbuf_release(&path); + 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.
quoted hunk
+ 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?
quoted hunk
+ +done: + + return list; +} +diff --git a/worktree.h b/worktree.h new file mode 100644 index 0000000..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; +}; + +/* Functions for acting on the information about worktrees. */ + +/* + * Get the list of all worktrees. The primary worktree will always be + * the first in the list followed by any other (linked) worktrees created + * by `git worktree add`. No specific ordering is done on the linked + * worktrees. + * + * The caller is responsible for freeing the memory from the returned list. + * (See worktree_list_release for this purpose). + */ +extern struct worktree_list *get_worktree_list();
extern struct worktree_list *get_worktree_list(void);
quoted hunk
+ +/* + * generic method to get a worktree + * - if 'id' is NULL, get the from $GIT_COMMON_DIR + * - if 'id' is not NULL, get the worktree found in $GIT_COMMON_DIR/worktrees/id if + * such a worktree exists + * + * The caller is responsible for freeing the memory from the returned + * worktree. (See worktree_release for this purpose) + */ +struct worktree *get_worktree(const char *id); + +/* + * Free up the memory for a worktree_list/worktree + */ +extern void worktree_list_release(struct worktree_list *); +extern void worktree_release(struct worktree *); + +#endif