From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 16 Dec 2013 17:48:10 GMT
Subject: Re: [PATCH v4 1/2] diff: move no-index detection to builtin/diff.c
Message-ID: <xmqqtxe8pu05.fsf@gitster.dls.corp.google.com>
In-Reply-To: <87zjo3lnn9.fsf@gmail.com>

Thomas Gummerer <t.gummerer@gmail.com> writes:

>> What happens if I run 'git diff --no-index /tmp git.c git.c' from
>> within a git repository?  With this, I fear I will get
>
> Thanks, I've missed that one.  It only happens when run outside a git
> repository, but the same  comments still apply.  Will fix and send a
> re-roll.

Please don't, as the last round has already been pushed on 'next'.

An incremental change on top would also illustrate more clearly what
breakage needed to be fixed, which would be another good thing. It
could even come with a new test that makes sure that the above
command line is diagnosed correctly as a mistake ;-).

Thanks.

>
>> 	Not a git repository
>> 	To compare two paths outside a working tree:
>> 	usage: git diff [--no-index] <path> <path>
>>
>> instead of the expected
>>
>> 	usage: git diff --no-index <path> <path>
>>
>> Something like
>>
>> 	if (no_index)
>> 		;
>> 	else if (nongit)
>> 		no_index = DIFF_NO_INDEX_IMPLICIT;
>> 	else if (argc != i + 2)
>> 		;
>> 	else if (!path_inside_repo(prefix, argv[i]) ||
>> 		 !path_inside_repo(prefix, argv[i + 1]))
>> 		no_index = DIFF_NO_INDEX_IMPLICIT;
>>
>> should work.  Or:
>>
>> 	if (!no_index) {
>> 		/*
>> 		 * Treat git diff with ...
>> 		 */
>> 		if (nongit || ...)
>> 			no_index = DIFF_NO_INDEX_IMPLICIT;
>> 	}
>>
>> Or the '!no_index &&' condition inserted some other way.
>>
>>> -	/* If this is a no-index diff, just run it and exit there. */
>>> -	diff_no_index(&rev, argc, argv, nongit, prefix);
>>> +	if (no_index) {
>>> +		if (argc != i + 2) {
>> [...]
>>> +			/* Give the usage message for non-repository usage and exit. */
>>> +			usagef("git diff %s <path> <path>",
>>> +			       no_index == DIFF_NO_INDEX_EXPLICIT ?
>>> +					"--no-index" : "[--no-index]");
>>> +
>>> +		}
>>> +		/* If this is a no-index diff, just run it and exit there. */
>>> +		diff_no_index(&rev, argc, argv, prefix);
>>> +	}
>>
>> Perhaps, to avoid some nesting:
>>
>> 	/* A no-index diff takes exactly two path arguments. */
>> 	if (no_index && argc != i + 2) {
>> 		...
>> 		usagef(...);
>> 	}
>>
>> 	if (no_index)
>> 		/* Just run the diff and exit. */
>> 		diff_no_index(...);
>>
>> Jonathan
>
> Thanks, will change in the re-roll.
>
> --
> Thomas