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

[Junio C Hamano <gitster@pobox.com>]

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

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