Re: Git for Windows v2.23.0-rc0, was Re: [ANNOUNCE] Git v2.23.0-rc0

4 messages, 4 authors, 2019-08-02 · open the first message on its own page

Re: Git for Windows v2.23.0-rc0, was Re: [ANNOUNCE] Git v2.23.0-rc0

From: Junio C Hamano <hidden>
Date: 2019-08-01 15:46:03

Jonathan Nieder [off-list ref] writes:

Although as Dscho mentions, it's particularly irritating because it is
not part of the paginated output.
...
Let's bite the bullet and jump straight to --use-mailmap in case (4).

While at it, add a new log.mailmap setting "auto" that can be used to
explicitly request the new automatic behavior (so that e.g. if
log.mailmap is set to "true" system-side, I can set it to "auto" in my
per-user configuration).

While I think "revert to hardcoded default" may be a good idea, I do
not think the hardcoded default you implemented that changes the
behaviour based on the output destination makes much sense.  If I
want to eradicate junkio@cox.net from my paged/interactive output, I
want to eradicate it also in the output piped to the script I use
for authorship stats.  

I suspect that you may have misread the "is interactive" bit in the
original; that was used only to decide if we are going to warn.

Anyway, how about this much simplified version?

-- >8 --
From: Junio C Hamano <redacted>
Date: Thu, 1 Aug 2019 08:32:44 -0700
Subject: [PATCH] log: flip the --mailmap default unconditionally

It turns out that being cautious to warn against upcoming default
change was an unpopular behaviour, and such a care can easily be
defeated by distro packagers to render it ineffective anyway.

Just flip the default, with only a mention in the release notes.

Signed-off-by: Junio C Hamano <redacted>
---
 Documentation/RelNotes/2.23.0.txt |  8 +++-----
 builtin/log.c                     | 16 +---------------
 t/t7006-pager.sh                  |  2 --
 3 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/Documentation/RelNotes/2.23.0.txt b/Documentation/RelNotes/2.23.0.txt
index 19e894a44e..6ef8f21b56 100644
--- a/Documentation/RelNotes/2.23.0.txt
+++ b/Documentation/RelNotes/2.23.0.txt

@@ -10,6 +10,9 @@ Backward compatibility note
    prerequisite patches in an unstable way, which has been updated to
    compute in a way that is compatible with "git patch-id --stable".
 
+ * The "git log" command by default behaves as if the --mailmap option
+   was given.
+
 
 UI, Workflows & Features

@@ -91,11 +94,6 @@ UI, Workflows & Features
    commit-graph files now, which allows the commit-graph files to be
    updated incrementally.
 
- * The "git log" command learns to issue a warning when log.mailmap
-   configuration is not set and --[no-]mailmap option is not used, to
-   prepare users for future versions of Git that uses the mailmap by
-   default.
-
  * "git range-diff" output has been tweaked for easier identification
    of which part of what file the patch shown is about.

diff --git a/builtin/log.c b/builtin/log.c
index 1cf9e37736..40b4cbf57d 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -156,16 +156,6 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 		parse_date_format(default_date_mode, &rev->date_mode);
 }
 
