Re: [PATCH v2 22/94] builtin/apply: move 'threeway' global into 'struct apply_state'

From: Christian Couder <christian.couder@gmail.com>
Date: May 12, 2016, 20:26 UTC
Message-ID: <CAP8UFD1VYW6+T_oTHJbemUhi8MPrMseaObWgoy0Xou=VCoKt-A@mail.gmail.com>
In-Reply-To: <xmqqoa8bdpym.fsf@gitster.mtv.corp.google.com>

On Thu, May 12, 2016 at 9:41 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

> Christian Couder <christian.couder@gmail.com> writes:
>
>> To libify the apply functionality the 'threeway' variable should
>> not be static and global to the file. Let's move it into
>> 'struct apply_state'.
>>
>> Reviewed-by: Stefan Beller <sbeller@google.com>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>>  builtin/apply.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> index 6216723..3650922 100644
>> --- a/builtin/apply.c
>> +++ b/builtin/apply.c
>> @@ -40,6 +40,7 @@ struct apply_state {
>>       int numstat;
>>
>>       int summary;
>> +     int threeway;
>
> This makes threeway look as if it is one of the cosmetic options
> like stat/numstat/summary, doesn't it?  If anything, the blank
> between numstat and summary should be moved below summary.

There is a blank because there is a comment on the line before numstat. The result looks like this:

    /* --cached updates only the cache without ever touching the
working tree. */
    int cached;

    /* --stat does just a diffstat, and doesn't actually apply */
    int diffstat;

    /* --numstat does numeric diffstat, and doesn't actually apply */
    int numstat;

    int summary;
    int threeway;
    int no_add;

Show 5 quoted lines

>  I'd
> group knobs that affect "what is affected (check, check_index,
> cached, etc.)", "how the application is done (allow-overlap,
> threeway, in-reverse, etc.)" and "cosmetics" and place the ones in
> the same group next to each other.

Ok, I will try to group knobs like that, but the comments tend to break the groups.