Re: [PATCH v12 4/6] fsmonitor: deal with synthetic firmlinks on macOS
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Sep 26, 2022, 15:27 UTC
- Message-ID
- <220926.86r0zyywp1.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <6efdc6ed74ec9224d93a1b88ff8be85d533cb30f.1664048782.git.gitgitgadget@gmail.com>
On Sat, Sep 24 2022, Eric DeCosta via GitGitGadget wrote:
> From: Eric DeCosta <edecosta@mathworks.com>
..one aspect I missed...
Show 5 quoted lines
> + 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...
Show 5 quoted lines
> +int fsmonitor__get_alias(const char *path, struct alias_info *info)
> +{
> + DIR * dir;
> + int read;
> + int retval;...we could just...
Show 5 quoted lines
> + 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)
Show 10 quoted lines
> +
> + 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...
Show 13 quoted lines
> + 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().
Show 5 quoted lines
> + */ > +char *fsmonitor__resolve_alias(const char *path, > + const struct alias_info *info); > + > +
nit: extra whitespace at end of file.
> #endif