[PATCH RFC 0/2] Kill manual ref parsing code in worktree.c

[PATCH RFC 0/2] Kill manual ref parsing code in worktree.c

[Nguyễn Thái Ngọc Duy]

A hundred years ago I added this code because a "standalone" ref
parsing code was not available from refs.c and the file was going through
some heavy changes that refactoring its ref parsing code out was not
an option. I promised to kill this parse_ref() eventually. I'm
fulfilling it today (or soon).

I would really like to you double check the approach I'm using here
(using submodule interface for accessing refs from another worktree)
since that may be the way forward to fix the "gc losing objects" in
multi worktrees. I've given it lots of thoughts in the last 24 hours.
Still can't find any fundamental flaw...

Nguyễn Thái Ngọc Duy (2):
  refs.c: add resolve_ref_submodule()
  worktree.c: use submodule interface to access refs from another worktree

 branch.c   |  3 +-
 refs.c     | 20 +++++++++----
 refs.h     |  3 ++
 worktree.c | 99 +++++++++++++++-----------------------------------------------
 worktree.h |  2 +-
 5 files changed, 44 insertions(+), 83 deletions(-)

-- 
2.11.0.157.gd943d85

[PATCH 1/2] refs.c: add resolve_ref_submodule()

[Nguyễn Thái Ngọc Duy]

This is basically the extended version of resolve_gitlink_ref() where we
have access to more info from the underlying resolve_ref_recursively() call.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 20 ++++++++++++++------
 refs.h |  3 +++
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index cd36b64ed9..02e35d83f3 100644
--- a/refs.c
+++ b/refs.c
@@ -1325,18 +1325,18 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 				       resolve_flags, sha1, flags);
 }
 
-int resolve_gitlink_ref(const char *submodule, const char *refname,
-			unsigned char *sha1)
+const char *resolve_ref_submodule(const char *submodule, const char *refname,
+				  int resolve_flags, unsigned char *sha1,
+				  int *flags)
 {
 	size_t len = strlen(submodule);
 	struct ref_store *refs;
-	int flags;
 
 	while (len && submodule[len - 1] == '/')
 		len--;
 
 	if (!len)
-		return -1;
+		return NULL;
 
 	if (submodule[len]) {
 		/* We need to strip off one or more trailing slashes */
@@ -1349,9 +1349,17 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
 	}
 
 	if (!refs)
-		return -1;
+		return NULL;
+
+	return resolve_ref_recursively(refs, refname, resolve_flags, sha1, flags);
+}
+
+int resolve_gitlink_ref(const char *submodule, const char *refname,
+			unsigned char *sha1)
+{
+	int flags;
 
-	if (!resolve_ref_recursively(refs, refname, 0, sha1, &flags) ||
+	if (!resolve_ref_submodule(submodule, refname, 0, sha1, &flags) ||
 	    is_null_sha1(sha1))
 		return -1;
 	return 0;
diff --git a/refs.h b/refs.h
index 9fbff90e79..74542468d8 100644
--- a/refs.h
+++ b/refs.h
@@ -88,6 +88,9 @@ int peel_ref(const char *refname, unsigned char *sha1);
  */
 int resolve_gitlink_ref(const char *submodule, const char *refname,
 			unsigned char *sha1);
+const char *resolve_ref_submodule(const char *submodule, const char *refname,
+				  int resolve_flags, unsigned char *sha1,
+				  int *flags);
 
 /*
  * Return true iff abbrev_name is a possible abbreviation for
-- 
2.11.0.157.gd943d85

Re: [PATCH 1/2] refs.c: add resolve_ref_submodule()

[Michael Haggerty]

On 02/08/2017 12:31 PM, Nguyễn Thái Ngọc Duy wrote:
> This is basically the extended version of resolve_gitlink_ref() where we
> have access to more info from the underlying resolve_ref_recursively() call.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  refs.c | 20 ++++++++++++++------
>  refs.h |  3 +++
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index cd36b64ed9..02e35d83f3 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1325,18 +1325,18 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
>  				       resolve_flags, sha1, flags);
>  }
>  
> -int resolve_gitlink_ref(const char *submodule, const char *refname,
> -			unsigned char *sha1)
> +const char *resolve_ref_submodule(const char *submodule, const char *refname,
> +				  int resolve_flags, unsigned char *sha1,
> +				  int *flags)
>  {
>  	size_t len = strlen(submodule);
>  	struct ref_store *refs;
> -	int flags;
>  
>  	while (len && submodule[len - 1] == '/')
>  		len--;
>  
>  	if (!len)
> -		return -1;
> +		return NULL;
>  
>  	if (submodule[len]) {
>  		/* We need to strip off one or more trailing slashes */
> @@ -1349,9 +1349,17 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
>  	}
>  
>  	if (!refs)
> -		return -1;
> +		return NULL;
> +
> +	return resolve_ref_recursively(refs, refname, resolve_flags, sha1, flags);
> +}
> +
> +int resolve_gitlink_ref(const char *submodule, const char *refname,
> +			unsigned char *sha1)
> +{
> +	int flags;
>  
> -	if (!resolve_ref_recursively(refs, refname, 0, sha1, &flags) ||
> +	if (!resolve_ref_submodule(submodule, refname, 0, sha1, &flags) ||
>  	    is_null_sha1(sha1))
>  		return -1;
>  	return 0;
> diff --git a/refs.h b/refs.h
> index 9fbff90e79..74542468d8 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -88,6 +88,9 @@ int peel_ref(const char *refname, unsigned char *sha1);
>   */
>  int resolve_gitlink_ref(const char *submodule, const char *refname,
>  			unsigned char *sha1);
> +const char *resolve_ref_submodule(const char *submodule, const char *refname,
> +				  int resolve_flags, unsigned char *sha1,
> +				  int *flags);

This function is the analog of resolve_ref_unsafe(); i.e., it returns a
pointer to either a static buffer or a pointer into the refname
argument. Therefore, I think it should have "unsafe" in its name. And/or
maybe there should be a safe version of the function analogous to
resolve_refdup().

Moreover, this function has inherited the code for stripping trailing
slashes from the submodule name. I have the feeling that this is a wart,
not a feature, and that it would be sad to see it spread. How about
moving the slash-stripping code to resolve_gitlink_ref() and making
resolve_ref_submodule() assume that its submodule name is already clean?

It would be nice to have a docstring here.

I also have some higher-level concerns about the approach of this patch
series, which I'll write about in a comment to patch 2/2.

Michael

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

[Nguyễn Thái Ngọc Duy]

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/

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;
-- 
2.11.0.157.gd943d85

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

[Stefan Beller]

On Wed, Feb 8, 2017 at 3:31 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> (**) At this point, we may want to rename refs *_submodule API to
> something more neutral, maybe s/_submodule/_remote/

I agree on (**), except that I am not sure if /_remote/ is a better name,
because there is already a concept of a "remote" as well as
"remote-tracking" in Git. (Usually it is not reachable on the same
FS, but resides on another machine).

So if we were to do s/_submodule/_remote/, we'd have e.g.

    for_each_ref_remote

which could mean that we do networking things to obtain the
actual remote refs or just talk about remote tracking refs.

My gut reaction would be to s/submodule/alternative/ here,
but we also have a thing called alternates already.

So we're looking for a name that describes refs for both
worktrees as well as submodules. (Not sure if we can generalize
to also include alternates, too)

And the one thing they share is that they have their refs
"not at the usual place", e.g. not at .git/refs but rather at
.git/{modules,worktrees}.

  Recently we had a tangential discussion in submodule land
  about the different places, when adding the
  "git submodule absorbgitdirs" command, that moves
  the submodule/.git directory into .git/modules/<name>.
  We chose "absorb" here as the name, because it
  was moved into the .git/ area.

So maybe one of:

    s/submodule_ref/unusual_ref/
    (to emphasize it is not a regular ref inside the repo, so:)
    s/submodule_ref/irregular_ref/

    s/resolve_ref_submodule/resolve_ref_out_of_place/
    s/resolve_ref_submodule/resolve_ref_gitlink_pointed/
    s/resolve_ref_submodule/resolve_ref_linked_pointer/

would be my current thinking.

Thanks,
Stefan

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Wed, Feb 8, 2017 at 3:31 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> (**) At this point, we may want to rename refs *_submodule API to
>> something more neutral, maybe s/_submodule/_remote/
>
> I agree on (**), except that I am not sure if /_remote/ is a better name,
> because there is already a concept of a "remote" as well as
> "remote-tracking" in Git. (Usually it is not reachable on the same
> FS, but resides on another machine).

I agree with you that the concept of remote is quite detached from
the concept of wt and submodule whose refs need to be peeked at from
the local repository.  After all, "remote" tracking branches are
part of local repository's refs.

> My gut reaction would be to s/submodule/alternative/ here,
> but we also have a thing called alternates already.

... and I tend to think that is far closer a concept.  You borrow
objects from your alternate object store, and that alternate object
store may have its own set of refs you would need to peek when you
are computing reachability from refs.

Also don't we already enumerate such refs that pin objects in the
alternate object store when doing object transfer negotiation in
order to send ".have" entries for their tips?  What API do we use to
do that, I wonder.

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> As such, we can just linked worktree's path as a submodule. We just need

Lack of verb made me read this three times.  "We can just treat
linked worktree's path as if it were a submodule"?

I agree with you that the "submodule" in the name is misleading.  We
would want to be able to walk refs from other local repositories
(and repository-like entities, like the .git/ thing in a liked
worktree), like our own submodule, or the repository we borrow
objects from via the alternate mechanism.

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

[Michael Haggerty]

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;
> 

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> 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` 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).

I am not quite sure what mental model you are suggesting as a
preferred solution.  We can

 - represent a set of refs stored for a particular worktree
   (i.e. HEAD, refs/bisect, and refs/worktree/<name>, iirc), as
   bunch of ref_stores;

 - represent a set of refs shared across a set of worktrees that
   share the primary one, as another ref_store;

 - a caller who wants to get a "logical" view of a single worktree
   user can pick one of the first kind and union that with the
   second one, and represent the result as a (synthetic) ref_store.

The third one is "stitching together from multiple sources".  By
"mixing logical and physical is a bad idea", do you mean that the
same abstraction "ref_store" should not be used for the first two
(which are physical) and the third one (which is logical)?  Do you
want to call the first two "physical_ref_store"and the last one
"ref_store" and keep them distinct?

For the purpose of anchoring objects in the object store shared by
multiple worktrees, we can either iterate over all the ref_stores
of the first two kind, or iterate over all the ref_stores of the
third kind for all worktrees.  The latter of course is less
efficient as the enumeration

	for worktree in all worktrees:
		for ref in get_ref_store(worktree)
			mark tip of ref reachable

will work on all the shared refs multiple times, but as an
abstraction that may be simpler.  The alternative of working at the
physical level would be more efficient

	for worktree in all worktrees:
		for ref in get_ref_store_specific_to_worktree(worktree):
	        	mark tip of ref reachable
	for ref in get_ref_store_shared_across_worktrees():
        	mark tip of ref reachable

but this consumer now _knows_ how the logical ref_store of a
worktree is constructed (i.e. by combining the two ref_stores),
which appears as a layering violation.

I am however not sure if these issues are what you are driving at,
and what exact design you are suggesting.

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

[Michael Haggerty]

On 02/09/2017 07:55 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 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` 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).
> 
> I am not quite sure what mental model you are suggesting as a
> preferred solution.  We can
> 
>  - represent a set of refs stored for a particular worktree
>    (i.e. HEAD, refs/bisect, and refs/worktree/<name>, iirc), as
>    bunch of ref_stores;
> 
>  - represent a set of refs shared across a set of worktrees that
>    share the primary one, as another ref_store;
> 
>  - a caller who wants to get a "logical" view of a single worktree
>    user can pick one of the first kind and union that with the
>    second one, and represent the result as a (synthetic) ref_store.
> 
> The third one is "stitching together from multiple sources".  By
> "mixing logical and physical is a bad idea", do you mean that the
> same abstraction "ref_store" should not be used for the first two
> (which are physical) and the third one (which is logical)?  Do you
> want to call the first two "physical_ref_store"and the last one
> "ref_store" and keep them distinct?

The existing `ref_store` abstraction, I think, is capable of
representing either kind of reference store. The stitching together to
get the "logical" view of a worktree should probably happen within the
refs code rather than forcing callers to deal with it. But yes, I think
that code should put together a compound `ref_store` object that
delegates to multiple underlying `ref_store` objects as you've described.

Which kind of `ref_store *` you have in your hand would depend on where
you got it. If you call the hypothetical `get_submodule_refs()`
function, you would get a `ref_store *` representing the references that
are logically visible from that submodule. There might be a separate
`get_worktree_specific_refs()` that returns a `ref_store *` representing
the worktree-specific references physically stored for the worktree. But
maybe the latter is not even necessary; see below.

> For the purpose of anchoring objects in the object store shared by
> multiple worktrees, we can either iterate over all the ref_stores
> of the first two kind, or iterate over all the ref_stores of the
> third kind for all worktrees.  The latter of course is less
> efficient as the enumeration
> 
> 	for worktree in all worktrees:
> 		for ref in get_ref_store(worktree)
> 			mark tip of ref reachable
> 
> will work on all the shared refs multiple times, but as an
> abstraction that may be simpler.  The alternative of working at the
> physical level would be more efficient
> 
> 	for worktree in all worktrees:
> 		for ref in get_ref_store_specific_to_worktree(worktree):
> 	        	mark tip of ref reachable
> 	for ref in get_ref_store_shared_across_worktrees():
>         	mark tip of ref reachable
> 
> but this consumer now _knows_ how the logical ref_store of a
> worktree is constructed (i.e. by combining the two ref_stores),
> which appears as a layering violation.
> 
> I am however not sure if these issues are what you are driving at,
> and what exact design you are suggesting.

Reachability is a special case, because it needs all of the references
that refer to a particular object store, even though the reference names
might overlap. I personally think that reachability roots should be
requested via a new refs API call separate from `for_each_rawref()` (or
whatever is used now). Internally it would be implemented much like your
second "efficient" algorithm, but the implementation would be within the
refs code, and the caller could remain ignorant of those details.

Externally, it might not even want to pass the caller the real reference
names (I assume that callers mainly only use the reference names for
diagnostic messages). For example, it might want to report references
`HEAD` and `refs/bisect/bad` in worktree `foo` under the pseudonyms
`worktree/foo/HEAD` and `worktree/foo/refs/bisect/bad`, so that they can
be distinguished from any homonyms in the main repo and in other
worktrees. If you ask for the reachability roots while in a worktree, it
would either automatically crawl up to the main repository and across to
sibling worktrees to get the full set of reachability roots, or maybe it
would refuse to run at all (if we want to require such commands to be
executed from the main repo).

I don't know exactly who would be the consumers of the reachability
roots, so maybe there are some problems with this suggestion.

Michael

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

[Duy Nguyen]

On Thu, Feb 9, 2017 at 1:07 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 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.

Yes. But in this context it was more of a guinea pig. I wanted
something simple enough to code up show we can see what the approach
looked like. Good thing I did it.

>
> 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`.

Yep.

> 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).
>
> ...
>
> 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.

Oh I was going to reply to Stefan about his comment to my (**)
footnote. Something along the this line

"Ideally we would introduce a new set of api, maybe with refs_ prefix,
that takes a refs_store. Then submodule people can get a ref store
somewhere and pass to it. Worktree people get maybe some other refs
store for it. The "old" api like for_each_ref() is a thin wrapper
around it, just like read_cache() vs read_index(&the_index). If the
*_submodule does not see much use, we might as well kill it and use
the generic refs_*".

If I didn't misunderstood anything else, then I think we're on the same page.

Now I need to see if I can get there in a reasonable time frame (so I
can fix my "gc in worktree" problem properly) or I would need
something temporary but not so hacky. I'll try to make this new api
and see how it works out. If you think I should not do it right away,
for whatever reason, stop me now.
-- 
Duy

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

[Michael Haggerty]

On 02/09/2017 12:59 PM, Duy Nguyen wrote:
> On Thu, Feb 9, 2017 at 1:07 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> 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.
> 
> Yes. But in this context it was more of a guinea pig. I wanted
> something simple enough to code up show we can see what the approach
> looked like. Good thing I did it.
> 
>>
>> 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`.
> 
> Yep.
> 
>> 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).
>>
>> ...
>>
>> 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.
> 
> Oh I was going to reply to Stefan about his comment to my (**)
> footnote. Something along the this line
> 
> "Ideally we would introduce a new set of api, maybe with refs_ prefix,
> that takes a refs_store. Then submodule people can get a ref store
> somewhere and pass to it. Worktree people get maybe some other refs
> store for it. The "old" api like for_each_ref() is a thin wrapper
> around it, just like read_cache() vs read_index(&the_index). If the
> *_submodule does not see much use, we might as well kill it and use
> the generic refs_*".
> 
> If I didn't misunderstood anything else, then I think we're on the same page.
> 
> Now I need to see if I can get there in a reasonable time frame (so I
> can fix my "gc in worktree" problem properly) or I would need
> something temporary but not so hacky. I'll try to make this new api
> and see how it works out. If you think I should not do it right away,
> for whatever reason, stop me now.

