Re: [PATCH v13 4/6] fsmonitor: deal with synthetic firmlinks on macOS

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 28, 2022, 05:55 UTC
Message-ID: <220928.867d1oxckc.gmgdl@evledraar.gmail.com>
In-Reply-To: <324eb5acd856f6a8122a5239f9fa1a9a8ecc9ee3.1664312254.git.gitgitgadget@gmail.com>

On Tue, Sep 27 2022, Eric DeCosta via GitGitGadget wrote:

> From: Eric DeCosta <edecosta@mathworks.com>
>

> +	if (fsmonitor__get_alias(state.path_worktree_watch.buf, &state.alias)) {
> +		err = error(_("could not get worktree alias"));

We could keep this, but if we'd error() in the function itself we'd just emit two error() lines when one would do.

> +	dir = opendir("/");

We should have a:

	const char *const dir = "/";

So we can pass it to this..

> +	if (!dir) {
> +		error_errno("opendir('/') failed");

...and as a parameter to a _()-translated string. That way the next translation doesn't need to translate opendir('/home') or whatever.

> +		return -1;

Just skip the braces and do "return error_errno()", these functions return -1 for thath reason.

> +	strbuf_init(&alias, 256);
>
> +	/* no way of knowing what the link will resolve to, so MAXPATHLEN */
> +	strbuf_init(&points_to, MAXPATHLEN);

Still need manual memory juggling? Ok.

Show 5 quoted lines

> +
> +	while ((de = readdir(dir)) != NULL) {
> +		strbuf_reset(&alias);
> +		strbuf_addch(&alias, '/');
> +		strbuf_add(&alias, de->d_name, strlen(de->d_name));

	strbuf_addf(&alias, "/%s", de->d_name);

Or rather:

	strbuf_addf(&alias, "%s%s", root_dir, de->d_name);

If you split that "/" into a variable?

Show 26 quoted lines

> +
> +		read = readlink(alias.buf, points_to.buf, MAXPATHLEN);
> +		if (read > 0) {
> +			strbuf_setlen(&points_to, read);
> +			if ((!strncmp(points_to.buf, path, points_to.len))
> +				&& path[points_to.len] == '/') {
> +				strbuf_addbuf(&info->alias, &alias);
> +				strbuf_addbuf(&info->points_to, &points_to);
> +				trace_printf_key(&trace_fsmonitor,
> +					"Found alias for '%s' : '%s' -> '%s'",
> +					path, info->alias.buf, info->points_to.buf);
> +				retval = 0;
> +				goto done;
> +			}
> +		} else if (!read) {
> +			BUG("readlink returned 0");
> +		} else if (errno != EINVAL) { /* Something other than not a link */
> +			error_errno("readlink('%s') failed", alias.buf);
> +			goto done;
> +		}
> +	}
> +	retval = 0; /* no alias */
> +
> +done:
> +	if (closedir(dir) < 0)
> +		warning_errno("closedir('/') failed");

Why not return an error for this? If you can't close the dir that usually means the write or similar didn't work.