External tool is ignored when using difftool with --no-index

External tool is ignored when using difftool with --no-index

[Sylvain Lacaze]

Hi,

I have “p4merge” setup as diff.tool and merge.tool, and it work great
in normal operation (i.e., “p4merge” opens), e.g:

$: git difftool HEAD~3 somePath/someFile.m

However, when I use “—no-index” to compare 2 arbitrary file, difftool
just uses diff:

$: git difftool --no-index somePath/someFile.m somePath/someOtherFile.m
$: git difftool --no-index --tool=p4merge somePath/someFile.m
somePath/someOtherFile.m

Both the above command just yield normal diff.

I’m using the latest GIT build:

$: git version
git version 2.20.1.windows.1


Best,

Sylvain

[PATCH] diff: reuse diff setup for --no-index case

[Jeff King]

On Wed, Feb 13, 2019 at 06:50:19PM +0000, Sylvain Lacaze wrote:

> I have “p4merge” setup as diff.tool and merge.tool, and it work great
> in normal operation (i.e., “p4merge” opens), e.g:
> 
> $: git difftool HEAD~3 somePath/someFile.m
> 
> However, when I use “—no-index” to compare 2 arbitrary file, difftool
> just uses diff:
> 
> $: git difftool --no-index somePath/someFile.m somePath/someOtherFile.m
> $: git difftool --no-index --tool=p4merge somePath/someFile.m
> somePath/someOtherFile.m
> 
> Both the above command just yield normal diff.

The root of the problem is that "git diff --no-index" does not enable
external diff tools by default. You can probably make it work by passing
"--no-index --ext-diff".

It seems to me that "git diff --no-index" should generally behave the
same as a regular "git diff" when possible, though. Maybe we should do
something like the patch below?

-- >8 --
Subject: [PATCH] diff: reuse diff setup for --no-index case

When "--no-index" is in effect (or implied by the arguments), git-diff
jumps early to a special code path to perform that diff. This means we
miss out on some settings like enabling --ext-diff and --textconv by
default.

Let's jump to the no-index path _after_ we've done more setup on
rev.diffopt. Some of these options won't affect us either way (e.g.,
items related to the index), but that makes it less likely for these two
paths to diverge again in the future.

Note that we also need to stop re-initializing the diffopt struct in
diff_no_index(). This should not be necessary, as it will already have
been initialized by cmd_diff() (and there are no other callers). That in
turn lets us drop the "repository" argument from diff_no_index (which
never made much sense, since the whole point is that you don't need a
repository).

Signed-off-by: Jeff King <peff@peff.net>
---
Generated with --inter-hunk-context=13 so you can see the actual list of
options.

 builtin/diff.c           | 7 ++++---
 diff-no-index.c          | 8 +-------
 diff.h                   | 2 +-
 t/t4053-diff-no-index.sh | 8 ++++++++
 4 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 9f6109224b..458ce326c8 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -338,28 +338,29 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		       "--no-index" : "[--no-index]");
 
 	}
-	if (no_index)
-		/* If this is a no-index diff, just run it and exit there. */
-		diff_no_index(the_repository, &rev, argc, argv);
 
 	/* Otherwise, we are doing the usual "git" diff */
 	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
 
 	/* Scale to real terminal size and respect statGraphWidth config */
 	rev.diffopt.stat_width = -1;
 	rev.diffopt.stat_graph_width = -1;
 
 	/* Default to let external and textconv be used */
 	rev.diffopt.flags.allow_external = 1;
 	rev.diffopt.flags.allow_textconv = 1;
 
 	/*
 	 * Default to intent-to-add entries invisible in the
 	 * index. This makes them show up as new files in diff-files
 	 * and not at all in diff-cached.
 	 */
 	rev.diffopt.ita_invisible_in_index = 1;
 
+	if (no_index)
+		/* If this is a no-index diff, just run it and exit there. */
+		diff_no_index(&rev, argc, argv);
+
 	if (nongit)
 		die(_("Not a git repository"));
 	argc = setup_revisions(argc, argv, &rev, NULL);
diff --git a/diff-no-index.c b/diff-no-index.c
index 9414e922d1..6001baecd4 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -233,20 +233,14 @@ static void fixup_paths(const char **path, struct strbuf *replacement)
 	}
 }
 
