Re: [PATCH 22/83] builtin/apply: move 'unsafe_paths' global into 'struct apply_state'

From: Christian Couder <christian.couder@gmail.com>
Date: Apr 27, 2016, 12:14 UTC
Message-ID: <CAP8UFD0Xx4d=751yhaHpRKUgDQ1G2FkuCEziwdZpVMiJbjyaMg@mail.gmail.com>
In-Reply-To: <xmqq37q8f748.fsf@gitster.mtv.corp.google.com>

On Tue, Apr 26, 2016 at 10:27 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 27 quoted lines

> Christian Couder <christian.couder@gmail.com> writes:
>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>>  builtin/apply.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> index 506357c..c45e481 100644
>> --- a/builtin/apply.c
>> +++ b/builtin/apply.c
>> @@ -57,6 +57,8 @@ struct apply_state {
>>       int unidiff_zero;
>>
>>       int update_index;
>> +
>> +     int unsafe_paths;
>>  };
>
> Having said
>
>     I like the way this series moves only a few variables at a time to
>     limit the scope of each step.
>
> it gets irritating to see all these unnecessary blank lines in the
> structure definition added by each step, which would mean all of
> these patches need to fix them in the next reroll.

The reason I added some blank lines is that I moved comments that were all in one block at the top. I moved each comment near the related variables and sometimes a comment is related to 2 variables, like in the extract below the comment that starts with "For "diff-stat" like behaviour,...":

------------------------------------------------------

    /* --index updates the cache as well. */
    int check_index;

    int unidiff_zero;

    int update_index;

    int unsafe_paths;

    int line_termination;

    /*
     * For "diff-stat" like behaviour, we keep track of the biggest change
     * we've seen, and the longest filename. That allows us to do simple
     * scaling.
     */
    int max_change;
    int max_len;

    /*
     * Various "current state", notably line numbers and what
     * file (and how) we're patching right now.. The "is_xxxx"
     * things are flags, where -1 means "don't know yet".
     */
    int linenr;

------------------------------------------------------

If I remove all the blank lines, I think it will make it harder to understand which comment belong to which variable(s).

Maybe a compromise would be to just remove blank lines between the variables that don't have any related comment like "unidiff_zero", "update_index", "unsafe_paths" and "line_termination" above.