git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH v2 0/3] document deprecation of log.mailmap=false default
@ 2019-07-12 23:02 Ariadne Conill
  2019-07-12 23:02 ` [PATCH v2 1/3] log: add warning for unspecified log.mailmap setting Ariadne Conill
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Ariadne Conill @ 2019-07-12 23:02 UTC (permalink / raw)
  To: git; +Cc: Ariadne Conill

Based on the discussion of the previous patches, we concluded that
changing the default will require a transitional period.

As such, I have introduced a deprecation warning that is printed when
the log builtin commands are used.

Thanks to Junio and everyone else for providing feedback on how to
proceed.

New in version 2:
- The warning is disabled when `--format` is used.
- The warning is disabled when not called from a controlling terminal.
- Tests which fake a controlling terminal have been defanged.

Ariadne Conill (3):
  log: add warning for unspecified log.mailmap setting
  documentation: mention --no-use-mailmap and log.mailmap false setting
  tests: defang pager tests by explicitly disabling the log.mailmap
    warning

 Documentation/config/log.txt |  3 ++-
 Documentation/git-log.txt    |  2 +-
 builtin/log.c                | 25 ++++++++++++++++++++++++-
 t/t7006-pager.sh             | 10 ++++++++++
 4 files changed, 37 insertions(+), 3 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/3] log: add warning for unspecified log.mailmap setting
  2019-07-12 23:02 [PATCH v2 0/3] document deprecation of log.mailmap=false default Ariadne Conill
@ 2019-07-12 23:02 ` Ariadne Conill
  2019-07-14 21:55   ` Junio C Hamano
  2019-07-12 23:02 ` [PATCH v2 2/3] documentation: mention --no-use-mailmap and log.mailmap false setting Ariadne Conill
  2019-07-12 23:02 ` [PATCH v2 3/3] tests: defang pager tests by explicitly disabling the log.mailmap warning Ariadne Conill
  2 siblings, 1 reply; 6+ messages in thread
From: Ariadne Conill @ 2019-07-12 23:02 UTC (permalink / raw)
  To: git; +Cc: Ariadne Conill

Based on discussions around changing the log.mailmap default to being
enabled, it was decided that a transitional period is required.

Accordingly, we announce this transitional period with a warning
message.

Signed-off-by: Ariadne Conill <ariadne@dereferenced.org>
---
 builtin/log.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 7c8767d3bc..559f42fe48 100644
--- a/builtin/log.c
+++ b/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;
+static int use_mailmap_config = -1;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
@@ -151,6 +151,16 @@ 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)
 {
@@ -199,6 +209,19 @@ 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) {
+		/*
+		 * Only display the warning if the session is interactive
+		 * and pretty_given is false. We determine that the session
+		 * is interactive by checking if auto_decoration_style()
+		 * returns non-zero.
+		 */
+		if (auto_decoration_style() && !rev->pretty_given)
+			warning("%s\n", _(warn_unspecified_mailmap_msg));
+
+		mailmap = 0;
+	}
+
 	if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
 		rev->show_notes = 1;
 	if (rev->show_notes)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 2/3] documentation: mention --no-use-mailmap and log.mailmap false setting
  2019-07-12 23:02 [PATCH v2 0/3] document deprecation of log.mailmap=false default Ariadne Conill
  2019-07-12 23:02 ` [PATCH v2 1/3] log: add warning for unspecified log.mailmap setting Ariadne Conill
@ 2019-07-12 23:02 ` Ariadne Conill
  2019-07-12 23:02 ` [PATCH v2 3/3] tests: defang pager tests by explicitly disabling the log.mailmap warning Ariadne Conill
  2 siblings, 0 replies; 6+ messages in thread
From: Ariadne Conill @ 2019-07-12 23:02 UTC (permalink / raw)
  To: git; +Cc: Ariadne Conill

The log.mailmap setting may be explicitly set to false, which disables
the mailmap feature implicity. In practice, doing so is equivalent to
always using the previously undocumented --no-use-mailmap option on the
command line.

Accordingly, we document both the existence of --no-use-mailmap as
well as briefly discuss the equivalence of it to log.mailmap=False.

Signed-off-by: Ariadne Conill <ariadne@dereferenced.org>
---
 Documentation/config/log.txt | 3 ++-
 Documentation/git-log.txt    | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index 78d9e4453a..7798e10cb0 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -40,4 +40,5 @@ log.showSignature::
 
 log.mailmap::
 	If true, makes linkgit:git-log[1], linkgit:git-show[1], and
-	linkgit:git-whatchanged[1] assume `--use-mailmap`.
+	linkgit:git-whatchanged[1] assume `--use-mailmap`, otherwise
+	assume `--no-use-mailmap`. False by default.
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index b02e922dc3..b406bc4c48 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -49,7 +49,7 @@ OPTIONS
 	Print out the ref name given on the command line by which each
 	commit was reached.
 
