gitk: add "grep diff" selection criterion (Re: find commit adding/removing could use changing option)

gitk: add "grep diff" selection criterion (Re: find commit adding/removing could use changing option)

[Jonathan Nieder]

Hi Chris,

Chris Smith wrote:

> One thing I find myself wishing for is the ability to find a line that has
> been modified by using gitk. It seems to me that if I have the following in a
> diff
> 
> - def foo(this)
> + def foo(this, that=False)
> 
> that I can't find the line by searching for foo.

Ah, so you want a cousin to ‘log -S’ that /does/ search through
diffs.  See [1].

As that thread explains, it is doable from the commandline already,
though not in a nicely packaged form.  But what about from gitk?

A possible hack would be to teach gitk to add arbitrary criteria to
the end of the ‘git diff-tree’ command line[2].  In this case, the
criterion would be ‘| search-diff $term’, where search-diff is a
script something like the following:

 #!/bin/sh
 while read -r commit
 do
	if
		git diff-tree -p -c "$commit" | grep -q "$@"
	then
		printf '%s\n' "$commit"
	fi
 done

If that proves useful, afterwards one could teach ‘diff-tree’ to do
the check itself, which would allow commands like

 git log --search-diff=foo

too.

Thoughts?
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/122478
[2] You can find the code one would need to touch for this by
searching for gdtargs and gdttype in the gitk script.

Re: gitk: add "grep diff" selection criterion (Re: find commit adding/removing could use changing option)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Ah, so you want a cousin to ‘log -S’ that /does/ search through
> diffs.  See [1].
> ...
> If that proves useful, afterwards one could teach ‘diff-tree’ to do
> the check itself...
> [1] http://thread.gmane.org/gmane.comp.version-control.git/122478

A better quote might have been:

    http://thread.gmane.org/gmane.comp.version-control.git/112077/focus=112114

And the attached should be a good starting point if anybody is interested.
I didn't bother to check for leaks and premature free()s, add tests nor
document it, though.

Not yet anyway.

-- >8 --
Subject: [PATCH] git log: add -G<regexp> that greps in the patch text

Teach "-G<regexp>" that is similar to "-S<regexp> --pickaxe-regexp" to the
"git diff" family of commands.  This limits the diff queue to filepairs
whose patch text actually has an added or deleted line that matches the
given regexp.  Unlike "-S<regexp>" changing other parts of the line that
has a substring that matches the given regexp IS counted as a change, as
such a change would appear as one deletion followed by one addition in a
patch text.

Unlike -S (pickaxe) that is intended to be used to quickly detect a commit
that changes the number of occurrences of hits between the preimage and
the postimage to serve as a part of larger toolchain, this new feature is
meant to be used as the top-level Porcelain feature.

This implementation unfortunately has to run "diff" twice if you are
running "log" family of commands to produce patches in the final output
(e.g. "git log -p" or "git format-patch").  I think we _could_ cache the
result in-core if we wanted to, but that would require larger surgery to
the diffcore machinery (i.e. adding an extra pointer in the filepair
structure to keep a pointer to a strbuf around, stuff the textual diff to
the strbuf inside diffgrep_consume(), and make use of it in later stages
when it is available) and it may not be worth it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile            |    1 +
 diff.c              |    6 ++-
 diff.h              |    1 +
 diffcore-log-grep.c |  145 +++++++++++++++++++++++++++++++++++++++++++++++++++
 diffcore.h          |    1 +
 5 files changed, 153 insertions(+), 1 deletions(-)
 create mode 100644 diffcore-log-grep.c

diff --git a/Makefile b/Makefile
index bc3c570..4213f40 100644
--- a/Makefile
+++ b/Makefile
@@ -562,6 +562,7 @@ LIB_OBJS += date.o
 LIB_OBJS += decorate.o
 LIB_OBJS += diffcore-break.o
 LIB_OBJS += diffcore-delta.o
+LIB_OBJS += diffcore-log-grep.o
 LIB_OBJS += diffcore-order.o
 LIB_OBJS += diffcore-pickaxe.o
 LIB_OBJS += diffcore-rename.o
