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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 21, 2020, 19:54 UTC
Message-ID: <xmqqa6yn93ll.fsf@gitster.c.googlers.com>
In-Reply-To: <377b1a2ad60c5ca30864f48c5921ff89b5aca65b.camel@gmail.com>

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

Show 20 quoted lines

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

Oh, so doing something that would cause the error message to be emitted itself is a bug.

Show 5 quoted lines

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

Yes, I 100% agree.

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

Yup.  That is a very good idea.

Or the caller of verify_submodule_committish() should refrain from calling it for the path? After all, it is checking sm_path is a path to where a submodule should be before calling the function (instead of calling it for every random path), iow its criteria to make the call currently may be "the path in the index says it is a submodule", but it should easily be updated to "the path in the index says it is a submodule, and the submodule actually is populated", right?

Show 12 quoted lines

> @@ -972,7 +972,7 @@ static char* verify_submodule_committish(const char *sm_path,
>         strvec_pushf(&cp_rev_parse.args, "%s^0", committish);
> ...
> 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.

Yup. It would have been nicer to catch these before the topic hit 'next'.

Thanks.