Michael Rappazzo <[email protected]> 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?
> 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?
> +
> +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
