Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 5, 2016, 07:19 UTC
Message-ID: <xmqqr35m6dwt.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161204194747.7100-1-jack@nottheoilrig.com>

Jack Bates <bk874k@nottheoilrig.com> writes:

Show 25 quoted lines

> The three cases where "git diff" operates outside of a repository are 1)
> when we run it outside of a repository, 2) when one of the files we're
> comparing is outside of the repository we're in, and 3) the --no-index
> option. Commit 4f03666 ("diff: handle sha1 abbreviations outside of
> repository", 2016-10-20) only worked in the first case.
> ---
>  builtin/diff.c                  |  4 +++-
>  t/t4063-diff-no-index-abbrev.sh | 50 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 1 deletion(-)
>  create mode 100755 t/t4063-diff-no-index-abbrev.sh
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index 7f91f6d..ec7c432 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -342,9 +342,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  		       "--no-index" : "[--no-index]");
>  
>  	}
> -	if (no_index)
> +	if (no_index) {
>  		/* If this is a no-index diff, just run it and exit there. */
> +		startup_info->have_repository = 0;
>  		diff_no_index(&rev, argc, argv);
> +	}

This kind of change makes me nervous (partly because I am not seeing the whole code but only this part of the patch).

Some code may react to "have_repository" being zero and do the right thing (which I think is what you are using from your previous "we did one of the three cases" change here), but the codepath that led to "have_repository" being set to non-zero previously must have done a lot more than just flipping that field to non-zero, and setting zero to this field alone would not "undo" what it did.

I wonder if we're better off if we made sure that diff_no_index() works the same way regardless of the value of "have_repository" field?