---use-mailmap::
+--[no-]use-mailmap::
 	Use mailmap file to map author and committer names and email
 	addresses to canonical real names and email addresses. See
 	linkgit:git-shortlog[1].
-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 3/3] tests: defang pager tests by explicitly disabling the log.mailmap warning
  2019-07-12 23:02 [PATCH v2 0/3] document deprecation of log.mailmap=false default Ariadne Conill
  2019-07-12 23:02 ` [PATCH v2 1/3] log: add warning for unspecified log.mailmap setting Ariadne Conill
  2019-07-12 23:02 ` [PATCH v2 2/3] documentation: mention --no-use-mailmap and log.mailmap false setting Ariadne Conill
@ 2019-07-12 23:02 ` Ariadne Conill
  2019-07-14 21:56   ` Junio C Hamano
  2 siblings, 1 reply; 6+ messages in thread
From: Ariadne Conill @ 2019-07-12 23:02 UTC (permalink / raw)
  To: git; +Cc: Ariadne Conill

In the previous patch, we added a deprecation warning for the current
log.mailmap setting. This warning only appears when git is attached to
a controlling terminal. Some tests however run under an emulated
terminal, so we need to disable the warning for those tests.

Signed-off-by: Ariadne Conill <ariadne@dereferenced.org>
---
 t/t7006-pager.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 00e09a375c..1c72aae197 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -304,6 +304,7 @@ test_expect_success 'tests can detect color' '
 
 test_expect_success 'no color when stdout is a regular file' '
 	rm -f colorless.log &&
+	test_config log.mailmap false &&
 	test_config color.ui auto &&
 	git log >colorless.log &&
 	! colorful colorless.log
@@ -311,6 +312,7 @@ test_expect_success 'no color when stdout is a regular file' '
 
 test_expect_success TTY 'color when writing to a pager' '
 	rm -f paginated.out &&
+	test_config log.mailmap false &&
 	test_config color.ui auto &&
 	test_terminal git log &&
 	colorful paginated.out
@@ -318,6 +320,7 @@ test_expect_success TTY 'color when writing to a pager' '
 
 test_expect_success TTY 'colors are suppressed by color.pager' '
 	rm -f paginated.out &&
+	test_config log.mailmap false &&
 	test_config color.ui auto &&
 	test_config color.pager false &&
 	test_terminal git log &&
@@ -326,6 +329,7 @@ test_expect_success TTY 'colors are suppressed by color.pager' '
 
 test_expect_success 'color when writing to a file intended for a pager' '
 	rm -f colorful.log &&
