Thanks for the feedback!
(and phew, I was a few minutes away from submitting v2 :P)
Taylor Blau <me@xxxxxxxxxxxx> writes:
> 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.
>> 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.
> 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.
