* [BUG] git log -S not respecting --no-textconv @ 2013-04-04 8:34 Matthieu Moy 2013-04-04 16:03 ` [PATCH 1/2] diffcore-pickaxe: respect --no-textconv Simon Ruderich 0 siblings, 1 reply; 27+ messages in thread From: Matthieu Moy @ 2013-04-04 8:34 UTC (permalink / raw) To: git Hi, It seems the command "git log --no-textconv -Sfoo" still runs the textconv filter (noticed because I have a broken textconv filter that lets "git log -S" error out). Steps to reproduce: Create a repo with *.txt file(s) in it $ echo '*.txt diff=wrong' > .gitattributes An incorrect textconv filter errors out as expected: $ git -c diff.wrong.textconv='xxx' log -Sfoo error: cannot run xxx: No such file or directory fatal: unable to read files to diff but --no-textconv has no effect: $ 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 -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/2] diffcore-pickaxe: respect --no-textconv 2013-04-04 8:34 [BUG] git log -S not respecting --no-textconv Matthieu Moy @ 2013-04-04 16:03 ` Simon Ruderich 2013-04-04 17:03 ` Matthieu Moy ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Simon Ruderich @ 2013-04-04 16:03 UTC (permalink / raw) To: Matthieu Moy; +Cc: git 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 Signed-off-by: Simon Ruderich <simon@ruderich.org> Reported-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> --- On Thu, Apr 04, 2013 at 10:34:17AM +0200, Matthieu Moy wrote: > Hi, > > It seems the command "git log --no-textconv -Sfoo" still runs the > textconv filter (noticed because I have a broken textconv filter that > lets "git log -S" error out). > > [snip] Hello Matthieu, This patch should fix it. All tests pass. Regards Simon diffcore-pickaxe.c | 15 ++++++++++++--- t/t4209-log-pickaxe.sh | 14 ++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index b097fa7..f814a52 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -78,7 +78,6 @@ static void fill_one(struct diff_filespec *one, mmfile_t *mf, struct userdiff_driver **textconv) { if (DIFF_FILE_VALID(one)) { - *textconv = get_textconv(one); mf->size = fill_textconv(*textconv, one, &mf->ptr); } else { memset(mf, 0, sizeof(*mf)); @@ -97,6 +96,11 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, if (diff_unmodified_pair(p)) return 0; + if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) { + 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); @@ -201,14 +205,19 @@ 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; + if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) { + textconv_one = get_textconv(p->one); + textconv_two = get_textconv(p->two); + } + /* * If we have an unmodified pair, we know that the count will be the * same and don't even have to load the blobs. Unless textconv is in diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh index eed7273..953cec8 100755 --- a/t/t4209-log-pickaxe.sh +++ b/t/t4209-log-pickaxe.sh @@ -116,4 +116,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 -- 1.8.2 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] diffcore-pickaxe: respect --no-textconv 2013-04-04 16:03 ` [PATCH 1/2] diffcore-pickaxe: respect --no-textconv Simon Ruderich @ 2013-04-04 17:03 ` Matthieu Moy 2013-04-04 17:43 ` Jeff King 2013-04-04 17:45 ` Junio C Hamano 2 siblings, 0 replies; 27+ messages in thread From: Matthieu Moy @ 2013-04-04 17:03 UTC (permalink / raw) To: Simon Ruderich; +Cc: git Simon Ruderich <simon@ruderich.org> writes: > This patch should fix it. It does. > --- a/diffcore-pickaxe.c > +++ b/diffcore-pickaxe.c > @@ -78,7 +78,6 @@ static void fill_one(struct diff_filespec *one, > mmfile_t *mf, struct userdiff_driver **textconv) > { > if (DIFF_FILE_VALID(one)) { > - *textconv = get_textconv(one); > mf->size = fill_textconv(*textconv, one, &mf->ptr); > } else { > memset(mf, 0, sizeof(*mf)); I don't understand why this is needed (not rethorical, I just didn't investigate), but the rest of the code looks straightforward and correct. Thanks, -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] diffcore-pickaxe: respect --no-textconv 2013-04-04 16:03 ` [PATCH 1/2] diffcore-pickaxe: respect --no-textconv Simon Ruderich 2013-04-04 17:03 ` Matthieu Moy @ 2013-04-04 17:43 ` Jeff King 2013-04-04 17:45 ` Junio C Hamano 2 siblings, 0 replies; 27+ messages in thread From: Jeff King @ 2013-04-04 17:43 UTC (permalink / raw) To: Simon Ruderich; +Cc: Matthieu Moy, git On Thu, Apr 04, 2013 at 06:03:59PM +0200, Simon Ruderich wrote: > 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 > > Signed-off-by: Simon Ruderich <simon@ruderich.org> > Reported-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> Usually these pseudo-headers are meant to be chronological, so you would swap the order. > > It seems the command "git log --no-textconv -Sfoo" still runs the > > textconv filter (noticed because I have a broken textconv filter that > > lets "git log -S" error out). > > > > [snip] > > Hello Matthieu, > > This patch should fix it. All tests pass. Thanks, the patch looks correct to me, although... > diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c > index b097fa7..f814a52 100644 > --- a/diffcore-pickaxe.c > +++ b/diffcore-pickaxe.c > @@ -78,7 +78,6 @@ static void fill_one(struct diff_filespec *one, > mmfile_t *mf, struct userdiff_driver **textconv) > { > if (DIFF_FILE_VALID(one)) { > - *textconv = get_textconv(one); > mf->size = fill_textconv(*textconv, one, &mf->ptr); Dropping this is the right thing to do with the rest of your patch, but like Matthieu, it took me a second to see why. Something like this should probably go into the commit message: We need to check that the ALLOW_TEXTCONV diff option is set before loading the textconv drivers. Rather than pass the diff options structure into the fill_one helper, let's just determine the textconv driver outside of that function and pass that in. Though I really think that justification would make more sense if your second cleanup patch came first, pulling the get_textconv calls back out to the callers (which is easy to justify, since one of the callers already has to load them itself _anyway_, which is just ugly). -Peff ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] diffcore-pickaxe: respect --no-textconv 2013-04-04 16:03 ` [PATCH 1/2] diffcore-pickaxe: respect --no-textconv Simon Ruderich 2013-04-04 17:03 ` Matthieu Moy 2013-04-04 17:43 ` Jeff King @ 2013-04-04 17:45 ` Junio C Hamano 2013-04-04 17:51 ` Jeff King 2 siblings, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2013-04-04 17:45 UTC (permalink / raw) To: Jeff King; +Cc: Simon Ruderich, Matthieu Moy, git Simon Ruderich <simon@ruderich.org> writes: > 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 <Matthieu.Moy@grenoble-inp.fr> > Signed-off-by: Simon Ruderich <simon@ruderich.org> > --- Sounds sensible. With this change fill_one() no longer needs to update textconv, it can just take a pointer to one, not a pointer to a pointer to one, which is [2/2]. Peff, anything I missed? > On Thu, Apr 04, 2013 at 10:34:17AM +0200, Matthieu Moy wrote: >> Hi, >> >> It seems the command "git log --no-textconv -Sfoo" still runs the >> textconv filter (noticed because I have a broken textconv filter that >> lets "git log -S" error out). >> >> [snip] > > Hello Matthieu, > > This patch should fix it. All tests pass. > > Regards > Simon > > diffcore-pickaxe.c | 15 ++++++++++++--- > t/t4209-log-pickaxe.sh | 14 ++++++++++++++ > 2 files changed, 26 insertions(+), 3 deletions(-) > > diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c > index b097fa7..f814a52 100644 > --- a/diffcore-pickaxe.c > +++ b/diffcore-pickaxe.c > @@ -78,7 +78,6 @@ static void fill_one(struct diff_filespec *one, > mmfile_t *mf, struct userdiff_driver **textconv) > { > if (DIFF_FILE_VALID(one)) { > - *textconv = get_textconv(one); > mf->size = fill_textconv(*textconv, one, &mf->ptr); > } else { > memset(mf, 0, sizeof(*mf)); > @@ -97,6 +96,11 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, > if (diff_unmodified_pair(p)) > return 0; > > + if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) { > + 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); > > @@ -201,14 +205,19 @@ 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; > > + if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) { > + textconv_one = get_textconv(p->one); > + textconv_two = get_textconv(p->two); > + } > + > /* > * If we have an unmodified pair, we know that the count will be the > * same and don't even have to load the blobs. Unless textconv is in > diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh > index eed7273..953cec8 100755 > --- a/t/t4209-log-pickaxe.sh > +++ b/t/t4209-log-pickaxe.sh > @@ -116,4 +116,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 > -- > 1.8.2 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] diffcore-pickaxe: respect --no-textconv 2013-04-04 17:45 ` Junio C Hamano @ 2013-04-04 17:51 ` Jeff King 2013-04-04 18:10 ` Junio C Hamano 0 siblings, 1 reply; 27+ messages in thread From: Jeff King @ 2013-04-04 17:51 UTC (permalink / raw) To: Junio C Hamano; +Cc: Simon Ruderich, Matthieu Moy, git On Thu, Apr 04, 2013 at 10:45:25AM -0700, Junio C Hamano wrote: > Simon Ruderich <simon@ruderich.org> writes: > > > 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 <Matthieu.Moy@grenoble-inp.fr> > > Signed-off-by: Simon Ruderich <simon@ruderich.org> > > --- > > Sounds sensible. > > With this change fill_one() no longer needs to update textconv, it > can just take a pointer to one, not a pointer to a pointer to one, > which is [2/2]. > > Peff, anything I missed? I'm OK with this as-is, but I would also be happy to see the re-ordering and extra cleanup I mentioned elsewhere. But either way: Acked-by: Jeff King <peff@peff.net> -Peff ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] diffcore-pickaxe: respect --no-textconv 2013-04-04 17:51 ` Jeff King @ 2013-04-04 18:10 ` Junio C Hamano 2013-04-04 20:20 ` [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv() Simon Ruderich ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Junio C Hamano @ 2013-04-04 18:10 UTC (permalink / raw) To: Jeff King; +Cc: Simon Ruderich, Matthieu Moy, git Jeff King <peff@peff.net> writes: > I'm OK with this as-is, but I would also be happy to see the re-ordering > and extra cleanup I mentioned elsewhere. Yeah, I agree that the order is the other way around. 2/2 could be retitled to say that fill_one() no longer needs to touch, but swapping the order with the extra clean-up would be much cleaner. > > But either way: > > Acked-by: Jeff King <peff@peff.net> > > -Peff ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv() 2013-04-04 18:10 ` Junio C Hamano @ 2013-04-04 20:20 ` Simon Ruderich 2013-04-04 20:48 ` Junio C Hamano 2013-04-04 20:21 ` [PATCH v2 2/3] diffcore-pickaxe: remove fill_one() Simon Ruderich 2013-04-04 20:21 ` [PATCH v2 3/3] diffcore-pickaxe: respect --no-textconv Simon Ruderich 2 siblings, 1 reply; 27+ messages in thread From: Simon Ruderich @ 2013-04-04 20:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Matthieu Moy, git get_textconv() is called in diff_grep() to determine the textconv driver before calling fill_one() and then again in fill_one(). Remove this unnecessary call by determining the textconv driver before calling fill_one(). 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 <simon@ruderich.org> --- Hello, I've reordered the patches as requested and included Jeff's cleanup patch. Regards Simon diffcore-pickaxe.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index b097fa7..8f955f8 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) -- 1.8.2 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv() 2013-04-04 20:20 ` [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv() Simon Ruderich @ 2013-04-04 20:48 ` Junio C Hamano 2013-04-04 21:10 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Junio C Hamano @ 2013-04-04 20:48 UTC (permalink / raw) To: Jeff King, Simon Ruderich; +Cc: Matthieu Moy, git Simon Ruderich <simon@ruderich.org> writes: > get_textconv() is called in diff_grep() to determine the textconv driver > before calling fill_one() and then again in fill_one(). Remove this > unnecessary call by determining the textconv driver before calling > fill_one(). If I am reading the code correctly, it is has_changes(), which is used for "log -S" (not "log -G" that uses diff_grep()), that does the unnecessary get_textconv() unconditionally. The way diff_grep() divides the work to make fill_one() responsible for filling the textconv as necessary is internally consistent, and there is no unnecessary call. Perhaps... 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<pattern>". The has_changes() function calls get_textconv() for two sides being compared, before it checks to see if it was asked to perform the pickaxe limiting with the -S option. Move the code around to avoid this wastage. After that, fill_one() is called to use the textconv. 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. Explained that way, it makes me wonder why we cannot fix it the other way around, that is, not fetching textconv in has_changes() and instead letting fill_one() to find textconv as needed. The answer is because has_changes() itself looks at the textconv. But we have to wonder why it is so. diff_grep() short-circuits when the two sides are identical and has_changes() has a similar but different logic to check if the identical two sides are processed with the same textconv filter before saying this filepair is uninteresting. Shouldn't that logic be unified as well? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv() 2013-04-04 20:48 ` Junio C Hamano @ 2013-04-04 21:10 ` Junio C Hamano 2013-04-04 21:11 ` Jeff King 2013-04-05 13:20 ` Simon Ruderich 2 siblings, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2013-04-04 21:10 UTC (permalink / raw) To: Jeff King; +Cc: Simon Ruderich, Matthieu Moy, git Junio C Hamano <gitster@pobox.com> writes: > Perhaps... > > 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<pattern>". > > The has_changes() function calls get_textconv() for two > sides being compared, before it checks to see if it was > asked to perform the pickaxe limiting with the -S option. > Move the code around to avoid this wastage. After that, > fill_one() is called to use the textconv. > > 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. > > Explained that way, it makes me wonder why we cannot fix it the > other way around, that is, not fetching textconv in has_changes() > and instead letting fill_one() to find textconv as needed. > > The answer is because has_changes() itself looks at the textconv. > > But we have to wonder why it is so. diff_grep() short-circuits when > the two sides are identical and has_changes() has a similar but > different logic to check if the identical two sides are processed > with the same textconv filter before saying this filepair is > uninteresting. > > Shouldn't that logic be unified as well? That is, something along this line. We may want to unify these two functions even more, but I do not offhand see a good way to do so. -- >8 -- Subject: [PATCH] log -S/-G: unify the short-cut logic a bit more 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<pattern>". The has_changes() function calls get_textconv() for two sides being compared, before it checks to see if it was asked to perform the pickaxe limiting with the -S option. Move the code around to avoid this wastage. 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; fill_one() no longer has to be passed a pointer to a pointer to textconv. The reason has_changes() calls get_textconv() early is because it wants to compare two textconv filters on both sides. When comparing the two sides that are otherwise identical, if the same textconv applies to both sides, we know pickaxe (either -S or -G) would not find anything before applying the textconv and comparing the result. Teach the same short-circuit logic to diff_grep() as well. The code in these two functions become more similar as the result. --- diffcore-pickaxe.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index b097fa7..6d285a7 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)); } @@ -94,11 +93,24 @@ 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; - 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); + + /* + * If we have an unmodified pair, we know that the count will be the + * same and don't even have to load the blobs. Unless textconv is in + * play, _and_ we are using two different textconv filters (e.g., + * because a pair is an exact rename with different textconv attributes + * for each side, which might generate different content). + */ + 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); if (!mf1.ptr) { if (!mf2.ptr) @@ -131,6 +143,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; } @@ -201,14 +215,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 +236,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) -- 1.8.2-634-g203ba97 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv() 2013-04-04 20:48 ` Junio C Hamano 2013-04-04 21:10 ` Junio C Hamano @ 2013-04-04 21:11 ` Jeff King 2013-04-04 22:51 ` Junio C Hamano 2013-04-05 13:20 ` Simon Ruderich 2 siblings, 1 reply; 27+ messages in thread From: Jeff King @ 2013-04-04 21:11 UTC (permalink / raw) To: Junio C Hamano; +Cc: Simon Ruderich, Matthieu Moy, git On Thu, Apr 04, 2013 at 01:48:52PM -0700, Junio C Hamano wrote: > If I am reading the code correctly, it is has_changes(), which is > used for "log -S" (not "log -G" that uses diff_grep()), that does > the unnecessary get_textconv() unconditionally. The way diff_grep() > divides the work to make fill_one() responsible for filling the > textconv as necessary is internally consistent, and there is no > unnecessary call. > > Perhaps... > > 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<pattern>". > > The has_changes() function calls get_textconv() for two > sides being compared, before it checks to see if it was > asked to perform the pickaxe limiting with the -S option. > Move the code around to avoid this wastage. After that, > fill_one() is called to use the textconv. > > 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. > > Explained that way, it makes me wonder why we cannot fix it the > other way around, that is, not fetching textconv in has_changes() > and instead letting fill_one() to find textconv as needed. > > The answer is because has_changes() itself looks at the textconv. > > But we have to wonder why it is so. diff_grep() short-circuits when > the two sides are identical and has_changes() has a similar but > different logic to check if the identical two sides are processed > with the same textconv filter before saying this filepair is > uninteresting. > > Shouldn't that logic be unified as well? I think it would be OK to perform the same optimization in diff_grep; I do not recall offhand why I didn't do so when touching this code last time, so it may have just been because I didn't think of it. But I do not think fill_one is the right interface for it. The reason has_changes calls get_textconv separately is that we do not want to fill the buffer (which may be expensive) if we can avoid it. So the correct sequence is: 1. Find out textconv_one. 2. Find out textconv_two. 3. Check "!hashcmp(one->sha1, two->sha1) && textconv_one == textconv_two"; if true, then we know the content we are about to compare will be identical, and we can return early. 4. Otherwise, retrieve the content for one (respecting textconv_one). 5. Retrieve the content for two (respecting textconv_two). You cannot implement the optimization looking only at one side (obviously), but you also need to split the textconv lookup from the "fill" procedure if you want the optimization to come at the right place. If you turned fill_one into "fill_both_sides" then you could share the code between diff_grep/pickaxe and do it in the right order in the helper. -Peff ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv() 2013-04-04 21:11 ` Jeff King @ 2013-04-04 22:51 ` Junio C Hamano 0 siblings, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2013-04-04 22:51 UTC (permalink / raw) To: Jeff King; +Cc: Simon Ruderich, Matthieu Moy, git Jeff King <peff@peff.net> writes: > But I do not think fill_one is the right interface for it. The reason > has_changes calls get_textconv separately is that we do not want to fill > the buffer (which may be expensive) if we can avoid it. So the correct > sequence is: ... > If you turned fill_one into "fill_both_sides" then you could share > the code between diff_grep/pickaxe and do it in the right order in the > helper. Yeah, I think that is a good way to refactor. A patch later in the series will be killing fill_one() anyway ;-) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv() 2013-04-04 20:48 ` Junio C Hamano 2013-04-04 21:10 ` Junio C Hamano 2013-04-04 21:11 ` Jeff King @ 2013-04-05 13:20 ` Simon Ruderich 2 siblings, 0 replies; 27+ messages in thread From: Simon Ruderich @ 2013-04-05 13:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Matthieu Moy, git On Thu, Apr 04, 2013 at 01:48:52PM -0700, Junio C Hamano wrote: > If I am reading the code correctly, it is has_changes(), which is > used for "log -S" (not "log -G" that uses diff_grep()), that does > the unnecessary get_textconv() unconditionally. The way diff_grep() > divides the work to make fill_one() responsible for filling the > textconv as necessary is internally consistent, and there is no > unnecessary call. Yes, of course. I meant has_changes() which has the unnecessary call. > Perhaps... > > 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<pattern>". > > The has_changes() function calls get_textconv() for two > sides being compared, before it checks to see if it was > asked to perform the pickaxe limiting with the -S option. > Move the code around to avoid this wastage. After that, > fill_one() is called to use the textconv. > > 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. Sounds good to me. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 2/3] diffcore-pickaxe: remove fill_one() 2013-04-04 18:10 ` Junio C Hamano 2013-04-04 20:20 ` [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv() Simon Ruderich @ 2013-04-04 20:21 ` Simon Ruderich 2013-04-05 0:08 ` Jeff King 2013-04-04 20:21 ` [PATCH v2 3/3] diffcore-pickaxe: respect --no-textconv Simon Ruderich 2 siblings, 1 reply; 27+ messages in thread From: Simon Ruderich @ 2013-04-04 20:21 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Matthieu Moy, git From: Jeff King <peff@peff.net> 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. Signed-off-by: Simon Ruderich <simon@ruderich.org> --- On Thu, Apr 04, 2013 at 01:49:42PM -0400, Jeff King wrote: > [snip] > > What do you think of something like this on top (this is on top of your > patches, but again, I would suggest re-ordering your two, so it would > come as patch 2/3): Hello Jeff, That's a good idea. I've added your patch, thanks. Signed-off? Regards Simon diffcore-pickaxe.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 8f955f8..3124f49 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) != -- 1.8.2 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/3] diffcore-pickaxe: remove fill_one() 2013-04-04 20:21 ` [PATCH v2 2/3] diffcore-pickaxe: remove fill_one() Simon Ruderich @ 2013-04-05 0:08 ` Jeff King 2013-04-05 4:43 ` Junio C Hamano 0 siblings, 1 reply; 27+ messages in thread From: Jeff King @ 2013-04-05 0:08 UTC (permalink / raw) To: Simon Ruderich; +Cc: Junio C Hamano, Matthieu Moy, git On Thu, Apr 04, 2013 at 10:21:08PM +0200, Simon Ruderich wrote: > From: Jeff King <peff@peff.net> > > 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. > > Signed-off-by: Simon Ruderich <simon@ruderich.org> > --- > On Thu, Apr 04, 2013 at 01:49:42PM -0400, Jeff King wrote: > > [snip] > > > > What do you think of something like this on top (this is on top of your > > patches, but again, I would suggest re-ordering your two, so it would > > come as patch 2/3): > > Hello Jeff, > > That's a good idea. I've added your patch, thanks. Signed-off? Thanks. The whole series looks good to me. I think Junio's proposed cleanup is a good direction, too, but I don't mind if that comes on top. Here's an updated version of the patch with my signoff; I also expanded on the commit message a little bit. The patch text is the same (I'm just including the whole patch for Junio's convenience in applying). -Peff -- >8 -- Subject: [PATCH] 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 <peff@peff.net> Signed-off-by: Simon Ruderich <simon@ruderich.org> --- diffcore-pickaxe.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 8f955f8..3124f49 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) != -- 1.8.2.rc0.33.gd915649 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/3] diffcore-pickaxe: remove fill_one() 2013-04-05 0:08 ` Jeff King @ 2013-04-05 4:43 ` Junio C Hamano 2013-04-05 4:45 ` [PATCH 1/3] diffcore-pickaxe: port optimization from has_changes() to diff_grep() Junio C Hamano 0 siblings, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2013-04-05 4:43 UTC (permalink / raw) To: Jeff King; +Cc: Simon Ruderich, Matthieu Moy, git Jeff King <peff@peff.net> writes: > Thanks. The whole series looks good to me. I think Junio's proposed > cleanup is a good direction, too, but I don't mind if that comes on top. I'll send out a three-patch follow-up shortly. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/3] diffcore-pickaxe: port optimization from has_changes() to diff_grep() 2013-04-05 4:43 ` Junio C Hamano @ 2013-04-05 4:45 ` Junio C Hamano 2013-04-05 4:45 ` [PATCH 2/3] diffcore-pickaxe: fix leaks in "log -S<block>" and "log -G<pattern>" Junio C Hamano 2013-04-05 4:45 ` [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G Junio C Hamano 0 siblings, 2 replies; 27+ messages in thread From: Junio C Hamano @ 2013-04-05 4:45 UTC (permalink / raw) To: git These two functions are called in the same codeflow to implement "log -S<block>" and "log -G<pattern>", 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 <gitster@pobox.com> --- diffcore-pickaxe.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 26ddf00..bfaabab 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; } -- 1.8.2-588-gbf1c992 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 2/3] diffcore-pickaxe: fix leaks in "log -S<block>" and "log -G<pattern>" 2013-04-05 4:45 ` [PATCH 1/3] diffcore-pickaxe: port optimization from has_changes() to diff_grep() Junio C Hamano @ 2013-04-05 4:45 ` Junio C Hamano 2013-04-05 4:45 ` [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G Junio C Hamano 1 sibling, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2013-04-05 4:45 UTC (permalink / raw) To: git 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 <gitster@pobox.com> --- diffcore-pickaxe.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index bfaabab..cadb071 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; -- 1.8.2-588-gbf1c992 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G 2013-04-05 4:45 ` [PATCH 1/3] diffcore-pickaxe: port optimization from has_changes() to diff_grep() Junio C Hamano 2013-04-05 4:45 ` [PATCH 2/3] diffcore-pickaxe: fix leaks in "log -S<block>" and "log -G<pattern>" Junio C Hamano @ 2013-04-05 4:45 ` Junio C Hamano 2013-04-05 5:28 ` Jeff King 1 sibling, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2013-04-05 4:45 UTC (permalink / raw) To: git The logic to decide early to do nothing and prepare the data to be inspected are the same between has_changes() and diff_grep(). Introduce pickaxe_setup() helper to share the same code. Similarly, introduce pickaxe_finish_filepair() to clean up after these two functions are done with a filepair. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diffcore-pickaxe.c | 103 ++++++++++++++++++++++++++++------------------------- 1 file changed, 55 insertions(+), 48 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index cadb071..ac5a28d 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -48,6 +48,54 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o, *q = outq; } +static int pickaxe_setup(struct diff_filepair *p, + struct diff_options *o, + mmfile_t *mf_one, + mmfile_t *mf_two, + unsigned *what_to_free) +{ + struct userdiff_driver *textconv_one = NULL; + struct userdiff_driver *textconv_two = NULL; + + if (!o->pickaxe[0]) + return 0; + + if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) { + textconv_one = get_textconv(p->one); + textconv_two = get_textconv(p->two); + } + + /* + * If we have an unmodified pair, we know that there is no + * interesting difference and we don't even have to load the + * blobs, unless textconv is in play, _and_ we are using two + * different textconv filters (e.g., because a pair is an + * exact rename with different textconv attributes for each + * side, which might generate different content). + */ + if (textconv_one == textconv_two && diff_unmodified_pair(p)) + return 0; + + mf_one->size = fill_textconv(textconv_one, p->one, &mf_one->ptr); + mf_two->size = fill_textconv(textconv_two, p->two, &mf_two->ptr); + + *what_to_free = (textconv_one ? 1 : 0) | (textconv_two ? 2 : 0); + return 1; +} + +static void pickaxe_finish_filepair(struct diff_filepair *p, + mmfile_t *mf_one, + mmfile_t *mf_two, + unsigned what_to_free) +{ + if (what_to_free & 1) + free(mf_one->ptr); + if (what_to_free & 2) + free(mf_two->ptr); + diff_free_filespec_data(p->one); + diff_free_filespec_data(p->two); +} + struct diffgrep_cb { regex_t *regexp; int hit; @@ -78,25 +126,13 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, regex_t *regexp, kwset_t kws) { regmatch_t regmatch; - struct userdiff_driver *textconv_one = NULL; - struct userdiff_driver *textconv_two = NULL; mmfile_t mf1, mf2; + unsigned what_to_free; int hit; - if (!o->pickaxe[0]) + if (!pickaxe_setup(p, o, &mf1, &mf2, &what_to_free)) return 0; - if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) { - textconv_one = get_textconv(p->one); - textconv_two = get_textconv(p->two); - } - - if (textconv_one == textconv_two && diff_unmodified_pair(p)) - return 0; - - mf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr); - mf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr); - if (!DIFF_FILE_VALID(p->one)) { if (!DIFF_FILE_VALID(p->two)) hit = 0; /* ignore unmerged */ @@ -125,12 +161,8 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, &xpp, &xecfg); hit = ecbdata.hit; } - if (textconv_one) - free(mf1.ptr); - if (textconv_two) - free(mf2.ptr); - diff_free_filespec_data(p->one); - diff_free_filespec_data(p->two); + + pickaxe_finish_filepair(p, &mf1, &mf2, what_to_free); return hit; } @@ -201,32 +233,13 @@ static unsigned int contains(mmfile_t *mf, struct diff_options *o, static int has_changes(struct diff_filepair *p, struct diff_options *o, regex_t *regexp, kwset_t kws) { - struct userdiff_driver *textconv_one = NULL; - struct userdiff_driver *textconv_two = NULL; mmfile_t mf1, mf2; + unsigned what_to_free; int ret; - if (!o->pickaxe[0]) - return 0; - - if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) { - textconv_one = get_textconv(p->one); - textconv_two = get_textconv(p->two); - } - - /* - * If we have an unmodified pair, we know that the count will be the - * same and don't even have to load the blobs. Unless textconv is in - * play, _and_ we are using two different textconv filters (e.g., - * because a pair is an exact rename with different textconv attributes - * for each side, which might generate different content). - */ - if (textconv_one == textconv_two && diff_unmodified_pair(p)) + if (!pickaxe_setup(p, o, &mf1, &mf2, &what_to_free)) 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)) ret = 0; /* ignore unmerged */ @@ -240,13 +253,7 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o, ret = contains(&mf1, o, regexp, kws) != contains(&mf2, o, regexp, kws); - if (textconv_one) - free(mf1.ptr); - if (textconv_two) - free(mf2.ptr); - diff_free_filespec_data(p->one); - diff_free_filespec_data(p->two); - + pickaxe_finish_filepair(p, &mf1, &mf2, what_to_free); return ret; } -- 1.8.2-588-gbf1c992 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G 2013-04-05 4:45 ` [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G Junio C Hamano @ 2013-04-05 5:28 ` Jeff King 2013-04-05 5:43 ` Junio C Hamano 0 siblings, 1 reply; 27+ messages in thread From: Jeff King @ 2013-04-05 5:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, Apr 04, 2013 at 09:45:26PM -0700, Junio C Hamano wrote: > The logic to decide early to do nothing and prepare the data to be > inspected are the same between has_changes() and diff_grep(). > Introduce pickaxe_setup() helper to share the same code. > > Similarly, introduce pickaxe_finish_filepair() to clean up after > these two functions are done with a filepair. All three patches look fine to me. I notice that you are stuck factoring out not just the setup, but also the cleanup, and I wondered if things could be made even simpler by just encapsulating the checking logic in a callback; then the setup and cleanup flow more naturally, as they are in a single function wrapper. Like this, which ends up saving 20 lines rather than adding 7: --- diffcore-pickaxe.c | 118 +++++++++++++++-------------------- 1 file changed, 49 insertions(+), 69 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index cadb071..63722f8 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -8,7 +8,12 @@ #include "xdiff-interface.h" #include "kwset.h" -typedef int (*pickaxe_fn)(struct diff_filepair *p, struct diff_options *o, regex_t *regexp, kwset_t kws); +typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two, + struct diff_options *o, + regex_t *regexp, kwset_t kws); + +static int pickaxe_match(struct diff_filepair *p, struct diff_options *o, + regex_t *regexp, kwset_t kws, pickaxe_fn fn); static void pickaxe(struct diff_queue_struct *q, struct diff_options *o, regex_t *regexp, kwset_t kws, pickaxe_fn fn) @@ -22,7 +27,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o, /* Showing the whole changeset if needle exists */ for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - if (fn(p, o, regexp, kws)) + if (pickaxe_match(p, o, regexp, kws, fn)) return; /* do not munge the queue */ } @@ -37,7 +42,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o, /* Showing only the filepairs that has the needle */ for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - if (fn(p, o, regexp, kws)) + if (pickaxe_match(p, o, regexp, kws, fn)) diff_q(&outq, p); else diff_free_filepair(p); @@ -74,64 +79,33 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, line[len] = hold; } -static int diff_grep(struct diff_filepair *p, struct diff_options *o, +static int diff_grep(mmfile_t *one, mmfile_t *two, + struct diff_options *o, regex_t *regexp, kwset_t kws) { regmatch_t regmatch; - struct userdiff_driver *textconv_one = NULL; - struct userdiff_driver *textconv_two = NULL; - mmfile_t mf1, mf2; - int hit; + struct diffgrep_cb ecbdata; + xpparam_t xpp; + xdemitconf_t xecfg; - if (!o->pickaxe[0]) - return 0; + if (!one) + return !regexec(regexp, two->ptr, 1, ®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,9 +172,20 @@ static int has_changes(struct diff_filepair *p, 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; mmfile_t mf1, mf2; @@ -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); ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G 2013-04-05 5:28 ` Jeff King @ 2013-04-05 5:43 ` Junio C Hamano 2013-04-05 5:45 ` Jeff King 0 siblings, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2013-04-05 5:43 UTC (permalink / raw) To: Jeff King; +Cc: git Jeff King <peff@peff.net> writes: > I notice that you are stuck factoring out not just the setup, but also > the cleanup, and I wondered if things could be made even simpler by just > encapsulating the checking logic in a callback; then the setup and > cleanup flow more naturally, as they are in a single function wrapper. > > Like this, which ends up saving 20 lines rather than adding 7: Oh, this is one of those many times I am reminded why I love having you in the reviewer/contributor pool ;-) > > --- > diffcore-pickaxe.c | 118 +++++++++++++++-------------------- > 1 file changed, 49 insertions(+), 69 deletions(-) > > diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c > index cadb071..63722f8 100644 > --- a/diffcore-pickaxe.c > +++ b/diffcore-pickaxe.c > @@ -8,7 +8,12 @@ > #include "xdiff-interface.h" > #include "kwset.h" > > -typedef int (*pickaxe_fn)(struct diff_filepair *p, struct diff_options *o, regex_t *regexp, kwset_t kws); > +typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two, > + struct diff_options *o, > + regex_t *regexp, kwset_t kws); > + > +static int pickaxe_match(struct diff_filepair *p, struct diff_options *o, > + regex_t *regexp, kwset_t kws, pickaxe_fn fn); > > static void pickaxe(struct diff_queue_struct *q, struct diff_options *o, > regex_t *regexp, kwset_t kws, pickaxe_fn fn) > @@ -22,7 +27,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o, > /* Showing the whole changeset if needle exists */ > for (i = 0; i < q->nr; i++) { > struct diff_filepair *p = q->queue[i]; > - if (fn(p, o, regexp, kws)) > + if (pickaxe_match(p, o, regexp, kws, fn)) > return; /* do not munge the queue */ > } > > @@ -37,7 +42,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o, > /* Showing only the filepairs that has the needle */ > for (i = 0; i < q->nr; i++) { > struct diff_filepair *p = q->queue[i]; > - if (fn(p, o, regexp, kws)) > + if (pickaxe_match(p, o, regexp, kws, fn)) > diff_q(&outq, p); > else > diff_free_filepair(p); > @@ -74,64 +79,33 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, > line[len] = hold; > } > > -static int diff_grep(struct diff_filepair *p, struct diff_options *o, > +static int diff_grep(mmfile_t *one, mmfile_t *two, > + struct diff_options *o, > regex_t *regexp, kwset_t kws) > { > regmatch_t regmatch; > - struct userdiff_driver *textconv_one = NULL; > - struct userdiff_driver *textconv_two = NULL; > - mmfile_t mf1, mf2; > - int hit; > + struct diffgrep_cb ecbdata; > + xpparam_t xpp; > + xdemitconf_t xecfg; > > - if (!o->pickaxe[0]) > - return 0; > + if (!one) > + return !regexec(regexp, two->ptr, 1, ®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,9 +172,20 @@ static int has_changes(struct diff_filepair *p, 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; > mmfile_t mf1, mf2; > @@ -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); ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G 2013-04-05 5:43 ` Junio C Hamano @ 2013-04-05 5:45 ` Jeff King 2013-04-05 16:44 ` Junio C Hamano 0 siblings, 1 reply; 27+ messages in thread From: Jeff King @ 2013-04-05 5:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Thu, Apr 04, 2013 at 10:43:14PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > I notice that you are stuck factoring out not just the setup, but also > > the cleanup, and I wondered if things could be made even simpler by just > > encapsulating the checking logic in a callback; then the setup and > > cleanup flow more naturally, as they are in a single function wrapper. > > > > Like this, which ends up saving 20 lines rather than adding 7: > > Oh, this is one of those many times I am reminded why I love having > you in the reviewer/contributor pool ;-) I didn't actually test that patch beyond compilation (but it's _obviously_ correct, right?), and I'm about to go to bed. Do you want to take care of adapting your commit message to it? -Peff ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G 2013-04-05 5:45 ` Jeff King @ 2013-04-05 16:44 ` Junio C Hamano 0 siblings, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2013-04-05 16:44 UTC (permalink / raw) To: Jeff King; +Cc: git Jeff King <peff@peff.net> writes: > I didn't actually test that patch beyond compilation (but it's > _obviously_ correct, right?), and I'm about to go to bed. Do you want to > take care of adapting your commit message to it? The code looks obviously correct, yes. We might have to revisit the "Is unmerged?" thing, Cf. d7c9bf22351e (diffcore-rename: don't consider unmerged path as source, 2011-03-23), but that is an unlikely corner case, especially for these options (-S/-G). -- >8 -- From: Jeff King <peff@peff.net> Date: Fri, 5 Apr 2013 01:28:10 -0400 Subject: [PATCH] diffcore-pickaxe: unify code for log -S/-G The logic flow of has_changes() used for "log -S" and diff_grep() used for "log -G" are essentially the same. See if we have both sides that could be different in any interesting way, slurp the contents in core, possibly after applying textconv, inspect the contents, clean-up and report the result. The only difference between the two is how "inspect" step works. Unify this codeflow in a helper, pickaxe_match(), which takes a callback function that implements the specific "inspect" step. After removing the common scaffolding code from the existing has_changes() and diff_grep(), they each becomes such a callback function suitable for passing to pickaxe_match(). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diffcore-pickaxe.c | 118 ++++++++++++++++++++++------------------------------- 1 file changed, 49 insertions(+), 69 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index cadb071..63722f8 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -8,7 +8,12 @@ #include "xdiff-interface.h" #include "kwset.h" -typedef int (*pickaxe_fn)(struct diff_filepair *p, struct diff_options *o, regex_t *regexp, kwset_t kws); +typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two, + struct diff_options *o, + regex_t *regexp, kwset_t kws); + +static int pickaxe_match(struct diff_filepair *p, struct diff_options *o, + regex_t *regexp, kwset_t kws, pickaxe_fn fn); static void pickaxe(struct diff_queue_struct *q, struct diff_options *o, regex_t *regexp, kwset_t kws, pickaxe_fn fn) @@ -22,7 +27,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o, /* Showing the whole changeset if needle exists */ for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - if (fn(p, o, regexp, kws)) + if (pickaxe_match(p, o, regexp, kws, fn)) return; /* do not munge the queue */ } @@ -37,7 +42,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o, /* Showing only the filepairs that has the needle */ for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - if (fn(p, o, regexp, kws)) + if (pickaxe_match(p, o, regexp, kws, fn)) diff_q(&outq, p); else diff_free_filepair(p); @@ -74,64 +79,33 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len) line[len] = hold; } -static int diff_grep(struct diff_filepair *p, struct diff_options *o, +static int diff_grep(mmfile_t *one, mmfile_t *two, + struct diff_options *o, regex_t *regexp, kwset_t kws) { regmatch_t regmatch; - struct userdiff_driver *textconv_one = NULL; - struct userdiff_driver *textconv_two = NULL; - mmfile_t mf1, mf2; - int hit; + struct diffgrep_cb ecbdata; + xpparam_t xpp; + xdemitconf_t xecfg; - if (!o->pickaxe[0]) - return 0; + if (!one) + return !regexec(regexp, two->ptr, 1, ®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,9 +172,20 @@ 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; mmfile_t mf1, mf2; @@ -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); -- 1.8.2-667-gbdcd2ef ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 3/3] diffcore-pickaxe: respect --no-textconv 2013-04-04 18:10 ` Junio C Hamano 2013-04-04 20:20 ` [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv() Simon Ruderich 2013-04-04 20:21 ` [PATCH v2 2/3] diffcore-pickaxe: remove fill_one() Simon Ruderich @ 2013-04-04 20:21 ` Simon Ruderich 2013-04-05 7:40 ` Matthieu Moy 2 siblings, 1 reply; 27+ messages in thread From: Simon Ruderich @ 2013-04-04 20:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Matthieu Moy, git 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 <Matthieu.Moy@grenoble-inp.fr> Signed-off-by: Simon Ruderich <simon@ruderich.org> --- diffcore-pickaxe.c | 12 ++++++++---- t/t4209-log-pickaxe.sh | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 3124f49..26ddf00 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 eed7273..953cec8 100755 --- a/t/t4209-log-pickaxe.sh +++ b/t/t4209-log-pickaxe.sh @@ -116,4 +116,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 -- 1.8.2 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/3] diffcore-pickaxe: respect --no-textconv 2013-04-04 20:21 ` [PATCH v2 3/3] diffcore-pickaxe: respect --no-textconv Simon Ruderich @ 2013-04-05 7:40 ` Matthieu Moy 2013-04-05 13:16 ` [PATCH v3 " Simon Ruderich 0 siblings, 1 reply; 27+ messages in thread From: Matthieu Moy @ 2013-04-05 7:40 UTC (permalink / raw) To: Simon Ruderich; +Cc: Junio C Hamano, Jeff King, git Simon Ruderich <simon@ruderich.org> writes: > --- a/t/t4209-log-pickaxe.sh > +++ b/t/t4209-log-pickaxe.sh > @@ -116,4 +116,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 > +' While we're there, we could test -G --no-textconv too. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 3/3] diffcore-pickaxe: respect --no-textconv 2013-04-05 7:40 ` Matthieu Moy @ 2013-04-05 13:16 ` Simon Ruderich 2013-04-05 17:31 ` Junio C Hamano 0 siblings, 1 reply; 27+ messages in thread From: Simon Ruderich @ 2013-04-05 13:16 UTC (permalink / raw) To: Matthieu Moy; +Cc: Junio C Hamano, Jeff King, git 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 <Matthieu.Moy@grenoble-inp.fr> Signed-off-by: Simon Ruderich <simon@ruderich.org> --- On Fri, Apr 05, 2013 at 09:40:21AM +0200, Matthieu Moy wrote: > While we're there, we could test -G --no-textconv too. Hello Matthieu, Good idea, I've added it. Regards Simon 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 3124f49..26ddf00 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 eed7273..38fb80f 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 -- 1.8.2 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3 3/3] diffcore-pickaxe: respect --no-textconv 2013-04-05 13:16 ` [PATCH v3 " Simon Ruderich @ 2013-04-05 17:31 ` Junio C Hamano 0 siblings, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2013-04-05 17:31 UTC (permalink / raw) To: Simon Ruderich; +Cc: Matthieu Moy, Jeff King, git Thanks; will replace the one in 'pu' with this. ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2013-04-06 16:52 UTC | newest] Thread overview: 27+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-04 8:34 [BUG] git log -S not respecting --no-textconv Matthieu Moy 2013-04-04 16:03 ` [PATCH 1/2] diffcore-pickaxe: respect --no-textconv Simon Ruderich 2013-04-04 17:03 ` Matthieu Moy 2013-04-04 17:43 ` Jeff King 2013-04-04 17:45 ` Junio C Hamano 2013-04-04 17:51 ` Jeff King 2013-04-04 18:10 ` Junio C Hamano 2013-04-04 20:20 ` [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv() Simon Ruderich 2013-04-04 20:48 ` Junio C Hamano 2013-04-04 21:10 ` Junio C Hamano 2013-04-04 21:11 ` Jeff King 2013-04-04 22:51 ` Junio C Hamano 2013-04-05 13:20 ` Simon Ruderich 2013-04-04 20:21 ` [PATCH v2 2/3] diffcore-pickaxe: remove fill_one() Simon Ruderich 2013-04-05 0:08 ` Jeff King 2013-04-05 4:43 ` Junio C Hamano 2013-04-05 4:45 ` [PATCH 1/3] diffcore-pickaxe: port optimization from has_changes() to diff_grep() Junio C Hamano 2013-04-05 4:45 ` [PATCH 2/3] diffcore-pickaxe: fix leaks in "log -S<block>" and "log -G<pattern>" Junio C Hamano 2013-04-05 4:45 ` [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G Junio C Hamano 2013-04-05 5:28 ` Jeff King 2013-04-05 5:43 ` Junio C Hamano 2013-04-05 5:45 ` Jeff King 2013-04-05 16:44 ` Junio C Hamano 2013-04-04 20:21 ` [PATCH v2 3/3] diffcore-pickaxe: respect --no-textconv Simon Ruderich 2013-04-05 7:40 ` Matthieu Moy 2013-04-05 13:16 ` [PATCH v3 " Simon Ruderich 2013-04-05 17:31 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).