Re: [PATCH 2/3] builtin/apply: make gitdiff_verify_name() return void

From: Christian Couder <christian.couder@gmail.com>
Date: Mar 22, 2016, 22:10 UTC
Message-ID: <CAP8UFD13ueLG1xdV7QYpiZLx9DZHgdrPcpi9zLc7NAh2BLd08w@mail.gmail.com>
In-Reply-To: <xmqq37rigq7o.fsf@gitster.mtv.corp.google.com>

On Tue, Mar 22, 2016 at 10:25 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

> Christian Couder <christian.couder@gmail.com> writes:
>
>> As the value returned by gitdiff_verify_name() is put into the
>> same variable that is passed as a parameter to this function,
>> it is simpler to pass the address of the variable and have
>> gitdiff_verify_name() change the variable itself.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>
>
> This change makes the function less useful by restricting the
> callers--only the ones that wants in-place update can call it after
> this change, while the old function signature allowed a caller to
> pass one variable as orig, receive the result in another variable
> (and probably compare them).
>
> It does not matter very much for this file scope static helper
> either way, and I would probably say the same thing if the patch
> were in reverse (i.e. if the patch were loosening the restriction),
> but I cannot offhand see why this is an improvement.  Puzzled...

In my opinion it is an improvement because:

- the feature of allowing a caller to pass one variable as orig and
receive the result in another is useless here and just makes it more
difficult to understand what the code is doing because you first have
to realize that in both calls the argument that is passed is also
assigned to, and

- using up the return value to return a result prevents from using it
to signal an error instead of die()ing, so it makes libifying the
function more difficult.

But if you prefer I can just resend a series with only patchs 1/3 and 3/3. And we can discuss later in the context of libifying "git apply" if this patch makes sense.