submodule status: propagate SIGPIPE

[phillipwood]

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.

Cc: Calvin Wan calvinwan@google.com Cc: Matt Liberty mliberty@precisioninno.com

[phillipwood]

/submit

[gitgitgadget[bot]]

Submitted as pull.1799.git.1726837642511.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1799/phillipwood/submodule-status-sigpipe-v1

To fetch this version to local tag pr-1799/phillipwood/submodule-status-sigpipe-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1799/phillipwood/submodule-status-sigpipe-v1

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> 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);
>   }

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

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/365fc4b6ce5065c970a4c2e082cab384cc715c3c.

[phillipwood]

/submit

[gitgitgadget[bot]]

Submitted as pull.1799.v2.git.1726925150113.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1799/phillipwood/submodule-status-sigpipe-v2

To fetch this version to local tag pr-1799/phillipwood/submodule-status-sigpipe-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1799/phillipwood/submodule-status-sigpipe-v2

[gitgitgadget[bot]]

On the Git mailing list, phillip.wood123@gmail.com wrote (reply to this):

Hi Junio

On 20/09/2024 21:06, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> 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
>
> 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 <mliberty@precisioninno.com>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>     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.

Thanks.  Queued.

[gitgitgadget[bot]]

This branch is now known as pw/submodule-process-sigpipe.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/cdf327e365c96183997acac746ed192d9d5b6ca9.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch pw/submodule-process-sigpipe on the Git mailing list:

When a subprocess to work in a submodule spawned by "git submodule"
fails with SIGPIPE, the parent Git process caught the death of it,
but gave a generic "failed to work in that submodule", which was
misleading.  We now behave as if the parent got SIGPIPE and die.

Will merge to 'next'.
source: <pull.1799.git.1726837642511.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/ae214bb04f55637236a18f52c7789fcca0dc055f.

[gitgitgadget[bot]]

This patch series was integrated into next via https://github.com/git/git/commit/9e1ce951c667370493ec0703190ed8738d2b0886.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/d3cc69e45035b26065bfb99fe113dcb7b5a50e75.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch pw/submodule-process-sigpipe on the Git mailing list:

When a subprocess to work in a submodule spawned by "git submodule"
fails with SIGPIPE, the parent Git process caught the death of it,
but gave a generic "failed to work in that submodule", which was
misleading.  We now behave as if the parent got SIGPIPE and die.

Will merge to 'master'.
source: <pull.1799.git.1726837642511.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/573dec9a118f4801859d5e31b7220be4c38e4545.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch pw/submodule-process-sigpipe on the Git mailing list:

When a subprocess to work in a submodule spawned by "git submodule"
fails with SIGPIPE, the parent Git process caught the death of it,
but gave a generic "failed to work in that submodule", which was
misleading.  We now behave as if the parent got SIGPIPE and die.

Will merge to 'master'.
source: <pull.1799.git.1726837642511.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/bcaa3d365c0511a3914c0fc65743381fe2537926.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/325ea81527deebcc052b05709095525d3c043608.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch pw/submodule-process-sigpipe on the Git mailing list:

When a subprocess to work in a submodule spawned by "git submodule"
fails with SIGPIPE, the parent Git process caught the death of it,
but gave a generic "failed to work in that submodule", which was
misleading.  We now behave as if the parent got SIGPIPE and die.

Will merge to 'master'.
source: <pull.1799.git.1726837642511.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/22baac889235bb3752ae36baec36f345871ac939.

[gitgitgadget[bot]]

This patch series was integrated into master via https://github.com/git/git/commit/22baac889235bb3752ae36baec36f345871ac939.

[gitgitgadget[bot]]

This patch series was integrated into next via https://github.com/git/git/commit/22baac889235bb3752ae36baec36f345871ac939.

[gitgitgadget[bot]]

Closed via 22baac889235bb3752ae36baec36f345871ac939.