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

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

On Tue, Oct 04 2022, Eric DeCosta via GitGitGadget wrote:

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

> +	struct strbuf alias;

Rather than init-ing here...
> +	struct strbuf points_to = STRBUF_INIT;
> +
> +	dir = opendir(root);
> +	if (!dir)
> +		return error_errno(_("opendir('%s') failed"), root);
> +
> +	strbuf_init(&alias, 256);

...we pre-grow here...

> +	while ((de = readdir(dir)) != NULL) {

...but as shown here, we may not even use this at all, but even then is
this micro-optimization worth it? If it is a reader would be helped with
an explanation of what the 256 is, is this meant to be some OSX-specific
PATH_MAX, but hardcoded?

> +		strbuf_reset(&alias);
> +		strbuf_addf(&alias, "%s%s", root, de->d_name);
> +
> +		if (lstat(alias.buf, &st) < 0) {
> +			error_errno(_("lstat('%s') failed"), alias.buf);
> +			goto done;
> +		}
> +
> +		if (!S_ISLNK(st.st_mode))
> +			continue;
> +
> +		if (strbuf_readlink(&points_to, alias.buf, st.st_size) < 0) {
> +			error_errno(_("strbuf_readlink('%s') failed"), alias.buf);
> +			goto done;
> +		}
> +

Maybe this code would be simpler if you split it into a trivial static
function, so you could pass in the "alias" and "points_to", and just do
"return error...(...)" here and in the other places.

> +		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);

Earlier you use strbuf_addf() for a "append two strings", maybe we could
save ourselves a line and do the same here...

> +			trace_printf_key(&trace_fsmonitor,
> +				"Found alias for '%s' : '%s' -> '%s'",
> +				path, info->alias.buf, info->points_to.buf);

...except we're only doing this to emit this, and then we'll free() it?
Can't we just use %s%s here instead of %s, and e.g. pass
"info->alias.buf, alias" instead of the now-appende-to
"info->alias.buf"?

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

I commented on this in a few other places, and I'll stop noting these
now, but you're mis-indenting function decls consistently, also in a *.h
change later in this commit.

Please look through those for this series.

> +{
> +	if (!info->alias.len)
> +		return NULL;

Maybe "check if we have a zero-length string" should belong in the
caller, as "resolve it as an alias" for "\0" is nonsense?

> +
> +	if ((!strncmp(info->alias.buf, path, info->alias.len))
> +		&& path[info->alias.len] == '/') {
> +		struct strbuf tmp = STRBUF_INIT;
> +		const char *remainder = path + info->alias.len;
> +
> +		strbuf_addbuf(&tmp, &info->points_to);
> +		strbuf_add(&tmp, remainder, strlen(remainder));

There's no point in strbuf_add() if you have to dynamically use strlen()
over just using strbuf_addstr() (which inline resolves to the same),
let's just use that.

Or just a single strbuf_addf()...

> +		return strbuf_detach(&tmp, NULL);

...Or actually, these last 3 lines can be replaced by a mere:

	return xstrfmt("%s%s", info->points_to.buf, remainder);

Please just use that. It's not exactly the same due to the pre-sizing
with "addbuf", but again (I commented on a similar case in an earlier
commit), is such micro-optimization worth it here over brevity?

> +/*
> + * No-op for now.
> + */

Please just drop this comment, it doesn't add any information. It would
be useful to say why we're seemingly faking "no aliase on win32", or to
say that it does have them, but implementing them is a "TODO".

But we can see it's a "no-op for now" from the code....

> +int fsmonitor__get_alias(const char *path, struct alias_info *info)
> +{
> +	return 0;
> +}
> +
> +/*
> + * No-op for now.
> + */

...ditto.

> +/*
> + * Get the alias in given path, if any.
> + *
> + * Sets alias to the first alias that matches any part of the path.
> + *
> + * If an alias is found, info.alias and info.points_to are set to the
> + * found mapping.
> + *
> + * Returns -1 on error, 0 otherwise.
> + *
> + * The caller owns the storage that is occupied by info.alias and
> + * info.points_to and is responsible for releasing it.
> + */
> +int fsmonitor__get_alias(const char *path, struct alias_info *info);

I have not looked carefully at this interface, but instead of all of
this explanation & the caller needing to carefully reason about what
parts of this struct it can and can't peek into couldn't we just make it
take two "char **" arguments, one for "alias" and another for
"points_to".

It would then be obvious what the semantics are, and who owns the
memory.

But maybe retaining the strbuf-ness is important (but even then, we
could pass a "struct strbuf *" to populate.
> +
> +/*
> + * Resolve the path against the given alias.
> + *
> + * Returns the resolved path if there is one, NULL otherwise.
> + *
> + * The caller owns the storage that the returned string occupies and
> + * is responsible for releasing it.
> + */
> +char *fsmonitor__resolve_alias(const char *path,
> +	const struct alias_info *info);
> +

Here we don't say anything about the ownership & freeing of "info", does
the same apply? But the API design comment above also applies.