Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G
Jeff King p...@peff.net writes:
I didn't actually test that patch beyond compilation (but it's
_obviously_ correct, right?), and I'm about to go to bed. Do you want to
take care of adapting your commit message to it?
The code looks obviously correct, yes.
We might have to revisit the Is unmerged? thing, Cf. d7c9bf22351e
(diffcore-rename: don't consider unmerged path as source,
2011-03-23), but that is an unlikely corner case, especially for
these options (-S/-G).
-- 8 --
From: Jeff King p...@peff.net
Date: Fri, 5 Apr 2013 01:28:10 -0400
Subject: [PATCH] diffcore-pickaxe: unify code for log -S/-G
The logic flow of has_changes() used for log -S and diff_grep()
used for log -G are essentially the same. See if we have both
sides that could be different in any interesting way, slurp the
contents in core, possibly after applying textconv, inspect the
contents, clean-up and report the result. The only difference
between the two is how inspect step works.
Unify this codeflow in a helper, pickaxe_match(), which takes a
callback function that implements the specific inspect step.
After removing the common scaffolding code from the existing
has_changes() and diff_grep(), they each becomes such a callback
function suitable for passing to pickaxe_match().
Signed-off-by: Jeff King p...@peff.net
Signed-off-by: Junio C Hamano gits...@pobox.com
---
diffcore-pickaxe.c | 118 ++---
1 file changed, 49 insertions(+), 69 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index cadb071..63722f8 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -8,7 +8,12 @@
#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);
+typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
+ struct diff_options *o,
+ regex_t *regexp, kwset_t kws);
+
+static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
+regex_t *regexp, kwset_t kws, pickaxe_fn fn);
static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,
regex_t *regexp, kwset_t kws, pickaxe_fn fn)
@@ -22,7 +27,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 (fn(p, o, regexp, kws))
+ if (pickaxe_match(p, o, regexp, kws, fn))
return; /* do not munge the queue */
}
@@ -37,7 +42,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 (fn(p, o, regexp, kws))
+ if (pickaxe_match(p, o, regexp, kws, fn))
diff_q(outq, p);
else
diff_free_filepair(p);
@@ -74,64 +79,33 @@ static void diffgrep_consume(void *priv, char *line,
unsigned long len)
line[len] = hold;
}
-static int diff_grep(struct diff_filepair *p, struct diff_options *o,
+static int diff_grep(mmfile_t *one, mmfile_t *two,
+struct diff_options *o,
regex_t *regexp, kwset_t kws)
{
regmatch_t regmatch;
- struct userdiff_driver *textconv_one = NULL;
- struct userdiff_driver *textconv_two = NULL;
- mmfile_t mf1, mf2;
- int hit;
+ struct diffgrep_cb ecbdata;
+ xpparam_t xpp;
+ xdemitconf_t xecfg;
- if (!o-pickaxe[0])
- return 0;
+ if (!one)
+ return !regexec(regexp, two-ptr, 1, regmatch, 0);
+ if (!two)
+ return !regexec(regexp, one-ptr, 1, regmatch, 0);
- if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
- textconv_one = get_textconv(p-one);
- textconv_two = get_textconv(p-two);
- }
-
- if (textconv_one == textconv_two diff_unmodified_pair(p))
- return 0;
-
- mf1.size = fill_textconv(textconv_one, p-one, mf1.ptr);
- mf2.size = fill_textconv(textconv_two, p-two, mf2.ptr);
-
- if (!DIFF_FILE_VALID(p-one)) {
- if (!DIFF_FILE_VALID(p-two))
- hit = 0; /* ignore unmerged */
- else
- /* created two -- does it have what we are looking
for? */
- hit = !regexec(regexp, mf2.ptr, 1, regmatch, 0);
- } else if (!DIFF_FILE_VALID(p-two)) {
- /* removed one -- did it have what we are looking for? */
- hit = !regexec(regexp, mf1.ptr, 1, regmatch, 0);
- } else {
-
[PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G
The logic to decide early to do nothing and prepare the data to be
inspected are the same between has_changes() and diff_grep().
Introduce pickaxe_setup() helper to share the same code.
Similarly, introduce pickaxe_finish_filepair() to clean up after
these two functions are done with a filepair.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
diffcore-pickaxe.c | 103 -
1 file changed, 55 insertions(+), 48 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index cadb071..ac5a28d 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -48,6 +48,54 @@ static void pickaxe(struct diff_queue_struct *q, struct
diff_options *o,
*q = outq;
}
+static int pickaxe_setup(struct diff_filepair *p,
+struct diff_options *o,
+mmfile_t *mf_one,
+mmfile_t *mf_two,
+unsigned *what_to_free)
+{
+ struct userdiff_driver *textconv_one = NULL;
+ struct userdiff_driver *textconv_two = NULL;
+
+ if (!o-pickaxe[0])
+ return 0;
+
+ if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
+ textconv_one = get_textconv(p-one);
+ textconv_two = get_textconv(p-two);
+ }
+
+ /*
+* If we have an unmodified pair, we know that there is no
+* interesting difference and we don't even have to load the
+* blobs, unless textconv is in play, _and_ we are using two
+* different textconv filters (e.g., because a pair is an
+* exact rename with different textconv attributes for each
+* side, which might generate different content).
+*/
+ if (textconv_one == textconv_two diff_unmodified_pair(p))
+ return 0;
+
+ mf_one-size = fill_textconv(textconv_one, p-one, mf_one-ptr);
+ mf_two-size = fill_textconv(textconv_two, p-two, mf_two-ptr);
+
+ *what_to_free = (textconv_one ? 1 : 0) | (textconv_two ? 2 : 0);
+ return 1;
+}
+
+static void pickaxe_finish_filepair(struct diff_filepair *p,
+ mmfile_t *mf_one,
+ mmfile_t *mf_two,
+ unsigned what_to_free)
+{
+ if (what_to_free 1)
+ free(mf_one-ptr);
+ if (what_to_free 2)
+ free(mf_two-ptr);
+ diff_free_filespec_data(p-one);
+ diff_free_filespec_data(p-two);
+}
+
struct diffgrep_cb {
regex_t *regexp;
int hit;
@@ -78,25 +126,13 @@ 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;
- struct userdiff_driver *textconv_two = NULL;
mmfile_t mf1, mf2;
+ unsigned what_to_free;
int hit;
- if (!o-pickaxe[0])
+ if (!pickaxe_setup(p, o, mf1, mf2, what_to_free))
return 0;
- if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
- textconv_one = get_textconv(p-one);
- textconv_two = get_textconv(p-two);
- }
-
- if (textconv_one == textconv_two diff_unmodified_pair(p))
- return 0;
-
- mf1.size = fill_textconv(textconv_one, p-one, mf1.ptr);
- mf2.size = fill_textconv(textconv_two, p-two, mf2.ptr);
-
if (!DIFF_FILE_VALID(p-one)) {
if (!DIFF_FILE_VALID(p-two))
hit = 0; /* ignore unmerged */
@@ -125,12 +161,8 @@ static int diff_grep(struct diff_filepair *p, struct
diff_options *o,
xpp, xecfg);
hit = ecbdata.hit;
}
- if (textconv_one)
- free(mf1.ptr);
- if (textconv_two)
- free(mf2.ptr);
- diff_free_filespec_data(p-one);
- diff_free_filespec_data(p-two);
+
+ pickaxe_finish_filepair(p, mf1, mf2, what_to_free);
return hit;
}
@@ -201,32 +233,13 @@ static unsigned int contains(mmfile_t *mf, struct
diff_options *o,
static int has_changes(struct diff_filepair *p, struct diff_options *o,
regex_t *regexp, kwset_t kws)
{
- struct userdiff_driver *textconv_one = NULL;
- struct userdiff_driver *textconv_two = NULL;
mmfile_t mf1, mf2;
+ unsigned what_to_free;
int ret;
- if (!o-pickaxe[0])
- return 0;
-
- if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
- textconv_one = get_textconv(p-one);
- textconv_two = get_textconv(p-two);
- }
-
- /*
-* If we have an unmodified pair, we know that the count will be the
-* same and don't even have to load the blobs. Unless textconv is in
-* play, _and_ we are using two different textconv filters (e.g.,
-* because a pair is an exact rename with different textconv attributes
-* for each side, which might
Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G
On Thu, Apr 04, 2013 at 09:45:26PM -0700, Junio C Hamano wrote:
The logic to decide early to do nothing and prepare the data to be
inspected are the same between has_changes() and diff_grep().
Introduce pickaxe_setup() helper to share the same code.
Similarly, introduce pickaxe_finish_filepair() to clean up after
these two functions are done with a filepair.
All three patches look fine to me.
I notice that you are stuck factoring out not just the setup, but also
the cleanup, and I wondered if things could be made even simpler by just
encapsulating the checking logic in a callback; then the setup and
cleanup flow more naturally, as they are in a single function wrapper.
Like this, which ends up saving 20 lines rather than adding 7:
---
diffcore-pickaxe.c | 118 +++
1 file changed, 49 insertions(+), 69 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index cadb071..63722f8 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -8,7 +8,12 @@
#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);
+typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
+ struct diff_options *o,
+ regex_t *regexp, kwset_t kws);
+
+static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
+regex_t *regexp, kwset_t kws, pickaxe_fn fn);
static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,
regex_t *regexp, kwset_t kws, pickaxe_fn fn)
@@ -22,7 +27,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 (fn(p, o, regexp, kws))
+ if (pickaxe_match(p, o, regexp, kws, fn))
return; /* do not munge the queue */
}
@@ -37,7 +42,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 (fn(p, o, regexp, kws))
+ if (pickaxe_match(p, o, regexp, kws, fn))
diff_q(outq, p);
else
diff_free_filepair(p);
@@ -74,64 +79,33 @@ static int diff_grep(struct diff_filepair *p, struct
diff_options *o,
line[len] = hold;
}
-static int diff_grep(struct diff_filepair *p, struct diff_options *o,
+static int diff_grep(mmfile_t *one, mmfile_t *two,
+struct diff_options *o,
regex_t *regexp, kwset_t kws)
{
regmatch_t regmatch;
- struct userdiff_driver *textconv_one = NULL;
- struct userdiff_driver *textconv_two = NULL;
- mmfile_t mf1, mf2;
- int hit;
+ struct diffgrep_cb ecbdata;
+ xpparam_t xpp;
+ xdemitconf_t xecfg;
- if (!o-pickaxe[0])
- return 0;
+ if (!one)
+ return !regexec(regexp, two-ptr, 1, regmatch, 0);
+ if (!two)
+ return !regexec(regexp, one-ptr, 1, regmatch, 0);
- if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
- textconv_one = get_textconv(p-one);
- textconv_two = get_textconv(p-two);
- }
-
- if (textconv_one == textconv_two diff_unmodified_pair(p))
- return 0;
-
- mf1.size = fill_textconv(textconv_one, p-one, mf1.ptr);
- mf2.size = fill_textconv(textconv_two, p-two, mf2.ptr);
-
- if (!DIFF_FILE_VALID(p-one)) {
- if (!DIFF_FILE_VALID(p-two))
- hit = 0; /* ignore unmerged */
- else
- /* created two -- does it have what we are looking
for? */
- hit = !regexec(regexp, mf2.ptr, 1, regmatch, 0);
- } else if (!DIFF_FILE_VALID(p-two)) {
- /* removed one -- did it have what we are looking for? */
- hit = !regexec(regexp, mf1.ptr, 1, regmatch, 0);
- } else {
- /*
-* We have both sides; need to run textual diff and see if
-* the pattern appears on added/deleted lines.
-*/
- 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;
- xecfg.ctxlen = o-context;
- xecfg.interhunkctxlen = o-interhunkcontext;
- xdi_diff_outf(mf1, mf2, diffgrep_consume, ecbdata,
-
Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G
Jeff King p...@peff.net writes:
I notice that you are stuck factoring out not just the setup, but also
the cleanup, and I wondered if things could be made even simpler by just
encapsulating the checking logic in a callback; then the setup and
cleanup flow more naturally, as they are in a single function wrapper.
Like this, which ends up saving 20 lines rather than adding 7:
Oh, this is one of those many times I am reminded why I love having
you in the reviewer/contributor pool ;-)
---
diffcore-pickaxe.c | 118 +++
1 file changed, 49 insertions(+), 69 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index cadb071..63722f8 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -8,7 +8,12 @@
#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);
+typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
+ struct diff_options *o,
+ regex_t *regexp, kwset_t kws);
+
+static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
+ regex_t *regexp, kwset_t kws, pickaxe_fn fn);
static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,
regex_t *regexp, kwset_t kws, pickaxe_fn fn)
@@ -22,7 +27,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 (fn(p, o, regexp, kws))
+ if (pickaxe_match(p, o, regexp, kws, fn))
return; /* do not munge the queue */
}
@@ -37,7 +42,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 (fn(p, o, regexp, kws))
+ if (pickaxe_match(p, o, regexp, kws, fn))
diff_q(outq, p);
else
diff_free_filepair(p);
@@ -74,64 +79,33 @@ static int diff_grep(struct diff_filepair *p, struct
diff_options *o,
line[len] = hold;
}
-static int diff_grep(struct diff_filepair *p, struct diff_options *o,
+static int diff_grep(mmfile_t *one, mmfile_t *two,
+ struct diff_options *o,
regex_t *regexp, kwset_t kws)
{
regmatch_t regmatch;
- struct userdiff_driver *textconv_one = NULL;
- struct userdiff_driver *textconv_two = NULL;
- mmfile_t mf1, mf2;
- int hit;
+ struct diffgrep_cb ecbdata;
+ xpparam_t xpp;
+ xdemitconf_t xecfg;
- if (!o-pickaxe[0])
- return 0;
+ if (!one)
+ return !regexec(regexp, two-ptr, 1, regmatch, 0);
+ if (!two)
+ return !regexec(regexp, one-ptr, 1, regmatch, 0);
- if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
- textconv_one = get_textconv(p-one);
- textconv_two = get_textconv(p-two);
- }
-
- if (textconv_one == textconv_two diff_unmodified_pair(p))
- return 0;
-
- mf1.size = fill_textconv(textconv_one, p-one, mf1.ptr);
- mf2.size = fill_textconv(textconv_two, p-two, mf2.ptr);
-
- if (!DIFF_FILE_VALID(p-one)) {
- if (!DIFF_FILE_VALID(p-two))
- hit = 0; /* ignore unmerged */
- else
- /* created two -- does it have what we are looking
for? */
- hit = !regexec(regexp, mf2.ptr, 1, regmatch, 0);
- } else if (!DIFF_FILE_VALID(p-two)) {
- /* removed one -- did it have what we are looking for? */
- hit = !regexec(regexp, mf1.ptr, 1, regmatch, 0);
- } else {
- /*
- * We have both sides; need to run textual diff and see if
- * the pattern appears on added/deleted lines.
- */
- 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;
- xecfg.ctxlen = o-context;
- xecfg.interhunkctxlen = o-interhunkcontext;
- 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);
- diff_free_filespec_data(p-one);
- diff_free_filespec_data(p-two);
- return hit;
+
Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G
On Thu, Apr 04, 2013 at 10:43:14PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: I notice that you are stuck factoring out not just the setup, but also the cleanup, and I wondered if things could be made even simpler by just encapsulating the checking logic in a callback; then the setup and cleanup flow more naturally, as they are in a single function wrapper. Like this, which ends up saving 20 lines rather than adding 7: Oh, this is one of those many times I am reminded why I love having you in the reviewer/contributor pool ;-) I didn't actually test that patch beyond compilation (but it's _obviously_ correct, right?), and I'm about to go to bed. Do you want to take care of adapting your commit message to it? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
