Re: [RFC PATCH 3/3] fsck: Check HEAD of other worktrees as well

[Junio C Hamano]

Duy Nguyen  writes:

> On Fri, Feb 09, 2018 at 03:13:30PM -0800, Elijah Newren wrote:
>> This takes advantage of the fact that "worktrees/$WORKTREE/HEAD" will
>> currently resolve as a ref, which may not be true in the future with
>> different ref backends.  I don't think it locks us in to supporting
>> resolving other worktree HEADs with this syntax, as I view the
>> user-visible error message as more of a pointer to a pathname that the
>> user will need to fix.  If the underlying ref storage changes, naturally
>> both this code and the hint may need to change to match.
>
> I'm leaning more about something like this patch below (which does not
> even build, just to demonstrate). A couple points:
>
> - Instead of doing the hacky refs worktrees/foo/HEAD, we get the
>   correct ref store for each worktree
> - We have an API for getting all (non-broken) worktrees. I realize for
>   fsck, we may even want to examine semi-broken worktrees as well, but
>   get_worktrees() can take a flag to accomplish that if needed.

Very sensible.  

When I saw that "fsck" thing, the first thing I wondered was "wait,
how are we doing prune and repack while making sure objects
reachable only from HEAD in other worktrees are not lost?  we must
already have an API that gives us where the refs of them are stored
in".

Thanks for a quick response for course correction.

Re: [RFC PATCH 3/3] fsck: Check HEAD of other worktrees as well

[Jeff King]

On Sat, Feb 10, 2018 at 04:59:52PM +0700, Duy Nguyen wrote:

> On Fri, Feb 09, 2018 at 03:13:30PM -0800, Elijah Newren wrote:
> > This takes advantage of the fact that "worktrees/$WORKTREE/HEAD" will
> > currently resolve as a ref, which may not be true in the future with
> > different ref backends.  I don't think it locks us in to supporting
> > resolving other worktree HEADs with this syntax, as I view the
> > user-visible error message as more of a pointer to a pathname that the
> > user will need to fix.  If the underlying ref storage changes, naturally
> > both this code and the hint may need to change to match.
> 
> I'm leaning more about something like this patch below (which does not
> even build, just to demonstrate). A couple points:
> 
> - Instead of doing the hacky refs worktrees/foo/HEAD, we get the
>   correct ref store for each worktree
> - We have an API for getting all (non-broken) worktrees. I realize for
>   fsck, we may even want to examine semi-broken worktrees as well, but
>   get_worktrees() can take a flag to accomplish that if needed.
> - As you can see, I print %p from the ref store instead of something
>   human friendly. This is something I was stuck at last time. I need
>   better ref store description (or even the worktree name)

Yeah, I think this is the right approach. We know about worktrees
internally, and we should be able to iterate over their ref stores, even
if we have no way to succinctly name the resulting ref.

> - This ref_name() function makes me think if we should have an
>   extended sha1 syntax for accessing per-worktree refs from a
>   different worktree, something like HEAD@{worktree:foo} to resolve to
>   worktrees/foo/HEAD. Then we have a better description here that can
>   actually be used from command line, as a regular ref, if needed.

Yeah, I agree this would be very useful. I peeked at how bad it would be
to hanlde this. The parsing is pretty easy in get_oid_basic(), but you'd
have to plumb through the ref_store in quite a few places:

 - interpret_nth_prior_checkout() would probably want to use the
   worktree's HEAD (for "@{worktree:foo}@{-1}")

 - dwim_ref() would need to know about the ref store

 - that implies that substitute_branch_name() knows about it, too

So it may turn into a big job. But I think it's moving in the right
direction. And it may not be the end of the world if all features do not
work from day one (e.g., if HEAD@{worktree:foo} works, but
HEAD@{worktree:foo}@{upstream} does not yet, with plans to eventually
make that work).

-Peff

Re: [RFC PATCH 3/3] fsck: Check HEAD of other worktrees as well

[Duy Nguyen]

On Fri, Feb 09, 2018 at 03:13:30PM -0800, Elijah Newren wrote:
> This takes advantage of the fact that "worktrees/$WORKTREE/HEAD" will
> currently resolve as a ref, which may not be true in the future with
> different ref backends.  I don't think it locks us in to supporting
> resolving other worktree HEADs with this syntax, as I view the
> user-visible error message as more of a pointer to a pathname that the
> user will need to fix.  If the underlying ref storage changes, naturally
> both this code and the hint may need to change to match.

