Re: [PATCH v2 2/2] diff: teach diff to read gitattribute diff-algorithm

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

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: John Cai <johncai86@gmail.com>
>
> It can be useful to specify diff algorithms per file type. For example,
> one may want to use the minimal diff algorithm for .json files, another
> for .c files, etc.
>
> Teach the diff machinery to check attributes for a diff driver. Also
> teach the diff driver parser a new type "algorithm" to look for in the
> config, which will be used if a driver has been specified through the
> attributes.
>
> Enforce precedence of diff algorithm by favoring the command line option,
> then looking at the driver attributes & config combination, then finally
> the diff.algorithm config.
>
> To enforce precedence order, use the `xdl_opts_command_line` member
> during options pasing to indicate the diff algorithm was set via command
> line args.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  Documentation/gitattributes.txt | 41 ++++++++++++++++++++++++++++++++-
>  diff.c                          | 25 +++++++++++++-------
>  diff.h                          |  2 ++
>  t/lib-diff-alternative.sh       | 38 +++++++++++++++++++++++++++++-
>  userdiff.c                      |  4 +++-
>  userdiff.h                      |  1 +
>  6 files changed, 100 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index c19e64ea0ef..7e69f509d0a 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -736,7 +736,6 @@ String::
>  	by the configuration variables in the "diff.foo" section of the
>  	Git config file.
>  
> -
>  Defining an external diff driver
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Unrelated change?  Wider paragraph gap between two sections than
other inter-paragraph gaps inside a single section is what original
had, and I think that is a reasonable thing to keep.

> @@ -758,6 +757,46 @@ with the above configuration, i.e. `j-c-diff`, with 7
>  parameters, just like `GIT_EXTERNAL_DIFF` program is called.
>  See linkgit:git[1] for details.

In other words, this new section wants another blank line before to match.

>  
> +Setting the internal diff algorithm
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The diff algorithm can be set through the `diff.algorithm` config key, but
> +sometimes it may be helpful to set the diff algorithm by path. For example, one
> +might wish to set a diff algorithm automatically for all `.json` files such that
> +the user would not need to pass in a separate command line `--diff-algorithm` flag each
> +time.

That's an overly wide paragraph.

> +
> +First, in `.gitattributes`, you would assign the `diff` attribute for paths.
> +
> +*Git attributes*

Discard this line (mimic an existing section, like "Defining a
custom hunk-header").

> +------------------------
> +*.json diff=<name>
> +------------------------
> +
> +Then, you would define a "diff.<name>.algorithm" configuration to specify the
> +diff algorithm, choosing from `meyers`, `patience`, `minimal`, and `histogram`.
> +
> +*Git config*

Likewise, discard this line (I won't repeat but the next hunk has
the same issue).

> diff --git a/diff.c b/diff.c
> index 92a0eab942e..24da439e56f 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4456,15 +4456,11 @@ static void run_diff_cmd(const char *pgm,
>  	const char *xfrm_msg = NULL;
>  	int complete_rewrite = (p->status == DIFF_STATUS_MODIFIED) && p->score;
>  	int must_show_header = 0;
> +	struct userdiff_driver *drv = userdiff_find_by_path(o->repo->index, attr_path);

Do we run this look-up unconditionally, even when .allow_external
bit is not set?  Why?

> -
> -	if (o->flags.allow_external) {
> -		struct userdiff_driver *drv;
> -
> -		drv = userdiff_find_by_path(o->repo->index, attr_path);
> +	if (o->flags.allow_external)
>  		if (drv && drv->external)
>  			pgm = drv->external;
> -	}
>  
>  	if (msg) {
>  		/*
> @@ -4481,12 +4477,17 @@ static void run_diff_cmd(const char *pgm,
>  		run_external_diff(pgm, name, other, one, two, xfrm_msg, o);
>  		return;
>  	}
> -	if (one && two)
> +	if (one && two) {
> +		if (!o->xdl_opts_command_line)
> +			if (drv && drv->algorithm)
> +				set_diff_algorithm(o, drv->algorithm);

The idea here seems to be "if there is no explicit instruction, and
if the diff driver specifies an algorithm, then use that one", which
is very straightforward and sensible.  Can we reliably tell if we
had an explicit instruction to override the driver?  That should
probably appear in other parts of the code, I guess.

>  		builtin_diff(name, other ? other : name,
>  			     one, two, xfrm_msg, must_show_header,
>  			     o, complete_rewrite);
> -	else
> +	} else {
>  		fprintf(o->file, "* Unmerged path %s\n", name);
> +	}
>  }



> @@ -4583,6 +4584,10 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,
>  	const char *name;
>  	const char *other;
>  
> +	struct userdiff_driver *drv = userdiff_find_by_path(o->repo->index, p->one->path);
> +	if (drv && drv->algorithm)
> +		set_diff_algorithm(o, drv->algorithm);

Interesting.  Does external diff play a role, like in run_diff_cmd()
we saw earlier?

> @@ -5130,6 +5135,8 @@ static int diff_opt_diff_algorithm(const struct option *opt,
>  		return error(_("option diff-algorithm accepts \"myers\", "
>  			       "\"minimal\", \"patience\" and \"histogram\""));
>  
> +	options->xdl_opts_command_line = 1;

OK, calling this member "xdl_" anything is highly misleading, as it
has nothing to do with the xdiff machinery.  How about calling it
after what it does, i.e. allowing the attribute driven diff driver
to specify the algorithm?  options.ignore_driver_algorithm or
something?  The options coming _from_ the command line may happen to
be the condition to trigger this behaviour in this current
implementation, but it does not have to stay that way forever.
Losing "command line" from the name of the flag would make it
clearer what is essential (i.e. this controls if the diff driver is
allowed to affect the choice of the algorithm) and what is not (i.e.
we happen to let it decided based on the presence or absense of
command line choice).

Thanks.