On Fri, Mar 10, 2023 at 12:49:04PM +0100, Patrick Steinhardt wrote:
> > There's no test here, and I think we _could_ make a reliable one with
> > something like:
> >
> > 1. Have a proc-receive hook that signals via fifo that it's running,
> > then pauses indefinitely.
> >
> > 2. Start a push in the background, then wait on the fifo signal.
> >
> > 3. Kill receive-pack.
> >
> > But it's awfully complicated for little gain. I'm fine with not worrying
> > about it (and I did something similar manually to make to sure it works
> > as we expect).
>
> I actually tried doing exactly what you're proposing here yesterday. It
> caused hangs, lingering processes, you had to play games to not trigger
> the &&-chain linter due to the backgrounding via &, and ultimately it
> somehow just didn't work.
>
> So at some point I came to the same conclusion as you did, that it's
> just too complicated for little gain.
So I don't think we should apply this, because it's horrific. But
because I like a challenge, here is a test that should be race-free,
doesn't hang, and doesn't leave lingering processes (the blocking hook
waits for its parent to die).
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 3f81f16e133..ca6950ef433 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -300,4 +300,37 @@ test_expect_success 'receive-pack de-dupes .have lines' '
test_cmp expect refs
'
+test_expect_success PIPE 'receive-pack cleans up .keep after signal' '
+ git init --bare keep.git &&
+ write_script keep.git/hooks/proc-receive <<-\EOF &&
+ # tell the other side we are in the hook
+ echo running proc-receive >&9
+ # and then wait until our parent receive-pack exits
+ wait "$PPID"
+ EOF
+ git -C keep.git config receive.procReceiveRefs refs/ &&
+
+ mkfifo rpstatus &&
+ (
+ # this will fail because we kill receive-pack while it hangs,
+ # but we will not see its exit code until we "wait" below.
+ git push \
+ --receive-pack="echo \$\$ >&9; exec git-receive-pack" \
+ --all keep.git 9>rpstatus &
+ ) &&
+
+ exec 9<rpstatus &&
+ read <&9 rp_pid &&
+ read <&9 ready_signal &&
+ kill -15 "$rp_pid" &&
+ # in theory we get the signal code from receive-pack here,
+ # but we are racing with send-pack to call wait(), so we
+ # might also get "not found" from wait
+ { wait "$rp_pid"; OUT=$?; } &&
+ { test_match_signal 15 $OUT || test "$OUT" = 127; } &&
+
+ find keep.git/objects/pack -name "*.keep" >actual &&
+ test_must_be_empty actual
+'
+
test_done
It fails as expected without your patch (the .keep file is left in
"actual"), and passes with it.
-Peff
