patchdiff: respect --no-ext-diff with typechange

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.

Signed-off-by: Jakub Vrana <jakub@vrana.cz>
---
 diff.c |    3 +++
 1 file changed, 3 insertions(+)

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

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

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

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" <jakub@vrana.cz> writes:

The impression I got from Peff's review was that the problem description in the proposed commit log message did not describe the reality at all, and the added three lines did not do what the message implied they do. So I do not see how it can be acceptable by anybody.

It also needs a test to protect this fix from being broken by other people in the future.

I've followed the codepath myself, and here is what I would have liked the submitted patch to look like. Note that run_diff_cmd() no longer needs to reset pgm to NULL based on ALLOW_EXTERNAL, but it still needs to look at it to decide if per-path external userdiff needs to be called.

-- >8 --
Subject: diff: correctly disable external_diff with --no-ext-diff

Upon seeing a type-change filepair, "diff --no-ext-diff" does not show the usual "deletion followed by addition" split patch and does not run the external diff driver either.

This is because the logic to disable external diff was placed at a wrong level in the callchain. run_diff_cmd() decides to show the split patch only when external diff driver is not configured or specified via GIT_EXTERNAL_DIFF environment, but this is done before checking if --no-ext-diff was given. To make things worse, run_diff_cmd() checks --no-ext-diff and disables the output for such a filepair completely, as the callchain below it (e.g. builtin_diff) does not want to handle typechange filepairs.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * The use of userdiff_find_by_path() in run_diff_cmd() may be iffy;
   it is probably OK to override diff.external with a more specific
   per-path configuration, but I think an external diff specified by
   the GIT_EXTERNAL_DIFF environment may want to trump the
   configured per-path one, as an environment is a stronger one-shot
   request.

   But this patch is not about changing that semantics, so I left it
   as-is. 

 diff.c                   |  8 +++++---
 t/t4020-diff-external.sh | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

On Tue, Jul 17, 2012 at 10:08:59PM -0700, Junio C Hamano wrote:

Yeah, exactly.

Your patch looks good to me.

I think this date all the way back to f1af60b (Support 'diff=pgm' attribute, 2007-04-22). There's a tradeoff here; usually environment variables trump config, but you end up using a large hammer (here is how to diff _all_ files externally) to hit a small nail (here is how to diff _just_ this file). I suspect it isn't that big a problem in practice because people tend to use either one mechanism or the other.

The most sensible thing to me is probably $GIT_EXTERNAL_DIFF, followed by attributes, followed by diff.external. That uses the more specific diff pulled from the on-disk config, but allows you to do one-shot overrides with the environment as long as you are careful to restrict your command (e.g., "GIT_EXTERNAL_DIFF=foo-differ git diff -- file.foo").

Sounds sensible.

-Peff

On Wed, Jul 18, 2012 at 02:23:29AM -0400, Jeff King wrote:

Here's a patch implementing that. This is definitely how I would have done it if I were starting from scratch. My only hesitation now is that I don't care too deeply either way, and it is technically a behavior change. So there is a chance of regression for something that nobody has actually complained about. To be honest, I doubt many people are using external diff at all these days; textconv is closer to what most people want, and is much easier to use.

-- >8 --
Subject: [PATCH] diff: fix precedence of external diff options

There are three ways to specify an external diff command: GIT_EXTERNAL_DIFF in the environment, diff.external in the config, or a "diff" gitattribute. The current order of precedence is:

  1. gitattribute

  2. GIT_EXTERNAL_DIFF

  3. diff.external

But usually our rule is that environment variables should take precedence over on-disk config (i.e., option 2 should come before option 1). This situation is trickier than some, because option 1 is more specific to the individual file than option 2 (which affects all files), so it might be preferable. So the current behavior can be seen as implementing "do the specific thing if we can, but fall back to this general thing".

However, since you can already implement that behavior (by combining attributes with a diff.external setting), and because environment variables can be useful for one-shot overrides (e.g., "GIT_EXTERNAL_DIFF=foo git diff -- bar" to override bar's gitattribute setting), let's switch the precedence of options 1 and 2 above.

While we're adding tests to t4020 for the precedence, let's also make sure that diff.external works at all by running it through the same tests as GIT_EXTERNAL_DIFF.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c                   | 35 +++++++++++++++--------------------
 t/t4020-diff-external.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 20 deletions(-)

Jeff King <peff@peff.net> writes:

Thanks. I share the same feeling, but the code after the patch looks much nicer, which is somewhat sad.

It depends on the payload and the output you want. I wouldn't think it is fair to challenge anybody to come up with a solution based on the textconv machinery to replicate what the compare-cooking.perl in the 'todo' branch (used as an external diff driver for whats-cooking.txt) does.

On Wed, Jul 18, 2012 at 03:47:21PM -0700, Junio C Hamano wrote:

If we're not going to do it, perhaps we should at least include the tests I wrote (modified for the current behavior), since there was no coverage in this area previously. Patch is below.

Oh, absolutely. The compare-cooking script is a great example of what you can do with the flexibility that external diff offers. But based on my experience and reading the list, the much more common request is to produce a meaningful diff of two binary word processor documents. :)

Googling around, the other common use seems to be to stuff the output into a visual diff tool. Though I think that "git difftool" is a better approach for that.

-Peff

-- >8 --
Subject: [PATCH] diff: test precedence of external diff drivers

There are three ways to specify an external diff command: GIT_EXTERNAL_DIFF in the environment, diff.external in the config, or a "diff" gitattribute. The current order of precedence is:

  1. gitattribute

  2. GIT_EXTERNAL_DIFF

  3. diff.external

Usually our rule is that environment variables should take precedence over on-disk config (i.e., option 2 should come before option 1). However, this situation is trickier than some, because option 1 is more specific to the individual file than option 2 (which affects all files), so it might be preferable. So the current behavior can be seen as implementing "do the specific thing if we can, but fall back to this general thing".

This is probably not what we would do if we were writing git from scratch, but it has been this way for several years, and is not worth changing. So let's at least document that this is the way it's supposed to work with a test.

While we're there, let's also make sure that diff.external (which was not previously tested at all) works by running it through the same tests as GIT_EXTERNAL_DIFF.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t4020-diff-external.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)