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

[Eric DeCosta <edecosta@mathworks.com>]

> -----Original Message-----
> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Sent: Monday, September 26, 2022 11:16 AM
> To: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>
> Cc: git@vger.kernel.org; Jeff Hostetler <git@jeffhostetler.com>; Eric Sunshine
> <sunshine@sunshineco.com>; Torsten Bögershausen <tboegi@web.de>;
> Ramsay Jones <ramsay@ramsayjones.plus.com>; Johannes Schindelin
> <Johannes.Schindelin@gmx.de>; Eric DeCosta <edecosta@mathworks.com>
> Subject: Re: [PATCH v12 4/6] fsmonitor: deal with synthetic firmlinks on
> macOS
> 
> 
> 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).
> 
OK, makes sense.

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

> Nit: labels shouldn't be indented.
> 
Fixed

> > +	closedir(dir);
> 
> We checked the opendir() return value, why not closedir() too?
> 
OK, will do.

> > +	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...
> 
Sure, just strbuf_add(). Then I don't need ptr_len or rem_len either.

> > +		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...
>
Certainly could do that.

-Eric