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

[John Cai <johncai86@gmail.com>]

On 23/02/14 07:20PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> 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?
> 
> Ah, this is perfectly fine.  It used to be that this codepath can
> tell that there is no need to check the diff driver when it is told
> never to use any external diff driver.  Now, even when it is computing
> the diff internally, it needs to check the diff driver to find out
> the favoured algorithm for the path.
> 
> Strictly speaking, if we are told NOT to use external diff driver,
> and if we are told NOT to pay attention to algorithm given by the
> diff driver, then we know we can skip the overhead of attribute
> look-up.  I.e. we could do this to avoid attribute look-up:
> 
> 	struct userdiff_driver *drv = NULL;
> 
> 	if (o->flags.allow_external || !o->ignore_driver_algorithm)
> 		drv = userdiff_find_by_path(...);
> 
> 	if (drv && o->flags.allow_external && drv->external)
> 		pgm = drv->external;
> 	...
> 	if (pgm)
> 		... do the external diff thing ...
> 	if (one && two) {
> 		if (drv && !o->ignore_driver_algorithm && drv->algorithm)
> 			set_diff_algo(...)
> 
> I was not sure if it would be worth it before writing the above
> down, but the resulting flow does not look _too_ bad.

Yes I think it's worth it to save on execution if we know we are not using
external diff algorithm.

> 
> >> @@ -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?
> 
> As whoever wrote "diffstat" did not think of counting output from
> external diff driver, of course in this codepath external diff would
> not appear.  So what we see is very much expected.
> 
> Just move the blank line we see before these new lines one line
> down, so that the variable decls are grouped together, with a blank
> line before the first executable statement.  I.e.
> 
> 	const char *name;
> 	const char *other;
> +       struct userdiff_driver *drv;
> +
> +	drv = userdiff_find_by_path(...);
> +	if (drv && drv->algorithm)
> +		set_diff_algorithm(o, drv->algorithm);

makes sense, thanks.

> 
> Shouldn't this function refrain from setting algorithm from the
> driver when the algorithm was given elsewhere?  E.g.
> 
> 	$ git show --histogram --stat
> 	
> or something?  IOW, shouldn't it also pay attention to
> o->ignore_driver_algorithm bit, just like run_diff_cmd() did?

Yes we should add the same guard as above.

> 
>