-static char warn_unspecified_mailmap_msg[] =
-N_("log.mailmap is not set; its implicit value will change in an\n"
-   "upcoming release. To squelch this message and preserve current\n"
-   "behaviour, set the log.mailmap configuration value to false.\n"
-   "\n"
-   "To squelch this message and adopt the new behaviour now, set the\n"
-   "log.mailmap configuration value to true.\n"
-   "\n"
-   "See 'git help config' and search for 'log.mailmap' for further information.");
-
 static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 			 struct rev_info *rev, struct setup_revision_opt *opt)
 {

@@ -214,12 +204,8 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 	memset(&w, 0, sizeof(w));
 	userformat_find_requirements(NULL, &w);
 
-	if (mailmap < 0) {
-		if (session_is_interactive() && !rev->pretty_given)
-			warning("%s\n", _(warn_unspecified_mailmap_msg));
-
+	if (mailmap < 0)
 		mailmap = 0;
-	}
 
 	if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
 		rev->show_notes = 1;

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 7976fa7bcc..00e09a375c 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh

@@ -7,8 +7,6 @@ test_description='Test automatic use of a pager.'
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success 'setup' '
-	: squelch advice messages during the transition &&
-	git config --global log.mailmap false &&
 	sane_unset GIT_PAGER GIT_PAGER_IN_USE &&
 	test_unconfig core.pager &&

-- 
2.23.0-rc0-144-gce799e40b3

Re: Git for Windows v2.23.0-rc0, was Re: [ANNOUNCE] Git v2.23.0-rc0

From: Ariadne Conill <hidden>
Date: 2019-08-01 16:12:16

Hello,

On Thu, Aug 1, 2019 at 10:46 AM Junio C Hamano [off-list ref] wrote:

Jonathan Nieder [off-list ref] writes:

quoted

Although as Dscho mentions, it's particularly irritating because it is
not part of the paginated output.
...
Let's bite the bullet and jump straight to --use-mailmap in case (4).

While at it, add a new log.mailmap setting "auto" that can be used to
explicitly request the new automatic behavior (so that e.g. if
log.mailmap is set to "true" system-side, I can set it to "auto" in my
per-user configuration).

While I think "revert to hardcoded default" may be a good idea, I do
not think the hardcoded default you implemented that changes the
behaviour based on the output destination makes much sense.  If I
want to eradicate junkio@cox.net from my paged/interactive output, I
want to eradicate it also in the output piped to the script I use
for authorship stats.

I suspect that you may have misread the "is interactive" bit in the
original; that was used only to decide if we are going to warn.

Anyway, how about this much simplified version?

It looks good to me.

Acked-by: Ariadne Conill <redacted>

quoted hunk

-- >8 --
From: Junio C Hamano <redacted>
Date: Thu, 1 Aug 2019 08:32:44 -0700
Subject: [PATCH] log: flip the --mailmap default unconditionally

It turns out that being cautious to warn against upcoming default
change was an unpopular behaviour, and such a care can easily be
defeated by distro packagers to render it ineffective anyway.

Just flip the default, with only a mention in the release notes.

Signed-off-by: Junio C Hamano <redacted>
---
 Documentation/RelNotes/2.23.0.txt |  8 +++-----
 builtin/log.c                     | 16 +---------------
 t/t7006-pager.sh                  |  2 --
 3 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/Documentation/RelNotes/2.23.0.txt b/Documentation/RelNotes/2.23.0.txt
index 19e894a44e..6ef8f21b56 100644
--- a/Documentation/RelNotes/2.23.0.txt
+++ b/Documentation/RelNotes/2.23.0.txt

@@ -10,6 +10,9 @@ Backward compatibility note
    prerequisite patches in an unstable way, which has been updated to
    compute in a way that is compatible with "git patch-id --stable".

+ * The "git log" command by default behaves as if the --mailmap option
+   was given.
+

 UI, Workflows & Features

@@ -91,11 +94,6 @@ UI, Workflows & Features
    commit-graph files now, which allows the commit-graph files to be
    updated incrementally.

- * The "git log" command learns to issue a warning when log.mailmap
-   configuration is not set and --[no-]mailmap option is not used, to
-   prepare users for future versions of Git that uses the mailmap by
-   default.
-
  * "git range-diff" output has been tweaked for easier identification
    of which part of what file the patch shown is about.

diff --git a/builtin/log.c b/builtin/log.c
index 1cf9e37736..40b4cbf57d 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -156,16 +156,6 @@ static void cmd_log_init_defaults(struct rev_info *rev)
                parse_date_format(default_date_mode, &rev->date_mode);
 }

-static char warn_unspecified_mailmap_msg[] =
-N_("log.mailmap is not set; its implicit value will change in an\n"
-   "upcoming release. To squelch this message and preserve current\n"
-   "behaviour, set the log.mailmap configuration value to false.\n"
-   "\n"
-   "To squelch this message and adopt the new behaviour now, set the\n"
-   "log.mailmap configuration value to true.\n"
-   "\n"
-   "See 'git help config' and search for 'log.mailmap' for further information.");
-
 static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
                         struct rev_info *rev, struct setup_revision_opt *opt)
 {

@@ -214,12 +204,8 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
        memset(&w, 0, sizeof(w));
        userformat_find_requirements(NULL, &w);

-       if (mailmap < 0) {
-               if (session_is_interactive() && !rev->pretty_given)
-                       warning("%s\n", _(warn_unspecified_mailmap_msg));
-
+       if (mailmap < 0)
                mailmap = 0;
-       }

        if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
                rev->show_notes = 1;

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 7976fa7bcc..00e09a375c 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh

@@ -7,8 +7,6 @@ test_description='Test automatic use of a pager.'
 . "$TEST_DIRECTORY"/lib-terminal.sh

 test_expect_success 'setup' '
-       : squelch advice messages during the transition &&
-       git config --global log.mailmap false &&
        sane_unset GIT_PAGER GIT_PAGER_IN_USE &&
        test_unconfig core.pager &&

--

2.23.0-rc0-144-gce799e40b3

Re: Git for Windows v2.23.0-rc0, was Re: [ANNOUNCE] Git v2.23.0-rc0

From: Jeff King <hidden>
Date: 2019-08-01 21:36:41

On Thu, Aug 01, 2019 at 08:45:54AM -0700, Junio C Hamano wrote:

While I think "revert to hardcoded default" may be a good idea, I do
not think the hardcoded default you implemented that changes the
behaviour based on the output destination makes much sense.  If I
want to eradicate junkio@cox.net from my paged/interactive output, I
want to eradicate it also in the output piped to the script I use
for authorship stats.  

I suspect that you may have misread the "is interactive" bit in the
original; that was used only to decide if we are going to warn.

Yeah, I wondered about the same misreading when I read the response in:

  https://public-inbox.org/git/20190801011448.GB176307@google.com/

Anyway, how about this much simplified version?

Yes, this is what I was thinking of in my earlier responses, but...

quoted hunk

@@ -214,12 +204,8 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 	memset(&w, 0, sizeof(w));
 	userformat_find_requirements(NULL, &w);
 
-	if (mailmap < 0) {
-		if (session_is_interactive() && !rev->pretty_given)
-			warning("%s\n", _(warn_unspecified_mailmap_msg));
-
+	if (mailmap < 0)
 		mailmap = 0;
-	}

This should be "mailmap = 1" to match the commit message, no? (Which
also implies we may want a new test).

I'd also be OK with leaving it at "0" for now, making a note of the
upcoming change in the release notes, and flipping it _next_ time. But
IMHO we don't need to be that conservative (and I share your pessimism
that enough people actually read the release notes to merit that kind of
advance notice).

