Re: [PATCH] clone: error specifically with --local and symlinked objects

From: Glen Choo <chooglen@google.com>
Date: Apr 6, 2023, 21:55 UTC
Message-ID: <kl6lmt3k3ccc.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <ZC87IcLcBnxBRCdr@nand.local>

Thanks for the feedback!

(and phew, I was a few minutes away from submitting v2 :P)

Taylor Blau <me@ttaylorr.com> writes:

Show 17 quoted lines

> We *could* teach the dir-iterator API to return a specialized error code either
> through a pointer, like:
>
>     struct dir_iterator *dir_iterator_begin(const char *path,
>                                             unsigned int flags, int *error)
>
> and set error to something like -DIR_ITERATOR_IS_SYMLINK when error is
> non-NULL.
>
> Or we could do something like this:
>
>     int dir_iterator_begin(struct dir_iterator **it,
>                            const char *path, unsigned int flags)
>
> and have the `dir_iterator_begin()` function return its specialized
> error and initialize the dir iterator through a pointer. Between these
> two, I prefer the latter, but I think it's up to individual taste.

Yeah, I've thought about doing the latter. That's also what I'd prefer. Since you've brought it up, I'll give that a try.

Show 8 quoted lines

>> 	if (!iter) {
>> 		struct stat st;
>>
>>     if (errno == ENOTDIR && lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))
>
> Couple of nit-picks here. Instead of writing "lstat(src->buf, &st ==
> 0)", we should write "!lstat(src->buf, &st)" to match
> Documentation/CodingGuidelines.

Ah, thanks.

Show 17 quoted lines

> But note that that call to lstat() will clobber your errno value, so
> you'd want to save it off beforehand. If you end up going this route,
> I'd probably do something like:
>
>     if (!iter) {
>       int saved_errno = errno;
>       if (errno == ENOTDIR) {
>         struct stat st;
>
>         if (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))
>           die(_("'%s' is a symlink, refusing to clone with `--local`"),
>               src->buf);
>       }
>       errno = saved_errno;
>
>       die_errno(_("failed to start iterator over '%s'"), src->buf);
>     }

Ah thanks, yes. I think it isn't different in practice, since we're lstat()-ing the same path, but we should always use the "real" errno to be safe.

This is another good reason to return an error code instead of overloading errno.