Re: [PATCH v2 2/6] push: fix the behavior of the Done message for porcelain
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Nov 25, 2024, 08:26 UTC
- Message-ID
- <Z0Q0tY8y58yNivFR@pks.im>
- In-Reply-To
- <9dfea9953c2a671600beebbaa0692a1e68afea34.1731603991.git.zhiyou.jx@alibaba-inc.com>
On Fri, Nov 15, 2024 at 01:15:33AM +0800, Jiang Xin wrote:
Show 17 quoted lines
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com> > > 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), with the following > behaviors: > > - Show a "Done" porcelain message if there are no errors.
It would be great to update our documentation so that we spell out when exactly the user can expect such a "Done" message.
> - Fail to update a ref in a --dry-run does not count as an error. > - Actual rejections in non --dry-run pushes do count as errors. > - Return a non-zero exit code if there are errors.
> However, the behavior of the "Done" message is not consistent when > pushing with different protocols. This is because the return values of > transport->vtable->hush_refs() across different protocols are
s/hush_refs/push_refs/
Show 13 quoted lines
> diff --git a/send-pack.c b/send-pack.c
> index 6677c44e8a..4845f63737 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -630,7 +630,7 @@ int send_pack(struct send_pack_args *args,
> reject_atomic_push(remote_refs, args->send_mirror);
> error("atomic push failed for ref %s. status: %d",
> ref->name, ref->status);
> - ret = args->porcelain ? 0 : -1;
> + ret = (args->porcelain && args->dry_run) ? 0 : -1;
> goto out;
> }
> /* else fallthrough */This feels weird to me. Why would we ignore an error with `--dry-run`? Doesn't that mean that the resulting behaviour is now different depending on whether or not you pass `--dry-run`?
Show 17 quoted lines
> @@ -760,11 +760,12 @@ int send_pack(struct send_pack_args *args,
>
> if (ret < 0)
> goto out;
> -
> - if (args->porcelain) {
> - ret = 0;
> + else if (args->porcelain && args->dry_run)
> + /*
> + * Knowing a ref will be rejected in a --dry-run does not
> + * count as an error.
> + */
> goto out;
> - }
>
> for (ref = remote_refs; ref; ref = ref->next) {
> switch (ref->status) {Same question here.
Show 11 quoted lines
> diff --git a/t/t5411/test-0001-standard-git-push--porcelain.sh b/t/t5411/test-0001-standard-git-push--porcelain.sh > index 373ec3d865..5ff901454a 100644 > --- a/t/t5411/test-0001-standard-git-push--porcelain.sh > +++ b/t/t5411/test-0001-standard-git-push--porcelain.sh > @@ -73,7 +73,6 @@ test_expect_success "non-fast-forward git-push ($PROTOCOL/porcelain)" ' > > To <URL/of/upstream.git> > > <COMMIT-B>:refs/heads/next <COMMIT-A>..<COMMIT-B> > > ! refs/heads/main:refs/heads/main [rejected] (non-fast-forward) > - > Done > EOF > test_cmp expect actual &&
77555854be (git-push: make git push --porcelain print "Done", 2010-02-26) seems to cover this case, where it mentions that a conflicting ref update does not count as error with "--dry-run", but does count as error without that option. And this change here seems to be in line with that.
Whether the documented behaviour of that commit is reasonable is a different question though. I find it to be of dubious nature to make any runtime behaviour (other than committing the actual change) depend on "--dry-run". The behaviour should be the exact same, as otherwise the dry run becomes useless if the behaviour differs.
Show 12 quoted lines
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index 331778bd42..b133ab6ffc 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -1207,7 +1206,7 @@ test_expect_success 'push --porcelain --dry-run rejected' ' > echo >>.git/foo "! refs/heads/main^:refs/heads/main [rejected] (non-fast-forward)" && > echo >>.git/foo "Done" && > > - test_must_fail git push >.git/bar --porcelain --dry-run testrepo refs/heads/main^:refs/heads/main && > + git push >.git/bar --porcelain --dry-run testrepo refs/heads/main^:refs/heads/main && > test_cmp .git/foo .git/bar > '
Huh. So this is the dubious part I mean. It seems like the push would fail without "--dry-run", but because we pass it it does not indicate that failure via the exit code. This behaviour does not make any sense to me. There are more cases in your change where we stop failing now.
Show 11 quoted lines
> diff --git a/transport.c b/transport.c > index 47fda6a773..9e03a7148c 100644 > --- a/transport.c > +++ b/transport.c > @@ -1486,7 +1486,18 @@ int transport_push(struct repository *r, > } else > push_ret = 0; > err = push_had_errors(remote_refs); > - ret = push_ret | err; > + /* > + * The return values of transport->vtable->hush_refs() across
s/hush_refs/push_refs/
Show 22 quoted lines
> + * different protocols are inconsistent. For the HTTP protocol,
> + * the return value is zero when there are no connection errors
> + * or protocol errors. We should reference the return code of
> + * push_had_errors() to check for failures in updating references.
> + * Since failing to update a reference in a --dry-run does not
> + * count as an error, we could ignore the result of
> + * push_had_errors() when both --porcelain and --dry-run options
> + * are set.
> + */
> + ret = (porcelain && pretend) ? push_ret : (push_ret | err);
>
> if (!quiet || err)
> transport_print_push_status(transport->url, remote_refs,
> @@ -1503,7 +1514,7 @@ int transport_push(struct repository *r,
> transport_update_tracking_ref(transport->remote, ref, verbose);
> }
>
> - if (porcelain && !push_ret)
> + if (porcelain && !ret)
> puts("Done");
> else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
> /* stable plumbing output; do not modify or localize */I'm not yet convinced that it makes sense to ignore either `ret` or `push_ret` in the above two hunks. Making this dependent on `porcelain` may be okay if we want to report errors via the output, but making the behaviour dependent on whether or not we perform a dry run continues to feel wrong to me.
Patrick