-Peff

Re: Git for Windows v2.23.0-rc0, was Re: [ANNOUNCE] Git v2.23.0-rc0

From: Jonathan Nieder <hidden>
Date: 2019-08-02 02:27:18

Junio C Hamano wrote:

I suspect that you may have misread the "is interactive" bit in the
original; that was used only to decide if we are going to warn.

Ah.  That was indeed confusing.  Anyway, it's nice to see the complexity
go away.

[...]

quoted hunk

+++ b/builtin/log.c

[...]

quoted hunk

@@ -214,12 +204,8 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 	memset(&w, 0, sizeof(w));
 	userformat_find_requirements(NULL, &w);
 
-	if (mailmap < 0) {
-		if (session_is_interactive() && !rev->pretty_given)
-			warning("%s\n", _(warn_unspecified_mailmap_msg));
-
+	if (mailmap < 0)
 		mailmap = 0;

As Peff noticed, this should say "mailmap = 1" (which I see you've done
in "pu").  We can simplify further by removing the "-1" case --- we do
not need to distinguish between "on" and "unspecified" any more.

We'll also want to update the docs.  And as Todd suggests, we should
cover how to disable mailmap in tests.

Signed-off-by: Jonathan Nieder <redacted>
---

diff --git i/Documentation/config/log.txt w/Documentation/config/log.txt
index 7798e10cb0..e9e1e397f3 100644
--- i/Documentation/config/log.txt
+++ w/Documentation/config/log.txt

@@ -41,4 +41,4 @@ log.showSignature::
 log.mailmap::
 	If true, makes linkgit:git-log[1], linkgit:git-show[1], and
 	linkgit:git-whatchanged[1] assume `--use-mailmap`, otherwise
-	assume `--no-use-mailmap`. False by default.
+	assume `--no-use-mailmap`. True by default.

diff --git i/builtin/log.c w/builtin/log.c
index 02fa179077..44b10b3415 100644
--- i/builtin/log.c
+++ w/builtin/log.c

@@ -47,7 +47,7 @@ static int default_follow;
 static int default_show_signature;
 static int decoration_style;
 static int decoration_given;
-static int use_mailmap_config = -1;
+static int use_mailmap_config = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;

@@ -160,7 +160,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 			 struct rev_info *rev, struct setup_revision_opt *opt)
 {
 	struct userformat_want w;
-	int quiet = 0, source = 0, mailmap = 0;
+	int quiet = 0, source = 0, mailmap;
 	static struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};
 	static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
 	static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;

@@ -204,9 +204,6 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 	memset(&w, 0, sizeof(w));
 	userformat_find_requirements(NULL, &w);
 
-	if (mailmap < 0)
-		mailmap = 1;
-
 	if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
 		rev->show_notes = 1;
 	if (rev->show_notes)

diff --git i/t/t4203-mailmap.sh w/t/t4203-mailmap.sh
index ede0c292af..918ada69eb 100755
--- i/t/t4203-mailmap.sh
+++ w/t/t4203-mailmap.sh

@@ -442,6 +442,34 @@ test_expect_success 'Log output with log.mailmap' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log.mailmap=false disables mailmap' '
+	cat >expect <<-\EOF &&
+	Author: CTO <cto@coompany.xx>
+	Author: claus <me@company.xx>
+	Author: santa <me@company.xx>
+	Author: nick2 <nick2@company.xx>
+	Author: nick2 <bugs@company.xx>
+	Author: nick1 <bugs@company.xx>
+	Author: A U Thor <author@example.com>
+	EOF
+	git -c log.mailmap=False log | grep Author > actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--no-use-mailmap disables mailmap' '
+	cat >expect <<-\EOF &&
+	Author: CTO <cto@coompany.xx>
+	Author: claus <me@company.xx>
+	Author: santa <me@company.xx>
+	Author: nick2 <nick2@company.xx>
+	Author: nick2 <bugs@company.xx>
+	Author: nick1 <bugs@company.xx>
+	Author: A U Thor <author@example.com>
+	EOF
+	git log --no-use-mailmap | grep Author > actual &&
+	test_cmp expect actual
+'
+
 cat >expect <<\EOF
 Author: Santa Claus <santa.claus@northpole.xx>
 Author: Santa Claus <santa.claus@northpole.xx>

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help