Re: [PATCH] submodule: separate out not-found and not-empty errors

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

Ian Wienand <iwienand@redhat.com> writes:

>  			    !is_empty_dir(empty_submodule_path.buf)) {
>  				spf->result = 1;
> -				strbuf_addf(err,
> -					    _("Could not access submodule '%s'\n"),
> -					    ce->name);
> +				/* is_empty_dir also catches missing dirtectories, but report separately */
> +				if (!is_directory(empty_submodule_path.buf)) {

I was hoping that inspecting errno after is_empty_dir() returned
might be sufficient (of course, we need to clear errno before
calling is_empty_dir() if we go that route), but because this is an
error codepath that we do not need to optimize, a call to
is_directory() that incurs another system call would be fine.

> +				  strbuf_addf(err,
> +					      _("Submodule directory '%s' not found (incorrect --git-dir?)\n"),

"not found" is something the code definitely knows (eh, not quite,
but let's read on).  

But let's not make an uninformed guess.  This code didn't even check
if the user gave a --git-dir option.

If the user is advanced enough to have given "--git-dir", "not found"
should be sufficient to hint that the way the user specified the
repository location incorrectly, and a wrong "--git-dir" might be
one of the many things the user might suspect on their own.

Another problem with the message is !is_directory() can mean "there
is no filesystem entity at the path" (i.e. "submodule directory '%s'
does not exist") and it can also mean "there is a filesystem entity
at the path, but that is not a directory).  "not found" is not exactly
a good message to give in the latter case.

We are giving two messages here in this codepath.  For example, the
original one would have said something like:

	Could not access submodule 'foo'
	Submodule directory 'foo' is not empty

So I suspect that a more appropriate phrasing for the other one (the
new one you added) would be something like

	Could not access submodule 'foo'
	Path to the submodule 'foo' is not a directory

perhaps?

Thanks.


> +					      empty_submodule_path.buf);
> +				} else {
> +				  strbuf_addf(err,
> +					      _("Submodule directory '%s' is not empty\n"),
> +					      empty_submodule_path.buf);
> +				}
>  			}
>  			strbuf_release(&empty_submodule_path);
>  		}