{"thread":{"id":"51478","subject":"[PATCH v2 0/3] document deprecation of log.mailmap=false default","startedAt":"2019-07-12T23:02:13Z","lastAt":"2019-07-14T21:56:50Z","messageCount":6,"participants":["Ariadne Conill","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"378911","messageId":"20190712230204.16749-1-ariadne@dereferenced.org","threadId":"51478","inReplyTo":null,"subject":"[PATCH v2 0/3] document deprecation of log.mailmap=false default","fromName":"Ariadne Conill","fromEmail":"ariadne@dereferenced.org","sentAt":"2019-07-12T23:02:01Z","receivedAt":"2019-07-12T23:02:13Z","isPatch":true,"sender":{"key":"ariadne@dereferenced.org","avatar":"https://avatars.githubusercontent.com/u/1522444?v=4"},"body":"Based on the discussion of the previous patches, we concluded that\nchanging the default will require a transitional period.\n\nAs such, I have introduced a deprecation warning that is printed when\nthe log builtin commands are used.\n\nThanks to Junio and everyone else for providing feedback on how to\nproceed.\n\nNew in version 2:\n- The warning is disabled when `--format` is used.\n- The warning is disabled when not called from a controlling terminal.\n- Tests which fake a controlling terminal have been defanged.\n\nAriadne Conill (3):\n  log: add warning for unspecified log.mailmap setting\n  documentation: mention --no-use-mailmap and log.mailmap false setting\n  tests: defang pager tests by explicitly disabling the log.mailmap\n    warning\n\n Documentation/config/log.txt |  3 ++-\n Documentation/git-log.txt    |  2 +-\n builtin/log.c                | 25 ++++++++++++++++++++++++-\n t/t7006-pager.sh             | 10 ++++++++++\n 4 files changed, 37 insertions(+), 3 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"378912","messageId":"20190712230204.16749-2-ariadne@dereferenced.org","threadId":"51478","inReplyTo":"20190712230204.16749-1-ariadne@dereferenced.org","subject":"[PATCH v2 1/3] log: add warning for unspecified log.mailmap setting","fromName":"Ariadne Conill","fromEmail":"ariadne@dereferenced.org","sentAt":"2019-07-12T23:02:02Z","receivedAt":"2019-07-12T23:02:13Z","isPatch":true,"sender":{"key":"ariadne@dereferenced.org","avatar":"https://avatars.githubusercontent.com/u/1522444?v=4"},"body":"Based on discussions around changing the log.mailmap default to being\nenabled, it was decided that a transitional period is required.\n\nAccordingly, we announce this transitional period with a warning\nmessage.\n\nSigned-off-by: Ariadne Conill <ariadne@dereferenced.org>\n---\n builtin/log.c | 25 ++++++++++++++++++++++++-\n 1 file changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 7c8767d3bc..559f42fe48 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -47,7 +47,7 @@ static int default_follow;\n static int default_show_signature;\n static int decoration_style;\n static int decoration_given;\n-static int use_mailmap_config;\n+static int use_mailmap_config = -1;\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n static const char *fmt_pretty;\n \n@@ -151,6 +151,16 @@ static void cmd_log_init_defaults(struct rev_info *rev)\n \t\tparse_date_format(default_date_mode, &rev->date_mode);\n }\n \n+static char warn_unspecified_mailmap_msg[] =\n+N_(\"log.mailmap is not set; its implicit value will change in an\\n\"\n+   \"upcoming release. To squelch this message and preserve current\\n\"\n+   \"behaviour, set the log.mailmap configuration value to false.\\n\"\n+   \"\\n\"\n+   \"To squelch this message and adopt the new behaviour now, set the\\n\"\n+   \"log.mailmap configuration value to true.\\n\"\n+   \"\\n\"\n+   \"See 'git help config' and search for 'log.mailmap' for further information.\");\n+\n static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\t\t struct rev_info *rev, struct setup_revision_opt *opt)\n {\n@@ -199,6 +209,19 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \tmemset(&w, 0, sizeof(w));\n \tuserformat_find_requirements(NULL, &w);\n \n+\tif (mailmap < 0) {\n+\t\t/*\n+\t\t * Only display the warning if the session is interactive\n+\t\t * and pretty_given is false. We determine that the session\n+\t\t * is interactive by checking if auto_decoration_style()\n+\t\t * returns non-zero.\n+\t\t */\n+\t\tif (auto_decoration_style() && !rev->pretty_given)\n+\t\t\twarning(\"%s\\n\", _(warn_unspecified_mailmap_msg));\n+\n+\t\tmailmap = 0;\n+\t}\n+\n \tif (!rev->show_notes_given && (!rev->pretty_given || w.notes))\n \t\trev->show_notes = 1;\n \tif (rev->show_notes)\n-- \n2.17.1\n\n"},{"id":"378913","messageId":"20190712230204.16749-3-ariadne@dereferenced.org","threadId":"51478","inReplyTo":"20190712230204.16749-1-ariadne@dereferenced.org","subject":"[PATCH v2 2/3] documentation: mention --no-use-mailmap and log.mailmap false setting","fromName":"Ariadne Conill","fromEmail":"ariadne@dereferenced.org","sentAt":"2019-07-12T23:02:03Z","receivedAt":"2019-07-12T23:02:15Z","isPatch":true,"sender":{"key":"ariadne@dereferenced.org","avatar":"https://avatars.githubusercontent.com/u/1522444?v=4"},"body":"The log.mailmap setting may be explicitly set to false, which disables\nthe mailmap feature implicity. In practice, doing so is equivalent to\nalways using the previously undocumented --no-use-mailmap option on the\ncommand line.\n\nAccordingly, we document both the existence of --no-use-mailmap as\nwell as briefly discuss the equivalence of it to log.mailmap=False.\n\nSigned-off-by: Ariadne Conill <ariadne@dereferenced.org>\n---\n Documentation/config/log.txt | 3 ++-\n Documentation/git-log.txt    | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\nindex 78d9e4453a..7798e10cb0 100644\n--- a/Documentation/config/log.txt\n+++ b/Documentation/config/log.txt\n@@ -40,4 +40,5 @@ log.showSignature::\n \n log.mailmap::\n \tIf true, makes linkgit:git-log[1], linkgit:git-show[1], and\n-\tlinkgit:git-whatchanged[1] assume `--use-mailmap`.\n+\tlinkgit:git-whatchanged[1] assume `--use-mailmap`, otherwise\n+\tassume `--no-use-mailmap`. False by default.\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex b02e922dc3..b406bc4c48 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -49,7 +49,7 @@ OPTIONS\n \tPrint out the ref name given on the command line by which each\n \tcommit was reached.\n \n---use-mailmap::\n+--[no-]use-mailmap::\n \tUse mailmap file to map author and committer names and email\n \taddresses to canonical real names and email addresses. See\n \tlinkgit:git-shortlog[1].\n-- \n2.17.1\n\n"},{"id":"378914","messageId":"20190712230204.16749-4-ariadne@dereferenced.org","threadId":"51478","inReplyTo":"20190712230204.16749-1-ariadne@dereferenced.org","subject":"[PATCH v2 3/3] tests: defang pager tests by explicitly disabling the log.mailmap warning","fromName":"Ariadne Conill","fromEmail":"ariadne@dereferenced.org","sentAt":"2019-07-12T23:02:04Z","receivedAt":"2019-07-12T23:02:16Z","isPatch":true,"sender":{"key":"ariadne@dereferenced.org","avatar":"https://avatars.githubusercontent.com/u/1522444?v=4"},"body":"In the previous patch, we added a deprecation warning for the current\nlog.mailmap setting. This warning only appears when git is attached to\na controlling terminal. Some tests however run under an emulated\nterminal, so we need to disable the warning for those tests.\n\nSigned-off-by: Ariadne Conill <ariadne@dereferenced.org>\n---\n t/t7006-pager.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 00e09a375c..1c72aae197 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -304,6 +304,7 @@ test_expect_success 'tests can detect color' '\n \n test_expect_success 'no color when stdout is a regular file' '\n \trm -f colorless.log &&\n+\ttest_config log.mailmap false &&\n \ttest_config color.ui auto &&\n \tgit log >colorless.log &&\n \t! colorful colorless.log\n@@ -311,6 +312,7 @@ test_expect_success 'no color when stdout is a regular file' '\n \n test_expect_success TTY 'color when writing to a pager' '\n \trm -f paginated.out &&\n+\ttest_config log.mailmap false &&\n \ttest_config color.ui auto &&\n \ttest_terminal git log &&\n \tcolorful paginated.out\n@@ -318,6 +320,7 @@ test_expect_success TTY 'color when writing to a pager' '\n \n test_expect_success TTY 'colors are suppressed by color.pager' '\n \trm -f paginated.out &&\n+\ttest_config log.mailmap false &&\n \ttest_config color.ui auto &&\n \ttest_config color.pager false &&\n \ttest_terminal git log &&\n@@ -326,6 +329,7 @@ test_expect_success TTY 'colors are suppressed by color.pager' '\n \n test_expect_success 'color when writing to a file intended for a pager' '\n \trm -f colorful.log &&\n+\ttest_config log.mailmap false &&\n \ttest_config color.ui auto &&\n \t(\n \t\tTERM=vt100 &&\n@@ -337,6 +341,7 @@ test_expect_success 'color when writing to a file intended for a pager' '\n '\n \n test_expect_success TTY 'colors are sent to pager for external commands' '\n+\ttest_config log.mailmap false &&\n \ttest_config alias.externallog \"!git log\" &&\n \ttest_config color.ui auto &&\n \ttest_terminal git -p externallog &&\n@@ -573,6 +578,7 @@ test_expect_success TTY 'command-specific pager' '\n \tsane_unset PAGER GIT_PAGER &&\n \techo \"foo:initial\" >expect &&\n \t>actual &&\n+\ttest_config log.mailmap false &&\n \ttest_unconfig core.pager &&\n \ttest_config pager.log \"sed s/^/foo:/ >actual\" &&\n \ttest_terminal git log --format=%s -1 &&\n@@ -583,6 +589,7 @@ test_expect_success TTY 'command-specific pager overrides core.pager' '\n \tsane_unset PAGER GIT_PAGER &&\n \techo \"foo:initial\" >expect &&\n \t>actual &&\n+\ttest_config log.mailmap false &&\n \ttest_config core.pager \"exit 1\" &&\n \ttest_config pager.log \"sed s/^/foo:/ >actual\" &&\n \ttest_terminal git log --format=%s -1 &&\n@@ -593,6 +600,7 @@ test_expect_success TTY 'command-specific pager overridden by environment' '\n \tGIT_PAGER=\"sed s/^/foo:/ >actual\" && export GIT_PAGER &&\n \t>actual &&\n \techo \"foo:initial\" >expect &&\n+\ttest_config log.mailmap false &&\n \ttest_config pager.log \"exit 1\" &&\n \ttest_terminal git log --format=%s -1 &&\n \ttest_cmp expect actual\n@@ -610,6 +618,7 @@ test_expect_success TTY 'command-specific pager works for external commands' '\n \tsane_unset PAGER GIT_PAGER &&\n \techo \"foo:initial\" >expect &&\n \t>actual &&\n+\ttest_config log.mailmap false &&\n \ttest_config pager.external \"sed s/^/foo:/ >actual\" &&\n \ttest_terminal git --exec-path=\"$(pwd)\" external log --format=%s -1 &&\n \ttest_cmp expect actual\n@@ -619,6 +628,7 @@ test_expect_success TTY 'sub-commands of externals use their own pager' '\n \tsane_unset PAGER GIT_PAGER &&\n \techo \"foo:initial\" >expect &&\n \t>actual &&\n+\ttest_config log.mailmap false &&\n \ttest_config pager.log \"sed s/^/foo:/ >actual\" &&\n \ttest_terminal git --exec-path=. external log --format=%s -1 &&\n \ttest_cmp expect actual\n-- \n2.17.1\n\n"},{"id":"378935","messageId":"xmqqzhlg46yg.fsf@gitster-ct.c.googlers.com","threadId":"51478","inReplyTo":"20190712230204.16749-2-ariadne@dereferenced.org","subject":"Re: [PATCH v2 1/3] log: add warning for unspecified log.mailmap setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-14T21:55:19Z","receivedAt":"2019-07-14T21:55:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ariadne Conill <ariadne@dereferenced.org> writes:\n\n> +\tif (mailmap < 0) {\n> +\t\t/*\n> +\t\t * Only display the warning if the session is interactive\n> +\t\t * and pretty_given is false. We determine that the session\n> +\t\t * is interactive by checking if auto_decoration_style()\n> +\t\t * returns non-zero.\n> +\t\t */\n> +\t\tif (auto_decoration_style() && !rev->pretty_given)\n> +\t\t\twarning(\"%s\\n\", _(warn_unspecified_mailmap_msg));\n\nThe huge comment can go if you refactored the helper function a\nlittle bit and will give us a much better better organization.\n\nstatic int auto_decoration_style(void)\n{\n\treturn (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;\n}\n\nThe existing helper is meant to help those who are interested in the\ndecoration feature, and the fact that it kicks in by default when\nthe condition (isatty(1) || pager_in_use()) is true is a mere\n\"decoration feature happens to be designed that way right now\".\nThere is no logical reason to expect that the decoration feature and\nmailmap feature's advicse messages will be triggered by the same\ncondition forever.\n\nThink a bit and what the condition \"means\".  You wrote a good one\nyourself above: \"the session is interactive\".  Introduce a helper\nthat checks exatly that, by reusing what auto_decoration_style()\nalready uses. i.e.\n\n\tstatic int session_is_interactive(void)\n\t{\n\t\treturn isatty(1) || pager_in_use();\n\t}\n\n\tstatic int auto_decoration_style(void)\n\t{\n\t\treturn session_is_interactive() ? DECORATE_SHORT_REFS : 0;\n\t}\n\nand then the above hunk becomes\n\n\tif (session_is_interactive() && !rev->pretty_given)\n\t\twarning(...);\n\nIt is clear enough and there is no need for your 2 sentence comment,\nas (1) the first sentence is exactly what the implementation is, and\n(2) we no longer abuse auto_decoration_style() outside its intended\npurpose.\n\n\n\n\n"},{"id":"378936","messageId":"xmqqy31046w1.fsf@gitster-ct.c.googlers.com","threadId":"51478","inReplyTo":"20190712230204.16749-4-ariadne@dereferenced.org","subject":"Re: [PATCH v2 3/3] tests: defang pager tests by explicitly disabling the log.mailmap warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-14T21:56:46Z","receivedAt":"2019-07-14T21:56:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ariadne Conill <ariadne@dereferenced.org> writes:\n\n> In the previous patch, we added a deprecation warning for the current\n> log.mailmap setting. This warning only appears when git is attached to\n> a controlling terminal. Some tests however run under an emulated\n> terminal, so we need to disable the warning for those tests.\n>\n> Signed-off-by: Ariadne Conill <ariadne@dereferenced.org>\n> ---\n>  t/t7006-pager.sh | 10 ++++++++++\n>  1 file changed, 10 insertions(+)\n\nHmm, this is horrible.  \n\nThese tests are primarily to see how use of color gets affected by\nvarious configuration and the use of the pager, and having to\nsprinkle log.mailmap configuration to just randomly selected 10\ntests among 50+ tests in the script makes readers wonder if the\nconfiguration has anything to do with the coloring (answer: no).\n\nThe primary reason why other 40+ tests do not need log.mailmap\ntweaked is not because log.mailmap does not affect the coloring.\nBut for a new developer who will be adding a new test to this file,\nhow would s/he decide if the new test needs log.mailmap=false like\nthese 10, or it is like the other 40+?\n\nIt almost makes me feel that it would be much better to just disable\nthe warning inside the setup part, perhaps like\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 00e09a375c..283de499fc 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -7,6 +7,8 @@ test_description='Test automatic use of a pager.'\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n test_expect_success 'setup' '\n+\t: squelch advise messages during the transition &&\n+\tgit config --global log.mailmap false &&\n \tsane_unset GIT_PAGER GIT_PAGER_IN_USE &&\n \ttest_unconfig core.pager &&\n \n"}]}