{"thread":{"id":"33379","subject":"[BUG] git log -S not respecting --no-textconv","startedAt":"2013-04-04T08:34:17Z","lastAt":"2013-04-05T17:31:40Z","messageCount":27,"participants":["Matthieu Moy","Simon Ruderich","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"213091","messageId":"vpqd2uay9rq.fsf@grenoble-inp.fr","threadId":"33379","inReplyTo":null,"subject":"[BUG] git log -S not respecting --no-textconv","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-04-04T08:34:17Z","receivedAt":"2013-04-04T08:34:17Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Hi,\n\nIt seems the command \"git log --no-textconv -Sfoo\" still runs the\ntextconv filter (noticed because I have a broken textconv filter that\nlets \"git log -S\" error out).\n\nSteps to reproduce:\n\nCreate a repo with *.txt file(s) in it\n$ echo '*.txt diff=wrong' > .gitattributes\n\nAn incorrect textconv filter errors out as expected:\n\n$ git -c diff.wrong.textconv='xxx' log -Sfoo \nerror: cannot run xxx: No such file or directory\nfatal: unable to read files to diff\n\nbut --no-textconv has no effect:\n\n$ git -c diff.wrong.textconv='xxx' log --no-textconv -Sfoo\nerror: cannot run xxx: No such file or directory\nfatal: unable to read files to diff\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"213123","messageId":"20130404160359.GA25232@ruderich.org","threadId":"33379","inReplyTo":"vpqd2uay9rq.fsf@grenoble-inp.fr","subject":"[PATCH 1/2] diffcore-pickaxe: respect --no-textconv","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2013-04-04T16:03:59Z","receivedAt":"2013-04-04T16:03:59Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"git log -S doesn't respect --no-textconv:\n\n $ echo '*.txt diff=wrong' > .gitattributes\n $ git -c diff.wrong.textconv='xxx' log --no-textconv -Sfoo\n error: cannot run xxx: No such file or directory\n fatal: unable to read files to diff\n\nSigned-off-by: Simon Ruderich \nReported-by: Matthieu Moy \n---\nOn Thu, Apr 04, 2013 at 10:34:17AM +0200, Matthieu Moy wrote:\n> Hi,\n>\n> It seems the command \"git log --no-textconv -Sfoo\" still runs the\n> textconv filter (noticed because I have a broken textconv filter that\n> lets \"git log -S\" error out).\n>\n> [snip]\n\nHello Matthieu,\n\nThis patch should fix it. All tests pass.\n\nRegards\nSimon\n\n diffcore-pickaxe.c | 15 ++++++++++++---\n t/t4209-log-pickaxe.sh | 14 ++++++++++++++\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex b097fa7..f814a52 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -78,7 +78,6 @@ static void fill_one(struct diff_filespec *one,\n \t\t mmfile_t *mf, struct userdiff_driver **textconv)\n {\n \tif (DIFF_FILE_VALID(one)) {\n-\t\t*textconv = get_textconv(one);\n \t\tmf->size = fill_textconv(*textconv, one, &mf->ptr);\n \t} else {\n \t\tmemset(mf, 0, sizeof(*mf));\n@@ -97,6 +96,11 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \tif (diff_unmodified_pair(p))\n \t\treturn 0;\n \n+\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n+\t\ttextconv_one = get_textconv(p->one);\n+\t\ttextconv_two = get_textconv(p->two);\n+\t}\n+\n \tfill_one(p->one, &mf1, &textconv_one);\n \tfill_one(p->two, &mf2, &textconv_two);\n \n@@ -201,14 +205,19 @@ static unsigned int contains(mmfile_t *mf, struct diff_options *o,\n static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws)\n {\n-\tstruct userdiff_driver *textconv_one = get_textconv(p->one);\n-\tstruct userdiff_driver *textconv_two = get_textconv(p->two);\n+\tstruct userdiff_driver *textconv_one = NULL;\n+\tstruct userdiff_driver *textconv_two = NULL;\n \tmmfile_t mf1, mf2;\n \tint ret;\n \n \tif (!o->pickaxe[0])\n \t\treturn 0;\n \n+\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n+\t\ttextconv_one = get_textconv(p->one);\n+\t\ttextconv_two = get_textconv(p->two);\n+\t}\n+\n \t/*\n \t * If we have an unmodified pair, we know that the count will be the\n \t * same and don't even have to load the blobs. Unless textconv is in\ndiff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\nindex eed7273..953cec8 100755\n--- a/t/t4209-log-pickaxe.sh\n+++ b/t/t4209-log-pickaxe.sh\n@@ -116,4 +116,18 @@ test_expect_success 'log -S -i (nomatch)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log -S --textconv (missing textconv tool)' '\n+\techo \"* diff=test\" >.gitattributes &&\n+\ttest_must_fail git -c diff.test.textconv=missing log -Sfoo &&\n+\trm .gitattributes\n+'\n+\n+test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n+\techo \"* diff=test\" >.gitattributes &&\n+\tgit -c diff.test.textconv=missing log -Sfoo --no-textconv >actual &&\n+\t>expect &&\n+\ttest_cmp expect actual &&\n+\trm .gitattributes\n+'\n+\n test_done\n-- \n1.8.2\n\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"213132","messageId":"vpqip42qlc1.fsf@grenoble-inp.fr","threadId":"33379","inReplyTo":"20130404160359.GA25232@ruderich.org","subject":"Re: [PATCH 1/2] diffcore-pickaxe: respect --no-textconv","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-04-04T17:03:58Z","receivedAt":"2013-04-04T17:03:58Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Simon Ruderich writes:\n\n> This patch should fix it.\n\nIt does.\n\n> --- a/diffcore-pickaxe.c\n> +++ b/diffcore-pickaxe.c\n> @@ -78,7 +78,6 @@ static void fill_one(struct diff_filespec *one,\n> \t\t mmfile_t *mf, struct userdiff_driver **textconv)\n> {\n> \tif (DIFF_FILE_VALID(one)) {\n> -\t\t*textconv = get_textconv(one);\n> \t\tmf->size = fill_textconv(*textconv, one, &mf->ptr);\n> \t} else {\n> \t\tmemset(mf, 0, sizeof(*mf));\n\nI don't understand why this is needed (not rethorical, I just didn't\ninvestigate), but the rest of the code looks straightforward and\ncorrect.\n\nThanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"213136","messageId":"20130404174301.GA13005@sigill.intra.peff.net","threadId":"33379","inReplyTo":"20130404160359.GA25232@ruderich.org","subject":"Re: [PATCH 1/2] diffcore-pickaxe: respect --no-textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-04T17:43:01Z","receivedAt":"2013-04-04T17:43:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2013 at 06:03:59PM +0200, Simon Ruderich wrote:\n\n> git log -S doesn't respect --no-textconv:\n> \n> $ echo '*.txt diff=wrong' > .gitattributes\n> $ git -c diff.wrong.textconv='xxx' log --no-textconv -Sfoo\n> error: cannot run xxx: No such file or directory\n> fatal: unable to read files to diff\n> \n> Signed-off-by: Simon Ruderich \n> Reported-by: Matthieu Moy \n\nUsually these pseudo-headers are meant to be chronological, so you would\nswap the order.\n\n> > It seems the command \"git log --no-textconv -Sfoo\" still runs the\n> > textconv filter (noticed because I have a broken textconv filter that\n> > lets \"git log -S\" error out).\n> >\n> > [snip]\n> \n> Hello Matthieu,\n> \n> This patch should fix it. All tests pass.\n\nThanks, the patch looks correct to me, although...\n\n> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> index b097fa7..f814a52 100644\n> --- a/diffcore-pickaxe.c\n> +++ b/diffcore-pickaxe.c\n> @@ -78,7 +78,6 @@ static void fill_one(struct diff_filespec *one,\n> \t\t mmfile_t *mf, struct userdiff_driver **textconv)\n> {\n> \tif (DIFF_FILE_VALID(one)) {\n> -\t\t*textconv = get_textconv(one);\n> \t\tmf->size = fill_textconv(*textconv, one, &mf->ptr);\n\nDropping this is the right thing to do with the rest of your patch, but\nlike Matthieu, it took me a second to see why. Something like this\nshould probably go into the commit message:\n\n We need to check that the ALLOW_TEXTCONV diff option is set before\n loading the textconv drivers. Rather than pass the diff options\n structure into the fill_one helper, let's just determine the textconv\n driver outside of that function and pass that in.\n\nThough I really think that justification would make more sense if your\nsecond cleanup patch came first, pulling the get_textconv calls back out\nto the callers (which is easy to justify, since one of the callers\nalready has to load them itself _anyway_, which is just ugly).\n\n-Peff\n"},{"id":"213137","messageId":"7vvc82i40a.fsf@alter.siamese.dyndns.org","threadId":"33379","inReplyTo":"20130404160359.GA25232@ruderich.org","subject":"Re: [PATCH 1/2] diffcore-pickaxe: respect --no-textconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-04T17:45:25Z","receivedAt":"2013-04-04T17:45:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon Ruderich writes:\n\n> git log -S doesn't respect --no-textconv:\n>\n> $ echo '*.txt diff=wrong' > .gitattributes\n> $ git -c diff.wrong.textconv='xxx' log --no-textconv -Sfoo\n> error: cannot run xxx: No such file or directory\n> fatal: unable to read files to diff\n>\n> Reported-by: Matthieu Moy \n> Signed-off-by: Simon Ruderich \n> ---\n\nSounds sensible.\n\nWith this change fill_one() no longer needs to update textconv, it\ncan just take a pointer to one, not a pointer to a pointer to one,\nwhich is [2/2].\n\nPeff, anything I missed?\n\n> On Thu, Apr 04, 2013 at 10:34:17AM +0200, Matthieu Moy wrote:\n>> Hi,\n>>\n>> It seems the command \"git log --no-textconv -Sfoo\" still runs the\n>> textconv filter (noticed because I have a broken textconv filter that\n>> lets \"git log -S\" error out).\n>>\n>> [snip]\n>\n> Hello Matthieu,\n>\n> This patch should fix it. All tests pass.\n>\n> Regards\n> Simon\n>\n> diffcore-pickaxe.c | 15 ++++++++++++---\n> t/t4209-log-pickaxe.sh | 14 ++++++++++++++\n> 2 files changed, 26 insertions(+), 3 deletions(-)\n>\n> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> index b097fa7..f814a52 100644\n> --- a/diffcore-pickaxe.c\n> +++ b/diffcore-pickaxe.c\n> @@ -78,7 +78,6 @@ static void fill_one(struct diff_filespec *one,\n> \t\t mmfile_t *mf, struct userdiff_driver **textconv)\n> {\n> \tif (DIFF_FILE_VALID(one)) {\n> -\t\t*textconv = get_textconv(one);\n> \t\tmf->size = fill_textconv(*textconv, one, &mf->ptr);\n> \t} else {\n> \t\tmemset(mf, 0, sizeof(*mf));\n> @@ -97,6 +96,11 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n> \tif (diff_unmodified_pair(p))\n> \t\treturn 0;\n> \n> +\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n> +\t\ttextconv_one = get_textconv(p->one);\n> +\t\ttextconv_two = get_textconv(p->two);\n> +\t}\n> +\n> \tfill_one(p->one, &mf1, &textconv_one);\n> \tfill_one(p->two, &mf2, &textconv_two);\n> \n> @@ -201,14 +205,19 @@ static unsigned int contains(mmfile_t *mf, struct diff_options *o,\n> static int has_changes(struct diff_filepair *p, struct diff_options *o,\n> \t\t regex_t *regexp, kwset_t kws)\n> {\n> -\tstruct userdiff_driver *textconv_one = get_textconv(p->one);\n> -\tstruct userdiff_driver *textconv_two = get_textconv(p->two);\n> +\tstruct userdiff_driver *textconv_one = NULL;\n> +\tstruct userdiff_driver *textconv_two = NULL;\n> \tmmfile_t mf1, mf2;\n> \tint ret;\n> \n> \tif (!o->pickaxe[0])\n> \t\treturn 0;\n> \n> +\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n> +\t\ttextconv_one = get_textconv(p->one);\n> +\t\ttextconv_two = get_textconv(p->two);\n> +\t}\n> +\n> \t/*\n> \t * If we have an unmodified pair, we know that the count will be the\n> \t * same and don't even have to load the blobs. Unless textconv is in\n> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> index eed7273..953cec8 100755\n> --- a/t/t4209-log-pickaxe.sh\n> +++ b/t/t4209-log-pickaxe.sh\n> @@ -116,4 +116,18 @@ test_expect_success 'log -S -i (nomatch)' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'log -S --textconv (missing textconv tool)' '\n> +\techo \"* diff=test\" >.gitattributes &&\n> +\ttest_must_fail git -c diff.test.textconv=missing log -Sfoo &&\n> +\trm .gitattributes\n> +'\n> +\n> +test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n> +\techo \"* diff=test\" >.gitattributes &&\n> +\tgit -c diff.test.textconv=missing log -Sfoo --no-textconv >actual &&\n> +\t>expect &&\n> +\ttest_cmp expect actual &&\n> +\trm .gitattributes\n> +'\n> +\n> test_done\n> -- \n> 1.8.2\n"},{"id":"213139","messageId":"20130404175150.GA15630@sigill.intra.peff.net","threadId":"33379","inReplyTo":"7vvc82i40a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] diffcore-pickaxe: respect --no-textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-04T17:51:50Z","receivedAt":"2013-04-04T17:51:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2013 at 10:45:25AM -0700, Junio C Hamano wrote:\n\n> Simon Ruderich writes:\n> \n> > git log -S doesn't respect --no-textconv:\n> >\n> > $ echo '*.txt diff=wrong' > .gitattributes\n> > $ git -c diff.wrong.textconv='xxx' log --no-textconv -Sfoo\n> > error: cannot run xxx: No such file or directory\n> > fatal: unable to read files to diff\n> >\n> > Reported-by: Matthieu Moy \n> > Signed-off-by: Simon Ruderich \n> > ---\n> \n> Sounds sensible.\n> \n> With this change fill_one() no longer needs to update textconv, it\n> can just take a pointer to one, not a pointer to a pointer to one,\n> which is [2/2].\n> \n> Peff, anything I missed?\n\nI'm OK with this as-is, but I would also be happy to see the re-ordering\nand extra cleanup I mentioned elsewhere.\n\nBut either way:\n\n Acked-by: Jeff King \n\n-Peff\n"},{"id":"213140","messageId":"7vr4iqi2uw.fsf@alter.siamese.dyndns.org","threadId":"33379","inReplyTo":"20130404175150.GA15630@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] diffcore-pickaxe: respect --no-textconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-04T18:10:15Z","receivedAt":"2013-04-04T18:10:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm OK with this as-is, but I would also be happy to see the re-ordering\n> and extra cleanup I mentioned elsewhere.\n\nYeah, I agree that the order is the other way around. 2/2 could be\nretitled to say that fill_one() no longer needs to touch, but swapping\nthe order with the extra clean-up would be much cleaner.\n\n>\n> But either way:\n>\n> Acked-by: Jeff King \n>\n> -Peff\n"},{"id":"213184","messageId":"ed31727421dc3000e943e62a8d82ac1af6589733.1365105971.git.simon@ruderich.org","threadId":"33379","inReplyTo":"7vr4iqi2uw.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv()","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2013-04-04T20:20:29Z","receivedAt":"2013-04-04T20:20:29Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"get_textconv() is called in diff_grep() to determine the textconv driver\nbefore calling fill_one() and then again in fill_one(). Remove this\nunnecessary call by determining the textconv driver before calling\nfill_one().\n\nWith this change it's also no longer necessary for fill_one() to\nmodify the textconv argument, therefore pass a pointer instead of\na pointer to a pointer.\n\nSigned-off-by: Simon Ruderich \n---\n\nHello,\n\nI've reordered the patches as requested and included Jeff's\ncleanup patch.\n\nRegards\nSimon\n\n diffcore-pickaxe.c | 23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex b097fa7..8f955f8 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -75,11 +75,10 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n }\n \n static void fill_one(struct diff_filespec *one,\n-\t\t mmfile_t *mf, struct userdiff_driver **textconv)\n+\t\t mmfile_t *mf, struct userdiff_driver *textconv)\n {\n \tif (DIFF_FILE_VALID(one)) {\n-\t\t*textconv = get_textconv(one);\n-\t\tmf->size = fill_textconv(*textconv, one, &mf->ptr);\n+\t\tmf->size = fill_textconv(textconv, one, &mf->ptr);\n \t} else {\n \t\tmemset(mf, 0, sizeof(*mf));\n \t}\n@@ -97,8 +96,11 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \tif (diff_unmodified_pair(p))\n \t\treturn 0;\n \n-\tfill_one(p->one, &mf1, &textconv_one);\n-\tfill_one(p->two, &mf2, &textconv_two);\n+\ttextconv_one = get_textconv(p->one);\n+\ttextconv_two = get_textconv(p->two);\n+\n+\tfill_one(p->one, &mf1, textconv_one);\n+\tfill_one(p->two, &mf2, textconv_two);\n \n \tif (!mf1.ptr) {\n \t\tif (!mf2.ptr)\n@@ -201,14 +203,17 @@ static unsigned int contains(mmfile_t *mf, struct diff_options *o,\n static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws)\n {\n-\tstruct userdiff_driver *textconv_one = get_textconv(p->one);\n-\tstruct userdiff_driver *textconv_two = get_textconv(p->two);\n+\tstruct userdiff_driver *textconv_one = NULL;\n+\tstruct userdiff_driver *textconv_two = NULL;\n \tmmfile_t mf1, mf2;\n \tint ret;\n \n \tif (!o->pickaxe[0])\n \t\treturn 0;\n \n+\ttextconv_one = get_textconv(p->one);\n+\ttextconv_two = get_textconv(p->two);\n+\n \t/*\n \t * If we have an unmodified pair, we know that the count will be the\n \t * same and don't even have to load the blobs. Unless textconv is in\n@@ -219,8 +224,8 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n \t\treturn 0;\n \n-\tfill_one(p->one, &mf1, &textconv_one);\n-\tfill_one(p->two, &mf2, &textconv_two);\n+\tfill_one(p->one, &mf1, textconv_one);\n+\tfill_one(p->two, &mf2, textconv_two);\n \n \tif (!mf1.ptr) {\n \t\tif (!mf2.ptr)\n-- \n1.8.2\n\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"213185","messageId":"004969e2ef9bb8017ce66e36b60a447ab35068d0.1365105971.git.simon@ruderich.org","threadId":"33379","inReplyTo":"7vr4iqi2uw.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2 2/3] diffcore-pickaxe: remove fill_one()","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2013-04-04T20:21:08Z","receivedAt":"2013-04-04T20:21:08Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"From: Jeff King \n\nfill_one is _almost_ identical to just calling fill_textconv; the\nexception is that for the !DIFF_FILE_VALID case, fill_textconv gives us\nan empty buffer rather than a NULL one.\n\nSigned-off-by: Simon Ruderich \n---\nOn Thu, Apr 04, 2013 at 01:49:42PM -0400, Jeff King wrote:\n> [snip]\n>\n> What do you think of something like this on top (this is on top of your\n> patches, but again, I would suggest re-ordering your two, so it would\n> come as patch 2/3):\n\nHello Jeff,\n\nThat's a good idea. I've added your patch, thanks. Signed-off?\n\nRegards\nSimon\n\n diffcore-pickaxe.c | 30 ++++++++++--------------------\n 1 file changed, 10 insertions(+), 20 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 8f955f8..3124f49 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -74,16 +74,6 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n \tline[len] = hold;\n }\n \n-static void fill_one(struct diff_filespec *one,\n-\t\t mmfile_t *mf, struct userdiff_driver *textconv)\n-{\n-\tif (DIFF_FILE_VALID(one)) {\n-\t\tmf->size = fill_textconv(textconv, one, &mf->ptr);\n-\t} else {\n-\t\tmemset(mf, 0, sizeof(*mf));\n-\t}\n-}\n-\n static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws)\n {\n@@ -99,15 +89,15 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \ttextconv_one = get_textconv(p->one);\n \ttextconv_two = get_textconv(p->two);\n \n-\tfill_one(p->one, &mf1, textconv_one);\n-\tfill_one(p->two, &mf2, textconv_two);\n+\tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n+\tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n \n-\tif (!mf1.ptr) {\n-\t\tif (!mf2.ptr)\n+\tif (!DIFF_FILE_VALID(p->one)) {\n+\t\tif (!DIFF_FILE_VALID(p->two))\n \t\t\treturn 0; /* ignore unmerged */\n \t\t/* created \"two\" -- does it have what we are looking for? */\n \t\thit = !regexec(regexp, mf2.ptr, 1, ®match, 0);\n-\t} else if (!mf2.ptr) {\n+\t} else if (!DIFF_FILE_VALID(p->two)) {\n \t\t/* removed \"one\" -- did it have what we are looking for? */\n \t\thit = !regexec(regexp, mf1.ptr, 1, ®match, 0);\n \t} else {\n@@ -224,16 +214,16 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n \t\treturn 0;\n \n-\tfill_one(p->one, &mf1, textconv_one);\n-\tfill_one(p->two, &mf2, textconv_two);\n+\tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n+\tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n \n-\tif (!mf1.ptr) {\n-\t\tif (!mf2.ptr)\n+\tif (!DIFF_FILE_VALID(p->one)) {\n+\t\tif (!DIFF_FILE_VALID(p->two))\n \t\t\tret = 0; /* ignore unmerged */\n \t\t/* created */\n \t\tret = contains(&mf2, o, regexp, kws) != 0;\n \t}\n-\telse if (!mf2.ptr) /* removed */\n+\telse if (!DIFF_FILE_VALID(p->two)) /* removed */\n \t\tret = contains(&mf1, o, regexp, kws) != 0;\n \telse\n \t\tret = contains(&mf1, o, regexp, kws) !=\n-- \n1.8.2\n\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"213186","messageId":"7d36738417942b594c185953115a244ad6f3c7a0.1365105971.git.simon@ruderich.org","threadId":"33379","inReplyTo":"7vr4iqi2uw.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2 3/3] diffcore-pickaxe: respect --no-textconv","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2013-04-04T20:21:55Z","receivedAt":"2013-04-04T20:21:55Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"git log -S doesn't respect --no-textconv:\n\n $ echo '*.txt diff=wrong' > .gitattributes\n $ git -c diff.wrong.textconv='xxx' log --no-textconv -Sfoo\n error: cannot run xxx: No such file or directory\n fatal: unable to read files to diff\n\nReported-by: Matthieu Moy \nSigned-off-by: Simon Ruderich \n---\n diffcore-pickaxe.c | 12 ++++++++----\n t/t4209-log-pickaxe.sh | 14 ++++++++++++++\n 2 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 3124f49..26ddf00 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -86,8 +86,10 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \tif (diff_unmodified_pair(p))\n \t\treturn 0;\n \n-\ttextconv_one = get_textconv(p->one);\n-\ttextconv_two = get_textconv(p->two);\n+\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n+\t\ttextconv_one = get_textconv(p->one);\n+\t\ttextconv_two = get_textconv(p->two);\n+\t}\n \n \tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n \tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n@@ -201,8 +203,10 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \tif (!o->pickaxe[0])\n \t\treturn 0;\n \n-\ttextconv_one = get_textconv(p->one);\n-\ttextconv_two = get_textconv(p->two);\n+\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n+\t\ttextconv_one = get_textconv(p->one);\n+\t\ttextconv_two = get_textconv(p->two);\n+\t}\n \n \t/*\n \t * If we have an unmodified pair, we know that the count will be the\ndiff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\nindex eed7273..953cec8 100755\n--- a/t/t4209-log-pickaxe.sh\n+++ b/t/t4209-log-pickaxe.sh\n@@ -116,4 +116,18 @@ test_expect_success 'log -S -i (nomatch)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log -S --textconv (missing textconv tool)' '\n+\techo \"* diff=test\" >.gitattributes &&\n+\ttest_must_fail git -c diff.test.textconv=missing log -Sfoo &&\n+\trm .gitattributes\n+'\n+\n+test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n+\techo \"* diff=test\" >.gitattributes &&\n+\tgit -c diff.test.textconv=missing log -Sfoo --no-textconv >actual &&\n+\t>expect &&\n+\ttest_cmp expect actual &&\n+\trm .gitattributes\n+'\n+\n test_done\n-- \n1.8.2\n\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"213195","messageId":"7vvc82ggy3.fsf@alter.siamese.dyndns.org","threadId":"33379","inReplyTo":"ed31727421dc3000e943e62a8d82ac1af6589733.1365105971.git.simon@ruderich.org","subject":"Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-04T20:48:52Z","receivedAt":"2013-04-04T20:48:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon Ruderich writes:\n\n> get_textconv() is called in diff_grep() to determine the textconv driver\n> before calling fill_one() and then again in fill_one(). Remove this\n> unnecessary call by determining the textconv driver before calling\n> fill_one().\n\nIf I am reading the code correctly, it is has_changes(), which is\nused for \"log -S\" (not \"log -G\" that uses diff_grep()), that does\nthe unnecessary get_textconv() unconditionally. The way diff_grep() \ndivides the work to make fill_one() responsible for filling the\ntextconv as necessary is internally consistent, and there is no\nunnecessary call.\n\nPerhaps...\n\n\tThe fill_one() function is responsible for finding and\n\tfilling the textconv filter as necessary, and is called by\n\tdiff_grep() function that implements \"git log -G\".\n\n\tThe has_changes() function calls get_textconv() for two\n\tsides being compared, before it checks to see if it was\n\tasked to perform the pickaxe limiting with the -S option.\n\tMove the code around to avoid this wastage. After that,\n\tfill_one() is called to use the textconv.\n\n\tBy adding get_textconv() to diff_grep() and relieving\n\tfill_one() of responsibility to find the textconv filter, we\n\tcan avoid calling get_textconv() twice.\n\nExplained that way, it makes me wonder why we cannot fix it the\nother way around, that is, not fetching textconv in has_changes()\nand instead letting fill_one() to find textconv as needed.\n\nThe answer is because has_changes() itself looks at the textconv.\n\nBut we have to wonder why it is so. diff_grep() short-circuits when\nthe two sides are identical and has_changes() has a similar but\ndifferent logic to check if the identical two sides are processed\nwith the same textconv filter before saying this filepair is\nuninteresting.\n\nShouldn't that logic be unified as well?\n"},{"id":"213200","messageId":"7vmwtegfy9.fsf@alter.siamese.dyndns.org","threadId":"33379","inReplyTo":"7vvc82ggy3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-04T21:10:22Z","receivedAt":"2013-04-04T21:10:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Perhaps...\n>\n> \tThe fill_one() function is responsible for finding and\n> \tfilling the textconv filter as necessary, and is called by\n> \tdiff_grep() function that implements \"git log -G\".\n>\n> \tThe has_changes() function calls get_textconv() for two\n> \tsides being compared, before it checks to see if it was\n> \tasked to perform the pickaxe limiting with the -S option.\n> \tMove the code around to avoid this wastage. After that,\n> \tfill_one() is called to use the textconv.\n>\n> \tBy adding get_textconv() to diff_grep() and relieving\n> \tfill_one() of responsibility to find the textconv filter, we\n> \tcan avoid calling get_textconv() twice.\n>\n> Explained that way, it makes me wonder why we cannot fix it the\n> other way around, that is, not fetching textconv in has_changes()\n> and instead letting fill_one() to find textconv as needed.\n>\n> The answer is because has_changes() itself looks at the textconv.\n>\n> But we have to wonder why it is so. diff_grep() short-circuits when\n> the two sides are identical and has_changes() has a similar but\n> different logic to check if the identical two sides are processed\n> with the same textconv filter before saying this filepair is\n> uninteresting.\n>\n> Shouldn't that logic be unified as well?\n\nThat is, something along this line. We may want to unify these two\nfunctions even more, but I do not offhand see a good way to do so.\n\n-- >8 --\nSubject: [PATCH] log -S/-G: unify the short-cut logic a bit more\n\nThe fill_one() function is responsible for finding and filling the\ntextconv filter as necessary, and is called by diff_grep() function\nthat implements \"git log -G\".\n\nThe has_changes() function calls get_textconv() for two sides being\ncompared, before it checks to see if it was asked to perform the\npickaxe limiting with the -S option. Move the code around to avoid\nthis wastage.\n\nBy adding get_textconv() to diff_grep() and relieving fill_one() of\nresponsibility to find the textconv filter, we can avoid calling\nget_textconv() twice; fill_one() no longer has to be passed a\npointer to a pointer to textconv.\n\nThe reason has_changes() calls get_textconv() early is because it\nwants to compare two textconv filters on both sides. When comparing\nthe two sides that are otherwise identical, if the same textconv\napplies to both sides, we know pickaxe (either -S or -G) would not\nfind anything before applying the textconv and comparing the result.\n\nTeach the same short-circuit logic to diff_grep() as well. The code\nin these two functions become more similar as the result.\n---\n diffcore-pickaxe.c | 37 +++++++++++++++++++++++++++----------\n 1 file changed, 27 insertions(+), 10 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex b097fa7..6d285a7 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -75,11 +75,10 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n }\n \n static void fill_one(struct diff_filespec *one,\n-\t\t mmfile_t *mf, struct userdiff_driver **textconv)\n+\t\t mmfile_t *mf, struct userdiff_driver *textconv)\n {\n \tif (DIFF_FILE_VALID(one)) {\n-\t\t*textconv = get_textconv(one);\n-\t\tmf->size = fill_textconv(*textconv, one, &mf->ptr);\n+\t\tmf->size = fill_textconv(textconv, one, &mf->ptr);\n \t} else {\n \t\tmemset(mf, 0, sizeof(*mf));\n \t}\n@@ -94,11 +93,24 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \tmmfile_t mf1, mf2;\n \tint hit;\n \n-\tif (diff_unmodified_pair(p))\n+\tif (!o->pickaxe[0])\n \t\treturn 0;\n \n-\tfill_one(p->one, &mf1, &textconv_one);\n-\tfill_one(p->two, &mf2, &textconv_two);\n+\ttextconv_one = get_textconv(p->one);\n+\ttextconv_two = get_textconv(p->two);\n+\n+\t/*\n+\t * If we have an unmodified pair, we know that the count will be the\n+\t * same and don't even have to load the blobs. Unless textconv is in\n+\t * play, _and_ we are using two different textconv filters (e.g.,\n+\t * because a pair is an exact rename with different textconv attributes\n+\t * for each side, which might generate different content).\n+\t */\n+\tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n+\t\treturn 0;\n+\n+\tfill_one(p->one, &mf1, textconv_one);\n+\tfill_one(p->two, &mf2, textconv_two);\n \n \tif (!mf1.ptr) {\n \t\tif (!mf2.ptr)\n@@ -131,6 +143,8 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \t\tfree(mf1.ptr);\n \tif (textconv_two)\n \t\tfree(mf2.ptr);\n+\tdiff_free_filespec_data(p->one);\n+\tdiff_free_filespec_data(p->two);\n \treturn hit;\n }\n \n@@ -201,14 +215,17 @@ static unsigned int contains(mmfile_t *mf, struct diff_options *o,\n static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws)\n {\n-\tstruct userdiff_driver *textconv_one = get_textconv(p->one);\n-\tstruct userdiff_driver *textconv_two = get_textconv(p->two);\n+\tstruct userdiff_driver *textconv_one = NULL;\n+\tstruct userdiff_driver *textconv_two = NULL;\n \tmmfile_t mf1, mf2;\n \tint ret;\n \n \tif (!o->pickaxe[0])\n \t\treturn 0;\n \n+\ttextconv_one = get_textconv(p->one);\n+\ttextconv_two = get_textconv(p->two);\n+\n \t/*\n \t * If we have an unmodified pair, we know that the count will be the\n \t * same and don't even have to load the blobs. Unless textconv is in\n@@ -219,8 +236,8 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n \t\treturn 0;\n \n-\tfill_one(p->one, &mf1, &textconv_one);\n-\tfill_one(p->two, &mf2, &textconv_two);\n+\tfill_one(p->one, &mf1, textconv_one);\n+\tfill_one(p->two, &mf2, textconv_two);\n \n \tif (!mf1.ptr) {\n \t\tif (!mf2.ptr)\n-- \n1.8.2-634-g203ba97\n"},{"id":"213201","messageId":"20130404211110.GB25811@sigill.intra.peff.net","threadId":"33379","inReplyTo":"7vvc82ggy3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-04T21:11:10Z","receivedAt":"2013-04-04T21:11:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2013 at 01:48:52PM -0700, Junio C Hamano wrote:\n\n> If I am reading the code correctly, it is has_changes(), which is\n> used for \"log -S\" (not \"log -G\" that uses diff_grep()), that does\n> the unnecessary get_textconv() unconditionally. The way diff_grep() \n> divides the work to make fill_one() responsible for filling the\n> textconv as necessary is internally consistent, and there is no\n> unnecessary call.\n> \n> Perhaps...\n> \n> \tThe fill_one() function is responsible for finding and\n> \tfilling the textconv filter as necessary, and is called by\n> \tdiff_grep() function that implements \"git log -G\".\n> \n> \tThe has_changes() function calls get_textconv() for two\n> \tsides being compared, before it checks to see if it was\n> \tasked to perform the pickaxe limiting with the -S option.\n> \tMove the code around to avoid this wastage. After that,\n> \tfill_one() is called to use the textconv.\n> \n> \tBy adding get_textconv() to diff_grep() and relieving\n> \tfill_one() of responsibility to find the textconv filter, we\n> \tcan avoid calling get_textconv() twice.\n> \n> Explained that way, it makes me wonder why we cannot fix it the\n> other way around, that is, not fetching textconv in has_changes()\n> and instead letting fill_one() to find textconv as needed.\n> \n> The answer is because has_changes() itself looks at the textconv.\n> \n> But we have to wonder why it is so. diff_grep() short-circuits when\n> the two sides are identical and has_changes() has a similar but\n> different logic to check if the identical two sides are processed\n> with the same textconv filter before saying this filepair is\n> uninteresting.\n> \n> Shouldn't that logic be unified as well?\n\nI think it would be OK to perform the same optimization in diff_grep; I\ndo not recall offhand why I didn't do so when touching this code last\ntime, so it may have just been because I didn't think of it.\n\nBut I do not think fill_one is the right interface for it. The reason\nhas_changes calls get_textconv separately is that we do not want to fill\nthe buffer (which may be expensive) if we can avoid it. So the correct\nsequence is:\n\n 1. Find out textconv_one.\n\n 2. Find out textconv_two.\n\n 3. Check \"!hashcmp(one->sha1, two->sha1) && textconv_one == textconv_two\";\n if true, then we know the content we are about to compare will be\n identical, and we can return early.\n\n 4. Otherwise, retrieve the content for one (respecting textconv_one).\n\n 5. Retrieve the content for two (respecting textconv_two).\n\nYou cannot implement the optimization looking only at one side\n(obviously), but you also need to split the textconv lookup from the\n\"fill\" procedure if you want the optimization to come at the right\nplace.\n\nIf you turned fill_one into \"fill_both_sides\" then you could share\nthe code between diff_grep/pickaxe and do it in the right order in the\nhelper.\n\n-Peff\n"},{"id":"213216","messageId":"7vppy9gb94.fsf@alter.siamese.dyndns.org","threadId":"33379","inReplyTo":"20130404211110.GB25811@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-04T22:51:51Z","receivedAt":"2013-04-04T22:51:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But I do not think fill_one is the right interface for it. The reason\n> has_changes calls get_textconv separately is that we do not want to fill\n> the buffer (which may be expensive) if we can avoid it. So the correct\n> sequence is: ...\n> If you turned fill_one into \"fill_both_sides\" then you could share\n> the code between diff_grep/pickaxe and do it in the right order in the\n> helper.\n\nYeah, I think that is a good way to refactor. A patch later in the\nseries will be killing fill_one() anyway ;-)\n"},{"id":"213224","messageId":"20130405000847.GB27775@sigill.intra.peff.net","threadId":"33379","inReplyTo":"004969e2ef9bb8017ce66e36b60a447ab35068d0.1365105971.git.simon@ruderich.org","subject":"Re: [PATCH v2 2/3] diffcore-pickaxe: remove fill_one()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T00:08:47Z","receivedAt":"2013-04-05T00:08:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2013 at 10:21:08PM +0200, Simon Ruderich wrote:\n\n> From: Jeff King \n> \n> fill_one is _almost_ identical to just calling fill_textconv; the\n> exception is that for the !DIFF_FILE_VALID case, fill_textconv gives us\n> an empty buffer rather than a NULL one.\n> \n> Signed-off-by: Simon Ruderich \n> ---\n> On Thu, Apr 04, 2013 at 01:49:42PM -0400, Jeff King wrote:\n> > [snip]\n> >\n> > What do you think of something like this on top (this is on top of your\n> > patches, but again, I would suggest re-ordering your two, so it would\n> > come as patch 2/3):\n> \n> Hello Jeff,\n> \n> That's a good idea. I've added your patch, thanks. Signed-off?\n\nThanks. The whole series looks good to me. I think Junio's proposed\ncleanup is a good direction, too, but I don't mind if that comes on top.\n\nHere's an updated version of the patch with my signoff; I also expanded\non the commit message a little bit. The patch text is the same (I'm just\nincluding the whole patch for Junio's convenience in applying).\n\n-Peff\n\n-- >8 --\nSubject: [PATCH] diffcore-pickaxe: remove fill_one()\n\nfill_one is _almost_ identical to just calling fill_textconv; the\nexception is that for the !DIFF_FILE_VALID case, fill_textconv gives us\nan empty buffer rather than a NULL one. Since we currently use the NULL\npointer as a signal that the file is not present on one side of the\ndiff, we must now switch to using DIFF_FILE_VALID to make the same\ncheck.\n\nSigned-off-by: Jeff King \nSigned-off-by: Simon Ruderich \n---\n diffcore-pickaxe.c | 30 ++++++++++--------------------\n 1 file changed, 10 insertions(+), 20 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 8f955f8..3124f49 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -74,16 +74,6 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n \tline[len] = hold;\n }\n \n-static void fill_one(struct diff_filespec *one,\n-\t\t mmfile_t *mf, struct userdiff_driver *textconv)\n-{\n-\tif (DIFF_FILE_VALID(one)) {\n-\t\tmf->size = fill_textconv(textconv, one, &mf->ptr);\n-\t} else {\n-\t\tmemset(mf, 0, sizeof(*mf));\n-\t}\n-}\n-\n static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws)\n {\n@@ -99,15 +89,15 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \ttextconv_one = get_textconv(p->one);\n \ttextconv_two = get_textconv(p->two);\n \n-\tfill_one(p->one, &mf1, textconv_one);\n-\tfill_one(p->two, &mf2, textconv_two);\n+\tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n+\tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n \n-\tif (!mf1.ptr) {\n-\t\tif (!mf2.ptr)\n+\tif (!DIFF_FILE_VALID(p->one)) {\n+\t\tif (!DIFF_FILE_VALID(p->two))\n \t\t\treturn 0; /* ignore unmerged */\n \t\t/* created \"two\" -- does it have what we are looking for? */\n \t\thit = !regexec(regexp, mf2.ptr, 1, ®match, 0);\n-\t} else if (!mf2.ptr) {\n+\t} else if (!DIFF_FILE_VALID(p->two)) {\n \t\t/* removed \"one\" -- did it have what we are looking for? */\n \t\thit = !regexec(regexp, mf1.ptr, 1, ®match, 0);\n \t} else {\n@@ -224,16 +214,16 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n \t\treturn 0;\n \n-\tfill_one(p->one, &mf1, textconv_one);\n-\tfill_one(p->two, &mf2, textconv_two);\n+\tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n+\tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n \n-\tif (!mf1.ptr) {\n-\t\tif (!mf2.ptr)\n+\tif (!DIFF_FILE_VALID(p->one)) {\n+\t\tif (!DIFF_FILE_VALID(p->two))\n \t\t\tret = 0; /* ignore unmerged */\n \t\t/* created */\n \t\tret = contains(&mf2, o, regexp, kws) != 0;\n \t}\n-\telse if (!mf2.ptr) /* removed */\n+\telse if (!DIFF_FILE_VALID(p->two)) /* removed */\n \t\tret = contains(&mf1, o, regexp, kws) != 0;\n \telse\n \t\tret = contains(&mf1, o, regexp, kws) !=\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213232","messageId":"7v1uapfuyp.fsf@alter.siamese.dyndns.org","threadId":"33379","inReplyTo":"20130405000847.GB27775@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] diffcore-pickaxe: remove fill_one()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-05T04:43:42Z","receivedAt":"2013-04-05T04:43:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Thanks. The whole series looks good to me. I think Junio's proposed\n> cleanup is a good direction, too, but I don't mind if that comes on top.\n\nI'll send out a three-patch follow-up shortly.\n"},{"id":"213233","messageId":"1365137126-21659-1-git-send-email-gitster@pobox.com","threadId":"33379","inReplyTo":"7v1uapfuyp.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/3] diffcore-pickaxe: port optimization from has_changes() to diff_grep()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-05T04:45:24Z","receivedAt":"2013-04-05T04:45:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These two functions are called in the same codeflow to implement\n\"log -S\" and \"log -G\", respectively, but the latter\nlacked two obvious optimizations the former implemented, namely:\n\n - When a pickaxe limit is not given at all, they should return\n without wasting any cycle;\n\n - When both sides of the filepair are the same, and the same\n textconv conversion apply to them, return early, as there will be\n no interesting differences between the two anyway.\n\nAlso release the filespec data once the processing is done (this is\nnot about leaking memory--it is about releasing data we finished\nlooking at as early as possible).\n\nSigned-off-by: Junio C Hamano \n---\n diffcore-pickaxe.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 26ddf00..bfaabab 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -83,7 +83,7 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \tmmfile_t mf1, mf2;\n \tint hit;\n \n-\tif (diff_unmodified_pair(p))\n+\tif (!o->pickaxe[0])\n \t\treturn 0;\n \n \tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n@@ -91,6 +91,9 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \t\ttextconv_two = get_textconv(p->two);\n \t}\n \n+\tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n+\t\treturn 0;\n+\n \tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n \tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n \n@@ -125,6 +128,8 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \t\tfree(mf1.ptr);\n \tif (textconv_two)\n \t\tfree(mf2.ptr);\n+\tdiff_free_filespec_data(p->one);\n+\tdiff_free_filespec_data(p->two);\n \treturn hit;\n }\n \n-- \n1.8.2-588-gbf1c992\n"},{"id":"213235","messageId":"1365137126-21659-2-git-send-email-gitster@pobox.com","threadId":"33379","inReplyTo":"1365137126-21659-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/3] diffcore-pickaxe: fix leaks in \"log -S\" and \"log -G\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-05T04:45:25Z","receivedAt":"2013-04-05T04:45:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The diff_grep() and has_changes() functions had early return\ncodepaths for unmerged filepairs, which simply returned 0. When we\ntaught textconv filter to them, one was ignored and continued to\nreturn early without freeing the result filtered by textconv, and\nthe other had a failed attempt to fix, which allowed the planned\nreturn value 0 to be overwritten by a bogus call to contains().\n\nSigned-off-by: Junio C Hamano \n---\n diffcore-pickaxe.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex bfaabab..cadb071 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -99,9 +99,10 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \n \tif (!DIFF_FILE_VALID(p->one)) {\n \t\tif (!DIFF_FILE_VALID(p->two))\n-\t\t\treturn 0; /* ignore unmerged */\n-\t\t/* created \"two\" -- does it have what we are looking for? */\n-\t\thit = !regexec(regexp, mf2.ptr, 1, ®match, 0);\n+\t\t\thit = 0; /* ignore unmerged */\n+\t\telse\n+\t\t\t/* created \"two\" -- does it have what we are looking for? */\n+\t\t\thit = !regexec(regexp, mf2.ptr, 1, ®match, 0);\n \t} else if (!DIFF_FILE_VALID(p->two)) {\n \t\t/* removed \"one\" -- did it have what we are looking for? */\n \t\thit = !regexec(regexp, mf1.ptr, 1, ®match, 0);\n@@ -229,8 +230,9 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \tif (!DIFF_FILE_VALID(p->one)) {\n \t\tif (!DIFF_FILE_VALID(p->two))\n \t\t\tret = 0; /* ignore unmerged */\n-\t\t/* created */\n-\t\tret = contains(&mf2, o, regexp, kws) != 0;\n+\t\telse\n+\t\t\t/* created */\n+\t\t\tret = contains(&mf2, o, regexp, kws) != 0;\n \t}\n \telse if (!DIFF_FILE_VALID(p->two)) /* removed */\n \t\tret = contains(&mf1, o, regexp, kws) != 0;\n-- \n1.8.2-588-gbf1c992\n"},{"id":"213234","messageId":"1365137126-21659-3-git-send-email-gitster@pobox.com","threadId":"33379","inReplyTo":"1365137126-21659-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-05T04:45:26Z","receivedAt":"2013-04-05T04:45:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The logic to decide early to do nothing and prepare the data to be\ninspected are the same between has_changes() and diff_grep().\nIntroduce pickaxe_setup() helper to share the same code.\n\nSimilarly, introduce pickaxe_finish_filepair() to clean up after\nthese two functions are done with a filepair.\n\nSigned-off-by: Junio C Hamano \n---\n diffcore-pickaxe.c | 103 ++++++++++++++++++++++++++++-------------------------\n 1 file changed, 55 insertions(+), 48 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex cadb071..ac5a28d 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -48,6 +48,54 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,\n \t*q = outq;\n }\n \n+static int pickaxe_setup(struct diff_filepair *p,\n+\t\t\t struct diff_options *o,\n+\t\t\t mmfile_t *mf_one,\n+\t\t\t mmfile_t *mf_two,\n+\t\t\t unsigned *what_to_free)\n+{\n+\tstruct userdiff_driver *textconv_one = NULL;\n+\tstruct userdiff_driver *textconv_two = NULL;\n+\n+\tif (!o->pickaxe[0])\n+\t\treturn 0;\n+\n+\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n+\t\ttextconv_one = get_textconv(p->one);\n+\t\ttextconv_two = get_textconv(p->two);\n+\t}\n+\n+\t/*\n+\t * If we have an unmodified pair, we know that there is no\n+\t * interesting difference and we don't even have to load the\n+\t * blobs, unless textconv is in play, _and_ we are using two\n+\t * different textconv filters (e.g., because a pair is an\n+\t * exact rename with different textconv attributes for each\n+\t * side, which might generate different content).\n+\t */\n+\tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n+\t\treturn 0;\n+\n+\tmf_one->size = fill_textconv(textconv_one, p->one, &mf_one->ptr);\n+\tmf_two->size = fill_textconv(textconv_two, p->two, &mf_two->ptr);\n+\n+\t*what_to_free = (textconv_one ? 1 : 0) | (textconv_two ? 2 : 0);\n+\treturn 1;\n+}\n+\n+static void pickaxe_finish_filepair(struct diff_filepair *p,\n+\t\t\t\t mmfile_t *mf_one,\n+\t\t\t\t mmfile_t *mf_two,\n+\t\t\t\t unsigned what_to_free)\n+{\n+\tif (what_to_free & 1)\n+\t\tfree(mf_one->ptr);\n+\tif (what_to_free & 2)\n+\t\tfree(mf_two->ptr);\n+\tdiff_free_filespec_data(p->one);\n+\tdiff_free_filespec_data(p->two);\n+}\n+\n struct diffgrep_cb {\n \tregex_t *regexp;\n \tint hit;\n@@ -78,25 +126,13 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws)\n {\n \tregmatch_t regmatch;\n-\tstruct userdiff_driver *textconv_one = NULL;\n-\tstruct userdiff_driver *textconv_two = NULL;\n \tmmfile_t mf1, mf2;\n+\tunsigned what_to_free;\n \tint hit;\n \n-\tif (!o->pickaxe[0])\n+\tif (!pickaxe_setup(p, o, &mf1, &mf2, &what_to_free))\n \t\treturn 0;\n \n-\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n-\t\ttextconv_one = get_textconv(p->one);\n-\t\ttextconv_two = get_textconv(p->two);\n-\t}\n-\n-\tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n-\t\treturn 0;\n-\n-\tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n-\tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n-\n \tif (!DIFF_FILE_VALID(p->one)) {\n \t\tif (!DIFF_FILE_VALID(p->two))\n \t\t\thit = 0; /* ignore unmerged */\n@@ -125,12 +161,8 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \t\t\t &xpp, &xecfg);\n \t\thit = ecbdata.hit;\n \t}\n-\tif (textconv_one)\n-\t\tfree(mf1.ptr);\n-\tif (textconv_two)\n-\t\tfree(mf2.ptr);\n-\tdiff_free_filespec_data(p->one);\n-\tdiff_free_filespec_data(p->two);\n+\n+\tpickaxe_finish_filepair(p, &mf1, &mf2, what_to_free);\n \treturn hit;\n }\n \n@@ -201,32 +233,13 @@ static unsigned int contains(mmfile_t *mf, struct diff_options *o,\n static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws)\n {\n-\tstruct userdiff_driver *textconv_one = NULL;\n-\tstruct userdiff_driver *textconv_two = NULL;\n \tmmfile_t mf1, mf2;\n+\tunsigned what_to_free;\n \tint ret;\n \n-\tif (!o->pickaxe[0])\n-\t\treturn 0;\n-\n-\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n-\t\ttextconv_one = get_textconv(p->one);\n-\t\ttextconv_two = get_textconv(p->two);\n-\t}\n-\n-\t/*\n-\t * If we have an unmodified pair, we know that the count will be the\n-\t * same and don't even have to load the blobs. Unless textconv is in\n-\t * play, _and_ we are using two different textconv filters (e.g.,\n-\t * because a pair is an exact rename with different textconv attributes\n-\t * for each side, which might generate different content).\n-\t */\n-\tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n+\tif (!pickaxe_setup(p, o, &mf1, &mf2, &what_to_free))\n \t\treturn 0;\n \n-\tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n-\tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n-\n \tif (!DIFF_FILE_VALID(p->one)) {\n \t\tif (!DIFF_FILE_VALID(p->two))\n \t\t\tret = 0; /* ignore unmerged */\n@@ -240,13 +253,7 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \t\tret = contains(&mf1, o, regexp, kws) !=\n \t\t contains(&mf2, o, regexp, kws);\n \n-\tif (textconv_one)\n-\t\tfree(mf1.ptr);\n-\tif (textconv_two)\n-\t\tfree(mf2.ptr);\n-\tdiff_free_filespec_data(p->one);\n-\tdiff_free_filespec_data(p->two);\n-\n+\tpickaxe_finish_filepair(p, &mf1, &mf2, what_to_free);\n \treturn ret;\n }\n \n-- \n1.8.2-588-gbf1c992\n"},{"id":"213240","messageId":"20130405052810.GA29815@sigill.intra.peff.net","threadId":"33379","inReplyTo":"1365137126-21659-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T05:28:10Z","receivedAt":"2013-04-05T05:28:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2013 at 09:45:26PM -0700, Junio C Hamano wrote:\n\n> The logic to decide early to do nothing and prepare the data to be\n> inspected are the same between has_changes() and diff_grep().\n> Introduce pickaxe_setup() helper to share the same code.\n> \n> Similarly, introduce pickaxe_finish_filepair() to clean up after\n> these two functions are done with a filepair.\n\nAll three patches look fine to me.\n\nI notice that you are stuck factoring out not just the setup, but also\nthe cleanup, and I wondered if things could be made even simpler by just\nencapsulating the checking logic in a callback; then the setup and\ncleanup flow more naturally, as they are in a single function wrapper.\n\nLike this, which ends up saving 20 lines rather than adding 7:\n\n---\n diffcore-pickaxe.c | 118 +++++++++++++++--------------------\n 1 file changed, 49 insertions(+), 69 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex cadb071..63722f8 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -8,7 +8,12 @@\n #include \"xdiff-interface.h\"\n #include \"kwset.h\"\n \n-typedef int (*pickaxe_fn)(struct diff_filepair *p, struct diff_options *o, regex_t *regexp, kwset_t kws);\n+typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,\n+\t\t\t struct diff_options *o,\n+\t\t\t regex_t *regexp, kwset_t kws);\n+\n+static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n+\t\t\t regex_t *regexp, kwset_t kws, pickaxe_fn fn);\n \n static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws, pickaxe_fn fn)\n@@ -22,7 +27,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,\n \t\t/* Showing the whole changeset if needle exists */\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n-\t\t\tif (fn(p, o, regexp, kws))\n+\t\t\tif (pickaxe_match(p, o, regexp, kws, fn))\n \t\t\t\treturn; /* do not munge the queue */\n \t\t}\n \n@@ -37,7 +42,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,\n \t\t/* Showing only the filepairs that has the needle */\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n-\t\t\tif (fn(p, o, regexp, kws))\n+\t\t\tif (pickaxe_match(p, o, regexp, kws, fn))\n \t\t\t\tdiff_q(&outq, p);\n \t\t\telse\n \t\t\t\tdiff_free_filepair(p);\n@@ -74,64 +79,33 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \tline[len] = hold;\n }\n \n-static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n+static int diff_grep(mmfile_t *one, mmfile_t *two,\n+\t\t struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws)\n {\n \tregmatch_t regmatch;\n-\tstruct userdiff_driver *textconv_one = NULL;\n-\tstruct userdiff_driver *textconv_two = NULL;\n-\tmmfile_t mf1, mf2;\n-\tint hit;\n+\tstruct diffgrep_cb ecbdata;\n+\txpparam_t xpp;\n+\txdemitconf_t xecfg;\n \n-\tif (!o->pickaxe[0])\n-\t\treturn 0;\n+\tif (!one)\n+\t\treturn !regexec(regexp, two->ptr, 1, ®match, 0);\n+\tif (!two)\n+\t\treturn !regexec(regexp, one->ptr, 1, ®match, 0);\n \n-\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n-\t\ttextconv_one = get_textconv(p->one);\n-\t\ttextconv_two = get_textconv(p->two);\n-\t}\n-\n-\tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n-\t\treturn 0;\n-\n-\tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n-\tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n-\n-\tif (!DIFF_FILE_VALID(p->one)) {\n-\t\tif (!DIFF_FILE_VALID(p->two))\n-\t\t\thit = 0; /* ignore unmerged */\n-\t\telse\n-\t\t\t/* created \"two\" -- does it have what we are looking for? */\n-\t\t\thit = !regexec(regexp, mf2.ptr, 1, ®match, 0);\n-\t} else if (!DIFF_FILE_VALID(p->two)) {\n-\t\t/* removed \"one\" -- did it have what we are looking for? */\n-\t\thit = !regexec(regexp, mf1.ptr, 1, ®match, 0);\n-\t} else {\n-\t\t/*\n-\t\t * We have both sides; need to run textual diff and see if\n-\t\t * the pattern appears on added/deleted lines.\n-\t\t */\n-\t\tstruct diffgrep_cb ecbdata;\n-\t\txpparam_t xpp;\n-\t\txdemitconf_t xecfg;\n-\n-\t\tmemset(&xpp, 0, sizeof(xpp));\n-\t\tmemset(&xecfg, 0, sizeof(xecfg));\n-\t\tecbdata.regexp = regexp;\n-\t\tecbdata.hit = 0;\n-\t\txecfg.ctxlen = o->context;\n-\t\txecfg.interhunkctxlen = o->interhunkcontext;\n-\t\txdi_diff_outf(&mf1, &mf2, diffgrep_consume, &ecbdata,\n-\t\t\t &xpp, &xecfg);\n-\t\thit = ecbdata.hit;\n-\t}\n-\tif (textconv_one)\n-\t\tfree(mf1.ptr);\n-\tif (textconv_two)\n-\t\tfree(mf2.ptr);\n-\tdiff_free_filespec_data(p->one);\n-\tdiff_free_filespec_data(p->two);\n-\treturn hit;\n+\t/*\n+\t * We have both sides; need to run textual diff and see if\n+\t * the pattern appears on added/deleted lines.\n+\t */\n+\tmemset(&xpp, 0, sizeof(xpp));\n+\tmemset(&xecfg, 0, sizeof(xecfg));\n+\tecbdata.regexp = regexp;\n+\tecbdata.hit = 0;\n+\txecfg.ctxlen = o->context;\n+\txecfg.interhunkctxlen = o->interhunkcontext;\n+\txdi_diff_outf(one, two, diffgrep_consume, &ecbdata,\n+\t\t &xpp, &xecfg);\n+\treturn ecbdata.hit;\n }\n \n static void diffcore_pickaxe_grep(struct diff_options *o)\n@@ -198,9 +172,20 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \treturn cnt;\n }\n \n-static int has_changes(struct diff_filepair *p, struct diff_options *o,\n+static int has_changes(mmfile_t *one, mmfile_t *two,\n+\t\t struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws)\n {\n+\tif (!one)\n+\t\treturn contains(two, o, regexp, kws) != 0;\n+\tif (!two)\n+\t\treturn contains(one, o, regexp, kws) != 0;\n+\treturn contains(one, o, regexp, kws) != contains(two, o, regexp, kws);\n+}\n+\n+static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n+\t\t\t regex_t *regexp, kwset_t kws, pickaxe_fn fn)\n+{\n \tstruct userdiff_driver *textconv_one = NULL;\n \tstruct userdiff_driver *textconv_two = NULL;\n \tmmfile_t mf1, mf2;\n@@ -209,6 +194,10 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \tif (!o->pickaxe[0])\n \t\treturn 0;\n \n+\t/* ignore unmerged */\n+\tif (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two))\n+\t\treturn 0;\n+\n \tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n \t\ttextconv_one = get_textconv(p->one);\n \t\ttextconv_two = get_textconv(p->two);\n@@ -227,18 +216,9 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n \tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n \n-\tif (!DIFF_FILE_VALID(p->one)) {\n-\t\tif (!DIFF_FILE_VALID(p->two))\n-\t\t\tret = 0; /* ignore unmerged */\n-\t\telse\n-\t\t\t/* created */\n-\t\t\tret = contains(&mf2, o, regexp, kws) != 0;\n-\t}\n-\telse if (!DIFF_FILE_VALID(p->two)) /* removed */\n-\t\tret = contains(&mf1, o, regexp, kws) != 0;\n-\telse\n-\t\tret = contains(&mf1, o, regexp, kws) !=\n-\t\t contains(&mf2, o, regexp, kws);\n+\tret = fn(DIFF_FILE_VALID(p->one) ? &mf1 : NULL,\n+\t\t DIFF_FILE_VALID(p->two) ? &mf2 : NULL,\n+\t\t o, regexp, kws);\n \n \tif (textconv_one)\n \t\tfree(mf1.ptr);\n"},{"id":"213242","messageId":"7vk3ohedn1.fsf@alter.siamese.dyndns.org","threadId":"33379","inReplyTo":"20130405052810.GA29815@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-05T05:43:14Z","receivedAt":"2013-04-05T05:43:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I notice that you are stuck factoring out not just the setup, but also\n> the cleanup, and I wondered if things could be made even simpler by just\n> encapsulating the checking logic in a callback; then the setup and\n> cleanup flow more naturally, as they are in a single function wrapper.\n>\n> Like this, which ends up saving 20 lines rather than adding 7:\n\nOh, this is one of those many times I am reminded why I love having\nyou in the reviewer/contributor pool ;-)\n\n>\n> ---\n> diffcore-pickaxe.c | 118 +++++++++++++++--------------------\n> 1 file changed, 49 insertions(+), 69 deletions(-)\n>\n> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> index cadb071..63722f8 100644\n> --- a/diffcore-pickaxe.c\n> +++ b/diffcore-pickaxe.c\n> @@ -8,7 +8,12 @@\n> #include \"xdiff-interface.h\"\n> #include \"kwset.h\"\n> \n> -typedef int (*pickaxe_fn)(struct diff_filepair *p, struct diff_options *o, regex_t *regexp, kwset_t kws);\n> +typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,\n> +\t\t\t struct diff_options *o,\n> +\t\t\t regex_t *regexp, kwset_t kws);\n> +\n> +static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n> +\t\t\t regex_t *regexp, kwset_t kws, pickaxe_fn fn);\n> \n> static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,\n> \t\t regex_t *regexp, kwset_t kws, pickaxe_fn fn)\n> @@ -22,7 +27,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,\n> \t\t/* Showing the whole changeset if needle exists */\n> \t\tfor (i = 0; i < q->nr; i++) {\n> \t\t\tstruct diff_filepair *p = q->queue[i];\n> -\t\t\tif (fn(p, o, regexp, kws))\n> +\t\t\tif (pickaxe_match(p, o, regexp, kws, fn))\n> \t\t\t\treturn; /* do not munge the queue */\n> \t\t}\n> \n> @@ -37,7 +42,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,\n> \t\t/* Showing only the filepairs that has the needle */\n> \t\tfor (i = 0; i < q->nr; i++) {\n> \t\t\tstruct diff_filepair *p = q->queue[i];\n> -\t\t\tif (fn(p, o, regexp, kws))\n> +\t\t\tif (pickaxe_match(p, o, regexp, kws, fn))\n> \t\t\t\tdiff_q(&outq, p);\n> \t\t\telse\n> \t\t\t\tdiff_free_filepair(p);\n> @@ -74,64 +79,33 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n> \tline[len] = hold;\n> }\n> \n> -static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n> +static int diff_grep(mmfile_t *one, mmfile_t *two,\n> +\t\t struct diff_options *o,\n> \t\t regex_t *regexp, kwset_t kws)\n> {\n> \tregmatch_t regmatch;\n> -\tstruct userdiff_driver *textconv_one = NULL;\n> -\tstruct userdiff_driver *textconv_two = NULL;\n> -\tmmfile_t mf1, mf2;\n> -\tint hit;\n> +\tstruct diffgrep_cb ecbdata;\n> +\txpparam_t xpp;\n> +\txdemitconf_t xecfg;\n> \n> -\tif (!o->pickaxe[0])\n> -\t\treturn 0;\n> +\tif (!one)\n> +\t\treturn !regexec(regexp, two->ptr, 1, ®match, 0);\n> +\tif (!two)\n> +\t\treturn !regexec(regexp, one->ptr, 1, ®match, 0);\n> \n> -\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n> -\t\ttextconv_one = get_textconv(p->one);\n> -\t\ttextconv_two = get_textconv(p->two);\n> -\t}\n> -\n> -\tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n> -\t\treturn 0;\n> -\n> -\tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n> -\tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n> -\n> -\tif (!DIFF_FILE_VALID(p->one)) {\n> -\t\tif (!DIFF_FILE_VALID(p->two))\n> -\t\t\thit = 0; /* ignore unmerged */\n> -\t\telse\n> -\t\t\t/* created \"two\" -- does it have what we are looking for? */\n> -\t\t\thit = !regexec(regexp, mf2.ptr, 1, ®match, 0);\n> -\t} else if (!DIFF_FILE_VALID(p->two)) {\n> -\t\t/* removed \"one\" -- did it have what we are looking for? */\n> -\t\thit = !regexec(regexp, mf1.ptr, 1, ®match, 0);\n> -\t} else {\n> -\t\t/*\n> -\t\t * We have both sides; need to run textual diff and see if\n> -\t\t * the pattern appears on added/deleted lines.\n> -\t\t */\n> -\t\tstruct diffgrep_cb ecbdata;\n> -\t\txpparam_t xpp;\n> -\t\txdemitconf_t xecfg;\n> -\n> -\t\tmemset(&xpp, 0, sizeof(xpp));\n> -\t\tmemset(&xecfg, 0, sizeof(xecfg));\n> -\t\tecbdata.regexp = regexp;\n> -\t\tecbdata.hit = 0;\n> -\t\txecfg.ctxlen = o->context;\n> -\t\txecfg.interhunkctxlen = o->interhunkcontext;\n> -\t\txdi_diff_outf(&mf1, &mf2, diffgrep_consume, &ecbdata,\n> -\t\t\t &xpp, &xecfg);\n> -\t\thit = ecbdata.hit;\n> -\t}\n> -\tif (textconv_one)\n> -\t\tfree(mf1.ptr);\n> -\tif (textconv_two)\n> -\t\tfree(mf2.ptr);\n> -\tdiff_free_filespec_data(p->one);\n> -\tdiff_free_filespec_data(p->two);\n> -\treturn hit;\n> +\t/*\n> +\t * We have both sides; need to run textual diff and see if\n> +\t * the pattern appears on added/deleted lines.\n> +\t */\n> +\tmemset(&xpp, 0, sizeof(xpp));\n> +\tmemset(&xecfg, 0, sizeof(xecfg));\n> +\tecbdata.regexp = regexp;\n> +\tecbdata.hit = 0;\n> +\txecfg.ctxlen = o->context;\n> +\txecfg.interhunkctxlen = o->interhunkcontext;\n> +\txdi_diff_outf(one, two, diffgrep_consume, &ecbdata,\n> +\t\t &xpp, &xecfg);\n> +\treturn ecbdata.hit;\n> }\n> \n> static void diffcore_pickaxe_grep(struct diff_options *o)\n> @@ -198,9 +172,20 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n> \treturn cnt;\n> }\n> \n> -static int has_changes(struct diff_filepair *p, struct diff_options *o,\n> +static int has_changes(mmfile_t *one, mmfile_t *two,\n> +\t\t struct diff_options *o,\n> \t\t regex_t *regexp, kwset_t kws)\n> {\n> +\tif (!one)\n> +\t\treturn contains(two, o, regexp, kws) != 0;\n> +\tif (!two)\n> +\t\treturn contains(one, o, regexp, kws) != 0;\n> +\treturn contains(one, o, regexp, kws) != contains(two, o, regexp, kws);\n> +}\n> +\n> +static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n> +\t\t\t regex_t *regexp, kwset_t kws, pickaxe_fn fn)\n> +{\n> \tstruct userdiff_driver *textconv_one = NULL;\n> \tstruct userdiff_driver *textconv_two = NULL;\n> \tmmfile_t mf1, mf2;\n> @@ -209,6 +194,10 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n> \tif (!o->pickaxe[0])\n> \t\treturn 0;\n> \n> +\t/* ignore unmerged */\n> +\tif (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two))\n> +\t\treturn 0;\n> +\n> \tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n> \t\ttextconv_one = get_textconv(p->one);\n> \t\ttextconv_two = get_textconv(p->two);\n> @@ -227,18 +216,9 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n> \tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n> \tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n> \n> -\tif (!DIFF_FILE_VALID(p->one)) {\n> -\t\tif (!DIFF_FILE_VALID(p->two))\n> -\t\t\tret = 0; /* ignore unmerged */\n> -\t\telse\n> -\t\t\t/* created */\n> -\t\t\tret = contains(&mf2, o, regexp, kws) != 0;\n> -\t}\n> -\telse if (!DIFF_FILE_VALID(p->two)) /* removed */\n> -\t\tret = contains(&mf1, o, regexp, kws) != 0;\n> -\telse\n> -\t\tret = contains(&mf1, o, regexp, kws) !=\n> -\t\t contains(&mf2, o, regexp, kws);\n> +\tret = fn(DIFF_FILE_VALID(p->one) ? &mf1 : NULL,\n> +\t\t DIFF_FILE_VALID(p->two) ? &mf2 : NULL,\n> +\t\t o, regexp, kws);\n> \n> \tif (textconv_one)\n> \t\tfree(mf1.ptr);\n"},{"id":"213244","messageId":"20130405054524.GB12705@sigill.intra.peff.net","threadId":"33379","inReplyTo":"7vk3ohedn1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T05:45:24Z","receivedAt":"2013-04-05T05:45:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2013 at 10:43:14PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I notice that you are stuck factoring out not just the setup, but also\n> > the cleanup, and I wondered if things could be made even simpler by just\n> > encapsulating the checking logic in a callback; then the setup and\n> > cleanup flow more naturally, as they are in a single function wrapper.\n> >\n> > Like this, which ends up saving 20 lines rather than adding 7:\n> \n> Oh, this is one of those many times I am reminded why I love having\n> you in the reviewer/contributor pool ;-)\n\nI didn't actually test that patch beyond compilation (but it's\n_obviously_ correct, right?), and I'm about to go to bed. Do you want to\ntake care of adapting your commit message to it?\n\n-Peff\n"},{"id":"213251","messageId":"vpq7gkhqvbu.fsf@grenoble-inp.fr","threadId":"33379","inReplyTo":"7d36738417942b594c185953115a244ad6f3c7a0.1365105971.git.simon@ruderich.org","subject":"Re: [PATCH v2 3/3] diffcore-pickaxe: respect --no-textconv","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-04-05T07:40:21Z","receivedAt":"2013-04-05T07:40:21Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Simon Ruderich writes:\n\n> --- a/t/t4209-log-pickaxe.sh\n> +++ b/t/t4209-log-pickaxe.sh\n> @@ -116,4 +116,18 @@ test_expect_success 'log -S -i (nomatch)' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'log -S --textconv (missing textconv tool)' '\n> +\techo \"* diff=test\" >.gitattributes &&\n> +\ttest_must_fail git -c diff.test.textconv=missing log -Sfoo &&\n> +\trm .gitattributes\n> +'\n> +\n> +test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n> +\techo \"* diff=test\" >.gitattributes &&\n> +\tgit -c diff.test.textconv=missing log -Sfoo --no-textconv >actual &&\n> +\t>expect &&\n> +\ttest_cmp expect actual &&\n> +\trm .gitattributes\n> +'\n\nWhile we're there, we could test -G --no-textconv too.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"213290","messageId":"20130405131630.GA23017@ruderich.org","threadId":"33379","inReplyTo":"vpq7gkhqvbu.fsf@grenoble-inp.fr","subject":"[PATCH v3 3/3] diffcore-pickaxe: respect --no-textconv","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2013-04-05T13:16:30Z","receivedAt":"2013-04-05T13:16:30Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"git log -S doesn't respect --no-textconv:\n\n $ echo '*.txt diff=wrong' > .gitattributes\n $ git -c diff.wrong.textconv='xxx' log --no-textconv -Sfoo\n error: cannot run xxx: No such file or directory\n fatal: unable to read files to diff\n\nReported-by: Matthieu Moy \nSigned-off-by: Simon Ruderich \n---\nOn Fri, Apr 05, 2013 at 09:40:21AM +0200, Matthieu Moy wrote:\n> While we're there, we could test -G --no-textconv too.\n\nHello Matthieu,\n\nGood idea, I've added it.\n\nRegards\nSimon\n\n diffcore-pickaxe.c | 12 ++++++++----\n t/t4209-log-pickaxe.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 4 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 3124f49..26ddf00 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -86,8 +86,10 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \tif (diff_unmodified_pair(p))\n \t\treturn 0;\n \n-\ttextconv_one = get_textconv(p->one);\n-\ttextconv_two = get_textconv(p->two);\n+\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n+\t\ttextconv_one = get_textconv(p->one);\n+\t\ttextconv_two = get_textconv(p->two);\n+\t}\n \n \tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n \tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n@@ -201,8 +203,10 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \tif (!o->pickaxe[0])\n \t\treturn 0;\n \n-\ttextconv_one = get_textconv(p->one);\n-\ttextconv_two = get_textconv(p->two);\n+\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n+\t\ttextconv_one = get_textconv(p->one);\n+\t\ttextconv_two = get_textconv(p->two);\n+\t}\n \n \t/*\n \t * If we have an unmodified pair, we know that the count will be the\ndiff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\nindex eed7273..38fb80f 100755\n--- a/t/t4209-log-pickaxe.sh\n+++ b/t/t4209-log-pickaxe.sh\n@@ -80,6 +80,20 @@ test_expect_success 'log -G -i (match)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log -G --textconv (missing textconv tool)' '\n+\techo \"* diff=test\" >.gitattributes &&\n+\ttest_must_fail git -c diff.test.textconv=missing log -Gfoo &&\n+\trm .gitattributes\n+'\n+\n+test_expect_success 'log -G --no-textconv (missing textconv tool)' '\n+\techo \"* diff=test\" >.gitattributes &&\n+\tgit -c diff.test.textconv=missing log -Gfoo --no-textconv >actual &&\n+\t>expect &&\n+\ttest_cmp expect actual &&\n+\trm .gitattributes\n+'\n+\n test_expect_success 'log -S (nomatch)' '\n \tgit log -Spicked --format=%H >actual &&\n \t>expect &&\n@@ -116,4 +130,18 @@ test_expect_success 'log -S -i (nomatch)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log -S --textconv (missing textconv tool)' '\n+\techo \"* diff=test\" >.gitattributes &&\n+\ttest_must_fail git -c diff.test.textconv=missing log -Sfoo &&\n+\trm .gitattributes\n+'\n+\n+test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n+\techo \"* diff=test\" >.gitattributes &&\n+\tgit -c diff.test.textconv=missing log -Sfoo --no-textconv >actual &&\n+\t>expect &&\n+\ttest_cmp expect actual &&\n+\trm .gitattributes\n+'\n+\n test_done\n-- \n1.8.2\n\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"213292","messageId":"20130405132033.GB23017@ruderich.org","threadId":"33379","inReplyTo":"7vvc82ggy3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv()","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2013-04-05T13:20:33Z","receivedAt":"2013-04-05T13:20:33Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Thu, Apr 04, 2013 at 01:48:52PM -0700, Junio C Hamano wrote:\n> If I am reading the code correctly, it is has_changes(), which is\n> used for \"log -S\" (not \"log -G\" that uses diff_grep()), that does\n> the unnecessary get_textconv() unconditionally. The way diff_grep()\n> divides the work to make fill_one() responsible for filling the\n> textconv as necessary is internally consistent, and there is no\n> unnecessary call.\n\nYes, of course. I meant has_changes() which has the unnecessary\ncall.\n\n> Perhaps...\n>\n> \tThe fill_one() function is responsible for finding and\n> \tfilling the textconv filter as necessary, and is called by\n> \tdiff_grep() function that implements \"git log -G\".\n>\n> \tThe has_changes() function calls get_textconv() for two\n> \tsides being compared, before it checks to see if it was\n> \tasked to perform the pickaxe limiting with the -S option.\n> \tMove the code around to avoid this wastage. After that,\n> \tfill_one() is called to use the textconv.\n>\n> \tBy adding get_textconv() to diff_grep() and relieving\n> \tfill_one() of responsibility to find the textconv filter, we\n> \tcan avoid calling get_textconv() twice.\n\nSounds good to me.\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"213293","messageId":"7vr4ipc4fv.fsf@alter.siamese.dyndns.org","threadId":"33379","inReplyTo":"20130405054524.GB12705@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-05T16:44:52Z","receivedAt":"2013-04-05T16:44:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I didn't actually test that patch beyond compilation (but it's\n> _obviously_ correct, right?), and I'm about to go to bed. Do you want to\n> take care of adapting your commit message to it?\n\nThe code looks obviously correct, yes.\n\nWe might have to revisit the \"Is unmerged?\" thing, Cf. d7c9bf22351e\n(diffcore-rename: don't consider unmerged path as source,\n2011-03-23), but that is an unlikely corner case, especially for\nthese options (-S/-G).\n\n-- >8 --\nFrom: Jeff King \nDate: Fri, 5 Apr 2013 01:28:10 -0400\nSubject: [PATCH] diffcore-pickaxe: unify code for log -S/-G\n\nThe logic flow of has_changes() used for \"log -S\" and diff_grep()\nused for \"log -G\" are essentially the same. See if we have both\nsides that could be different in any interesting way, slurp the\ncontents in core, possibly after applying textconv, inspect the\ncontents, clean-up and report the result. The only difference\nbetween the two is how \"inspect\" step works.\n\nUnify this codeflow in a helper, pickaxe_match(), which takes a\ncallback function that implements the specific \"inspect\" step.\n\nAfter removing the common scaffolding code from the existing\nhas_changes() and diff_grep(), they each becomes such a callback\nfunction suitable for passing to pickaxe_match().\n\nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n diffcore-pickaxe.c | 118 ++++++++++++++++++++++-------------------------------\n 1 file changed, 49 insertions(+), 69 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex cadb071..63722f8 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -8,7 +8,12 @@\n #include \"xdiff-interface.h\"\n #include \"kwset.h\"\n \n-typedef int (*pickaxe_fn)(struct diff_filepair *p, struct diff_options *o, regex_t *regexp, kwset_t kws);\n+typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,\n+\t\t\t struct diff_options *o,\n+\t\t\t regex_t *regexp, kwset_t kws);\n+\n+static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n+\t\t\t regex_t *regexp, kwset_t kws, pickaxe_fn fn);\n \n static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws, pickaxe_fn fn)\n@@ -22,7 +27,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,\n \t\t/* Showing the whole changeset if needle exists */\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n-\t\t\tif (fn(p, o, regexp, kws))\n+\t\t\tif (pickaxe_match(p, o, regexp, kws, fn))\n \t\t\t\treturn; /* do not munge the queue */\n \t\t}\n \n@@ -37,7 +42,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,\n \t\t/* Showing only the filepairs that has the needle */\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n-\t\t\tif (fn(p, o, regexp, kws))\n+\t\t\tif (pickaxe_match(p, o, regexp, kws, fn))\n \t\t\t\tdiff_q(&outq, p);\n \t\t\telse\n \t\t\t\tdiff_free_filepair(p);\n@@ -74,64 +79,33 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n \tline[len] = hold;\n }\n \n-static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n+static int diff_grep(mmfile_t *one, mmfile_t *two,\n+\t\t struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws)\n {\n \tregmatch_t regmatch;\n-\tstruct userdiff_driver *textconv_one = NULL;\n-\tstruct userdiff_driver *textconv_two = NULL;\n-\tmmfile_t mf1, mf2;\n-\tint hit;\n+\tstruct diffgrep_cb ecbdata;\n+\txpparam_t xpp;\n+\txdemitconf_t xecfg;\n \n-\tif (!o->pickaxe[0])\n-\t\treturn 0;\n+\tif (!one)\n+\t\treturn !regexec(regexp, two->ptr, 1, ®match, 0);\n+\tif (!two)\n+\t\treturn !regexec(regexp, one->ptr, 1, ®match, 0);\n \n-\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n-\t\ttextconv_one = get_textconv(p->one);\n-\t\ttextconv_two = get_textconv(p->two);\n-\t}\n-\n-\tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n-\t\treturn 0;\n-\n-\tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n-\tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n-\n-\tif (!DIFF_FILE_VALID(p->one)) {\n-\t\tif (!DIFF_FILE_VALID(p->two))\n-\t\t\thit = 0; /* ignore unmerged */\n-\t\telse\n-\t\t\t/* created \"two\" -- does it have what we are looking for? */\n-\t\t\thit = !regexec(regexp, mf2.ptr, 1, ®match, 0);\n-\t} else if (!DIFF_FILE_VALID(p->two)) {\n-\t\t/* removed \"one\" -- did it have what we are looking for? */\n-\t\thit = !regexec(regexp, mf1.ptr, 1, ®match, 0);\n-\t} else {\n-\t\t/*\n-\t\t * We have both sides; need to run textual diff and see if\n-\t\t * the pattern appears on added/deleted lines.\n-\t\t */\n-\t\tstruct diffgrep_cb ecbdata;\n-\t\txpparam_t xpp;\n-\t\txdemitconf_t xecfg;\n-\n-\t\tmemset(&xpp, 0, sizeof(xpp));\n-\t\tmemset(&xecfg, 0, sizeof(xecfg));\n-\t\tecbdata.regexp = regexp;\n-\t\tecbdata.hit = 0;\n-\t\txecfg.ctxlen = o->context;\n-\t\txecfg.interhunkctxlen = o->interhunkcontext;\n-\t\txdi_diff_outf(&mf1, &mf2, diffgrep_consume, &ecbdata,\n-\t\t\t &xpp, &xecfg);\n-\t\thit = ecbdata.hit;\n-\t}\n-\tif (textconv_one)\n-\t\tfree(mf1.ptr);\n-\tif (textconv_two)\n-\t\tfree(mf2.ptr);\n-\tdiff_free_filespec_data(p->one);\n-\tdiff_free_filespec_data(p->two);\n-\treturn hit;\n+\t/*\n+\t * We have both sides; need to run textual diff and see if\n+\t * the pattern appears on added/deleted lines.\n+\t */\n+\tmemset(&xpp, 0, sizeof(xpp));\n+\tmemset(&xecfg, 0, sizeof(xecfg));\n+\tecbdata.regexp = regexp;\n+\tecbdata.hit = 0;\n+\txecfg.ctxlen = o->context;\n+\txecfg.interhunkctxlen = o->interhunkcontext;\n+\txdi_diff_outf(one, two, diffgrep_consume, &ecbdata,\n+\t\t &xpp, &xecfg);\n+\treturn ecbdata.hit;\n }\n \n static void diffcore_pickaxe_grep(struct diff_options *o)\n@@ -198,9 +172,20 @@ static unsigned int contains(mmfile_t *mf, struct diff_options *o,\n \treturn cnt;\n }\n \n-static int has_changes(struct diff_filepair *p, struct diff_options *o,\n+static int has_changes(mmfile_t *one, mmfile_t *two,\n+\t\t struct diff_options *o,\n \t\t regex_t *regexp, kwset_t kws)\n {\n+\tif (!one)\n+\t\treturn contains(two, o, regexp, kws) != 0;\n+\tif (!two)\n+\t\treturn contains(one, o, regexp, kws) != 0;\n+\treturn contains(one, o, regexp, kws) != contains(two, o, regexp, kws);\n+}\n+\n+static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n+\t\t\t regex_t *regexp, kwset_t kws, pickaxe_fn fn)\n+{\n \tstruct userdiff_driver *textconv_one = NULL;\n \tstruct userdiff_driver *textconv_two = NULL;\n \tmmfile_t mf1, mf2;\n@@ -209,6 +194,10 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \tif (!o->pickaxe[0])\n \t\treturn 0;\n \n+\t/* ignore unmerged */\n+\tif (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two))\n+\t\treturn 0;\n+\n \tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n \t\ttextconv_one = get_textconv(p->one);\n \t\ttextconv_two = get_textconv(p->two);\n@@ -227,18 +216,9 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \tmf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);\n \tmf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);\n \n-\tif (!DIFF_FILE_VALID(p->one)) {\n-\t\tif (!DIFF_FILE_VALID(p->two))\n-\t\t\tret = 0; /* ignore unmerged */\n-\t\telse\n-\t\t\t/* created */\n-\t\t\tret = contains(&mf2, o, regexp, kws) != 0;\n-\t}\n-\telse if (!DIFF_FILE_VALID(p->two)) /* removed */\n-\t\tret = contains(&mf1, o, regexp, kws) != 0;\n-\telse\n-\t\tret = contains(&mf1, o, regexp, kws) !=\n-\t\t contains(&mf2, o, regexp, kws);\n+\tret = fn(DIFF_FILE_VALID(p->one) ? &mf1 : NULL,\n+\t\t DIFF_FILE_VALID(p->two) ? &mf2 : NULL,\n+\t\t o, regexp, kws);\n \n \tif (textconv_one)\n \t\tfree(mf1.ptr);\n-- \n1.8.2-667-gbdcd2ef\n"},{"id":"213296","messageId":"7v4nfkdgub.fsf@alter.siamese.dyndns.org","threadId":"33379","inReplyTo":"20130405131630.GA23017@ruderich.org","subject":"Re: [PATCH v3 3/3] diffcore-pickaxe: respect --no-textconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-05T17:31:40Z","receivedAt":"2013-04-05T17:31:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; will replace the one in 'pu' with this.\n"}]}