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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 17, 2021, 09:39 UTC
Message-ID: <xmqqfsrv2kar.fsf@gitster.g>
In-Reply-To: <YZQ5Zk0ItWvfr8sF@fedora19.localdomain>

Ian Wienand <iwienand@redhat.com> writes:

Show 7 quoted lines

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

Show 9 quoted lines

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