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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

>      * 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".

> 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.

> +			/* 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?