From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 27 Nov 2012 17:00:02 GMT
Subject: Re: [PATCH v6 p2 3/9] transport-helper: trivial code shuffle
Message-ID: <7vk3t7nfql.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1353727520-26039-4-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Just shuffle the die() part to make it more explicit, and cleanup the
> code-style.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  transport-helper.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 32ad877..0c95101 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -775,6 +775,9 @@ static int push_refs_with_export(struct transport *transport,
>  		char *private;
>  		unsigned char sha1[20];
>  
> +		if (ref->deletion)
> +			die("remote-helpers do not support ref deletion");
> +
>  		if (!data->refspecs)
>  			continue;

This is not just "just shuffle the die to make it explicit" but it
does change the semantics; earlier ref->deletion was perfectly fine
as long as data->refspecs is not given, but the new code always
dies.

If this semantic change is a good thing, please explain why it is so
in the log message.  If the change is "it does not matter because
when data->refspecs is not given and ref->deletion is set, we die
later elsewhere in the code anyway", then it needs to be described.


Thanks.

> @@ -784,10 +787,6 @@ static int push_refs_with_export(struct transport *transport,
>  		}
>  		free(private);
>  
> -		if (ref->deletion) {
> -			die("remote-helpers do not support ref deletion");
> -		}
> -
>  		if (ref->peer_ref)
>  			string_list_append(&revlist_args, ref->peer_ref->name);