git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Jeff King <peff@peff.net>
To: Ramkumar Ramachandra <artagnon@gmail.com>
Cc: Git List <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>, René Scharfe <l.s.r@web.de>
Subject: Re: [PATCH v2] .clang-format: introduce the use of clang-format
Date: Wed, 21 Jan 2015 17:09:03 -0500
Message-ID: <20150121220903.GA10267@peff.net> (raw)
In-Reply-To: <CALkWK0knxJ5VTJoKhR_t4GS7pfg6PPYox9Srf3bvaX=m+sjqVw@mail.gmail.com>

On Wed, Jan 21, 2015 at 04:28:00PM -0500, Ramkumar Ramachandra wrote:

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

> >   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/,'

      reply index

Thread overview: 7+ messages in thread (expand / mbox.gz / Atom feed / [top])
2015-01-17 21:30 [PATCH] " Ramkumar Ramachandra
2015-01-18 11:31 ` René Scharfe
2015-01-21 17:06   ` Ramkumar Ramachandra
2015-01-21 17:01 ` [PATCH v2] " Ramkumar Ramachandra
2015-01-21 20:45   ` Jeff King
2015-01-21 21:28     ` Ramkumar Ramachandra
2015-01-21 22:09       ` Jeff King [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150121220903.GA10267@peff.net \
    --to=peff@peff.net \
    --cc=artagnon@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox