From bc6158981b547db3d3eb754213467fed97109d57 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Thu, 4 Apr 2013 22:20:29 +0200 Subject: [PATCH 1/6] diffcore-pickaxe: remove unnecessary call to get_textconv() The fill_one() function is responsible for finding and filling the textconv filter as necessary, and is called by diff_grep() function that implements "git log -G". The has_changes() function that implements "git log -S" calls get_textconv() for two sides being compared, before it checks to see if it was asked to perform the pickaxe limiting. Move the code around to avoid this wastage. After has_changes() calls get_textconv() to obtain textconv for both sides, fill_one() is called to use them. By adding get_textconv() to diff_grep() and relieving fill_one() of responsibility to find the textconv filter, we can avoid calling get_textconv() twice in has_changes(). With this change it's also no longer necessary for fill_one() to modify the textconv argument, therefore pass a pointer instead of a pointer to a pointer. Signed-off-by: Simon Ruderich Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index b097fa7661..8f955f87ef 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -75,11 +75,10 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len) } static void fill_one(struct diff_filespec *one, - mmfile_t *mf, struct userdiff_driver **textconv) + mmfile_t *mf, struct userdiff_driver *textconv) { if (DIFF_FILE_VALID(one)) { - *textconv = get_textconv(one); - mf->size = fill_textconv(*textconv, one, &mf->ptr); + mf->size = fill_textconv(textconv, one, &mf->ptr); } else { memset(mf, 0, sizeof(*mf)); } @@ -97,8 +96,11 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, if (diff_unmodified_pair(p)) return 0; - fill_one(p->one, &mf1, &textconv_one); - fill_one(p->two, &mf2, &textconv_two); + textconv_one = get_textconv(p->one); + textconv_two = get_textconv(p->two); + + fill_one(p->one, &mf1, textconv_one); + fill_one(p->two, &mf2, textconv_two); if (!mf1.ptr) { if (!mf2.ptr) @@ -201,14 +203,17 @@ 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 = get_textconv(p->one); - struct userdiff_driver *textconv_two = get_textconv(p->two); + struct userdiff_driver *textconv_one = NULL; + struct userdiff_driver *textconv_two = NULL; mmfile_t mf1, mf2; int ret; if (!o->pickaxe[0]) return 0; + 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 @@ -219,8 +224,8 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o, if (textconv_one == textconv_two && diff_unmodified_pair(p)) return 0; - fill_one(p->one, &mf1, &textconv_one); - fill_one(p->two, &mf2, &textconv_two); + fill_one(p->one, &mf1, textconv_one); + fill_one(p->two, &mf2, textconv_two); if (!mf1.ptr) { if (!mf2.ptr) From 7cdb9b42c359000b1d3d604f847598afd015b7c7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 4 Apr 2013 20:08:47 -0400 Subject: [PATCH 2/6] diffcore-pickaxe: remove fill_one() fill_one is _almost_ identical to just calling fill_textconv; the exception is that for the !DIFF_FILE_VALID case, fill_textconv gives us an empty buffer rather than a NULL one. Since we currently use the NULL pointer as a signal that the file is not present on one side of the diff, we must now switch to using DIFF_FILE_VALID to make the same check. Signed-off-by: Jeff King Signed-off-by: Simon Ruderich Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 8f955f87ef..3124f49dc3 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -74,16 +74,6 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len) line[len] = hold; } -static void fill_one(struct diff_filespec *one, - mmfile_t *mf, struct userdiff_driver *textconv) -{ - if (DIFF_FILE_VALID(one)) { - mf->size = fill_textconv(textconv, one, &mf->ptr); - } else { - memset(mf, 0, sizeof(*mf)); - } -} - static int diff_grep(struct diff_filepair *p, struct diff_options *o, regex_t *regexp, kwset_t kws) { @@ -99,15 +89,15 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, textconv_one = get_textconv(p->one); textconv_two = get_textconv(p->two); - fill_one(p->one, &mf1, textconv_one); - fill_one(p->two, &mf2, textconv_two); + mf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr); + mf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr); - if (!mf1.ptr) { - if (!mf2.ptr) + if (!DIFF_FILE_VALID(p->one)) { + if (!DIFF_FILE_VALID(p->two)) return 0; /* ignore unmerged */ /* created "two" -- does it have what we are looking for? */ hit = !regexec(regexp, mf2.ptr, 1, ®match, 0); - } else if (!mf2.ptr) { + } else if (!DIFF_FILE_VALID(p->two)) { /* removed "one" -- did it have what we are looking for? */ hit = !regexec(regexp, mf1.ptr, 1, ®match, 0); } else { @@ -224,16 +214,16 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o, if (textconv_one == textconv_two && diff_unmodified_pair(p)) return 0; - fill_one(p->one, &mf1, textconv_one); - fill_one(p->two, &mf2, textconv_two); + mf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr); + mf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr); - if (!mf1.ptr) { - if (!mf2.ptr) + if (!DIFF_FILE_VALID(p->one)) { + if (!DIFF_FILE_VALID(p->two)) ret = 0; /* ignore unmerged */ /* created */ ret = contains(&mf2, o, regexp, kws) != 0; } - else if (!mf2.ptr) /* removed */ + else if (!DIFF_FILE_VALID(p->two)) /* removed */ ret = contains(&mf1, o, regexp, kws) != 0; else ret = contains(&mf1, o, regexp, kws) != From a8f6109428b868611c0a59e6894e2b6b38c34e1b Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Fri, 5 Apr 2013 15:16:30 +0200 Subject: [PATCH 3/6] diffcore-pickaxe: respect --no-textconv git log -S doesn't respect --no-textconv: $ echo '*.txt diff=wrong' > .gitattributes $ git -c diff.wrong.textconv='xxx' log --no-textconv -Sfoo error: cannot run xxx: No such file or directory fatal: unable to read files to diff Reported-by: Matthieu Moy Signed-off-by: Simon Ruderich Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 12 ++++++++---- t/t4209-log-pickaxe.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 3124f49dc3..26ddf00aa3 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -86,8 +86,10 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, if (diff_unmodified_pair(p)) return 0; - textconv_one = get_textconv(p->one); - textconv_two = get_textconv(p->two); + if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) { + textconv_one = get_textconv(p->one); + textconv_two = get_textconv(p->two); + } mf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr); mf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr); @@ -201,8 +203,10 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o, if (!o->pickaxe[0]) return 0; - textconv_one = get_textconv(p->one); - textconv_two = get_textconv(p->two); + 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 diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh index eed727341d..38fb80f643 100755 --- a/t/t4209-log-pickaxe.sh +++ b/t/t4209-log-pickaxe.sh @@ -80,6 +80,20 @@ test_expect_success 'log -G -i (match)' ' test_cmp expect actual ' +test_expect_success 'log -G --textconv (missing textconv tool)' ' + echo "* diff=test" >.gitattributes && + test_must_fail git -c diff.test.textconv=missing log -Gfoo && + rm .gitattributes +' + +test_expect_success 'log -G --no-textconv (missing textconv tool)' ' + echo "* diff=test" >.gitattributes && + git -c diff.test.textconv=missing log -Gfoo --no-textconv >actual && + >expect && + test_cmp expect actual && + rm .gitattributes +' + test_expect_success 'log -S (nomatch)' ' git log -Spicked --format=%H >actual && >expect && @@ -116,4 +130,18 @@ test_expect_success 'log -S -i (nomatch)' ' test_cmp expect actual ' +test_expect_success 'log -S --textconv (missing textconv tool)' ' + echo "* diff=test" >.gitattributes && + test_must_fail git -c diff.test.textconv=missing log -Sfoo && + rm .gitattributes +' + +test_expect_success 'log -S --no-textconv (missing textconv tool)' ' + echo "* diff=test" >.gitattributes && + git -c diff.test.textconv=missing log -Sfoo --no-textconv >actual && + >expect && + test_cmp expect actual && + rm .gitattributes +' + test_done From ebb722625805a0e98b5b8c062b79abd8eca1f639 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 4 Apr 2013 20:40:31 -0700 Subject: [PATCH 4/6] diffcore-pickaxe: port optimization from has_changes() to diff_grep() These two functions are called in the same codeflow to implement "log -S" and "log -G", respectively, but the latter lacked two obvious optimizations the former implemented, namely: - When a pickaxe limit is not given at all, they should return without wasting any cycle; - When both sides of the filepair are the same, and the same textconv conversion apply to them, return early, as there will be no interesting differences between the two anyway. Also release the filespec data once the processing is done (this is not about leaking memory--it is about releasing data we finished looking at as early as possible). Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 26ddf00aa3..bfaababbe5 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -83,7 +83,7 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, mmfile_t mf1, mf2; int hit; - if (diff_unmodified_pair(p)) + if (!o->pickaxe[0]) return 0; if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) { @@ -91,6 +91,9 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, 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); @@ -125,6 +128,8 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, free(mf1.ptr); if (textconv_two) free(mf2.ptr); + diff_free_filespec_data(p->one); + diff_free_filespec_data(p->two); return hit; } From 88ff684dd54f2a4793387f7162357005a4777ff2 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 4 Apr 2013 21:03:21 -0700 Subject: [PATCH 5/6] diffcore-pickaxe: fix leaks in "log -S" and "log -G" The diff_grep() and has_changes() functions had early return codepaths for unmerged filepairs, which simply returned 0. When we taught textconv filter to them, one was ignored and continued to return early without freeing the result filtered by textconv, and the other had a failed attempt to fix, which allowed the planned return value 0 to be overwritten by a bogus call to contains(). Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index bfaababbe5..cadb071a44 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -99,9 +99,10 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, if (!DIFF_FILE_VALID(p->one)) { if (!DIFF_FILE_VALID(p->two)) - return 0; /* ignore unmerged */ - /* created "two" -- does it have what we are looking for? */ - hit = !regexec(regexp, mf2.ptr, 1, ®match, 0); + hit = 0; /* ignore unmerged */ + else + /* created "two" -- does it have what we are looking for? */ + hit = !regexec(regexp, mf2.ptr, 1, ®match, 0); } else if (!DIFF_FILE_VALID(p->two)) { /* removed "one" -- did it have what we are looking for? */ hit = !regexec(regexp, mf1.ptr, 1, ®match, 0); @@ -229,8 +230,9 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o, if (!DIFF_FILE_VALID(p->one)) { if (!DIFF_FILE_VALID(p->two)) ret = 0; /* ignore unmerged */ - /* created */ - ret = contains(&mf2, o, regexp, kws) != 0; + else + /* created */ + ret = contains(&mf2, o, regexp, kws) != 0; } else if (!DIFF_FILE_VALID(p->two)) /* removed */ ret = contains(&mf1, o, regexp, kws) != 0; From 61690bf4a1ad499a673995b92cd8ab51104a431c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 5 Apr 2013 01:28:10 -0400 Subject: [PATCH 6/6] 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 Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 118 +++++++++++++++++++-------------------------- 1 file changed, 49 insertions(+), 69 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index cadb071a44..63722f86dc 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, ®match, 0); + if (!two) + return !regexec(regexp, one->ptr, 1, ®match, 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, ®match, 0); - } else if (!DIFF_FILE_VALID(p->two)) { - /* removed "one" -- did it have what we are looking for? */ - hit = !regexec(regexp, mf1.ptr, 1, ®match, 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; + /* + * We have both sides; need to run textual diff and see if + * the pattern appears on added/deleted lines. + */ + 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(one, two, diffgrep_consume, &ecbdata, + &xpp, &xecfg); + return ecbdata.hit; } static void diffcore_pickaxe_grep(struct diff_options *o) @@ -198,8 +172,19 @@ static unsigned int contains(mmfile_t *mf, struct diff_options *o, return cnt; } -static int has_changes(struct diff_filepair *p, struct diff_options *o, +static int has_changes(mmfile_t *one, mmfile_t *two, + struct diff_options *o, regex_t *regexp, kwset_t kws) +{ + if (!one) + return contains(two, o, regexp, kws) != 0; + if (!two) + return contains(one, o, regexp, kws) != 0; + return contains(one, o, regexp, kws) != contains(two, o, regexp, kws); +} + +static int pickaxe_match(struct diff_filepair *p, struct diff_options *o, + regex_t *regexp, kwset_t kws, pickaxe_fn fn) { struct userdiff_driver *textconv_one = NULL; struct userdiff_driver *textconv_two = NULL; @@ -209,6 +194,10 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o, if (!o->pickaxe[0]) return 0; + /* ignore unmerged */ + if (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two)) + return 0; + if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) { textconv_one = get_textconv(p->one); textconv_two = get_textconv(p->two); @@ -227,18 +216,9 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o, 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)) - ret = 0; /* ignore unmerged */ - else - /* created */ - ret = contains(&mf2, o, regexp, kws) != 0; - } - else if (!DIFF_FILE_VALID(p->two)) /* removed */ - ret = contains(&mf1, o, regexp, kws) != 0; - else - ret = contains(&mf1, o, regexp, kws) != - contains(&mf2, o, regexp, kws); + ret = fn(DIFF_FILE_VALID(p->one) ? &mf1 : NULL, + DIFF_FILE_VALID(p->two) ? &mf2 : NULL, + o, regexp, kws); if (textconv_one) free(mf1.ptr);