{"thread":{"id":"62153","subject":"[PATCH] submodule status: propagate SIGPIPE","startedAt":"2024-09-20T13:07:26Z","lastAt":"2024-09-23T16:26:45Z","messageCount":6,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"503122","messageId":"pull.1799.git.1726837642511.gitgitgadget@gmail.com","threadId":"62153","inReplyTo":null,"subject":"[PATCH] submodule status: propagate SIGPIPE","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-20T13:07:22Z","receivedAt":"2024-09-20T13:07:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIt has been reported than running\n\n     git submodule status --recurse | grep -q ^+\n\nresults in an unexpected error message\n\n    fatal: failed to recurse into submodule $submodule\n\nWhen \"git submodule--helper\" recurses into a submodule it creates a\nchild process. If that process fails then the error message above is\ndisplayed by the parent. In the case above the child is killed by\nSIGPIPE as \"grep -q\" exits as soon as it sees the first match. Fix this\nby propagating SIGPIPE so that it is visible to the process running\ngit. We could propagate other signals but I'm not sure there is much\nvalue in doing that. In the common case of the user pressing Ctrl-C or\nCtrl-\\ then SIGINT or SIGQUIT will be sent to the foreground process\ngroup and so the parent process will receive the same signal as the\nchild.\n\nReported-by: Matt Liberty <mliberty@precisioninno.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    submodule status: propagate SIGPIPE\n    \n    Note that I haven't checked if any other submodule subcommands are\n    affected by this - I'll leave that to someone more familiar with the\n    code.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1799%2Fphillipwood%2Fsubmodule-status-sigpipe-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1799/phillipwood/submodule-status-sigpipe-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1799\n\n builtin/submodule--helper.c | 7 ++++++-\n t/t7422-submodule-output.sh | 7 +++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex a46ffd49b34..a8e497ef3c6 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -30,6 +30,7 @@\n #include \"advice.h\"\n #include \"branch.h\"\n #include \"list-objects-filter-options.h\"\n+#include <signal.h>\n \n #define OPT_QUIET (1 << 0)\n #define OPT_CACHED (1 << 1)\n@@ -695,6 +696,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \n \tif (flags & OPT_RECURSIVE) {\n \t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n+\t\tint res;\n \n \t\tcpr.git_cmd = 1;\n \t\tcpr.dir = path;\n@@ -710,7 +712,10 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \t\tif (flags & OPT_QUIET)\n \t\t\tstrvec_push(&cpr.args, \"--quiet\");\n \n-\t\tif (run_command(&cpr))\n+\t\tres = run_command(&cpr);\n+\t\tif (res == SIGPIPE + 128)\n+\t\t\traise(SIGPIPE);\n+\t\telse if (res)\n \t\t\tdie(_(\"failed to recurse into submodule '%s'\"), path);\n \t}\n \ndiff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\nindex ab946ec9405..c1686d6bb5f 100755\n--- a/t/t7422-submodule-output.sh\n+++ b/t/t7422-submodule-output.sh\n@@ -167,4 +167,11 @@ do\n \t'\n done\n \n+test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE' '\n+\t{ git submodule status --recursive 2>err; echo $?>status; } |\n+\t\tgrep -q X/S &&\n+\ttest_must_be_empty err &&\n+\ttest_match_signal 13 \"$(cat status)\"\n+'\n+\n test_done\n\nbase-commit: ed155187b429a2a6b6475efe1767053df37ccfe1\n-- \ngitgitgadget\n"},{"id":"503156","messageId":"xmqqbk0igndk.fsf@gitster.g","threadId":"62153","inReplyTo":"pull.1799.git.1726837642511.gitgitgadget@gmail.com","subject":"Re: [PATCH] submodule status: propagate SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-20T19:01:59Z","receivedAt":"2024-09-20T19:02:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> It has been reported than running\n>\n>      git submodule status --recurse | grep -q ^+\n>\n> results in an unexpected error message\n>\n>     fatal: failed to recurse into submodule $submodule\n> ...\n> -\t\tif (run_command(&cpr))\n> +\t\tres = run_command(&cpr);\n> +\t\tif (res == SIGPIPE + 128)\n> +\t\t\traise(SIGPIPE);\n\nOK, that is straight-forward.  This makes sure that we do the same\nthing we would do if we, not our child, got a SIGPIPE.\n\n> +\t\telse if (res)\n>  \t\t\tdie(_(\"failed to recurse into submodule '%s'\"), path);\n>  \t}\n\n> diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\n> index ab946ec9405..c1686d6bb5f 100755\n> --- a/t/t7422-submodule-output.sh\n> +++ b/t/t7422-submodule-output.sh\n> @@ -167,4 +167,11 @@ do\n>  \t'\n>  done\n>  \n> +test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE' '\n> +\t{ git submodule status --recursive 2>err; echo $?>status; } |\n> +\t\tgrep -q X/S &&\n> +\ttest_must_be_empty err &&\n> +\ttest_match_signal 13 \"$(cat status)\"\n\nI am not a huge fun of assuming SIGPIPE is 13 everywhere, but at\nleast we can tweak test_match_signal when we find oddball systems,\nso ... OK.\n\nIn practice, we only use 13 and 15 with test_match_signal, so we\ncould have a new \"test-tool signal-name\" that maps textual signal\nnames to the number the platform gives to them for the platform on\nwhich the tests are running, if it ever turns out to be a problem.\n\nLooking good.\n\nWill queue.  Thanks.\n"},{"id":"503161","messageId":"xmqqfrpuf5u7.fsf@gitster.g","threadId":"62153","inReplyTo":"pull.1799.git.1726837642511.gitgitgadget@gmail.com","subject":"Re: [PATCH] submodule status: propagate SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-20T20:06:08Z","receivedAt":"2024-09-20T20:06:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index a46ffd49b34..a8e497ef3c6 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -30,6 +30,7 @@\n>  #include \"advice.h\"\n>  #include \"branch.h\"\n>  #include \"list-objects-filter-options.h\"\n> +#include <signal.h>\n\nDo we really need this?\n\nAs with any other Git built-in that relies on git-compat-util.h to\nhandle such system-dependencies, direct inclusion of system headers\nlike this is highly questionable.\n\n"},{"id":"503188","messageId":"ae114c58-2a9b-415a-9f47-312e4d080492@gmail.com","threadId":"62153","inReplyTo":"xmqqfrpuf5u7.fsf@gitster.g","subject":"Re: [PATCH] submodule status: propagate SIGPIPE","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-09-21T13:23:34Z","receivedAt":"2024-09-21T13:23:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 20/09/2024 21:06, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index a46ffd49b34..a8e497ef3c6 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -30,6 +30,7 @@\n>>   #include \"advice.h\"\n>>   #include \"branch.h\"\n>>   #include \"list-objects-filter-options.h\"\n>> +#include <signal.h>\n> \n> Do we really need this?\n> \n> As with any other Git built-in that relies on git-compat-util.h to\n> handle such system-dependencies, direct inclusion of system headers\n> like this is highly questionable.\n\nGood point - I really need to figure out how to stop emacs' lsp mode \nautomatically adding includes. I removed its \"helpful\" addition of \n<csignal> but forgot to remove <signal.h> as well.\n\nThanks\n\nPhillip\n"},{"id":"503189","messageId":"pull.1799.v2.git.1726925150113.gitgitgadget@gmail.com","threadId":"62153","inReplyTo":"pull.1799.git.1726837642511.gitgitgadget@gmail.com","subject":"[PATCH v2] submodule status: propagate SIGPIPE","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-21T13:25:49Z","receivedAt":"2024-09-21T13:25:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIt has been reported than running\n\n     git submodule status --recurse | grep -q ^+\n\nresults in an unexpected error message\n\n    fatal: failed to recurse into submodule $submodule\n\nWhen \"git submodule--helper\" recurses into a submodule it creates a\nchild process. If that process fails then the error message above is\ndisplayed by the parent. In the case above the child is killed by\nSIGPIPE as \"grep -q\" exits as soon as it sees the first match. Fix this\nby propagating SIGPIPE so that it is visible to the process running\ngit. We could propagate other signals but I'm not sure there is much\nvalue in doing that. In the common case of the user pressing Ctrl-C or\nCtrl-\\ then SIGINT or SIGQUIT will be sent to the foreground process\ngroup and so the parent process will receive the same signal as the\nchild.\n\nReported-by: Matt Liberty <mliberty@precisioninno.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    submodule status: propagate SIGPIPE\n    \n    Note that I haven't checked if any other submodule subcommands are\n    affected by this - I'll leave that to someone more familiar with the\n    code.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1799%2Fphillipwood%2Fsubmodule-status-sigpipe-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1799/phillipwood/submodule-status-sigpipe-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1799\n\nRange-diff vs v1:\n\n 1:  64233d5ee0a ! 1:  169e2c06f1a submodule status: propagate SIGPIPE\n     @@ Commit message\n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n       ## builtin/submodule--helper.c ##\n     -@@\n     - #include \"advice.h\"\n     - #include \"branch.h\"\n     - #include \"list-objects-filter-options.h\"\n     -+#include <signal.h>\n     - \n     - #define OPT_QUIET (1 << 0)\n     - #define OPT_CACHED (1 << 1)\n      @@ builtin/submodule--helper.c: static void status_submodule(const char *path, const struct object_id *ce_oid,\n       \n       \tif (flags & OPT_RECURSIVE) {\n\n\n builtin/submodule--helper.c | 6 +++++-\n t/t7422-submodule-output.sh | 7 +++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex a46ffd49b34..4daca494b80 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -695,6 +695,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \n \tif (flags & OPT_RECURSIVE) {\n \t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n+\t\tint res;\n \n \t\tcpr.git_cmd = 1;\n \t\tcpr.dir = path;\n@@ -710,7 +711,10 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \t\tif (flags & OPT_QUIET)\n \t\t\tstrvec_push(&cpr.args, \"--quiet\");\n \n-\t\tif (run_command(&cpr))\n+\t\tres = run_command(&cpr);\n+\t\tif (res == SIGPIPE + 128)\n+\t\t\traise(SIGPIPE);\n+\t\telse if (res)\n \t\t\tdie(_(\"failed to recurse into submodule '%s'\"), path);\n \t}\n \ndiff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\nindex ab946ec9405..c1686d6bb5f 100755\n--- a/t/t7422-submodule-output.sh\n+++ b/t/t7422-submodule-output.sh\n@@ -167,4 +167,11 @@ do\n \t'\n done\n \n+test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE' '\n+\t{ git submodule status --recursive 2>err; echo $?>status; } |\n+\t\tgrep -q X/S &&\n+\ttest_must_be_empty err &&\n+\ttest_match_signal 13 \"$(cat status)\"\n+'\n+\n test_done\n\nbase-commit: ed155187b429a2a6b6475efe1767053df37ccfe1\n-- \ngitgitgadget\n"},{"id":"503263","messageId":"xmqqo74e49q4.fsf@gitster.g","threadId":"62153","inReplyTo":"pull.1799.v2.git.1726925150113.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] submodule status: propagate SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-23T16:26:43Z","receivedAt":"2024-09-23T16:26:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> It has been reported than running\n>\n>      git submodule status --recurse | grep -q ^+\n>\n> results in an unexpected error message\n>\n>     fatal: failed to recurse into submodule $submodule\n>\n> When \"git submodule--helper\" recurses into a submodule it creates a\n> child process. If that process fails then the error message above is\n> displayed by the parent. In the case above the child is killed by\n> SIGPIPE as \"grep -q\" exits as soon as it sees the first match. Fix this\n> by propagating SIGPIPE so that it is visible to the process running\n> git. We could propagate other signals but I'm not sure there is much\n> value in doing that. In the common case of the user pressing Ctrl-C or\n> Ctrl-\\ then SIGINT or SIGQUIT will be sent to the foreground process\n> group and so the parent process will receive the same signal as the\n> child.\n>\n> Reported-by: Matt Liberty <mliberty@precisioninno.com>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>     submodule status: propagate SIGPIPE\n>     \n>     Note that I haven't checked if any other submodule subcommands are\n>     affected by this - I'll leave that to someone more familiar with the\n>     code.\n\nThanks.  Queued.\n"}]}