Re: [PATCH v4 2/4] fsmonitor: generate unique Unix socket file name in the desired location

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 31, 2022, 19:49 UTC
Message-ID: <220831.86czcg89wx.gmgdl@evledraar.gmail.com>
In-Reply-To: <2cb026a631704b004b06e4a944c79a434df08440.1661962145.git.gitgitgadget@gmail.com>

On Wed, Aug 31 2022, Eric DeCosta via GitGitGadget wrote:

Show 7 quoted lines

> From: Eric DeCosta <edecosta@mathworks.com>
>
> Based on the values of fsmonitor.allowRemote and fsmonitor.socketDir
> locate the Unix domain socket file in the desired location (either
> the .git directory, $HOME, or fsmonitor.socketDir). If the location
> is other than the .git directory, generate a unique file name based
> on the SHA1 has of the path to the .git directory.

Per:

	fsmonitor-ipc.h- * Returns the pathname to the IPC named pipe or Unix domain socket
	fsmonitor-ipc.h- * where a `git-fsmonitor--daemon` process will listen.  This is a
	fsmonitor-ipc.h- * per-worktree value.

Show 11 quoted lines

> +	git_dir = get_git_dir();
> +	sock_dir = fsm_settings__get_socket_dir(the_repository);
> +
> +	SHA1_Init(&sha1ctx);
> +	SHA1_Update(&sha1ctx, git_dir, strlen(git_dir));
> +	SHA1_Final(hash, &sha1ctx);
> +
> +	if (sock_dir && *sock_dir)
> +		strbuf_addf(&ipc_file, "%s/.git-fsmonitor-%s",
> +					sock_dir, hash_to_hex(hash));
> +	else

But here we (from eyeballing this, maybe I've missed something):

 1. Get the path to the git dir
 2. SHA-1 hash that path, presumably to make it fixed size & get rid of
    slashes etc.
 3. Make that the IPC filename

Per the "per worktree" can't we just check if:

 * We have a .git/worktree/? If so derive the name from that.
 * We don't? Then we just have one? Stick it in in there? I.e. isn't the
   hash_to_hex() here redundant?

...

> +		strbuf_addf(&ipc_file, "~/.git-fsmonitor-%s", hash_to_hex(hash));

... but not here, so is it just for this "else", but got carried over above?

I think if we're creating a new global cookie file, and presumably potentially a *lot* of them in the user's ~ we should at least prominently document that somewhere.

But more generally couldn't this?:

 * Play nice with $HOME/.config/git/ etc, as is the usual convention these days on *nix
 * We would have to the equivalent of a "mkdir -p", but if we stick it
   in .config/git/fsmonitor-sockets or something we could have a nested
   path there that mirrors the path to the repo.

The latter can really help with debugging, you have this random .git-fsmonitor-XXXXX file, where the XXXX is totally mysterious, until you eventually find this code & discover it's a SHA-1 hash of some other path...