On 23/02/14 07:20PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> 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.
>
>
