Re: [PATCH 2/2] worktree.c: use submodule interface to access refs from another worktree

[Michael Haggerty <mhagger@alum.mit.edu>]

On 02/08/2017 12:31 PM, Nguyễn Thái Ngọc Duy wrote:
> The patch itself is relatively simple: manual parsing code is replaced
> with a call to resolve_ref_submodule(). The manual parsing code must die
> because only refs/files-backend.c should do that. Why submodule here is
> a more interesting question.
> 
> From an outside look, any .git/worktrees/foo is seen as a "normal"
> repository. You can set GIT_DIR to it and have access to everything,
> even shared things that are not literally inside that directory, like
> object db or shared refs.
> 
> On top of that, linked worktrees point to those directories with ".git"
> files. These two make a linked worktree's path "X" a "submodule" (*) (**)
> because X/.git is a file that points to a repository somewhere.
> 
> As such, we can just linked worktree's path as a submodule. We just need
> to make sure they are unique because they are used to lookup submodule
> refs store.
> 
> Main worktree is a a bit trickier. If we stand at a linked worktree, we
> may still need to peek into main worktree's HEAD, for example. We can
> treat main worktree's path as submodule as well since git_path_submodule()
> can tolerate ".git" dirs, in addition to ".git" files.
> 
> The constraint is, if main worktree is X, then the git repo must be at
> X/.git. If the user separates .git repo far away and tell git to point
> to it via GIT_DIR or something else, then the "main worktree as submodule"
> trick fails. Within multiple worktree context, I think we can limit
> support to "standard" layout, at least for now.
> 
> (*) The differences in sharing object database and refs between
> submodules and linked worktrees don't really matter in this context.
> 
> (**) At this point, we may want to rename refs *_submodule API to
> something more neutral, maybe s/_submodule/_remote/

It is unquestionably a good goal to avoid parsing references outside of
`refs/files-backend.c`. But I'm not a fan of this approach.

There are two meanings of the concept of a "ref store", and I think this
change muddles them:

1. The references that happen to be *physically* stored in a particular
   location, for example the `refs/bisect/*` references in a worktree.

2. The references that *logically* should be considered part of a
   particular repository. This might require stitching together
   references from multiple sources, for example `HEAD` and
   `refs/bisect` from a worktree's own directory with other
   references from the main repository.

Either of these concepts can be implemented via the `ref_store` abstraction.

The `ref_store` for a submodule should represent the references
logically visible from the submodule. The main program shouldn't care
whether the references are stored in a single physical location or
spread across multiple locations (for example, if the submodule were
itself a linked worktree).

The `ref_store` that you want here for a worktree is not the worktree's
*logical* `ref_store`. You want the worktree's *physical* `ref_store`.
Mixing logical and physical reference stores together is a bad idea
(even if we were willing to ignore the fact that worktrees are not
submodules in the accepted sense of the word).

The point of my `submodule-hash` branch [1] was to separate these
concepts better by breaking the current 1:1 connection between
`ref_store`s and submodules. This would allow `ref_store`s to be created
for other purposes, such as to represent worktree refs. If you want the
*logical* `ref_store` for a submodule, you access it through the
`submodule_ref_stores` table. If you want the *physical* `ref_store` for
a worktree, you should access it through a different table.

I think the best solution would be to expose the concept of `ref_store`
in the public refs API. Then users of submodules would essentially do

    struct ref_store *refs = get_submodule_refs(submodule_path);
    ... resolve_ref_recursively(refs, refname, 0, sha1, &flags) ...
    ... for_each_ref(refs, fn, cb_data) ...

whereas for a worktree you'd have to look up the `ref_store` instance
somewhere else (or maybe keep it as part of some worktree structure, if
there is one) but you would use it via the same API.

Michael

