Re: [PATCH v2 2/2] Makefile: add style build rule

[Jeff King <peff@peff.net>]

On Mon, Aug 14, 2017 at 04:47:45PM -0700, Junio C Hamano wrote:

> > By the way, I do not know which vintage of /usr/bin/git-clang-format
> > I happen to have on my box, but I needed a crude workaround patch
> > (attached at the end) ...
> 
> I guess you hit the same thing while our messages crossing ;-)

Yep. Our solutions were at opposite ends of the spectrum, though. :)

> > As to what it does, the first example I tried may not have been a
> > great one.  I got this:
> >
> >         git clang-format --style file --diff --extensions c,h
> >         diff --git a/cache.h b/cache.h
> >         index 73e0085186..6462fe25bc 100644
> >         --- a/cache.h
> >         +++ b/cache.h
> >         @@ -1498,11 +1498,8 @@ struct checkout {
> >                 const char *base_dir;
> >                 int base_dir_len;
> >                 struct delayed_checkout *delayed_checkout;
> >         -	unsigned force:1,
> >         -		 quiet:1,
> >         -		 not_new:1,
> >         -		a_new_field:1,
> >         -		 refresh_cache:1;
> >         +	unsigned force : 1, quiet : 1, not_new : 1, a_new_field : 1,
> >         +		refresh_cache : 1;
> >          };
> >          #define CHECKOUT_INIT { NULL, "" }
> >  
> > which is not wrong per-se, but I have a mixed feelings.  I do not
> > want it to complain if the original tried to fit many items on a
> > single line, but if the original wanted to have one item per line,
> > I'd rather see it kept as-is.
> 
> To clarify, the above is after I added a_new_field that is one-bit
> wide without doing anything else.  I do not mind the checker
> complaining the existing force, quiet, etc. whose widths are all
> spelled without SP around ':', because they appear near-by, as a
> collateral damage.  My only gripe is that the result got squished
> into a single line.

Yes, agreed. My personal rule with a list like this is often "once you
have to start breaking across multiple lines, you should put one per
line". I don't know if there's a way to codify that in clang-format,
though.

The case I fed it (which is just nonsense I made up that does not fit
our style) also left me a bit confused at first, but I think it was
because the .clang-format parser was bailing as soon as it found an
unrecognized entry, but then formatting according to bogus rules. With
the original file from Brandon I got:

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 7e8371670b..8994450e0c 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -21,10 +21,7 @@ static int label_cb(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-static
-int foo(void* bar,int baz) {
-	/* nothing */
-}
+static int foo(void *bar, int baz) { /* nothing */ }
 
 int cmd_merge_file(int argc, const char **argv, const char *prefix)
 {

which is clearly not our style. And then after removing the entries I
mentioned elsewhere, I get:

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 7e8371670b..574ba6d86f 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -21,8 +21,8 @@ static int label_cb(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-static
-int foo(void* bar,int baz) {
+static int foo(void *bar, int baz)
+{
 	/* nothing */
 }

which looks right. So you might want to double-check that it was
respecting our settings, and there were no warnings to stderr.

-Peff