{"thread":{"id":"50495","subject":"External tool is ignored when using difftool with --no-index","startedAt":"2019-02-13T18:50:35Z","lastAt":"2019-02-24T15:24:33Z","messageCount":6,"participants":["Sylvain Lacaze","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"369246","messageId":"CAH1FtBpcZJ8V4B=RJ0SfC+kehpmpaBGafDzXvi8i33rc++jOUw@mail.gmail.com","threadId":"50495","inReplyTo":null,"subject":"External tool is ignored when using difftool with --no-index","fromName":"Sylvain Lacaze","fromEmail":"sylvain@lacaze.me","sentAt":"2019-02-13T18:50:19Z","receivedAt":"2019-02-13T18:50:35Z","isPatch":false,"sender":{"key":"sylvain@lacaze.me","avatar":null},"body":"Hi,\n\nI have “p4merge” setup as diff.tool and merge.tool, and it work great\nin normal operation (i.e., “p4merge” opens), e.g:\n\n$: git difftool HEAD~3 somePath/someFile.m\n\nHowever, when I use “—no-index” to compare 2 arbitrary file, difftool\njust uses diff:\n\n$: git difftool --no-index somePath/someFile.m somePath/someOtherFile.m\n$: git difftool --no-index --tool=p4merge somePath/someFile.m\nsomePath/someOtherFile.m\n\nBoth the above command just yield normal diff.\n\nI’m using the latest GIT build:\n\n$: git version\ngit version 2.20.1.windows.1\n\n\nBest,\n\nSylvain\n"},{"id":"369250","messageId":"20190213201118.GA13261@sigill.intra.peff.net","threadId":"50495","inReplyTo":"CAH1FtBpcZJ8V4B=RJ0SfC+kehpmpaBGafDzXvi8i33rc++jOUw@mail.gmail.com","subject":"[PATCH] diff: reuse diff setup for --no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-13T20:11:19Z","receivedAt":"2019-02-13T20:11:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 13, 2019 at 06:50:19PM +0000, Sylvain Lacaze wrote:\n\n> I have “p4merge” setup as diff.tool and merge.tool, and it work great\n> in normal operation (i.e., “p4merge” opens), e.g:\n> \n> $: git difftool HEAD~3 somePath/someFile.m\n> \n> However, when I use “—no-index” to compare 2 arbitrary file, difftool\n> just uses diff:\n> \n> $: git difftool --no-index somePath/someFile.m somePath/someOtherFile.m\n> $: git difftool --no-index --tool=p4merge somePath/someFile.m\n> somePath/someOtherFile.m\n> \n> Both the above command just yield normal diff.\n\nThe root of the problem is that \"git diff --no-index\" does not enable\nexternal diff tools by default. You can probably make it work by passing\n\"--no-index --ext-diff\".\n\nIt seems to me that \"git diff --no-index\" should generally behave the\nsame as a regular \"git diff\" when possible, though. Maybe we should do\nsomething like the patch below?\n\n-- >8 --\nSubject: [PATCH] diff: reuse diff setup for --no-index case\n\nWhen \"--no-index\" is in effect (or implied by the arguments), git-diff\njumps early to a special code path to perform that diff. This means we\nmiss out on some settings like enabling --ext-diff and --textconv by\ndefault.\n\nLet's jump to the no-index path _after_ we've done more setup on\nrev.diffopt. Some of these options won't affect us either way (e.g.,\nitems related to the index), but that makes it less likely for these two\npaths to diverge again in the future.\n\nNote that we also need to stop re-initializing the diffopt struct in\ndiff_no_index(). This should not be necessary, as it will already have\nbeen initialized by cmd_diff() (and there are no other callers). That in\nturn lets us drop the \"repository\" argument from diff_no_index (which\nnever made much sense, since the whole point is that you don't need a\nrepository).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nGenerated with --inter-hunk-context=13 so you can see the actual list of\noptions.\n\n builtin/diff.c           | 7 ++++---\n diff-no-index.c          | 8 +-------\n diff.h                   | 2 +-\n t/t4053-diff-no-index.sh | 8 ++++++++\n 4 files changed, 14 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 9f6109224b..458ce326c8 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -338,28 +338,29 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t       \"--no-index\" : \"[--no-index]\");\n \n \t}\n-\tif (no_index)\n-\t\t/* If this is a no-index diff, just run it and exit there. */\n-\t\tdiff_no_index(the_repository, &rev, argc, argv);\n \n \t/* Otherwise, we are doing the usual \"git\" diff */\n \trev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n \n \t/* Scale to real terminal size and respect statGraphWidth config */\n \trev.diffopt.stat_width = -1;\n \trev.diffopt.stat_graph_width = -1;\n \n \t/* Default to let external and textconv be used */\n \trev.diffopt.flags.allow_external = 1;\n \trev.diffopt.flags.allow_textconv = 1;\n \n \t/*\n \t * Default to intent-to-add entries invisible in the\n \t * index. This makes them show up as new files in diff-files\n \t * and not at all in diff-cached.\n \t */\n \trev.diffopt.ita_invisible_in_index = 1;\n \n+\tif (no_index)\n+\t\t/* If this is a no-index diff, just run it and exit there. */\n+\t\tdiff_no_index(&rev, argc, argv);\n+\n \tif (nongit)\n \t\tdie(_(\"Not a git repository\"));\n \targc = setup_revisions(argc, argv, &rev, NULL);\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 9414e922d1..6001baecd4 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -233,20 +233,14 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n \t}\n }\n \n-void diff_no_index(struct repository *r,\n-\t\t   struct rev_info *revs,\n+void diff_no_index(struct rev_info *revs,\n \t\t   int argc, const char **argv)\n {\n \tint i;\n \tconst char *paths[2];\n \tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n \n-\t/*\n-\t * FIXME: --no-index should not look at index and we should be\n-\t * able to pass NULL repo. Maybe later.\n-\t */\n-\trepo_diff_setup(r, &revs->diffopt);\n \tfor (i = 1; i < argc - 2; ) {\n \t\tint j;\n \t\tif (!strcmp(argv[i], \"--no-index\"))\ndiff --git a/diff.h b/diff.h\nindex b512d0477a..a01e03985a 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -435,7 +435,7 @@ int diff_flush_patch_id(struct diff_options *, struct object_id *, int);\n \n int diff_result_code(struct diff_options *, int);\n \n-void diff_no_index(struct repository *, struct rev_info *, int, const char **);\n+void diff_no_index(struct rev_info *, int, const char **);\n \n int index_differs_from(struct repository *r, const char *def,\n \t\t       const struct diff_flags *flags,\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 6e0dd6f9e5..4331b3118a 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -137,4 +137,12 @@ test_expect_success 'diff --no-index from repo subdir with absolute paths' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'diff --no-index allows external diff' '\n+\ttest_expect_code 1 \\\n+\t\tenv GIT_EXTERNAL_DIFF=\"echo external ;:\" \\\n+\t\tgit diff --no-index non/git/a non/git/b >actual &&\n+\techo external >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.21.0.rc0.586.gffba1126a0\n\n"},{"id":"369331","messageId":"xmqqo97e885l.fsf@gitster-ct.c.googlers.com","threadId":"50495","inReplyTo":"20190213201118.GA13261@sigill.intra.peff.net","subject":"Re: [PATCH] diff: reuse diff setup for --no-index case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-14T19:47:02Z","receivedAt":"2019-02-14T19:47:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: [PATCH] diff: reuse diff setup for --no-index case\n>\n> When \"--no-index\" is in effect (or implied by the arguments), git-diff\n> jumps early to a special code path to perform that diff. This means we\n> miss out on some settings like enabling --ext-diff and --textconv by\n> default.\n>\n> Let's jump to the no-index path _after_ we've done more setup on\n> rev.diffopt. Some of these options won't affect us either way (e.g.,\n> items related to the index), but that makes it less likely for these two\n> paths to diverge again in the future.\n\nOK.\n\n> Note that we also need to stop re-initializing the diffopt struct in\n> diff_no_index(). This should not be necessary, as it will already have\n> been initialized by cmd_diff() (and there are no other callers). That in\n> turn lets us drop the \"repository\" argument from diff_no_index (which\n> never made much sense, since the whole point is that you don't need a\n> repository).\n\nI really like this part of the change.\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Generated with --inter-hunk-context=13 so you can see the actual list of\n> options.\n>\n>  builtin/diff.c           | 7 ++++---\n>  diff-no-index.c          | 8 +-------\n>  diff.h                   | 2 +-\n>  t/t4053-diff-no-index.sh | 8 ++++++++\n>  4 files changed, 14 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index 9f6109224b..458ce326c8 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -338,28 +338,29 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>  \t\t       \"--no-index\" : \"[--no-index]\");\n>  \n>  \t}\n> -\tif (no_index)\n> -\t\t/* If this is a no-index diff, just run it and exit there. */\n> -\t\tdiff_no_index(the_repository, &rev, argc, argv);\n>  \n>  \t/* Otherwise, we are doing the usual \"git\" diff */\n\nThis \"Otherwise, \" can be replaced with \"We've dealt with the\n'--no-index' mode with the above.  In the remainder of the\nfunction,\".\n\n>  \trev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n\nThis does not hurt but by definition is irrelevant in \"--no-index\" mode.\n\n>  \t/* Scale to real terminal size and respect statGraphWidth config */\n>  \trev.diffopt.stat_width = -1;\n>  \trev.diffopt.stat_graph_width = -1;\n>  \n>  \t/* Default to let external and textconv be used */\n>  \trev.diffopt.flags.allow_external = 1;\n>  \trev.diffopt.flags.allow_textconv = 1;\n\nThese four do matter in \"--no-index\" mode.\n\n>  \n>  \t/*\n>  \t * Default to intent-to-add entries invisible in the\n>  \t * index. This makes them show up as new files in diff-files\n>  \t * and not at all in diff-cached.\n>  \t */\n>  \trev.diffopt.ita_invisible_in_index = 1;\n\nThis falls into the same category as skip_stat_unmatch.\n\n> +\tif (no_index)\n> +\t\t/* If this is a no-index diff, just run it and exit there. */\n> +\t\tdiff_no_index(&rev, argc, argv);\n> +\n>  \tif (nongit)\n>  \t\tdie(_(\"Not a git repository\"));\n>  \targc = setup_revisions(argc, argv, &rev, NULL);\n\nTo summarize, I would suspect that two further improvements on this\npatch are:\n\n (1) move \"Otherwise\" comment to the right place\n\n (2) make the two assignments that are irrelevant to \"--no-index\"\n     after we jumped to diff_no_index().\n\nThe latter is optional, but may be good for code health by making\ndevelopers _think_ if an option is applicable to \"--no-index\" mode.\nI dunno.\n\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 9414e922d1..6001baecd4 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -233,20 +233,14 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n>  \t}\n>  }\n>  \n> -void diff_no_index(struct repository *r,\n> -\t\t   struct rev_info *revs,\n> +void diff_no_index(struct rev_info *revs,\n>  \t\t   int argc, const char **argv)\n>  {\n>  \tint i;\n>  \tconst char *paths[2];\n>  \tstruct strbuf replacement = STRBUF_INIT;\n>  \tconst char *prefix = revs->prefix;\n>  \n> -\t/*\n> -\t * FIXME: --no-index should not look at index and we should be\n> -\t * able to pass NULL repo. Maybe later.\n> -\t */\n> -\trepo_diff_setup(r, &revs->diffopt);\n\n;-)\n\n"},{"id":"369432","messageId":"20190216065755.GB27641@sigill.intra.peff.net","threadId":"50495","inReplyTo":"xmqqo97e885l.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] diff: reuse diff setup for --no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-16T06:57:56Z","receivedAt":"2019-02-16T06:58:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 14, 2019 at 11:47:02AM -0800, Junio C Hamano wrote:\n\n> > +\tif (no_index)\n> > +\t\t/* If this is a no-index diff, just run it and exit there. */\n> > +\t\tdiff_no_index(&rev, argc, argv);\n> > +\n> >  \tif (nongit)\n> >  \t\tdie(_(\"Not a git repository\"));\n> >  \targc = setup_revisions(argc, argv, &rev, NULL);\n> \n> To summarize, I would suspect that two further improvements on this\n> patch are:\n> \n>  (1) move \"Otherwise\" comment to the right place\n> \n>  (2) make the two assignments that are irrelevant to \"--no-index\"\n>      after we jumped to diff_no_index().\n> \n> The latter is optional, but may be good for code health by making\n> developers _think_ if an option is applicable to \"--no-index\" mode.\n> I dunno.\n\nYeah, I very much agree with (1). For (2), I intentionally left it as\none mixed block, because I didn't want people to accidentally add new\nsetup lines to the wrong block. But maybe with sufficient comments, it\nwould be clear (and even make the code flow a bit more obvious).\n\nHere's an attempt at that.  I did drop a few comments that seemed\npointless to me, as they're just re-stating the lines they describe.\n\n-- >8 --\nSubject: [PATCH] diff: reuse diff setup for --no-index case\n\nWhen \"--no-index\" is in effect (or implied by the arguments), git-diff\njumps early to a special code path to perform that diff. This means we\nmiss out on some settings like enabling --ext-diff and --textconv by\ndefault.\n\nLet's jump to the no-index path _after_ we've done more setup on\nrev.diffopt. Since some of the options don't affect us (e.g., items\nrelated to the index), let's re-order the setup into two blocks (see the\nin-code comments).\n\nNote that we also need to stop re-initializing the diffopt struct in\ndiff_no_index(). This should not be necessary, as it will already have\nbeen initialized by cmd_diff() (and there are no other callers). That in\nturn lets us drop the \"repository\" argument from diff_no_index (which\nnever made much sense, since the whole point is that you don't need a\nrepository).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/diff.c           | 20 +++++++++++---------\n diff-no-index.c          |  8 +-------\n diff.h                   |  2 +-\n t/t4053-diff-no-index.sh |  8 ++++++++\n 4 files changed, 21 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 9f6109224b..53d4234ff4 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -338,21 +338,23 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t       \"--no-index\" : \"[--no-index]\");\n \n \t}\n-\tif (no_index)\n-\t\t/* If this is a no-index diff, just run it and exit there. */\n-\t\tdiff_no_index(the_repository, &rev, argc, argv);\n-\n-\t/* Otherwise, we are doing the usual \"git\" diff */\n-\trev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n \n-\t/* Scale to real terminal size and respect statGraphWidth config */\n+\t/* Set up defaults that will apply to both no-index and regular diffs. */\n \trev.diffopt.stat_width = -1;\n \trev.diffopt.stat_graph_width = -1;\n-\n-\t/* Default to let external and textconv be used */\n \trev.diffopt.flags.allow_external = 1;\n \trev.diffopt.flags.allow_textconv = 1;\n \n+\t/* If this is a no-index diff, just run it and exit there. */\n+\tif (no_index)\n+\t\tdiff_no_index(&rev, argc, argv);\n+\n+\t/*\n+\t * Otherwise, we are doing the usual \"git\" diff; set up any\n+\t * further defaults that apply to regular diffs.\n+\t */\n+\trev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n+\n \t/*\n \t * Default to intent-to-add entries invisible in the\n \t * index. This makes them show up as new files in diff-files\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 9414e922d1..6001baecd4 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -233,8 +233,7 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n \t}\n }\n \n-void diff_no_index(struct repository *r,\n-\t\t   struct rev_info *revs,\n+void diff_no_index(struct rev_info *revs,\n \t\t   int argc, const char **argv)\n {\n \tint i;\n@@ -242,11 +241,6 @@ void diff_no_index(struct repository *r,\n \tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n \n-\t/*\n-\t * FIXME: --no-index should not look at index and we should be\n-\t * able to pass NULL repo. Maybe later.\n-\t */\n-\trepo_diff_setup(r, &revs->diffopt);\n \tfor (i = 1; i < argc - 2; ) {\n \t\tint j;\n \t\tif (!strcmp(argv[i], \"--no-index\"))\ndiff --git a/diff.h b/diff.h\nindex b512d0477a..a01e03985a 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -435,7 +435,7 @@ int diff_flush_patch_id(struct diff_options *, struct object_id *, int);\n \n int diff_result_code(struct diff_options *, int);\n \n-void diff_no_index(struct repository *, struct rev_info *, int, const char **);\n+void diff_no_index(struct rev_info *, int, const char **);\n \n int index_differs_from(struct repository *r, const char *def,\n \t\t       const struct diff_flags *flags,\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 6e0dd6f9e5..4331b3118a 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -137,4 +137,12 @@ test_expect_success 'diff --no-index from repo subdir with absolute paths' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'diff --no-index allows external diff' '\n+\ttest_expect_code 1 \\\n+\t\tenv GIT_EXTERNAL_DIFF=\"echo external ;:\" \\\n+\t\tgit diff --no-index non/git/a non/git/b >actual &&\n+\techo external >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.21.0.rc1.581.g7e4aa7bafd\n\n"},{"id":"370056","messageId":"20190224094545.GA11538@sigill.intra.peff.net","threadId":"50495","inReplyTo":"20190216065755.GB27641@sigill.intra.peff.net","subject":"Re: [PATCH] diff: reuse diff setup for --no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-24T09:45:46Z","receivedAt":"2019-02-24T09:45:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 16, 2019 at 01:57:56AM -0500, Jeff King wrote:\n\n> On Thu, Feb 14, 2019 at 11:47:02AM -0800, Junio C Hamano wrote:\n> \n> > > +\tif (no_index)\n> > > +\t\t/* If this is a no-index diff, just run it and exit there. */\n> > > +\t\tdiff_no_index(&rev, argc, argv);\n> > > +\n> > >  \tif (nongit)\n> > >  \t\tdie(_(\"Not a git repository\"));\n> > >  \targc = setup_revisions(argc, argv, &rev, NULL);\n> > \n> > To summarize, I would suspect that two further improvements on this\n> > patch are:\n> > \n> >  (1) move \"Otherwise\" comment to the right place\n> > \n> >  (2) make the two assignments that are irrelevant to \"--no-index\"\n> >      after we jumped to diff_no_index().\n> > \n> > The latter is optional, but may be good for code health by making\n> > developers _think_ if an option is applicable to \"--no-index\" mode.\n> > I dunno.\n> \n> Yeah, I very much agree with (1). For (2), I intentionally left it as\n> one mixed block, because I didn't want people to accidentally add new\n> setup lines to the wrong block. But maybe with sufficient comments, it\n> would be clear (and even make the code flow a bit more obvious).\n> \n> Here's an attempt at that.  I did drop a few comments that seemed\n> pointless to me, as they're just re-stating the lines they describe.\n\nIt looks like the original got merged to next. I think we at least need\nto deal with the \"Otherwise...\" comment, but I think the layout I posted\nin my followup is nicer overall. Was it a mistake to merge the first\none?\n\nIf so, do you want an incremental, or do you just want to drop it and\nredo as part of the post-2.21 rewind?\n\n-Peff\n"},{"id":"370091","messageId":"xmqqpnrhmcpf.fsf@gitster-ct.c.googlers.com","threadId":"50495","inReplyTo":"20190224094545.GA11538@sigill.intra.peff.net","subject":"Re: [PATCH] diff: reuse diff setup for --no-index case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-24T15:24:28Z","receivedAt":"2019-02-24T15:24:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Here's an attempt at that.  I did drop a few comments that seemed\n>> pointless to me, as they're just re-stating the lines they describe.\n>\n> It looks like the original got merged to next. I think we at least need\n> to deal with the \"Otherwise...\" comment, but I think the layout I posted\n> in my followup is nicer overall. Was it a mistake to merge the first\n> one?\n>\n> If so, do you want an incremental, or do you just want to drop it and\n> redo as part of the post-2.21 rewind?\n\nWill do a revert-and-replace just like I did for a few others I\nscrewed up recently.\n\nThanks.\n"}]}