From: Jeff King <peff@peff•net>
To: Junio C Hamano <gitster@pobox•com>
Cc: git@vger•kernel.org
Subject: Re: What's cooking in git.git (Mar 2026, #10)
Date: Wed, 25 Mar 2026 02:13:57 -0400 [thread overview]
Message-ID: <20260325061357.GA3772970@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqldfgy1ye.fsf@gitster.g>
On Tue, Mar 24, 2026 at 09:20:41PM -0700, Junio C Hamano wrote:
> * aa/reap-transport-child-processes (2026-03-12) 1 commit
> . transport-helper, connect: use clean_on_exit to reap children on abnormal exit
>
> A few code paths that spawned child processes for network
> connection weren't wait(2)ing for their children and letting "init"
> reap them instead; they have been tightened.
>
> CI failures on Windows in t0061.24?
> cf. <20260318040423.GA2858991@coredump•intra.peff.net>
> source: <20260312214945.4050010-1-cshung@gmail•com>
After a grueling session with tmate to connect to a GitHub Actions
Windows runner, I can confirm that the patch below fixes the failure.
I had a lingering worry that maybe the test was a canary in the coal
mine, telling us that other real-world cases may be mad about getting
kill()ed. But after looking at it, I'm not sure. The fake ssh command is
_so_ fake here that it produces garbage output immediately and is
killed before even doing its logging. A real failure would happen
farther along in the process. And presumably a real ssh client that has
cleanup to do would catch the signal and do that cleanup.
So...it's probably OK to fix the test and proceed with graduating the
topic, I think?
Anyway, here's the fix I came up with. But see below the --- line, too.
-- >8 --
Subject: [PATCH] t0061: simplify .bat test
The test added by 71f4960b91 (t0061: fix test for argv[0] with spaces
(MINGW only), 2019-10-01) checks that we can use a .bat file with spaces
as GIT_SSH.
This is a good test in the sense that it's how the original bug was
detected. And as the commit message there describes, there are some
elements of the bug that are likely to come up with GIT_SSH and not
elsewhere: namely that in addition to the .bat file having spaces, we
must pass an argument with spaces (which happens naturally with ssh,
since we pass the upload-pack shell command for the other side to run).
But using GIT_SSH does complicate matters:
1. We actually run the ssh command _twice_, once to probe the ssh
variant with "-G" in fill_ssh_args(), and then a second time to
actually make the connection. So we have to account for that when
checking the output.
2. Our fake ssh .bat file does not actually run ssh. So we expect the
command to fail, but not before the .bat file has touched the "out"
marker file that tells us it has run.
This works now, but is fragile. In particular, the .bat file by
default will echo commands it runs to stdout. From the perspective
of the parent Git process, this is protocol-breaking garbage, and
upon seeing it will die().
That is OK for now because we don't bother to do any cleanup of the
child process. But there is a patch under discussion, dd3693eb08
(transport-helper, connect: use clean_on_exit to reap children on
abnormal exit, 2026-03-12), which causes us to kill() the .bat
process. This happens before it actually touches the "out" file,
causing the test to fail.
We can simplify this by just using the "test-tool run-command" helper.
That lets us run whatever command we like with the arguments we want.
The argument here has a space, which is enough to trigger the original
bug that 71f4960b91 was testing. I verified that by reverting eb7c786314
(mingw: support spawning programs containing spaces in their names,
2019-07-16), the original fix, and confirming that the test fails (but
succeeds without the revert).
Signed-off-by: Jeff King <peff@peff•net>
---
One thing that puzzled me but that I figured out while writing the
commit message above is why the .bat file was writing garbage to stdout
in the first place. But then through the haze of DOS memories, I
recalled the horror of having to put "@echo off" at the top of every
batch file.
So probably the smallest fix is to just add that to the start of the
.bat file, or even just put "@" at the start of the single command in
the file. That would suppress the extra stdout, and Git would not
realize the bogus ssh command is failing until it exits without writing
anything (which is after it has touched the "out" file).
I didn't have the stomach or patience to spin up the tmate environment
again and test it, though. I do like how much simpler the test gets with
the t/helper program, but arguably using GIT_SSH is more realistic. I
dunno.
t/t0061-run-command.sh | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 2f77fde0d9..60cfe65979 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -287,16 +287,8 @@ test_expect_success MINGW 'can spawn .bat with argv[0] containing spaces' '
rm -f out &&
echo "echo %* >>out" >"$bat" &&
- # Ask git to invoke .bat; clone will fail due to fake SSH helper
- test_must_fail env GIT_SSH="$bat" git clone myhost:src ssh-clone &&
-
- # Spawning .bat can fail if there are two quoted cmd.exe arguments.
- # .bat itself is first (due to spaces in name), so just one more is
- # needed to verify. GIT_SSH will invoke .bat multiple times:
- # 1) -G myhost
- # 2) myhost "git-upload-pack src"
- # First invocation will always succeed. Test the second one.
- grep "git-upload-pack" out
+ test-tool run-command run-command "$bat" "arg with spaces" &&
+ test_grep "arg with spaces" out
'
test_done
--
2.53.0.1057.g8b84bc5fb6
next prev parent reply other threads:[~2026-03-25 6:13 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-25 4:20 What's cooking in git.git (Mar 2026, #10) Junio C Hamano
2026-03-25 6:13 ` Jeff King [this message]
2026-03-25 8:33 ` Shreyansh Paliwal
2026-03-25 19:50 ` Junio C Hamano
2026-03-26 18:07 ` kh/name-rev-custom-format Kristoffer Haugsbakk
2026-03-26 18:48 ` kh/name-rev-custom-format Junio C Hamano
2026-03-26 19:22 ` kh/name-rev-custom-format Kristoffer Haugsbakk
2026-03-26 19:44 ` kh/name-rev-custom-format Junio C Hamano
-- strict thread matches above, loose matches on Subject: below --
2026-03-25 21:11 What's cooking in git.git (Mar 2026, #10) Shreyansh Paliwal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260325061357.GA3772970@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox