Re: [PATCH v3] Add user.explicit boolean for when ident shouldn't be guessed

From: Jeff King <peff@peff.net>
Date: Feb 4, 2016, 04:19 UTC
Message-ID: <20160204041909.GB27371@sigill.intra.peff.net>
In-Reply-To: <20160204040111.GA27371@sigill.intra.peff.net>

On Wed, Feb 03, 2016 at 11:01:11PM -0500, Jeff King wrote:

Show 15 quoted lines

> > +	if (ident_explicit) {
> > +		if (name == git_default_name.buf &&
> > +		    !(committer_ident_explicitly_given & IDENT_NAME_GIVEN) &&
> > +		    !(author_ident_explicitly_given & IDENT_NAME_GIVEN))
> > +			die("requested explicitly given ident in config, "
> > +			    "but user.name is not set, or environment is "
> > +			    "not set");
> > +		if (email == git_default_email.buf &&
> > +		    !(committer_ident_explicitly_given & IDENT_MAIL_GIVEN) &&
> > +		    !(author_ident_explicitly_given & IDENT_MAIL_GIVEN))
> > +			die("requested explicitly given ident in config, "
> > +			    "but user.email is not set, or environment is "
> > +			    "not set");
> > +	}
> > +

By the way, while reading fmt_ident, I found it to be pretty convoluted, and the comparisons to the global strbuf are a bit gross. Maybe this should be a good preparatory cleanup?

-- >8 --
Subject: [PATCH] fmt_ident: refactor strictness checks

This function has evolved quite a bit over time, and as a result, the logic for "is this an OK ident" has been sprinkled throughout. This ends up with a lot of redundant conditionals, like checking want_name repeatedly. Worse, we want to know in many cases whether we are using the "default" ident, and we do so by comparing directly to the global strbuf, which violates the abstraction of the ident_default_* functions.

Let's reorganize the function into a hierarchy of conditionals to handle similar cases together. The only case that doesn't just work naturally for this is that of an empty name, where our advice is different based on whether we came from ident_default_name() or not. We can use a simple flag to cover this case.

Signed-off-by: Jeff King <peff@peff.net>
---
Another alternative for the last paragraph would be to break the
empty-name-handling out to its own function and call it from two places.

 ident.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/ident.c b/ident.c
index 3da5556..f3a431f 100644
--- a/ident.c
+++ b/ident.c
@@ -345,32 +345,34 @@ const char *fmt_ident(const char *name, const char *email,
 	int want_date = !(flag & IDENT_NO_DATE);
 	int want_name = !(flag & IDENT_NO_NAME);
 
-	if (want_name && !name)
-		name = ident_default_name();
-	if (!email)
-		email = ident_default_email();
-
-	if (want_name && !*name) {
-		struct passwd *pw;
-
-		if (strict) {
-			if (name == git_default_name.buf)
+	if (want_name) {
+		int using_default = 0;
+		if (!name) {
+			name = ident_default_name();
+			using_default = 1;
+			if (strict && default_name_is_bogus) {
 				fputs(env_hint, stderr);
-			die("empty ident name (for <%s>) not allowed", email);
+				die("unable to auto-detect name (got '%s')", name);
+			}
+		}
+		if (!*name) {
+			struct passwd *pw;
+			if (strict) {
+				if (using_default)
+					fputs(env_hint, stderr);
+				die("empty ident name (for <%s>) not allowed", email);
+			}
+			pw = xgetpwuid_self(NULL);
+			name = pw->pw_name;
 		}
-		pw = xgetpwuid_self(NULL);
-		name = pw->pw_name;
-	}
-
-	if (want_name && strict &&
-	    name == git_default_name.buf && default_name_is_bogus) {
-		fputs(env_hint, stderr);
-		die("unable to auto-detect name (got '%s')", name);
 	}
 
-	if (strict && email == git_default_email.buf && default_email_is_bogus) {
-		fputs(env_hint, stderr);
-		die("unable to auto-detect email address (got '%s')", email);
+	if (!email) {
+		email = ident_default_email();
+		if (strict && default_email_is_bogus) {
+			fputs(env_hint, stderr);
+			die("unable to auto-detect email address (got '%s')", email);
+		}
 	}
 
 	strbuf_reset(&ident);

-- 
2.7.0.513.g5aae4e5