Sounds good. A couple of hints and points to remember:

* I think `struct ref_store *` should remain opaque outside of the refs
  code.

* The virtual function interface embodied in `struct ref_storage_be`
  isn't meant to be an external interface (i.e., don't just expose it
  and have external callers use it directly).

* One important distinction between the main reference store and
  submodule reference stores is that we know the object store for
  the former but not the latter. That implies that some operations
  are, or should be, impossible for submodules (e.g., anything that
  involves looking up objects, including peeling refs). However, we
  *do* know the object store for linked worktrees. So they don't have
  to be as dumbed-down as submodule ref stores. (This might be
  irrelevant if you don't need any additional features for your
  current purposes.)

Also, I just sent my submodule-hash patch series to the mailing list in
case you want to build on that.

Michael

Re: [PATCH RFC 0/2] Kill manual ref parsing code in worktree.c

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> A hundred years ago I added this code because a "standalone" ref
> parsing code was not available from refs.c and the file was going through
> some heavy changes that refactoring its ref parsing code out was not
> an option. I promised to kill this parse_ref() eventually. I'm
> fulfilling it today (or soon).

;-)  

Thanks.  And thanks for looping Michael in.  I'd appreciate his
input in this area.

> I would really like to you double check the approach I'm using here
> (using submodule interface for accessing refs from another worktree)
> since that may be the way forward to fix the "gc losing objects" in
> multi worktrees. I've given it lots of thoughts in the last 24 hours.
> Still can't find any fundamental flaw...

I see that you posted a separate message outlining the idea
yesterday and I didn't see any response (and I was sick and lacked
energy to think it through); I think the basic approach to use "an
API to bring set of refs hidden from your normal view" is sensible.
Except for the unfortunate naming of the interface that makes it
sound as if it is only to access submodules, but that is where the
feature original came from, so let's not complain too loudly ;-)

> Nguyễn Thái Ngọc Duy (2):
>   refs.c: add resolve_ref_submodule()
>   worktree.c: use submodule interface to access refs from another worktree
>
>  branch.c   |  3 +-
>  refs.c     | 20 +++++++++----
>  refs.h     |  3 ++
>  worktree.c | 99 +++++++++++++++-----------------------------------------------
>  worktree.h |  2 +-
>  5 files changed, 44 insertions(+), 83 deletions(-)

[PATCH v2 0/5] Kill manual ref parsing code in worktree.c

[Nguyễn Thái Ngọc Duy]

v2 still kills parse_ref(), but the series now depends on my other
series [1] and as a result does not make any confusing "submodule"
calls any more.

It also kills another function, introduced for multi-worktree, that
should not have been there to begin with. Good riddance.

Again, the naming convention with refs_ prefix for new APIs may not be
the best idea...

[1] public-inbox.org/git/20170213152011.12050-1-pclouds@gmail.com

Nguyễn Thái Ngọc Duy (5):
  refs: introduce get_worktree_ref_store()
  refs.c: add refs_resolve_ref_unsafe()
  worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
  refs: add refs_create_symref()
  refs: kill set_worktree_head_symref()

 branch.c             |  15 ++++----
 refs.c               |  58 ++++++++++++++++++++++++-----
 refs.h               |  21 ++++++-----
 refs/files-backend.c |  43 +---------------------
 refs/refs-internal.h |   5 ---
 worktree.c           | 102 ++++++++++++++-------------------------------------
 worktree.h           |   2 +-
 7 files changed, 98 insertions(+), 148 deletions(-)

-- 
2.11.0.157.gd943d85

[PATCH v2 1/5] refs: introduce get_worktree_ref_store()

[Nguyễn Thái Ngọc Duy]

files-backend at this point is still aware of the per-repo/worktree
separation in refs, so it can handle a linked worktree.

Note: accessing a worktree of a submodule remains unaddressed. Perhaps
after get_worktrees() can access submodule (or rather a new function
get_submodule_worktrees(), that lists worktrees of a submodule), we can
update this function to work with submodules as well.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 27 +++++++++++++++++++++++++++
 refs.h |  2 ++
 2 files changed, 29 insertions(+)

diff --git a/refs.c b/refs.c
index e7206a420..ba4d9420c 100644
--- a/refs.c
+++ b/refs.c
@@ -10,6 +10,7 @@
 #include "object.h"
 #include "tag.h"
 #include "submodule.h"
+#include "worktree.h"
 
 /*
  * List of all available backends
@@ -1486,6 +1487,32 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 	return refs;
 }
 
+struct ref_store *get_worktree_ref_store(const struct worktree *wt)
+{
+	struct ref_store *refs;
+
+	if (wt->is_current)
+		return get_main_ref_store();
+
+	/*
+	 * We share the same hash map with submodules for
+	 * now. submodule paths are always relative (to topdir) while
+	 * worktree paths are always absolute. No chance of conflict.
+	 */
+	refs = lookup_submodule_ref_store(wt->path);
+	if (refs)
+		return refs;
+
+	if (wt->id)
+		refs = ref_store_init(git_common_path("worktrees/%s", wt->id));
+	else
+		refs = ref_store_init(get_git_common_dir());
+
+	if (refs)
+		register_submodule_ref_store(refs, wt->path);
+	return refs;
+}
+
 void base_ref_store_init(struct ref_store *refs,
 			 const struct ref_storage_be *be)
 {
diff --git a/refs.h b/refs.h
index 1287ba59c..464cc384a 100644
--- a/refs.h
+++ b/refs.h
@@ -565,5 +565,7 @@ struct ref_store *get_main_ref_store(void);
  * submodule==NULL.
  */
 struct ref_store *get_submodule_ref_store(const char *submodule);
+struct worktree;
+struct ref_store *get_worktree_ref_store(const struct worktree *wt);
 
 #endif /* REFS_H */
-- 
2.11.0.157.gd943d85

[PATCH v2 2/5] refs.c: add refs_resolve_ref_unsafe()

[Nguyễn Thái Ngọc Duy]

Surprise surprise. This is just rename and introduce
resolve_ref_recursively() to the public.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c               | 9 +++++----
 refs.h               | 6 ++++++
 refs/files-backend.c | 2 +-
 refs/refs-internal.h | 5 -----
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index ba4d9420c..e0e191107 100644
--- a/refs.c
+++ b/refs.c
@@ -1226,7 +1226,7 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 }
 
 /* This function needs to return a meaningful errno on failure */
-const char *resolve_ref_recursively(struct ref_store *refs,
+const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 				    const char *refname,
 				    int resolve_flags,
 				    unsigned char *sha1, int *flags)
