Re: [PATCH v2] .clang-format: introduce the use of clang-format
- From
Jeff King <peff@peff.net>
- Date
- Jan 21, 2015, 22:09 UTC
- Message-ID
- <20150121220903.GA10267@peff.net>
- In-Reply-To
- <CALkWK0knxJ5VTJoKhR_t4GS7pfg6PPYox9Srf3bvaX=m+sjqVw@mail.gmail.com>
On Wed, Jan 21, 2015 at 04:28:00PM -0500, Ramkumar Ramachandra wrote:
Show 6 quoted lines
> > So overall I think it has some promise, but I do not think it is quite > > flexible enough yet for us to use day-to-day. > > The big negative is that it will probably never be. I'll try to look > at the larger issues later this week, if you can compromise on the > fine details that are probably too hard to fix.
The key to me is that we do not have necessarily take every suggestion the tool makes. So it does not have to be perfect, just "pretty good".
But...I think it is not quite so simple. The clang-format-diff script (and git-clang-format) _do_ seem to operate on more than just the lines I've changed. I'm not clear on whether they're examining the whole file (just with the patches applied), or there's something in between happening.
So rejecting the tool's suggestion one day may mean it suggests the same change to you any other time you touch nearby parts of the file, which could be annoying.
Show 9 quoted lines
> > 2. It can operate on patches (and generates patches for you to apply! > > You could add a git-add--interactive mode to selectively take its > > suggestions). > > There's a git-clang-format in the $CLANG_ROOT/tools/clang-format/. I do: > > $ g cf @~ > > ... with the appropriate aliases.
Neat. Debian's package does not ship with that. I hacked-in very rudimentary interactive-add support for clang-format-diff (below) before getting your response. It would be better built around "git-clang-format --diff" (though that script would need to be taught to do the right thing with the --color argument).
However, because of the "suggest the same change" thing I mentioned above, I am not sure whether interactively selecting is a good idea or not. You might end up having to say "no" to the same suggestions a lot.
Anyway, here it is, for reference. You can use it like:
git add--interactive --patch=format --
and you could probably even stick an "exec" line into an interactive rebase to go through and fixup individual patches in a whole series.
---
diff --git a/.gitignore b/.gitignore index a052419..6f5b815 100644 --- a/.gitignore +++ b/.gitignore @@ -30,6 +30,7 @@ /git-checkout-index /git-cherry /git-cherry-pick +/git-clang-format-diff /git-clean /git-clone /git-column diff --git a/Makefile b/Makefile index c44eb3a..113534e 100644 --- a/Makefile +++ b/Makefile @@ -455,6 +455,7 @@ unexport CDPATH SCRIPT_SH += git-am.sh SCRIPT_SH += git-bisect.sh +SCRIPT_SH += git-clang-format-diff.sh SCRIPT_SH += git-difftool--helper.sh SCRIPT_SH += git-filter-branch.sh SCRIPT_SH += git-merge-octopus.sh diff --git a/git-add--interactive.perl b/git-add--interactive.perl index c725674..fd83adf 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -167,6 +167,16 @@ my %patch_modes = ( FILTER => undef, IS_REVERSE => 0, }, + 'format' => { + DIFF => 'clang-format-diff', + APPLY => sub { apply_patch_for_checkout_commit '', @_ }, + APPLY_CHECK => 'apply', + VERB => 'Apply', + TARGET => 'to index and worktree', + PARTICIPLE => 'applying', + FILTER => undef, + IS_REVERSE => 0 + }, ); my %patch_mode_flavour = %{$patch_modes{stage}}; @@ -1591,6 +1601,15 @@ sub process_args { 'checkout_head' : 'checkout_nothead'); $arg = shift @ARGV or die "missing --"; } + } elsif ($1 eq 'format') { + $patch_mode = $1; + $arg = shift @ARGV or die "missing --"; + if ($arg eq '--') { + $patch_mode_revision = 'HEAD^'; + } else { + $patch_mode_revision = $arg; + $arg = shift @ARGV or die "missing --"; + } } elsif ($1 eq 'stage' or $1 eq 'stash') { $patch_mode = $1; $arg = shift @ARGV or die "missing --"; diff --git a/git-clang-format-diff.sh b/git-clang-format-diff.sh new file mode 100755 index 0000000..9351883 --- /dev/null +++ b/git-clang-format-diff.sh @@ -0,0 +1,20 @@ +#!/bin/sh + +# This is what it's called in the Debian package, but it seems +# like there ought to be a symlink without the version... +CFD=clang-format-diff-3.6 + +# Strip out --color, as clang's patch reader cannot handle it. +# Robustly handling arrays in bourne shell is insane. +eval "set -- $( + for i in "$@"; do + test "--color" = "$i" && continue + printf " '" + printf '%s' "$i" | sed "s/'/'\\\\''/g" + printf "'" + done +)" + +git diff-index -p "$@" | +$CFD -p1 | +sed -e 's,^--- ,&a/,' -e 's,^+++ ,&b/,'