I'm leaning more about something like this patch below (which does not
even build, just to demonstrate). A couple points:

- Instead of doing the hacky refs worktrees/foo/HEAD, we get the
  correct ref store for each worktree
- We have an API for getting all (non-broken) worktrees. I realize for
  fsck, we may even want to examine semi-broken worktrees as well, but
  get_worktrees() can take a flag to accomplish that if needed.
- As you can see, I print %p from the ref store instead of something
  human friendly. This is something I was stuck at last time. I need
  better ref store description (or even the worktree name)
- This ref_name() function makes me think if we should have an
  extended sha1 syntax for accessing per-worktree refs from a
  different worktree, something like HEAD@{worktree:foo} to resolve to
  worktrees/foo/HEAD. Then we have a better description here that can
  actually be used from command line, as a regular ref, if needed.

-- 8< --
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 4132034170..73cfcbc07a 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -16,6 +16,7 @@
 #include "streaming.h"
 #include "decorate.h"
 #include "packfile.h"
+#include "worktree.h"
 
 #define REACHABLE 0x0001
 #define SEEN  0x0002
@@ -451,17 +452,39 @@ static int fsck_handle_ref(const char *refname, const 
struct object_id *oid,
return 0;
 }
 
-static int fsck_head_link(const char **head_points_at,
+static const char *ref_name(struct ref_store *refs, const char *name)
+{
+   static struct strbuf sb = STRBUF_INIT;
+
+   if (!refs)
+   return name;
+   strbuf_reset();
+   strbuf_addf(, "%s (from %p)", name, refs);
+   return sb.buf;
+}
+
+static int fsck_head_link(struct ref_store *refs,
+ const char **head_points_at,
  struct object_id *head_oid);
 
 static void get_default_heads(void)
 {
const char *head_points_at;
struct object_id head_oid;
+   struct worktree **worktrees, **wt;
 
-   fsck_head_link(_points_at, _oid);
+   fsck_head_link(NULL, _points_at, _oid);
if (head_points_at && !is_null_oid(_oid))
fsck_handle_ref("HEAD", _oid, 0, NULL);
+
+   worktrees = get_worktrees(0);
+   for (wt = worktrees; *wt; wt++) {
+   fsck_head_link(get_worktree_ref_store(*wt), _points_at, 
_oid);
+   if (head_points_at && !is_null_oid(_oid))
+   fsck_handle_ref(*wt, "HEAD", _oid, 0, NULL);
+   }
+   free_worktrees(wt);
+
for_each_rawref(fsck_handle_ref, NULL);
if (include_reflogs)
for_each_reflog(fsck_handle_reflog, NULL);
@@ -553,34 +576,36 @@ static void fsck_object_dir(const char *path)
stop_progress();
 }
 
-static int fsck_head_link(const char **head_points_at,
+static int fsck_head_link(struct ref_store *refs,
+ const char **head_points_at,
  struct object_id *head_oid)
 {
int null_is_error = 0;
 
if (verbose)
-   fprintf(stderr, "Checking HEAD link\n");
+   fprintf(stderr, "Checking %s link\n", ref_name(refs, "HEAD"));
 
-   *head_points_at = resolve_ref_unsafe("HEAD", 0, head_oid, NULL);
+   *head_points_at = refs_resolve_ref_unsafe(refs, "HEAD", 0, head_oid, 
NULL);
if (!*head_points_at) {
errors_found |= ERROR_REFS;
-   return error("Invalid HEAD");
+   return error("Invalid HEAD from ref-store %p", refs);
}
if (!strcmp(*head_points_at, "HEAD"))
/* detached HEAD */
null_is_error = 1;
else if (!starts_with(*head_points_at, "refs/heads/")) {
errors_found |= ERROR_REFS;
-   return error("HEAD points to something strange (%s)",
-*head_points_at);
+   return error("%s points to something strange (%s)",
+ref_name(refs, "HEAD"), *head_points_at);
}
if (is_null_oid(head_oid)) {
if (null_is_error) {
errors_found |= ERROR_REFS;
-   return error("HEAD: detached HEAD points at nothing");
+   return error("%s: detached HEAD points at nothing",
+ref_name(refs, "HEAD"));
}
-   fprintf(stderr, 

[RFC PATCH 3/3] fsck: Check HEAD of other worktrees as well

[Elijah Newren]

This takes advantage of the fact that "worktrees/$WORKTREE/HEAD" will
currently resolve as a ref, which may not be true in the future with
different ref backends.  I don't think it locks us in to supporting
resolving other worktree HEADs with this syntax, as I view the
user-visible error message as more of a pointer to a pathname that the
user will need to fix.  If the underlying ref storage changes, naturally
both this code and the hint may need to change to match.

Signed-off-by: Elijah Newren 
---
 builtin/fsck.c  | 60 +
 t/t1450-fsck.sh |  6 +++---
 2 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 4132034170..850b217e93 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -451,17 +451,51 @@ static int fsck_handle_ref(const char *refname, const 
struct object_id *oid,
return 0;
 }
 
-static int fsck_head_link(const char **head_points_at,
+static void get_worktree_names(struct string_list *names)
+{
+   struct strbuf path = STRBUF_INIT;
+   DIR *dir;
+   struct dirent *d;
+
+   strbuf_addf(, "%s/worktrees", get_git_common_dir());
+   dir = opendir(path.buf);
+   strbuf_release();
+   if (dir) {
+   while ((d = readdir(dir)) != NULL) {
+   if (!is_dot_or_dotdot(d->d_name))
+   string_list_append(names, d->d_name);
+   }
+   closedir(dir);
+   }
+}
+
+static int fsck_head_link(const char *head_ref_name,
+ const char **head_points_at,
  struct object_id *head_oid);
 
 static void get_default_heads(void)
 {
const char *head_points_at;
struct object_id head_oid;
+   struct string_list worktree_names = STRING_LIST_INIT_DUP;
+   struct string_list_item *worktree_item;
+   struct strbuf head_name = STRBUF_INIT;
 
-   fsck_head_link(_points_at, _oid);
+   fsck_head_link("HEAD", _points_at, _oid);
if (head_points_at && !is_null_oid(_oid))
fsck_handle_ref("HEAD", _oid, 0, NULL);
+
+   get_worktree_names(_names);
+   for_each_string_list_item(worktree_item, _names) {
+   strbuf_reset(_name);
+   strbuf_addf(_name, "worktrees/%s/HEAD",
+   worktree_item->string);
+   fsck_head_link(head_name.buf, _points_at, _oid);
+   if (head_points_at && !is_null_oid(_oid))
+   fsck_handle_ref(head_name.buf, _oid, 0, NULL);
+   }
+   strbuf_release(_name);
+
for_each_rawref(fsck_handle_ref, NULL);
if (include_reflogs)
for_each_reflog(fsck_handle_reflog, NULL);
@@ -553,34 +587,36 @@ static void fsck_object_dir(const char *path)
stop_progress();
 }
 
-static int fsck_head_link(const char **head_points_at,
+static int fsck_head_link(const char *head_ref_name,
+ const char **head_points_at,
  struct object_id *head_oid)
 {
int null_is_error = 0;
 
if (verbose)
-   fprintf(stderr, "Checking HEAD link\n");
+   fprintf(stderr, "Checking %s link\n", head_ref_name);
 
-   *head_points_at = resolve_ref_unsafe("HEAD", 0, head_oid, NULL);
+   *head_points_at = resolve_ref_unsafe(head_ref_name, 0, head_oid, NULL);
if (!*head_points_at) {
errors_found |= ERROR_REFS;
-   return error("Invalid HEAD");
+   return error("Invalid %s", head_ref_name);
}
-   if (!strcmp(*head_points_at, "HEAD"))
+   if (!strcmp(*head_points_at, head_ref_name))
/* detached HEAD */
null_is_error = 1;
else if (!starts_with(*head_points_at, "refs/heads/")) {
errors_found |= ERROR_REFS;
-   return error("HEAD points to something strange (%s)",
-*head_points_at);
+   return error("%s points to something strange (%s)",
+head_ref_name, *head_points_at);
}
if (is_null_oid(head_oid)) {
if (null_is_error) {
errors_found |= ERROR_REFS;
-   return error("HEAD: detached HEAD points at nothing");
+   return error("%s: detached HEAD points at nothing",
+head_ref_name);
}
-   fprintf(stderr, "notice: HEAD points to an unborn branch 
(%s)\n",
-   *head_points_at + 11);
+   fprintf(stderr, "notice: %s points to an unborn branch (%s)\n",
+   head_ref_name, *head_points_at + 11);
}
return 0;
 }
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index fa94c59458..3da651be4c 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -102,7 +102,7 @@ test_expect_success 'HEAD link