+	test_config log.mailmap false &&
 	test_config color.ui auto &&
 	(
 		TERM=vt100 &&
@@ -337,6 +341,7 @@ test_expect_success 'color when writing to a file intended for a pager' '
 '
 
 test_expect_success TTY 'colors are sent to pager for external commands' '
+	test_config log.mailmap false &&
 	test_config alias.externallog "!git log" &&
 	test_config color.ui auto &&
 	test_terminal git -p externallog &&
@@ -573,6 +578,7 @@ test_expect_success TTY 'command-specific pager' '
 	sane_unset PAGER GIT_PAGER &&
 	echo "foo:initial" >expect &&
 	>actual &&
+	test_config log.mailmap false &&
 	test_unconfig core.pager &&
 	test_config pager.log "sed s/^/foo:/ >actual" &&
 	test_terminal git log --format=%s -1 &&
@@ -583,6 +589,7 @@ test_expect_success TTY 'command-specific pager overrides core.pager' '
 	sane_unset PAGER GIT_PAGER &&
 	echo "foo:initial" >expect &&
 	>actual &&
+	test_config log.mailmap false &&
 	test_config core.pager "exit 1" &&
 	test_config pager.log "sed s/^/foo:/ >actual" &&
 	test_terminal git log --format=%s -1 &&
@@ -593,6 +600,7 @@ test_expect_success TTY 'command-specific pager overridden by environment' '
 	GIT_PAGER="sed s/^/foo:/ >actual" && export GIT_PAGER &&
 	>actual &&
 	echo "foo:initial" >expect &&
+	test_config log.mailmap false &&
 	test_config pager.log "exit 1" &&
 	test_terminal git log --format=%s -1 &&
 	test_cmp expect actual
@@ -610,6 +618,7 @@ test_expect_success TTY 'command-specific pager works for external commands' '
 	sane_unset PAGER GIT_PAGER &&
 	echo "foo:initial" >expect &&
 	>actual &&
+	test_config log.mailmap false &&
 	test_config pager.external "sed s/^/foo:/ >actual" &&
 	test_terminal git --exec-path="$(pwd)" external log --format=%s -1 &&
 	test_cmp expect actual
@@ -619,6 +628,7 @@ test_expect_success TTY 'sub-commands of externals use their own pager' '
 	sane_unset PAGER GIT_PAGER &&
 	echo "foo:initial" >expect &&
 	>actual &&
+	test_config log.mailmap false &&
 	test_config pager.log "sed s/^/foo:/ >actual" &&
 	test_terminal git --exec-path=. external log --format=%s -1 &&
 	test_cmp expect actual
-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/3] log: add warning for unspecified log.mailmap setting
  2019-07-12 23:02 ` [PATCH v2 1/3] log: add warning for unspecified log.mailmap setting Ariadne Conill
@ 2019-07-14 21:55   ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2019-07-14 21:55 UTC (permalink / raw)
  To: Ariadne Conill; +Cc: git

Ariadne Conill <ariadne@dereferenced.org> writes:

> +	if (mailmap < 0) {
> +		/*
> +		 * Only display the warning if the session is interactive
> +		 * and pretty_given is false. We determine that the session
> +		 * is interactive by checking if auto_decoration_style()
> +		 * returns non-zero.
> +		 */
> +		if (auto_decoration_style() && !rev->pretty_given)
> +			warning("%s\n", _(warn_unspecified_mailmap_msg));

The huge comment can go if you refactored the helper function a
little bit and will give us a much better better organization.

static int auto_decoration_style(void)
{
	return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
}

The existing helper is meant to help those who are interested in the
decoration feature, and the fact that it kicks in by default when
the condition (isatty(1) || pager_in_use()) is true is a mere
"decoration feature happens to be designed that way right now".
There is no logical reason to expect that the decoration feature and
mailmap feature's advicse messages will be triggered by the same
condition forever.

Think a bit and what the condition "means".  You wrote a good one
yourself above: "the session is interactive".  Introduce a helper
that checks exatly that, by reusing what auto_decoration_style()
already uses. i.e.

	static int session_is_interactive(void)
	{
		return isatty(1) || pager_in_use();
	}

	static int auto_decoration_style(void)
	{
		return session_is_interactive() ? DECORATE_SHORT_REFS : 0;
	}

and then the above hunk becomes

	if (session_is_interactive() && !rev->pretty_given)
		warning(...);

It is clear enough and there is no need for your 2 sentence comment,
as (1) the first sentence is exactly what the implementation is, and
(2) we no longer abuse auto_decoration_style() outside its intended
purpose.





^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 3/3] tests: defang pager tests by explicitly disabling the log.mailmap warning
  2019-07-12 23:02 ` [PATCH v2 3/3] tests: defang pager tests by explicitly disabling the log.mailmap warning Ariadne Conill
@ 2019-07-14 21:56   ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2019-07-14 21:56 UTC (permalink / raw)
  To: Ariadne Conill; +Cc: git

Ariadne Conill <ariadne@dereferenced.org> writes:

> In the previous patch, we added a deprecation warning for the current
> log.mailmap setting. This warning only appears when git is attached to
> a controlling terminal. Some tests however run under an emulated
> terminal, so we need to disable the warning for those tests.
>
> Signed-off-by: Ariadne Conill <ariadne@dereferenced.org>
> ---
>  t/t7006-pager.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Hmm, this is horrible.  

These tests are primarily to see how use of color gets affected by
various configuration and the use of the pager, and having to
sprinkle log.mailmap configuration to just randomly selected 10
tests among 50+ tests in the script makes readers wonder if the
configuration has anything to do with the coloring (answer: no).

The primary reason why other 40+ tests do not need log.mailmap
tweaked is not because log.mailmap does not affect the coloring.
But for a new developer who will be adding a new test to this file,
how would s/he decide if the new test needs log.mailmap=false like
these 10, or it is like the other 40+?

It almost makes me feel that it would be much better to just disable
the warning inside the setup part, perhaps like

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 00e09a375c..283de499fc 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -7,6 +7,8 @@ test_description='Test automatic use of a pager.'
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success 'setup' '
+	: squelch advise messages during the transition &&
+	git config --global log.mailmap false &&
 	sane_unset GIT_PAGER GIT_PAGER_IN_USE &&
 	test_unconfig core.pager &&
 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-07-14 21:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-12 23:02 [PATCH v2 0/3] document deprecation of log.mailmap=false default Ariadne Conill
2019-07-12 23:02 ` [PATCH v2 1/3] log: add warning for unspecified log.mailmap setting Ariadne Conill
2019-07-14 21:55   ` Junio C Hamano
2019-07-12 23:02 ` [PATCH v2 2/3] documentation: mention --no-use-mailmap and log.mailmap false setting Ariadne Conill
2019-07-12 23:02 ` [PATCH v2 3/3] tests: defang pager tests by explicitly disabling the log.mailmap warning Ariadne Conill
2019-07-14 21:56   ` Junio C Hamano

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).