git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Cc: Shourya Shukla <shouryashukla.oo@gmail.com>,
	Johannes.Schindelin@gmx.de, chriscool@tuxfamily.org,
	christian.couder@gmail.com, git@vger.kernel.org,
	liu.denton@gmail.com, pc44800@gmail.com, stefanbeller@gmail.com
Subject: Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C
Date: Fri, 21 Aug 2020 12:54:46 -0700	[thread overview]
Message-ID: <xmqqa6yn93ll.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <377b1a2ad60c5ca30864f48c5921ff89b5aca65b.camel@gmail.com> (Kaartic Sivaraam's message of "Sat, 22 Aug 2020 00:24:01 +0530")

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.

  reply	other threads:[~2020-08-21 19:55 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-06 16:40 [GSoC][PATCH v2 0/5] submodule: port " Shourya Shukla
2020-08-06 16:40 ` [PATCH v2 1/5] submodule: expose the '--for-status' option of summary Shourya Shukla
2020-08-08 14:40   ` Kaartic Sivaraam
2020-08-08 20:25     ` Christian Couder
2020-08-08 23:26       ` Junio C Hamano
2020-08-06 16:40 ` [PATCH v2 2/5] submodule: remove extra line feeds between callback struct and macro Shourya Shukla
2020-08-06 16:41 ` [PATCH v2 3/5] submodule: rename helper functions to avoid ambiguity Shourya Shukla
2020-08-06 16:41 ` [PATCH v2 4/5] t7421: introduce a test script for verifying 'summary' output Shourya Shukla
2020-08-06 16:41 ` [PATCH v2 5/5] submodule: port submodule subcommand 'summary' from shell to C Shourya Shukla
2020-08-06 22:45   ` Junio C Hamano
2020-08-07 16:31     ` Shourya Shukla
2020-08-07 17:15       ` Junio C Hamano
2020-08-12 19:44 ` [GSoC][PATCH v3 0/4] submodule: port " Shourya Shukla
2020-08-12 19:44   ` [PATCH v3 1/4] submodule: remove extra line feeds between callback struct and macro Shourya Shukla
2020-08-12 19:44   ` [PATCH v3 2/4] submodule: rename helper functions to avoid ambiguity Shourya Shukla
2020-08-12 19:44   ` [PATCH v3 3/4] t7421: introduce a test script for verifying 'summary' output Shourya Shukla
2020-08-12 19:44   ` [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C Shourya Shukla
2020-08-18  2:08     ` Jeff King
2020-08-21  5:22       ` Shourya Shukla
2020-08-21 15:17     ` Johannes Schindelin
2020-08-21 16:35       ` Junio C Hamano
2020-08-21 17:17         ` Shourya Shukla
2020-08-21 18:09           ` Junio C Hamano
2020-08-21 18:54             ` Kaartic Sivaraam
2020-08-21 19:54               ` Junio C Hamano [this message]
2020-08-23 20:03                 ` Kaartic Sivaraam
2020-08-23 20:12                   ` Kaartic Sivaraam
2020-08-24  7:26                   ` Shourya Shukla
2020-08-24  8:46                     ` Shourya Shukla
2020-08-24 11:08                       ` Kaartic Sivaraam
2020-08-24 17:50                         ` Shourya Shukla
2020-08-24 17:54     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqa6yn93ll.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=kaartic.sivaraam@gmail.com \
    --cc=liu.denton@gmail.com \
    --cc=pc44800@gmail.com \
    --cc=shouryashukla.oo@gmail.com \
    --cc=stefanbeller@gmail.com \
    --subject='Re: [PATCH v3 4/4] submodule: port submodule subcommand '\''summary'\'' from shell to C' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).