On Sat, Sep 24 2022, Eric DeCosta via GitGitGadget wrote:
> From: Eric DeCosta <edecosta@xxxxxxxxxxxxx>
..one aspect I missed...
> + 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...
> +int fsmonitor__get_alias(const char *path, struct alias_info *info)
> +{
> + DIR * dir;
> + int read;
> + int retval;
...we could just...
> + 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)
> +
> + 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...
> + 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().
> + */
> +char *fsmonitor__resolve_alias(const char *path,
> + const struct alias_info *info);
> +
> +
nit: extra whitespace at end of file.
> #endif
