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

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

[Ian Wienand]

After upgrading past 505a2765963 a long-working script to cache git
repos started failing with

 Could not access submodule '...'

for every updated submodule on each fetch [1].

Ultimately this turned out to be using "--git-dir=" from outside the
repo; i.e. we really wanted "-C" in this script (the man page does
warn about this -- but it was working for a long time).

Although obvious in hindsight, this was very difficult to diagnose
from the error message.  It required me adding debugging to these
functions to determine why it was falling into this path when
everything looked right on disk.

This proposes separate messages for the directory missing v. being
present but having unexpected contents.  Both messages are modified to
give the path that is being examined.

[1] https://review.opendev.org/c/openstack/diskimage-builder/+/818053

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 submodule.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index c689070524..910ee6ba7d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1521,9 +1521,16 @@ static int get_next_submodule(struct child_process *cp,
 			if (S_ISGITLINK(ce->ce_mode) &&
 			    !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)) {
+				  strbuf_addf(err,
+					      _("Submodule directory '%s' not found (incorrect --git-dir?)\n"),
+					      empty_submodule_path.buf);
+				} else {
+				  strbuf_addf(err,
+					      _("Submodule directory '%s' is not empty\n"),
+					      empty_submodule_path.buf);
+				}
 			}
 			strbuf_release(&empty_submodule_path);
 		}
-- 
2.33.1

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

[Junio C Hamano]

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);
>  		}

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

[Ian Wienand]

Thanks for the review!

On Wed, Nov 17, 2021 at 01:39:08AM -0800, Junio C Hamano wrote:
> 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.

That was my thinking too, I do admit it's a bit of an obscure case to
end up in, but the regular path doesn't need any more checking.

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

Ok, I can drop that bit, as you say it's a guess.

> 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

Actually in the original patch I dropped the "Could not access..."
bit, but I agree it makes more sense with the more specific details
added.  I've put that back and append the secondary message below.

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

Done

-i

From 6ea39c3e43eccf93e4567329cfb1b9c642154283 Mon Sep 17 00:00:00 2001
From: Ian Wienand <iwienand@redhat.com>
Date: Wed, 17 Nov 2021 09:39:40 +1100
Subject: [PATCH] submodule: separate out not-found and not-empty errors

After upgrading past 505a2765963 a long-working script to cache git
repos started failing with

 Could not access submodule '...'

for every updated submodule on each fetch [1].

Ultimately this turned out to be using "--git-dir=" from outside the
repo; i.e. we really wanted "-C" in this script (the man page does
warn about this -- but it was working for a long time).

Although obvious in hindsight, this was very difficult to diagnose
from the error message.  It required me adding debugging to these
functions to determine why it was falling into this path when
everything looked right on disk.

This proposes separate messages for the directory missing v. being
present but having unexpected contents.  Both messages are modified to
give the path that is being examined.

[1] https://review.opendev.org/c/openstack/diskimage-builder/+/818053

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 submodule.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/submodule.c b/submodule.c
index c689070524..352ee50f2e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1524,6 +1524,16 @@ static int get_next_submodule(struct child_process *cp,
 				strbuf_addf(err,
 					    _("Could not access submodule '%s'\n"),
 					    ce->name);
+				/* is_empty_dir also catches missing directories, but report separately */
+				if (!is_directory(empty_submodule_path.buf)) {
+				  strbuf_addf(err,
+					      _("Submodule path '%s' is not a directory\n"),
+					      empty_submodule_path.buf);
+				} else {
+				  strbuf_addf(err,
+					      _("Submodule directory '%s' is not empty\n"),
+					      empty_submodule_path.buf);
+				}
 			}
 			strbuf_release(&empty_submodule_path);
 		}
-- 
2.33.1