Re: [PATCH v3 1/2] diff: move no-index detection to builtin/diff.c

[Thomas Gummerer <t.gummerer@gmail.com>]

Jonathan Nieder <jrnieder@gmail.com> writes:

> (pruning cc list)
> Hi,
>
> Thomas Gummerer wrote:
>
>> Currently the --no-index option is parsed in diff_no_index().  Move the
>> detection if a no-index diff should be executed to builtin/diff.c
>
> No functional change intended, I assume?

Correct, I thought I'd split the refactoring from the functionality
changes.

> [...]
>> --- a/builtin/diff.c
>> +++ b/builtin/diff.c
>> @@ -283,14 +283,53 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
> [...]
>>  	prefix = setup_git_directory_gently(&nongit);
>> +	/*
>> +	 * Treat git diff with at least one path outside of the
>> +	 * repo the same as if the command would have been executed
>> +	 * outside of a git repository.  In this case it behaves
>> +	 * the same way as "git diff --no-index <a> <b>", which acts
>> +	 * as a colourful "diff" replacement.
>> +	 */
>> +	nongit |= (argc == i + 2) && !(path_inside_repo(prefix, argv[i]) &&
>> +				       path_inside_repo(prefix, argv[i + 1]));
>
> I would find this easier to read as
>
> 	if (argc == i + 2 &&
> 	    (!path_inside_repo(prefix, argv[i]) ||
> 	     !path_inside_repo(prefix, argv[i + 1])))
> 		nongit = 1;
>
> Or maybe using a different variable than 'nongit':
>
>  #define DIFF_NO_INDEX_EXPLICIT 1
>  #define DIFF_NO_INDEX_IMPLICIT 2
>
> 	...
> 	if (argc == i + 2 && ...)
> 		no_index = DIFF_NO_INDEX_IMPLICIT;

Thanks, that makes sense, will change in the re-roll.

> [...]
>>  	gitmodules_config();
>>  	git_config(git_diff_ui_config, NULL);
>>
>>  	init_revisions(&rev, prefix);
>>
>> -	/* If this is a no-index diff, just run it and exit there. */
>> -	diff_no_index(&rev, argc, argv, nongit, prefix);
>> +	if (no_index || nongit) {
> [...]
>> +	}
>
> Ok.
>
> [...]
>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -181,56 +181,14 @@ static int queue_diff(struct diff_options *o,
>>  	}
>>  }
>>
>> -void diff_no_index(struct rev_info *revs,
>> +int diff_no_index(struct rev_info *revs,
>
> Why the change in return type?

That was an oversight, no change is required.  Will fix in the
re-roll. Thanks for taking the time for reviewing this.

> Hope that helps,
> Jonathan

--
Thomas