diff --git a/diff.c b/diff.c
index 17873f3..f4bf1fd 100644
--- a/diff.c
+++ b/diff.c
@@ -2904,7 +2904,7 @@ int 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->log_grep)
 		DIFF_OPT_SET(options, RECURSIVE);
 	/*
 	 * When patches are generated, submodules diffed against the work tree
@@ -3184,6 +3184,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->rename_limit = strtoul(arg+2, NULL, 10);
 	else if (!prefixcmp(arg, "-S"))
 		options->pickaxe = arg + 2;
+	else if (!prefixcmp(arg, "-G"))
+		options->log_grep = arg + 2;
 	else if (!strcmp(arg, "--pickaxe-all"))
 		options->pickaxe_opts = DIFF_PICKAXE_ALL;
 	else if (!strcmp(arg, "--pickaxe-regex"))
@@ -4075,6 +4077,8 @@ void diffcore_std(struct diff_options *options)
 		diffcore_merge_broken();
 	if (options->pickaxe)
 		diffcore_pickaxe(options->pickaxe, options->pickaxe_opts);
+	if (options->log_grep)
+		diffcore_log_grep(options->log_grep, options->pickaxe_opts);
 	if (options->orderfile)
 		diffcore_order(options->orderfile);
 	diff_resolve_rename_copy();
diff --git a/diff.h b/diff.h
index 063d10a..ebd128b 100644
--- a/diff.h
+++ b/diff.h
@@ -96,6 +96,7 @@ struct diff_options {
 	const char *filter;
 	const char *orderfile;
 	const char *pickaxe;
+	const char *log_grep;
 	const char *single_follow;
 	const char *a_prefix, *b_prefix;
 	unsigned flags;
diff --git a/diffcore-log-grep.c b/diffcore-log-grep.c
new file mode 100644
index 0000000..501ae46
--- /dev/null
+++ b/diffcore-log-grep.c
@@ -0,0 +1,145 @@
+/*
+ * Copyright (C) 2010 Junio C Hamano
+ */
+#include "cache.h"
+#include "diff.h"
+#include "diffcore.h"
+#include "xdiff-interface.h"
+
+struct diffgrep_cb {
+	regex_t *regexp;
+	int hit;
+};
+
+static void diffgrep_consume(void *priv, char *line, unsigned long len)
+{
+	struct diffgrep_cb *data = priv;
+	regmatch_t regmatch;
+	int hold;
+
+	if (line[0] != '+' && line[0] != '-')
+		return;
+	if (data->hit)
+		/*
+		 * NEEDSWORK: we should have a way to terminate the
+		 * caller early.
+		 */
+		return;
+	/* Yuck -- line ought to be "const char *"! */
+	hold = line[len];
+	line[len] = '\0';
+	data->hit = !regexec(data->regexp, line + 1, 1, &regmatch, 0);
+	line[len] = hold;
+}
+
+static void fill_one(struct diff_filespec *one,
+		     mmfile_t *mf, struct userdiff_driver **textconv)
+{
+	if (DIFF_FILE_VALID(one)) {
+		*textconv = get_textconv(one);
+		mf->size = fill_textconv(*textconv, one, &mf->ptr);
+	} else {
+		memset(mf, 0, sizeof(*mf));
+	}
+}
+
+static int diff_grep(struct diff_filepair *p, regex_t *regexp)
+{
+	regmatch_t regmatch;
+	struct userdiff_driver *textconv_one = NULL;
+	struct userdiff_driver *textconv_two = NULL;
+	mmfile_t mf1, mf2;
+	int hit;
+
+	if (diff_unmodified_pair(p))
+		return 0;
+
+	fill_one(p->one, &mf1, &textconv_one);
+	fill_one(p->two, &mf2, &textconv_two);
+
+	if (!mf1.ptr) {
+		if (!mf2.ptr)
+			return 0; /* ignore unmerged */
+		/* created "two" -- does it have what we are looking for? */
+		hit = !regexec(regexp, p->two->data, 1, &regmatch, 0);
+	} else if (!mf2.ptr) {
+		/* removed "one" -- did it have what we are looking for? */
+		hit = !regexec(regexp, p->one->data, 1, &regmatch, 0);
+	} else {
+		/*
+		 * We have both sides; need to run textual diff and see if a
+		 * line that match the pattern appears in +/- line.
+		 */
+		struct diffgrep_cb ecbdata;
+		xpparam_t xpp;
+		xdemitconf_t xecfg;
+
+		memset(&xpp, 0, sizeof(xpp));
+		memset(&xecfg, 0, sizeof(xecfg));
+		ecbdata.regexp = regexp;
+		ecbdata.hit = 0;
+		xdi_diff_outf(&mf1, &mf2, diffgrep_consume, &ecbdata,
+			      &xpp, &xecfg);
+		hit = ecbdata.hit;
+	}
+	if (textconv_one)
+		free(mf1.ptr);
+	if (textconv_two)
+		free(mf2.ptr);
+	return hit;
+}
+
+void diffcore_log_grep(const char *needle, int opts)
+{
+	struct diff_queue_struct *q = &diff_queued_diff;
+	int i, has_changes, err;
+	regex_t regex, *regexp = NULL;
+	struct diff_queue_struct outq;
+	outq.queue = NULL;
+	outq.nr = outq.alloc = 0;
+
+	err = regcomp(&regex, needle, REG_EXTENDED | REG_NEWLINE);
+	if (err) {
+		char errbuf[1024];
+		regerror(err, &regex, errbuf, 1024);
+		regfree(&regex);
+		die("invalid log-grep regex: %s", errbuf);
+	}
+	regexp = &regex;
+
+	if (opts & DIFF_PICKAXE_ALL) {
+		/* Showing the whole changeset if needle exists */
+		for (i = has_changes = 0; !has_changes && i < q->nr; i++) {
+			struct diff_filepair *p = q->queue[i];
+			if (diff_grep(p, regexp))
+				has_changes++;
+		}
+		if (has_changes)
+			return; /* not munge the queue */
+
+		/* otherwise we will clear the whole queue
+		 * by copying the empty outq at the end of this
+		 * function, but first clear the current entries
+		 * in the queue.
+		 */
+		for (i = 0; i < q->nr; i++)
+			diff_free_filepair(q->queue[i]);
+	} else {
+		/* Showing only the filepairs that has the needle */
+		for (i = 0; i < q->nr; i++) {
+			struct diff_filepair *p = q->queue[i];
+			if (diff_grep(p, regexp))
+				diff_q(&outq, p);
+			else
+				diff_free_filepair(p);
+		}
+	}
+
+	if (opts & DIFF_PICKAXE_REGEX) {
+		regfree(&regex);
+	}
+
+	free(q->queue);
+	*q = outq;
+	return;
+}
diff --git a/diffcore.h b/diffcore.h
index 491bea0..e3f8f05 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -110,6 +110,7 @@ extern void diffcore_break(int);
 extern void diffcore_rename(struct diff_options *);
 extern void diffcore_merge_broken(void);
 extern void diffcore_pickaxe(const char *needle, int opts);
+extern void diffcore_log_grep(const char *needle, int opts);
 extern void diffcore_order(const char *orderfile);
 
 #define DIFF_DEBUG 0
-- 
1.7.2.rc3.270.gb7c60