Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Apr 27, 2017 at 04:50:37PM -0400, Marc Branchaud wrote:

> Subject: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.
>
> This makes the commands respect diff configuration options, such as
> indentHeuristic.
> 
> Signed-off-by: Marc Branchaud <marcnarc@xxxxxxxxxxx>

I think it would be helpful to future readers to explain what is going
on here. I.e., the bit about calling diff_setup_done(), which copies the
globals into the diff struct.

The same comments about the subject line from the last patch apply here,
too.

>  builtin/diff-files.c | 2 +-
>  builtin/diff-index.c | 2 +-
>  builtin/diff-tree.c  | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

It would be nice to have a test. Testing that dirstat's permille option
has an effect might be the easiest way to do so.

> diff --git a/builtin/diff-files.c b/builtin/diff-files.c
> index 15c61fd8d..a572da9d5 100644
> --- a/builtin/diff-files.c
> +++ b/builtin/diff-files.c
> @@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
>  	int result;
>  	unsigned options = 0;
>  
> +	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>  	init_revisions(&rev, prefix);
>  	gitmodules_config();
> -	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>  	rev.abbrev = 0;
>  	precompose_argv(argc, argv);

It's somewhat odd to me that diff_files uses a rev_info struct at all.
It doesn't traverse at all, and doesn't respect most of the options.
There's a half-hearted attempt to reject some obviously bogus cases, but
most of the options are just silently ignored.

I think it's mostly a historical wart (especially around the fact that
some diff options like combine_merges are in rev_info, which they
probably should not be). Anyway, none of that is your problem, and is
way outside the scope of this patch.

-Peff



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]