Junio C Hamano <gitster@xxxxxxxxx> writes:
> If you want to do lstat(2) yourself, the canonical way to check its
> success is to see the returned value is 0, not "not negative", but
> let's first see how dir_iterator_begin() can fail.
Ah, thanks.
> Unfortunately, if lstat(2) failed
> with ENOTDIR (e.g. dir_iterator_begin() gets called with a path
> whose leading component is not a directory), the caller will also
> see ENOTDIR, but the distinction may not matter in practice. I
> haven't thought things through.
>
> ...
>
> if (!iter) {
> if (errno == ENOTDIR)
> die(_("'%s' is not a directory, refusing to clone with --local"),
> src->buf);
> else
> die_errno(_("failed to stat '%s'"), src->buf);
> }
>
> may be sufficient. But because this is an error codepath, it is not
> worth optimizing what happens there, and an extra lstat(2) is not
> too bad, if the code gains extra clarity.
Yeah, the considerations here make sense to me.
Since this is an error code path, I think the extra lstat() is probably
worth it since it lets us be really specific about the error. Maybe:
if (!iter) {
struct stat st;
if (errno == ENOTDIR && lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))
die(_("'%s' is a symlink, refusing to clone with --local"),
src->buf);
die_errno(_("failed to start iterator over '%s'"), src->buf);
}
Doing the extra lstat() only makes sense if we saw ENOTDIR orignally
anyway.
Alternatively, since we do care about the distinction between ENOTDIR
from lstat vs ENOTDIR because dir_iterator_begin() saw a symlink, maybe
it's better to refactor dir_iterator_begin() so that we stop
piggybacking on "errno = ENOTDIR" in these two cases. I didn't want to
do this because I wasn't sure who might be relying on this behavior, but
this check was pretty recently introduced anyway (bffc762f87
(dir-iterator: prevent top-level symlinks without FOLLOW_SYMLINKS,
2023-01-24), so maybe nobody needs it.
