Re: [PATCH v6 p2 3/9] transport-helper: trivial code shuffle

[Felipe Contreras <felipe.contreras@gmail.com>]

On Tue, Nov 27, 2012 at 6:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

refspecs are optional, but when they are not present the code doesn't
work at all. This patch changes the behavior that was totally broken
anyway.

-- 
Felipe Contreras