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

From: Glen Choo <chooglen@google.com>
Date: Apr 5, 2023, 16:48 UTC
Message-ID: <kl6ly1n65l7t.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqq7curxk22.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> 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.

Show 19 quoted lines

>                                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.