-void diff_no_index(struct repository *r,
-		   struct rev_info *revs,
+void diff_no_index(struct rev_info *revs,
 		   int argc, const char **argv)
 {
 	int i;
 	const char *paths[2];
 	struct strbuf replacement = STRBUF_INIT;
 	const char *prefix = revs->prefix;
 
-	/*
-	 * FIXME: --no-index should not look at index and we should be
-	 * able to pass NULL repo. Maybe later.
-	 */
-	repo_diff_setup(r, &revs->diffopt);
 	for (i = 1; i < argc - 2; ) {
 		int j;
 		if (!strcmp(argv[i], "--no-index"))
diff --git a/diff.h b/diff.h
index b512d0477a..a01e03985a 100644
--- a/diff.h
+++ b/diff.h
@@ -435,7 +435,7 @@ int diff_flush_patch_id(struct diff_options *, struct object_id *, int);
 
 int diff_result_code(struct diff_options *, int);
 
-void diff_no_index(struct repository *, struct rev_info *, int, const char **);
+void diff_no_index(struct rev_info *, int, const char **);
 
 int index_differs_from(struct repository *r, const char *def,
 		       const struct diff_flags *flags,
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 6e0dd6f9e5..4331b3118a 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -137,4 +137,12 @@ test_expect_success 'diff --no-index from repo subdir with absolute paths' '
 	test_cmp expect actual
 '
 
+test_expect_success 'diff --no-index allows external diff' '
+	test_expect_code 1 \
+		env GIT_EXTERNAL_DIFF="echo external ;:" \
+		git diff --no-index non/git/a non/git/b >actual &&
+	echo external >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.21.0.rc0.586.gffba1126a0

Re: [PATCH] diff: reuse diff setup for --no-index case

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Subject: [PATCH] diff: reuse diff setup for --no-index case
>
> When "--no-index" is in effect (or implied by the arguments), git-diff
> jumps early to a special code path to perform that diff. This means we
> miss out on some settings like enabling --ext-diff and --textconv by
> default.
>
> Let's jump to the no-index path _after_ we've done more setup on
> rev.diffopt. Some of these options won't affect us either way (e.g.,
> items related to the index), but that makes it less likely for these two
> paths to diverge again in the future.

OK.

> Note that we also need to stop re-initializing the diffopt struct in
> diff_no_index(). This should not be necessary, as it will already have
> been initialized by cmd_diff() (and there are no other callers). That in
> turn lets us drop the "repository" argument from diff_no_index (which
> never made much sense, since the whole point is that you don't need a
> repository).

I really like this part of the change.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Generated with --inter-hunk-context=13 so you can see the actual list of
> options.
>
>  builtin/diff.c           | 7 ++++---
>  diff-no-index.c          | 8 +-------
>  diff.h                   | 2 +-
>  t/t4053-diff-no-index.sh | 8 ++++++++
>  4 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index 9f6109224b..458ce326c8 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -338,28 +338,29 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  		       "--no-index" : "[--no-index]");
>  
>  	}
> -	if (no_index)
> -		/* If this is a no-index diff, just run it and exit there. */
> -		diff_no_index(the_repository, &rev, argc, argv);
>  
>  	/* Otherwise, we are doing the usual "git" diff */

This "Otherwise, " can be replaced with "We've dealt with the
'--no-index' mode with the above.  In the remainder of the
function,".

>  	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;

This does not hurt but by definition is irrelevant in "--no-index" mode.

>  	/* Scale to real terminal size and respect statGraphWidth config */
>  	rev.diffopt.stat_width = -1;
>  	rev.diffopt.stat_graph_width = -1;
>  
>  	/* Default to let external and textconv be used */
>  	rev.diffopt.flags.allow_external = 1;
>  	rev.diffopt.flags.allow_textconv = 1;

These four do matter in "--no-index" mode.

>  
>  	/*
>  	 * Default to intent-to-add entries invisible in the
>  	 * index. This makes them show up as new files in diff-files
>  	 * and not at all in diff-cached.
>  	 */
>  	rev.diffopt.ita_invisible_in_index = 1;

This falls into the same category as skip_stat_unmatch.

> +	if (no_index)
> +		/* If this is a no-index diff, just run it and exit there. */
> +		diff_no_index(&rev, argc, argv);
> +
>  	if (nongit)
>  		die(_("Not a git repository"));
>  	argc = setup_revisions(argc, argv, &rev, NULL);

To summarize, I would suspect that two further improvements on this
patch are:

 (1) move "Otherwise" comment to the right place

 (2) make the two assignments that are irrelevant to "--no-index"
     after we jumped to diff_no_index().

The latter is optional, but may be good for code health by making
developers _think_ if an option is applicable to "--no-index" mode.
I dunno.

> diff --git a/diff-no-index.c b/diff-no-index.c
> index 9414e922d1..6001baecd4 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -233,20 +233,14 @@ static void fixup_paths(const char **path, struct strbuf *replacement)
>  	}
>  }
>  
> -void diff_no_index(struct repository *r,
> -		   struct rev_info *revs,
> +void diff_no_index(struct rev_info *revs,
>  		   int argc, const char **argv)
>  {
>  	int i;
>  	const char *paths[2];
>  	struct strbuf replacement = STRBUF_INIT;
>  	const char *prefix = revs->prefix;
>  
> -	/*
> -	 * FIXME: --no-index should not look at index and we should be
> -	 * able to pass NULL repo. Maybe later.
> -	 */
> -	repo_diff_setup(r, &revs->diffopt);

;-)

Re: [PATCH] diff: reuse diff setup for --no-index case

[Jeff King]

On Thu, Feb 14, 2019 at 11:47:02AM -0800, Junio C Hamano wrote:

> > +	if (no_index)
> > +		/* If this is a no-index diff, just run it and exit there. */
> > +		diff_no_index(&rev, argc, argv);
> > +
> >  	if (nongit)
> >  		die(_("Not a git repository"));
> >  	argc = setup_revisions(argc, argv, &rev, NULL);
> 
> To summarize, I would suspect that two further improvements on this
> patch are:
> 
>  (1) move "Otherwise" comment to the right place
> 
>  (2) make the two assignments that are irrelevant to "--no-index"
>      after we jumped to diff_no_index().
> 
> The latter is optional, but may be good for code health by making
> developers _think_ if an option is applicable to "--no-index" mode.
> I dunno.

Yeah, I very much agree with (1). For (2), I intentionally left it as
one mixed block, because I didn't want people to accidentally add new
setup lines to the wrong block. But maybe with sufficient comments, it
would be clear (and even make the code flow a bit more obvious).

Here's an attempt at that.  I did drop a few comments that seemed
pointless to me, as they're just re-stating the lines they describe.

-- >8 --
Subject: [PATCH] diff: reuse diff setup for --no-index case

When "--no-index" is in effect (or implied by the arguments), git-diff
jumps early to a special code path to perform that diff. This means we
miss out on some settings like enabling --ext-diff and --textconv by
default.

Let's jump to the no-index path _after_ we've done more setup on
rev.diffopt. Since some of the options don't affect us (e.g., items
related to the index), let's re-order the setup into two blocks (see the
in-code comments).

