diff mbox series

[v2,04/27] fsmonitor-settings: bare repos are incompatible with FSMonitor

Message ID 8c4f90ae4fd5d9fbac9acb9307ee82ceffc7df08.1646777727.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Builtin FSMonitor Part 3 | expand

Commit Message

Jeff Hostetler March 8, 2022, 10:15 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 |  9 ++++++
 builtin/update-index.c      |  7 +++++
 fsmonitor-settings.c        | 57 +++++++++++++++++++++++++++++++++++++
 fsmonitor-settings.h        | 12 ++++++++
 t/t7519-status-fsmonitor.sh | 23 +++++++++++++++
 5 files changed, 108 insertions(+)

Comments

Ævar Arnfjörð Bjarmason March 11, 2022, 1:31 a.m. UTC | #1 
On Tue, Mar 08 2022, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
> [...]
> +	prepare_repo_settings(the_repository);
> +	fsm_settings__set_ipc(the_repository);
> +
> +	if (fsm_settings__get_mode(the_repository) == FSMONITOR_MODE_INCOMPATIBLE) {
> +		const char *msg = fsm_settings__get_reason_msg(the_repository);
> +
> +		return error("%s '%s'", msg ? msg : "???", xgetcwd());
> +	}
> +
>  	if (!strcmp(subcmd, "start"))
>  		return !!try_to_start_background_daemon();
>  
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index d335f1ac72a..8f460e7195f 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1237,6 +1237,13 @@ 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) {
> +			const char *msg = fsm_settings__get_reason_msg(r);
> +
> +			return error("%s '%s'", msg ? msg : "???", xgetcwd());
> +		}
> +
>  		if (fsm_mode == FSMONITOR_MODE_DISABLED) {
>  			advise(_("core.fsmonitor is unset; "
>  				 "set it if you really want to "

Can w assert somewhere earlier that ->mode can't be
FSMONITOR_MODE_INCOMPATIBLE at the same time that ->reason ==
FSMONITOR_REASON_OK, should that ever happen?

Then we can get rid of the "???" case here.

The "%s '%s'" here should really be marked for translation, but just
"some reason '$path'" is a pretty confusing message. This will emit
e.g.:

    "bare repos are incompatible with fsmonitor '/some/path/to/repo'"

Since we always hand these to error maybe have the helper do e.g.:

    error(_("bare repository '%s' is incompatible with fsmonitor"), path);

I find the second-guessing in fsmonitor-settings.c really hard to
follow, i.e. how seemingly every function has some "not loaded yet? load
it" instead of a more typical "init it", "use it", "free it"
pattern. Including stuff like this:
	
	enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
	{
	        if (!r)
	                r = the_repository;

But anyway, seeing as we do try really hard to load the_repository (or a
repository) can't we use the_repository->gitdir etc. here instead of
xgetcwd(), or the_repository->worktree when non-bare?
Jeff Hostetler March 11, 2022, 10:25 p.m. UTC | #2 
On 3/10/22 8:31 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Mar 08 2022, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>> [...]
>> +	prepare_repo_settings(the_repository);
>> +	fsm_settings__set_ipc(the_repository);
>> +
>> +	if (fsm_settings__get_mode(the_repository) == FSMONITOR_MODE_INCOMPATIBLE) {
>> +		const char *msg = fsm_settings__get_reason_msg(the_repository);
>> +
>> +		return error("%s '%s'", msg ? msg : "???", xgetcwd());
>> +	}
>> +
>>   	if (!strcmp(subcmd, "start"))
>>   		return !!try_to_start_background_daemon();
>>   
>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index d335f1ac72a..8f460e7195f 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -1237,6 +1237,13 @@ 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) {
>> +			const char *msg = fsm_settings__get_reason_msg(r);
>> +
>> +			return error("%s '%s'", msg ? msg : "???", xgetcwd());
>> +		}
>> +
>>   		if (fsm_mode == FSMONITOR_MODE_DISABLED) {
>>   			advise(_("core.fsmonitor is unset; "
>>   				 "set it if you really want to "
> 
> Can w assert somewhere earlier that ->mode can't be
> FSMONITOR_MODE_INCOMPATIBLE at the same time that ->reason ==
> FSMONITOR_REASON_OK, should that ever happen?
> 
> Then we can get rid of the "???" case here.
> 

Yeah, it would be nice to assert() the pair and simplify things.  I'll
make a note to look at that.


> The "%s '%s'" here should really be marked for translation, but just
> "some reason '$path'" is a pretty confusing message. This will emit
> e.g.:

I already have translations in the code that looks up the message,
so doing it here for a pair of %s's felt wrong.

> 
>      "bare repos are incompatible with fsmonitor '/some/path/to/repo'"
> 
> Since we always hand these to error maybe have the helper do e.g.:
> 
>      error(_("bare repository '%s' is incompatible with fsmonitor"), path);
> 

I'm wondering now if we should just drop the path from the message
and use the error message from __get_reason_msg() as is.  (It was
useful during debugging, but I could see it going away.)


> I find the second-guessing in fsmonitor-settings.c really hard to
> follow, i.e. how seemingly every function has some "not loaded yet? load
> it" instead of a more typical "init it", "use it", "free it"
> pattern. Including stuff like this:
> 	
> 	enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
> 	{
> 	        if (!r)
> 	                r = the_repository;
> 
> But anyway, seeing as we do try really hard to load the_repository (or a
> repository) can't we use the_repository->gitdir etc. here instead of
> xgetcwd(), or the_repository->worktree when non-bare?
> 

I'll take a look and see if I can simplify it.

Thanks
Jeff
diff mbox series

Patch

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 97ca2a356e5..00eaffbb945 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1443,6 +1443,15 @@  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) {
+		const char *msg = fsm_settings__get_reason_msg(the_repository);
+
+		return error("%s '%s'", msg ? msg : "???", xgetcwd());
+	}
+
 	if (!strcmp(subcmd, "start"))
 		return !!try_to_start_background_daemon();
 
diff --git a/builtin/update-index.c b/builtin/update-index.c
index d335f1ac72a..8f460e7195f 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1237,6 +1237,13 @@  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) {
+			const char *msg = fsm_settings__get_reason_msg(r);
+
+			return error("%s '%s'", msg ? msg : "???", xgetcwd());
+		}
+
 		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 3b54e7a51f6..8410cc73404 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;
@@ -87,6 +111,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);
 }
@@ -98,6 +125,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);
@@ -111,5 +141,32 @@  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_OK;
 	FREE_AND_NULL(r->settings.fsmonitor->hook_path);
 }
+
+enum fsmonitor_reason fsm_settings__get_reason(struct repository *r)
+{
+	if (!r)
+		r = the_repository;
+
+	lookup_fsmonitor_settings(r);
+
+	return r->settings.fsmonitor->reason;
+}
+
+const char *fsm_settings__get_reason_msg(struct repository *r)
+{
+	enum fsmonitor_reason reason = fsm_settings__get_reason(r);
+
+	switch (reason) {
+	case FSMONITOR_REASON_OK:
+		return NULL;
+
+	case FSMONITOR_REASON_BARE:
+		return _("bare repos are incompatible with fsmonitor");
+	}
+
+	BUG("Unhandled case in fsm_settings__get_reason_msg '%d'",
+	    reason);
+}
diff --git a/fsmonitor-settings.h b/fsmonitor-settings.h
index a4c5d7b4889..a1af058a287 100644
--- a/fsmonitor-settings.h
+++ b/fsmonitor-settings.h
@@ -4,11 +4,20 @@ 
 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_OK = 0, /* no incompatibility or when disbled */
+	FSMONITOR_REASON_BARE,
+};
+
 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);
@@ -16,6 +25,9 @@  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);
+const char *fsm_settings__get_reason_msg(struct repository *r);
+
 struct fsmonitor_settings;
 
 #endif /* FSMONITOR_SETTINGS_H */
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index a6308acf006..cf258d88f04 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -55,6 +55,29 @@  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 &&
+
+	test_must_fail \
+		git -C ./bare-clone -c core.fsmonitor=foo \
+			update-index --fsmonitor 2>actual &&
+	grep "bare repos are incompatible with fsmonitor" actual &&
+
+	test_must_fail \
+		git -C ./bare-clone -c core.fsmonitor=true \
+			update-index --fsmonitor 2>actual &&
+	grep "bare repos are incompatible with fsmonitor" 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 &&