[v2,1/3] log: add warning for unspecified log.mailmap setting
diff mbox series

Message ID 20190712230204.16749-2-ariadne@dereferenced.org
State New
Headers show
Series
  • document deprecation of log.mailmap=false default
Related show

Commit Message

Ariadne Conill July 12, 2019, 11:02 p.m. UTC
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(-)

Comments

Junio C Hamano July 14, 2019, 9:55 p.m. UTC | #1
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.

Patch
diff mbox series

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)