Re: [PATCH] diff: reuse diff setup for --no-index case

[Junio C Hamano <gitster@pobox.com>]

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);

;-)