Re: [PATCH 2/4] diff.c: return filepair from diff_unmerge()

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 25, 2011, 01:18 UTC
Message-ID: <7vtydnku1a.fsf@alter.siamese.dyndns.org>
In-Reply-To: <BANLkTi=fnzRVc8Lx_Wba4yqa7u8t3xGAtQ@mail.gmail.com>

Thiago Farina <tfransosi@gmail.com> writes:

Show 11 quoted lines

> Some unrelated style comments below.
> ...
>> -void diff_unmerge(struct diff_options *options,
>> -                 const char *path,
>> -                 unsigned mode, const unsigned char *sha1)
>> +struct diff_filepair *diff_unmerge(struct diff_options *options,
>> +                                  const char *path,
>> +                                  unsigned mode, const unsigned char *sha1)
>>  {
>
> While you are here, why not write one arg per line?

No, actually I reindented the two lines by accident; I didn't mean to.

Show 16 quoted lines

>> diff --git a/diff.h b/diff.h
>> index bf2f44d..f51a8ee 100644
>> --- a/diff.h
>> +++ b/diff.h
>> @@ -209,7 +209,7 @@ extern void diff_change(struct diff_options *,
>>                        const char *fullpath,
>>                        unsigned dirty_submodule1, unsigned dirty_submodule2);
>>
>> -extern void diff_unmerge(struct diff_options *,
>> +extern struct diff_filepair *diff_unmerge(struct diff_options *,
>
> While you are here, why not add the argument name |options| here too?
>
>>                         const char *path,
>>                         unsigned mode,
>>                         const unsigned char *sha1);

The patch was meant to be minimum to begin with. Besides, there is no risk of confusion from not naming the option in this case, so even if we were shooting for clean-up, we wouldn't be adding such noise here. The other parameters do benefit from descriptive names, so there is no need to remove the names either.

In other case, your "why not" is totally unwarranted for this hunk.