[PATCH 00/10] [RFC] pickaxe for function names

[PATCH 00/10] [RFC] pickaxe for function names

[David A. Dalrymple (and Bhushan G. Lodha)]

This series introduces a --function-name=<pattern> option for git-log, intended
to search for commits which touch a function matching a certain pattern (a
feature we've seen requested and are interested in using ourselves).

This is our first attempt to patch git; we've tried to observe and follow the
community standards, but we would greatly appreciate feedback. We've been
working on this for a few weeks, and I just noticed that René Scharfe has done
conflicing (and better) refactoring work in diffcore-pickaxe.c a few days ago.
We'd be happy to rebase our changes and resolve the conflicts once René's
patches are committed to master, but we thought we may as well ask for comments
on the version we have working now.

Thanks for your time!

  .gitattributes: specify the language used
  diffcore-pickaxe.c: refactor regex compilation
  diffcore-pickaxe.c: Refactor pickaxe_fn signature
  diff.c/diff.h: expose userdiff_funcname
  diffcore-pickaxe.c: set up funcname pattern
  log: --function-name pickaxe
  xdiff: add XDL_EMIT_MOREFUNCNAMES to try harder
  xdiff: add XDL_EMIT_MOREHUNKHEADS to split hunks
  t4213: test --function-name option
  Documentation: Document --function-name usage

 .gitattributes                 |   2 +-
 Documentation/diff-options.txt |   9 +++
 Documentation/gitdiffcore.txt  |  17 ++++-
 builtin/log.c                  |   2 +-
 diff.c                         |  13 +++-
 diff.h                         |   3 +
 diffcore-pickaxe.c             | 162 +++++++++++++++++++++++++++++++++++-----------
 revision.c                     |   3 +-
 t/t4213-log-function-name.sh   |  73 +++++++++++++++++++++
 xdiff/xdiff.h                  |   2 +
 xdiff/xdiffi.c                 |   2 +-
 xdiff/xemit.c                  |  99 ++++++++++++++++++++++------
 xdiff/xemit.h                  |   4 +-
 13 files changed, 323 insertions(+), 68 deletions(-)

[PATCH 01/10] .gitattributes: specify the language used

[David A. Dalrymple (and Bhushan G. Lodha)]

From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>

Since git can intelligently emit diff hunk headers based on the
programming language of each file, assuming that the language is
specified in .gitattributes, it makes sense to specify our own
language (cpp) in our own .gitattributes file.

Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
---
 .gitattributes | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitattributes b/.gitattributes
index 5e98806..320e33c 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -1,3 +1,3 @@
 * whitespace=!indent,trail,space
-*.[ch] whitespace=indent,trail,space
+*.[ch] whitespace=indent,trail,space diff=cpp
 *.sh whitespace=indent,trail,space
-- 
1.7.12.4 (Apple Git-37)

[PATCH 02/10] diffcore-pickaxe.c: refactor regex compilation

[David A. Dalrymple (and Bhushan G. Lodha)]

From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>

In this file, two functions use identical blocks of code to call the
POSIX regex compiling function and handle a possible error. Here we
factor that block into its own function, in anticipation of using the
same code a third time.

Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
---
 diffcore-pickaxe.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 401eb72..0d36a3c 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -12,6 +12,8 @@ typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
 			  struct diff_options *o,
 			  regex_t *regexp, kwset_t kws);
 
+static void compile_regex(regex_t *r, const char *s, int cflags);
+
 static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 			 regex_t *regexp, kwset_t kws, pickaxe_fn fn);
 
@@ -110,20 +112,13 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
 
 static void diffcore_pickaxe_grep(struct diff_options *o)
 {
-	int err;
 	regex_t regex;
 	int cflags = REG_EXTENDED | REG_NEWLINE;
 
 	if (DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE))
 		cflags |= REG_ICASE;
 
-	err = regcomp(&regex, o->pickaxe, cflags);
-	if (err) {
-		char errbuf[1024];
-		regerror(err, &regex, errbuf, 1024);
-		regfree(&regex);
-		die("invalid regex: %s", errbuf);
-	}
+	compile_regex(&regex, o->pickaxe, cflags);
 
 	pickaxe(&diff_queued_diff, o, &regex, NULL, diff_grep);
 
@@ -180,6 +175,18 @@ static int has_changes(mmfile_t *one, mmfile_t *two,
 	return one_contains != two_contains;
 }
 
+static void compile_regex(regex_t *r, const char *s, int cflags)
+{
+	int err;
+	err = regcomp(r, s, cflags);
+	if (err) {
+		char errbuf[1024];
+		regerror(err, r, errbuf, 1024);
+		regfree(r);
+		die("invalid regex: %s", errbuf);
+	}
+}
+
 static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 			 regex_t *regexp, kwset_t kws, pickaxe_fn fn)
 {
@@ -236,15 +243,7 @@ static void diffcore_pickaxe_count(struct diff_options *o)
 	kwset_t kws = NULL;
 
 	if (opts & DIFF_PICKAXE_REGEX) {
-		int err;
-		err = regcomp(&regex, needle, REG_EXTENDED | REG_NEWLINE);
-		if (err) {
-			/* The POSIX.2 people are surely sick */
-			char errbuf[1024];
-			regerror(err, &regex, errbuf, 1024);
-			regfree(&regex);
-			die("invalid regex: %s", errbuf);
-		}
+		compile_regex(&regex, needle, REG_EXTENDED | REG_NEWLINE);
 		regexp = &regex;
 	} else {
 		kws = kwsalloc(DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE)
-- 
1.7.12.4 (Apple Git-37)

[PATCH 03/10] diffcore-pickaxe.c: Refactor pickaxe_fn signature

[David A. Dalrymple (and Bhushan G. Lodha)]

From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>

This function type previously accepted separate regex_t and kwset_t
parameters, which conceptually go together. Here we create a struct to
encapsulate them, in anticipation of adding a third field that
pickaxe_fn's may require.

This parallels the existing diffgrep_cb structure for passing possibly
relevant values through to the callbacks invoked by xdi_diff_outf.

Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
---
 diffcore-pickaxe.c | 50 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 0d36a3c..7e65095 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -8,17 +8,22 @@
 #include "xdiff-interface.h"
 #include "kwset.h"
 
+struct fn_options {
+	regex_t *regex;
+	kwset_t kws;
+};
+
 typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
 			  struct diff_options *o,
-			  regex_t *regexp, kwset_t kws);
+			  struct fn_options *fno);
 
 static void compile_regex(regex_t *r, const char *s, int cflags);
 
 static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
-			 regex_t *regexp, kwset_t kws, pickaxe_fn fn);
+			 pickaxe_fn fn, struct fn_options *fno);
 
 static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,
-		    regex_t *regexp, kwset_t kws, pickaxe_fn fn)
+		    pickaxe_fn fn, struct fn_options *fno)
 {
 	int i;
 	struct diff_queue_struct outq;
@@ -29,7 +34,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,
 		/* Showing the whole changeset if needle exists */
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
-			if (pickaxe_match(p, o, regexp, kws, fn))
+			if (pickaxe_match(p, o, fn, fno))
 				return; /* do not munge the queue */
 		}
 
@@ -44,7 +49,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,
 		/* Showing only the filepairs that has the needle */
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
-			if (pickaxe_match(p, o, regexp, kws, fn))
+			if (pickaxe_match(p, o, fn, fno))
 				diff_q(&outq, p);
 			else
 				diff_free_filepair(p);
