submodule status: propagate SIGPIPE

Commit Message

Phillip Wood Sept. 20, 2024, 1:07 p.m. UTC

From: Phillip Wood <[e-mail]>

It has been reported than running

     git submodule status --recurse | grep -q ^+

results in an unexpected error message

    fatal: failed to recurse into submodule $submodule

When "git submodule--helper" recurses into a submodule it creates a
child process. If that process fails then the error message above is
displayed by the parent. In the case above the child is killed by
SIGPIPE as "grep -q" exits as soon as it sees the first match. Fix this
by propagating SIGPIPE so that it is visible to the process running
git. We could propagate other signals but I'm not sure there is much
value in doing that. In the common case of the user pressing Ctrl-C or
Ctrl-\ then SIGINT or SIGQUIT will be sent to the foreground process
group and so the parent process will receive the same signal as the
child.

Reported-by: Matt Liberty <[e-mail]>
Signed-off-by: Phillip Wood <[e-mail]>
---
    submodule status: propagate SIGPIPE
    
    Note that I haven't checked if any other submodule subcommands are
    affected by this - I'll leave that to someone more familiar with the
    code.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1799%2Fphillipwood%2Fsubmodule-status-sigpipe-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1799/phillipwood/submodule-status-sigpipe-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1799

 builtin/submodule--helper.c | 7 ++++++-
 t/t7422-submodule-output.sh | 7 +++++++
 2 files changed, 13 insertions(+), 1 deletion(-)


base-commit: ed155187b429a2a6b6475efe1767053df37ccfe1

Comments

Junio C Hamano Sept. 20, 2024, 7:01 p.m. UTC | #1

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> From: Phillip Wood <[e-mail]>
>
> It has been reported than running
>
>      git submodule status --recurse | grep -q ^+
>
> results in an unexpected error message
>
>     fatal: failed to recurse into submodule $submodule
> ...
> -		if (run_command(&cpr))
> +		res = run_command(&cpr);
> +		if (res == SIGPIPE + 128)
> +			raise(SIGPIPE);

OK, that is straight-forward.  This makes sure that we do the same
thing we would do if we, not our child, got a SIGPIPE.

> +		else if (res)
>  			die(_("failed to recurse into submodule '%s'"), path);
>  	}

> diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
> index ab946ec9405..c1686d6bb5f 100755
> --- a/t/t7422-submodule-output.sh
> +++ b/t/t7422-submodule-output.sh
> @@ -167,4 +167,11 @@ do
>  	'
>  done
>  
> +test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE' '
> +	{ git submodule status --recursive 2>err; echo $?>status; } |
> +		grep -q X/S &&
> +	test_must_be_empty err &&
> +	test_match_signal 13 "$(cat status)"

I am not a huge fun of assuming SIGPIPE is 13 everywhere, but at
least we can tweak test_match_signal when we find oddball systems,
so ... OK.

In practice, we only use 13 and 15 with test_match_signal, so we
could have a new "test-tool signal-name" that maps textual signal
names to the number the platform gives to them for the platform on
which the tests are running, if it ever turns out to be a problem.

Looking good.

Will queue.  Thanks.

Junio C Hamano Sept. 20, 2024, 8:06 p.m. UTC | #2

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a46ffd49b34..a8e497ef3c6 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -30,6 +30,7 @@
>  #include "advice.h"
>  #include "branch.h"
>  #include "list-objects-filter-options.h"
> +#include <signal.h>

Do we really need this?

As with any other Git built-in that relies on git-compat-util.h to
handle such system-dependencies, direct inclusion of system headers
like this is highly questionable.

Phillip Wood Sept. 21, 2024, 1:23 p.m. UTC | #3

Hi Junio

On 20/09/2024 21:06, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[e-mail]> writes:
> 
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index a46ffd49b34..a8e497ef3c6 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -30,6 +30,7 @@
>>   #include "advice.h"
>>   #include "branch.h"
>>   #include "list-objects-filter-options.h"
>> +#include <signal.h>
> 
> Do we really need this?
> 
> As with any other Git built-in that relies on git-compat-util.h to
> handle such system-dependencies, direct inclusion of system headers
> like this is highly questionable.

Good point - I really need to figure out how to stop emacs' lsp mode 
automatically adding includes. I removed its "helpful" addition of 
<csignal> but forgot to remove <signal.h> as well.

Thanks

Phillip

Patch

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a46ffd49b34..a8e497ef3c6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -30,6 +30,7 @@ 
 #include "advice.h"
 #include "branch.h"
 #include "list-objects-filter-options.h"
+#include <signal.h>
 
 #define OPT_QUIET (1 << 0)
 #define OPT_CACHED (1 << 1)
@@ -695,6 +696,7 @@  static void status_submodule(const char *path, const struct object_id *ce_oid,
 
 	if (flags & OPT_RECURSIVE) {
 		struct child_process cpr = CHILD_PROCESS_INIT;
+		int res;
 
 		cpr.git_cmd = 1;
 		cpr.dir = path;
@@ -710,7 +712,10 @@  static void status_submodule(const char *path, const struct object_id *ce_oid,
 		if (flags & OPT_QUIET)
 			strvec_push(&cpr.args, "--quiet");
 
-		if (run_command(&cpr))
+		res = run_command(&cpr);
+		if (res == SIGPIPE + 128)
+			raise(SIGPIPE);
+		else if (res)
 			die(_("failed to recurse into submodule '%s'"), path);
 	}
 
diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
index ab946ec9405..c1686d6bb5f 100755
--- a/t/t7422-submodule-output.sh
+++ b/t/t7422-submodule-output.sh
@@ -167,4 +167,11 @@  do
 	'
 done
 
+test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE' '
+	{ git submodule status --recursive 2>err; echo $?>status; } |
+		grep -q X/S &&
+	test_must_be_empty err &&
+	test_match_signal 13 "$(cat status)"
+'
+
 test_done