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

[Duy Nguyen <pclouds@gmail.com>]

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(&sb);
+	strbuf_addf(&sb, "%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(&head_points_at, &head_oid);
+	fsck_head_link(NULL, &head_points_at, &head_oid);
 	if (head_points_at && !is_null_oid(&head_oid))
 		fsck_handle_ref("HEAD", &head_oid, 0, NULL);
+
+	worktrees = get_worktrees(0);
+	for (wt = worktrees; *wt; wt++) {
+		fsck_head_link(get_worktree_ref_store(*wt), &head_points_at, &head_oid);
+		if (head_points_at && !is_null_oid(&head_oid))
+			fsck_handle_ref(*wt, "HEAD", &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(&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, "notice: HEAD points to an unborn branch (%s)\n",
-			*head_points_at + 11);
+		fprintf(stderr, "notice: %s points to an unborn branch (%s)\n",
+			ref_name(refs, "HEAD"), *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 pointing at a funny place' '
 	grep "HEAD points to something strange" out
 '
 
-test_expect_failure 'other worktree HEAD link pointing at a funny object' '
+test_expect_success 'other worktree HEAD link pointing at a funny object' '
 	test_when_finished "rm -rf .git/worktrees" &&
 	mkdir -p .git/worktrees/other &&
 	echo 0000000000000000000000000000000000000000 >.git/worktrees/other/HEAD &&
@@ -111,7 +111,7 @@ test_expect_failure 'other worktree HEAD link pointing at a funny object' '
 	grep "worktrees/other/HEAD: detached HEAD points" out
 '
 
-test_expect_failure 'other worktree HEAD link pointing at missing object' '
+test_expect_success 'other worktree HEAD link pointing at missing object' '
 	test_when_finished "rm -rf .git/worktrees" &&
 	mkdir -p .git/worktrees/other &&
 	echo "Contents missing from repo" | git hash-object --stdin >.git/worktrees/other/HEAD &&
@@ -120,7 +120,7 @@ test_expect_failure 'other worktree HEAD link pointing at missing object' '
 	grep "worktrees/other/HEAD: invalid sha1 pointer" out
 '
 
-test_expect_failure 'other worktree HEAD link pointing at a funny place' '
+test_expect_success 'other worktree HEAD link pointing at a funny place' '
 	test_when_finished "rm -rf .git/worktrees" &&
 	mkdir -p .git/worktrees/other &&
 	echo "ref: refs/funny/place" >.git/worktrees/other/HEAD &&


-- 8< --