diff mbox series

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

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

Commit Message

Jeff Hostetler March 24, 2022, 4:50 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 |  6 ++++
 builtin/update-index.c      |  4 +++
 fsmonitor-settings.c        | 60 +++++++++++++++++++++++++++++++++++++
 fsmonitor-settings.h        | 12 ++++++++
 t/t7519-status-fsmonitor.sh | 23 ++++++++++++++
 5 files changed, 105 insertions(+)

Comments

Ævar Arnfjörð Bjarmason April 19, 2022, 9:44 a.m. UTC | #1 
On Thu, Mar 24 2022, Jeff Hostetler via GitGitGadget wrote:

> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
> index 46be55a4618..50ae3cca575 100644
> --- a/builtin/fsmonitor--daemon.c
> +++ b/builtin/fsmonitor--daemon.c
> @@ -1449,6 +1449,12 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
>  		die(_("invalid 'ipc-threads' value (%d)"),
>  		    fsmonitor__ipc_threads);

I think that structurally the way things are done in
fsmonitor-settings.c make its use really hard to follow. E.g. here:

> +	prepare_repo_settings(the_repository);

We prep the repo, OK.

> +	fsm_settings__set_ipc(the_repository);

Set IPC.

> +	if (fsm_settings__error_if_incompatible(the_repository))

And here we'll error out if we're incompatible, and this is in the
top-level cmd_fsmonitor__daemon() function. All OK, except why didn't we
check this before "set IPC?".

Anyway, re-arranging some of the diff below:

> @@ -86,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);
>  }

Here in fsm_settings__set_ipc we return with a NOOP if we're not
compatible.

Then:

> +int fsm_settings__error_if_incompatible(struct repository *r)
> +{
> +	enum fsmonitor_reason reason = fsm_settings__get_reason(r);
> +
> +	switch (reason) {
> +	case FSMONITOR_REASON_OK:
> +		return 0;
> +
> +	case FSMONITOR_REASON_BARE:
> +		error(_("bare repository '%s' is incompatible with fsmonitor"),
> +		      xgetcwd());
> +		return 1;
> +	}
> +
> +	BUG("Unhandled case in fsm_settings__error_if_incompatible: '%d'",
> +	    reason);
> +}

Here we'll call fsm_settings__get_reason() which does the same.

> +enum fsmonitor_reason fsm_settings__get_reason(struct repository *r)
> +{
> +	if (!r)
> +		r = the_repository;
> +
> +	lookup_fsmonitor_settings(r);
> +
> +	return r->settings.fsmonitor->reason;
> +}

Is there a reason we can't skip this indirection when using the API like
this and e.g. do:

	enum fsmonitor_reason reason;
	prepare_repo_settings(the_repository);
	reason = fsmonitor_check_for_incompatible(the_repository)
        if (reason != FSMONITOR_REASON_OK)
        	die("%s", fsm_settings__get_reason_string(reason));

There's just two callers of this API in "seen", and neither need/want
this pattern where every method needs to lazy load itself, or the
indirection where fsmonitor-settings.c needs to be used as a
clearing-house for state management.

Maybe I'm missing something, but why not make check_for_incompatible()
non-static and have the callers use that (and then it would return
"fsmonitor_reason", not "int", the int return value being redundant to
the enum)>

> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 876112abb21..d29048f16f2 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1237,6 +1237,10 @@ 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_settings__error_if_incompatible(the_repository))
> +			return 1;
> +
>  		if (fsm_mode == FSMONITOR_MODE_DISABLED) {
>  			warning(_("core.fsmonitor is unset; "
>  				"set it if you really want to "

This looks like a bug, we knew before aquiring the lockfile that we
weren't compatible, so why wait until here to error out? This seems to
skip the rollback_lock_file(), so won't we leave a stale lock?
Jeff Hostetler April 22, 2022, 2:47 p.m. UTC | #2 
On 4/19/22 5:44 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Mar 24 2022, Jeff Hostetler via GitGitGadget wrote:
> 
>> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
>> index 46be55a4618..50ae3cca575 100644
>> --- a/builtin/fsmonitor--daemon.c
>> +++ b/builtin/fsmonitor--daemon.c
>> @@ -1449,6 +1449,12 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
>>   		die(_("invalid 'ipc-threads' value (%d)"),
>>   		    fsmonitor__ipc_threads);
> 
> I think that structurally the way things are done in
> fsmonitor-settings.c make its use really hard to follow. E.g. here:
> 
>> +	prepare_repo_settings(the_repository);
> 
> We prep the repo, OK.
> 
>> +	fsm_settings__set_ipc(the_repository);
> 
> Set IPC.
> 
>> +	if (fsm_settings__error_if_incompatible(the_repository))
> 
> And here we'll error out if we're incompatible, and this is in the
> top-level cmd_fsmonitor__daemon() function. All OK, except why didn't we
> check this before "set IPC?".
> 
> Anyway, re-arranging some of the diff below:
> 
>> @@ -86,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);
>>   }
> 
> Here in fsm_settings__set_ipc we return with a NOOP if we're not
> compatible.
> 
> Then:
> 
>> +int fsm_settings__error_if_incompatible(struct repository *r)
>> +{
>> +	enum fsmonitor_reason reason = fsm_settings__get_reason(r);
>> +
>> +	switch (reason) {
>> +	case FSMONITOR_REASON_OK:
>> +		return 0;
>> +
>> +	case FSMONITOR_REASON_BARE:
>> +		error(_("bare repository '%s' is incompatible with fsmonitor"),
>> +		      xgetcwd());
>> +		return 1;
>> +	}
>> +
>> +	BUG("Unhandled case in fsm_settings__error_if_incompatible: '%d'",
>> +	    reason);
>> +}
> 
> Here we'll call fsm_settings__get_reason() which does the same.
> 
>> +enum fsmonitor_reason fsm_settings__get_reason(struct repository *r)
>> +{
>> +	if (!r)
>> +		r = the_repository;
>> +
>> +	lookup_fsmonitor_settings(r);
>> +
>> +	return r->settings.fsmonitor->reason;
>> +}
> 
> Is there a reason we can't skip this indirection when using the API like
> this and e.g. do:
> 
> 	enum fsmonitor_reason reason;
> 	prepare_repo_settings(the_repository);
> 	reason = fsmonitor_check_for_incompatible(the_repository)
>          if (reason != FSMONITOR_REASON_OK)
>          	die("%s", fsm_settings__get_reason_string(reason));
> 
> There's just two callers of this API in "seen", and neither need/want
> this pattern where every method needs to lazy load itself, or the
> indirection where fsmonitor-settings.c needs to be used as a
> clearing-house for state management.
> 
> Maybe I'm missing something, but why not make check_for_incompatible()
> non-static and have the callers use that (and then it would return
> "fsmonitor_reason", not "int", the int return value being redundant to
> the enum)>

I suppose we could rearrange things to hide less of the
state management.  I'm not sure it matters one way or the
other, but I'll give it a try and see if simplifies things.

> 
>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index 876112abb21..d29048f16f2 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -1237,6 +1237,10 @@ 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_settings__error_if_incompatible(the_repository))
>> +			return 1;
>> +
>>   		if (fsm_mode == FSMONITOR_MODE_DISABLED) {
>>   			warning(_("core.fsmonitor is unset; "
>>   				"set it if you really want to "
> 
> This looks like a bug, we knew before aquiring the lockfile that we
> weren't compatible, so why wait until here to error out? This seems to
> skip the rollback_lock_file(), so won't we leave a stale lock?
> 

Yes, good catch.  The `return` here will bypass the rollback.
Hopefully, the above rearrangement will make this go away.

I do have to wonder about the rest of this function.  There are
several `die()`, `exit()`, `usage()`, `BUG()`, and other `return`
statements after the index lock is taken that won't hit the
rollback.  Should those be investigated too?  (I can't do that
now in the context of this series, though.)

Jeff
diff mbox series

Patch

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 46be55a4618..50ae3cca575 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1449,6 +1449,12 @@  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__error_if_incompatible(the_repository))
+		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 876112abb21..d29048f16f2 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1237,6 +1237,10 @@  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_settings__error_if_incompatible(the_repository))
+			return 1;
+
 		if (fsm_mode == FSMONITOR_MODE_DISABLED) {
 			warning(_("core.fsmonitor is unset; "
 				"set it if you really want to "
diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index 757d230d538..86c09bd35fe 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;
@@ -23,6 +47,7 @@  static void lookup_fsmonitor_settings(struct repository *r)
 
 	CALLOC_ARRAY(s, 1);
 	s->mode = FSMONITOR_MODE_DISABLED;
+	s->reason = FSMONITOR_REASON_OK;
 
 	r->settings.fsmonitor = s;
 
@@ -86,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);
 }
@@ -97,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);
@@ -110,5 +141,34 @@  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;
+}
+
+int fsm_settings__error_if_incompatible(struct repository *r)
+{
+	enum fsmonitor_reason reason = fsm_settings__get_reason(r);
+
+	switch (reason) {
+	case FSMONITOR_REASON_OK:
+		return 0;
+
+	case FSMONITOR_REASON_BARE:
+		error(_("bare repository '%s' is incompatible with fsmonitor"),
+		      xgetcwd());
+		return 1;
+	}
+
+	BUG("Unhandled case in fsm_settings__error_if_incompatible: '%d'",
+	    reason);
+}
diff --git a/fsmonitor-settings.h b/fsmonitor-settings.h
index a4c5d7b4889..8654edf33d8 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);
+int fsm_settings__error_if_incompatible(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..9a8e21c5608 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 repository .* is incompatible with fsmonitor" actual &&
+
+	test_must_fail \
+		git -C ./bare-clone -c core.fsmonitor=true \
+			update-index --fsmonitor 2>actual &&
+	grep "bare repository .* is 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 repository .* is incompatible with fsmonitor" actual
+'
+
 test_expect_success 'setup' '
 	mkdir -p .git/hooks &&
 	: >tracked &&