On Fri, Oct 14 2022, Eric DeCosta via GitGitGadget wrote:
> From: Eric DeCosta <edecosta@xxxxxxxxxxxxx>
> [..]
> +const char *fsmonitor_ipc__get_path(struct repository *r)
> +{
> + static const char *ipc_path = NULL;
Urm, I see some of this interface made it into master already with
6beb2688d33 (fsmonitor: relocate socket file if .git directory is
remote, 2022-10-04), but this is just weird. We already have "struct
fsmonitor_settings" which "lazily loads" these various settings.
So e.g. in refresh_fsmonitor() we might get the hook path, which we
"lazy load" and cache there.
And here we have an extra layer of another type of lazy loading, and
this will e.g. be called from fsmonitor-settings.c.
Is there really a good reason for why the relevant codepaths can't
either not-lazy-load this (e.g. when we get the socket), or why we can't
just use fsmonitor-settings.c's existing caching?
Just wondering...
> + repo_config_get_string(r, "fsmonitor.socketdir", &sock_dir);
> +
> + /* Create the socket file in either socketDir or $HOME */
> + if (sock_dir && *sock_dir) {
> + strbuf_addf(&ipc_file, "%s/.git-fsmonitor-%s",
> + sock_dir, hash_to_hex(hash));
> + } else {
> + strbuf_addf(&ipc_file, "~/.git-fsmonitor-%s", hash_to_hex(hash));
> + }
> + free(sock_dir);
Instead of this init sock_dir to NULL & then check if it's non-NULL
etc. Just do:
char *sock_dir;
if (!repo_config_get_string_tmp(r, "fsmonitor.socketdir", &sock_dir) &&
*sock_dir)
...
else
...
Note also the *_tmp(), then you won't have to free() it.
> +
> + ipc_path = interpolate_path(ipc_file.buf, 1);
> + if (!ipc_path)
> + die(_("Invalid path: %s"), ipc_file.buf);
> +
> + strbuf_release(&ipc_file);
> + return ipc_path;
Why can't we just do something more simpler like this in the *nix case?
I think because you're creating this socket for a NFS-mounted repo, so
of course we might have multiple clients.
But you also seem to be making it "unique" by hashing r->worktree, which
is the path to the repo work tree. At least in my use-cases for NFS
mounted repos (which have been limited) all of those paths would be the
same for me across my systems, because I'd have them at
/home/avar/g/some-git-tree/...
In builtin/gc.c we create a "unique for system" lock file using
xgethostname(), isn't that a lot simpler & closer to what you actually
want?
In any case, some details about motivations, why etc. are sorely missing
in the commit message. We can see *what* you're doing, but not *why*, or
what other simpler solutions etc. might have been tried & dismissed...
