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

From: Jeff King <peff@peff.net>
Date: Aug 14, 2017, 22:57 UTC
Message-ID: <20170814225716.p5airw6j5o6dszbn@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kbAp0p8-y-UpamU0J65A2P0B9r7KEWM0ZCBv=x4E77sGw@mail.gmail.com>

On Mon, Aug 14, 2017 at 03:28:50PM -0700, Stefan Beller wrote:

Show 10 quoted lines

> >> +.PHONY: style
> >> +style:
> >> +     git clang-format --style file --diff --extensions c,h
> >
> > Did we get "git clang-format" subcommand, or is "s/git //" implied
> > somewhere?
> 
> You need to have clang-format installed (debian/Ubuntu package, or
> however it is named in your distribution), which provides this command
> for us.

Sadly it is called git-clang-format-3.8, git-clang-format-5.0, etc, in the Debian packages. I think we need a CLANG_FORMAT variable that can be overridden.

I am surprised that there's no base "git-clang-format" symlink for the default version. There is for "clang-format" and "clang" themselves. So arguably this is a bug in the Debian packaging.

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.

-Peff