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

From: Jiang Xin <worldhello.net@gmail.com>
Date: Nov 14, 2024, 08:57 UTC
Message-ID: <CANYiYbHGdJsTgdzJj1r4sPdTAcZCf3C-qA538fcXbD1jjB1BUw@mail.gmail.com>
In-Reply-To: <20241113-pks-push-atomic-respect-exit-code-v1-2-7965f01e7f4e@pks.im>

On Wed, Nov 13, 2024 at 7:25 PM Patrick Steinhardt <ps@pks.im> wrote:

Show 23 quoted lines

>
> 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:

Show 7 quoted lines

> -    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