Re: [PATCH 1/2] diffcore-pickaxe: respect --no-textconv

From: Jeff King <peff@peff.net>
Date: Apr 4, 2013, 17:43 UTC
Message-ID: <20130404174301.GA13005@sigill.intra.peff.net>
In-Reply-To: <20130404160359.GA25232@ruderich.org>

On Thu, Apr 04, 2013 at 06:03:59PM +0200, Simon Ruderich wrote:

Show 9 quoted lines

> 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.

Show 9 quoted lines

> > 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...

Show 10 quoted lines

> 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