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

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

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

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

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

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