[PATCH 0/7] pickaxe: plug memory leaks, deduplicate code

[PATCH 0/7] pickaxe: plug memory leaks, deduplicate code

[René Scharfe]

Hello,

this series aims to clean up the code for pickaxe (-S/-G).  The first
three patches plug minor to medium memory leaks.

	[PATCH 1/7] pickaxe: plug diff filespec leak with empty needle
	[PATCH 2/7] pickaxe: plug regex leak
	[PATCH 3/7] pickaxe: plug regex/kws leak

The next one moves a duplicate if/else cascade into its own helper
function:

	[PATCH 4/7] pickaxe: factor out has_changes

The remainder unifies the code for pickaxe (-S) and log grep (-G).

	[PATCH 5/7] pickaxe: pass diff_options to contains and has_changes
	[PATCH 6/7] pickaxe: give diff_grep the same signature as has_changes
	[PATCH 7/7] pickaxe: factor out pickaxe

As a result the code should be shorter and easier to maintain.

 diffcore-pickaxe.c |  178 ++++++++++++++++++++-------------------------------
 1 files changed, 70 insertions(+), 108 deletions(-)

[PATCH 1/7] pickaxe: plug diff filespec leak with empty needle

[René Scharfe]

Check first for the unlikely case of an empty needle string and only
then populate the filespec, lest we leak it.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 diffcore-pickaxe.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index c3760cf..0835a3b 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -152,10 +152,10 @@ static unsigned int contains(struct diff_filespec *one,
 	unsigned int cnt;
 	unsigned long sz;
 	const char *data;
-	if (diff_populate_filespec(one, 0))
-		return 0;
 	if (!len)
 		return 0;
+	if (diff_populate_filespec(one, 0))
+		return 0;
 
 	sz = one->size;
 	data = one->data;
-- 
1.7.7

[PATCH 2/7] pickaxe: plug regex leak

[René Scharfe]

With -G... --pickaxe-all, free the regex before returning even if we
found a match.  Also get rid of the variable has_changes, as we can
simply break out of the loop.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 diffcore-pickaxe.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 0835a3b..96f7ea6 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -96,7 +96,7 @@ static int diff_grep(struct diff_filepair *p, regex_t *regexp, struct diff_optio
 static void diffcore_pickaxe_grep(struct diff_options *o)
 {
 	struct diff_queue_struct *q = &diff_queued_diff;
-	int i, has_changes, err;
+	int i, err;
 	regex_t regex;
 	struct diff_queue_struct outq;
 	outq.queue = NULL;
@@ -112,13 +112,11 @@ static void diffcore_pickaxe_grep(struct diff_options *o)
 
 	if (o->pickaxe_opts & DIFF_PICKAXE_ALL) {
 		/* Showing the whole changeset if needle exists */
-		for (i = has_changes = 0; !has_changes && i < q->nr; i++) {
+		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (diff_grep(p, &regex, o))
-				has_changes++;
+				goto out; /* do not munge the queue */
 		}
-		if (has_changes)
-			return; /* do not munge the queue */
 
 		/*
 		 * Otherwise we will clear the whole queue by copying
@@ -138,10 +136,11 @@ static void diffcore_pickaxe_grep(struct diff_options *o)
 		}
 	}
 
-	regfree(&regex);
-
 	free(q->queue);
 	*q = outq;
+
+ out:
+	regfree(&regex);
 	return;
 }
 
-- 
1.7.7

[PATCH 3/7] pickaxe: plug regex/kws leak

[René Scharfe]

With -S... --pickaxe-all, free the regex or the kws before returning
even if we found a match.  Also get rid of the variable has_changes,
as we can simply break out of the loop.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 diffcore-pickaxe.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 96f7ea6..c367d8d 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -221,7 +221,7 @@ static void diffcore_pickaxe_count(struct diff_options *o)
 
 	if (opts & DIFF_PICKAXE_ALL) {
 		/* Showing the whole changeset if needle exists */
-		for (i = has_changes = 0; !has_changes && i < q->nr; i++) {
+		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (!DIFF_FILE_VALID(p->one)) {
 				if (!DIFF_FILE_VALID(p->two))
@@ -238,9 +238,9 @@ static void diffcore_pickaxe_count(struct diff_options *o)
 				 contains(p->one, needle, len, regexp, kws) !=
 				 contains(p->two, needle, len, regexp, kws))
 				has_changes++;
+			if (has_changes)
+				goto out; /* do not munge the queue */
 		}
-		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
@@ -278,13 +278,14 @@ static void diffcore_pickaxe_count(struct diff_options *o)
 				diff_free_filepair(p);
 		}
 
+	free(q->queue);
+	*q = outq;
+
+ out:
 	if (opts & DIFF_PICKAXE_REGEX)
 		regfree(&regex);
 	else
 		kwsfree(kws);
-
-	free(q->queue);
-	*q = outq;
 	return;
 }
 
-- 
1.7.7

[PATCH 4/7] pickaxe: factor out has_changes

[René Scharfe]

Move duplicate if/else construct into its own helper function.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 diffcore-pickaxe.c |   57 +++++++++++++++++++--------------------------------
 1 files changed, 21 insertions(+), 36 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index c367d8d..4347ec1 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -190,13 +190,31 @@ static unsigned int contains(struct diff_filespec *one,
 	return cnt;
 }
 
+static int has_changes(struct diff_filepair *p, const char *needle,
+		       unsigned long len, regex_t *regexp, kwset_t kws)
+{
+	if (!DIFF_FILE_VALID(p->one)) {
+		if (!DIFF_FILE_VALID(p->two))
+			return 0; /* ignore unmerged */
+		/* created */
+		return contains(p->two, needle, len, regexp, kws) != 0;
+	}
+	if (!DIFF_FILE_VALID(p->two))
+		return contains(p->one, needle, len, regexp, kws) != 0;
+	if (!diff_unmodified_pair(p)) {
+		return contains(p->one, needle, len, regexp, kws) !=
+		       contains(p->two, needle, len, regexp, kws);
+	}
+	return 0;
+}
+
 static void diffcore_pickaxe_count(struct diff_options *o)
 {
 	const char *needle = o->pickaxe;
 	int opts = o->pickaxe_opts;
 	struct diff_queue_struct *q = &diff_queued_diff;
 	unsigned long len = strlen(needle);
-	int i, has_changes;
+	int i;
 	regex_t regex, *regexp = NULL;
 	kwset_t kws = NULL;
 	struct diff_queue_struct outq;
@@ -223,22 +241,7 @@ static void diffcore_pickaxe_count(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 (!DIFF_FILE_VALID(p->one)) {
-				if (!DIFF_FILE_VALID(p->two))
-					continue; /* ignore unmerged */
-				/* created */
-				if (contains(p->two, needle, len, regexp, kws))
-					has_changes++;
-			}
-			else if (!DIFF_FILE_VALID(p->two)) {
-				if (contains(p->one, needle, len, regexp, kws))
-					has_changes++;
-			}
-			else if (!diff_unmodified_pair(p) &&
-				 contains(p->one, needle, len, regexp, kws) !=
-				 contains(p->two, needle, len, regexp, kws))
-				has_changes++;
-			if (has_changes)
+			if (has_changes(p, needle, len, regexp, kws))
 				goto out; /* do not munge the queue */
 		}
 
@@ -254,25 +257,7 @@ static void diffcore_pickaxe_count(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];
-			has_changes = 0;
-			if (!DIFF_FILE_VALID(p->one)) {
-				if (!DIFF_FILE_VALID(p->two))
-					; /* ignore unmerged */
-				/* created */
-				else if (contains(p->two, needle, len, regexp,
-						  kws))
-					has_changes = 1;
-			}
-			else if (!DIFF_FILE_VALID(p->two)) {
-				if (contains(p->one, needle, len, regexp, kws))
-					has_changes = 1;
-			}
-			else if (!diff_unmodified_pair(p) &&
-				 contains(p->one, needle, len, regexp, kws) !=
-				 contains(p->two, needle, len, regexp, kws))
-				has_changes = 1;
-
-			if (has_changes)
+			if (has_changes(p, needle, len, regexp, kws))
 				diff_q(&outq, p);
 			else
 				diff_free_filepair(p);
-- 
1.7.7

[PATCH 5/7] pickaxe: pass diff_options to contains and has_changes

[René Scharfe]

Remove the unused parameter needle from contains() and has_changes().

Also replace the parameter len with a pointer to the diff_options.  We
can use its member pickaxe to check if the needle is an empty string
and use the kwsmatch structure to find out the length of the match
instead.

This change is done as a preparation to unify the signatures of
has_changes() and diff_grep(), which will be used in the patch after
the next one to factor out common code.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 diffcore-pickaxe.c |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 4347ec1..4d66ba9 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -144,14 +144,13 @@ static void diffcore_pickaxe_grep(struct diff_options *o)
 	return;
 }
 
-static unsigned int contains(struct diff_filespec *one,
-			     const char *needle, unsigned long len,
+static unsigned int contains(struct diff_filespec *one, struct diff_options *o,
 			     regex_t *regexp, kwset_t kws)
 {
 	unsigned int cnt;
 	unsigned long sz;
 	const char *data;
-	if (!len)
+	if (!o->pickaxe[0])
 		return 0;
 	if (diff_populate_filespec(one, 0))
 		return 0;
@@ -175,14 +174,15 @@ static unsigned int contains(struct diff_filespec *one,
 
 	} else { /* Classic exact string match */
 		while (sz) {
-			size_t offset = kwsexec(kws, data, sz, NULL);
+			struct kwsmatch kwsm;
+			size_t offset = kwsexec(kws, data, sz, &kwsm);
 			const char *found;
 			if (offset == -1)
 				break;
 			else
 				found = data + offset;
-			sz -= found - data + len;
-			data = found + len;
+			sz -= found - data + kwsm.size[0];
+			data = found + kwsm.size[0];
 			cnt++;
 		}
 	}
@@ -190,20 +190,20 @@ static unsigned int contains(struct diff_filespec *one,
 	return cnt;
 }
 
-static int has_changes(struct diff_filepair *p, const char *needle,
-		       unsigned long len, regex_t *regexp, kwset_t kws)
+static int has_changes(struct diff_filepair *p, struct diff_options *o,
+		       regex_t *regexp, kwset_t kws)
 {
 	if (!DIFF_FILE_VALID(p->one)) {
 		if (!DIFF_FILE_VALID(p->two))
 			return 0; /* ignore unmerged */
 		/* created */
-		return contains(p->two, needle, len, regexp, kws) != 0;
+		return contains(p->two, o, regexp, kws) != 0;
 	}
 	if (!DIFF_FILE_VALID(p->two))
-		return contains(p->one, needle, len, regexp, kws) != 0;
+		return contains(p->one, o, regexp, kws) != 0;
 	if (!diff_unmodified_pair(p)) {
-		return contains(p->one, needle, len, regexp, kws) !=
-		       contains(p->two, needle, len, regexp, kws);
+		return contains(p->one, o, regexp, kws) !=
+		       contains(p->two, o, regexp, kws);
 	}
 	return 0;
 }
@@ -241,7 +241,7 @@ static void diffcore_pickaxe_count(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 (has_changes(p, needle, len, regexp, kws))
+			if (has_changes(p, o, regexp, kws))
 				goto out; /* do not munge the queue */
 		}
 
@@ -257,7 +257,7 @@ static void diffcore_pickaxe_count(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 (has_changes(p, needle, len, regexp, kws))
+			if (has_changes(p, o, regexp, kws))
 				diff_q(&outq, p);
 			else
 				diff_free_filepair(p);
-- 
1.7.7

[PATCH 6/7] pickaxe: give diff_grep the same signature as has_changes

[René Scharfe]

Change diff_grep() to match the signature of has_changes() as a
preparation for the next patch that will use function pointers to
the two.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 diffcore-pickaxe.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 4d66ba9..226fa0c 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -45,7 +45,8 @@ static void fill_one(struct diff_filespec *one,
 	}
 }
 
-static int diff_grep(struct diff_filepair *p, regex_t *regexp, struct diff_options *o)
+static int diff_grep(struct diff_filepair *p, struct diff_options *o,
+		     regex_t *regexp, kwset_t kws)
 {
 	regmatch_t regmatch;
 	struct userdiff_driver *textconv_one = NULL;
@@ -114,7 +115,7 @@ static void diffcore_pickaxe_grep(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 (diff_grep(p, &regex, o))
+			if (diff_grep(p, o, &regex, NULL))
 				goto out; /* do not munge the queue */
 		}
 
@@ -129,7 +130,7 @@ static void diffcore_pickaxe_grep(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 (diff_grep(p, &regex, o))
+			if (diff_grep(p, o, &regex, NULL))
 				diff_q(&outq, p);
 			else
 				diff_free_filepair(p);
-- 
1.7.7

[PATCH 7/7] pickaxe: factor out pickaxe

[René Scharfe]

Move the duplicate diff queue loop into its own function that accepts
a match function: has_changes() for -S and diff_grep() for -G.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 diffcore-pickaxe.c |  110 ++++++++++++++++++++-------------------------------
 1 files changed, 43 insertions(+), 67 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 226fa0c..380a837 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -8,6 +8,46 @@
 #include "xdiff-interface.h"
 #include "kwset.h"
 
+typedef int (*pickaxe_fn)(struct diff_filepair *p, struct diff_options *o, regex_t *regexp, kwset_t kws);
+
+static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,
+		    regex_t *regexp, kwset_t kws, pickaxe_fn fn)
+{
+	int i;
+	struct diff_queue_struct outq;
+
+	DIFF_QUEUE_CLEAR(&outq);
+
+	if (o->pickaxe_opts & DIFF_PICKAXE_ALL) {
+		/* Showing the whole changeset if needle exists */
+		for (i = 0; i < q->nr; i++) {
+			struct diff_filepair *p = q->queue[i];
+			if (fn(p, o, regexp, kws))
+				return; /* do 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 (fn(p, o, regexp, kws))
+				diff_q(&outq, p);
+			else
+				diff_free_filepair(p);
+		}
+	}
+
+	free(q->queue);
+	*q = outq;
+}
+
 struct diffgrep_cb {
 	regex_t *regexp;
 	int hit;
@@ -96,12 +136,8 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,
 
 static void diffcore_pickaxe_grep(struct diff_options *o)
 {
-	struct diff_queue_struct *q = &diff_queued_diff;
-	int i, err;
+	int err;
 	regex_t regex;
-	struct diff_queue_struct outq;
-	outq.queue = NULL;
-	outq.nr = outq.alloc = 0;
 
 	err = regcomp(&regex, o->pickaxe, REG_EXTENDED | REG_NEWLINE);
 	if (err) {
@@ -111,36 +147,8 @@ static void diffcore_pickaxe_grep(struct diff_options *o)
 		die("invalid log-grep regex: %s", errbuf);
 	}
 
-	if (o->pickaxe_opts & DIFF_PICKAXE_ALL) {
-		/* Showing the whole changeset if needle exists */
-		for (i = 0; i < q->nr; i++) {
-			struct diff_filepair *p = q->queue[i];
-			if (diff_grep(p, o, &regex, NULL))
-				goto out; /* do not munge the queue */
-		}
+	pickaxe(&diff_queued_diff, o, &regex, NULL, diff_grep);
 
-		/*
-		 * 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, o, &regex, NULL))
-				diff_q(&outq, p);
-			else
-				diff_free_filepair(p);
-		}
-	}
-
-	free(q->queue);
-	*q = outq;
-
- out:
 	regfree(&regex);
 	return;
 }
@@ -213,13 +221,9 @@ static void diffcore_pickaxe_count(struct diff_options *o)
 {
 	const char *needle = o->pickaxe;
 	int opts = o->pickaxe_opts;
-	struct diff_queue_struct *q = &diff_queued_diff;
 	unsigned long len = strlen(needle);
-	int i;
 	regex_t regex, *regexp = NULL;
 	kwset_t kws = NULL;
-	struct diff_queue_struct outq;
-	DIFF_QUEUE_CLEAR(&outq);
 
 	if (opts & DIFF_PICKAXE_REGEX) {
 		int err;
@@ -238,36 +242,8 @@ static void diffcore_pickaxe_count(struct diff_options *o)
 		kwsprep(kws);
 	}
 
-	if (opts & DIFF_PICKAXE_ALL) {
-		/* Showing the whole changeset if needle exists */
-		for (i = 0; i < q->nr; i++) {
-			struct diff_filepair *p = q->queue[i];
-			if (has_changes(p, o, regexp, kws))
-				goto out; /* do 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 (has_changes(p, o, regexp, kws))
-				diff_q(&outq, p);
-			else
-				diff_free_filepair(p);
-		}
-
-	free(q->queue);
-	*q = outq;
+	pickaxe(&diff_queued_diff, o, regexp, kws, has_changes);
 
- out:
 	if (opts & DIFF_PICKAXE_REGEX)
 		regfree(&regex);
 	else
-- 
1.7.7

Re: [PATCH 0/7] pickaxe: plug memory leaks, deduplicate code

[Junio C Hamano]

Impressed; very nicely done.

Thanks.