@@ -83,7 +88,7 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)
 
 static int diff_grep(mmfile_t *one, mmfile_t *two,
 		     struct diff_options *o,
-		     regex_t *regexp, kwset_t kws)
+		     struct fn_options *fno)
 {
 	regmatch_t regmatch;
 	struct diffgrep_cb ecbdata;
@@ -91,9 +96,9 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
 	xdemitconf_t xecfg;
 
 	if (!one)
-		return !regexec(regexp, two->ptr, 1, &regmatch, 0);
+		return !regexec(fno->regex, two->ptr, 1, &regmatch, 0);
 	if (!two)
-		return !regexec(regexp, one->ptr, 1, &regmatch, 0);
+		return !regexec(fno->regex, one->ptr, 1, &regmatch, 0);
 
 	/*
 	 * We have both sides; need to run textual diff and see if
@@ -101,7 +106,7 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
 	 */
 	memset(&xpp, 0, sizeof(xpp));
 	memset(&xecfg, 0, sizeof(xecfg));
-	ecbdata.regexp = regexp;
+	ecbdata.regexp = fno->regex;
 	ecbdata.hit = 0;
 	xecfg.ctxlen = o->context;
 	xecfg.interhunkctxlen = o->interhunkcontext;
@@ -113,6 +118,7 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
 static void diffcore_pickaxe_grep(struct diff_options *o)
 {
 	regex_t regex;
+	struct fn_options fno;
 	int cflags = REG_EXTENDED | REG_NEWLINE;
 
 	if (DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE))
@@ -120,13 +126,14 @@ static void diffcore_pickaxe_grep(struct diff_options *o)
 
 	compile_regex(&regex, o->pickaxe, cflags);
 
-	pickaxe(&diff_queued_diff, o, &regex, NULL, diff_grep);
+	fno.regex = &regex;
+	pickaxe(&diff_queued_diff, o, diff_grep, &fno);
 
 	regfree(&regex);
 	return;
 }
 
-static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
+static unsigned int contains(mmfile_t *mf, struct fn_options *fno)
 {
 	unsigned int cnt;
 	unsigned long sz;
@@ -136,12 +143,12 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
 	data = mf->ptr;
 	cnt = 0;
 
-	if (regexp) {
+	if (fno->regex) {
 		regmatch_t regmatch;
 		int flags = 0;
 
 		assert(data[sz] == '\0');
-		while (*data && !regexec(regexp, data, 1, &regmatch, flags)) {
+		while (*data && !regexec(fno->regex, data, 1, &regmatch, flags)) {
 			flags |= REG_NOTBOL;
 			data += regmatch.rm_eo;
 			if (*data && regmatch.rm_so == regmatch.rm_eo)
@@ -152,7 +159,7 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
 	} else { /* Classic exact string match */
 		while (sz) {
 			struct kwsmatch kwsm;
-			size_t offset = kwsexec(kws, data, sz, &kwsm);
+			size_t offset = kwsexec(fno->kws, data, sz, &kwsm);
 			const char *found;
 			if (offset == -1)
 				break;
@@ -168,10 +175,10 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
 
 static int has_changes(mmfile_t *one, mmfile_t *two,
 		       struct diff_options *o,
-		       regex_t *regexp, kwset_t kws)
+		       struct fn_options *fno)
 {
-	unsigned int one_contains = one ? contains(one, regexp, kws) : 0;
-	unsigned int two_contains = two ? contains(two, regexp, kws) : 0;
+	unsigned int one_contains = one ? contains(one, fno) : 0;
+	unsigned int two_contains = two ? contains(two, fno) : 0;
 	return one_contains != two_contains;
 }
 
@@ -188,7 +195,7 @@ static void compile_regex(regex_t *r, const char *s, int cflags)
 }
 
 static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
-			 regex_t *regexp, kwset_t kws, pickaxe_fn fn)
+			 pickaxe_fn fn, struct fn_options *fno)
 {
 	struct userdiff_driver *textconv_one = NULL;
 	struct userdiff_driver *textconv_two = NULL;
@@ -222,7 +229,7 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 
 	ret = fn(DIFF_FILE_VALID(p->one) ? &mf1 : NULL,
 		 DIFF_FILE_VALID(p->two) ? &mf2 : NULL,
-		 o, regexp, kws);
+		 o, fno);
 
 	if (textconv_one)
 		free(mf1.ptr);
@@ -236,6 +243,7 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 
 static void diffcore_pickaxe_count(struct diff_options *o)
 {
+	struct fn_options fno;
 	const char *needle = o->pickaxe;
 	int opts = o->pickaxe_opts;
 	unsigned long len = strlen(needle);
@@ -252,7 +260,9 @@ static void diffcore_pickaxe_count(struct diff_options *o)
 		kwsprep(kws);
 	}
 
-	pickaxe(&diff_queued_diff, o, regexp, kws, has_changes);
+	fno.regex = regexp;
+	fno.kws = kws;
+	pickaxe(&diff_queued_diff, o, has_changes, &fno);
 
 	if (opts & DIFF_PICKAXE_REGEX)
 		regfree(&regex);
-- 
1.7.12.4 (Apple Git-37)

Re: [PATCH 03/10] diffcore-pickaxe.c: Refactor pickaxe_fn signature

[Jakub Narębski]

W dniu 2014-03-27 19:50, David A. Dalrymple (and Bhushan G. Lodha) pisze:
> From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>
>
> This function type previously accepted separate regex_t and kwset_t
> parameters, which conceptually go together. Here we create a struct to
> encapsulate them, in anticipation of adding a third field that
> pickaxe_fn's may require.
>
> This parallels the existing diffgrep_cb structure for passing possibly
> relevant values through to the callbacks invoked by xdi_diff_outf.

If it parallels existing diffgrep_cb structure, why not name this
equivalent in simular way, i.e. pickaxe_cb or pickaxe_options or
pickaxe_cb_opts instead of generic name fn_options?

> Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
> ---
>   diffcore-pickaxe.c | 50 ++++++++++++++++++++++++++++++--------------------
>   1 file changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
> index 0d36a3c..7e65095 100644
> --- a/diffcore-pickaxe.c
> +++ b/diffcore-pickaxe.c
> @@ -8,17 +8,22 @@
>   #include "xdiff-interface.h"
>   #include "kwset.h"
>
> +struct fn_options {
> +	regex_t *regex;
> +	kwset_t kws;
> +};

[PATCH 04/10] diff.c/diff.h: expose userdiff_funcname

[David A. Dalrymple (and Bhushan G. Lodha)]

From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>

The functionality of userdiff_funcname (determining the language in use
for a given file and setting up patterns to match "function names" in
that language) is useful outside of diff.c, so here we remove its static
specifier and declare it in diff.h.

Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
---
 diff.c | 2 +-
 diff.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index e343191..f978ee7 100644
--- a/diff.c
+++ b/diff.c
@@ -2203,7 +2203,7 @@ int diff_filespec_is_binary(struct diff_filespec *one)
 	return one->is_binary;
 }
 
-static const struct userdiff_funcname *diff_funcname_pattern(struct diff_filespec *one)
+const struct userdiff_funcname *diff_funcname_pattern(struct diff_filespec *one)
 {
 	diff_filespec_load_driver(one);
 	return one->driver->funcname.pattern ? &one->driver->funcname : NULL;
diff --git a/diff.h b/diff.h
index a24a767..9e96fc9 100644
--- a/diff.h
+++ b/diff.h
@@ -349,4 +349,6 @@ extern int print_stat_summary(FILE *fp, int files,
 			      int insertions, int deletions);
 extern void setup_diff_pager(struct diff_options *);
 
+const struct userdiff_funcname *diff_funcname_pattern(struct diff_filespec *);
+
 #endif /* DIFF_H */
-- 
1.7.12.4 (Apple Git-37)

[PATCH 05/10] diffcore-pickaxe.c: set up funcname pattern

[David A. Dalrymple (and Bhushan G. Lodha)]

From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>

We use userdiff_funcname to make the filetype-dependent function name
pattern available to pickaxe functions.

Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
---
 diffcore-pickaxe.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 7e65095..103fe6c 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -7,10 +7,12 @@
 #include "diffcore.h"
 #include "xdiff-interface.h"
 #include "kwset.h"
+#include "userdiff.h"
 
 struct fn_options {
 	regex_t *regex;
 	kwset_t kws;
+	const struct userdiff_funcname *funcname_pattern;
 };
 
 typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
@@ -224,6 +226,13 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 	if (textconv_one == textconv_two && diff_unmodified_pair(p))
 		return 0;
 
+	const struct userdiff_funcname *funcname_pattern;
+	funcname_pattern = diff_funcname_pattern(p->one);
+	if (!funcname_pattern)
+		funcname_pattern = diff_funcname_pattern(p->two);
+
+	fno->funcname_pattern = funcname_pattern;
+
 	mf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);
 	mf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);
 