@@ -1313,8 +1313,9 @@ int refs_init_db(struct strbuf *err)
 const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 			       unsigned char *sha1, int *flags)
 {
-	return resolve_ref_recursively(get_main_ref_store(), refname,
-				       resolve_flags, sha1, flags);
+	return refs_resolve_ref_unsafe(get_main_ref_store(),
+				       refname, resolve_flags,
+				       sha1, flags);
 }
 
 int resolve_gitlink_ref(const char *submodule, const char *refname,
@@ -1343,7 +1344,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
 	if (!refs)
 		return -1;
 
-	if (!resolve_ref_recursively(refs, refname, 0, sha1, &flags) ||
+	if (!refs_resolve_ref_unsafe(refs, refname, 0, sha1, &flags) ||
 	    is_null_sha1(sha1))
 		return -1;
 	return 0;
diff --git a/refs.h b/refs.h
index 464cc384a..10c2cfc00 100644
--- a/refs.h
+++ b/refs.h
@@ -568,4 +568,10 @@ struct ref_store *get_submodule_ref_store(const char *submodule);
 struct worktree;
 struct ref_store *get_worktree_ref_store(const struct worktree *wt);
 
+const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+				    const char *refname,
+				    int resolve_flags,
+				    unsigned char *sha1,
+				    int *flags);
+
 #endif /* REFS_H */
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 685ea5c14..f3be620ab 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1276,7 +1276,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 					 create_dir_entry(refs, refname.buf,
 							  refname.len, 1));
 		} else {
-			if (!resolve_ref_recursively(&refs->base,
+			if (!refs_resolve_ref_unsafe(&refs->base,
 						     refname.buf,
 						     RESOLVE_REF_READING,
 						     sha1, &flag)) {
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index cb6882779..6b29dc3b1 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -634,11 +634,6 @@ struct ref_store {
 void base_ref_store_init(struct ref_store *refs,
 			 const struct ref_storage_be *be);
 
-const char *resolve_ref_recursively(struct ref_store *refs,
-				    const char *refname,
-				    int resolve_flags,
-				    unsigned char *sha1, int *flags);
-
 static inline int is_per_worktree_ref(const char *refname)
 {
 	return !starts_with(refname, "refs/") ||
-- 
2.11.0.157.gd943d85

[PATCH v2 3/5] worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()

[Nguyễn Thái Ngọc Duy]

The manual parsing code is replaced with a call to refs_resolve_ref_unsafe().
The manual parsing code must die because only refs/files-backend.c
should do that.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 branch.c   |   3 +-
 worktree.c | 102 ++++++++++++++++---------------------------------------------
 worktree.h |   2 +-
 3 files changed, 30 insertions(+), 77 deletions(-)

diff --git a/branch.c b/branch.c
index c431cbf6a..fcb4a765c 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 eb6121263..f8f520036 100644
--- a/worktree.c
+++ b/worktree.c
@@ -19,54 +19,25 @@ 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 = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
+					 "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 +48,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 +60,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 +72,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 +91,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 +293,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 +302,11 @@ 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];
+		struct ref_store *refs;
+		int flags;
+
 		if (wt->is_bare)
 			continue;
 
@@ -359,24 +321,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)) {
+		refs = get_worktree_ref_store(wt);
+		symref_target = refs_resolve_ref_unsafe(refs, 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 d59ce1fee..961252e71 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;
-- 
2.11.0.157.gd943d85

[PATCH v2 4/5] refs: add refs_create_symref()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 22 +++++++++++++++++-----
 refs.h |  4 ++++
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index e0e191107..23e0a8eda 100644
--- a/refs.c
+++ b/refs.c
@@ -1535,13 +1535,25 @@ int peel_ref(const char *refname, unsigned char *sha1)
 	return refs->be->peel_ref(refs, refname, sha1);
 }
 
-int create_symref(const char *ref_target, const char *refs_heads_master,
+int refs_create_symref(struct ref_store *refs,
+		       const char *ref_target,
+		       const char *refs_heads_master,
+		       const char *logmsg)
+{
+	return refs->be->create_symref(refs,
+				       ref_target,
+				       refs_heads_master,
+				       logmsg);
+}
+
+int create_symref(const char *ref_target,
+		  const char *refs_heads_master,
 		  const char *logmsg)
 {
-	struct ref_store *refs = get_main_ref_store();
-
-	return refs->be->create_symref(refs, ref_target, refs_heads_master,
-				       logmsg);
+	return refs_create_symref(get_main_ref_store(),
+				  ref_target,
+				  refs_heads_master,
+				  logmsg);
 }
 
 int ref_transaction_commit(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 10c2cfc00..694769963 100644
--- a/refs.h
+++ b/refs.h
@@ -573,5 +573,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 				    int resolve_flags,
 				    unsigned char *sha1,
 				    int *flags);
+int refs_create_symref(struct ref_store *refs,
+		       const char *refname,
+		       const char *target,
+		       const char *logmsg);
 
 #endif /* REFS_H */
-- 
2.11.0.157.gd943d85

[PATCH v2 5/5] refs: kill set_worktree_head_symref()

[Nguyễn Thái Ngọc Duy]

70999e9cec (branch -m: update all per-worktree HEADs - 2016-03-27)
added this function in order to update HEADs of all relevant
worktrees, when a branch is renamed.

It, as a public ref api, kind of breaks abstraction when it uses
internal functions of files backend. With the introduction of
refs_create_symref(), we can move back pretty close to the code before
70999e9cec, where create_symref() was used for updating HEAD.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 branch.c             | 12 ++++++------
 refs.h               |  9 ---------
 refs/files-backend.c | 41 -----------------------------------------
 3 files changed, 6 insertions(+), 56 deletions(-)

diff --git a/branch.c b/branch.c
index fcb4a765c..ad0cc0489 100644
--- a/branch.c
+++ b/branch.c
@@ -352,18 +352,18 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref)
 	int i;
 
 	for (i = 0; worktrees[i]; i++) {
+		struct ref_store *refs;
+
 		if (worktrees[i]->is_detached)
 			continue;
 		if (worktrees[i]->head_ref &&
 		    strcmp(oldref, worktrees[i]->head_ref))
 			continue;
 
-		if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
-					     newref)) {
-			ret = -1;
-			error(_("HEAD of working tree %s is not updated"),
-			      worktrees[i]->path);
-		}
+		refs = get_worktree_ref_store(worktrees[i]);
+		if (refs_create_symref(refs, "HEAD", newref, NULL))
+			ret = error(_("HEAD of working tree %s is not updated"),
+				    worktrees[i]->path);
 	}
 
 	free_worktrees(worktrees);
diff --git a/refs.h b/refs.h
index 694769963..bce77891a 100644
--- a/refs.h
+++ b/refs.h
@@ -325,15 +325,6 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg);
 
 int create_symref(const char *refname, const char *target, const char *logmsg);
 
-/*
- * Update HEAD of the specified gitdir.
- * Similar to create_symref("relative-git-dir/HEAD", target, NULL), but
- * this can update the main working tree's HEAD regardless of where
- * $GIT_DIR points to.
- * Return 0 if successful, non-zero otherwise.
- * */
-int set_worktree_head_symref(const char *gitdir, const char *target);
-
 enum action_on_err {
 	UPDATE_REFS_MSG_ON_ERR,
 	UPDATE_REFS_DIE_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f3be620ab..ba56e46d4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3108,47 +3108,6 @@ static int files_create_symref(struct ref_store *ref_store,
 	return ret;
 }
 
-int set_worktree_head_symref(const char *gitdir, const char *target)
-{
-	/*
-	 * FIXME: this obviously will not work well for future refs
-	 * backends. This function needs to die.
-	 */
-	struct files_ref_store *refs =
-		files_downcast(get_main_ref_store(), "set_head_symref");
-	static struct lock_file head_lock;
-	struct ref_lock *lock;
-	struct strbuf head_path = STRBUF_INIT;
-	const char *head_rel;
-	int ret;
-
-	strbuf_addf(&head_path, "%s/HEAD", absolute_path(gitdir));
-	if (hold_lock_file_for_update(&head_lock, head_path.buf,
-				      LOCK_NO_DEREF) < 0) {
-		struct strbuf err = STRBUF_INIT;
-		unable_to_lock_message(head_path.buf, errno, &err);
-		error("%s", err.buf);
-		strbuf_release(&err);
-		strbuf_release(&head_path);
-		return -1;
-	}
-
-	/* head_rel will be "HEAD" for the main tree, "worktrees/wt/HEAD" for
-	   linked trees */
-	head_rel = remove_leading_path(head_path.buf,
-				       absolute_path(get_git_common_dir()));
-	/* to make use of create_symref_locked(), initialize ref_lock */
-	lock = xcalloc(1, sizeof(struct ref_lock));
-	lock->lk = &head_lock;
-	lock->ref_name = xstrdup(head_rel);
-
-	ret = create_symref_locked(refs, lock, head_rel, target, NULL);
-
-	unlock_ref(lock); /* will free lock */
-	strbuf_release(&head_path);
-	return ret;
-}
-
 static int files_reflog_exists(struct ref_store *ref_store,
 			       const char *refname)
 {
-- 
2.11.0.157.gd943d85

[PATCH v3 0/4] Kill manual ref parsing code in worktree.c

[Nguyễn Thái Ngọc Duy]

v3 is a rebased version on latest nd/files-backend-git-dir [1]. Since
that series added a bunch of new refs_* functions, v2's 02/05 and
04/05 are removed. The new 01/04 could be an indepedent fix, but at
test-ref-store.c requires it, so I put it here. More tests are added
now that we have test-ref-store.c (yay!)

[1] http://public-inbox.org/git/%3C20170318020337.22767-1-pclouds@gmail.com%3E/

Nguyễn Thái Ngọc Duy (4):
  environment.c: fix potential segfault by get_git_common_dir()
  refs: introduce get_worktree_ref_store()
  worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
  refs: kill set_worktree_head_symref()

 branch.c                               |  15 ++---
 environment.c                          |   2 +
 refs.c                                 |  32 +++++++++++
 refs.h                                 |  12 +---
 refs/files-backend.c                   |  44 --------------
 t/helper/test-ref-store.c              |  19 ++++++
 t/t1407-worktree-ref-store.sh (new +x) |  52 +++++++++++++++++
 worktree.c                             | 102 +++++++++------------------------
 worktree.h                             |   2 +-
 9 files changed, 143 insertions(+), 137 deletions(-)
 create mode 100755 t/t1407-worktree-ref-store.sh

-- 
2.11.0.157.gd943d85

[PATCH v3 1/4] environment.c: fix potential segfault by get_git_common_dir()

[Nguyễn Thái Ngọc Duy]

setup_git_env() must be called before this function to initialize
git_common_dir so that it returns a non NULL string. And it must return
a non NULL string or segfault can happen because all callers expect so.

Normally if somebody has called get_git_dir(), or set_git_dir() then
setup_git_env() is already called. But if you do setup_git_directory()
at top dir (which skips set_git_dir) and never call get_git_dir, you'll
get NULL here.

test-ref-store.c will hit this problem because it's very lightweight,
just enough initialization to exercise refs code, and get_git_dir() will
never be called until get_worktrees() is, which uses get_git_common_dir().
---
 environment.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/environment.c b/environment.c
index 42dc3106d2..2986ee7200 100644
--- a/environment.c
+++ b/environment.c
@@ -214,6 +214,8 @@ const char *get_git_dir(void)
 
 const char *get_git_common_dir(void)
 {
+	if (!git_dir)
+		setup_git_env();
 	return git_common_dir;
 }
 
-- 
2.11.0.157.gd943d85

Re: [PATCH v3 1/4] environment.c: fix potential segfault by get_git_common_dir()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> setup_git_env() must be called before this function to initialize
> git_common_dir so that it returns a non NULL string. And it must return
> a non NULL string or segfault can happen because all callers expect so.
>
> Normally if somebody has called get_git_dir(), or set_git_dir() then
> setup_git_env() is already called. But if you do setup_git_directory()
> at top dir (which skips set_git_dir) and never call get_git_dir, you'll
> get NULL here.

Hmph, and the solution for the problem not being "so let's make sure
get_git_dir() is called even when the command is started at the top
directory" is because...?

> test-ref-store.c will hit this problem because it's very lightweight,
> just enough initialization to exercise refs code, and get_git_dir() will
> never be called until get_worktrees() is, which uses get_git_common_dir().
> ---

Missing sign-off.



>  environment.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/environment.c b/environment.c
> index 42dc3106d2..2986ee7200 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -214,6 +214,8 @@ const char *get_git_dir(void)
>  
>  const char *get_git_common_dir(void)
>  {
> +	if (!git_dir)
> +		setup_git_env();
>  	return git_common_dir;
>  }

Re: [PATCH v3 1/4] environment.c: fix potential segfault by get_git_common_dir()

[Duy Nguyen]

On Sun, Mar 19, 2017 at 12:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> setup_git_env() must be called before this function to initialize
>> git_common_dir so that it returns a non NULL string. And it must return
>> a non NULL string or segfault can happen because all callers expect so.
>>
>> Normally if somebody has called get_git_dir(), or set_git_dir() then
>> setup_git_env() is already called. But if you do setup_git_directory()
>> at top dir (which skips set_git_dir) and never call get_git_dir, you'll
>> get NULL here.
>
> Hmph, and the solution for the problem not being "so let's make sure
> get_git_dir() is called even when the command is started at the top
> directory" is because...?

-EHARDTOPARSE. There's a hidden dependency between get_git_dir() and
get_git_common_dir() which is not good. If we lazily call
set_git_env(), make sure we do it lazily but consistently at all
relevant function calls (i.e. including get_git_common_dir).

Alternatively (I was thinking of this but didn't really follow up
because this was side issue) we should make sure setup_git_env() is
always called at the end of setup_git_dir...() and remove the laziness
in get_git_dir(). This may be more in line of recent attempts to catch
repo access without calling setup_git_directory..() first. But sadly I
haven't read Jeff's series, so I can't say whether it's true.
-- 
Duy

[PATCH v3 2/4] refs: introduce get_worktree_ref_store()

[Nguyễn Thái Ngọc Duy]

files-backend at this point is still aware of the per-repo/worktree
separation in refs, so it can handle a linked worktree.

Some refs operations are known not working when current files-backend is
used in a linked worktree (e.g. reflog). Tests will be written when
refs_* functions start to be called with worktree backend to verify that
they work as expected.

Note: accessing a worktree of a submodule remains unaddressed. Perhaps
after get_worktrees() can access submodule (or rather a new function
get_submodule_worktrees(), that lists worktrees of a submodule), we can
update this function to work with submodules as well.
---
 refs.c | 32 ++++++++++++++++++++++++++++++++
 refs.h |  2 ++
 2 files changed, 34 insertions(+)

diff --git a/refs.c b/refs.c
index 77a39f8b17..6695140cfe 100644
--- a/refs.c
+++ b/refs.c
@@ -10,6 +10,7 @@
 #include "object.h"
 #include "tag.h"
 #include "submodule.h"
+#include "worktree.h"
 
 /*
  * List of all available backends
@@ -1593,6 +1594,37 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 	return refs;
 }
 
+struct ref_store *get_worktree_ref_store(const struct worktree *wt)
+{
+	struct ref_store *refs;
+	unsigned int refs_all_capabilities =
+		REF_STORE_READ | REF_STORE_WRITE |
+		REF_STORE_ODB | REF_STORE_MAIN;
+
+	if (wt->is_current)
+		return get_main_ref_store();
+
+	/*
+	 * We share the same hash map with submodules for
+	 * now. submodule paths are always relative (to topdir) while
+	 * worktree paths are always absolute. No chance of conflict.
+	 */
+	refs = lookup_submodule_ref_store(wt->path);
+	if (refs)
+		return refs;
+
+	if (wt->id)
+		refs = ref_store_init(git_common_path("worktrees/%s", wt->id),
+				      refs_all_capabilities);
+	else
+		refs = ref_store_init(get_git_common_dir(),
+				      refs_all_capabilities);
+
+	if (refs)
+		register_submodule_ref_store(refs, wt->path);
+	return refs;
+}
+
 void base_ref_store_init(struct ref_store *refs,
 			 const struct ref_storage_be *be)
 {
diff --git a/refs.h b/refs.h
index 49e97d7d5f..6df69a2adb 100644
--- a/refs.h
+++ b/refs.h
@@ -5,6 +5,7 @@ struct object_id;
 struct ref_store;
 struct strbuf;
 struct string_list;
+struct worktree;
 
 /*
  * Resolve a reference, recursively following symbolic refererences.
@@ -655,5 +656,6 @@ struct ref_store *get_main_ref_store(void);
  * submodule==NULL.
  */
 struct ref_store *get_submodule_ref_store(const char *submodule);
+struct ref_store *get_worktree_ref_store(const struct worktree *wt);
 
 #endif /* REFS_H */
-- 
2.11.0.157.gd943d85

Re: [PATCH v3 2/4] refs: introduce get_worktree_ref_store()

[Michael Haggerty]

On 03/18/2017 11:02 AM, Nguyễn Thái Ngọc Duy wrote:
> files-backend at this point is still aware of the per-repo/worktree
> separation in refs, so it can handle a linked worktree.
> 
> Some refs operations are known not working when current files-backend is
> used in a linked worktree (e.g. reflog). Tests will be written when
> refs_* functions start to be called with worktree backend to verify that
> they work as expected.
> 
> Note: accessing a worktree of a submodule remains unaddressed. Perhaps
> after get_worktrees() can access submodule (or rather a new function
> get_submodule_worktrees(), that lists worktrees of a submodule), we can
> update this function to work with submodules as well.
> ---
>  refs.c | 32 ++++++++++++++++++++++++++++++++
>  refs.h |  2 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/refs.c b/refs.c
> index 77a39f8b17..6695140cfe 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -10,6 +10,7 @@
>  #include "object.h"
>  #include "tag.h"
>  #include "submodule.h"
> +#include "worktree.h"
>  
>  /*
>   * List of all available backends
> @@ -1593,6 +1594,37 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>  	return refs;
>  }
>  
> +struct ref_store *get_worktree_ref_store(const struct worktree *wt)
> +{
> +	struct ref_store *refs;
> +	unsigned int refs_all_capabilities =
> +		REF_STORE_READ | REF_STORE_WRITE |
> +		REF_STORE_ODB | REF_STORE_MAIN;
> +
> +	if (wt->is_current)
> +		return get_main_ref_store();
> +
> +	/*
> +	 * We share the same hash map with submodules for
> +	 * now. submodule paths are always relative (to topdir) while
> +	 * worktree paths are always absolute. No chance of conflict.
> +	 */
> +	refs = lookup_submodule_ref_store(wt->path);
> +	if (refs)
> +		return refs;
> +
> +	if (wt->id)
> +		refs = ref_store_init(git_common_path("worktrees/%s", wt->id),
> +				      refs_all_capabilities);
> +	else
> +		refs = ref_store_init(get_git_common_dir(),
> +				      refs_all_capabilities);
> +
> +	if (refs)
> +		register_submodule_ref_store(refs, wt->path);
> +	return refs;
> +}
> +

I guess I can hold my nose and accept storing worktree and submodule
`ref_store`s together in a single hashmap, but not if the code continues
to use "submodule" in the names, because that will be a continuing
source of confusion to readers. For example,

* lookup_submodule_ref_store()
* register_submodule_ref_store()
* submodule_hash_entry
* submodule_hash_cmp()
* alloc_submodule_hash_entry()
* submodule_ref_stores

Any docstrings would also have to be updated.

As a new naming scheme, maybe use s/submodule/other/, for example
`lookup_other_ref_store()` etc.

> [...]

Michael

Re: [PATCH v3 2/4] refs: introduce get_worktree_ref_store()

[Duy Nguyen]

On Mon, Mar 20, 2017 at 1:59 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> I guess I can hold my nose and accept storing worktree and submodule
> `ref_store`s together in a single hashmap

Release your nose, I'll add a new hashmap :) But while we're here,
what are your thoughts about moving submodule-specific to submodule.c?
That includes the hashmap, get_submodule_ref_store() and all other
submodule stuff, to submodule.c where submodule-specific stuff stays.
The same for worktree stuff, to worktree.c. That keeps refs.c to core
refs business. And the hashmap can be used for more than just refs.
For example, submodule has configs, and worktree also has bunch of
other stuff that I would like to just cache and not readdir() and
parse every time.
-- 
Duy

Re: [PATCH v3 2/4] refs: introduce get_worktree_ref_store()

[Michael Haggerty]

On 03/20/2017 01:01 PM, Duy Nguyen wrote:
> On Mon, Mar 20, 2017 at 1:59 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> I guess I can hold my nose and accept storing worktree and submodule
>> `ref_store`s together in a single hashmap
> 
> Release your nose, I'll add a new hashmap :)

Thanks :-)

> But while we're here,
> what are your thoughts about moving submodule-specific to submodule.c?
> That includes the hashmap, get_submodule_ref_store() and all other
> submodule stuff, to submodule.c where submodule-specific stuff stays.
> The same for worktree stuff, to worktree.c. That keeps refs.c to core
> refs business. And the hashmap can be used for more than just refs.
> For example, submodule has configs, and worktree also has bunch of
> other stuff that I would like to just cache and not readdir() and
> parse every time.

I can't think of any objections, though there might be some devils in
the details. I assume you mean that the submodule-specific hashmap
wouldn't be `{name -> struct ref_store}` anymore, but rather `{name ->
struct submodule}`, and `struct submodule` would hold the associated
`ref_store`? That sounds very reasonable.

Instead of moving all of the `for_each_*_submodule()` functions over, I
encourage you to consider getting rid of them entirely and let the
end-users call the `refs_for_each_*()` versions of the functions. Again,
I'm not sure that there won't be friction in doing so, but it seems like
it's worth a try.

Michael

Re: [PATCH v3 2/4] refs: introduce get_worktree_ref_store()

[Duy Nguyen]

On Mon, Mar 20, 2017 at 9:25 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Instead of moving all of the `for_each_*_submodule()` functions over, I
> encourage you to consider getting rid of them entirely and let the
> end-users call the `refs_for_each_*()` versions of the functions. Again,
> I'm not sure that there won't be friction in doing so, but it seems like
> it's worth a try.

They are getting rid of. If you look at pu (or nd/prune-in-worktree
actually) there's only head_ref_submodule() and
for_each_remote_ref_submodule() left. head_ref_submodule() has no
caller but is still there, I'll need to kill it. Killing the latter
can be done separately since the callers in submodule.c need some
update.
-- 
Duy

[PATCH v3 3/4] worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()

[Nguyễn Thái Ngọc Duy]

The manual parsing code is replaced with a call to refs_resolve_ref_unsafe().
The manual parsing code must die because only refs/files-backend.c
should do that.
---
 branch.c                               |   3 +-
 t/helper/test-ref-store.c              |  19 ++++++
 t/t1407-worktree-ref-store.sh (new +x) |  40 +++++++++++++
 worktree.c                             | 102 +++++++++------------------------
 worktree.h                             |   2 +-
 5 files changed, 89 insertions(+), 77 deletions(-)
 create mode 100755 t/t1407-worktree-ref-store.sh

diff --git a/branch.c b/branch.c
index 5c12036b02..0b949b7fb2 100644
--- a/branch.c
+++ b/branch.c
@@ -355,7 +355,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/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 57849ee59a..3c926ed85e 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "refs.h"
+#include "worktree.h"
 
 static const char *notnull(const char *arg, const char *name)
 {
@@ -32,6 +33,23 @@ static const char **get_store(const char **argv, struct ref_store **refs)
 		strbuf_release(&sb);
 
 		*refs = get_submodule_ref_store(gitdir);
+	} else if (skip_prefix(argv[0], "worktree:", &gitdir)) {
+		struct worktree **p, **worktrees = get_worktrees(0);
+
+		for (p = worktrees; *p; p++) {
+			struct worktree *wt = *p;
+
+			if (!wt->id) {
+				/* special case for main worktree */
+				if (!strcmp(gitdir, "main"))
+					break;
+			} else if (!strcmp(gitdir, wt->id))
+				break;
+		}
+		if (!*p)
+			die("no such worktree: %s", gitdir);
+
+		*refs = get_worktree_ref_store(*p);
 	} else
 		die("unknown backend %s", argv[0]);
 
@@ -261,6 +279,7 @@ int cmd_main(int argc, const char **argv)
 	const char *func;
 	struct command *cmd;
 
+	setup_git_directory();
 	argv = get_store(argv + 1, &refs);
 
 	func = *argv++;
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
new file mode 100755
index 0000000000..04d1e9d177
--- /dev/null
+++ b/t/t1407-worktree-ref-store.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='test worktree ref store api'
+
+. ./test-lib.sh
+
+RWT="test-ref-store worktree:wt"
+RMAIN="test-ref-store worktree:main"
+
+test_expect_success 'setup' '
+	test_commit first &&
+	git worktree add -b wt-master wt &&
+	(
+		cd wt &&
+		test_commit second
+	)
+'
+
+test_expect_success 'resolve_ref(<shared-ref>)' '
+	SHA1=`git rev-parse master` &&
+	echo "$SHA1 refs/heads/master 0x0" >expected &&
+	$RWT resolve-ref refs/heads/master 0 >actual &&
+	test_cmp expected actual &&
+	$RMAIN resolve-ref refs/heads/master 0 >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'resolve_ref(<per-worktree-ref>)' '
+	SHA1=`git -C wt rev-parse HEAD` &&
+	echo "$SHA1 refs/heads/wt-master 0x1" >expected &&
+	$RWT resolve-ref HEAD 0 >actual &&
+	test_cmp expected actual &&
+
+	SHA1=`git rev-parse HEAD` &&
+	echo "$SHA1 refs/heads/master 0x1" >expected &&
+	$RMAIN resolve-ref HEAD 0 >actual &&
+	test_cmp expected actual
+'
+
+test_done
diff --git a/worktree.c b/worktree.c
index fa7bc67a50..c1ec334b06 100644
--- a/worktree.c
+++ b/worktree.c
@@ -19,54 +19,25 @@ 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 = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
+					 "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 +48,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 +60,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 +72,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 +91,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 +293,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 +302,11 @@ 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];
+		struct ref_store *refs;
+		int flags;
+
 		if (wt->is_bare)
 			continue;
 
@@ -359,25 +321,15 @@ 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)) {
+		refs = get_worktree_ref_store(wt);
+		symref_target = refs_resolve_ref_unsafe(refs, 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;
-- 
2.11.0.157.gd943d85

[PATCH v3 4/4] refs: kill set_worktree_head_symref()

[Nguyễn Thái Ngọc Duy]

70999e9cec (branch -m: update all per-worktree HEADs - 2016-03-27)
added this function in order to update HEADs of all relevant
worktrees, when a branch is renamed.

It, as a public ref api, kind of breaks abstraction when it uses
internal functions of files backend. With the introduction of
refs_create_symref(), we can move back pretty close to the code before
70999e9cec, where create_symref() was used for updating HEAD.
---
 branch.c                      | 12 ++++++------
 refs.h                        | 10 ----------
 refs/files-backend.c          | 44 -------------------------------------------
 t/t1407-worktree-ref-store.sh | 12 ++++++++++++
 4 files changed, 18 insertions(+), 60 deletions(-)

diff --git a/branch.c b/branch.c
index 0b949b7fb2..69d5eea84b 100644
--- a/branch.c
+++ b/branch.c
@@ -353,18 +353,18 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref,
 	int i;
 
 	for (i = 0; worktrees[i]; i++) {
+		struct ref_store *refs;
+
 		if (worktrees[i]->is_detached)
 			continue;
 		if (worktrees[i]->head_ref &&
 		    strcmp(oldref, worktrees[i]->head_ref))
 			continue;
 
-		if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
-					     newref, logmsg)) {
-			ret = -1;
-			error(_("HEAD of working tree %s is not updated"),
-			      worktrees[i]->path);
-		}
+		refs = get_worktree_ref_store(worktrees[i]);
+		if (refs_create_symref(refs, "HEAD", newref, logmsg))
+			ret = error(_("HEAD of working tree %s is not updated"),
+				    worktrees[i]->path);
 	}
 
 	free_worktrees(worktrees);
diff --git a/refs.h b/refs.h
index 6df69a2adb..447381d378 100644
--- a/refs.h
+++ b/refs.h
@@ -402,16 +402,6 @@ int refs_create_symref(struct ref_store *refs, const char *refname,
 		       const char *target, const char *logmsg);
 int create_symref(const char *refname, const char *target, const char *logmsg);
 
-/*
- * Update HEAD of the specified gitdir.
- * Similar to create_symref("relative-git-dir/HEAD", target, NULL), but
- * this can update the main working tree's HEAD regardless of where
- * $GIT_DIR points to.
- * Return 0 if successful, non-zero otherwise.
- * */
-int set_worktree_head_symref(const char *gitdir, const char *target,
-			     const char *logmsg);
-
 enum action_on_err {
 	UPDATE_REFS_MSG_ON_ERR,
 	UPDATE_REFS_DIE_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4242486118..62d8e0713a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3163,50 +3163,6 @@ static int files_create_symref(struct ref_store *ref_store,
 	return ret;
 }
 
-int set_worktree_head_symref(const char *gitdir, const char *target, const char *logmsg)
-{
-	/*
-	 * FIXME: this obviously will not work well for future refs
-	 * backends. This function needs to die.
-	 */
-	struct files_ref_store *refs =
-		files_downcast(get_main_ref_store(),
-			       REF_STORE_WRITE,
-			       "set_head_symref");
-
-	static struct lock_file head_lock;
-	struct ref_lock *lock;
-	struct strbuf head_path = STRBUF_INIT;
-	const char *head_rel;
-	int ret;
-
-	strbuf_addf(&head_path, "%s/HEAD", absolute_path(gitdir));
-	if (hold_lock_file_for_update(&head_lock, head_path.buf,
-				      LOCK_NO_DEREF) < 0) {
-		struct strbuf err = STRBUF_INIT;
-		unable_to_lock_message(head_path.buf, errno, &err);
-		error("%s", err.buf);
-		strbuf_release(&err);
-		strbuf_release(&head_path);
-		return -1;
-	}
-
-	/* head_rel will be "HEAD" for the main tree, "worktrees/wt/HEAD" for
-	   linked trees */
-	head_rel = remove_leading_path(head_path.buf,
-				       absolute_path(get_git_common_dir()));
-	/* to make use of create_symref_locked(), initialize ref_lock */
-	lock = xcalloc(1, sizeof(struct ref_lock));
-	lock->lk = &head_lock;
-	lock->ref_name = xstrdup(head_rel);
-
-	ret = create_symref_locked(refs, lock, head_rel, target, logmsg);
-
-	unlock_ref(lock); /* will free lock */
-	strbuf_release(&head_path);
-	return ret;
-}
-
 static int files_reflog_exists(struct ref_store *ref_store,
 			       const char *refname)
 {
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
index 04d1e9d177..5df06f3556 100755
--- a/t/t1407-worktree-ref-store.sh
+++ b/t/t1407-worktree-ref-store.sh
@@ -37,4 +37,16 @@ test_expect_success 'resolve_ref(<per-worktree-ref>)' '
 	test_cmp expected actual
 '
 
+test_expect_success 'create_symref(FOO, refs/heads/master)' '
+	$RWT create-symref FOO refs/heads/master nothing &&
+	echo refs/heads/master >expected &&
+	git -C wt symbolic-ref FOO >actual &&
+	test_cmp expected actual &&
+
+	$RMAIN create-symref FOO refs/heads/wt-master nothing &&
+	echo refs/heads/wt-master >expected &&
+	git symbolic-ref FOO >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.11.0.157.gd943d85

Re: [PATCH v3 0/4] Kill manual ref parsing code in worktree.c

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> v3 is a rebased version on latest nd/files-backend-git-dir [1]. Since
> that series added a bunch of new refs_* functions, v2's 02/05 and
> 04/05 are removed. The new 01/04 could be an indepedent fix, but at
> test-ref-store.c requires it, so I put it here. More tests are added
> now that we have test-ref-store.c (yay!)

I think the corresponding ones were queued on a separate topic
nd/worktree-kill-parse-ref that is based on nd/files-backend-git-dir
but I do not mind making these 4 patches just part of the latter
topic.

Will send comments on individual patches separately.

Thanks.  

>
> [1] http://public-inbox.org/git/%3C20170318020337.22767-1-pclouds@gmail.com%3E/
>
> Nguyễn Thái Ngọc Duy (4):
>   environment.c: fix potential segfault by get_git_common_dir()
>   refs: introduce get_worktree_ref_store()
>   worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
>   refs: kill set_worktree_head_symref()
>
>  branch.c                               |  15 ++---
>  environment.c                          |   2 +
>  refs.c                                 |  32 +++++++++++
>  refs.h                                 |  12 +---
>  refs/files-backend.c                   |  44 --------------
>  t/helper/test-ref-store.c              |  19 ++++++
>  t/t1407-worktree-ref-store.sh (new +x) |  52 +++++++++++++++++
>  worktree.c                             | 102 +++++++++------------------------
>  worktree.h                             |   2 +-
>  9 files changed, 143 insertions(+), 137 deletions(-)
>  create mode 100755 t/t1407-worktree-ref-store.sh

[PATCH v4 0/5] Kill manual ref parsing code in worktree.c

[Nguyễn Thái Ngọc Duy]

v4 adds a new patch, 2/5, which makes the hashmap related functions in
refs.c generic, so I could add a new map for worktree ref stores and
not abuse submodule hashmap.

I mentioned about moving these hashmap back to submodule.c and
worktree.c where it can map more than just ref stores (in worktree
case, we can cache 'struct worktree', for example). But I'm not doing
it now to keep things small.

The commit message in 1/5 is rephrased a bit, hopefully clearer.

Nguyễn Thái Ngọc Duy (5):
  environment.c: fix potential segfault by get_git_common_dir()
  refs.c: make submodule ref store hashmap generic
  refs: introduce get_worktree_ref_store()
  worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
  refs: kill set_worktree_head_symref()

 branch.c                               |  15 ++---
 environment.c                          |   2 +
 refs.c                                 |  95 +++++++++++++++++++-----------
 refs.h                                 |  12 +---
 refs/files-backend.c                   |  44 --------------
 t/helper/test-ref-store.c              |  18 ++++++
 t/t1407-worktree-ref-store.sh (new +x) |  52 +++++++++++++++++
 worktree.c                             | 102 +++++++++------------------------
 worktree.h                             |   2 +-
 9 files changed, 173 insertions(+), 169 deletions(-)
 create mode 100755 t/t1407-worktree-ref-store.sh

-- 
2.11.0.157.gd943d85

[PATCH v4 1/5] environment.c: fix potential segfault by get_git_common_dir()

[Nguyễn Thái Ngọc Duy]

setup_git_env() must be called before this function to initialize
git_common_dir so that it returns a non NULL string. And it must return
a non NULL string or segfault can happen because all callers expect so.

It does not do so explicitly though and depends on get_git_dir() being
called first (which will guarantee setup_git_env()). Avoid this
dependency and call setup_git_env() by itself.

test-ref-store.c will hit this problem because it's very lightweight,
just enough initialization to exercise refs code, and get_git_dir() will
never be called until get_worktrees() is, which uses get_git_common_dir
and hits a segfault.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 environment.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/environment.c b/environment.c
index 42dc3106d2..2986ee7200 100644
--- a/environment.c
+++ b/environment.c
@@ -214,6 +214,8 @@ const char *get_git_dir(void)
 
 const char *get_git_common_dir(void)
 {
+	if (!git_dir)
+		setup_git_env();
 	return git_common_dir;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v4 2/5] refs.c: make submodule ref store hashmap generic

[Nguyễn Thái Ngọc Duy]

This removes the "submodule" from submodule_hash_entry and other
function names. The goal is to reuse the same code and data structure
for other ref store types. The first one is worktree ref stores.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 64 ++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/refs.c b/refs.c
index ec1f563824..875e30a0b8 100644
--- a/refs.c
+++ b/refs.c
@@ -1450,32 +1450,32 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
 	return 0;
 }
 
-struct submodule_hash_entry
+struct ref_store_hash_entry
 {
 	struct hashmap_entry ent; /* must be the first member! */
 
 	struct ref_store *refs;
 
-	/* NUL-terminated name of submodule: */
-	char submodule[FLEX_ARRAY];
+	/* NUL-terminated identifier of the ref store: */
+	char name[FLEX_ARRAY];
 };
 
-static int submodule_hash_cmp(const void *entry, const void *entry_or_key,
+static int ref_store_hash_cmp(const void *entry, const void *entry_or_key,
 			      const void *keydata)
 {
-	const struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key;
-	const char *submodule = keydata ? keydata : e2->submodule;
+	const struct ref_store_hash_entry *e1 = entry, *e2 = entry_or_key;
+	const char *name = keydata ? keydata : e2->name;
 
-	return strcmp(e1->submodule, submodule);
+	return strcmp(e1->name, name);
 }
 
-static struct submodule_hash_entry *alloc_submodule_hash_entry(
-		const char *submodule, struct ref_store *refs)
+static struct ref_store_hash_entry *alloc_ref_store_hash_entry(
+		const char *name, struct ref_store *refs)
 {
-	struct submodule_hash_entry *entry;
+	struct ref_store_hash_entry *entry;
 
-	FLEX_ALLOC_STR(entry, submodule, submodule);
-	hashmap_entry_init(entry, strhash(submodule));
+	FLEX_ALLOC_STR(entry, name, name);
+	hashmap_entry_init(entry, strhash(name));
 	entry->refs = refs;
 	return entry;
 }
@@ -1487,19 +1487,19 @@ static struct ref_store *main_ref_store;
 static struct hashmap submodule_ref_stores;
 
 /*
- * Return the ref_store instance for the specified submodule. If that
- * ref_store hasn't been initialized yet, return NULL.
+ * Look up a ref store by name. If that ref_store hasn't been
+ * registered yet, return NULL.
  */
-static struct ref_store *lookup_submodule_ref_store(const char *submodule)
+static struct ref_store *lookup_ref_store_map(struct hashmap *map,
+					      const char *name)
 {
-	struct submodule_hash_entry *entry;
+	struct ref_store_hash_entry *entry;
 
-	if (!submodule_ref_stores.tablesize)
+	if (!map->tablesize)
 		/* It's initialized on demand in register_ref_store(). */
 		return NULL;
 
-	entry = hashmap_get_from_hash(&submodule_ref_stores,
-				      strhash(submodule), submodule);
+	entry = hashmap_get_from_hash(map, strhash(name), name);
 	return entry ? entry->refs : NULL;
 }
 
@@ -1535,20 +1535,19 @@ struct ref_store *get_main_ref_store(void)
 }
 
 /*
- * Register the specified ref_store to be the one that should be used
- * for submodule. It is a fatal error to call this function twice for
- * the same submodule.
+ * Associate a ref store with a name. It is a fatal error to call this
+ * function twice for the same name.
  */
-static void register_submodule_ref_store(struct ref_store *refs,
-					 const char *submodule)
+static void register_ref_store_map(struct hashmap *map,
+				   const char *type,
+				   struct ref_store *refs,
+				   const char *name)
 {
-	if (!submodule_ref_stores.tablesize)
-		hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 0);
+	if (!map->tablesize)
+		hashmap_init(map, ref_store_hash_cmp, 0);
 
-	if (hashmap_put(&submodule_ref_stores,
-			alloc_submodule_hash_entry(submodule, refs)))
-		die("BUG: ref_store for submodule '%s' initialized twice",
-		    submodule);
+	if (hashmap_put(map, alloc_ref_store_hash_entry(name, refs)))
+		die("BUG: %s ref_store '%s' initialized twice", type, name);
 }
 
 struct ref_store *get_submodule_ref_store(const char *submodule)
@@ -1565,7 +1564,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 		return get_main_ref_store();
 	}
 
-	refs = lookup_submodule_ref_store(submodule);
+	refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
 	if (refs)
 		return refs;
 
@@ -1584,7 +1583,8 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 	/* pretend that add_submodule_odb() has been called */
 	refs = ref_store_init(submodule_sb.buf,
 			      REF_STORE_READ | REF_STORE_ODB);
-	register_submodule_ref_store(refs, submodule);
+	register_ref_store_map(&submodule_ref_stores, "submodule",
+			       refs, submodule);
 
 	strbuf_release(&submodule_sb);
 	return refs;
-- 
2.11.0.157.gd943d85

[PATCH v4 3/5] refs: introduce get_worktree_ref_store()

[Nguyễn Thái Ngọc Duy]

files-backend at this point is still aware of the per-repo/worktree
separation in refs, so it can handle a linked worktree.

Some refs operations are known not working when current files-backend is
used in a linked worktree (e.g. reflog). Tests will be written when
refs_* functions start to be called with worktree backend to verify that
they work as expected.

Note: accessing a worktree of a submodule remains unaddressed. Perhaps
after get_worktrees() can access submodule (or rather a new function
get_submodule_worktrees(), that lists worktrees of a submodule), we can
update this function to work with submodules as well.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 31 +++++++++++++++++++++++++++++++
 refs.h |  2 ++
 2 files changed, 33 insertions(+)

diff --git a/refs.c b/refs.c
index 875e30a0b8..a4083caf6a 100644
--- a/refs.c
+++ b/refs.c
@@ -10,6 +10,7 @@
 #include "object.h"
 #include "tag.h"
 #include "submodule.h"
+#include "worktree.h"
 
 /*
  * List of all available backends
@@ -1486,6 +1487,9 @@ static struct ref_store *main_ref_store;
 /* A hashmap of ref_stores, stored by submodule name: */
 static struct hashmap submodule_ref_stores;
 
+/* A hashmap of ref_stores, stored by worktree id: */
+static struct hashmap worktree_ref_stores;
+
 /*
  * Look up a ref store by name. If that ref_store hasn't been
  * registered yet, return NULL.
@@ -1590,6 +1594,33 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 	return refs;
 }
 
+struct ref_store *get_worktree_ref_store(const struct worktree *wt)
+{
+	struct ref_store *refs;
+	unsigned int refs_all_capabilities =
+		REF_STORE_READ | REF_STORE_WRITE |
+		REF_STORE_ODB | REF_STORE_MAIN;
+
+	if (wt->is_current)
+		return get_main_ref_store();
+
+	refs = lookup_ref_store_map(&worktree_ref_stores, wt->id);
+	if (refs)
+		return refs;
+
+	if (wt->id)
+		refs = ref_store_init(git_common_path("worktrees/%s", wt->id),
+				      refs_all_capabilities);
+	else
+		refs = ref_store_init(get_git_common_dir(),
+				      refs_all_capabilities);
+
+	if (refs)
+		register_ref_store_map(&worktree_ref_stores, "worktree",
+				       refs, wt->id);
+	return refs;
+}
+
 void base_ref_store_init(struct ref_store *refs,
 			 const struct ref_storage_be *be)
 {
diff --git a/refs.h b/refs.h
index 49e97d7d5f..6df69a2adb 100644
--- a/refs.h
+++ b/refs.h
@@ -5,6 +5,7 @@ struct object_id;
 struct ref_store;
 struct strbuf;
 struct string_list;
+struct worktree;
 
 /*
  * Resolve a reference, recursively following symbolic refererences.
@@ -655,5 +656,6 @@ struct ref_store *get_main_ref_store(void);
  * submodule==NULL.
  */
 struct ref_store *get_submodule_ref_store(const char *submodule);
+struct ref_store *get_worktree_ref_store(const struct worktree *wt);
 
 #endif /* REFS_H */
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 3/5] refs: introduce get_worktree_ref_store()

[Michael Haggerty]

On 04/04/2017 12:21 PM, Nguyễn Thái Ngọc Duy wrote:
> files-backend at this point is still aware of the per-repo/worktree
> separation in refs, so it can handle a linked worktree.
> 
> Some refs operations are known not working when current files-backend is
> used in a linked worktree (e.g. reflog). Tests will be written when
> refs_* functions start to be called with worktree backend to verify that
> they work as expected.
> 
> Note: accessing a worktree of a submodule remains unaddressed. Perhaps
> after get_worktrees() can access submodule (or rather a new function
> get_submodule_worktrees(), that lists worktrees of a submodule), we can
> update this function to work with submodules as well.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  refs.c | 31 +++++++++++++++++++++++++++++++
>  refs.h |  2 ++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/refs.c b/refs.c
> index 875e30a0b8..a4083caf6a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -10,6 +10,7 @@
>  #include "object.h"
>  #include "tag.h"
>  #include "submodule.h"
> +#include "worktree.h"
>  
>  /*
>   * List of all available backends
> @@ -1486,6 +1487,9 @@ static struct ref_store *main_ref_store;
>  /* A hashmap of ref_stores, stored by submodule name: */
>  static struct hashmap submodule_ref_stores;
>  
> +/* A hashmap of ref_stores, stored by worktree id: */
> +static struct hashmap worktree_ref_stores;
> +
>  /*
>   * Look up a ref store by name. If that ref_store hasn't been
>   * registered yet, return NULL.
> @@ -1590,6 +1594,33 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>  	return refs;
>  }
>  
> +struct ref_store *get_worktree_ref_store(const struct worktree *wt)
> +{
> +	struct ref_store *refs;
> +	unsigned int refs_all_capabilities =
> +		REF_STORE_READ | REF_STORE_WRITE |
> +		REF_STORE_ODB | REF_STORE_MAIN;

This constant appears another place, too. It might make sense to define
a constant `REF_STORE_ALL_CAPABILITIES` in `refs-internal.h` alongside
the individual bit values.

If you prefer not to, please at least declare this variable `const` to
spare the reader the trouble of looking to see whether it is modified
before it is used.

Otherwise, looks fine to me.

> [...]

Michael

[PATCH v4 4/5] worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()

[Nguyễn Thái Ngọc Duy]

The manual parsing code is replaced with a call to refs_resolve_ref_unsafe().
The manual parsing code must die because only refs/files-backend.c
should do that.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 branch.c                               |   3 +-
 t/helper/test-ref-store.c              |  18 ++++++
 t/t1407-worktree-ref-store.sh (new +x) |  40 +++++++++++++
 worktree.c                             | 102 +++++++++------------------------
 worktree.h                             |   2 +-
 5 files changed, 88 insertions(+), 77 deletions(-)
 create mode 100755 t/t1407-worktree-ref-store.sh

diff --git a/branch.c b/branch.c
index 5c12036b02..0b949b7fb2 100644
--- a/branch.c
+++ b/branch.c
@@ -355,7 +355,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/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 2d84c45ffe..4a487c014e 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "refs.h"
+#include "worktree.h"
 
 static const char *notnull(const char *arg, const char *name)
 {
@@ -32,6 +33,23 @@ static const char **get_store(const char **argv, struct ref_store **refs)
 		strbuf_release(&sb);
 
 		*refs = get_submodule_ref_store(gitdir);
+	} else if (skip_prefix(argv[0], "worktree:", &gitdir)) {
+		struct worktree **p, **worktrees = get_worktrees(0);
+
+		for (p = worktrees; *p; p++) {
+			struct worktree *wt = *p;
+
+			if (!wt->id) {
+				/* special case for main worktree */
+				if (!strcmp(gitdir, "main"))
+					break;
+			} else if (!strcmp(gitdir, wt->id))
+				break;
+		}
+		if (!*p)
+			die("no such worktree: %s", gitdir);
+
+		*refs = get_worktree_ref_store(*p);
 	} else
 		die("unknown backend %s", argv[0]);
 
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
new file mode 100755
index 0000000000..04d1e9d177
--- /dev/null
+++ b/t/t1407-worktree-ref-store.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='test worktree ref store api'
+
+. ./test-lib.sh
+
+RWT="test-ref-store worktree:wt"
+RMAIN="test-ref-store worktree:main"
+
+test_expect_success 'setup' '
+	test_commit first &&
+	git worktree add -b wt-master wt &&
+	(
+		cd wt &&
+		test_commit second
+	)
+'
+
+test_expect_success 'resolve_ref(<shared-ref>)' '
+	SHA1=`git rev-parse master` &&
+	echo "$SHA1 refs/heads/master 0x0" >expected &&
+	$RWT resolve-ref refs/heads/master 0 >actual &&
+	test_cmp expected actual &&
+	$RMAIN resolve-ref refs/heads/master 0 >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'resolve_ref(<per-worktree-ref>)' '
+	SHA1=`git -C wt rev-parse HEAD` &&
+	echo "$SHA1 refs/heads/wt-master 0x1" >expected &&
+	$RWT resolve-ref HEAD 0 >actual &&
+	test_cmp expected actual &&
+
+	SHA1=`git rev-parse HEAD` &&
+	echo "$SHA1 refs/heads/master 0x1" >expected &&
+	$RMAIN resolve-ref HEAD 0 >actual &&
+	test_cmp expected actual
+'
+
+test_done
diff --git a/worktree.c b/worktree.c
index fa7bc67a50..c1ec334b06 100644
--- a/worktree.c
+++ b/worktree.c
@@ -19,54 +19,25 @@ 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 = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
+					 "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 +48,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 +60,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 +72,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 +91,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 +293,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 +302,11 @@ 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];
+		struct ref_store *refs;
+		int flags;
+
 		if (wt->is_bare)
 			continue;
 
@@ -359,25 +321,15 @@ 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)) {
+		refs = get_worktree_ref_store(wt);
+		symref_target = refs_resolve_ref_unsafe(refs, 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;
-- 
2.11.0.157.gd943d85

[PATCH v4 5/5] refs: kill set_worktree_head_symref()

[Nguyễn Thái Ngọc Duy]

70999e9cec (branch -m: update all per-worktree HEADs - 2016-03-27)
added this function in order to update HEADs of all relevant
worktrees, when a branch is renamed.

It, as a public ref api, kind of breaks abstraction when it uses
internal functions of files backend. With the introduction of
refs_create_symref(), we can move back pretty close to the code before
70999e9cec, where create_symref() was used for updating HEAD.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 branch.c                      | 12 ++++++------
 refs.h                        | 10 ----------
 refs/files-backend.c          | 44 -------------------------------------------
 t/t1407-worktree-ref-store.sh | 12 ++++++++++++
 4 files changed, 18 insertions(+), 60 deletions(-)

diff --git a/branch.c b/branch.c
index 0b949b7fb2..69d5eea84b 100644
--- a/branch.c
+++ b/branch.c
@@ -353,18 +353,18 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref,
 	int i;
 
 	for (i = 0; worktrees[i]; i++) {
+		struct ref_store *refs;
+
 		if (worktrees[i]->is_detached)
 			continue;
 		if (worktrees[i]->head_ref &&
 		    strcmp(oldref, worktrees[i]->head_ref))
 			continue;
 
-		if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
-					     newref, logmsg)) {
-			ret = -1;
-			error(_("HEAD of working tree %s is not updated"),
-			      worktrees[i]->path);
-		}
+		refs = get_worktree_ref_store(worktrees[i]);
+		if (refs_create_symref(refs, "HEAD", newref, logmsg))
+			ret = error(_("HEAD of working tree %s is not updated"),
+				    worktrees[i]->path);
 	}
 
 	free_worktrees(worktrees);
diff --git a/refs.h b/refs.h
index 6df69a2adb..447381d378 100644
--- a/refs.h
+++ b/refs.h
@@ -402,16 +402,6 @@ int refs_create_symref(struct ref_store *refs, const char *refname,
 		       const char *target, const char *logmsg);
 int create_symref(const char *refname, const char *target, const char *logmsg);
 
-/*
- * Update HEAD of the specified gitdir.
- * Similar to create_symref("relative-git-dir/HEAD", target, NULL), but
- * this can update the main working tree's HEAD regardless of where
- * $GIT_DIR points to.
- * Return 0 if successful, non-zero otherwise.
- * */
-int set_worktree_head_symref(const char *gitdir, const char *target,
-			     const char *logmsg);
-
 enum action_on_err {
 	UPDATE_REFS_MSG_ON_ERR,
 	UPDATE_REFS_DIE_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a5b405436f..e428d1abff 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3161,50 +3161,6 @@ static int files_create_symref(struct ref_store *ref_store,
 	return ret;
 }
 
-int set_worktree_head_symref(const char *gitdir, const char *target, const char *logmsg)
-{
-	/*
-	 * FIXME: this obviously will not work well for future refs
-	 * backends. This function needs to die.
-	 */
-	struct files_ref_store *refs =
-		files_downcast(get_main_ref_store(),
-			       REF_STORE_WRITE,
-			       "set_head_symref");
-
-	static struct lock_file head_lock;
-	struct ref_lock *lock;
-	struct strbuf head_path = STRBUF_INIT;
-	const char *head_rel;
-	int ret;
-
-	strbuf_addf(&head_path, "%s/HEAD", absolute_path(gitdir));
-	if (hold_lock_file_for_update(&head_lock, head_path.buf,
-				      LOCK_NO_DEREF) < 0) {
-		struct strbuf err = STRBUF_INIT;
-		unable_to_lock_message(head_path.buf, errno, &err);
-		error("%s", err.buf);
-		strbuf_release(&err);
-		strbuf_release(&head_path);
-		return -1;
-	}
-
-	/* head_rel will be "HEAD" for the main tree, "worktrees/wt/HEAD" for
-	   linked trees */
-	head_rel = remove_leading_path(head_path.buf,
-				       absolute_path(get_git_common_dir()));
-	/* to make use of create_symref_locked(), initialize ref_lock */
-	lock = xcalloc(1, sizeof(struct ref_lock));
-	lock->lk = &head_lock;
-	lock->ref_name = xstrdup(head_rel);
-
-	ret = create_symref_locked(refs, lock, head_rel, target, logmsg);
-
-	unlock_ref(lock); /* will free lock */
-	strbuf_release(&head_path);
-	return ret;
-}
-
 static int files_reflog_exists(struct ref_store *ref_store,
 			       const char *refname)
 {
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
index 04d1e9d177..5df06f3556 100755
--- a/t/t1407-worktree-ref-store.sh
+++ b/t/t1407-worktree-ref-store.sh
@@ -37,4 +37,16 @@ test_expect_success 'resolve_ref(<per-worktree-ref>)' '
 	test_cmp expected actual
 '
 
+test_expect_success 'create_symref(FOO, refs/heads/master)' '
+	$RWT create-symref FOO refs/heads/master nothing &&
+	echo refs/heads/master >expected &&
+	git -C wt symbolic-ref FOO >actual &&
+	test_cmp expected actual &&
+
+	$RMAIN create-symref FOO refs/heads/wt-master nothing &&
+	echo refs/heads/wt-master >expected &&
+	git symbolic-ref FOO >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 0/5] Kill manual ref parsing code in worktree.c

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> v4 adds a new patch, 2/5, which makes the hashmap related functions in
> refs.c generic, so I could add a new map for worktree ref stores and
> not abuse submodule hashmap.
>
> I mentioned about moving these hashmap back to submodule.c and
> worktree.c where it can map more than just ref stores (in worktree
> case, we can cache 'struct worktree', for example). But I'm not doing
> it now to keep things small.
>
> The commit message in 1/5 is rephrased a bit, hopefully clearer.

Michael, does this look good to replace what has been queued?

I did not spot any discussion on this topic after v4 when I skimmed
the backlog messages, so I am hoping that I can pick this up without
having to worry about seeing another reroll immediately after that
(in which case I'd have been better off if I tended other topics in
flight in the meantime).

Thanks, both.

Re: [PATCH v4 0/5] Kill manual ref parsing code in worktree.c

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> v4 adds a new patch, 2/5, which makes the hashmap related functions in
>> refs.c generic, so I could add a new map for worktree ref stores and
>> not abuse submodule hashmap.
>>
>> I mentioned about moving these hashmap back to submodule.c and
>> worktree.c where it can map more than just ref stores (in worktree
>> case, we can cache 'struct worktree', for example). But I'm not doing
>> it now to keep things small.
>>
>> The commit message in 1/5 is rephrased a bit, hopefully clearer.
>
> Michael, does this look good to replace what has been queued?
>
> I did not spot any discussion on this topic after v4 when I skimmed
> the backlog messages, so I am hoping that I can pick this up without
> having to worry about seeing another reroll immediately after that
> (in which case I'd have been better off if I tended other topics in
> flight in the meantime).
>
> Thanks, both.

Oops, I shouldn't have done that.  When applied on top of the
files-backend thing (or have you updated that one and is my tree
lacking it???), this breaks quite a few tests.

t0001#41 dumps core from "git worktree add ../linked-worktree" for
example.

Re: [PATCH v4 0/5] Kill manual ref parsing code in worktree.c

[Duy Nguyen]

On Thu, Apr 13, 2017 at 07:02:22PM -0700, Junio C Hamano wrote:
> Oops, I shouldn't have done that.  When applied on top of the
> files-backend thing (or have you updated that one and is my tree
> lacking it???), this breaks quite a few tests.
> 
> t0001#41 dumps core from "git worktree add ../linked-worktree" for
> example.

This is embarassing. I worked on and off on this series over a long
period of time. I guess at the end I thought everything was ok "since
the last time" and just sent it away without realizing I hadn't
actually run the test suite, because it does fail here, now.

This fixup patch should make the test suite pass. I'm sorry for
wasting your time on this.

-- 8< --
From 09c6d432490b8e9de99eca09cdbdaebf53bbad2c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?=
 <pclouds@gmail.com>
Date: Fri, 14 Apr 2017 19:39:53 +0700
Subject: [PATCH] fixup! refs: introduce get_worktree_ref_store()

---
 refs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index a4083caf6a..05045a64e1 100644
--- a/refs.c
+++ b/refs.c
@@ -1600,11 +1600,13 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)
 	unsigned int refs_all_capabilities =
 		REF_STORE_READ | REF_STORE_WRITE |
 		REF_STORE_ODB | REF_STORE_MAIN;
+	const char *id;
 
 	if (wt->is_current)
 		return get_main_ref_store();
 
-	refs = lookup_ref_store_map(&worktree_ref_stores, wt->id);
+	id = wt->id ? wt->id : "/";
+	refs = lookup_ref_store_map(&worktree_ref_stores, id);
 	if (refs)
 		return refs;
 
@@ -1617,7 +1619,7 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)
 
 	if (refs)
 		register_ref_store_map(&worktree_ref_stores, "worktree",
-				       refs, wt->id);
+				       refs, id);
 	return refs;
 }
 
-- 
2.11.0.157.gd943d85

-- 8< --

Re: [PATCH v4 0/5] Kill manual ref parsing code in worktree.c

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> This is embarassing. I worked on and off on this series over a long
> period of time. I guess at the end I thought everything was ok "since
> the last time" and just sent it away without realizing I hadn't
> actually run the test suite, because it does fail here, now.
>
> This fixup patch should make the test suite pass. I'm sorry for
> wasting your time on this.

Thanks.  Will squash it in.

Re: [PATCH v4 0/5] Kill manual ref parsing code in worktree.c

[Michael Haggerty]

On 04/14/2017 03:40 AM, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> 
>> v4 adds a new patch, 2/5, which makes the hashmap related functions in
>> refs.c generic, so I could add a new map for worktree ref stores and
>> not abuse submodule hashmap.
>>
>> I mentioned about moving these hashmap back to submodule.c and
>> worktree.c where it can map more than just ref stores (in worktree
>> case, we can cache 'struct worktree', for example). But I'm not doing
>> it now to keep things small.
>>
>> The commit message in 1/5 is rephrased a bit, hopefully clearer.
> 
> Michael, does this look good to replace what has been queued?

I finally reviewed this patch series. The refs-related changes look
fine, and the submodule-related changes (which I'm not so familiar with)
looks plausible.

It's a nice cleanup :-)

Michael

[PATCH v5 0/6] Kill manual ref parsing code in worktree.c

[Nguyễn Thái Ngọc Duy]

v5 chanes are mostly cosmetic:

diff --git a/refs.c b/refs.c
index 5d31fb6bcf..7972720256 100644
--- a/refs.c
+++ b/refs.c
@@ -1530,11 +1530,7 @@ struct ref_store *get_main_ref_store(void)
 	if (main_ref_store)
 		return main_ref_store;
 
-	main_ref_store = ref_store_init(get_git_dir(),
-					(REF_STORE_READ |
-					 REF_STORE_WRITE |
-					 REF_STORE_ODB |
-					 REF_STORE_MAIN));
+	main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);
 	return main_ref_store;
 }
 
@@ -1597,9 +1593,6 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 struct ref_store *get_worktree_ref_store(const struct worktree *wt)
 {
 	struct ref_store *refs;
-	unsigned int refs_all_capabilities =
-		REF_STORE_READ | REF_STORE_WRITE |
-		REF_STORE_ODB | REF_STORE_MAIN;
 	const char *id;
 
 	if (wt->is_current)
@@ -1612,10 +1605,10 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)
 
 	if (wt->id)
 		refs = ref_store_init(git_common_path("worktrees/%s", wt->id),
-				      refs_all_capabilities);
+				      REF_STORE_ALL_CAPS);
 	else
 		refs = ref_store_init(get_git_common_dir(),
-				      refs_all_capabilities);
+				      REF_STORE_ALL_CAPS);
 
 	if (refs)
 		register_ref_store_map(&worktree_ref_stores, "worktree",
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 690498698e..b26f7e41ce 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -467,6 +467,10 @@ struct ref_store;
 #define REF_STORE_WRITE		(1 << 1) /* can perform update operations */
 #define REF_STORE_ODB		(1 << 2) /* has access to object database */
 #define REF_STORE_MAIN		(1 << 3)
+#define REF_STORE_ALL_CAPS	(REF_STORE_READ | \
+				 REF_STORE_WRITE | \
+				 REF_STORE_ODB | \
+				 REF_STORE_MAIN)
 
 /*
  * Initialize the ref_store for the specified gitdir. These functions

Nguyễn Thái Ngọc Duy (6):
  environment.c: fix potential segfault by get_git_common_dir()
  refs.c: make submodule ref store hashmap generic
  refs: add REFS_STORE_ALL_CAPS
  refs: introduce get_worktree_ref_store()
  worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
  refs: kill set_worktree_head_symref()

 branch.c                               |  15 ++---
 environment.c                          |   2 +
 refs.c                                 | 100 ++++++++++++++++++++------------
 refs.h                                 |  12 +---
 refs/files-backend.c                   |  44 --------------
 refs/refs-internal.h                   |   4 ++
 t/helper/test-ref-store.c              |  18 ++++++
 t/t1407-worktree-ref-store.sh (new +x) |  52 +++++++++++++++++
 worktree.c                             | 102 +++++++++------------------------
 worktree.h                             |   2 +-
 10 files changed, 177 insertions(+), 174 deletions(-)
 create mode 100755 t/t1407-worktree-ref-store.sh

-- 
2.11.0.157.gd943d85

[PATCH v5 1/6] environment.c: fix potential segfault by get_git_common_dir()

[Nguyễn Thái Ngọc Duy]

setup_git_env() must be called before this function to initialize
git_common_dir so that it returns a non NULL string. And it must return
a non NULL string or segfault can happen because all callers expect so.

It does not do so explicitly though and depends on get_git_dir() being
called first (which will guarantee setup_git_env()). Avoid this
dependency and call setup_git_env() by itself.

test-ref-store.c will hit this problem because it's very lightweight,
just enough initialization to exercise refs code, and get_git_dir() will
never be called until get_worktrees() is, which uses get_git_common_dir
and hits a segfault.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 environment.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/environment.c b/environment.c
index 42dc3106d2..2986ee7200 100644
--- a/environment.c
+++ b/environment.c
@@ -214,6 +214,8 @@ const char *get_git_dir(void)
 
 const char *get_git_common_dir(void)
 {
+	if (!git_dir)
+		setup_git_env();
 	return git_common_dir;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v5 2/6] refs.c: make submodule ref store hashmap generic

[Nguyễn Thái Ngọc Duy]

This removes the "submodule" from submodule_hash_entry and other
function names. The goal is to reuse the same code and data structure
for other ref store types. The first one is worktree ref stores.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs.c | 64 ++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/refs.c b/refs.c
index bad05ba861..96eba92475 100644
--- a/refs.c
+++ b/refs.c
@@ -1450,32 +1450,32 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
 	return 0;
 }
 
-struct submodule_hash_entry
+struct ref_store_hash_entry
 {
 	struct hashmap_entry ent; /* must be the first member! */
 
 	struct ref_store *refs;
 
-	/* NUL-terminated name of submodule: */
-	char submodule[FLEX_ARRAY];
+	/* NUL-terminated identifier of the ref store: */
+	char name[FLEX_ARRAY];
 };
 
-static int submodule_hash_cmp(const void *entry, const void *entry_or_key,
+static int ref_store_hash_cmp(const void *entry, const void *entry_or_key,
 			      const void *keydata)
 {
-	const struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key;
-	const char *submodule = keydata ? keydata : e2->submodule;
+	const struct ref_store_hash_entry *e1 = entry, *e2 = entry_or_key;
+	const char *name = keydata ? keydata : e2->name;
 
-	return strcmp(e1->submodule, submodule);
+	return strcmp(e1->name, name);
 }
 
-static struct submodule_hash_entry *alloc_submodule_hash_entry(
-		const char *submodule, struct ref_store *refs)
+static struct ref_store_hash_entry *alloc_ref_store_hash_entry(
+		const char *name, struct ref_store *refs)
 {
-	struct submodule_hash_entry *entry;
+	struct ref_store_hash_entry *entry;
 
-	FLEX_ALLOC_STR(entry, submodule, submodule);
-	hashmap_entry_init(entry, strhash(submodule));
+	FLEX_ALLOC_STR(entry, name, name);
+	hashmap_entry_init(entry, strhash(name));
 	entry->refs = refs;
 	return entry;
 }
@@ -1487,19 +1487,19 @@ static struct ref_store *main_ref_store;
 static struct hashmap submodule_ref_stores;
 
 /*
- * Return the ref_store instance for the specified submodule. If that
- * ref_store hasn't been initialized yet, return NULL.
+ * Look up a ref store by name. If that ref_store hasn't been
+ * registered yet, return NULL.
  */
-static struct ref_store *lookup_submodule_ref_store(const char *submodule)
+static struct ref_store *lookup_ref_store_map(struct hashmap *map,
+					      const char *name)
 {
-	struct submodule_hash_entry *entry;
+	struct ref_store_hash_entry *entry;
 
-	if (!submodule_ref_stores.tablesize)
+	if (!map->tablesize)
 		/* It's initialized on demand in register_ref_store(). */
 		return NULL;
 
-	entry = hashmap_get_from_hash(&submodule_ref_stores,
-				      strhash(submodule), submodule);
+	entry = hashmap_get_from_hash(map, strhash(name), name);
 	return entry ? entry->refs : NULL;
 }
 
@@ -1535,20 +1535,19 @@ struct ref_store *get_main_ref_store(void)
 }
 
 /*
- * Register the specified ref_store to be the one that should be used
- * for submodule. It is a fatal error to call this function twice for
- * the same submodule.
+ * Associate a ref store with a name. It is a fatal error to call this
+ * function twice for the same name.
  */
-static void register_submodule_ref_store(struct ref_store *refs,
-					 const char *submodule)
+static void register_ref_store_map(struct hashmap *map,
+				   const char *type,
+				   struct ref_store *refs,
+				   const char *name)
 {
-	if (!submodule_ref_stores.tablesize)
-		hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 0);
+	if (!map->tablesize)
+		hashmap_init(map, ref_store_hash_cmp, 0);
 
-	if (hashmap_put(&submodule_ref_stores,
-			alloc_submodule_hash_entry(submodule, refs)))
-		die("BUG: ref_store for submodule '%s' initialized twice",
-		    submodule);
+	if (hashmap_put(map, alloc_ref_store_hash_entry(name, refs)))
+		die("BUG: %s ref_store '%s' initialized twice", type, name);
 }
 
 struct ref_store *get_submodule_ref_store(const char *submodule)
@@ -1565,7 +1564,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 		return get_main_ref_store();
 	}
 
-	refs = lookup_submodule_ref_store(submodule);
+	refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
 	if (refs)
 		return refs;
 
@@ -1584,7 +1583,8 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 	/* assume that add_submodule_odb() has been called */
 	refs = ref_store_init(submodule_sb.buf,
 			      REF_STORE_READ | REF_STORE_ODB);
-	register_submodule_ref_store(refs, submodule);
+	register_ref_store_map(&submodule_ref_stores, "submodule",
+			       refs, submodule);
 
 	strbuf_release(&submodule_sb);
 	return refs;
-- 
2.11.0.157.gd943d85

[PATCH v5 3/6] refs: add REFS_STORE_ALL_CAPS

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c               | 6 +-----
 refs/refs-internal.h | 4 ++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 96eba92475..6e6f0ec5be 100644
--- a/refs.c
+++ b/refs.c
@@ -1526,11 +1526,7 @@ struct ref_store *get_main_ref_store(void)
 	if (main_ref_store)
 		return main_ref_store;
 
-	main_ref_store = ref_store_init(get_git_dir(),
-					(REF_STORE_READ |
-					 REF_STORE_WRITE |
-					 REF_STORE_ODB |
-					 REF_STORE_MAIN));
+	main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);
 	return main_ref_store;
 }
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 690498698e..b26f7e41ce 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -467,6 +467,10 @@ struct ref_store;
 #define REF_STORE_WRITE		(1 << 1) /* can perform update operations */
 #define REF_STORE_ODB		(1 << 2) /* has access to object database */
 #define REF_STORE_MAIN		(1 << 3)
+#define REF_STORE_ALL_CAPS	(REF_STORE_READ | \
+				 REF_STORE_WRITE | \
+				 REF_STORE_ODB | \
+				 REF_STORE_MAIN)
 
 /*
  * Initialize the ref_store for the specified gitdir. These functions
-- 
2.11.0.157.gd943d85

[PATCH v5 4/6] refs: introduce get_worktree_ref_store()

[Nguyễn Thái Ngọc Duy]

files-backend at this point is still aware of the per-repo/worktree
separation in refs, so it can handle a linked worktree.

Some refs operations are known not working when current files-backend is
used in a linked worktree (e.g. reflog). Tests will be written when
refs_* functions start to be called with worktree backend to verify that
they work as expected.

Note: accessing a worktree of a submodule remains unaddressed. Perhaps
after get_worktrees() can access submodule (or rather a new function
get_submodule_worktrees(), that lists worktrees of a submodule), we can
update this function to work with submodules as well.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 30 ++++++++++++++++++++++++++++++
 refs.h |  2 ++
 2 files changed, 32 insertions(+)

diff --git a/refs.c b/refs.c
index 6e6f0ec5be..7972720256 100644
--- a/refs.c
+++ b/refs.c
@@ -10,6 +10,7 @@
 #include "object.h"
 #include "tag.h"
 #include "submodule.h"
+#include "worktree.h"
 
 /*
  * List of all available backends
@@ -1486,6 +1487,9 @@ static struct ref_store *main_ref_store;
 /* A hashmap of ref_stores, stored by submodule name: */
 static struct hashmap submodule_ref_stores;
 
+/* A hashmap of ref_stores, stored by worktree id: */
+static struct hashmap worktree_ref_stores;
+
 /*
  * Look up a ref store by name. If that ref_store hasn't been
  * registered yet, return NULL.
@@ -1586,6 +1590,32 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 	return refs;
 }
 
+struct ref_store *get_worktree_ref_store(const struct worktree *wt)
+{
+	struct ref_store *refs;
+	const char *id;
+
+	if (wt->is_current)
+		return get_main_ref_store();
+
+	id = wt->id ? wt->id : "/";
+	refs = lookup_ref_store_map(&worktree_ref_stores, id);
+	if (refs)
+		return refs;
+
+	if (wt->id)
+		refs = ref_store_init(git_common_path("worktrees/%s", wt->id),
+				      REF_STORE_ALL_CAPS);
+	else
+		refs = ref_store_init(get_git_common_dir(),
+				      REF_STORE_ALL_CAPS);
+
+	if (refs)
+		register_ref_store_map(&worktree_ref_stores, "worktree",
+				       refs, id);
+	return refs;
+}
+
 void base_ref_store_init(struct ref_store *refs,
 			 const struct ref_storage_be *be)
 {
diff --git a/refs.h b/refs.h
index 49e97d7d5f..6df69a2adb 100644
--- a/refs.h
+++ b/refs.h
@@ -5,6 +5,7 @@ struct object_id;
 struct ref_store;
 struct strbuf;
 struct string_list;
+struct worktree;
 
 /*
  * Resolve a reference, recursively following symbolic refererences.
@@ -655,5 +656,6 @@ struct ref_store *get_main_ref_store(void);
  * submodule==NULL.
  */
 struct ref_store *get_submodule_ref_store(const char *submodule);
+struct ref_store *get_worktree_ref_store(const struct worktree *wt);
 
 #endif /* REFS_H */
-- 
2.11.0.157.gd943d85

[PATCH v5 5/6] worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()

[Nguyễn Thái Ngọc Duy]

The manual parsing code is replaced with a call to refs_resolve_ref_unsafe().
The manual parsing code must die because only refs/files-backend.c
should do that.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 branch.c                               |   3 +-
 t/helper/test-ref-store.c              |  18 ++++++
 t/t1407-worktree-ref-store.sh (new +x) |  40 +++++++++++++
 worktree.c                             | 102 +++++++++------------------------
 worktree.h                             |   2 +-
 5 files changed, 88 insertions(+), 77 deletions(-)
 create mode 100755 t/t1407-worktree-ref-store.sh

diff --git a/branch.c b/branch.c
index 5c12036b02..0b949b7fb2 100644
--- a/branch.c
+++ b/branch.c
@@ -355,7 +355,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/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 2d84c45ffe..4a487c014e 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "refs.h"
+#include "worktree.h"
 
 static const char *notnull(const char *arg, const char *name)
 {
@@ -32,6 +33,23 @@ static const char **get_store(const char **argv, struct ref_store **refs)
 		strbuf_release(&sb);
 
 		*refs = get_submodule_ref_store(gitdir);
+	} else if (skip_prefix(argv[0], "worktree:", &gitdir)) {
+		struct worktree **p, **worktrees = get_worktrees(0);
+
+		for (p = worktrees; *p; p++) {
+			struct worktree *wt = *p;
+
+			if (!wt->id) {
+				/* special case for main worktree */
+				if (!strcmp(gitdir, "main"))
+					break;
+			} else if (!strcmp(gitdir, wt->id))
+				break;
+		}
+		if (!*p)
+			die("no such worktree: %s", gitdir);
+
+		*refs = get_worktree_ref_store(*p);
 	} else
 		die("unknown backend %s", argv[0]);
 
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
new file mode 100755
index 0000000000..04d1e9d177
--- /dev/null
+++ b/t/t1407-worktree-ref-store.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='test worktree ref store api'
+
+. ./test-lib.sh
+
+RWT="test-ref-store worktree:wt"
+RMAIN="test-ref-store worktree:main"
+
+test_expect_success 'setup' '
+	test_commit first &&
+	git worktree add -b wt-master wt &&
+	(
+		cd wt &&
+		test_commit second
+	)
+'
+
+test_expect_success 'resolve_ref(<shared-ref>)' '
+	SHA1=`git rev-parse master` &&
+	echo "$SHA1 refs/heads/master 0x0" >expected &&
+	$RWT resolve-ref refs/heads/master 0 >actual &&
+	test_cmp expected actual &&
+	$RMAIN resolve-ref refs/heads/master 0 >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'resolve_ref(<per-worktree-ref>)' '
+	SHA1=`git -C wt rev-parse HEAD` &&
+	echo "$SHA1 refs/heads/wt-master 0x1" >expected &&
+	$RWT resolve-ref HEAD 0 >actual &&
+	test_cmp expected actual &&
+
+	SHA1=`git rev-parse HEAD` &&
+	echo "$SHA1 refs/heads/master 0x1" >expected &&
+	$RMAIN resolve-ref HEAD 0 >actual &&
+	test_cmp expected actual
+'
+
+test_done
diff --git a/worktree.c b/worktree.c
index fa7bc67a50..c1ec334b06 100644
--- a/worktree.c
+++ b/worktree.c
@@ -19,54 +19,25 @@ 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 = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
+					 "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 +48,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 +60,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 +72,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 +91,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 +293,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 +302,11 @@ 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];
+		struct ref_store *refs;
+		int flags;
+
 		if (wt->is_bare)
 			continue;
 
@@ -359,25 +321,15 @@ 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)) {
+		refs = get_worktree_ref_store(wt);
+		symref_target = refs_resolve_ref_unsafe(refs, 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;
-- 
2.11.0.157.gd943d85

[PATCH v5 6/6] refs: kill set_worktree_head_symref()

[Nguyễn Thái Ngọc Duy]

70999e9cec (branch -m: update all per-worktree HEADs - 2016-03-27)
added this function in order to update HEADs of all relevant
worktrees, when a branch is renamed.

It, as a public ref api, kind of breaks abstraction when it uses
internal functions of files backend. With the introduction of
refs_create_symref(), we can move back pretty close to the code before
70999e9cec, where create_symref() was used for updating HEAD.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 branch.c                      | 12 ++++++------
 refs.h                        | 10 ----------
 refs/files-backend.c          | 44 -------------------------------------------
 t/t1407-worktree-ref-store.sh | 12 ++++++++++++
 4 files changed, 18 insertions(+), 60 deletions(-)

diff --git a/branch.c b/branch.c
index 0b949b7fb2..69d5eea84b 100644
--- a/branch.c
+++ b/branch.c
@@ -353,18 +353,18 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref,
 	int i;
 
 	for (i = 0; worktrees[i]; i++) {
+		struct ref_store *refs;
+
 		if (worktrees[i]->is_detached)
 			continue;
 		if (worktrees[i]->head_ref &&
 		    strcmp(oldref, worktrees[i]->head_ref))
 			continue;
 
-		if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
-					     newref, logmsg)) {
-			ret = -1;
-			error(_("HEAD of working tree %s is not updated"),
-			      worktrees[i]->path);
-		}
+		refs = get_worktree_ref_store(worktrees[i]);
+		if (refs_create_symref(refs, "HEAD", newref, logmsg))
+			ret = error(_("HEAD of working tree %s is not updated"),
+				    worktrees[i]->path);
 	}
 
 	free_worktrees(worktrees);
diff --git a/refs.h b/refs.h
index 6df69a2adb..447381d378 100644
--- a/refs.h
+++ b/refs.h
@@ -402,16 +402,6 @@ int refs_create_symref(struct ref_store *refs, const char *refname,
 		       const char *target, const char *logmsg);
 int create_symref(const char *refname, const char *target, const char *logmsg);
 
-/*
- * Update HEAD of the specified gitdir.
- * Similar to create_symref("relative-git-dir/HEAD", target, NULL), but
- * this can update the main working tree's HEAD regardless of where
- * $GIT_DIR points to.
- * Return 0 if successful, non-zero otherwise.
- * */
-int set_worktree_head_symref(const char *gitdir, const char *target,
-			     const char *logmsg);
-
 enum action_on_err {
 	UPDATE_REFS_MSG_ON_ERR,
 	UPDATE_REFS_DIE_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4d705b4037..4149943a6e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3160,50 +3160,6 @@ static int files_create_symref(struct ref_store *ref_store,
 	return ret;
 }
 
-int set_worktree_head_symref(const char *gitdir, const char *target, const char *logmsg)
-{
-	/*
-	 * FIXME: this obviously will not work well for future refs
-	 * backends. This function needs to die.
-	 */
-	struct files_ref_store *refs =
-		files_downcast(get_main_ref_store(),
-			       REF_STORE_WRITE,
-			       "set_head_symref");
-
-	static struct lock_file head_lock;
-	struct ref_lock *lock;
-	struct strbuf head_path = STRBUF_INIT;
-	const char *head_rel;
-	int ret;
-
-	strbuf_addf(&head_path, "%s/HEAD", absolute_path(gitdir));
-	if (hold_lock_file_for_update(&head_lock, head_path.buf,
-				      LOCK_NO_DEREF) < 0) {
-		struct strbuf err = STRBUF_INIT;
-		unable_to_lock_message(head_path.buf, errno, &err);
-		error("%s", err.buf);
-		strbuf_release(&err);
-		strbuf_release(&head_path);
-		return -1;
-	}
-
-	/* head_rel will be "HEAD" for the main tree, "worktrees/wt/HEAD" for
-	   linked trees */
-	head_rel = remove_leading_path(head_path.buf,
-				       absolute_path(get_git_common_dir()));
-	/* to make use of create_symref_locked(), initialize ref_lock */
-	lock = xcalloc(1, sizeof(struct ref_lock));
-	lock->lk = &head_lock;
-	lock->ref_name = xstrdup(head_rel);
-
-	ret = create_symref_locked(refs, lock, head_rel, target, logmsg);
-
-	unlock_ref(lock); /* will free lock */
-	strbuf_release(&head_path);
-	return ret;
-}
-
 static int files_reflog_exists(struct ref_store *ref_store,
 			       const char *refname)
 {
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
index 04d1e9d177..5df06f3556 100755
--- a/t/t1407-worktree-ref-store.sh
+++ b/t/t1407-worktree-ref-store.sh
@@ -37,4 +37,16 @@ test_expect_success 'resolve_ref(<per-worktree-ref>)' '
 	test_cmp expected actual
 '
 
+test_expect_success 'create_symref(FOO, refs/heads/master)' '
+	$RWT create-symref FOO refs/heads/master nothing &&
+	echo refs/heads/master >expected &&
+	git -C wt symbolic-ref FOO >actual &&
+	test_cmp expected actual &&
+
+	$RMAIN create-symref FOO refs/heads/wt-master nothing &&
+	echo refs/heads/wt-master >expected &&
+	git symbolic-ref FOO >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.11.0.157.gd943d85

Re: [PATCH v5 0/6] Kill manual ref parsing code in worktree.c

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> v5 chanes are mostly cosmetic:

Good changes.  Will re-queue.

Thanks.

>
> diff --git a/refs.c b/refs.c
> index 5d31fb6bcf..7972720256 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1530,11 +1530,7 @@ struct ref_store *get_main_ref_store(void)
>  	if (main_ref_store)
>  		return main_ref_store;
>  
> -	main_ref_store = ref_store_init(get_git_dir(),
> -					(REF_STORE_READ |
> -					 REF_STORE_WRITE |
> -					 REF_STORE_ODB |
> -					 REF_STORE_MAIN));
> +	main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);
>  	return main_ref_store;
>  }
>  
> @@ -1597,9 +1593,6 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>  struct ref_store *get_worktree_ref_store(const struct worktree *wt)
>  {
>  	struct ref_store *refs;
> -	unsigned int refs_all_capabilities =
> -		REF_STORE_READ | REF_STORE_WRITE |
> -		REF_STORE_ODB | REF_STORE_MAIN;
>  	const char *id;
>  
>  	if (wt->is_current)
> @@ -1612,10 +1605,10 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)
>  
>  	if (wt->id)
>  		refs = ref_store_init(git_common_path("worktrees/%s", wt->id),
> -				      refs_all_capabilities);
> +				      REF_STORE_ALL_CAPS);
>  	else
>  		refs = ref_store_init(get_git_common_dir(),
> -				      refs_all_capabilities);
> +				      REF_STORE_ALL_CAPS);
>  
>  	if (refs)
>  		register_ref_store_map(&worktree_ref_stores, "worktree",
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 690498698e..b26f7e41ce 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -467,6 +467,10 @@ struct ref_store;
>  #define REF_STORE_WRITE		(1 << 1) /* can perform update operations */
>  #define REF_STORE_ODB		(1 << 2) /* has access to object database */
>  #define REF_STORE_MAIN		(1 << 3)
> +#define REF_STORE_ALL_CAPS	(REF_STORE_READ | \
> +				 REF_STORE_WRITE | \
> +				 REF_STORE_ODB | \
> +				 REF_STORE_MAIN)
>  
>  /*
>   * Initialize the ref_store for the specified gitdir. These functions
>
> Nguyễn Thái Ngọc Duy (6):
>   environment.c: fix potential segfault by get_git_common_dir()
>   refs.c: make submodule ref store hashmap generic
>   refs: add REFS_STORE_ALL_CAPS
>   refs: introduce get_worktree_ref_store()
>   worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
>   refs: kill set_worktree_head_symref()
>
>  branch.c                               |  15 ++---
>  environment.c                          |   2 +
>  refs.c                                 | 100 ++++++++++++++++++++------------
>  refs.h                                 |  12 +---
>  refs/files-backend.c                   |  44 --------------
>  refs/refs-internal.h                   |   4 ++
>  t/helper/test-ref-store.c              |  18 ++++++
>  t/t1407-worktree-ref-store.sh (new +x) |  52 +++++++++++++++++
>  worktree.c                             | 102 +++++++++------------------------
>  worktree.h                             |   2 +-
>  10 files changed, 177 insertions(+), 174 deletions(-)
>  create mode 100755 t/t1407-worktree-ref-store.sh