git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Ariadne Conill <ariadne@dereferenced.org>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 1/3] log: add warning for unspecified log.mailmap setting
Date: Sun, 14 Jul 2019 14:55:19 -0700	[thread overview]
Message-ID: <xmqqzhlg46yg.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190712230204.16749-2-ariadne@dereferenced.org> (Ariadne Conill's message of "Fri, 12 Jul 2019 18:02:02 -0500")

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.





  reply	other threads:[~2019-07-14 21:55 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqzhlg46yg.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=ariadne@dereferenced.org \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).