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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Sep 24 2022, Eric DeCosta via GitGitGadget wrote:

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

..one aspect I missed...

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

Okey, it errored and we call error() to say it didn't work, good so far,
but...

> +int fsmonitor__get_alias(const char *path, struct alias_info *info)
> +{
> +	DIR * dir;
> +	int read;
> +	int retval;

...we could just...

> +	struct dirent *de;
> +	struct strbuf alias;
> +	struct strbuf points_to;
> +
> +	retval = 0;

...have initialized that above if we do it unconditionally, but more on
this below...

> +	dir = opendir("/");
> +	if (!dir)
> +		return -1;

Here in the actual implementation, which looking at the end-state we
*only* end up calling from that one caller we could have called
error_errno() to get a better message, but didn't.

I think much better would be to skip that above entirely, or keep it you
want two errors, but then just have the more meaningful error_errno()
here, where we're closer to the error, and can report a better one.

Of course we might sometimes have a good error, and sometimes a bad one,
but...(continued below)

> +
> +	strbuf_init(&alias, 256);
> +	strbuf_init(&points_to, MAXPATHLEN);
> +
> +	while ((de = readdir(dir)) != NULL) {
> +		strbuf_reset(&alias);
> +		strbuf_addch(&alias, '/');
> +		strbuf_add(&alias, de->d_name, strlen(de->d_name));
> +
> +		read = readlink(alias.buf, points_to.buf, MAXPATHLEN);

I think a:

	if (!read)
		BUG("got 0 from readlink?");

Or something would be a good paranoia addition, as you're technically
relying on...

> +		if (read > 0) {
> +			strbuf_setlen(&points_to, read);
> +			if ((strncmp(points_to.buf, path, points_to.len) == 0)
> +				&& path[points_to.len] == '/') {
> +				info->alias = strbuf_detach(&alias, NULL);
> +				info->points_to = strbuf_detach(&points_to, NULL);
> +				trace_printf_key(&trace_fsmonitor,
> +					"Found alias for '%s' : '%s' -> '%s'",
> +					path, info->alias, info->points_to);
> +				retval = 0;
> +				goto done;
> +			}
> +		} else if (errno != EINVAL) { /* Something other than not a link */

...the possibility that we return 0 but a stale errno happens to be set,
I don't think it'll happen in practice and that it always returned -1 if
we get here, but being strict with calling syscalls is generally good.

> +			trace_printf_key(&trace_fsmonitor, "Error %s", strerror(errno));

(continued from above)..here we see the only codepath that sets retval
!= 0,

> +			retval = -1;

Here we could have just called error_errno() instead.

> + * The caller owns the storage that the returned string occupies and
> + * is responsible for releasing it with `free(3)` when done.

nit: we could just put a full stop after "it" and skip the
rest. I.e. trust that the reader knows that allocated memory is freed
with free().

> + */
> +char *fsmonitor__resolve_alias(const char *path,
> +	const struct alias_info *info);
> +
> +

nit: extra whitespace at end of file.
>  #endif