git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Shourya Shukla <shouryashukla.oo@gmail.com>
Cc: christian.couder@gmail.com, git@vger.kernel.org,
	gitster@pobox.com, kaartic.sivaraam@gmail.com,
	liu.denton@gmail.com, Prathamesh Chavan <pc44800@gmail.com>,
	Christian Couder <chriscool@tuxfamily.org>,
	Stefan Beller <stefanbeller@gmail.com>
Subject: Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C
Date: Fri, 21 Aug 2020 17:17:42 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2008211708280.56@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20200812194404.17028-5-shouryashukla.oo@gmail.com>

Hi Shourya,

On Thu, 13 Aug 2020, Shourya Shukla wrote:

> [...]
> diff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh
> index 829fe26d6d..59a9b00467 100755
> --- a/t/t7421-submodule-summary-add.sh
> +++ b/t/t7421-submodule-summary-add.sh
> @@ -58,7 +58,7 @@ 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 &&
> -	test_i18ngrep "fatal:.*my-subm" err &&
> +	grep "fatal:.*my-subm" err &&

Sadly, this breaks on Windows: on Linux (and before this patch, also on
Windows), the error message reads somewhat like this:

	fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory

However, with the built-in `git submodule summary`, on Windows the error
message reads like this:

	error: cannot spawn git: No such file or directory

Now, this is of course not the best way to present this error message, but
please note that even providing a better error message does not fix the
erroneous expectation of the `fatal:` prefix (Git typically produces this
when `die()`ing, which can be done in the POSIX version that uses `fork()`
and `exec()` but not in the Windows version that needs to use
`CreateProcessW()` instead).

Therefore, I propose this patch on top:

-- snipsnap --
[PATCH] mingw: mention if `mingw_spawnve()` failed due to a missing directory

When we recently converted the `summary` subcommand of `git submodule`
to be mostly built-in, a bug was uncovered where a very unhelpful error
message was produced when a process could not be spawned because the
directory in which it was supposed to be run does not exist.

Even so, we _still_ have to adjust the `git submodule summary` test, to
accommodate for the fact that the `mingw_spawnve()` function will return
with an error instead of `die()`ing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c                   | 4 ++++
 t/t7421-submodule-summary-add.sh | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 1a64d4efb26b..3c30d0cab589 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1850,6 +1850,10 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
 	/* Make sure to override previous errors, if any */
 	errno = 0;

+	if (dir && !is_directory(dir))
+		return error_errno(_("could not exec '%s' in '%s'"),
+				   argv[0], dir);
+
 	if (restrict_handle_inheritance < 0)
 		restrict_handle_inheritance = core_restrict_inherited_handles;
 	/*
diff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh
index 59a9b00467dc..f00d69ca29ea 100755
--- a/t/t7421-submodule-summary-add.sh
+++ b/t/t7421-submodule-summary-add.sh
@@ -58,7 +58,7 @@ 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 &&
+	grep "my-subm" err &&
 	cat >expected <<-EOF &&
 	* my-subm ${rev}...0000000:

--
2.28.0.windows.1


  parent reply	other threads:[~2020-08-21 15:19 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 [this message]
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
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=nycvar.QRO.7.76.6.2008211708280.56@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --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).