-- 
1.7.12.4 (Apple Git-37)

[PATCH 06/10] log: --function-name pickaxe

[David A. Dalrymple (and Bhushan G. Lodha)]

From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>

This is similar to the pickaxe grep option (-G), but applies the
provided regex only to diff hunk headers, thereby showing only those
commits which affect a "function" with a definition line matching the
pattern. These are "functions" in the same sense as with
--function-context, i.e., they may be classes, structs, etc. depending
on the programming-language-specific pattern specified by the "diff"
attribute in .gitattributes.

builtin/log.c:
	as with pickaxe, set always_show_header when using --function-name
diff.c:
	parse option; as with pickaxe, always set the RECURSIVE option
	for --function-name
diff.h:
	include "funcname" field in struct diff_options
diffcore-pickaxe.c:
	implementation of --function-name filtering (diffcore_funcname), like
	the existing diffcore_pickaxe_grep and diffcore_pickaxe_count
revision.c:
	as with pickaxe, set revs->diff to always generate diffs when
	using --function-name

Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
---
 builtin/log.c      |  2 +-
 diff.c             |  8 +++++--
 diff.h             |  1 +
 diffcore-pickaxe.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 revision.c         |  3 ++-
 5 files changed, 77 insertions(+), 6 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b97373d..78694de 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -158,7 +158,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 	if (rev->show_notes)
 		init_display_notes(&rev->notes_opt);
 
-	if (rev->diffopt.pickaxe || rev->diffopt.filter)
+	if (rev->diffopt.pickaxe || rev->diffopt.filter || rev->diffopt.funcname)
 		rev->always_show_header = 0;
 	if (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {
 		rev->always_show_header = 0;
diff --git a/diff.c b/diff.c
index f978ee7..2f6dbc1 100644
--- a/diff.c
+++ b/diff.c
@@ -3298,7 +3298,7 @@ void diff_setup_done(struct diff_options *options)
 	/*
 	 * Also pickaxe would not work very well if you do not say recursive
 	 */
-	if (options->pickaxe)
+	if (options->pickaxe || options->funcname)
 		DIFF_OPT_SET(options, RECURSIVE);
 	/*
 	 * When patches are generated, submodules diffed against the work tree
@@ -3821,6 +3821,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->orderfile = optarg;
 		return argcount;
 	}
+	else if ((argcount = parse_long_opt("function-name", av, &optarg))) {
+		options->funcname = optarg;
+		return argcount;
+	}
 	else if ((argcount = parse_long_opt("diff-filter", av, &optarg))) {
 		int offending = parse_diff_filter_opt(optarg, options);
 		if (offending)
@@ -4768,7 +4772,7 @@ void diffcore_std(struct diff_options *options)
 		if (options->break_opt != -1)
 			diffcore_merge_broken();
 	}
-	if (options->pickaxe)
+	if (options->pickaxe || options->funcname)
 		diffcore_pickaxe(options);
 	if (options->orderfile)
 		diffcore_order(options->orderfile);
diff --git a/diff.h b/diff.h
index 9e96fc9..0fd5f1d 100644
--- a/diff.h
+++ b/diff.h
@@ -107,6 +107,7 @@ enum diff_words_type {
 struct diff_options {
 	const char *orderfile;
 	const char *pickaxe;
+	const char *funcname;
 	const char *single_follow;
 	const char *a_prefix, *b_prefix;
 	unsigned flags;
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 103fe6c..259a8fa 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -67,6 +67,12 @@ struct diffgrep_cb {
 	int hit;
 };
 
+struct funcname_cb {
+	struct userdiff_funcname *pattern;
+	regex_t *regex;
+	int hit;
+};
+
 static void diffgrep_consume(void *priv, char *line, unsigned long len)
 {
 	struct diffgrep_cb *data = priv;
@@ -88,6 +94,20 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)
 	line[len] = hold;
 }
 
+static void match_funcname(void *priv, char *line, unsigned long len)
+{
+	regmatch_t regmatch;
+	int hold;
+	struct funcname_cb *data = priv;
+	hold = line[len];
+	line[len] = '\0';
+
+	if (line[0] == '@' && line[1] == '@')
+		if (!regexec(data->regex, line, 1, &regmatch, 0))
+			data->hit = 1;
+	line[len] = hold;
+}
+
 static int diff_grep(mmfile_t *one, mmfile_t *two,
 		     struct diff_options *o,
 		     struct fn_options *fno)
@@ -117,6 +137,38 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
 	return ecbdata.hit;
 }
 
+static int diff_funcname_filter(mmfile_t *one, mmfile_t *two,
+				struct diff_options *o,
+				struct fn_options *fno)
+{
+	struct funcname_cb ecbdata;
+	xpparam_t xpp;
+	xdemitconf_t xecfg;
+
+	mmfile_t empty;
+	empty.ptr = "";
+	empty.size = 0;
+	if (!one)
+		one = &empty;
+	if (!two)
+		two = &empty;
+	memset(&xpp, 0, sizeof(xpp));
+	memset(&xecfg, 0, sizeof(xecfg));
+	ecbdata.regex = fno->regex;
+	ecbdata.hit = 0;
+	xecfg.ctxlen = o->context;
+
+	if (fno->funcname_pattern)
+		xdiff_set_find_func(&xecfg, fno->funcname_pattern->pattern,
+					    fno->funcname_pattern->cflags);
+	xecfg.interhunkctxlen = o->interhunkcontext;
+	if (!(one && two))
+		xecfg.flags = XDL_EMIT_FUNCCONTEXT;
+	xecfg.flags |= XDL_EMIT_FUNCNAMES;
+	xdi_diff_outf(one, two, match_funcname, &ecbdata, &xpp, &xecfg);
+	return ecbdata.hit;
+}
+
 static void diffcore_pickaxe_grep(struct diff_options *o)
 {
 	regex_t regex;
@@ -204,7 +256,7 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 	mmfile_t mf1, mf2;
 	int ret;
 
-	if (!o->pickaxe[0])
+	if (o->pickaxe && !o->pickaxe[0])
 		return 0;
 
 	/* ignore unmerged */
@@ -280,11 +332,24 @@ static void diffcore_pickaxe_count(struct diff_options *o)
 	return;
 }
 
+static void diffcore_funcname(struct diff_options *o)
+{
+	struct fn_options fno;
+	regex_t regex;
+
+	fno.regex = &regex;
+	compile_regex(&regex, o->funcname, REG_EXTENDED | REG_NEWLINE);
+	pickaxe(&diff_queued_diff, o, diff_funcname_filter, &fno);
+	regfree(&regex);
+}
+
 void diffcore_pickaxe(struct diff_options *o)
 {
+	if (o->funcname)
+		diffcore_funcname(o);
 	/* Might want to warn when both S and G are on; I don't care... */
 	if (o->pickaxe_opts & DIFF_PICKAXE_KIND_G)
 		diffcore_pickaxe_grep(o);
-	else
+	else if (o-> pickaxe_opts & DIFF_PICKAXE_KIND_S)
 		diffcore_pickaxe_count(o);
 }
diff --git a/revision.c b/revision.c
index 8508550..d81d9b9 100644
--- a/revision.c
+++ b/revision.c
@@ -2211,8 +2211,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT)
 		revs->diff = 1;
 
