Re: [PATCH v7 1/3] worktree: add top-level worktree.c
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.
+#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.
+ 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.
+ 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?
+ } else if (is_detached) {
+ *is_detached = 1;
+ }
+ }+ return 0; +} +
+struct worktree_list *get_worktree_list()
struct worktree_list *get_worktree_list(void)
+{
+ 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.
+ list->worktree = current_worktree; + list->next = NULL; + current_entry = list; + }
If the above if() is not taken, current_entry is left as NULL
+ 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.
+ 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);
+ +/* + * 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