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 <[email protected]>
> Reported-by: Matthieu Moy <[email protected]>
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
