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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 14, 2017, 23:29 UTC
Message-ID: <xmqqzib1sp6z.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170814225716.p5airw6j5o6dszbn@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 11 quoted lines

> I suspect the "-p" version is going to be the one people invoke the most
> often. Should it take the coveted "make style" slot, and the diff get
> pushed off to another target?
>
> I was also confused at first that the "-p" version requires you to stage
> the changes first. I don't know if we can make that less confusing via a
> "make style". Or if it's just something people would get used to. But
> sadly it makes the command not-quite orthogonal to "make test" in the
> workflow. You can't "make style && make test && git add -p".  You have
> to add first, then check style, then you'd want to test that result to
> make sure it didn't change the meaning of the code.

Perhaps.

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) to get it even run. The first thing it does is to call load_git_config() and it barfs because I have boolean configuration variables set to true in the correct way, which it does not seem to recognise.

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.

Anyway, we cannot have perfect checker from the day one, and considering this is an initial attempt, I'd say it is a good start.

Thanks.

diff --git a/git-clang-format b/git-clang-format
index 60cd4fb25b..e8429b2750 100755
--- a/usr/bin/git-clang-format
+++ b/usr/local/google/home/jch/g/Ubuntu-14.04-x86_64/gitstuff/bin/git-clang-format
@@ -191,10 +191,13 @@ def load_git_config(non_string_options=None):
   out = {}
   for entry in run('git', 'config', '--list', '--null').split('\0'):
     if entry:
-      name, value = entry.split('\n', 1)
-      if name in non_string_options:
-        value = run('git', 'config', non_string_options[name], name)
-      out[name] = value
+      if '\n' in entry:
+        name, value = entry.split('\n', 1)
+        if name in non_string_options:
+          value = run('git', 'config', non_string_options[name], name)
+        out[name] = value
+      else:
+	out[entry] = "true";
   return out