Re: [PATCH 2/2] transport: don't ignore git-receive-pack(1) exit code on atomic push

[Jiang Xin <worldhello.net@gmail.com>]

On Wed, Nov 13, 2024 at 7:25 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> When executing git-push(1) with the "--porcelain" flag, then we will
> print updated references in a machine-readable format that looks like
> this:
>
>     To destination
>     =   refs/heads/noop:refs/heads/noop [up to date]
>     !   refs/heads/rejected:refs/heads/rejected [rejected] (atomic push failed)
>     !   refs/heads/noff:refs/heads/(off (non-fast-forward)
>     Done
>
> The final "Done" stanza was introduced via 77555854be (git-push: make
> git push --porcelain print "Done", 2010-02-26), where it was printed
> "unless any errors have occurred". This behaviour was later changed via
> 7dcbeaa0df (send-pack: fix inconsistent porcelain output, 2020-04-17)
> such that the stanza will also be printed when there was an error with
> atomic pushes, which was previously inconsistent with non-atomic pushes.
> The fixup commit has introduced a new issue though. During atomic pushes
> it is expected that git-receive-pack(1) may exit early, and that causes
> us to receive an error on the client-side. We (seemingly) still want to
> print the "Done" stanza, but given that we only do so when the push has
> been successful we started to ignore the error code by the child process
> completely when doing an atomic push.

I introduced the commit 7dcbeaa0df (send-pack: fix inconsistent porcelain
output, 2020-04-17), because the porcelain output for git push over local
file protocol and HTTP protocol are different and was hard to write the
some test cases to work for both protocols. I acknowledge the patch was
not perfect.

I read the commit 77555854be (git-push: make git push --porcelain
print "Done", 2010-02-26) and the code path of git push over two
protocols a second time, and find something:

The code snippet from 77555854be:

> -    ret = transport->push_refs(transport, remote_refs, flags);
> +    push_ret = transport->push_refs(transport, remote_refs, flags);
>      err = push_had_errors(remote_refs);
> -
> -    ret |= err;
> +    ret = push_ret | err;
>

The return code "push_ret" of push_refs() from different transport
vtable is not consist. For HTTP protocol, "push_ret" is zero if no
connection error or no protocol errors. So we should consider
“push_ret" as a protocol error rather than a reference update error.

If we want to print "Done" in porcelain mode when there are no
errors. (In dry run mode, ref-update-errors should not be
considered as errors, but the opposite should be regarded as errors).

Instead of using the following code,

> +    if (porcelain && !push_ret)
> +         puts("Done");

We can use like this ("pretend" is the flag for dry-run mode):

+    ret = pretend ? push_ret : (push_ret | err);
+    if (porcelain && !ret)
+         puts("Done");

I will send patches follow this patch series.

--
Jiang Xin