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

From: Junio C Hamano <[email-masked]>
Date: Wed, 27 Apr 2016 08:26:38 -0700

Christian Couder <[email-masked]> writes:

> 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,...":

Don't.

If you have four things there, and two are related, you would do

    /* used for --distim option */
    int distim_weight;
    int distim_target;

    int unrelated;

    int another;

And the first two are shown clearly to be related and unrelated to
the other ones.  That sound nice in theory, but think what you would
do when you want to comment further on one of the related ones.  You
would likely do this:

    /* used for --distim option */
    int distim_weight;
   -int distim_target;
   +int distim_target; /* which dosh to distim? */

    int unrelated;

For the same reason, if you are to comment on one of the ungrouped
things, you would do this:

   -int unrelated;
   +int unrelated; /* this is not used for --distim */

    int another;

Notice that a blank between unrelated and another is not helping
anything?  There is no reason to keep blanks in between each and
every unrelated things.

See how "struct diff_options" is done for a better example.  A blank
line every once in a while that roups related things together is
good.  Blindly applying it to all groups of one defeats the benefit
by making the result harder to read.