Re: [PATCH 1/2] diffcore-pickaxe: respect --no-textconv
From: Jeff King <hidden>
Date: 2016-06-15 22:56:41
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 <redacted>
Reported-by: Matthieu Moy <redacted>Usually these pseudo-headers are meant to be chronological, so you would swap the order.
quoted
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...
quoted hunk ↗ jump to hunk
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