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

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

Thiago Farina <tfransosi@gmail.com> writes:

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

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