{"thread":{"id":"51474","subject":"[PATCH v1 0/2] document deprecation of log.mailmap=false default","startedAt":"2019-07-12T15:59:11Z","lastAt":"2019-07-12T19:30:13Z","messageCount":6,"participants":["Ariadne Conill","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"378889","messageId":"20190712155901.19210-1-ariadne@dereferenced.org","threadId":"51474","inReplyTo":null,"subject":"[PATCH v1 0/2] document deprecation of log.mailmap=false default","fromName":"Ariadne Conill","fromEmail":"ariadne@dereferenced.org","sentAt":"2019-07-12T15:58:59Z","receivedAt":"2019-07-12T15:59:11Z","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\nAriadne Conill (2):\n  log: add warning for unspecified log.mailmap setting\n  documentation: mention --no-use-mailmap and log.mailmap false setting\n\n Documentation/config/log.txt |  3 ++-\n Documentation/git-log.txt    |  2 +-\n builtin/log.c                | 17 ++++++++++++++++-\n 3 files changed, 19 insertions(+), 3 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"378890","messageId":"20190712155901.19210-2-ariadne@dereferenced.org","threadId":"51474","inReplyTo":"20190712155901.19210-1-ariadne@dereferenced.org","subject":"[PATCH v1 1/2] log: add warning for unspecified log.mailmap setting","fromName":"Ariadne Conill","fromEmail":"ariadne@dereferenced.org","sentAt":"2019-07-12T15:59:00Z","receivedAt":"2019-07-12T15:59: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 | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 7c8767d3bc..5ee150be04 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@@ -188,6 +198,11 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\t\t     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |\n \t\t\t     PARSE_OPT_KEEP_DASHDASH);\n \n+\tif (mailmap < 0) {\n+\t\twarning(\"%s\\n\", _(warn_unspecified_mailmap_msg));\n+\t\tmailmap = 0;\n+\t}\n+\n \tif (quiet)\n \t\trev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;\n \targc = setup_revisions(argc, argv, rev, opt);\n-- \n2.17.1\n\n"},{"id":"378891","messageId":"20190712155901.19210-3-ariadne@dereferenced.org","threadId":"51474","inReplyTo":"20190712155901.19210-1-ariadne@dereferenced.org","subject":"[PATCH v1 2/2] documentation: mention --no-use-mailmap and log.mailmap false setting","fromName":"Ariadne Conill","fromEmail":"ariadne@dereferenced.org","sentAt":"2019-07-12T15:59:01Z","receivedAt":"2019-07-12T15:59: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":"378897","messageId":"xmqqpnmfp2gs.fsf@gitster-ct.c.googlers.com","threadId":"51474","inReplyTo":"20190712155901.19210-1-ariadne@dereferenced.org","subject":"Re: [PATCH v1 0/2] document deprecation of log.mailmap=false default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-12T17:49:55Z","receivedAt":"2019-07-12T17:50:03Z","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> Based on the discussion of the previous patches, we concluded that\n> changing the default will require a transitional period.\n\nOK.\n\n> As such, I have introduced a deprecation warning that is printed when\n> the log builtin commands are used.\n\n... used when the user did not configure and did not give --[no-]use-mailmap\noption from the command line, right?  Otherwise the warning would be annoying.\n\nHave you run the test suite before posting?  To me, at least t0203,\nt4212, t7006 and t9700 seem to be having trouble with this change,\nthat may need adjusting.\n\n0203.1 expects that \"git show\" gives nothing to its standard error\nstream.  Perhaps something like this should serve as a good\nworkaround.\n\ndiff --git a/t/t0203-gettext-setlocale-sanity.sh b/t/t0203-gettext-setlocale-sanity.sh\nindex 0ce1f22eff..76f774c8b8 100755\n--- a/t/t0203-gettext-setlocale-sanity.sh\n+++ b/t/t0203-gettext-setlocale-sanity.sh\n@@ -10,7 +10,7 @@ test_description=\"The Git C functions aren't broken by setlocale(3)\"\n test_expect_success 'git show a ISO-8859-1 commit under C locale' '\n \t. \"$TEST_DIRECTORY\"/t3901/8859-1.txt &&\n \ttest_commit \"iso-c-commit\" iso-under-c &&\n-\tgit show >out 2>err &&\n+\tgit show --use-mailmap >out 2>err &&\n \ttest_must_be_empty err &&\n \tgrep -q \"iso-c-commit\" out\n '\n\n\n4212.3 is the same way.\n\n4212.4 raises an interesting question.  It wants to see the output\nof this command:\n\n\tgit log --format=\"%an+%ae+%ad\" broken_email >actual.out 2>actual.err &&\n\nThe question is, when the user explicitly asked for the \"true\"\nidentity values (not the mapped one via %aN, %aE and their friends),\nif --use-mailmap should affect the outcome?\n\nA secondary question is if we should be issuing a warning against\nthis command line in the first place, if the answer to the above\nquestion is \"no\" (i.e. --[no-]use-mailmap, and the future default\nswitch, do not affect the outcome).  There is no point issuing a\nwarning if the command line is already future-proofed.\n\n7006.43 (among others) expects its \"colorful\" shell helper function\nto be a reliable way to detect if color escape sequence is in use in\nthe output but the helper only reads the first line and expects it\nto always begin with the colored \"commit 0c9f6db7183a...\", which fails\nwhen an error or a warning message comes first.  I think the best fix\nthere should be to make the helper more robust, perhaps by borrowing\nideas from test-lib-functions.sh::test_decode_color, i.e. find byte\nsequence \"\\033[m\" in there (I think assuming the presence of RESET\nsequence alone is sufficient for the purpose of t7006 tests that\nwants to see \"have we tried to color or emit non-colored output?\".\n\nA possible workaround for 9007 would be to set log.mailmap=true in\nthe set-up step, but there may be a better alternative.\n\ndiff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\nindex 102c133112..a7a336947d 100755\n--- a/t/t9700-perl-git.sh\n+++ b/t/t9700-perl-git.sh\n@@ -36,6 +36,7 @@ test_expect_success \\\n      echo \"changed file 1\" > file1 &&\n      git commit -a -m \"second commit\" &&\n \n+     git config log.mailmap true &&\n      git config --add color.test.slot1 green &&\n      git config --add test.string value &&\n      git config --add test.dupstring value1 &&\n\n\nThere may be issues with other tests.  I didn't do any exhaustive\nvetting.\n\nThanks.\n"},{"id":"378898","messageId":"CAAOiGNzcNhWmmr+COXhv2p9=KF5k4hHLEsNsfA1H+P0JQOTTqg@mail.gmail.com","threadId":"51474","inReplyTo":"xmqqpnmfp2gs.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 0/2] document deprecation of log.mailmap=false default","fromName":"Ariadne Conill","fromEmail":"ariadne@dereferenced.org","sentAt":"2019-07-12T18:00:51Z","receivedAt":"2019-07-12T18:01:04Z","isPatch":true,"sender":{"key":"ariadne@dereferenced.org","avatar":"https://avatars.githubusercontent.com/u/1522444?v=4"},"body":"Hello,\n\nOn Fri, Jul 12, 2019 at 12:50 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Ariadne Conill <ariadne@dereferenced.org> writes:\n>\n> > Based on the discussion of the previous patches, we concluded that\n> > changing the default will require a transitional period.\n>\n> OK.\n>\n> > As such, I have introduced a deprecation warning that is printed when\n> > the log builtin commands are used.\n>\n> ... used when the user did not configure and did not give --[no-]use-mailmap\n> option from the command line, right?  Otherwise the warning would be annoying.\n\nYes, the default state is changed to -1, which using the command line\noptions or explicitly configuring the option would change to either 0\nor 1.\n\n> Have you run the test suite before posting?  To me, at least t0203,\n> t4212, t7006 and t9700 seem to be having trouble with this change,\n> that may need adjusting.\n\nI ran parts of the testsuite, but I can go through the whole thing and\nwill fix the fallout.\n\n> 0203.1 expects that \"git show\" gives nothing to its standard error\n> stream.  Perhaps something like this should serve as a good\n> workaround.\n>\n> diff --git a/t/t0203-gettext-setlocale-sanity.sh b/t/t0203-gettext-setlocale-sanity.sh\n> index 0ce1f22eff..76f774c8b8 100755\n> --- a/t/t0203-gettext-setlocale-sanity.sh\n> +++ b/t/t0203-gettext-setlocale-sanity.sh\n> @@ -10,7 +10,7 @@ test_description=\"The Git C functions aren't broken by setlocale(3)\"\n>  test_expect_success 'git show a ISO-8859-1 commit under C locale' '\n>         . \"$TEST_DIRECTORY\"/t3901/8859-1.txt &&\n>         test_commit \"iso-c-commit\" iso-under-c &&\n> -       git show >out 2>err &&\n> +       git show --use-mailmap >out 2>err &&\n>         test_must_be_empty err &&\n>         grep -q \"iso-c-commit\" out\n>  '\n>\n>\n> 4212.3 is the same way.\n>\n> 4212.4 raises an interesting question.  It wants to see the output\n> of this command:\n>\n>         git log --format=\"%an+%ae+%ad\" broken_email >actual.out 2>actual.err &&\n>\n> The question is, when the user explicitly asked for the \"true\"\n> identity values (not the mapped one via %aN, %aE and their friends),\n> if --use-mailmap should affect the outcome?\n\nI don't think it should.\n\n> A secondary question is if we should be issuing a warning against\n> this command line in the first place, if the answer to the above\n> question is \"no\" (i.e. --[no-]use-mailmap, and the future default\n> switch, do not affect the outcome).  There is no point issuing a\n> warning if the command line is already future-proofed.\n\nI will disable the warning if a custom format is specified.\n\n> 7006.43 (among others) expects its \"colorful\" shell helper function\n> to be a reliable way to detect if color escape sequence is in use in\n> the output but the helper only reads the first line and expects it\n> to always begin with the colored \"commit 0c9f6db7183a...\", which fails\n> when an error or a warning message comes first.  I think the best fix\n> there should be to make the helper more robust, perhaps by borrowing\n> ideas from test-lib-functions.sh::test_decode_color, i.e. find byte\n> sequence \"\\033[m\" in there (I think assuming the presence of RESET\n> sequence alone is sufficient for the purpose of t7006 tests that\n> wants to see \"have we tried to color or emit non-colored output?\".\n\nThe colorful shell helper function irritates me from time to time with\nthat sort of thing.  I might look into fixing it, but I think for now\nit makes more sense to just defang those tests by killing the warning.\n\nA thought I had was to perhaps use isatty(STDERR_FILENO) to determine\nif stderr is attached to a terminal session and only issue the warning\nin that case.  What do you think?  It should fix all of these tests.\n\n> A possible workaround for 9007 would be to set log.mailmap=true in\n> the set-up step, but there may be a better alternative.\n>\n> diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\n> index 102c133112..a7a336947d 100755\n> --- a/t/t9700-perl-git.sh\n> +++ b/t/t9700-perl-git.sh\n> @@ -36,6 +36,7 @@ test_expect_success \\\n>       echo \"changed file 1\" > file1 &&\n>       git commit -a -m \"second commit\" &&\n>\n> +     git config log.mailmap true &&\n>       git config --add color.test.slot1 green &&\n>       git config --add test.string value &&\n>       git config --add test.dupstring value1 &&\n\nThis seems reasonable.\n\n> There may be issues with other tests.  I didn't do any exhaustive\n> vetting.\n\nThanks wholeheartedly for all of your help so far, I know there are\nmany git users who will be very happy once all of this work is done\n(basically anyone who has changed their name).\n\nAriadne\n"},{"id":"378907","messageId":"xmqqd0ifoxtu.fsf@gitster-ct.c.googlers.com","threadId":"51474","inReplyTo":"CAAOiGNzcNhWmmr+COXhv2p9=KF5k4hHLEsNsfA1H+P0JQOTTqg@mail.gmail.com","subject":"Re: [PATCH v1 0/2] document deprecation of log.mailmap=false default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-12T19:30:05Z","receivedAt":"2019-07-12T19:30:13Z","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>> 4212.4 raises an interesting question.  It wants to see the output\n>> of this command:\n>>\n>>         git log --format=\"%an+%ae+%ad\" broken_email >actual.out 2>actual.err &&\n>>\n>> The question is, when the user explicitly asked for the \"true\"\n>> identity values (not the mapped one via %aN, %aE and their friends),\n>> if --use-mailmap should affect the outcome?\n>\n> I don't think it should.\n>\n>> A secondary question is if we should be issuing a warning against\n>> this command line in the first place, if the answer to the above\n>> question is \"no\" (i.e. --[no-]use-mailmap, and the future default\n>> switch, do not affect the outcome).  There is no point issuing a\n>> warning if the command line is already future-proofed.\n>\n> I will disable the warning if a custom format is specified.\n\nI briefly thought that this is tricky enough that we should wait for\nother people to chime in before casting the decision in stone.  But\nwe can continue with what we consider the better variant in the\nmeantime.\n\n> A thought I had was to perhaps use isatty(STDERR_FILENO) to determine\n> if stderr is attached to a terminal session and only issue the warning\n> in that case.  What do you think?  It should fix all of these tests.\n\nSome of them but it should not affect that one, which is run under\ntest_terminal, as the whole point of that test helper is to see how\nwe behave in a terminal ;-)\n\n> Thanks wholeheartedly for all of your help so far, I know there are\n> many git users who will be very happy once all of this work is done\n> (basically anyone who has changed their name).\n\nThanks.\n"}]}