On Fri, Nov 15, 2024 at 01:15:33AM +0800, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx>
>
> 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/
> 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`?
> @@ -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.
> 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.
> 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.
> 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/
> + * 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
