Re: [PATCH] diff: reuse diff setup for --no-index case
- From
Jeff King <peff@peff.net>
- Date
- Feb 16, 2019, 06:57 UTC
- Message-ID
- <20190216065755.GB27641@sigill.intra.peff.net>
- In-Reply-To
- <xmqqo97e885l.fsf@gitster-ct.c.googlers.com>
On Thu, Feb 14, 2019 at 11:47:02AM -0800, Junio C Hamano wrote:
Show 19 quoted lines
> > + 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