Re: [PATCHv2 3/6] send-pack.c: add --atomic command line argument

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 16, 2014, 19:31 UTC
Message-ID: <xmqq1tnzbddu.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1418755747-22506-3-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 6 quoted lines

>      * Now we only need a very small change in the existing code and have
>        a new static function, which cares about error reporting.
>     	  Junio wrote:
>     	  > 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?

I suspect that you completely mis-read me.

If you add an extra arg that is *specifically* for atomic push, like this:

    -static int update_to_send(...);
    +static int update_to_send(..., int *atomic_push_failed);
    
what signal does it send to the next person who may want to add
a new "nuclear push" option?  Should the patch look like

    -static int update_to_send(..., int *atomic_push_failed);
    +static int update_to_send(..., int *atomic_push_failed, int *nuclear_push_failed);

by adding yet another separate variable for error reporting?

I would have understood if the new variable was named something like "failure_reason", which may be set to PUSH_FAILURE_ATOMIC when an atomic push failure was detected. Making the helper return the reason why the push failed would be another way, like you did in the 2/6 patch in this round.

Either way, the next person would know to add a code to do his "nuclear push" and set the reason to PUSH_FAILURE_NUCLEAR when it fails, instead of piling yet another error reporting variable that is specific to the feature.

This is all about code maintainability, which is very different from "who cares about error reporting".

Show 17 quoted lines

> diff --git a/remote.h b/remote.h
> index 8b62efd..f346524 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -115,7 +115,8 @@ struct ref {
>  		REF_STATUS_REJECT_SHALLOW,
>  		REF_STATUS_UPTODATE,
>  		REF_STATUS_REMOTE_REJECT,
> -		REF_STATUS_EXPECTING_REPORT
> +		REF_STATUS_EXPECTING_REPORT,
> +		REF_STATUS_ATOMIC_PUSH_FAILED
>  	} status;
> ...
>  	for (ref = remote_refs; ref; ref = ref->next) {
> -		if (no_ref_update_to_be_sent(ref, args))
> +		int reject_reason;
> +		if ((reject_reason = no_ref_update_to_be_sent(ref, args))) {

We avoid assignment inside a conditional.

Show 8 quoted lines

> +			/* When we know the server would reject a ref update if
> +			 * we were to send it and we're trying to send the refs
> +			 * atomically, abort the whole operation */
> +			if (use_atomic && reject_reason == 2)
> +				return atomic_push_failure(args,
> +							   remote_refs,
> +							   ref);
>  			continue;

Perhaps

		switch (check_to_send_update(...)) {
                case 0: /* no error */
                	break;
		case -REF_STATUS_ATOMIC_PUSH_FAILED:                        
                	return atomic_push_failure(args, remote_refs, ref);
                	break;
		default:
			continue;
		}

or something?