{"thread":{"id":"31534","subject":"[PATCH] Add diff.context option to specify default context","startedAt":"2012-09-14T18:16:03Z","lastAt":"2012-10-19T20:54:55Z","messageCount":8,"participants":["Jeff Muizelaar","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"199066","messageId":"A74D9034-E5BA-4CD5-B621-1C388EF4CE2B@mozilla.com","threadId":"31534","inReplyTo":null,"subject":"[PATCH] Add diff.context option to specify default context","fromName":"Jeff Muizelaar","fromEmail":"jmuizelaar@mozilla.com","sentAt":"2012-09-14T18:16:03Z","receivedAt":"2012-09-14T18:16:03Z","isPatch":true,"sender":{"key":"jmuizelaar@mozilla.com","avatar":"https://avatars.githubusercontent.com/u/332653?v=4"},"body":"This adds a diff.context config option to allow specifying\nthe number of lines of context. This is similar to Mercurial's\n'unified' option.\n\n"},{"id":"199072","messageId":"7vobl82u26.fsf@alter.siamese.dyndns.org","threadId":"31534","inReplyTo":"A74D9034-E5BA-4CD5-B621-1C388EF4CE2B@mozilla.com","subject":"Re: [PATCH] Add diff.context option to specify default context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-14T21:06:25Z","receivedAt":"2012-09-14T21:06:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Muizelaar <jmuizelaar@mozilla.com> writes:\n\n> This adds a diff.context config option to allow specifying\n> the number of lines of context. This is similar to Mercurial's\n> 'unified' option.\n\nRandom thoughts.\n\n* Please refer to Documentation/SubmittingPatches.  Saving your\n  message in a mbox and applying it would produce this crap:\n\n    commit ba4c972eacb91058f1317dbcd4ff77b471fa938e\n    Author: Jeff Muizelaar <jmuizelaar@mozilla.com>\n    Date:   Fri Sep 14 14:16:03 2012 -0400\n\n        Add diff.context option to specify default context\n\n        This adds a diff.context config option to allow specifying\n        the number of lines of context. This is similar to Mercurial's\n        'unified' option.\n\n        commit 1bd81c75de6824c39852bc8516acd0733737ed83\n        Author: Jeff Muizelaar <jmuizelaar@mozilla.com>\n        Date:   Fri Sep 14 13:55:02 2012 -0400\n\n            [PATCH] Add diff.context option to specify default context\n\n            This adds a diff.context config option to allow specifying\n            the number of lines of context. This is similar to\n            Mercurial's\n            'unified' option.\n\n  which is not acceptable.\n\n* Sign-off your patch.\n\n* Citing similaritly to options in other systems does not add much\n  value for people who read the proposed log message.  In this case,\n  I think the first sentence is written clearly enough that it is\n  sufficient without such clarification.  If anything, it should\n  instead say:\n\n\tdiff: diff.context configuration gives default to -U\n\n\tIntroduce a configuration variable diff.context that tells\n\tPorcelain commands to use a non-default number of context\n\tlines instead of 3 (the default).  With this variable, users\n\tdo not have to keep repeating \"git log -U8\" from the command\n\tline; instead, it becomes sufficient to say \"git config\n\tdiff.context 8\" just once.\n\n  or something like that to make it clear that it is related to our\n  -U option.\n\n* That relationship with the -U option may worth mentioning in the\n  documentation, not just in the log message.\n\n* The configuration is read only in diff_ui_config and not in the\n  lower-level diff_config.  What the patch does is the right thing.\n\n  It however needs to be documented in the patch to diff-config.txt\n  that it affects only the Porcelain commands, and does not break\n  plumbing commands.\n\n* Tests?  Minimally, the cases you may want to check are:\n\n  - What happens with various values set to this variable, and does\n    the code properly diagnose errors?\n\n    [diff]\n\tcontext ;# boolean?\n        context = no\n        context = 0\n        context = -1\n        context = 8\n\n  - What happens when the variable is set and the command line gives\n    a different value with -U?\n\n    git config diff.context 8\n    git log -U4 -1\n\n  - Does it really keep plumbing intact?\n\n    git config diff.context 8\n    git diff-files -p\n\n\nThanks.\n"},{"id":"200013","messageId":"C4993A4E-E443-4DB4-9DCA-20128CADDC6E@mozilla.com","threadId":"31534","inReplyTo":"7vobl82u26.fsf@alter.siamese.dyndns.org","subject":"[PATCH] diff: diff.context configuration gives default to -U","fromName":"Jeff Muizelaar","fromEmail":"jmuizelaar@mozilla.com","sentAt":"2012-09-27T15:04:44Z","receivedAt":"2012-09-27T15:04:44Z","isPatch":true,"sender":{"key":"jmuizelaar@mozilla.com","avatar":"https://avatars.githubusercontent.com/u/332653?v=4"},"body":"Introduce a configuration variable diff.context that tells\nPorcelain commands to use a non-default number of context\nlines instead of 3 (the default).  With this variable, users\ndo not have to keep repeating \"git log -U8\" from the command\nline; instead, it becomes sufficient to say \"git config\ndiff.context 8\" just once.\n\nSigned-off-by: Jeff Muizelaar <jmuizelaar@mozilla.com>\n---\n Documentation/diff-config.txt |    4 +\n diff.c                        |    9 +++-\n t/t4060-diff-context.sh       |  127 +++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 139 insertions(+), 1 deletions(-)\n create mode 100755 t/t4060-diff-context.sh\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 67a90a8..75ab8a5 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -56,6 +56,10 @@ diff.statGraphWidth::\n \tLimit the width of the graph part in --stat output. If set, applies\n \tto all commands generating --stat output except format-patch.\n \n+diff.context::\n+\tGenerate diffs with <n> lines of context instead of the default of\n+\t3. This value is overridden by the -U option.\n+\n diff.external::\n \tIf this config variable is set, diff generation is not\n \tperformed using the internal diff machinery, but using the\ndiff --git a/diff.c b/diff.c\nindex 35d3f07..86e5f2a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -26,6 +26,7 @@ static int diff_detect_rename_default;\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\n+static int diff_context_default = 3;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n int diff_auto_refresh_index = 1;\n@@ -141,6 +142,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\tdiff_use_color_default = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"diff.context\")) {\n+\t\tdiff_context_default = git_config_int(var, value);\n+\t\tif (diff_context_default < 0)\n+\t\t\treturn -1;\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"diff.renames\")) {\n \t\tdiff_detect_rename_default = git_config_rename(var, value);\n \t\treturn 0;\n@@ -3170,7 +3177,7 @@ void diff_setup(struct diff_options *options)\n \toptions->break_opt = -1;\n \toptions->rename_limit = -1;\n \toptions->dirstat_permille = diff_dirstat_permille_default;\n-\toptions->context = 3;\n+\toptions->context = diff_context_default;\n \tDIFF_OPT_SET(options, RENAME_EMPTY);\n \n \toptions->change = diff_change;\ndiff --git a/t/t4060-diff-context.sh b/t/t4060-diff-context.sh\nnew file mode 100755\nindex 0000000..76fa3c3\n--- /dev/null\n+++ b/t/t4060-diff-context.sh\n@@ -0,0 +1,127 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2012 Mozilla Foundation\n+#\n+\n+test_description='diff.context configuration'\n+\n+. ./test-lib.sh\n+\n+cat << EOF > x\n+firstline\n+b\n+c\n+d\n+e\n+f\n+preline\n+postline\n+i\n+j\n+k\n+l\n+m\n+n\n+EOF\n+test_expect_success 'initial add' '\n+\tgit update-index --add x &&\n+\tgit commit -m initial\n+'\n+\n+cat << EOF > x\n+firstline\n+b\n+c\n+d\n+e\n+f\n+preline\n+1\n+postline\n+i\n+j\n+k\n+l\n+m\n+n\n+EOF\n+\n+test_expect_success 'next commit' '\n+\tgit update-index --add x &&\n+\tgit commit -m next\n+'\n+cat << EOF > x\n+firstline\n+b\n+c\n+d\n+e\n+f\n+preline\n+2\n+postline\n+i\n+j\n+k\n+l\n+m\n+n\n+EOF\n+\n+\n+\n+\n+test_expect_success 'diff.context affects log' '\n+\tgit log -1 -p | grep -q -v firstline\n+\tgit config diff.context 8 &&\n+\tgit log -1 -p | grep -q firstline\n+'\n+test_expect_success 'different -U value' '\n+\tgit config diff.context 8 &&\n+\tgit log -U4 -1 | grep -q -v firstline\n+'\n+\n+test_expect_success 'diff.context affects diff' '\n+\tgit config diff.context 8 &&\n+\tgit diff | grep -q firstline\n+'\n+\n+test_expect_success 'plumbing not affected' '\n+\tgit config diff.context 8 &&\n+\tgit diff-files -p | grep -q -v firstline\n+'\n+\n+cat > .git/config << EOF\n+[diff]\n+\tcontext = no\n+EOF\n+test_expect_success 'config parsing' '\n+\tgit diff 2>&1 | grep -q \"bad config value\"\n+'\n+\n+cat > .git/config << EOF\n+[diff]\n+\tcontext = 0\n+EOF\n+test_expect_success 'config parsing' '\n+\tgit diff | grep -v preline\n+'\n+\n+cat > .git/config << EOF\n+[diff]\n+\tcontext = -1\n+EOF\n+test_expect_success 'config parsing' '\n+\tgit diff 2>&1 | grep -q \"bad config file\"\n+'\n+\n+cat > .git/config << EOF\n+[diff]\n+\tcontext = 8\n+EOF\n+test_expect_success 'config parsing' '\n+\tgit diff 2>&1 | grep \"postline\"\n+'\n+\n+\n+test_done\n-- \n1.7.4.4\n"},{"id":"200026","messageId":"7vr4pnqs8l.fsf@alter.siamese.dyndns.org","threadId":"31534","inReplyTo":"C4993A4E-E443-4DB4-9DCA-20128CADDC6E@mozilla.com","subject":"Re: [PATCH] diff: diff.context configuration gives default to -U","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-27T17:40:26Z","receivedAt":"2012-09-27T17:40:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Muizelaar <jmuizelaar@mozilla.com> writes:\n\n> Introduce a configuration variable diff.context that tells\n> Porcelain commands to use a non-default number of context\n> lines instead of 3 (the default).  With this variable, users\n> do not have to keep repeating \"git log -U8\" from the command\n> line; instead, it becomes sufficient to say \"git config\n> diff.context 8\" just once.\n>\n> Signed-off-by: Jeff Muizelaar <jmuizelaar@mozilla.com>\n> ---\n>  Documentation/diff-config.txt |    4 +\n>  diff.c                        |    9 +++-\n>  t/t4060-diff-context.sh       |  127 +++++++++++++++++++++++++++++++++++++++++\n>  3 files changed, 139 insertions(+), 1 deletions(-)\n>  create mode 100755 t/t4060-diff-context.sh\n\nSigh, we don't have existing tests to check the number of context\nlines given with -U option and we need to allocate a new test number\nfor it.  What is the gap between 4054 (the last one in the tests for\nthe diff family) and 4060 for?\n\n> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\n> index 67a90a8..75ab8a5 100644\n> --- a/Documentation/diff-config.txt\n> +++ b/Documentation/diff-config.txt\n> @@ -56,6 +56,10 @@ diff.statGraphWidth::\n>  \tLimit the width of the graph part in --stat output. If set, applies\n>  \tto all commands generating --stat output except format-patch.\n>  \n> +diff.context::\n> +\tGenerate diffs with <n> lines of context instead of the default of\n> +\t3. This value is overridden by the -U option.\n> +\n>  diff.external::\n>  \tIf this config variable is set, diff generation is not\n>  \tperformed using the internal diff machinery, but using the\n> diff --git a/diff.c b/diff.c\n> index 35d3f07..86e5f2a 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -26,6 +26,7 @@ static int diff_detect_rename_default;\n>  static int diff_rename_limit_default = 400;\n>  static int diff_suppress_blank_empty;\n>  static int diff_use_color_default = -1;\n> +static int diff_context_default = 3;\n>  static const char *diff_word_regex_cfg;\n>  static const char *external_diff_cmd_cfg;\n>  int diff_auto_refresh_index = 1;\n> @@ -141,6 +142,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n>  \t\tdiff_use_color_default = git_config_colorbool(var, value);\n>  \t\treturn 0;\n>  \t}\n> +\tif (!strcmp(var, \"diff.context\")) {\n> +\t\tdiff_context_default = git_config_int(var, value);\n> +\t\tif (diff_context_default < 0)\n> +\t\t\treturn -1;\n> +\t\treturn 0;\n> +\t}\n>  \tif (!strcmp(var, \"diff.renames\")) {\n>  \t\tdiff_detect_rename_default = git_config_rename(var, value);\n>  \t\treturn 0;\n> @@ -3170,7 +3177,7 @@ void diff_setup(struct diff_options *options)\n>  \toptions->break_opt = -1;\n>  \toptions->rename_limit = -1;\n>  \toptions->dirstat_permille = diff_dirstat_permille_default;\n> -\toptions->context = 3;\n> +\toptions->context = diff_context_default;\n>  \tDIFF_OPT_SET(options, RENAME_EMPTY);\n>  \n>  \toptions->change = diff_change;\n\nThanks; looks sensible.\n\n> diff --git a/t/t4060-diff-context.sh b/t/t4060-diff-context.sh\n> new file mode 100755\n> index 0000000..76fa3c3\n> --- /dev/null\n> +++ b/t/t4060-diff-context.sh\n> @@ -0,0 +1,127 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2012 Mozilla Foundation\n> +#\n> +\n> +test_description='diff.context configuration'\n> +\n> +. ./test-lib.sh\n> +\n> +cat << EOF > x\n> +firstline\n> +b\n> +c\n> +d\n> +e\n> +f\n> +preline\n> +postline\n> +i\n> +j\n> +k\n> +l\n> +m\n> +n\n> +EOF\n\nI know ancient tests are written like this, but we are slowly trying\nto migrate them to have these test-vector preparation inside\ntest_expect_success block, e.g.\n\n\n\ttest_expect_success setup '\n\t\tcat >x <<-\\EOF &&\n                firstline\n                b\n                ...\n                n\n                EOF\n\t\tgit add x &&\n                git commit -m initial\n\t'\n\n> +test_expect_success 'diff.context affects log' '\n> +\tgit log -1 -p | grep -q -v firstline\n> +\tgit config diff.context 8 &&\n> +\tgit log -1 -p | grep -q firstline\n> +'\n\nThree points:\n\n - Please avoid \"grep -q\", which does not help people who ran tests\n   (the output is hidden by default) and hurts people who want to\n   debug tests.\n\n - Your test will ignore breakage from the first \"log 1\" output and\n   goes on running \"git config\".  Make sure you got your && cascades\n   right.\n\n - Because an error from the command on the upstream side of the\n   pipe is ignored, we tend to prefer writing things like this:\n\n\tgit log -n 1 -p >output &&\n        grep -v firstline output &&\n\t...\n\n> +cat > .git/config << EOF\n> +[diff]\n> +\tcontext = no\n> +EOF\n> +test_expect_success 'config parsing' '\n> +\tgit diff 2>&1 | grep -q \"bad config value\"\n> +'\n\nHow does the \"git diff\" command exit?  That is far more important\nthan the actual error message, but being on the left side of the\npipe, the exit status from the command is not being tested.\n"},{"id":"200036","messageId":"20120927184309.GC2519@sigill.intra.peff.net","threadId":"31534","inReplyTo":"7vr4pnqs8l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: diff.context configuration gives default to -U","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-27T18:43:10Z","receivedAt":"2012-09-27T18:43:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 27, 2012 at 10:40:26AM -0700, Junio C Hamano wrote:\n\n> > +test_expect_success 'diff.context affects log' '\n> > +\tgit log -1 -p | grep -q -v firstline\n> > +\tgit config diff.context 8 &&\n> > +\tgit log -1 -p | grep -q firstline\n> > +'\n> \n> Three points:\n> \n>  - Please avoid \"grep -q\", which does not help people who ran tests\n>    (the output is hidden by default) and hurts people who want to\n>    debug tests.\n> \n>  - Your test will ignore breakage from the first \"log 1\" output and\n>    goes on running \"git config\".  Make sure you got your && cascades\n>    right.\n> \n>  - Because an error from the command on the upstream side of the\n>    pipe is ignored, we tend to prefer writing things like this:\n> \n> \tgit log -n 1 -p >output &&\n>         grep -v firstline output &&\n\nI agree with all of that. But also, is \"grep -v\" the right thing? I\nthink the intent of the test is \"firstline does not appear\". But that is\nnot what  \"grep -v\" will tell you. It will tell you whether any line\nthat did not have \"firstline\" in it was shown (which it would be, since\nthere are a bunch of other lines shown).\n\nI think \"! grep firstline\" is what is needed here. Or even just\nexplicitly matching the diff that we expect via test_cmp. I like the\nlatter much better anyway, as a failure will show exactly what is wrong.\nWhereas if the grep ends up not matching, there is no helpful output for\nsomebody reading the test.\n\nWe already produce nice messages from things like test_must_fail. Maybe\nit would be nice to have:\n\n  test_contains () {\n          if ! grep \"$1\" \"$2\"; then\n\t          echo >&2 \"File '$2' does not contain a line with '$1'\"\n\t\t  return 1\n\t  fi\n  }\n\nand likewise a \"test_not_contains\" or something to negate it. That makes\nboth the tests and their failure output readable.\n\n-Peff\n"},{"id":"200039","messageId":"B358ACEB-83C2-45C6-887F-D85C25A3496E@mozilla.com","threadId":"31534","inReplyTo":"7vr4pnqs8l.fsf@alter.siamese.dyndns.org","subject":"[PATCH] diff: diff.context configuration gives default to -U","fromName":"Jeff Muizelaar","fromEmail":"jmuizelaar@mozilla.com","sentAt":"2012-09-27T19:12:52Z","receivedAt":"2012-09-27T19:12:52Z","isPatch":true,"sender":{"key":"jmuizelaar@mozilla.com","avatar":"https://avatars.githubusercontent.com/u/332653?v=4"},"body":"Introduce a configuration variable diff.context that tells\nPorcelain commands to use a non-default number of context\nlines instead of 3 (the default).  With this variable, users\ndo not have to keep repeating \"git log -U8\" from the command\nline; instead, it becomes sufficient to say \"git config\ndiff.context 8\" just once.\n\nSigned-off-by: Jeff Muizelaar <jmuizelaar@mozilla.com>\n---\n\nThis fixes the tests to behave better and avoids 'grep -v'\n\n Documentation/diff-config.txt |    4 +\n diff.c                        |    9 +++-\n t/t4055-diff-context.sh       |  128 +++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 140 insertions(+), 1 deletions(-)\n create mode 100755 t/t4055-diff-context.sh\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 67a90a8..75ab8a5 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -56,6 +56,10 @@ diff.statGraphWidth::\n \tLimit the width of the graph part in --stat output. If set, applies\n \tto all commands generating --stat output except format-patch.\n \n+diff.context::\n+\tGenerate diffs with <n> lines of context instead of the default of\n+\t3. This value is overridden by the -U option.\n+\n diff.external::\n \tIf this config variable is set, diff generation is not\n \tperformed using the internal diff machinery, but using the\ndiff --git a/diff.c b/diff.c\nindex 35d3f07..86e5f2a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -26,6 +26,7 @@ static int diff_detect_rename_default;\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\n+static int diff_context_default = 3;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n int diff_auto_refresh_index = 1;\n@@ -141,6 +142,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\tdiff_use_color_default = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"diff.context\")) {\n+\t\tdiff_context_default = git_config_int(var, value);\n+\t\tif (diff_context_default < 0)\n+\t\t\treturn -1;\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"diff.renames\")) {\n \t\tdiff_detect_rename_default = git_config_rename(var, value);\n \t\treturn 0;\n@@ -3170,7 +3177,7 @@ void diff_setup(struct diff_options *options)\n \toptions->break_opt = -1;\n \toptions->rename_limit = -1;\n \toptions->dirstat_permille = diff_dirstat_permille_default;\n-\toptions->context = 3;\n+\toptions->context = diff_context_default;\n \tDIFF_OPT_SET(options, RENAME_EMPTY);\n \n \toptions->change = diff_change;\ndiff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh\nnew file mode 100755\nindex 0000000..8a31448\n--- /dev/null\n+++ b/t/t4055-diff-context.sh\n@@ -0,0 +1,128 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2012 Mozilla Foundation\n+#\n+\n+test_description='diff.context configuration'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tcat >x <<-EOF &&\n+firstline\n+b\n+c\n+d\n+e\n+f\n+preline\n+postline\n+i\n+j\n+k\n+l\n+m\n+n\n+EOF\n+\tgit update-index --add x &&\n+\tgit commit -m initial &&\n+\tcat >x <<-\\EOF &&\n+firstline\n+b\n+c\n+d\n+e\n+f\n+preline\n+1\n+postline\n+i\n+j\n+k\n+l\n+m\n+n\n+EOF\n+\tgit update-index --add x &&\n+\tgit commit -m next &&\n+cat >x <<-\\EOF\n+firstline\n+b\n+c\n+d\n+e\n+f\n+preline\n+2\n+postline\n+i\n+j\n+k\n+l\n+m\n+n\n+EOF\n+'\n+\n+test_expect_success 'diff.context affects log' '\n+\tgit log -1 -p >output &&\n+\t! grep firstline output &&\n+\tgit config diff.context 8 &&\n+\tgit log -1 -p >output &&\n+\tgrep firstline output\n+'\n+\n+test_expect_success 'different -U value' '\n+\tgit config diff.context 8 &&\n+\tgit log -U4 -1 >output &&\n+\t! grep firstline output\n+'\n+\n+test_expect_success 'diff.context affects diff' '\n+\tgit config diff.context 8 &&\n+\tgit diff >output &&\n+\tgrep firstline output\n+'\n+\n+test_expect_success 'plumbing not affected' '\n+\tgit config diff.context 8 &&\n+\tgit diff-files -p > output &&\n+\t! grep firstline output\n+'\n+test_expect_success 'non-integer config parsing' '\n+\tcat > .git/config <<-\\EOF &&\n+[diff]\n+\tcontext = no\n+EOF\n+\t! git diff 2>output &&\n+\tgrep \"bad config value\" output\n+'\n+\n+test_expect_success 'negative integer config parsing' '\n+\tcat >.git/config <<-\\EOF &&\n+[diff]\n+\tcontext = -1\n+EOF\n+\t! git diff 2>output &&\n+\tgrep \"bad config file\" output\n+'\n+\n+test_expect_success '0 config parsing' '\n+\tcat > .git/config <<-\\EOF &&\n+[diff]\n+\tcontext = 0\n+EOF\n+\tgit diff >output &&\n+\tgrep preline output\n+'\n+\n+test_expect_success 'config parsing' '\n+\tcat >.git/config <<-\\EOF &&\n+[diff]\n+\tcontext = 8\n+EOF\n+\tgit diff >output &&\n+\tgrep postline output\n+'\n+\n+test_done\n-- \n1.7.4.4\n"},{"id":"200053","messageId":"7v3923p0tb.fsf@alter.siamese.dyndns.org","threadId":"31534","inReplyTo":"B358ACEB-83C2-45C6-887F-D85C25A3496E@mozilla.com","subject":"Re: [PATCH] diff: diff.context configuration gives default to -U","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-27T22:18:08Z","receivedAt":"2012-09-27T22:18:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Muizelaar <jmuizelaar@mozilla.com> writes:\n\n> +\tif (!strcmp(var, \"diff.context\")) {\n> +\t\tdiff_context_default = git_config_int(var, value);\n> +\t\tif (diff_context_default < 0)\n> +\t\t\treturn -1;\n> +\t\treturn 0;\n\nI am somewhat torn on this part. This fails the entire command when\ndiff.context is set to non integer or negative integer, which means\ntrouble for a user of a future version of git that accepts such a\nvalue to do something intelligent we do not anticipate today. The\nuseful configuration value cannot be given unless the user is\ncertain that .gitconfig file will never be read by older version of\ngit.\n\nPerhaps it is OK, at least for now.  We'd have the same worry for\nwhat is given to -U<n> anyway.\n\n> +\t}\n>  \tif (!strcmp(var, \"diff.renames\")) {\n>  \t\tdiff_detect_rename_default = git_config_rename(var, value);\n>  \t\treturn 0;\n> @@ -3170,7 +3177,7 @@ void diff_setup(struct diff_options *options)\n>  \toptions->break_opt = -1;\n>  \toptions->rename_limit = -1;\n>  \toptions->dirstat_permille = diff_dirstat_permille_default;\n> -\toptions->context = 3;\n> +\toptions->context = diff_context_default;\n>  \tDIFF_OPT_SET(options, RENAME_EMPTY);\n>  \n>  \toptions->change = diff_change;\n> diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh\n> new file mode 100755\n> index 0000000..8a31448\n> --- /dev/null\n> +++ b/t/t4055-diff-context.sh\n> @@ -0,0 +1,128 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2012 Mozilla Foundation\n> +#\n> +\n> +test_description='diff.context configuration'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\tcat >x <<-EOF &&\n> +firstline\n> +b\n> +...\n> +n\n> +EOF\n> +\tgit update-index --add x &&\n> +\tgit commit -m initial &&\n> +\tcat >x <<-\\EOF &&\n> +firstline\n> +b\n\nThe dash after here-document redirection (i.e. \"<<-\") allows you to\nindent the meat of the here-document and the end-of-here-document\nmarker.  Please take advantage of the facility to make the result\nmore readable.\n\nAlso unless there is something that needs interpolation by the\nshell, we prefer to use quoted here-document (i.e. \"<<-\\EOF\") to\nreduce the mental burden by the readers of the code.  So\n\n\ttest_expect_success setup '\n\t\tcat >x <<-\\EOF &&\n\t\tfirstline\n\t\tb\n\t\t...\n\t\tn\n\t\tEOF\n\t\tuse x in the test &&\n\t\tcheck the result\n\t'\n\nThis comment applies to all other uses of <<- in this patch.\n\n> +test_expect_success 'diff.context affects log' '\n> +\tgit log -1 -p >output &&\n> +\t! grep firstline output &&\n> +\tgit config diff.context 8 &&\n> +\tgit log -1 -p >output &&\n> +\tgrep firstline output\n> +'\n\nIs there a reason to favor \"log -1 -p\" over something a lot simpler\nlike \"show\"?  Not requesting to change anything, but just being\ncurious.\n\n> +test_expect_success 'different -U value' '\n> +\tgit config diff.context 8 &&\n> +\tgit log -U4 -1 >output &&\n> +\t! grep firstline output\n> +'\n\nOK, so -U4 overrides configured diff.context setting and you make\nsure by asking -U4 that is too small to show firstline (but if 8\nwere used, the line would have been shown).\n\n> +test_expect_success 'diff.context affects diff' '\n> +\tgit config diff.context 8 &&\n> +\tgit diff >output &&\n> +\tgrep firstline output\n> +'\n\nDon't we want to make sure that without diff.context the output will\ngive you the default 3 lines of context?  It is a common mistake\nshared by people who want to demonstrate their shiny new toys to\ntest only the positive cases while forgetting to test non-regression.\n\n> +test_expect_success 'plumbing not affected' '\n> +\tgit config diff.context 8 &&\n> +\tgit diff-files -p > output &&\n\nStyle.  No SP between redirection and the filename, i.e.\n\n\tgit diff-files -p >output &&\n\n> +\t! grep firstline output\n> +'\n\nA blank line between tests.\n\n> +test_expect_success 'non-integer config parsing' '\n> +\tcat > .git/config <<-\\EOF &&\n\n> +[diff]\n> +\tcontext = no\n> +EOF\n> +\t! git diff 2>output &&\n> +\tgrep \"bad config value\" output\n> +'\n\nWe are not in the business of debugging \"grep\", so writing \"! grep ...\"\nto make sure grep does not find something is perfectly fine, but\nwhen expecting an error from \"git\" command, please write it like\n\n\ttest_must_fail git diff 2>error &&\n        test_i18ngrep \"bad config value\" error\n\ninstead.  The error messages meant for human consumption could be\nlocalized, and use of test_i18ngrep allows use to ignore its error\ncondition when the test is run under \"funny\" locale.\n\n> +test_expect_success 'negative integer config parsing' '\n> +\tcat >.git/config <<-\\EOF &&\n> +[diff]\n> +\tcontext = -1\n> +EOF\n> +\t! git diff 2>output &&\n> +\tgrep \"bad config file\" output\n> +'\n> +\n> +test_expect_success '0 config parsing' '\n> +\tcat > .git/config <<-\\EOF &&\n> +[diff]\n> +\tcontext = 0\n> +EOF\n> +\tgit diff >output &&\n> +\tgrep preline output\n> +'\n\nHrm, is this correct?  Your test vectors change a single line that\nis surrounded by \"preline\" and \"postline\", so -U0 patch should show\nonly the change for that single line, without showing \"preline\" or\n\"postline\".\n\nI think the \"grep\" pattern you used is too loose to be useful.  You\nwould probably want to be grepping for something like\n\n\tgit diff >output &&\n        grep \"^-1\" output &&\n        grep \"^+2\" output\n\ninstead.\n\n> +test_expect_success 'config parsing' '\n> +\tcat >.git/config <<-\\EOF &&\n> +[diff]\n> +\tcontext = 8\n> +EOF\n> +\tgit diff >output &&\n> +\tgrep postline output\n> +'\n\nI do not think this last test adds any value; you already checked it\nwith \"git config diff.context 8\" much earlier, no?  This script is\nnot about parsing of the handcrafted configuration files.\n\n> +test_done\n\nTo summarize...\n\n t/t4055-diff-context.sh | 135 ++++++++++++++++++------------------------------\n 1 file changed, 50 insertions(+), 85 deletions(-)\n\ndiff --git c/t/t4055-diff-context.sh w/t/t4055-diff-context.sh\nindex 8a31448..3527686 100755\n--- c/t/t4055-diff-context.sh\n+++ w/t/t4055-diff-context.sh\n@@ -8,121 +8,86 @@ test_description='diff.context configuration'\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-\tcat >x <<-EOF &&\n-firstline\n-b\n-c\n-d\n-e\n-f\n-preline\n-postline\n-i\n-j\n-k\n-l\n-m\n-n\n-EOF\n+\tcat >x <<-\\EOF &&\n+\tfirstline\n+\tb\n+\tc\n+\td\n+\te\n+\tf\n+\tpreline\n+\tpostline\n+\ti\n+\tj\n+\tk\n+\tl\n+\tm\n+\tn\n+\tEOF\n \tgit update-index --add x &&\n \tgit commit -m initial &&\n-\tcat >x <<-\\EOF &&\n-firstline\n-b\n-c\n-d\n-e\n-f\n-preline\n-1\n-postline\n-i\n-j\n-k\n-l\n-m\n-n\n-EOF\n+\n+\tgit cat-file blob HEAD:x |\n+\tsed \"/preline/a\\\n+\tADDED\" >x &&\n \tgit update-index --add x &&\n \tgit commit -m next &&\n-cat >x <<-\\EOF\n-firstline\n-b\n-c\n-d\n-e\n-f\n-preline\n-2\n-postline\n-i\n-j\n-k\n-l\n-m\n-n\n-EOF\n+\n+\tgit cat-file blob HEAD:x |\n+\tsed s/ADDED/MODIFIED/ >x\n '\n \n-test_expect_success 'diff.context affects log' '\n+test_expect_success 'the default number of context lines is 3' '\n+\tgit diff >output &&\n+\t! grep \"^ d\" output &&\n+\tgrep \"^ e\" output &&\n+\tgrep \"^ j\" output &&\n+\t! grep \"^ k\" output\n+'\n+\n+test_expect_success 'diff.context honored by \"log\"' '\n \tgit log -1 -p >output &&\n \t! grep firstline output &&\n \tgit config diff.context 8 &&\n \tgit log -1 -p >output &&\n-\tgrep firstline output\n+\tgrep \"^ firstline\" output\n '\n \n-test_expect_success 'different -U value' '\n+test_expect_success 'The -U option overrides diff.context' '\n \tgit config diff.context 8 &&\n \tgit log -U4 -1 >output &&\n-\t! grep firstline output\n+\t! grep \"^ firstline\" output\n '\n \n-test_expect_success 'diff.context affects diff' '\n+test_expect_success 'diff.context honored by \"diff\"' '\n \tgit config diff.context 8 &&\n \tgit diff >output &&\n-\tgrep firstline output\n+\tgrep \"^ firstline\" output\n '\n \n test_expect_success 'plumbing not affected' '\n \tgit config diff.context 8 &&\n-\tgit diff-files -p > output &&\n-\t! grep firstline output\n+\tgit diff-files -p >output &&\n+\t! grep \"^ firstline\" output\n '\n+\n test_expect_success 'non-integer config parsing' '\n-\tcat > .git/config <<-\\EOF &&\n-[diff]\n-\tcontext = no\n-EOF\n-\t! git diff 2>output &&\n-\tgrep \"bad config value\" output\n+\tgit config diff.context no &&\n+\ttest_must_fail git diff 2>output &&\n+\ttest_i18ngrep \"bad config value\" output\n '\n \n test_expect_success 'negative integer config parsing' '\n-\tcat >.git/config <<-\\EOF &&\n-[diff]\n-\tcontext = -1\n-EOF\n-\t! git diff 2>output &&\n-\tgrep \"bad config file\" output\n-'\n-\n-test_expect_success '0 config parsing' '\n-\tcat > .git/config <<-\\EOF &&\n-[diff]\n-\tcontext = 0\n-EOF\n-\tgit diff >output &&\n-\tgrep preline output\n+\tgit config diff.context -1 &&\n+\ttest_must_fail git diff 2>output &&\n+\ttest_i18ngrep \"bad config file\" output\n '\n \n-test_expect_success 'config parsing' '\n-\tcat >.git/config <<-\\EOF &&\n-[diff]\n-\tcontext = 8\n-EOF\n+test_expect_success '-U0 is valid, so is diff.context=0' '\n+\tgit config diff.context 0 &&\n \tgit diff >output &&\n-\tgrep postline output\n+\tgrep \"^-ADDED\" output &&\n+\tgrep \"^+MODIFIED\" output\n '\n \n test_done\n"},{"id":"201570","messageId":"B3AF2A63-5243-43CF-ADCB-6F40C474F9B1@mozilla.com","threadId":"31534","inReplyTo":"7v3923p0tb.fsf@alter.siamese.dyndns.org","subject":"[PATCH] diff: diff.context configuration gives default to -U","fromName":"Jeff Muizelaar","fromEmail":"jmuizelaar@mozilla.com","sentAt":"2012-10-19T20:54:55Z","receivedAt":"2012-10-19T20:54:55Z","isPatch":true,"sender":{"key":"jmuizelaar@mozilla.com","avatar":"https://avatars.githubusercontent.com/u/332653?v=4"},"body":"Introduce a configuration variable diff.context that tells\nPorcelain commands to use a non-default number of context\nlines instead of 3 (the default).  With this variable, users\ndo not have to keep repeating \"git log -U8\" from the command\nline; instead, it becomes sufficient to say \"git config\ndiff.context 8\" just once.\n\nSigned-off-by: Jeff Muizelaar <jmuizelaar@mozilla.com>\n---\nOn 2012-09-27, at 6:18 PM, Junio C Hamano wrote:\n\n> Jeff Muizelaar <jmuizelaar@mozilla.com> writes:\n> \n>> +\tif (!strcmp(var, \"diff.context\")) {\n>> +\t\tdiff_context_default = git_config_int(var, value);\n>> +\t\tif (diff_context_default < 0)\n>> +\t\t\treturn -1;\n>> +\t\treturn 0;\n> \n> I am somewhat torn on this part. This fails the entire command when\n> diff.context is set to non integer or negative integer, which means\n> trouble for a user of a future version of git that accepts such a\n> value to do something intelligent we do not anticipate today. The\n> useful configuration value cannot be given unless the user is\n> certain that .gitconfig file will never be read by older version of\n> git.\n> \n> Perhaps it is OK, at least for now.  We'd have the same worry for\n> what is given to -U<n> anyway.\n\nI left this as is.\n\n>> +test_expect_success 'diff.context affects log' '\n>> +\tgit log -1 -p >output &&\n>> +\t! grep firstline output &&\n>> +\tgit config diff.context 8 &&\n>> +\tgit log -1 -p >output &&\n>> +\tgrep firstline output\n>> +'\n> \n> Is there a reason to favor \"log -1 -p\" over something a lot simpler\n> like \"show\"?  Not requesting to change anything, but just being\n> curious.\n\nNo. I changed it to \"show\" to avoid the surprise.\n\nOther than that I included the test changes you suggested. I did however\nadjust the sed 'a' command. It seems that GNU sed and BSD sed have incompatible behaviour\nhere. Unfortunately this also seems to screw up the indentation.\n\n---\n Documentation/diff-config.txt |    4 ++\n diff.c                        |    9 ++++-\n t/t4055-diff-context.sh       |   94 +++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 106 insertions(+), 1 deletions(-)\n create mode 100755 t/t4055-diff-context.sh\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 67a90a8..75ab8a5 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -56,6 +56,10 @@ diff.statGraphWidth::\n \tLimit the width of the graph part in --stat output. If set, applies\n \tto all commands generating --stat output except format-patch.\n \n+diff.context::\n+\tGenerate diffs with <n> lines of context instead of the default of\n+\t3. This value is overridden by the -U option.\n+\n diff.external::\n \tIf this config variable is set, diff generation is not\n \tperformed using the internal diff machinery, but using the\ndiff --git a/diff.c b/diff.c\nindex 35d3f07..86e5f2a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -26,6 +26,7 @@ static int diff_detect_rename_default;\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\n+static int diff_context_default = 3;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n int diff_auto_refresh_index = 1;\n@@ -141,6 +142,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\tdiff_use_color_default = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"diff.context\")) {\n+\t\tdiff_context_default = git_config_int(var, value);\n+\t\tif (diff_context_default < 0)\n+\t\t\treturn -1;\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"diff.renames\")) {\n \t\tdiff_detect_rename_default = git_config_rename(var, value);\n \t\treturn 0;\n@@ -3170,7 +3177,7 @@ void diff_setup(struct diff_options *options)\n \toptions->break_opt = -1;\n \toptions->rename_limit = -1;\n \toptions->dirstat_permille = diff_dirstat_permille_default;\n-\toptions->context = 3;\n+\toptions->context = diff_context_default;\n \tDIFF_OPT_SET(options, RENAME_EMPTY);\n \n \toptions->change = diff_change;\ndiff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh\nnew file mode 100755\nindex 0000000..60348b7\n--- /dev/null\n+++ b/t/t4055-diff-context.sh\n@@ -0,0 +1,94 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2012 Mozilla Foundation\n+#\n+\n+test_description='diff.context configuration'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tcat >x <<-\\EOF &&\n+\tfirstline\n+\tb\n+\tc\n+\td\n+\te\n+\tf\n+\tpreline\n+\tpostline\n+\ti\n+\tj\n+\tk\n+\tl\n+\tm\n+\tn\n+\tEOF\n+\tgit update-index --add x &&\n+\tgit commit -m initial &&\n+\n+\tgit cat-file blob HEAD:x |\n+\tsed \"s/preline/preline\\\\\n+ADDED/\" >x &&\n+\tgit update-index --add x &&\n+\tgit commit -m next &&\n+\n+\tgit cat-file blob HEAD:x |\n+\tsed s/ADDED/MODIFIED/ >x\n+'\n+\n+test_expect_success 'the default number of context lines is 3' '\n+\tgit diff >output &&\n+\t! grep \"^ d\" output &&\n+\tgrep \"^ e\" output &&\n+\tgrep \"^ j\" output &&\n+\t! grep \"^ k\" output\n+'\n+\n+test_expect_success 'diff.context honored by \"show\"' '\n+\tgit show >output &&\n+\t! grep firstline output &&\n+\tgit config diff.context 8 &&\n+\tgit show >output &&\n+\tgrep \"^ firstline\" output\n+'\n+\n+test_expect_success 'The -U option overrides diff.context' '\n+\tgit config diff.context 8 &&\n+\tgit log -U4 -1 >output &&\n+\t! grep \"^ firstline\" output\n+'\n+\n+test_expect_success 'diff.context honored by \"diff\"' '\n+\tgit config diff.context 8 &&\n+\tgit diff >output &&\n+\tgrep \"^ firstline\" output\n+'\n+\n+test_expect_success 'plumbing not affected' '\n+\tgit config diff.context 8 &&\n+\tgit diff-files -p >output &&\n+\t! grep \"^ firstline\" output\n+'\n+\n+test_expect_success 'non-integer config parsing' '\n+\tgit config diff.context no &&\n+\ttest_must_fail git diff 2>output &&\n+\ttest_i18ngrep \"bad config value\" output\n+'\n+\n+test_expect_success 'negative integer config parsing' '\n+\tgit config diff.context -1 &&\n+\ttest_must_fail git diff 2>output &&\n+\ttest_i18ngrep \"bad config file\" output\n+'\n+\n+test_expect_success '-U0 is valid, so is diff.context=0' '\n+\tgit config diff.context 0 &&\n+\tgit diff >output &&\n+\tgrep \"^-ADDED\" output &&\n+\tgrep \"^+MODIFIED\" output &&\n+\t! grep \"^ postline\" output\n+'\n+\n+test_done\n-- \n1.7.4.4\n"}]}