RE: [PATCH v12 4/6] fsmonitor: deal with synthetic firmlinks on macOS
Show 54 quoted lines
> -----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).
> Show 15 quoted lines
> > +
> > + 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?
>
Show 18 quoted lines
> > + 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.
Show 9 quoted lines
> > + 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
Eric DeCosta <edecosta@mathworks.com>