-	/* Pickaxe, diff-filter and rename following need diffs */
+	/* Pickaxe, funcname, diff-filter and rename following need diffs */
 	if (revs->diffopt.pickaxe ||
+	    revs->diffopt.funcname ||
 	    revs->diffopt.filter ||
 	    DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES))
 		revs->diff = 1;
-- 
1.7.12.4 (Apple Git-37)

Re: [PATCH 06/10] log: --function-name pickaxe

[René Scharfe]

Am 27.03.2014 19:50, schrieb David A. Dalrymple (and Bhushan G. Lodha):
> From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>
>
> This is similar to the pickaxe grep option (-G), but applies the
> provided regex only to diff hunk headers, thereby showing only those
> commits which affect a "function" with a definition line matching the
> pattern. These are "functions" in the same sense as with
> --function-context, i.e., they may be classes, structs, etc. depending
> on the programming-language-specific pattern specified by the "diff"
> attribute in .gitattributes.

With that approach you depend on the hunk header and apparently need to 
add XDL_EMIT_MOREFUNCNAMES and XDL_EMIT_MOREHUNKHEADS to improve the 
results.  This approach feels fragile.

Would it perhaps be more robust to not base the implementation on diff 
and instead to scan the raw file contents?  You'd search both files for 
a matching function signature, then search for a non-matching one from 
there.  The parts in between are function bodies and can be compared. 
If they match, you'd search for matching function starts again etc.

Or would it make sense to make use of the diff option FUNCCONTEXT (git 
diff -W) and look for the function signature in the diff body instead of 
in the hunk header?  Such a diff contains whole functions, but a single 
hunk could contain multiple ones.

René

Re: [PATCH 06/10] log: --function-name pickaxe

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> With that approach you depend on the hunk header and apparently need
> to add XDL_EMIT_MOREFUNCNAMES and XDL_EMIT_MOREHUNKHEADS to improve
> the results.  This approach feels fragile.
>
> Would it perhaps be more robust to not base the implementation on diff
> and instead to scan the raw file contents?

That is an interesting idea.

Perhaps this can be implemented as a new stage in the transformation
pipeline, I wonder?  There is currently no transformation that
modifies the blob contents being compared, but I do not think there
is anything fundamental that prevents one from being written.  The
new "limit to this function body" transformation would perhaps sit
before the diffcore-rename and would transform all the blobs to
empty, except for the part that is the body of the function the user
is interested in.

Re: [PATCH 06/10] log: --function-name pickaxe

[Jakub Narębski]

W dniu 2014-04-03 23:44, Junio C Hamano pisze:
> René Scharfe <l.s.r@web.de> writes:
>
>> With that approach you depend on the hunk header and apparently need
>> to add XDL_EMIT_MOREFUNCNAMES and XDL_EMIT_MOREHUNKHEADS to improve
>> the results.  This approach feels fragile.
>>
>> Would it perhaps be more robust to not base the implementation on diff
>> and instead to scan the raw file contents?
>
> That is an interesting idea.
>
> Perhaps this can be implemented as a new stage in the transformation
> pipeline, I wonder?  There is currently no transformation that
> modifies the blob contents being compared, but I do not think there
> is anything fundamental that prevents one from being written.  The
> new "limit to this function body" transformation would perhaps sit
> before the diffcore-rename and would transform all the blobs to
> empty, except for the part that is the body of the function the user
> is interested in.

Well, there is 'texconv', e.g.

   .gitattributes
   *.jpg diff=jpg

   .git/config
   [diff "jpg"]
          textconv = exif

Doesn't it fit in said place in the transformation pipeline?

-- 
Jakub Narębski

Re: [PATCH 06/10] log: --function-name pickaxe

[Junio C Hamano]

Jakub Narębski <jnareb@gmail.com> writes:

> W dniu 2014-04-03 23:44, Junio C Hamano pisze:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> With that approach you depend on the hunk header and apparently need
>>> to add XDL_EMIT_MOREFUNCNAMES and XDL_EMIT_MOREHUNKHEADS to improve
>>> the results.  This approach feels fragile.
>>>
>>> Would it perhaps be more robust to not base the implementation on diff
>>> and instead to scan the raw file contents?
>>
>> That is an interesting idea.
>>
>> Perhaps this can be implemented as a new stage in the transformation
>> pipeline, I wonder?  There is currently no transformation that
>> modifies the blob contents being compared, but I do not think there
>> is anything fundamental that prevents one from being written.  The
>> new "limit to this function body" transformation would perhaps sit
>> before the diffcore-rename and would transform all the blobs to
>> empty, except for the part that is the body of the function the user
>> is interested in.
>
> Well, there is 'texconv', e.g.
>
>   .gitattributes
>   *.jpg diff=jpg
>
>   .git/config
>   [diff "jpg"]
>          textconv = exif

;-)  So you could define this textconv

    sed -n -e '/^int main(/,/^}/p'

to limit the output only to the definition of the function main().

> Doesn't it fit in said place in the transformation pipeline?

Not at all, unfortunately.  The textconv conversion happens in the
final output stage and comes way too late to influence the earlier
stages like renames and pickaxe, which will still see the whole
contents outside the definition of the function main().

Re: [PATCH 06/10] log: --function-name pickaxe

[Bhushan Lodha]

I plan to work on this in few weeks. If anybody has more suggestion or
want to discuss the implementation let me know

On Fri, Apr 4, 2014 at 2:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narębski <jnareb@gmail.com> writes:
>
>> W dniu 2014-04-03 23:44, Junio C Hamano pisze:
>>> René Scharfe <l.s.r@web.de> writes:
>>>
>>>> With that approach you depend on the hunk header and apparently need
>>>> to add XDL_EMIT_MOREFUNCNAMES and XDL_EMIT_MOREHUNKHEADS to improve
>>>> the results.  This approach feels fragile.
>>>>
>>>> Would it perhaps be more robust to not base the implementation on diff
>>>> and instead to scan the raw file contents?
>>>
>>> That is an interesting idea.
>>>
>>> Perhaps this can be implemented as a new stage in the transformation
>>> pipeline, I wonder?  There is currently no transformation that
>>> modifies the blob contents being compared, but I do not think there
>>> is anything fundamental that prevents one from being written.  The
>>> new "limit to this function body" transformation would perhaps sit
>>> before the diffcore-rename and would transform all the blobs to
>>> empty, except for the part that is the body of the function the user
>>> is interested in.
>>
>> Well, there is 'texconv', e.g.
>>
>>   .gitattributes
>>   *.jpg diff=jpg
>>
>>   .git/config
>>   [diff "jpg"]
>>          textconv = exif
>
> ;-)  So you could define this textconv
>
>     sed -n -e '/^int main(/,/^}/p'
>
> to limit the output only to the definition of the function main().
>
>> Doesn't it fit in said place in the transformation pipeline?
>
> Not at all, unfortunately.  The textconv conversion happens in the
> final output stage and comes way too late to influence the earlier
> stages like renames and pickaxe, which will still see the whole
> contents outside the definition of the function main().
>
>
>

[PATCH 07/10] xdiff: add XDL_EMIT_MOREFUNCNAMES

[David A. Dalrymple (and Bhushan G. Lodha)]

From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>

