Re: [PATCH] diff: reuse diff setup for --no-index case
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
