From: Jeff King <peff@peff.net>
Date: Fri, 05 Feb 2010 19:56:44 GMT
Subject: Re: [PATCH 3/3] make git push --dry-run --porcelain exit with status 0 even if updates will be rejected
Message-ID: <20100205195644.GE24474@coredump.intra.peff.net>
In-Reply-To: <1265398462-17316-3-git-send-email-larry@elder-gods.org>

On Fri, Feb 05, 2010 at 02:34:22PM -0500, Larry D'Anna wrote:

> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index 76c7206..dfd7470 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -478,6 +478,11 @@ int send_pack(struct send_pack_args *args,
>  		return ret;
>  	for (ref = remote_refs; ref; ref = ref->next) {
>  		switch (ref->status) {
> +		case REF_STATUS_REJECT_NONFASTFORWARD:
> +		case REF_STATUS_REJECT_NODELETE:
> +			if (args->porcelain && args->dry_run)
> +				break;
> +			return -1;
>  		case REF_STATUS_NONE:
>  		case REF_STATUS_UPTODATE:
>  		case REF_STATUS_OK:

Why just these two status flags? Based on your reasoning elsewhere, I
would assume the logic should be:

  - if we had some transport-related error, return failure

  - if not, then return success, as any ref's failure is already
    indicated in the porcelain output

So shouldn't it just be:

  if (args->porcelain && args->dry_run)
          return 0;

after we check for transport errors but before the loop that you are
modifying.

> -static int push_had_errors(struct ref *ref)
> +static int push_had_errors(struct ref *ref, int flags)
>  {
>  	for (; ref; ref = ref->next) {
>  		switch (ref->status) {
> +		case REF_STATUS_REJECT_NONFASTFORWARD:
> +		case REF_STATUS_REJECT_NODELETE:
> +			if (flags & TRANSPORT_PUSH_DRY_RUN && flags & TRANSPORT_PUSH_PORCELAIN)
> +				break;
> +			else
> +				return 1;

Ditto here.

-Peff