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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Wed, 28 Sep 2022 07:55:56 +0200

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

> From: Eric DeCosta <[email-masked]>
>

> +	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.

> +
> +	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?

> +
> +		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.