git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Sep 10, 2015, 20:04 UTC
Message-ID
<xmqqk2ry2g8e.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<1441402769-35897-2-git-send-email-rappazzo@gmail.com>
Michael Rappazzo <rappazzo@gmail.com> 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?

Show 9 quoted lines
> 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.
Show 12 quoted lines
> +#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.

Show 10 quoted lines
> +		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.

Show 8 quoted lines
> +		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.
Show 5 quoted lines
> + */
> +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).
Show 7 quoted lines
> +			/* 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)
Show 11 quoted lines
> +{
> +	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
Show 11 quoted lines
> +	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.
Show 7 quoted lines
> +				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?

Show 41 quoted lines
> +
> +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);
Show 19 quoted lines
> +
> +/*
> + * 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
Previous: Michael RappazzoNext: Mike Rappazzo
Message 3 of 27 in “worktree: worktree.c functions and list builtin command”
  1. 0/3 worktree: worktree.c functions and list builtin commandMichael Rappazzo, Sep 4, 2015
  2. 1/3 worktree: add top-level worktree.cMichael Rappazzo, Sep 4, 2015
  3. Junio C HamanoSep 10, 2015
  4. Mike RappazzoSep 11, 2015
  5. Eric SunshineSep 13, 2015
  6. Eric SunshineSep 13, 2015
  7. Mike RappazzoSep 14, 2015
  8. Junio C HamanoSep 14, 2015
  9. Eric SunshineSep 16, 2015
  10. Eric SunshineSep 16, 2015
  11. Mike RappazzoSep 16, 2015
  12. Eric SunshineSep 22, 2015
  13. Junio C HamanoSep 22, 2015
  14. 2/3 worktree: move/refactor find_shared_symref from branch.cMichael Rappazzo, Sep 4, 2015
  15. Junio C HamanoSep 11, 2015
  16. Mike RappazzoSep 11, 2015
  17. Junio C HamanoSep 11, 2015
  18. Eric SunshineSep 11, 2015
  19. Mike RappazzoSep 12, 2015
  20. Eric SunshineSep 13, 2015
  21. Mike RappazzoSep 14, 2015
  22. Eric SunshineSep 16, 2015
  23. Mike RappazzoSep 16, 2015
  24. Eric SunshineSep 22, 2015
  25. 3/3 worktree: add 'list' commandMichael Rappazzo, Sep 4, 2015
  26. Junio C HamanoSep 11, 2015
  27. Eric SunshineSep 13, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.