Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Aug 21, 2020, 18:54 UTC
Message-ID: <377b1a2ad60c5ca30864f48c5921ff89b5aca65b.camel@gmail.com>
In-Reply-To: <xmqq5z9ban27.fsf@gitster.c.googlers.com>

On Fri, 2020-08-21 at 11:09 -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> 
> > > I think a test that relies on platform-specific error string is a
> > > bug.  It's like expecting an exact string out of strerror(),
> > > which
> > > we had to fix a few times.
> > > So I am not sure we would want to butcher compat/mingw.c only to
> > > match such an expectation by a (buggy) test.
> > 
> > Alright. That is understandable. What alternative do you suggest?
> > Should
> > we change the check in the test?
> 
> A buggy check should of course be changed.
> 
> It should be sufficient to ensure "git submodule summary" fails,
> regardless of what exact error message it issues, no?
>

Unfortunately, we can't do that here. See below.

> If the command does not exit with non-zero exit status, when it
> gives a "fatal" error message, that may indicate another bug,
> though.

Here's the error message with context of the trash directory of that test:

-- 8< -- $ cd t $ ./t7421-submodule-summary-add.sh -d $ cd trash\ directory.t7421-submodule-summary-add/

$ git submodule summary HEAD^^
fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
* my-subm 35b40f1...0000000:

* subm 0000000...dbd5fc8 (2):
  > add file2

-- >8 --

That 'fatal' is a consequence of spawning a process in `verify_submodule_committish` of `builtin/submodule--helper.c` even for non-existent submodules. I don't think that 'fatal: ' message is giving any useful information here. The fact that submodule 'my-subm' doesn't exist can easily be inferred just by looking at the destination mode (0000000). If anything that 'fatal' message is just confusing and unnecessary, IMO.

So, we could easily suppress it by doing something like this (while also fixing the test):

-- 8< --

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 63ea39025d..d45be7fbdf 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -972,7 +972,7 @@ static char* verify_submodule_committish(const char *sm_path,
        strvec_pushf(&cp_rev_parse.args, "%s^0", committish);
        strvec_push(&cp_rev_parse.args, "--");
 
-       if (capture_command(&cp_rev_parse, &result, 0))
+       if (!is_directory(sm_path) || capture_command(&cp_rev_parse, &result, 0))
                return NULL;
 
        strbuf_trim_trailing_newline(&result);
diff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh
index 59a9b00467..8a2c2b38b6 100755
--- a/t/t7421-submodule-summary-add.sh
+++ b/t/t7421-submodule-summary-add.sh
@@ -58,7 +58,6 @@ test_expect_success 'submodule summary output for submodules with changed paths'
        git commit -m "change submodule path" &&
        rev=$(git -C sm rev-parse --short HEAD^) &&
        git submodule summary HEAD^^ -- my-subm >actual 2>err &&
-       grep "fatal:.*my-subm" err &&
        cat >expected <<-EOF &&
        * my-subm ${rev}...0000000:
 
-- >8 --

BTW, I noted that `print_submodule_summary` has the following
definition:

   static void print_submodule_summary(struct summary_cb *info, char* errmsg
   				    ...

Note how '*' is placed near 'char' for 'errmsg' with an incorrect style. Ditto
for the return type of `verify_submodule_committish`. This might make
for a nice cleanup patch.

-- 
Sivaraam