Re: [PATCH v19 03/10] receive-pack: add new proc-receive hook

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Thu, Jan 21, 2021 at 10:21:36AM +0800, Jiang Xin wrote:
> SZEDER Gábor <szeder.dev@gmail.com> 于2021年1月20日周三 下午8:29写道：
> >
> > On Mon, Jan 18, 2021 at 04:24:11PM +0800, Jiang Xin wrote:
> > > SZEDER Gábor <szeder.dev@gmail.com> 于2021年1月18日周一 上午6:21写道：
> > > >
> > > >
> > > > This patch added a whole lot of test cases, and two of them '86 -
> > > > proc-receive: not support push options (builtin protocol)' and '95 -
> > > > proc-receive: not support push options (builtin protocol/porcelain)'
> > > > are prone to rare failures.
> > > >
> > > > On Thu, Aug 27, 2020 at 11:45:44AM -0400, Jiang Xin wrote:
> > > > > diff --git a/t/t5411/test-0026-push-options.sh b/t/t5411/test-0026-push-options.sh
> > > > > new file mode 100644
> > > > > index 0000000000..d0c4da8b23
> > > > > --- /dev/null
> > > > > +++ b/t/t5411/test-0026-push-options.sh
> > > >
> > > > > +# Refs of upstream : master(A)
> > > > > +# Refs of workbench: master(A)  tags/v123
> > > > > +# git push -o ...  :                       refs/for/master/topic
> > > > > +test_expect_success "proc-receive: not support push options ($PROTOCOL)" '
> > > > > +     test_must_fail git -C workbench push \
> > > > > +             -o issue=123 \
> > > > > +             -o reviewer=user1 \
> > > > > +             origin \
> > > > > +             HEAD:refs/for/master/topic \
> > > > > +             >out 2>&1 &&
> > > >
> > > > Three relevant things are happening here:
> > > >
> > > >   - 'git push' is executed with its standard output and error
> > > >     redirected to the file 'out'.
> > > >
> > > >   - 'git push' executes 'git receive-pack' internally, which inherits
> > > >     the open file descriptors, so its output and error goes into that
> > > >     same 'out' file.
> > > >
> > > >   - 'git push' is expected to fail when it finds out that the other
> > > >     side doesn't support push options, but it does so with a simple
> > > >     die() right away, without waiting for its child 'git receive-pack'
> > > >     process to finish.
> > > >
> > > > > +     make_user_friendly_and_stable_output <out >actual &&
> > > > > +     test_i18ngrep "fatal: the receiving end does not support push options" \
> > > > > +             actual &&
> > > > > +     git -C "$upstream" show-ref >out &&
> > > >
> > > > Here the shell opens and truncates the file 'out' to write 'git
> > > > show-ref's output, i.e. it is still the same 'out' file that was used
> > > > earlier.
> > > >
> > > > Consequently, it is possible that 'git receive-pack' is still running,
> > > > its open file descriptors to 'out' are still valid, and its "fatal:
> > > > the remote end hung up unexpectedly" error message about the suddenly
> > > > disappeared 'git push' can partially overwrite the output from 'git
> > > > show-ref'.
> >
> >
> > > > I think these are the only two tests that can cause this racy
> > > > behavior: by instrumenting finish_command() I found that in all other
> > > > tests where 'git push' is expected to fail it errors out gracefully
> > > > and waits for its 'git receive-pack' child process.
> > >
> > > Atomic push may have the same problem.
> >
> > I don't think so, because send_pack() doesn't die() when a ref is
> > rejected in an atomic push, but returns, and lets its caller terminate
> > in an usual way, including waiting for 'git receive-pack'.
> 
> I find many places where the client side will die() before closing the
> service side gracefully:
> 
>  + In `transport_push()`, if fail to push a submodule, will die().
>  + In `git_transport_push()`, will die() for an unimplemented v2 protocol.
>  + In `send_pack()`, will die() if
>     the server side has an incompatible hash algorithm, or
>     the receiving end does not support --signed push, or
>     the receiving end does not support --atomic push, or
>     the receiving end does not support push options

Sure, but it seems that those are not covered in thet 5411.

You can build Git with the patch below, run t5411 with '-V -x', and
then look through all the cases to see where 'git push' waited or did
not wait for its 'git receive-pack' child process.

diff --git a/run-command.c b/run-command.c
index ea4d0fb4b1..4accdb343e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -988,6 +988,7 @@ int start_command(struct child_process *cmd)
 int finish_command(struct child_process *cmd)
 {
 	int ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);
+	dprintf(3, "finish_command(): '%s'\n", cmd->argv[0]);
 	trace2_child_exit(cmd, ret);
 	child_process_clear(cmd);
 	return ret;