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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html