"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> > > The test 'diff --no-index reads from pipes' starts a couple of > background processes that write to the pipes that are passed to "diff > --no-index". If the test passes then we expect these processes to exit > as all their output will have been read. However if the test fails > then we want to make sure they do not hang about on the users machine > and the test remembers they should be killed by calling > > test_when_finished "! kill $!" > > after each background process is created. Unfortunately there is a > race where test_when_finished may run before the background process > exits even when all its output has been read resulting in the kill > command succeeding which causes the test to fail. Fix this by ignoring > the exit status of the kill command. If the diff is successful we > could instead wait for the background process to exit and check their > status but that feels like it is testing the platform's printf > implementation rather than git's code. > > Reported-by: Jeff King <peff@xxxxxxxx> > Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> > --- > t4053: avoid race when killing background processes > > Thanks to Peff for reporting this. Junio - this fixes a regression > introduced in the current cycle. It is fairly minor though so I'm not > sure if you want to pick it up now or wait until 2.42.0 is out. This did not cut -rc1 that was scheduled for yesterday, but a fix for a new test added during the cycle is a very welcome addition. While I can see that "kill" in the when-finished handler may or may not find the backgrounded process by the time it is run, and ignoring its exit status (hence keeping test_when_finished happy) would be a reasonable thing to do. I can understand if this patch is to fix a different symptom, namely, when-finished handler sometimes fails and makes the test fail. But I am not sure how this causes the test to "hang", which presumably is a symptom that somebody is trying to read from a pipe that nobody is making progress to write into? We will send a signal either way to the writers, and the only difference is that we ignore the exit code. Granted, when-finished handlers are concatenated with "&&-", and one "kill"s failure will cause the other "kill" not to run, so we may send a signal to only one but not to the other, but that should all happen after "diff --no-index" returns, so it still does not explain the "hang". Puzzled... > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1571%2Fphillipwood%2Fdiff-no-index-pipes-fixes-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1571/phillipwood/diff-no-index-pipes-fixes-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1571 > > t/t4053-diff-no-index.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh > index a28b9ff2434..1fb7d334620 100755 > --- a/t/t4053-diff-no-index.sh > +++ b/t/t4053-diff-no-index.sh > @@ -248,11 +248,11 @@ test_expect_success PIPE,SYMLINKS 'diff --no-index reads from pipes' ' > { > (test_write_lines a b c >old) & > } && > - test_when_finished "! kill $!" && > + test_when_finished "kill $! || :" && > { > (test_write_lines a x c >new) & > } && > - test_when_finished "! kill $!" && > + test_when_finished "kill $! || :" && > > cat >expect <<-EOF && > diff --git a/old b/new-link > > base-commit: a82fb66fed250e16d3010c75404503bea3f0ab61
