diff mbox series

[05/23] fsmonitor-settings: bare repos are incompatible with FSMonitor

Message ID 44cc61e186cb65fa6b2c1d5a0f080fc0b2265e57.1644940773.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Builtin FSMonitor Part 3 | expand

Commit Message

Jeff Hostetler Feb. 15, 2022, 3:59 p.m. UTC 
From: Jeff Hostetler <jeffhost@microsoft.com>

Bare repos do not have a worktree, so there is nothing for the
daemon watch.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/fsmonitor--daemon.c | 11 +++++++
 builtin/update-index.c      |  9 ++++++
 fsmonitor-settings.c        | 62 +++++++++++++++++++++++++++++++++++++
 fsmonitor-settings.h        | 11 +++++++
 t/t7519-status-fsmonitor.sh | 26 ++++++++++++++++
 5 files changed, 119 insertions(+)

Comments

Ævar Arnfjörð Bjarmason Feb. 25, 2022, 8:42 p.m. UTC | #1 
On Tue, Feb 15 2022, Jeff Hostetler via GitGitGadget wrote:

> +static void create_reason_message(struct repository *r,
> +				  struct strbuf *buf_reason)
> +{
> +	struct fsmonitor_settings *s = r->settings.fsmonitor;
> +
> +	switch (s->reason) {
> +	case FSMONITOR_REASON_ZERO:
> +		return;
> +
> +	case FSMONITOR_REASON_BARE:
> +		strbuf_addstr(buf_reason,
> +			      _("bare repos are incompatible with fsmonitor"));
> +		return;
> +
> +	default:
> +		BUG("Unhandled case in create_reason_message '%d'", s->reason);
> +	}
> +}
> +
> +enum fsmonitor_reason fsm_settings__get_reason(struct repository *r,
> +					       struct strbuf *buf_reason)
> +{
> +	lookup_fsmonitor_settings(r);
> +
> +	strbuf_reset(buf_reason);
> +	if (r->settings.fsmonitor->mode == FSMONITOR_MODE_INCOMPATIBLE)
> +		create_reason_message(r, buf_reason);
> +
> +	return r->settings.fsmonitor->reason;
> +}

This API (just looking at one small bit discussed because related bits
conflict with another series) seems to require a lot of ceremony just to
get a const char * error.

