* External tool is ignored when using difftool with --no-index @ 2019-02-13 18:50 Sylvain Lacaze 2019-02-13 20:11 ` [PATCH] diff: reuse diff setup for --no-index case Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Sylvain Lacaze @ 2019-02-13 18:50 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] diff: reuse diff setup for --no-index case 2019-02-13 18:50 External tool is ignored when using difftool with --no-index Sylvain Lacaze @ 2019-02-13 20:11 ` Jeff King 2019-02-14 19:47 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2019-02-13 20:11 UTC (permalink / raw) To: Sylvain Lacaze; +Cc: git 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] diff: reuse diff setup for --no-index case 2019-02-13 20:11 ` [PATCH] diff: reuse diff setup for --no-index case Jeff King @ 2019-02-14 19:47 ` Junio C Hamano 2019-02-16 6:57 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2019-02-14 19:47 UTC (permalink / raw) To: Jeff King; +Cc: Sylvain Lacaze, git 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); ;-) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] diff: reuse diff setup for --no-index case 2019-02-14 19:47 ` Junio C Hamano @ 2019-02-16 6:57 ` Jeff King 2019-02-24 9:45 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2019-02-16 6:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sylvain Lacaze, git 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] diff: reuse diff setup for --no-index case 2019-02-16 6:57 ` Jeff King @ 2019-02-24 9:45 ` Jeff King 2019-02-24 15:24 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2019-02-24 9:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sylvain Lacaze, git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] diff: reuse diff setup for --no-index case 2019-02-24 9:45 ` Jeff King @ 2019-02-24 15:24 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2019-02-24 15:24 UTC (permalink / raw) To: Jeff King; +Cc: Sylvain Lacaze, git 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-24 15:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-13 18:50 External tool is ignored when using difftool with --no-index Sylvain Lacaze 2019-02-13 20:11 ` [PATCH] diff: reuse diff setup for --no-index case Jeff King 2019-02-14 19:47 ` Junio C Hamano 2019-02-16 6:57 ` Jeff King 2019-02-24 9:45 ` Jeff King 2019-02-24 15:24 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.