submodule status: propagate SIGPIPE #1799
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
phillipwood
force-pushed
the
submodule-status-sigpipe
branch
from
c5dbfd4
to
64233d5
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Phillip Wood via GitGitGadget" <[email protected]> writes:
> From: Phillip Wood <[email protected]>
>
> 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. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Phillip Wood via GitGitGadget" <[email protected]> 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.
|
|
This patch series was integrated into seen via git@365fc4b. |
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 <[email protected]>
Signed-off-by: Phillip Wood <[email protected]>
phillipwood
force-pushed
the
submodule-status-sigpipe
branch
from
64233d5
to
169e2c0
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, [email protected] wrote (reply to this): Hi Junio
On 20/09/2024 21:06, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[email protected]> 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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Phillip Wood via GitGitGadget" <[email protected]> writes:
> From: Phillip Wood <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Phillip Wood <[email protected]>
> ---
> 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. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@cdf327e. |
|
There was a status update in the "Cooking" section about the branch 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: <[email protected]> |
|
This patch series was integrated into seen via git@ae214bb. |
|
This patch series was integrated into next via git@9e1ce95. |
|
This patch series was integrated into seen via git@d3cc69e. |
|
There was a status update in the "Cooking" section about the branch 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: <[email protected]> |
|
This patch series was integrated into seen via git@573dec9. |
|
There was a status update in the "Cooking" section about the branch 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: <[email protected]> |
|
This patch series was integrated into seen via git@bcaa3d3. |
|
This patch series was integrated into seen via git@325ea81. |
|
There was a status update in the "Cooking" section about the branch 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: <[email protected]> |
|
This patch series was integrated into seen via git@22baac8. |
|
This patch series was integrated into master via git@22baac8. |
|
This patch series was integrated into next via git@22baac8. |
|
Closed via 22baac8. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 [email protected]
Cc: Matt Liberty [email protected]