Note that we also need to stop re-initializing the diffopt struct in
diff_no_index(). This should not be necessary, as it will already have
been initialized by cmd_diff() (and there are no other callers). That in
turn lets us drop the "repository" argument from diff_no_index (which
never made much sense, since the whole point is that you don't need a
repository).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/diff.c           | 20 +++++++++++---------
 diff-no-index.c          |  8 +-------
 diff.h                   |  2 +-
 t/t4053-diff-no-index.sh |  8 ++++++++
 4 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 9f6109224b..53d4234ff4 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -338,21 +338,23 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		       "--no-index" : "[--no-index]");
 
 	}
-	if (no_index)
-		/* If this is a no-index diff, just run it and exit there. */
-		diff_no_index(the_repository, &rev, argc, argv);
-
-	/* Otherwise, we are doing the usual "git" diff */
-	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
 
-	/* Scale to real terminal size and respect statGraphWidth config */
+	/* Set up defaults that will apply to both no-index and regular diffs. */
 	rev.diffopt.stat_width = -1;
 	rev.diffopt.stat_graph_width = -1;
-
-	/* Default to let external and textconv be used */
 	rev.diffopt.flags.allow_external = 1;
 	rev.diffopt.flags.allow_textconv = 1;
 
+	/* If this is a no-index diff, just run it and exit there. */
+	if (no_index)
+		diff_no_index(&rev, argc, argv);
+
+	/*
+	 * Otherwise, we are doing the usual "git" diff; set up any
+	 * further defaults that apply to regular diffs.
+	 */
+	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
+
 	/*
 	 * Default to intent-to-add entries invisible in the
 	 * index. This makes them show up as new files in diff-files
diff --git a/diff-no-index.c b/diff-no-index.c
index 9414e922d1..6001baecd4 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -233,8 +233,7 @@ static void fixup_paths(const char **path, struct strbuf *replacement)
 	}
 }
 
-void diff_no_index(struct repository *r,
-		   struct rev_info *revs,
+void diff_no_index(struct rev_info *revs,
 		   int argc, const char **argv)
 {
 	int i;
@@ -242,11 +241,6 @@ void diff_no_index(struct repository *r,
 	struct strbuf replacement = STRBUF_INIT;
 	const char *prefix = revs->prefix;
 
-	/*
-	 * FIXME: --no-index should not look at index and we should be
-	 * able to pass NULL repo. Maybe later.
-	 */
-	repo_diff_setup(r, &revs->diffopt);
 	for (i = 1; i < argc - 2; ) {
 		int j;
 		if (!strcmp(argv[i], "--no-index"))
diff --git a/diff.h b/diff.h
index b512d0477a..a01e03985a 100644
--- a/diff.h
+++ b/diff.h
@@ -435,7 +435,7 @@ int diff_flush_patch_id(struct diff_options *, struct object_id *, int);
 
 int diff_result_code(struct diff_options *, int);
 
-void diff_no_index(struct repository *, struct rev_info *, int, const char **);
+void diff_no_index(struct rev_info *, int, const char **);
 
 int index_differs_from(struct repository *r, const char *def,
 		       const struct diff_flags *flags,
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 6e0dd6f9e5..4331b3118a 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -137,4 +137,12 @@ test_expect_success 'diff --no-index from repo subdir with absolute paths' '
 	test_cmp expect actual
 '
 
+test_expect_success 'diff --no-index allows external diff' '
+	test_expect_code 1 \
+		env GIT_EXTERNAL_DIFF="echo external ;:" \
+		git diff --no-index non/git/a non/git/b >actual &&
+	echo external >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.21.0.rc1.581.g7e4aa7bafd

Re: [PATCH] diff: reuse diff setup for --no-index case

[Jeff King]

On Sat, Feb 16, 2019 at 01:57:56AM -0500, Jeff King wrote:

> On Thu, Feb 14, 2019 at 11:47:02AM -0800, Junio C Hamano wrote:
> 
> > > +	if (no_index)
> > > +		/* If this is a no-index diff, just run it and exit there. */
> > > +		diff_no_index(&rev, argc, argv);
> > > +
> > >  	if (nongit)
> > >  		die(_("Not a git repository"));
> > >  	argc = setup_revisions(argc, argv, &rev, NULL);
> > 
> > To summarize, I would suspect that two further improvements on this
> > patch are:
> > 
> >  (1) move "Otherwise" comment to the right place
> > 
> >  (2) make the two assignments that are irrelevant to "--no-index"
> >      after we jumped to diff_no_index().
> > 
> > The latter is optional, but may be good for code health by making
> > developers _think_ if an option is applicable to "--no-index" mode.
> > I dunno.
> 
> Yeah, I very much agree with (1). For (2), I intentionally left it as
> one mixed block, because I didn't want people to accidentally add new
> setup lines to the wrong block. But maybe with sufficient comments, it
> would be clear (and even make the code flow a bit more obvious).
> 
> Here's an attempt at that.  I did drop a few comments that seemed
> pointless to me, as they're just re-stating the lines they describe.

It looks like the original got merged to next. I think we at least need
to deal with the "Otherwise..." comment, but I think the layout I posted
in my followup is nicer overall. Was it a mistake to merge the first
one?

If so, do you want an incremental, or do you just want to drop it and
redo as part of the post-2.21 rewind?

-Peff

Re: [PATCH] diff: reuse diff setup for --no-index case

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Here's an attempt at that.  I did drop a few comments that seemed
>> pointless to me, as they're just re-stating the lines they describe.
>
> It looks like the original got merged to next. I think we at least need
> to deal with the "Otherwise..." comment, but I think the layout I posted
> in my followup is nicer overall. Was it a mistake to merge the first
> one?
>
> If so, do you want an incremental, or do you just want to drop it and
> redo as part of the post-2.21 rewind?

Will do a revert-and-replace just like I did for a few others I
screwed up recently.

Thanks.