Re: [PATCH v3 2/2] diff: teach diff to read algorithm from diff driver
From: Junio C Hamano <hidden>
Date: 2023-02-17 21:51:04
"John Cai via GitGitGadget" [off-list ref] writes: Looking good. Some comments below. Many of them minor.
+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
I would have expected "per path" instead of "by path".
+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.
While this is not incorrect per-se, I think the first paragraph of the proposed commit log message was a lot more convincing. Your changes may not be limited to a single kind of files, and a command line option is simply not enough. You may want one algorithm for ".json" while using another for ".c", which was really an excellent example you gave.
+This diff algorithm applies to user facing diff output like git-diff(1), +git-show(1) and is used for the `--stat` output as well. The merge machinery +will not use the diff algorithm set through this method.
Is "format-patch" considered "user-facing"?
+NOTE: If the `command` key also exists, then Git will treat this as an external +diff and attempt to use the value set for `command` as an external program. For +instance, the following config, combined with the above `.gitattributes` file, +will result in `command` favored over `algorithm`. + +---------------------------------------------------------------- +[diff "<name>"] + command = j-c-diff + algorithm = histogram +----------------------------------------------------------------
Isn't this a bit too verbose, given that the reader has just seen
the external diff driver section. I wonder something like this is
sufficient, without any sample configuration?
NOTE: If `diff.<name>.command` is defined for path with the
`diff=<name>` attribute, it is executed as an external diff driver
(see above), and adding `diff.<name>.algorithm` has no effect (the
algorithm is not passed to the external diff driver).
quoted hunk
diff --git a/diff.c b/diff.c index 5efc22ca06b..04469da6d34 100644 --- a/diff.c +++ b/diff.c@@ -4456,15 +4456,13 @@ 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 = NULL; - - if (o->flags.allow_external) { - struct userdiff_driver *drv; - + if (o->flags.allow_external || !o->ignore_driver_algorithm) drv = userdiff_find_by_path(o->repo->index, attr_path); - if (drv && drv->external) - pgm = drv->external; - } + + if (o->flags.allow_external && drv && drv->external) + pgm = drv->external;
OK. There is no explicit "pgm = NULL" initialization in this function, but that is done by the caller passing NULL to the function as a parameter, so it all makes sense.
quoted hunk
@@ -4481,12 +4479,16 @@ 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 (drv && !o->ignore_driver_algorithm && drv->algorithm) + set_diff_algorithm(o, drv->algorithm);
For symmetry with the above choice of pgm we just saw, the order of the condition might be easier to follow if written like so: if (!o->ignore_driver_algorithm && drv && drv->algorithm) It would not make any measurable difference performance-wise either way.
quoted hunk
@@ -4583,6 +4585,14 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o, const char *name; const char *other; + if (!o->ignore_driver_algorithm) { + struct userdiff_driver *drv = userdiff_find_by_path(o->repo->index, p->one->path);
That's an overlong line.
+
+ if (drv && drv->algorithm) {
+ set_diff_algorithm(o, drv->algorithm);
+ }
No need to have {} around a single statement block.
quoted hunk
+ } + if (DIFF_PAIR_UNMERGED(p)) { /* unmerged */ builtin_diffstat(p->one->path, NULL, NULL, NULL,@@ -5130,6 +5140,8 @@ static int diff_opt_diff_algorithm(const struct option *opt, return error(_("option diff-algorithm accepts \"myers\", " "\"minimal\", \"patience\" and \"histogram\"")); + options->ignore_driver_algorithm = 1; + return 0; }@@ -5145,6 +5157,8 @@ static int diff_opt_diff_algorithm_no_arg(const struct option *opt, BUG("available diff algorithms include \"myers\", " "\"minimal\", \"patience\" and \"histogram\""); + options->ignore_driver_algorithm = 1; + return 0; }@@ -5285,6 +5299,7 @@ static int diff_opt_patience(const struct option *opt, for (i = 0; i < options->anchors_nr; i++) free(options->anchors[i]); options->anchors_nr = 0; + options->ignore_driver_algorithm = 1; return set_diff_algorithm(options, "patience"); }
I was hoping that set_diff_algorithm() can be the shared common one that signals we were told to use a specific algorithm, but it also is called from internal codepaths so it cannot be it. It is probably not worth introducing an extra helper that only calls set_diff_algorithm() and sets ignore_driver_algorithm bit only for that to reduce three-times repetition. OK.
quoted hunk
diff --git a/userdiff.c b/userdiff.c index d71b82feb74..ff25cfc4b4c 100644 --- a/userdiff.c +++ b/userdiff.c@@ -293,7 +293,7 @@ PATTERNS("scheme", "|([^][)(}{[ \t])+"), PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$", "\\\\[a-zA-Z@]+|\\\\.|[a-zA-Z0-9\x80-\xff]+"), -{ "default", NULL, -1, { NULL, 0 } }, +{ "default", NULL, NULL, -1, { NULL, 0 } }, };
I was surprised that there is so little damage to the built-in userdiff driver definitions, but this is thanks to the PATTERNS() and IPATTERN() macro that use designated initializers. Very nice. Nicely done.