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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 5, 2023, 00:13 UTC
Message-ID: <xmqq7curxk22.fsf@gitster.g>
In-Reply-To: <pull.1488.git.git.1680652122547.gitgitgadget@gmail.com>

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

>  	iter = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC);
>  
> -	if (!iter)
> +	if (!iter) {
> +		struct stat st;
> +		if (lstat(src->buf, &st) >= 0 && S_ISLNK(st.st_mode))
> +			die(_("'%s' is a symlink, refusing to clone with --local"),
> +			    src->buf);

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. I suspect it may not necessary to do another lstat(2). The function returns NULL:

 * if lstat() fails; errno is left from the failed lstat() in this case.

 * if lstat() succeeds, but the path is *not* a directory; errno is
   explicitly set to ENOTDIR.  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.

Assuming that the distinction does not matter, then,

	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.