From: Christian Couder Date: Thu, 12 May 2016 20:26:34 GMT Subject: Re: [PATCH v2 22/94] builtin/apply: move 'threeway' global into 'struct apply_state' Message-ID: In-Reply-To: On Thu, May 12, 2016 at 9:41 PM, Junio C Hamano wrote: > Christian Couder 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 >> Signed-off-by: Christian Couder >> --- >> 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; > 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.