Re: [PATCH 2/5] send-pack.c: add an --atomic-push command line argument

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 15, 2014, 21:01 UTC
Message-ID: <xmqqsiggd3vk.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1418673368-20785-3-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> -static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args)
> +static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args, int *atomic_push_failed)

Hmph. Is "atomic push" so special that it deserves a separate parameter? When we come up with yet another mode of failure, would we add another parameter to the callers to this function?

Show 8 quoted lines

> @@ -203,6 +203,12 @@ static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_a
>  	case REF_STATUS_REJECT_NEEDS_FORCE:
>  	case REF_STATUS_REJECT_STALE:
>  	case REF_STATUS_REJECT_NODELETE:
> +		if (atomic_push_failed) {
> +			fprintf(stderr, "Atomic push failed for ref %s. "
> +				"Status:%d\n", ref->name, ref->status);
> +			*atomic_push_failed = 1;

All other error message that come from the codepaths around here seem to avoid uppercase. Also maybe you want to use error() here?

> +	if (args->use_atomic_push && !atomic_push_supported) {
> +		fprintf(stderr, "Server does not support atomic-push.");
> +		return -1;
> +	}

This check logically belongs to the previous step, no?

> +	atomic_push = atomic_push_supported && args->use_atomic_push;

Show 32 quoted lines

>  
>  	if (status_report)
>  		strbuf_addstr(&cap_buf, " report-status");
> @@ -365,7 +376,8 @@ int send_pack(struct send_pack_args *args,
>  	 * the pack data.
>  	 */
>  	for (ref = remote_refs; ref; ref = ref->next) {
> -		if (!ref_update_to_be_sent(ref, args))
> +		if (!ref_update_to_be_sent(ref, args,
> +			args->use_atomic_push ? &atomic_push_failed : NULL))
>  			continue;
>  
>  		if (!ref->deletion)
> @@ -377,6 +389,23 @@ int send_pack(struct send_pack_args *args,
>  			ref->status = REF_STATUS_EXPECTING_REPORT;
>  	}
>  
> +	if (atomic_push_failed) {
> +		for (ref = remote_refs; ref; ref = ref->next) {
> +			if (!ref->peer_ref && !args->send_mirror)
> +				continue;
> +
> +			switch (ref->status) {
> +			case REF_STATUS_EXPECTING_REPORT:
> +				ref->status = REF_STATUS_ATOMIC_PUSH_FAILED;
> +				continue;
> +			default:
> +				; /* do nothing */
> +			}
> +		}
> +		fprintf(stderr, "Atomic push failed.");
> +		return -1;

The same comment as the other fprintf() applies here.

Show 7 quoted lines

> @@ -728,6 +728,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
>  						 ref->deletion ? NULL : ref->peer_ref,
>  						 "remote failed to report status", porcelain);
>  		break;
> +	case REF_STATUS_ATOMIC_PUSH_FAILED:
> +		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
> +						 "atomic-push-failed", porcelain);

Why dashed-words-here?

> +		break;
>  	case REF_STATUS_OK:
>  		print_ok_ref_status(ref, porcelain);
>  		break;