Jeff King <peff@xxxxxxxx> 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@xxxxxxxx>
> ---
> 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);
;-)
