RE: [PATCH] diff: respect --no-ext-diff with typechange

["Jakub Vrana" <jakub@vrana.cz>]

Yes, I was fixing the invalid (!pgm) condition, sorry for a non-precise description.

Does it mean that my patch is accepted or is there something else I need to do?

-- 
Jakub Vrana


-----Original Message-----
From: Jeff King [mailto:peff@peff.net] 
Sent: Monday, July 16, 2012 9:16 PM
To: Jakub Vrana
Cc: git@vger.kernel.org; gitster@pobox.com
Subject: Re: [PATCH] diff: respect --no-ext-diff with typechange

On Mon, Jul 16, 2012 at 05:27:00PM -0700, Jakub Vrana wrote:

> If external diff is specified through diff.external then it is used 
> even if `git diff --no-ext-diff` is used when there is a typechange.

Eek. That has some minor security implications, as it means that it is dangerous to run even plumbing inspection command in somebody else's repository.

However...

>  diff.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/diff.c b/diff.c
> index 208096f..898d610 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3074,6 +3074,9 @@ static void run_diff(struct diff_filepair *p, 
> struct diff_options *o)
>  	if (o->prefix_length)
>  		strip_prefix(o->prefix_length, &name, &other);
>  
> +	if (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))
> +		pgm = NULL;
> +
>  	if (DIFF_PAIR_UNMERGED(p)) {
>  		run_diff_cmd(pgm, name, NULL, attr_path,
>  			     NULL, NULL, NULL, o, p);

run_diff_cmd already checks the ALLOW_EXTERNAL bit and sets pgm to NULL there. So as far as I can tell, we are not actually running the external diff. However, there is still a problem. Later in run_diff we do:

        if (!pgm &&
            DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two) &&
            (S_IFMT & one->mode) != (S_IFMT & two->mode)) {
                /*
                 * a filepair that changes between file and symlink
                 * needs to be split into deletion and creation.
                 */
                struct diff_filespec *null = alloc_filespec(two->path);
                run_diff_cmd(NULL, name, other, attr_path,
                             one, null, &msg, o, p);
                free(null);
                strbuf_release(&msg);

                null = alloc_filespec(one->path);
                run_diff_cmd(NULL, name, other, attr_path,
                             null, two, &msg, o, p);
                free(null);
        }
        else
                run_diff_cmd(pgm, name, other, attr_path,
                             one, two, &msg, o, p);

IOW, we split up a typechange if we are feeding it to the internal diff generator, because builtin_diff will not show diffs between different types. But the check for "!pgm" here is not right; we don't know yet whether we will be builtin or external, because we have not checked ALLOW_EXTERNAL yet.

So I think your fix is the right thing, but the bug it is fixing is not "do not run external diff even when --no-ext-diff is specified". It is "do not accidentally feed typechange diffs to builtin_diff".

You can see the difference in output with this script (and it works fine with your patch applied):

    git init -q repo && cd repo &&
    echo content >file && git add file && git commit -q -m regular &&
    rm file && ln -s dest file && git commit -q -a -m typechange &&
    export GIT_PAGER=cat &&
    export GIT_EXTERNAL_DIFF='echo doing external diff' &&
    git show HEAD^ --format='=== %s, ext ===' --ext-diff &&
    git show HEAD^ --format='=== %s, no-ext ===' --no-ext-diff &&
    git show HEAD  --format='=== %s, ext ===' --ext-diff &&
    git show HEAD  --format='=== %s, no-ext ===' --no-ext-diff

-Peff