{"thread":{"id":"29894","subject":"[PATCH] diff: Introduce diff.algorithm variable","startedAt":"2012-03-09T13:48:03Z","lastAt":"2012-03-13T20:48:26Z","messageCount":6,"participants":["Michal Privoznik","Junio C Hamano","Tay Ray Chuan","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"186564","messageId":"06652d553040ad6b25608dc69d632f1ee38eaeca.1331300343.git.mprivozn@redhat.com","threadId":"29894","inReplyTo":null,"subject":"[PATCH] diff: Introduce diff.algorithm variable","fromName":"Michal Privoznik","fromEmail":"mprivozn@redhat.com","sentAt":"2012-03-09T13:48:03Z","receivedAt":"2012-03-09T13:48:03Z","isPatch":true,"sender":{"key":"mprivozn@redhat.com","avatar":"https://avatars.githubusercontent.com/u/12559331?v=4"},"body":"Some users have preference over various diff algorithms. However,\nnow they are forced to use appropriate argument every time they\nrun git-diff and tools using it. This is impractical. Therefore\ncreate new variable which can set preferred algorithm. Of course,\nthis can be overridden on command line via --diff-algorithm=*.\nAccepted values are myers (default), histogram, minimal, patience.\n\nSigned-off-by: Michal Privoznik <mprivozn@redhat.com>\n---\nThis is basically v2 for:\n\n   http://www.spinics.net/lists/git/msg176100.html\n\nAs we agreed on list, I've switched from diff.patience to\ndiff.algorithm and created new argument --diff-algorithm.\n\nPlease keep me CC'ed as I am not signed into the list.\n\n Documentation/diff-config.txt          |   21 +++++++++++++++++++\n Documentation/diff-options.txt         |   23 +++++++++++++++++++++\n contrib/completion/git-completion.bash |   13 +++++++++++\n diff.c                                 |   35 ++++++++++++++++++++++++++++++++\n 4 files changed, 92 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 6aa1be0..1047e81 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -1,3 +1,24 @@\n+diff.algorithm::\n+    Choose a diff algorithm.  The variants are as follows:\n++\n+--\n+histogram::\n+    This is the fastest algorithm.\n+\n+myers::\n+    The classical Myers diff algorithm. This is the default.\n+\n+minimal::\n+    Like 'myers', but spend extra time making sure that the diff\n+    is the shortest possible for the set of changes performed.\n+\n+patience::\n+    The patience diff algorithm, which first matches unique lines\n+    with each other.  This sometimes results in more readable (if\n+    longer) patches than the other algorithms.\n+--\n++\n+\n diff.autorefreshindex::\n \tWhen using 'git diff' to compare with work tree\n \tfiles, do not consider stat-only change as changed.\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 7d4566f..4e8bc5b 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -55,6 +55,29 @@ endif::git-format-patch[]\n --histogram::\n \tGenerate a diff using the \"histogram diff\" algorithm.\n \n+--diff-algorithm={histogram|myers|minimal|patience}::\n+    Choose a diff algorithm. The defaults are controlled\n+    by the `diff.algorithm` configuration variable\n+    (see linkgit:git-config[1]). The variants are as follows:\n++\n+--\n+histogram::\n+    This is the fastest algorithm.\n+\n+myers::\n+    The classical Myers diff algorithm. This is the default.\n+\n+minimal::\n+    Like 'myers', but spend extra time making sure that the diff\n+    is the shortest possible for the set of changes performed.\n+\n+patience::\n+    The patience diff algorithm, which first matches unique lines\n+    with each other.  This sometimes results in more readable (if\n+    longer) patches than the other algorithms.\n+--\n++\n+\n --stat[=<width>[,<name-width>[,<count>]]]::\n \tGenerate a diffstat. By default, as much space as necessary\n \twill be used for the filename part, and the rest for the graph\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex fba076d..d54f3a3 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1299,6 +1299,7 @@ __git_diff_common_options=\"--stat --numstat --shortstat --summary\n \t\t\t--raw\n \t\t\t--dirstat --dirstat= --dirstat-by-file\n \t\t\t--dirstat-by-file= --cumulative\n+\t\t\t--diff-algorithm=\n \"\n \n _git_diff ()\n@@ -1306,6 +1307,10 @@ _git_diff ()\n \t__git_has_doubledash && return\n \n \tcase \"$cur\" in\n+\t--diff-algorithm=*)\n+\t\t__gitcomp \"myers histogram minimal patience\"\n+\t\treturn\n+\t\t;;\n \t--*)\n \t\t__gitcomp \"--cached --staged --pickaxe-all --pickaxe-regex\n \t\t\t--base --ours --theirs --no-index\n@@ -1569,6 +1574,10 @@ _git_log ()\n \t\t__gitcomp \"long short\" \"\" \"${cur##--decorate=}\"\n \t\treturn\n \t\t;;\n+    --diff-algorithm=*)\n+\t\t__gitcomp \"myers histogram minimal patience\"\n+\t\treturn\n+\t\t;;\n \t--*)\n \t\t__gitcomp \"\n \t\t\t$__git_log_common_options\n@@ -2376,6 +2385,10 @@ _git_show ()\n \t\t\t\" \"\" \"${cur#*=}\"\n \t\treturn\n \t\t;;\n+\t--diff-algorithm=*)\n+\t\t__gitcomp \"myers histogram minimal patience\"\n+\t\treturn\n+\t\t;;\n \t--*)\n \t\t__gitcomp \"--pretty= --format= --abbrev-commit --oneline\n \t\t\t$__git_diff_common_options\ndiff --git a/diff.c b/diff.c\nindex 377ec1e..f5c965b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -34,6 +34,7 @@ static int diff_no_prefix;\n static int diff_stat_graph_width;\n static int diff_dirstat_permille_default = 30;\n static struct diff_options default_diff_options;\n+static int diff_algorithm = 0;\n \n static char diff_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_RESET,\n@@ -47,6 +48,20 @@ static char diff_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_NORMAL,\t/* FUNCINFO */\n };\n \n+static int diff_algorithm_parse(const char *alg)\n+{\n+\tif (!alg || !strcmp(alg, \"myers\"))\n+\t\treturn 0;\n+\telse if (!strcmp(alg, \"patience\"))\n+\t\treturn XDF_PATIENCE_DIFF;\n+\telse if (!strcmp(alg, \"histogram\"))\n+\t\treturn XDF_HISTOGRAM_DIFF;\n+\telse if (!strcmp(alg, \"minimal\"))\n+\t\treturn XDF_NEED_MINIMAL;\n+\n+\treturn -1;\n+}\n+\n static int parse_diff_color_slot(const char *var, int ofs)\n {\n \tif (!strcasecmp(var+ofs, \"plain\"))\n@@ -169,6 +184,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"diff.ignoresubmodules\"))\n \t\thandle_ignore_submodules_arg(&default_diff_options, value);\n \n+\tif (!strcmp(var, \"diff.algorithm\")) {\n+\t\tif ((diff_algorithm = diff_algorithm_parse(value)) < 0)\n+\t\t\tdie(\"bad diff.algorithm value: %s\", value);\n+\t\treturn 0;\n+\t}\n+\n \tif (git_color_config(var, value, cb) < 0)\n \t\treturn -1;\n \n@@ -3250,6 +3271,9 @@ int diff_setup_done(struct diff_options *options)\n \t\tDIFF_OPT_SET(options, EXIT_WITH_STATUS);\n \t}\n \n+\t/* set diff algorithm */\n+\toptions->xdl_opts |= diff_algorithm;\n+\n \treturn 0;\n }\n \n@@ -3528,6 +3552,17 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tDIFF_XDL_SET(options, PATIENCE_DIFF);\n \telse if (!strcmp(arg, \"--histogram\"))\n \t\tDIFF_XDL_SET(options, HISTOGRAM_DIFF);\n+\telse if ((argcount = parse_long_opt(\"diff-algorithm\", av, &optarg))) {\n+\t\tint alg = diff_algorithm_parse(optarg);\n+\t\tif (alg < 0)\n+\t\t\tdie(\"unknown algorithm: %s\", optarg);\n+\t\tDIFF_XDL_CLR(options, NEED_MINIMAL);\n+\t\tDIFF_XDL_CLR(options, HISTOGRAM_DIFF);\n+\t\tDIFF_XDL_CLR(options, PATIENCE_DIFF);\n+\t\toptions->xdl_opts |= alg;\n+\t\treturn argcount;\n+\t}\n+\n \n \t/* flags options */\n \telse if (!strcmp(arg, \"--binary\")) {\n-- \n1.7.8.5\n"},{"id":"186576","messageId":"7vmx7p62k1.fsf@alter.siamese.dyndns.org","threadId":"29894","inReplyTo":"06652d553040ad6b25608dc69d632f1ee38eaeca.1331300343.git.mprivozn@redhat.com","subject":"Re: [PATCH] diff: Introduce diff.algorithm variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-09T18:40:14Z","receivedAt":"2012-03-09T18:40:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michal Privoznik <mprivozn@redhat.com> writes:\n\n> Some users have preference over various diff algorithms. However,\n> now they are forced to use appropriate argument every time they\n> run git-diff and tools using it. This is impractical. Therefore\n> create new variable which can set preferred algorithm. Of course,\n\nWe tend to prefer avoiding these subjective and judgemental words\nlike \"forced\" and \"impractical\" when the paragraph conveys the same\nreasoning even when they are toned down.\n\n> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\n> index 6aa1be0..1047e81 100644\n> --- a/Documentation/diff-config.txt\n> +++ b/Documentation/diff-config.txt\n> @@ -1,3 +1,24 @@\n> +diff.algorithm::\n> +    Choose a diff algorithm.  The variants are as follows:\n\nIn order to avoid confusing users, we may want to mention that it is\ndeliberate that this configuration does not apply to plumbing\ncommands.\n\nIt may make sense to drop the enumeration of \"The variants are...\",\nand say that it can take the values valid for the \"--diff-algorithm\"\nparameter to the \"diff\" family of commands.  We do not have to worry\nabout keeping two enumerations in sync if we did so.\n\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 7d4566f..4e8bc5b 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -55,6 +55,29 @@ endif::git-format-patch[]\n>  --histogram::\n>  \tGenerate a diff using the \"histogram diff\" algorithm.\n>  \n> +--diff-algorithm={histogram|myers|minimal|patience}::\n> +    Choose a diff algorithm. The defaults are controlled\n> +    by the `diff.algorithm` configuration variable\n\ns/$/ for the Porcelain commands/; perhaps?\n\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index fba076d..d54f3a3 100755\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1299,6 +1299,7 @@ __git_diff_common_options=\"--stat --numstat --shortstat --summary\n>  \t\t\t--raw\n>  \t\t\t--dirstat --dirstat= --dirstat-by-file\n>  \t\t\t--dirstat-by-file= --cumulative\n> +\t\t\t--diff-algorithm=\n>  \"\n\nSo this helps me when I do \"git diff --diff-al<TAB>\" and expands it\nto \"git diff --diff-algorithm=\" and then...\n\n>  _git_diff ()\n> @@ -1306,6 +1307,10 @@ _git_diff ()\n>  \t__git_has_doubledash && return\n>  \n>  \tcase \"$cur\" in\n> +\t--diff-algorithm=*)\n> +\t\t__gitcomp \"myers histogram minimal patience\"\n> +\t\treturn\n> +\t\t;;\n\n... when typing <TAB> again, this helps me by reminding what choices\nare available.  Is that the idea?  Nice.\n\nBut I see you repeated this four-element list three times.  Can't we\ndo it in just one place?\n\n> diff --git a/diff.c b/diff.c\n> index 377ec1e..f5c965b 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -34,6 +34,7 @@ static int diff_no_prefix;\n>  static int diff_stat_graph_width;\n>  static int diff_dirstat_permille_default = 30;\n>  static struct diff_options default_diff_options;\n> +static int diff_algorithm = 0;\n\nPlease let BSS take care of this initialization by omitting \" = 0\".\n\n> @@ -169,6 +184,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n>  \tif (!strcmp(var, \"diff.ignoresubmodules\"))\n>  \t\thandle_ignore_submodules_arg(&default_diff_options, value);\n>  \n> +\tif (!strcmp(var, \"diff.algorithm\")) {\n> +\t\tif ((diff_algorithm = diff_algorithm_parse(value)) < 0)\n> +\t\t\tdie(\"bad diff.algorithm value: %s\", value);\n\nI am not sure if this is a good idea.  What happens when Git 2.0\nadds a new algorithm, the user names it in ~/.gitconfig, and an\nolder version of Git sees this value?  Do you force the user to\nmodify ~/.gitconfig to use one of the older ones?\n\nIt might be more friendly to just warn and use the default.\n\n> @@ -3250,6 +3271,9 @@ int diff_setup_done(struct diff_options *options)\n>  \t\tDIFF_OPT_SET(options, EXIT_WITH_STATUS);\n>  \t}\n>  \n> +\t/* set diff algorithm */\n> +\toptions->xdl_opts |= diff_algorithm;\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -3528,6 +3552,17 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n>  \t\tDIFF_XDL_SET(options, PATIENCE_DIFF);\n>  \telse if (!strcmp(arg, \"--histogram\"))\n>  \t\tDIFF_XDL_SET(options, HISTOGRAM_DIFF);\n> +\telse if ((argcount = parse_long_opt(\"diff-algorithm\", av, &optarg))) {\n> +\t\tint alg = diff_algorithm_parse(optarg);\n> +\t\tif (alg < 0)\n> +\t\t\tdie(\"unknown algorithm: %s\", optarg);\n> +\t\tDIFF_XDL_CLR(options, NEED_MINIMAL);\n> +\t\tDIFF_XDL_CLR(options, HISTOGRAM_DIFF);\n> +\t\tDIFF_XDL_CLR(options, PATIENCE_DIFF);\n> +\t\toptions->xdl_opts |= alg;\n> +\t\treturn argcount;\n> +\t}\n> +\n\nThe last two hunks gives me a queasy feeling for some reason.  I\nthink I recently saw a patch to clean up this area to clarify that\nPATIENCE/HISTOGRAM are not independent option bits (but I cannot\nremember where) and we probably should use the approach taken by\nthat patch.\n\nOther than that, both the design and the implementation seems to be\ndone reasonably competently.  Don't we want to add tests for this?\n"},{"id":"186677","messageId":"7vsjhev0hy.fsf@alter.siamese.dyndns.org","threadId":"29894","inReplyTo":"7vmx7p62k1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: Introduce diff.algorithm variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-12T05:42:17Z","receivedAt":"2012-03-12T05:42:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The last two hunks gives me a queasy feeling for some reason.  I\n> think I recently saw a patch to clean up this area to clarify that\n> PATIENCE/HISTOGRAM are not independent option bits (but I cannot\n> remember where) and we probably should use the approach taken by\n> that patch.\n\nI did some digging; no wonder I remember it---it is one of the stuff\nI wrote and then later discarded.\n\n  http://thread.gmane.org/gmane.comp.version-control.git/191036/focus=191038\n\nI think this \"algorithm\" thing should build on that patch, even\nthough the later parts of the series in that thread has turned out\nto be unwanted.\n\nThanks.\n"},{"id":"186864","messageId":"CALUzUxpR_bPLvnHRJrQ2wVpqJD6Ccg6r3RMa_sCDKHTapnJsdw@mail.gmail.com","threadId":"29894","inReplyTo":"06652d553040ad6b25608dc69d632f1ee38eaeca.1331300343.git.mprivozn@redhat.com","subject":"Re: [PATCH] diff: Introduce diff.algorithm variable","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-03-13T14:42:50Z","receivedAt":"2012-03-13T14:42:50Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Fri, Mar 9, 2012 at 9:48 PM, Michal Privoznik <mprivozn@redhat.com> wrote:\n> Some users have preference over various diff algorithms. However,\n> now they are forced to use appropriate argument every time they\n> run git-diff and tools using it. This is impractical. Therefore\n> create new variable which can set preferred algorithm. Of course,\n> this can be overridden on command line via --diff-algorithm=*.\n> Accepted values are myers (default), histogram, minimal, patience.\n>\n> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>\n> ---\n> This is basically v2 for:\n>\n>   http://www.spinics.net/lists/git/msg176100.html\n>\n> As we agreed on list, I've switched from diff.patience to\n> diff.algorithm and created new argument --diff-algorithm.\n>\n> Please keep me CC'ed as I am not signed into the list.\n>\n>  Documentation/diff-config.txt          |   21 +++++++++++++++++++\n>  Documentation/diff-options.txt         |   23 +++++++++++++++++++++\n>  contrib/completion/git-completion.bash |   13 +++++++++++\n>  diff.c                                 |   35 ++++++++++++++++++++++++++++++++\n>  4 files changed, 92 insertions(+), 0 deletions(-)\n>\n> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\n> index 6aa1be0..1047e81 100644\n> --- a/Documentation/diff-config.txt\n> +++ b/Documentation/diff-config.txt\n> @@ -1,3 +1,24 @@\n> +diff.algorithm::\n> +    Choose a diff algorithm.  The variants are as follows:\n> ++\n> +--\n> +histogram::\n> +    This is the fastest algorithm.\n> +\n> +myers::\n> +    The classical Myers diff algorithm. This is the default.\n> +\n> +minimal::\n> +    Like 'myers', but spend extra time making sure that the diff\n> +    is the shortest possible for the set of changes performed.\n> +\n> +patience::\n> +    The patience diff algorithm, which first matches unique lines\n> +    with each other.  This sometimes results in more readable (if\n> +    longer) patches than the other algorithms.\n> +--\n> ++\n> +\n\nConsidering that --minimal isn't really an algorithm, could this be\ninstead made to a \"default options to always pass to diff\" config? I\nimagine this would make the codepath involved in this patch much\nsimpler (just reuse the argc/argv opt parsing machinery).\n\nAdditionally, it would be useful for users who want to, say, set the\ncontext to 5 lines always.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"186878","messageId":"7v399cxuk9.fsf@alter.siamese.dyndns.org","threadId":"29894","inReplyTo":"CALUzUxpR_bPLvnHRJrQ2wVpqJD6Ccg6r3RMa_sCDKHTapnJsdw@mail.gmail.com","subject":"Re: [PATCH] diff: Introduce diff.algorithm variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-13T17:46:46Z","receivedAt":"2012-03-13T17:46:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan <rctay89@gmail.com> writes:\n\n> Considering that --minimal isn't really an algorithm,...\n\nI think this discussion thread already settled that \"minimal\" *is* an\nalgorithm for the purpose of this topic, considering that the --minimal\noption does not apply anything but myers.\n"},{"id":"186901","messageId":"20120313204826.GA27436@sigill.intra.peff.net","threadId":"29894","inReplyTo":"7v399cxuk9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: Introduce diff.algorithm variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-13T20:48:26Z","receivedAt":"2012-03-13T20:48:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 13, 2012 at 10:46:46AM -0700, Junio C Hamano wrote:\n\n> Tay Ray Chuan <rctay89@gmail.com> writes:\n> \n> > Considering that --minimal isn't really an algorithm,...\n> \n> I think this discussion thread already settled that \"minimal\" *is* an\n> algorithm for the purpose of this topic, considering that the --minimal\n> option does not apply anything but myers.\n\nI don't know that it is settled. I proposed it as a concept, and nobody\nelse objected. It is true that right _now_ --minimal doesn't do anything\nfor non-myers algorithms, but in theory it could. IOW, it is a matter of\nthe user's perspective whether the flag is \"use the minimal Myers diff\nalgorithm\" versus \"spend extra effort to make the diff more minimal, no\nmatter which algorithm is being used\". If the latter, it is only that we\nhave not (yet) implemented the extra effort tweaks to the other\nalgorithms.\n\nSo I think we could go either way.\n\n-Peff\n"}]}