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