[1] https://github.com/mhagger/git

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  branch.c   |  3 +-
>  worktree.c | 99 +++++++++++++++-----------------------------------------------
>  worktree.h |  2 +-
>  3 files changed, 27 insertions(+), 77 deletions(-)
> 
> diff --git a/branch.c b/branch.c
> index b955d4f316..db5843718f 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -354,7 +354,8 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref)
>  	for (i = 0; worktrees[i]; i++) {
>  		if (worktrees[i]->is_detached)
>  			continue;
> -		if (strcmp(oldref, worktrees[i]->head_ref))
> +		if (worktrees[i]->head_ref &&
> +		    strcmp(oldref, worktrees[i]->head_ref))
>  			continue;
>  
>  		if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
> diff --git a/worktree.c b/worktree.c
> index d633761575..25e5bc9a3e 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -19,54 +19,24 @@ void free_worktrees(struct worktree **worktrees)
>  	free (worktrees);
>  }
>  
> -/*
> - * read 'path_to_ref' into 'ref'.  Also if is_detached is not NULL,
> - * set is_detached to 1 (0) if the ref is detached (is not detached).
> - *
> - * $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.
> - *
> - * return -1 if the ref is not a proper ref, 0 otherwise (success)
> - */
> -static int parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
> -{
> -	if (is_detached)
> -		*is_detached = 0;
> -	if (!strbuf_readlink(ref, path_to_ref, 0)) {
> -		/* HEAD is symbolic link */
> -		if (!starts_with(ref->buf, "refs/") ||
> -				check_refname_format(ref->buf, 0))
> -			return -1;
> -	} else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
> -		/* textual symref or detached */
> -		if (!starts_with(ref->buf, "ref:")) {
> -			if (is_detached)
> -				*is_detached = 1;
> -		} else {
> -			strbuf_remove(ref, 0, strlen("ref:"));
> -			strbuf_trim(ref);
> -			if (check_refname_format(ref->buf, 0))
> -				return -1;
> -		}
> -	} else
> -		return -1;
> -	return 0;
> -}
> -
>  /**
> - * Add the head_sha1 and head_ref (if not detached) to the given worktree
> + * Update head_sha1, head_ref and is_detached of the given worktree
>   */
> -static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)
> +static void add_head_info(struct worktree *wt)
>  {
> -	if (head_ref->len) {
> -		if (worktree->is_detached) {
> -			get_sha1_hex(head_ref->buf, worktree->head_sha1);
> -		} else {
> -			resolve_ref_unsafe(head_ref->buf, 0, worktree->head_sha1, NULL);
> -			worktree->head_ref = strbuf_detach(head_ref, NULL);
> -		}
> -	}
> +	int flags;
> +	const char *target;
> +
> +	target = resolve_ref_submodule(wt->path, "HEAD",
> +				       RESOLVE_REF_READING,
> +				       wt->head_sha1, &flags);
> +	if (!target)
> +		return;
> +
> +	if (flags & REF_ISSYMREF)
> +		wt->head_ref = xstrdup(target);
> +	else
> +		wt->is_detached = 1;
>  }
>  
>  /**
> @@ -77,9 +47,7 @@ static struct worktree *get_main_worktree(void)
>  	struct worktree *worktree = NULL;
>  	struct strbuf path = STRBUF_INIT;
>  	struct strbuf worktree_path = STRBUF_INIT;
> -	struct strbuf head_ref = STRBUF_INIT;
>  	int is_bare = 0;
> -	int is_detached = 0;
>  
>  	strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
>  	is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
> @@ -91,13 +59,10 @@ static struct worktree *get_main_worktree(void)
>  	worktree = xcalloc(1, sizeof(*worktree));
>  	worktree->path = strbuf_detach(&worktree_path, NULL);
>  	worktree->is_bare = is_bare;
> -	worktree->is_detached = is_detached;
> -	if (!parse_ref(path.buf, &head_ref, &is_detached))
> -		add_head_info(&head_ref, worktree);
> +	add_head_info(worktree);
>  
>  	strbuf_release(&path);
>  	strbuf_release(&worktree_path);
> -	strbuf_release(&head_ref);
>  	return worktree;
>  }
>  
> @@ -106,8 +71,6 @@ static struct worktree *get_linked_worktree(const char *id)
>  	struct worktree *worktree = NULL;
>  	struct strbuf path = STRBUF_INIT;
>  	struct strbuf worktree_path = STRBUF_INIT;
> -	struct strbuf head_ref = STRBUF_INIT;
> -	int is_detached = 0;
>  
>  	if (!id)
>  		die("Missing linked worktree name");
> @@ -127,19 +90,14 @@ static struct worktree *get_linked_worktree(const char *id)
>  	strbuf_reset(&path);
>  	strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
>  
> -	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
> -		goto done;
> -
>  	worktree = xcalloc(1, sizeof(*worktree));
>  	worktree->path = strbuf_detach(&worktree_path, NULL);
>  	worktree->id = xstrdup(id);
> -	worktree->is_detached = is_detached;
> -	add_head_info(&head_ref, worktree);
> +	add_head_info(worktree);
>  
>  done:
>  	strbuf_release(&path);
>  	strbuf_release(&worktree_path);
> -	strbuf_release(&head_ref);
>  	return worktree;
>  }
>  
> @@ -334,8 +292,6 @@ const struct worktree *find_shared_symref(const char *symref,
>  					  const char *target)
>  {
>  	const struct worktree *existing = NULL;
> -	struct strbuf path = STRBUF_INIT;
> -	struct strbuf sb = STRBUF_INIT;
>  	static struct worktree **worktrees;
>  	int i = 0;
>  
> @@ -345,6 +301,10 @@ const struct worktree *find_shared_symref(const char *symref,
>  
>  	for (i = 0; worktrees[i]; i++) {
>  		struct worktree *wt = worktrees[i];
> +		const char *symref_target;
> +		unsigned char sha1[20];
> +		int flags;
> +
>  		if (wt->is_bare)
>  			continue;
>  
> @@ -359,25 +319,14 @@ const struct worktree *find_shared_symref(const char *symref,
>  			}
>  		}
>  
> -		strbuf_reset(&path);
> -		strbuf_reset(&sb);
> -		strbuf_addf(&path, "%s/%s",
> -			    get_worktree_git_dir(wt),
> -			    symref);
> -
> -		if (parse_ref(path.buf, &sb, NULL)) {
> -			continue;
> -		}
> -
> -		if (!strcmp(sb.buf, target)) {
> +		symref_target = resolve_ref_submodule(wt->path, symref, 0,
> +						      sha1, &flags);
> +		if ((flags & REF_ISSYMREF) && !strcmp(symref_target, target)) {
>  			existing = wt;
>  			break;
>  		}
>  	}
>  
> -	strbuf_release(&path);
> -	strbuf_release(&sb);
> -
>  	return existing;
>  }
>  
> diff --git a/worktree.h b/worktree.h
> index 6bfb985203..5ea5e503fb 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -4,7 +4,7 @@
>  struct worktree {
>  	char *path;
>  	char *id;
> -	char *head_ref;
> +	char *head_ref;		/* NULL if HEAD is broken or detached */
>  	char *lock_reason;	/* internal use */
>  	unsigned char head_sha1[20];
>  	int is_detached;
>