I tried this on top of "seen", and the parts I compile on Linux (so not
the fsmonitor--daemon.c) were happy with it.

 builtin/fsmonitor--daemon.c | 14 +++++++-------
 builtin/update-index.c      |  9 ++++-----
 fsmonitor-settings.c        | 47 +++++++++++++++------------------------------
 fsmonitor-settings.h        |  5 +++--
 4 files changed, 29 insertions(+), 46 deletions(-)

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 591433e897d..7ad7bc718b3 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1496,7 +1496,7 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
 {
 	const char *subcmd;
 	int free_console = 0;
-
+	enum fsmonitor_mode fsm_mode;
 	struct option options[] = {
 		OPT_BOOL(0, "free-console", &free_console, N_("free console")),
 		OPT_INTEGER(0, "ipc-threads",
@@ -1524,12 +1524,12 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
 	prepare_repo_settings(the_repository);
 	fsm_settings__set_ipc(the_repository);
 
-	if (fsm_settings__get_mode(the_repository) == FSMONITOR_MODE_INCOMPATIBLE) {
-		struct strbuf buf_reason = STRBUF_INIT;
-		fsm_settings__get_reason(the_repository, &buf_reason);
-		error("%s '%s'", buf_reason.buf, xgetcwd());
-		strbuf_release(&buf_reason);
-		return -1;
+	fsm_mode = fsm_settings__get_mode(the_repository);
+	if (fsm_mode == FSMONITOR_MODE_INCOMPATIBLE) {
+		enum fsmonitor_reason fsm_reason = fsm_settings__get_reason(the_repository);
+		const char *reason = fsm_settings_incompatible_reason_msg(fsm_mode, fsm_reason);
+
+		return error("%s", reason ? reason : "???");
 	}
 
 	if (!strcmp(subcmd, "start"))
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 61b0b98ccaf..f8f638d33d9 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1237,13 +1237,12 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 	if (fsmonitor > 0) {
 		enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
+		enum fsmonitor_reason fsm_reason = fsm_settings__get_reason(r);
 
 		if (fsm_mode == FSMONITOR_MODE_INCOMPATIBLE) {
-			struct strbuf buf_reason = STRBUF_INIT;
-			fsm_settings__get_reason(r, &buf_reason);
-			error("%s", buf_reason.buf);
-			strbuf_release(&buf_reason);
-			return -1;
+			const char *reason = fsm_settings_incompatible_reason_msg(fsm_mode, fsm_reason);
+
+			return error("%s", reason ? reason : "???");
 		}
 
 		if (fsm_mode == FSMONITOR_MODE_DISABLED) {
diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index de69ace246a..e37b342aa2b 100644
--- a/fsmonitor-settings.c
+++ b/fsmonitor-settings.c
@@ -103,6 +103,13 @@ enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
 	return r->settings.fsmonitor->mode;
 }
 
+enum fsmonitor_reason fsm_settings__get_reason(struct repository *r)
+{
+	lookup_fsmonitor_settings(r);
+
+	return r->settings.fsmonitor->reason;
+}
+
 const char *fsm_settings__get_hook_path(struct repository *r)
 {
 	lookup_fsmonitor_settings(r);
@@ -142,43 +149,19 @@ void fsm_settings__set_disabled(struct repository *r)
 	FREE_AND_NULL(r->settings.fsmonitor->hook_path);
 }
 
-static void create_reason_message(struct repository *r,
-				  struct strbuf *buf_reason)
+const char *fsm_settings_incompatible_reason_msg(enum fsmonitor_mode mode,
+						 enum fsmonitor_reason reason)
 {
-	struct fsmonitor_settings *s = r->settings.fsmonitor;
+	assert(mode == FSMONITOR_MODE_INCOMPATIBLE);
 
-	switch (s->reason) {
+	switch (reason) {
 	case FSMONITOR_REASON_ZERO:
-		return;
-
+		return NULL;
 	case FSMONITOR_REASON_BARE:
-		strbuf_addstr(buf_reason,
-			      _("bare repos are incompatible with fsmonitor"));
-		return;
-
+		return _("bare repos are incompatible with fsmonitor");
 	case FSMONITOR_REASON_VIRTUAL:
-		strbuf_addstr(buf_reason,
-			      _("virtual repos are incompatible with fsmonitor"));
-		return;
-
+		return _("virtual repos are incompatible with fsmonitor");
 	case FSMONITOR_REASON_REMOTE:
-		strbuf_addstr(buf_reason,
-			      _("remote repos are incompatible with fsmonitor"));
-		return;
-
-	default:
-		BUG("Unhandled case in create_reason_message '%d'", s->reason);
+		return _("remote repos are incompatible with fsmonitor");
 	}
 }
-
-enum fsmonitor_reason fsm_settings__get_reason(struct repository *r,
-					       struct strbuf *buf_reason)
-{
-	lookup_fsmonitor_settings(r);
-
-	strbuf_reset(buf_reason);
-	if (r->settings.fsmonitor->mode == FSMONITOR_MODE_INCOMPATIBLE)
-		create_reason_message(r, buf_reason);
-
-	return r->settings.fsmonitor->reason;
-}
diff --git a/fsmonitor-settings.h b/fsmonitor-settings.h
index fca25887c0f..81be1ef1801 100644
--- a/fsmonitor-settings.h
+++ b/fsmonitor-settings.h
@@ -25,9 +25,10 @@ void fsm_settings__set_hook(struct repository *r, const char *path);
 void fsm_settings__set_disabled(struct repository *r);
 
 enum fsmonitor_mode fsm_settings__get_mode(struct repository *r);
+enum fsmonitor_reason fsm_settings__get_reason(struct repository *r);
 const char *fsm_settings__get_hook_path(struct repository *r);
-enum fsmonitor_reason fsm_settings__get_reason(struct repository *r,
-					       struct strbuf *buf_reason);
+const char *fsm_settings_incompatible_reason_msg(enum fsmonitor_mode mode,
+						 enum fsmonitor_reason reason);
 
 struct fsmonitor_settings;
Jeff Hostetler March 2, 2022, 9:09 p.m. UTC | #2 
On 2/25/22 3:42 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Feb 15 2022, Jeff Hostetler via GitGitGadget wrote:
> 
>> +static void create_reason_message(struct repository *r,
>> +				  struct strbuf *buf_reason)
>> +{
>> +	struct fsmonitor_settings *s = r->settings.fsmonitor;
>> +
>> +	switch (s->reason) {
>> +	case FSMONITOR_REASON_ZERO:
>> +		return;
>> +
>> +	case FSMONITOR_REASON_BARE:
>> +		strbuf_addstr(buf_reason,
>> +			      _("bare repos are incompatible with fsmonitor"));
>> +		return;
>> +
>> +	default:
>> +		BUG("Unhandled case in create_reason_message '%d'", s->reason);
>> +	}
>> +}
>> +
>> +enum fsmonitor_reason fsm_settings__get_reason(struct repository *r,
>> +					       struct strbuf *buf_reason)
>> +{
>> +	lookup_fsmonitor_settings(r);
>> +
>> +	strbuf_reset(buf_reason);
>> +	if (r->settings.fsmonitor->mode == FSMONITOR_MODE_INCOMPATIBLE)
>> +		create_reason_message(r, buf_reason);
>> +
>> +	return r->settings.fsmonitor->reason;
>> +}
> 
> This API (just looking at one small bit discussed because related bits
> conflict with another series) seems to require a lot of ceremony just to
> get a const char * error.

My thought at the time was that the __get_reason() code might want to
format a message and include details from the repo (such as the working
directory root) or the kind of objection (such as a remote SMB mount).
So I had it take a strbuf rather than a "const char *" return value.

But so far (in the successive commits) all of the reason messages have
been constant and I think I'm OK with having the non-specific messages.

So yes, I could simplify it.

Thanks
Jeff
diff mbox series

Patch

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 6011fe42ee0..899355c55aa 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1424,6 +1424,17 @@  int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
 		die(_("invalid 'ipc-threads' value (%d)"),
 		    fsmonitor__ipc_threads);
 
+	prepare_repo_settings(the_repository);
+	fsm_settings__set_ipc(the_repository);
+
+	if (fsm_settings__get_mode(the_repository) == FSMONITOR_MODE_INCOMPATIBLE) {
+		struct strbuf buf_reason = STRBUF_INIT;
+		fsm_settings__get_reason(the_repository, &buf_reason);
+		error("%s '%s'", buf_reason.buf, xgetcwd());
+		strbuf_release(&buf_reason);
+		return -1;
+	}
+
 	if (!strcmp(subcmd, "start"))
 		return !!try_to_start_background_daemon();
 
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 1232f0d214e..9a2c45f2e56 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1215,6 +1215,15 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 	if (fsmonitor > 0) {
 		enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
+
+		if (fsm_mode == FSMONITOR_MODE_INCOMPATIBLE) {
+			struct strbuf buf_reason = STRBUF_INIT;
+			fsm_settings__get_reason(r, &buf_reason);
+			error("%s", buf_reason.buf);
+			strbuf_release(&buf_reason);
+			return -1;
+		}
+
 		if (fsm_mode == FSMONITOR_MODE_DISABLED) {
 			advise(_("core.fsmonitor is unset; "
 				 "set it if you really want to "
diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index eb4d661c81e..0fc5566eb8a 100644
--- a/fsmonitor-settings.c
+++ b/fsmonitor-settings.c
@@ -9,9 +9,33 @@ 
  */
 struct fsmonitor_settings {
 	enum fsmonitor_mode mode;
+	enum fsmonitor_reason reason;
 	char *hook_path;
 };
 
+static void set_incompatible(struct repository *r,
+			     enum fsmonitor_reason reason)
+{
+	struct fsmonitor_settings *s = r->settings.fsmonitor;
+
+	s->mode = FSMONITOR_MODE_INCOMPATIBLE;
+	s->reason = reason;
+}
+
+static int check_for_incompatible(struct repository *r)
+{
+	if (!r->worktree) {
+		/*
+		 * Bare repositories don't have a working directory and
+		 * therefore have nothing to watch.
+		 */
+		set_incompatible(r, FSMONITOR_REASON_BARE);
+		return 1;
+	}
+
+	return 0;
+}
+
 static void lookup_fsmonitor_settings(struct repository *r)
 {
 	struct fsmonitor_settings *s;
@@ -78,6 +102,9 @@  void fsm_settings__set_ipc(struct repository *r)
 {
 	lookup_fsmonitor_settings(r);
 
+	if (check_for_incompatible(r))
+		return;
+
 	r->settings.fsmonitor->mode = FSMONITOR_MODE_IPC;
 	FREE_AND_NULL(r->settings.fsmonitor->hook_path);
 }
@@ -86,6 +113,9 @@  void fsm_settings__set_hook(struct repository *r, const char *path)
 {
 	lookup_fsmonitor_settings(r);
 
+	if (check_for_incompatible(r))
+		return;
+
 	r->settings.fsmonitor->mode = FSMONITOR_MODE_HOOK;
 	FREE_AND_NULL(r->settings.fsmonitor->hook_path);
 	r->settings.fsmonitor->hook_path = strdup(path);
@@ -96,5 +126,37 @@  void fsm_settings__set_disabled(struct repository *r)
 	lookup_fsmonitor_settings(r);
 
 	r->settings.fsmonitor->mode = FSMONITOR_MODE_DISABLED;
+	r->settings.fsmonitor->reason = FSMONITOR_REASON_ZERO;
 	FREE_AND_NULL(r->settings.fsmonitor->hook_path);
 }
+
+static void create_reason_message(struct repository *r,
+				  struct strbuf *buf_reason)
+{
+	struct fsmonitor_settings *s = r->settings.fsmonitor;
+
+	switch (s->reason) {
+	case FSMONITOR_REASON_ZERO:
+		return;
+
+	case FSMONITOR_REASON_BARE:
+		strbuf_addstr(buf_reason,
+			      _("bare repos are incompatible with fsmonitor"));
+		return;
+
+	default:
+		BUG("Unhandled case in create_reason_message '%d'", s->reason);
+	}
+}
+
+enum fsmonitor_reason fsm_settings__get_reason(struct repository *r,
+					       struct strbuf *buf_reason)
+{
+	lookup_fsmonitor_settings(r);
+
+	strbuf_reset(buf_reason);
+	if (r->settings.fsmonitor->mode == FSMONITOR_MODE_INCOMPATIBLE)
+		create_reason_message(r, buf_reason);
+
+	return r->settings.fsmonitor->reason;
+}
diff --git a/fsmonitor-settings.h b/fsmonitor-settings.h
index a4c5d7b4889..e5f145a2f79 100644
--- a/fsmonitor-settings.h
+++ b/fsmonitor-settings.h
@@ -4,17 +4,28 @@ 
 struct repository;
 
 enum fsmonitor_mode {
+	FSMONITOR_MODE_INCOMPATIBLE = -1, /* see _reason */
 	FSMONITOR_MODE_DISABLED = 0,
 	FSMONITOR_MODE_HOOK = 1, /* core.fsmonitor=<hook_path> */
 	FSMONITOR_MODE_IPC = 2,  /* core.fsmonitor=<true> */
 };
 
+/*
+ * Incompatibility reasons.
+ */
+enum fsmonitor_reason {
+	FSMONITOR_REASON_ZERO = 0,
+	FSMONITOR_REASON_BARE = 1,
+};
+
 void fsm_settings__set_ipc(struct repository *r);
 void fsm_settings__set_hook(struct repository *r, const char *path);
 void fsm_settings__set_disabled(struct repository *r);
 
 enum fsmonitor_mode fsm_settings__get_mode(struct repository *r);
 const char *fsm_settings__get_hook_path(struct repository *r);
+enum fsmonitor_reason fsm_settings__get_reason(struct repository *r,
+					       struct strbuf *buf_reason);
 
 struct fsmonitor_settings;
 
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index f488d930dfd..3c4e6f5f89c 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -55,6 +55,32 @@  test_lazy_prereq UNTRACKED_CACHE '
 	test $ret -ne 1
 '
 
+# Test that we detect and disallow repos that are incompatible with FSMonitor.
+test_expect_success 'incompatible bare repo' '
+	test_when_finished "rm -rf ./bare-clone actual expect" &&
+	git init --bare bare-clone &&
+	cat >expect <<-\EOF &&
+	error: bare repos are incompatible with fsmonitor
+	EOF
+
+	test_must_fail \
+		git -C ./bare-clone -c core.fsmonitor=foo \
+			update-index --fsmonitor 2>actual &&
+	test_cmp expect actual &&
+
+	test_must_fail \
+		git -C ./bare-clone -c core.fsmonitor=true \
+			update-index --fsmonitor 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success FSMONITOR_DAEMON 'run fsmonitor-daemon in bare repo' '
+	test_when_finished "rm -rf ./bare-clone actual" &&
+	git init --bare bare-clone &&
+	test_must_fail git -C ./bare-clone fsmonitor--daemon run 2>actual &&
+	grep "bare repos are incompatible with fsmonitor" actual
+'
+
 test_expect_success 'setup' '
 	mkdir -p .git/hooks &&
 	: >tracked &&