Re: [PATCH v7 1/3] worktree: add top-level worktree.c
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Sep 13, 2015, 02:39 UTC
- Message-ID
- <CAPig+cQRx-uKZnnx2O0pG34HkcEdg1GbGNFd9PHdw+m0O3aJEA@mail.gmail.com>
- In-Reply-To
- <1441402769-35897-2-git-send-email-rappazzo@gmail.com>
On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> Including functions to get the list of all worktrees, and to get > a specific worktree (primary or linked).
Some comments below in addition to those by Junio...
Show 22 quoted lines
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> 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"
> +#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);It's safe to invoke free() on NULL, so as long as worktree->head_ref holds NULL in the detached case, then there's no need for the conditional at all (or the comment).
Show 21 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);
> + 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)
> + */
> +int _parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)Mentioned already by Junio in his review of patch 2: Make this function static and drop the leading underscore from the name.
Show 5 quoted lines
> +{
> + if (!strbuf_readlink(ref, path_to_ref, 0)) {
> + if (!starts_with(ref->buf, "refs/") || check_refname_format(ref->buf, 0)) {
> + /* invalid ref - something is awry with this repo */
> + return 1;In this codebase, it's common to return -1 on error. You could also probably drop the unnecessary braces (here and a few other places).
Show 7 quoted lines
> + }
> + } 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);
> + } else if (is_detached) {
> + *is_detached = 1;The code allows the caller to pass in NULL for is_detached if not interested in this information, however, the comment block documenting the function doesn't mention this.
Also, don't you need to clear *is_detached when not detached since you don't know what garbage value *is_detached holds upon entry?
I find the placement of the detached detection logic here a bit strange. The only case for which it might trigger is the so-called "main worktree", yet having it in this general purpose parse function seems to imply misleadingly that any worktree could be detached. Also, given the current world order[1], wouldn't missing "ref:" indicate an error for any worktree other than the main one? Consequently, this detection probably ought to be done only for the main worktree (outside of this function, probably).
[1]: Though, this might not hold true in a future world order involving merging of notes, if I correctly understood that discussion, since notes merging wouldn't require a "worktree", per se.
Show 40 quoted lines
> + }
> + }
> + return 0;
> +}
> +
> +struct worktree *get_worktree(const char *id)
> +{
> + struct worktree *worktree = NULL;
> + struct strbuf path = STRBUF_INIT;
> + struct strbuf worktree_path = STRBUF_INIT;
> + struct strbuf git_dir = STRBUF_INIT;
> + struct strbuf head_ref = STRBUF_INIT;
> + int is_bare = 0;
> + int is_detached = 0;
> +
> + if (id) {
> + strbuf_addf(&git_dir, "%s/worktrees/%s", absolute_path(get_git_common_dir()), id);
> + strbuf_addf(&path, "%s/gitdir", git_dir.buf);
> + if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0) {
> + /* invalid git_dir file */
> + goto done;
> + }
> + strbuf_rtrim(&worktree_path);
> + if (!strbuf_strip_suffix(&worktree_path, "/.git")) {
> + strbuf_reset(&worktree_path);
> + strbuf_addstr(&worktree_path, absolute_path("."));
> + strbuf_strip_suffix(&worktree_path, "/.");
> + }
> +
> + strbuf_reset(&path);
> + strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
> + } else {
> + strbuf_addf(&git_dir, "%s", absolute_path(get_git_common_dir()));
> + strbuf_addf(&worktree_path, "%s", absolute_path(get_git_common_dir()));
> + is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
> + if (is_bare)
> + strbuf_strip_suffix(&worktree_path, "/.");
> +
> + strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
> + }I realize that this is modeled closely after existing code in branch.c, but, with the exception of parsing the ref file and constructing a worktree structure, the main worktree case (id == NULL) is entirely disjoint from the linked worktree case (id != NULL). This suggests strongly that get_worktree() should be split into two functions, one for the main worktree and one for linked worktrees, which would make the code easier to understand. You might call the functions get_main_worktree() and get_linked_worktree(id) (or perhaps drop "linked" from the latter name).
> + /* > + * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so for linked worktrees, > + * `resolve_ref_unsafe()` won't work (it uses git_path). Parse the ref ourselves. > + */
This comment probably belongs inside _parse_ref() as it's explaining its internal workings, rather than here at the call-site.
Show 10 quoted lines
> + if (_parse_ref(path.buf, &head_ref, &is_detached))
> + goto done;
> +
> + worktree = malloc(sizeof(struct worktree));
> + worktree->path = strbuf_detach(&worktree_path, NULL);
> + worktree->git_dir = strbuf_detach(&git_dir, NULL);
> + worktree->is_bare = is_bare;
> + worktree->head_ref = NULL;
> + worktree->is_detached = is_detached;
> + if (strlen(head_ref.buf) > 0) {More idiomatic:
if (head_ref.len) {Show 16 quoted lines
> + if (!is_detached) {
> + resolve_ref_unsafe(head_ref.buf, 0, worktree->head_sha1, NULL);
> + worktree->head_ref = strbuf_detach(&head_ref, NULL);
> + } else {
> + get_sha1_hex(head_ref.buf, worktree->head_sha1);
> + }
> + }
> +done:
> + strbuf_release(&path);
> + strbuf_release(&git_dir);
> + strbuf_release(&head_ref);
> + strbuf_release(&worktree_path);
> + return worktree;
> +}
> +
> +struct worktree_list *get_worktree_list()Can we be more concise and call this get_worktrees()?
Show 35 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));
> + list->worktree = current_worktree;
> + list->next = NULL;
> + current_entry = list;
> + }
> + 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));
> + current_entry = current_entry->next;
> + current_entry->worktree = current_worktree;
> + current_entry->next = NULL;
> + }
> + }
> + closedir(dir);
> + }
> +
> +done:What is this label for? It doesn't seem to have any corresponding goto's.
Show 26 quoted lines
> +
> + 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;
> +};I don't care too strongly, but an alternate approach (which I probably would have taken) would be to have get_worktrees() simply return an array of 'struct worktree' objects, hence no need for the additional 'struct worktree_list'. The slight complication with this approach, though, is that get_worktrees() either also needs to return the length of the array, or the array should end with some sort of end-of-array sentinel. An obvious sentinel would be path==NULL or git_dir==NULL or all of the above.
Client iteration is just about the same with the array approach as with the linked-list approach.
Show 33 quoted lines
> +/* 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(); > + > +/* > + * 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 > -- > 2.5.0