Re: [PATCH] submodule status: propagate SIGPIPE

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 20, 2024, 19:01 UTC
Message-ID: <xmqqbk0igndk.fsf@gitster.g>
In-Reply-To: <pull.1799.git.1726837642511.gitgitgadget@gmail.com>

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 14 quoted lines

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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);
>  	}

Show 13 quoted lines

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