For filtering commits by function name, it's useful to identify the
function name in cases such as adding a new function to a file (where
the default functionality will not emit a function name in the hunk
header, because it isn't part of the context).

This adds a flag asking xdiff to be more aggressive in finding function
names to emit, and turns the flag on when the --function-name option is
in use.

Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
---
 diff.c             |  2 ++
 diffcore-pickaxe.c |  2 +-
 xdiff/xdiff.h      |  1 +
 xdiff/xemit.c      | 39 +++++++++++++++++++++++----------------
 4 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/diff.c b/diff.c
index 2f6dbc1..914b4a2 100644
--- a/diff.c
+++ b/diff.c
@@ -2380,6 +2380,8 @@ static void builtin_diff(const char *name_a,
 		xecfg.ctxlen = o->context;
 		xecfg.interhunkctxlen = o->interhunkcontext;
 		xecfg.flags = XDL_EMIT_FUNCNAMES;
+		if (o->funcname)
+			xecfg.flags |= XDL_EMIT_MOREFUNCNAMES;
 		if (DIFF_OPT_TST(o, FUNCCONTEXT))
 			xecfg.flags |= XDL_EMIT_FUNCCONTEXT;
 		if (pe)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 259a8fa..ab31c18 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -164,7 +164,7 @@ static int diff_funcname_filter(mmfile_t *one, mmfile_t *two,
 	xecfg.interhunkctxlen = o->interhunkcontext;
 	if (!(one && two))
 		xecfg.flags = XDL_EMIT_FUNCCONTEXT;
-	xecfg.flags |= XDL_EMIT_FUNCNAMES;
+	xecfg.flags |= XDL_EMIT_FUNCNAMES | XDL_EMIT_MOREFUNCNAMES;
 	xdi_diff_outf(one, two, match_funcname, &ecbdata, &xpp, &xecfg);
 	return ecbdata.hit;
 }
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index c033991..469bded 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -44,6 +44,7 @@
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_COMMON (1 << 1)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)
+#define XDL_EMIT_MOREFUNCNAMES (1 << 3)
 
 #define XDL_MMB_READONLY (1 << 0)
 
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 4266ada..0ddb094 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -23,6 +23,10 @@
 #include "xinclude.h"
 
 
+struct func_line {
+	long len;
+	char buf[80];
+};
 
 
 static long xdl_get_rec(xdfile_t *xdf, long ri, char const **rec);
@@ -135,12 +139,7 @@ static int xdl_emit_common(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 	return 0;
 }
 
-struct func_line {
-	long len;
-	char buf[80];
-};
-
-static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,
+static long get_func_line(xdfile_t *xdf, xdemitconf_t const *xecfg,
 			  struct func_line *func_line, long start, long limit)
 {
 	find_func_t ff = xecfg->find_func ? xecfg->find_func : def_ff;
@@ -150,9 +149,9 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,
 	buf = func_line ? func_line->buf : dummy;
 	size = func_line ? sizeof(func_line->buf) : sizeof(dummy);
 
-	for (l = start; l != limit && 0 <= l && l < xe->xdf1.nrec; l += step) {
+	for (l = start; l != limit && 0 <= l && l < xdf->nrec; l += step) {
 		const char *rec;
-		long reclen = xdl_get_rec(&xe->xdf1, l, &rec);
+		long reclen = xdl_get_rec(xdf, l, &rec);
 		long len = ff(rec, reclen, buf, size, xecfg->find_func_priv);
 		if (len >= 0) {
 			if (func_line)
@@ -167,7 +166,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 		  xdemitconf_t const *xecfg) {
 	long s1, s2, e1, e2, lctx;
 	xdchange_t *xch, *xche;
-	long funclineprev = -1;
+	long funclineprev1 = -1, funclineprev2 = -1;
 	struct func_line func_line = { 0 };
 
 	if (xecfg->flags & XDL_EMIT_COMMON)
@@ -182,7 +181,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 		s2 = XDL_MAX(xch->i2 - xecfg->ctxlen, 0);
 
 		if (xecfg->flags & XDL_EMIT_FUNCCONTEXT) {
-			long fs1 = get_func_line(xe, xecfg, NULL, xch->i1, -1);
+			long fs1 = get_func_line(&xe->xdf1, xecfg, NULL, xch->i1, -1);
 			if (fs1 < 0)
 				fs1 = 0;
 			if (fs1 < s1) {
@@ -200,7 +199,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 		e2 = xche->i2 + xche->chg2 + lctx;
 
 		if (xecfg->flags & XDL_EMIT_FUNCCONTEXT) {
-			long fe1 = get_func_line(xe, xecfg, NULL,
+			long fe1 = get_func_line(&xe->xdf1, xecfg, NULL,
 						 xche->i1 + xche->chg1,
 						 xe->xdf1.nrec);
 			if (fe1 < 0)
@@ -218,7 +217,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 			if (xche->next) {
 				long l = xche->next->i1;
 				if (l <= e1 ||
-				    get_func_line(xe, xecfg, NULL, l, e1) < 0) {
+				    get_func_line(&xe->xdf1, xecfg, NULL, l, e1) < 0) {
 					xche = xche->next;
 					goto again;
 				}
@@ -229,10 +228,18 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 		 * Emit current hunk header.
 		 */
 
-		if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
-			get_func_line(xe, xecfg, &func_line,
-				      s1 - 1, funclineprev);
-			funclineprev = s1 - 1;
+		if (xecfg->flags & XDL_EMIT_MOREFUNCNAMES) {
+			long fl_in_xch1 = get_func_line(&xe->xdf1, xecfg,
+					&func_line, xch->i1, xch->i1+xch->chg1);
+			if (fl_in_xch1 < 0) {
+				get_func_line(&xe->xdf2, xecfg, &func_line,
+					      xch->i2, funclineprev2);
+				funclineprev2 = xch->i2;
+			}
+		} else if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
+			get_func_line(&xe->xdf1, xecfg, &func_line,
+				      s1 - 1, funclineprev1);
+			funclineprev1 = s1 - 1;
 		}
 		if (xdl_emit_hunk_hdr(s1 + 1, e1 - s1, s2 + 1, e2 - s2,
 				      func_line.buf, func_line.len, ecb) < 0)
-- 
1.7.12.4 (Apple Git-37)

[PATCH 08/10] xdiff: add XDL_EMIT_MOREHUNKHEADS

[David A. Dalrymple (and Bhushan G. Lodha)]

From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>

For filtering by function names, it's useful to split hunks whenever a
function line is encountered, so that each function name being deleted
or inserted gets its own hunk header (which then can be easily detected
by the filter).

This adds a flag, XDL_EMIT_MOREHUNKHEADS, which triggers this
nonstandard behavior, and enables it only in case the --function-name
option is being used.

Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
---
 diff.c             |  3 ++-
 diffcore-pickaxe.c |  3 ++-
 xdiff/xdiff.h      |  1 +
 xdiff/xdiffi.c     |  2 +-
 xdiff/xemit.c      | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 xdiff/xemit.h      |  4 +++-
 6 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index 914b4a2..a86206c 100644
--- a/diff.c
+++ b/diff.c
@@ -2381,7 +2381,8 @@ static void builtin_diff(const char *name_a,
 		xecfg.interhunkctxlen = o->interhunkcontext;
 		xecfg.flags = XDL_EMIT_FUNCNAMES;
 		if (o->funcname)
-			xecfg.flags |= XDL_EMIT_MOREFUNCNAMES;
+			xecfg.flags |= XDL_EMIT_MOREFUNCNAMES
+				    | XDL_EMIT_MOREHUNKHEADS;
 		if (DIFF_OPT_TST(o, FUNCCONTEXT))
 			xecfg.flags |= XDL_EMIT_FUNCCONTEXT;
 		if (pe)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index ab31c18..d9f4c30 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -164,7 +164,8 @@ static int diff_funcname_filter(mmfile_t *one, mmfile_t *two,
 	xecfg.interhunkctxlen = o->interhunkcontext;
 	if (!(one && two))
 		xecfg.flags = XDL_EMIT_FUNCCONTEXT;
-	xecfg.flags |= XDL_EMIT_FUNCNAMES | XDL_EMIT_MOREFUNCNAMES;
+	xecfg.flags |= XDL_EMIT_FUNCNAMES | XDL_EMIT_MOREFUNCNAMES
+		    | XDL_EMIT_MOREHUNKHEADS;
 	xdi_diff_outf(one, two, match_funcname, &ecbdata, &xpp, &xecfg);
 	return ecbdata.hit;
 }
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 469bded..787c376 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -45,6 +45,7 @@
 #define XDL_EMIT_COMMON (1 << 1)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)
 #define XDL_EMIT_MOREFUNCNAMES (1 << 3)
+#define XDL_EMIT_MOREHUNKHEADS (1 << 4)
 
 #define XDL_MMB_READONLY (1 << 0)
 
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 2358a2d..c29804e 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -545,7 +545,7 @@ static int xdl_call_hunk_func(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 	xdchange_t *xch, *xche;
 
 	for (xch = xscr; xch; xch = xche->next) {
-		xche = xdl_get_hunk(&xch, xecfg);
+		xche = xdl_get_hunk(xe, &xch, xecfg);
 		if (!xch)
 			break;
 		if (xecfg->hunk_func(xch->i1, xche->i1 + xche->chg1 - xch->i1,
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 0ddb094..f49eaaf 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -29,6 +29,9 @@ struct func_line {
 };
 
 
+static long get_func_line(xdfile_t *xdf, xdemitconf_t const *xecfg,
+			  struct func_line *func_line, long start, long limit);
+
 static long xdl_get_rec(xdfile_t *xdf, long ri, char const **rec);
 static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *ecb);
 
@@ -62,7 +65,7 @@ static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *
  * inside the differential hunk according to the specified configuration.
  * Also advance xscr if the first changes must be discarded.
  */
-xdchange_t *xdl_get_hunk(xdchange_t **xscr, xdemitconf_t const *xecfg)
+xdchange_t *xdl_get_hunk(xdfenv_t *xe, xdchange_t **xscr, xdemitconf_t const *xecfg)
 {
 	xdchange_t *xch, *xchp, *lxch;
 	long max_common = 2 * xecfg->ctxlen + xecfg->interhunkctxlen;
@@ -83,6 +86,59 @@ xdchange_t *xdl_get_hunk(xdchange_t **xscr, xdemitconf_t const *xecfg)
 
 	lxch = *xscr;
 
+	if (xecfg->flags & XDL_EMIT_MOREHUNKHEADS)
+		for (xch = *xscr; xch; xch=xch->next) {
+			/*
+			 * If a current change contains a func_line, end this
+			 * hunk immediately before and create a new hunk
+			 * starting from that line.
+			 */
+			long fl_in_xch1 = get_func_line(&xe->xdf1, xecfg, NULL,
+						xch->i1, xch->i1+xch->chg1);
+			long fl_in_xch2 = get_func_line(&xe->xdf2, xecfg, NULL,
+						xch->i2, xch->i2+xch->chg2);
+			if (fl_in_xch1 >= xch->i1 && fl_in_xch2 >= xch->i2) {
+				xdchange_t *new_next =
+					(xdchange_t *)xdl_malloc(sizeof(xdchange_t));
+				new_next->i1 = xch->i1+xch->chg1;
+				new_next->chg1 = 0;
+				new_next->i2 = xch->i2;
+				new_next->chg2 = xch->chg2;
+				new_next->ignore = xch->ignore;
+				new_next->next = xch->next;
+				xch->next = new_next;
+				xch->chg2 = 0;
+				return xch;
+			}
+			if (fl_in_xch1 > xch->i1) {
+				xdchange_t *new_next =
+					(xdchange_t *)xdl_malloc(sizeof(xdchange_t));
+				new_next->i1 = fl_in_xch1;
+				new_next->chg1 = (xch->i1+xch->chg1)-fl_in_xch1;
+				new_next->i2 = xch->i2;
+				new_next->chg2 = xch->chg2;
+				new_next->ignore = xch->ignore;
+				new_next->next = xch->next;
+				xch->next = new_next;
+				xch->chg1 = fl_in_xch1 - xch->i1;
+				xch->chg2 = 0;
+				return xch;
+			}
+			if (fl_in_xch2 > xch->i2) {
+				xdchange_t *new_next =
+					(xdchange_t *)xdl_malloc(sizeof(xdchange_t));
+				new_next->i2 = fl_in_xch2;
+				new_next->chg2 = (xch->i2+xch->chg2)-fl_in_xch2;
+				new_next->i1 = xch->i1+xch->chg1;
+				new_next->chg1 = 0;
+				new_next->ignore = xch->ignore;
+				new_next->next = xch->next;
+				xch->next = new_next;
+				xch->chg2 = fl_in_xch2 - xch->i2;
+				return xch;
+			}
+		}
+
 	for (xchp = *xscr, xch = xchp->next; xch; xchp = xch, xch = xch->next) {
 		long distance = xch->i1 - (xchp->i1 + xchp->chg1);
 		if (distance > max_common)
@@ -173,7 +229,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 		return xdl_emit_common(xe, xscr, ecb, xecfg);
 
 	for (xch = xscr; xch; xch = xche->next) {
-		xche = xdl_get_hunk(&xch, xecfg);
+		xche = xdl_get_hunk(xe, &xch, xecfg);
 		if (!xch)
 			break;
 
diff --git a/xdiff/xemit.h b/xdiff/xemit.h
index d297107..4d584f5 100644
--- a/xdiff/xemit.h
+++ b/xdiff/xemit.h
@@ -27,7 +27,9 @@
 typedef int (*emit_func_t)(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 			   xdemitconf_t const *xecfg);
 
-xdchange_t *xdl_get_hunk(xdchange_t **xscr, xdemitconf_t const *xecfg);
+xdchange_t *xdl_get_hunk(xdfenv_t *xe, xdchange_t **xscr,
+			 xdemitconf_t const *xecfg);
+
 int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 		  xdemitconf_t const *xecfg);
 
-- 
1.7.12.4 (Apple Git-37)

[PATCH 09/10] t4213: test --function-name option

[David A. Dalrymple (and Bhushan G. Lodha)]

From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>

This test builds a sample C file, adding and removing functions, and
checks that the right commits are filtered by --function-name matching.

Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
---
 t/t4213-log-function-name.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100755 t/t4213-log-function-name.sh

diff --git a/t/t4213-log-function-name.sh b/t/t4213-log-function-name.sh
new file mode 100755
index 0000000..1243ce5
--- /dev/null
+++ b/t/t4213-log-function-name.sh
@@ -0,0 +1,73 @@
+#!/bin/sh
+
+test_description='log --function-name'
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo "* diff=cpp" > .gitattributes
+
+	>file &&
+	git add file &&
+	test_tick &&
+	git commit -m initial &&
+
+	printf "int main(){\n\treturn 0;\n}\n" >> file &&
+	test_tick &&
+	git commit -am second
+
+	printf "void newfunc(){\n\treturn;\n}\n" >> file &&
+	test_tick &&
+	git commit -am third
+
+	printf "void newfunc2(){\n\treturn;\n}\n" | cat - file > temp &&
+	mv temp file &&
+	test_tick &&
+	git commit -am fourth
+
+	printf "void newfunc3(){\n\treturn;\n}\n" | cat - file > temp &&
+	mv temp file &&
+	test_tick &&
+	git commit -am fifth
+
+	sed -i -e "s/void newfunc2/void newfunc4/" file &&
+	test_tick &&
+	git commit -am sixth
+'
+
+test_expect_success 'log --function-name=main' '
+	git log --function-name=main >actual &&
+	git log --grep=second >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --function-name "newfunc\W"' '
+	git log --function-name "newfunc\W" >actual &&
+	git log --grep=third >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --function-name "newfunc2"' '
+	git log --function-name newfunc2 >actual &&
+	git log -E --grep "sixth|fourth" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --function-name "newfunc3"' '
+	git log --function-name newfunc3 >actual &&
+	git log --grep=fifth >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --function-name "newfunc4"' '
+	git log --function-name newfunc4 >actual &&
+	git log --grep=sixth >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --function-name "newfunc"' '
+	git log --function-name newfunc >actual &&
+	git log -E --grep "third|fourth|fifth|sixth" >expect &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.7.12.4 (Apple Git-37)

Re: [PATCH 09/10] t4213: test --function-name option

[Johannes Sixt]

Am 3/27/2014 19:50, schrieb David A. Dalrymple (and Bhushan G. Lodha):
> From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>
> 
> This test builds a sample C file, adding and removing functions, and
> checks that the right commits are filtered by --function-name matching.

This is probably the most important patch in your series as it documents
the expected behavior. Unfortunately, I find its clarity very lacking. :(

This new feature uses the userdiff driver, IIUC. Does it do so in all
respects? In particular, does it also evaluate the negative patterns? For
example, when there is a label in the code, is it not mistaken as the
beginning of a function? A test for this case would be very instructive.

Furthermore, consider a patch for a change at the very beginning of a
function. Then the function name would appear in the pre-context of the
hunk, but the hunk header would show the function before the one with the
change. Would such a change confuse your implementation? I guess not.
Again, a test case would remove any doubts.

Is it possible to search for a change that is before any functions? It
would be useful to enumerate commits that change #include lines.

> 
> Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
> ---
>  t/t4213-log-function-name.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100755 t/t4213-log-function-name.sh
> 
> diff --git a/t/t4213-log-function-name.sh b/t/t4213-log-function-name.sh
> new file mode 100755
> index 0000000..1243ce5
> --- /dev/null
> +++ b/t/t4213-log-function-name.sh
> @@ -0,0 +1,73 @@
> +#!/bin/sh
> +
> +test_description='log --function-name'
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	echo "* diff=cpp" > .gitattributes
> +
> +	>file &&
> +	git add file &&
> +	test_tick &&
> +	git commit -m initial &&
> +
> +	printf "int main(){\n\treturn 0;\n}\n" >> file &&
> +	test_tick &&
> +	git commit -am second

Broken && chain here and later as well. Please be careful.

> +
> +	printf "void newfunc(){\n\treturn;\n}\n" >> file &&
> +	test_tick &&
> +	git commit -am third

	git commit -am "append a function" &&

> +
> +	printf "void newfunc2(){\n\treturn;\n}\n" | cat - file > temp &&
> +	mv temp file &&
> +	test_tick &&
> +	git commit -am fourth

	git commit -am "prepend a function" &&

etc. You get the picture.

> +
> +	printf "void newfunc3(){\n\treturn;\n}\n" | cat - file > temp &&
> +	mv temp file &&
> +	test_tick &&
> +	git commit -am fifth
> +
> +	sed -i -e "s/void newfunc2/void newfunc4/" file &&
> +	test_tick &&
> +	git commit -am sixth
> +'
> +
> +test_expect_success 'log --function-name=main' '

test_expect_success 'log --function-name finds a function with a change' '

> +	git log --function-name=main >actual &&
> +	git log --grep=second >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'log --function-name "newfunc\W"' '

test_expect_success 'log --function-name with extended regexp' '

etc. You get the picture.

> +	git log --function-name "newfunc\W" >actual &&
> +	git log --grep=third >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'log --function-name "newfunc2"' '
> +	git log --function-name newfunc2 >actual &&
> +	git log -E --grep "sixth|fourth" >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'log --function-name "newfunc3"' '
> +	git log --function-name newfunc3 >actual &&
> +	git log --grep=fifth >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'log --function-name "newfunc4"' '
> +	git log --function-name newfunc4 >actual &&
> +	git log --grep=sixth >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'log --function-name "newfunc"' '
> +	git log --function-name newfunc >actual &&
> +	git log -E --grep "third|fourth|fifth|sixth" >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_done
> 

Re: [PATCH 09/10] t4213: test --function-name option

[Eric Sunshine]

On Fri, Mar 28, 2014 at 3:25 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 3/27/2014 19:50, schrieb David A. Dalrymple (and Bhushan G. Lodha):
>> From: "Bhushan G. Lodha & David A. Dalrymple" <dad-bgl@mit.edu>
>>
>> This test builds a sample C file, adding and removing functions, and
>> checks that the right commits are filtered by --function-name matching.
>
> This is probably the most important patch in your series as it documents
> the expected behavior. Unfortunately, I find its clarity very lacking. :(
>
> This new feature uses the userdiff driver, IIUC. Does it do so in all
> respects? In particular, does it also evaluate the negative patterns? For
> example, when there is a label in the code, is it not mistaken as the
> beginning of a function? A test for this case would be very instructive.
>
> Furthermore, consider a patch for a change at the very beginning of a
> function. Then the function name would appear in the pre-context of the
> hunk, but the hunk header would show the function before the one with the
> change. Would such a change confuse your implementation? I guess not.
> Again, a test case would remove any doubts.
>
> Is it possible to search for a change that is before any functions? It
> would be useful to enumerate commits that change #include lines.
>
>>
>> Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
>> ---
>>  t/t4213-log-function-name.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 73 insertions(+)
>>  create mode 100755 t/t4213-log-function-name.sh
>>
>> diff --git a/t/t4213-log-function-name.sh b/t/t4213-log-function-name.sh
>> new file mode 100755
>> index 0000000..1243ce5
>> --- /dev/null
>> +++ b/t/t4213-log-function-name.sh
>> @@ -0,0 +1,73 @@
>> +#!/bin/sh
>> +
>> +test_description='log --function-name'
>> +. ./test-lib.sh
>> +
>> +test_expect_success setup '
>> +     echo "* diff=cpp" > .gitattributes

Broken &&-chain here, as well.

>> +
>> +     >file &&
>> +     git add file &&
>> +     test_tick &&
>> +     git commit -m initial &&
>> +
>> +     printf "int main(){\n\treturn 0;\n}\n" >> file &&
>> +     test_tick &&
>> +     git commit -am second
>
> Broken && chain here and later as well. Please be careful.
>
>> +
>> +     printf "void newfunc(){\n\treturn;\n}\n" >> file &&
>> +     test_tick &&
>> +     git commit -am third
>
>         git commit -am "append a function" &&
>
>> +
>> +     printf "void newfunc2(){\n\treturn;\n}\n" | cat - file > temp &&
>> +     mv temp file &&
>> +     test_tick &&
>> +     git commit -am fourth
>
>         git commit -am "prepend a function" &&
>
> etc. You get the picture.
>
>> +
>> +     printf "void newfunc3(){\n\treturn;\n}\n" | cat - file > temp &&
>> +     mv temp file &&
>> +     test_tick &&
>> +     git commit -am fifth
>> +
>> +     sed -i -e "s/void newfunc2/void newfunc4/" file &&
>> +     test_tick &&
>> +     git commit -am sixth
>> +'
>> +
>> +test_expect_success 'log --function-name=main' '
>
> test_expect_success 'log --function-name finds a function with a change' '
>
>> +     git log --function-name=main >actual &&
>> +     git log --grep=second >expect &&
>> +     test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'log --function-name "newfunc\W"' '
>
> test_expect_success 'log --function-name with extended regexp' '
>
> etc. You get the picture.
>
>> +     git log --function-name "newfunc\W" >actual &&
>> +     git log --grep=third >expect &&
>> +     test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'log --function-name "newfunc2"' '
>> +     git log --function-name newfunc2 >actual &&
>> +     git log -E --grep "sixth|fourth" >expect &&
>> +     test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'log --function-name "newfunc3"' '
>> +     git log --function-name newfunc3 >actual &&
>> +     git log --grep=fifth >expect &&
>> +     test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'log --function-name "newfunc4"' '
>> +     git log --function-name newfunc4 >actual &&
>> +     git log --grep=sixth >expect &&
>> +     test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'log --function-name "newfunc"' '
>> +     git log --function-name newfunc >actual &&
>> +     git log -E --grep "third|fourth|fifth|sixth" >expect &&
>> +     test_cmp expect actual
>> +'
>> +
>> +test_done
>>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/10] t4213: test --function-name option

[Johannes Sixt]

>> +	sed -i -e "s/void newfunc2/void newfunc4/" file &&

I forgot to mention that sed -i is not portable.

-- Hannes

Re: [PATCH 09/10] t4213: test --function-name option

[Jakub Narębski]

W dniu 2014-03-27 19:50, David A. Dalrymple (and Bhushan G. Lodha) pisze:

> +test_expect_success setup '
> +	echo "* diff=cpp" > .gitattributes
> +
> +	>file &&
> +	git add file &&
> +	test_tick &&
> +	git commit -m initial &&
> +
> +	printf "int main(){\n\treturn 0;\n}\n" >> file &&
> +	test_tick &&
> +	git commit -am second

Wouldn't it be more readable to use "cat \-EOF" or separate files
instead of printf-s with embedded newlines "\n" and tabs "\t"?

-- 
Jakub Narębski

[PATCH 10/10] Documentation: Document --function-name usage

[David A. Dalrymple (and Bhushan G. Lodha)]

From: "Bhushan Lodha & David A. Dalrymple" <dad-bgl@mit.edu>

Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
---
 Documentation/diff-options.txt |  9 +++++++++
 Documentation/gitdiffcore.txt  | 17 ++++++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 9b37b2a..a778dff 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -427,6 +427,15 @@ information.
 --pickaxe-regex::
 	Treat the <string> given to `-S` as an extended POSIX regular
 	expression to match.
+
+--function-name<regex>::
+	Look for differences whose patch text contains hunk headers that match
+	<regex>. This can be useful to locate changes to a particular function
+	or other semantic element in a program, since hunk headers are intended
+	to indicate the "function context" (in the sense of
+	`--function-context`) in which the particular change occurs. The
+	function context is determined by the diff driver corresponding to the
+	file in question; see linkgit:gitattributes[7] for details.
 endif::git-format-patch[]
 
 -O<orderfile>::
diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt
index c8b3e51..b8477ce 100644
--- a/Documentation/gitdiffcore.txt
+++ b/Documentation/gitdiffcore.txt
@@ -222,10 +222,11 @@ version prefixed with '+'.
 diffcore-pickaxe: For Detecting Addition/Deletion of Specified String
 ---------------------------------------------------------------------
 
-This transformation limits the set of filepairs to those that change
+This transformation limits the set of filepairs to those that involve
 specified strings between the preimage and the postimage in a certain
-way.  -S<block of text> and -G<regular expression> options are used to
-specify different ways these strings are sought.
+way.  -S<block of text>, -G<regular expression>, and
+--function-name<regular expression> options are used to specify
+different ways these strings are sought.
 
 "-S<block of text>" detects filepairs whose preimage and postimage
 have different number of occurrences of the specified block of text.
@@ -251,6 +252,16 @@ criterion in a changeset, the entire changeset is kept.  This behavior
 is designed to make reviewing changes in the context of the whole
 changeset easier.
 
+"--function-name<regular expression>" detects filepairs whose textual
+diff contains a hunk header that matches the given regular expression.
+The hunk header is generated via the diff driver specified in
+`.gitattributes`, and is intended to reflect the "function context"
+(in the sense of `--function-context`) in which the change occurs,
+with programming-language-dependent heuristics. As of now, the
+programming language is not auto-detected in any way. Also note that
+hunks whose headers do not match the regular expression are not
+currently filtered out; this is only a filepair filter.
+
 diffcore-order: For Sorting the Output Based on Filenames
 ---------------------------------------------------------
 
-- 
1.7.12.4 (Apple Git-37)

Re: [PATCH 10/10] Documentation: Document --function-name usage

[Eric Sunshine]

On Thu, Mar 27, 2014 at 2:50 PM, David A. Dalrymple (and Bhushan G.
Lodha) <dad-bgl@mit.edu> wrote:
> From: "Bhushan Lodha & David A. Dalrymple" <dad-bgl@mit.edu>
>
> Signed-off-by: David Dalrymple (on zayin) <davidad@alum.mit.edu>
> ---
>  Documentation/diff-options.txt |  9 +++++++++
>  Documentation/gitdiffcore.txt  | 17 ++++++++++++++---
>  2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 9b37b2a..a778dff 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -427,6 +427,15 @@ information.
>  --pickaxe-regex::
>         Treat the <string> given to `-S` as an extended POSIX regular
>         expression to match.
> +
> +--function-name<regex>::

Missing '=' before <regex>. Ditto for the several additional instances
in this patch.

> +       Look for differences whose patch text contains hunk headers that match
> +       <regex>. This can be useful to locate changes to a particular function
> +       or other semantic element in a program, since hunk headers are intended
> +       to indicate the "function context" (in the sense of
> +       `--function-context`) in which the particular change occurs. The
> +       function context is determined by the diff driver corresponding to the
> +       file in question; see linkgit:gitattributes[7] for details.
>  endif::git-format-patch[]
>
>  -O<orderfile>::
> diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt
> index c8b3e51..b8477ce 100644
> --- a/Documentation/gitdiffcore.txt
> +++ b/Documentation/gitdiffcore.txt
> @@ -222,10 +222,11 @@ version prefixed with '+'.
>  diffcore-pickaxe: For Detecting Addition/Deletion of Specified String
>  ---------------------------------------------------------------------
>
> -This transformation limits the set of filepairs to those that change
> +This transformation limits the set of filepairs to those that involve
>  specified strings between the preimage and the postimage in a certain
> -way.  -S<block of text> and -G<regular expression> options are used to
> -specify different ways these strings are sought.
> +way.  -S<block of text>, -G<regular expression>, and
> +--function-name<regular expression> options are used to specify
> +different ways these strings are sought.
>
>  "-S<block of text>" detects filepairs whose preimage and postimage
>  have different number of occurrences of the specified block of text.
> @@ -251,6 +252,16 @@ criterion in a changeset, the entire changeset is kept.  This behavior
>  is designed to make reviewing changes in the context of the whole
>  changeset easier.
>
> +"--function-name<regular expression>" detects filepairs whose textual
> +diff contains a hunk header that matches the given regular expression.
> +The hunk header is generated via the diff driver specified in
> +`.gitattributes`, and is intended to reflect the "function context"
> +(in the sense of `--function-context`) in which the change occurs,
> +with programming-language-dependent heuristics. As of now, the
> +programming language is not auto-detected in any way. Also note that
> +hunks whose headers do not match the regular expression are not
> +currently filtered out; this is only a filepair filter.
> +
>  diffcore-order: For Sorting the Output Based on Filenames
>  ---------------------------------------------------------
>
> --
> 1.7.12.4 (Apple Git-37)
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/10] [RFC] pickaxe for function names

[Jeff King]

On Thu, Mar 27, 2014 at 02:50:46PM -0400, David A. Dalrymple (and Bhushan G. Lodha) wrote:

> This series introduces a --function-name=<pattern> option for git-log, intended
> to search for commits which touch a function matching a certain pattern (a
> feature we've seen requested and are interested in using ourselves).

How does your feature compare with the line-level history viewer? E.g.:

  git log -L:myfunc:foo.c

I guess by being part of pickaxe, it can be used to generally select
commits (whereas "-L" is about drilling down a particular set of lines,
so something like --full-diff would not work).

But "-L" can do many things that pickaxe can't. It is not just about
finding lines touched within a pattern, but uses the pattern to
determine an initial set of lines, and then recursively "blames" those
lines going back through history. So how you specify the pattern is more
flexible (you can give any line range, for example), and it should be
able to cross boundaries like function renames.

-Peff