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

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

quoted

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. :)

quoted

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