git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
	Shourya Shukla <shouryashukla.oo@gmail.com>
Cc: 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: Sat, 22 Aug 2020 00:24:01 +0530	[thread overview]
Message-ID: <377b1a2ad60c5ca30864f48c5921ff89b5aca65b.camel@gmail.com> (raw)
In-Reply-To: <xmqq5z9ban27.fsf@gitster.c.googlers.com>

On Fri, 2020-08-21 at 11:09 -0700, Junio C Hamano wrote:
> 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



  reply	other threads:[~2020-08-21 18: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 [this message]
2020-08-21 19:54               ` Junio C Hamano
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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=377b1a2ad60c5ca30864f48c5921ff89b5aca65b.camel@gmail.com \
    --to=kaartic.sivaraam@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).