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>
> [...]
> +	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;
> +	}

As we can see here this is in the one-off setup code...

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

...more of a code clarity comment than anything, else, but...

> +
> +	retval = 0;
> +	dir = opendir("/");
> +	if (!dir)
> +		return -1;
> +
> +	strbuf_init(&alias, 256);
> +	strbuf_init(&points_to, MAXPATHLEN);


...can't we just use the STRBUF_INIT macro here instead? most paths are
nowhere near MAXPATHLEN, but more importantly we try to avoid these
sorts of memory micro-managements except for hot codepaths.

In this case it's just the one-off setup of fsmonitor, isn't it? So just
using the default allocation pattern seems worthwhile, and will save
e.g. anyone grepping for MAXPATHLEN looking for bugs (the MAXPATHLEN is
sometimes not the actual maximum pathlen).

> +
> +	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);
> +		if (read > 0) {
> +			strbuf_setlen(&points_to, read);
> +			if ((strncmp(points_to.buf, path, points_to.len) == 0)

We usually do (!strcmp()), not strcmp() == 0, ditto strncmp. See
CodingGuidelines.
> +	done:

Nit: labels shouldn't be indented.

> +	closedir(dir);

We checked the opendir() return value, why not closedir() too?

> +	if ((strncmp(info->alias, path, len) == 0)

ditto !foo() v.s. foo() == 0.

> +		&& path[len] == '/') {
> +		struct strbuf tmp;
> +		const char *remainder = path + len;
> +		int ptr_len = strlen(info->points_to);
> +		int rem_len = strlen(remainder);

Make these s/int/size_t/.

> +
> +		strbuf_init(&tmp, ptr_len + rem_len);

And use st_add() here instead of " + ". I don't think it'll overflow,
but it's good to guard overflows out of habit...

> +		strbuf_add(&tmp, info->points_to, ptr_len);

Earlier you constructed a strbuf, and then strbuf_detached() it into
this new "struct alias_info" you made. And now we're having to strlen()
that to get the lenght that we knew earlier?

Can't we just make the member a "struct strbuf" instead? Maybe not, I
have not reviewed that aspect carefully...