possible bug: inconsistent CLI behaviour for empty user.name

possible bug: inconsistent CLI behaviour for empty user.name

[bs.x.ttp]

The problem is that GIT accepts a user.name of " " for some operations (for example when doing a simple "git commit"), but does require a "non-empty" user.name for others (like git commit --amend and git rebase). In case of the latter commands GIT fails with the message "fatal: empty ident name (for <email@address>) not allowed".

As people tend to do simple commits first, before amending or rebasing something, they may have to change their name after some dozen of commits which doesn't look nice.

This is certainly not a big issue, but it turns out to be quite annoying and I've already rewritten the history of a GIT repository once because of it, so that all my commits had the same author.

Proposed solution: GIT's requirements for user.name should not depend on the operation. Either user.name should be enforced to be non-empty everywhere or an empty user.name should be accepted everywhere. Perhaps filling out one of user.name and user.email could be sufficient.

Re: possible bug: inconsistent CLI behaviour for empty user.name

[Jeff King]

On Fri, Feb 03, 2017 at 05:13:09AM +0100, bs.x.ttp@recursor.net wrote:

> The problem is that GIT accepts a user.name of " " for some operations
> (for example when doing a simple "git commit"), but does require a
> "non-empty" user.name for others (like git commit --amend and git
> rebase). In case of the latter commands GIT fails with the message
> "fatal: empty ident name (for <email@address>) not allowed".

I think it's a bug. We try to always reject empty usernames, but the
"empty" check is done before we cut off leading and trailing cruft (like
whitespace).

The "--amend" command notices because it actually parses the name out of
the existing commit. That version has already had its whitespace eaten
up (when it was written into the original commit), and so it ends up as
blank.

Here's a series which fixes that along with a few other oddities I
noticed.

  [1/4]: ident: mark error messages for translation
  [2/4]: ident: handle NULL email when complaining of empty name
  [3/4]: ident: reject all-crud ident name
  [4/4]: ident: do not ignore empty config name/email

 ident.c                       | 49 ++++++++++++++++++++++++++-----------------
 t/t7518-ident-corner-cases.sh | 36 +++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 19 deletions(-)
 create mode 100755 t/t7518-ident-corner-cases.sh


-Peff

[PATCH 1/4] ident: mark error messages for translation

[Jeff King]

We already translate the big "please tell me who you are"
hint, but missed the individual error messages that go with
it.

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/ident.c b/ident.c
index ac4ae02b4..dde82983a 100644
--- a/ident.c
+++ b/ident.c
@@ -357,13 +357,13 @@ const char *fmt_ident(const char *name, const char *email,
 			if (strict && ident_use_config_only
 			    && !(ident_config_given & IDENT_NAME_GIVEN)) {
 				fputs(_(env_hint), stderr);
-				die("no name was given and auto-detection is disabled");
+				die(_("no name was given and auto-detection is disabled"));
 			}
 			name = ident_default_name();
 			using_default = 1;
 			if (strict && default_name_is_bogus) {
 				fputs(_(env_hint), stderr);
-				die("unable to auto-detect name (got '%s')", name);
+				die(_("unable to auto-detect name (got '%s')"), name);
 			}
 		}
 		if (!*name) {
@@ -371,7 +371,7 @@ const char *fmt_ident(const char *name, const char *email,
 			if (strict) {
 				if (using_default)
 					fputs(_(env_hint), stderr);
-				die("empty ident name (for <%s>) not allowed", email);
+				die(_("empty ident name (for <%s>) not allowed"), email);
 			}
 			pw = xgetpwuid_self(NULL);
 			name = pw->pw_name;
@@ -382,12 +382,12 @@ const char *fmt_ident(const char *name, const char *email,
 		if (strict && ident_use_config_only
 		    && !(ident_config_given & IDENT_MAIL_GIVEN)) {
 			fputs(_(env_hint), stderr);
-			die("no email was given and auto-detection is disabled");
+			die(_("no email was given and auto-detection is disabled"));
 		}
 		email = ident_default_email();
 		if (strict && default_email_is_bogus) {
 			fputs(_(env_hint), stderr);
-			die("unable to auto-detect email address (got '%s')", email);
+			die(_("unable to auto-detect email address (got '%s')"), email);
 		}
 	}
 
@@ -403,7 +403,7 @@ const char *fmt_ident(const char *name, const char *email,
 		strbuf_addch(&ident, ' ');
 		if (date_str && date_str[0]) {
 			if (parse_date(date_str, &ident) < 0)
-				die("invalid date format: %s", date_str);
+				die(_("invalid date format: %s"), date_str);
 		}
 		else
 			strbuf_addstr(&ident, ident_default_date());
-- 
2.12.0.rc2.597.g959f68882

[PATCH 2/4] ident: handle NULL email when complaining of empty name

[Jeff King]

If we see an empty name, we complain about and mention the
matching email in the error message (to give it some
context). However, the "email" pointer may be NULL here if
we were planning to fill it in later from ident_default_email().

This was broken by 59f929596 (fmt_ident: refactor strictness
checks, 2016-02-04). Prior to that commit, we would look up
the default name and email before doing any other actions.
So one solution would be to go back to that.

However, we can't just do so blindly. The logic for handling
the "!email" condition has grown since then. In particular,
looking up the default email can die if getpwuid() fails,
but there are other errors that should take precedence.
Commit 734c7789a (ident: check for useConfigOnly before
auto-detection of name/email, 2016-03-30) reordered the
checks so that we prefer the error message for
useConfigOnly.

Instead, we can observe that while the name-handling depends
on "email" being set, the reverse is not true. So we can
simply set up the email variable first.

This does mean that if both are bogus, we'll complain about
the email before the name. But between the two, there is no
reason to prefer one over the other.

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c                       | 26 +++++++++++++-------------
 t/t7518-ident-corner-cases.sh | 20 ++++++++++++++++++++
 2 files changed, 33 insertions(+), 13 deletions(-)
 create mode 100755 t/t7518-ident-corner-cases.sh

diff --git a/ident.c b/ident.c
index dde82983a..ea6034581 100644
--- a/ident.c
+++ b/ident.c
@@ -351,6 +351,19 @@ 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 (!email) {
+		if (strict && ident_use_config_only
+		    && !(ident_config_given & IDENT_MAIL_GIVEN)) {
+			fputs(_(env_hint), stderr);
+			die(_("no email was given and auto-detection is disabled"));
+		}
+		email = ident_default_email();
+		if (strict && default_email_is_bogus) {
+			fputs(_(env_hint), stderr);
+			die(_("unable to auto-detect email address (got '%s')"), email);
+		}
+	}
+
 	if (want_name) {
 		int using_default = 0;
 		if (!name) {
@@ -378,19 +391,6 @@ const char *fmt_ident(const char *name, const char *email,
 		}
 	}
 
-	if (!email) {
-		if (strict && ident_use_config_only
-		    && !(ident_config_given & IDENT_MAIL_GIVEN)) {
-			fputs(_(env_hint), stderr);
-			die(_("no email was given and auto-detection is disabled"));
-		}
-		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);
 	if (want_name) {
 		strbuf_addstr_without_crud(&ident, name);
diff --git a/t/t7518-ident-corner-cases.sh b/t/t7518-ident-corner-cases.sh
new file mode 100755
index 000000000..6c057afc1
--- /dev/null
+++ b/t/t7518-ident-corner-cases.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+test_description='corner cases in ident strings'
+. ./test-lib.sh
+
+# confirm that we do not segfault _and_ that we do not say "(null)", as
+# glibc systems will quietly handle our NULL pointer
+#
+# Note also that we can't use "env" here because we need to unset a variable,
+# and "-u" is not portable.
+test_expect_success 'empty name and missing email' '
+	(
+		sane_unset GIT_AUTHOR_EMAIL &&
+		GIT_AUTHOR_NAME= &&
+		test_must_fail git commit --allow-empty -m foo 2>err &&
+		test_i18ngrep ! null err
+	)
+'
+
+test_done
-- 
2.12.0.rc2.597.g959f68882

[PATCH 3/4] ident: reject all-crud ident name

[Jeff King]

An ident name consisting of only "crud" characters (like
whitespace or punctuation) is effectively the same as an
empty one, because our strbuf_addstr_without_crud() will
remove those characters.

We reject an empty name when formatting a strict ident, but
don't notice an all-crud one because our check happens
before the crud-removal step.

We could skip past the crud before checking for an empty
name, but let's make it a separate code path, for two
reasons. One is that we can give a more specific error
message. And two is that unlike a blank name, we probably
don't want to kick in the fallback-to-username behavior.

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c                       | 11 +++++++++++
 t/t7518-ident-corner-cases.sh |  5 +++++
 2 files changed, 16 insertions(+)

diff --git a/ident.c b/ident.c
index ea6034581..ead09ff7f 100644
--- a/ident.c
+++ b/ident.c
@@ -203,6 +203,15 @@ static int crud(unsigned char c)
 		c == '\'';
 }
 
+static int has_non_crud(const char *str)
+{
+	for (; *str; str++) {
+		if (!crud(*str))
+			return 1;
+	}
+	return 0;
+}
+
 /*
  * Copy over a string to the destination, but avoid special
  * characters ('\n', '<' and '>') and remove crud at the end
@@ -389,6 +398,8 @@ const char *fmt_ident(const char *name, const char *email,
 			pw = xgetpwuid_self(NULL);
 			name = pw->pw_name;
 		}
+		if (strict && !has_non_crud(name))
+			die(_("name consists only of disallowed characters: %s"), name);
 	}
 
 	strbuf_reset(&ident);
diff --git a/t/t7518-ident-corner-cases.sh b/t/t7518-ident-corner-cases.sh
index 6c057afc1..3d2560c3c 100755
--- a/t/t7518-ident-corner-cases.sh
+++ b/t/t7518-ident-corner-cases.sh
@@ -17,4 +17,9 @@ test_expect_success 'empty name and missing email' '
 	)
 '
 
+test_expect_success 'commit rejects all-crud name' '
+	test_must_fail env GIT_AUTHOR_NAME=" .;<>" \
+		git commit --allow-empty -m foo
+'
+
 test_done
-- 
2.12.0.rc2.597.g959f68882

[PATCH 4/4] ident: do not ignore empty config name/email

[Jeff King]

When we read user.name and user.email from a config file,
they go into strbufs. When a caller asks ident_default_name()
for the value, we fallback to auto-detecting if the strbuf
is empty.

That means that explicitly setting an empty string in the
config is identical to not setting it at all. This is
potentially confusing, as we usually accept a configured
value as the final value.

Signed-off-by: Jeff King <peff@peff.net>
---
This one is perhaps questionable. Maybe somebody is relying on setting a
per-repo user.name to override a ~/.gitconfig value and enforce
auto-detection?

 ident.c                       |  4 ++--
 t/t7518-ident-corner-cases.sh | 11 +++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/ident.c b/ident.c
index ead09ff7f..c0364fe3a 100644
--- a/ident.c
+++ b/ident.c
@@ -153,7 +153,7 @@ static void copy_email(const struct passwd *pw, struct strbuf *email,
 
 const char *ident_default_name(void)
 {
-	if (!git_default_name.len) {
+	if (!(ident_config_given & IDENT_NAME_GIVEN) && !git_default_name.len) {
 		copy_gecos(xgetpwuid_self(&default_name_is_bogus), &git_default_name);
 		strbuf_trim(&git_default_name);
 	}
@@ -162,7 +162,7 @@ const char *ident_default_name(void)
 
 const char *ident_default_email(void)
 {
-	if (!git_default_email.len) {
+	if (!(ident_config_given & IDENT_MAIL_GIVEN) && !git_default_email.len) {
 		const char *email = getenv("EMAIL");
 
 		if (email && email[0]) {
diff --git a/t/t7518-ident-corner-cases.sh b/t/t7518-ident-corner-cases.sh
index 3d2560c3c..ef570ac62 100755
--- a/t/t7518-ident-corner-cases.sh
+++ b/t/t7518-ident-corner-cases.sh
@@ -22,4 +22,15 @@ test_expect_success 'commit rejects all-crud name' '
 		git commit --allow-empty -m foo
 '
 
+# We must test the actual error message here, as an unwanted
+# auto-detection could fail for other reasons.
+test_expect_success 'empty configured name does not auto-detect' '
+	(
+		sane_unset GIT_AUTHOR_NAME &&
+		test_must_fail \
+			git -c user.name= commit --allow-empty -m foo 2>err &&
+		test_i18ngrep "empty ident name" err
+	)
+'
+
 test_done
-- 
2.12.0.rc2.597.g959f68882

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This one is perhaps questionable. Maybe somebody is relying on setting a
> per-repo user.name to override a ~/.gitconfig value and enforce
> auto-detection?

Thanks for splitting this step out.  1/4 and 2/4 are obvious
improvements, and 3/4 is a very sensible fix.  Compared to those
three, this one does smell questionable, because I do not quite see
any other reasonable fallback other than the auto-detection if the
user gives an empty ident on purpose.  

Erroring out to say "don't do that" is probably not too bad, but
perhaps we are being run by a script that is doing a best-effort
conversion from $ANOTHER_SCM using a list of known authors that is
incomplete, ending up feeding empty ident and allowing us to fall
back to attribute them to the user who runs the script.  I do not
see a point in breaking that user and having her or him update the
script to stuff in a truly bogus "Unknown <unknown>" name.

>
>  ident.c                       |  4 ++--
>  t/t7518-ident-corner-cases.sh | 11 +++++++++++
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/ident.c b/ident.c
> index ead09ff7f..c0364fe3a 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -153,7 +153,7 @@ static void copy_email(const struct passwd *pw, struct strbuf *email,
>  
>  const char *ident_default_name(void)
>  {
> -	if (!git_default_name.len) {
> +	if (!(ident_config_given & IDENT_NAME_GIVEN) && !git_default_name.len) {
>  		copy_gecos(xgetpwuid_self(&default_name_is_bogus), &git_default_name);
>  		strbuf_trim(&git_default_name);
>  	}
> @@ -162,7 +162,7 @@ const char *ident_default_name(void)
>  
>  const char *ident_default_email(void)
>  {
> -	if (!git_default_email.len) {
> +	if (!(ident_config_given & IDENT_MAIL_GIVEN) && !git_default_email.len) {
>  		const char *email = getenv("EMAIL");
>  
>  		if (email && email[0]) {
> diff --git a/t/t7518-ident-corner-cases.sh b/t/t7518-ident-corner-cases.sh
> index 3d2560c3c..ef570ac62 100755
> --- a/t/t7518-ident-corner-cases.sh
> +++ b/t/t7518-ident-corner-cases.sh
> @@ -22,4 +22,15 @@ test_expect_success 'commit rejects all-crud name' '
>  		git commit --allow-empty -m foo
>  '
>  
> +# We must test the actual error message here, as an unwanted
> +# auto-detection could fail for other reasons.
> +test_expect_success 'empty configured name does not auto-detect' '
> +	(
> +		sane_unset GIT_AUTHOR_NAME &&
> +		test_must_fail \
> +			git -c user.name= commit --allow-empty -m foo 2>err &&
> +		test_i18ngrep "empty ident name" err
> +	)
> +'
> +
>  test_done

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Jeff King]

On Thu, Feb 23, 2017 at 12:58:39PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This one is perhaps questionable. Maybe somebody is relying on setting a
> > per-repo user.name to override a ~/.gitconfig value and enforce
> > auto-detection?
> 
> Thanks for splitting this step out.  1/4 and 2/4 are obvious
> improvements, and 3/4 is a very sensible fix.  Compared to those
> three, this one does smell questionable, because I do not quite see
> any other reasonable fallback other than the auto-detection if the
> user gives an empty ident on purpose.

The outcomes are basically:

  1. In strict mode (making a commit, etc), we'll die with "empty name
     not allowed". My thinking was that this is less confusing for the
     user.

  2. In non-strict mode, we'd use a blank name instead of trying your
     username (or dying if you don't have an /etc/passwd entry).

> Erroring out to say "don't do that" is probably not too bad, but
> perhaps we are being run by a script that is doing a best-effort
> conversion from $ANOTHER_SCM using a list of known authors that is
> incomplete, ending up feeding empty ident and allowing us to fall
> back to attribute them to the user who runs the script.  I do not
> see a point in breaking that user and having her or him update the
> script to stuff in a truly bogus "Unknown <unknown>" name.

Keep in mind this _only_ affects Git's config variables. So a script
feeding git via GIT_AUTHOR_NAME, etc, shouldn't change at all with this
code. If your script is doing "git -c user.name=whatever commit", I
think you should reconsider your script. :)

So I dunno. I could really go either way on it. Feel free to drop it, or
even move it into a separate topic to be cooked longer.

-Peff

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Keep in mind this _only_ affects Git's config variables. So a script
> feeding git via GIT_AUTHOR_NAME, etc, shouldn't change at all with this
> code.

Ah, that changes the equation somewhat ;-)

> So I dunno. I could really go either way on it. Feel free to drop it, or
> even move it into a separate topic to be cooked longer.

If it were 5 years ago, it would have been different, but I do not
think cooking it longer in 'next' would smoke out breakages in
obscure scripts any longer.  Git is used by too many people who have
never seen its source these days.

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Jeff King]

On Thu, Feb 23, 2017 at 08:11:11PM -0800, Junio C Hamano wrote:

> > So I dunno. I could really go either way on it. Feel free to drop it, or
> > even move it into a separate topic to be cooked longer.
> 
> If it were 5 years ago, it would have been different, but I do not
> think cooking it longer in 'next' would smoke out breakages in
> obscure scripts any longer.  Git is used by too many people who have
> never seen its source these days.

Yeah, I have noticed that, too. I wonder if it would be interesting to
cut "weeklies" or something of "master" or even "next" that people could
install with a single click.

Of course it's not like we have a binary installer in the first place,
so I guess that's a prerequisite.

-Peff

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Dennis Kaarsemaker]

On Thu, 2017-02-23 at 23:18 -0500, Jeff King wrote:
> On Thu, Feb 23, 2017 at 08:11:11PM -0800, Junio C Hamano wrote:
> 
> > > So I dunno. I could really go either way on it. Feel free to drop it, or
> > > even move it into a separate topic to be cooked longer.
> > 
> > If it were 5 years ago, it would have been different, but I do not
> > think cooking it longer in 'next' would smoke out breakages in
> > obscure scripts any longer.  Git is used by too many people who have
> > never seen its source these days.
> 
> Yeah, I have noticed that, too. I wonder if it would be interesting to
> cut "weeklies" or something of "master" or even "next" that people could
> install with a single click.
> 
> Of course it's not like we have a binary installer in the first place,
> so I guess that's a prerequisite.

I provide daily[*] snapshots of git's master and next tree as packages
for Ubuntu, Debian, Fedora and CentOS on launchpad and SuSE's
openbuildservice. If there's sufficient interest in this (I know of
only a few users), I can try to put more effort into this.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

[*]When the tooling isn't broken for some reason.

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> On Thu, 2017-02-23 at 23:18 -0500, Jeff King wrote:
>> On Thu, Feb 23, 2017 at 08:11:11PM -0800, Junio C Hamano wrote:
>> 
>> > > So I dunno. I could really go either way on it. Feel free to drop it, or
>> > > even move it into a separate topic to be cooked longer.
>> > 
>> > If it were 5 years ago, it would have been different, but I do not
>> > think cooking it longer in 'next' would smoke out breakages in
>> > obscure scripts any longer.  Git is used by too many people who have
>> > never seen its source these days.
>> 
>> Yeah, I have noticed that, too. I wonder if it would be interesting to
>> cut "weeklies" or something of "master" or even "next" that people could
>> install with a single click.
>> 
>> Of course it's not like we have a binary installer in the first place,
>> so I guess that's a prerequisite.
>
> I provide daily[*] snapshots of git's master and next tree as packages
> for Ubuntu, Debian, Fedora and CentOS on launchpad and SuSE's
> openbuildservice. If there's sufficient interest in this (I know of
> only a few users), I can try to put more effort into this.

That sounds handy for people who do not build from the source
themselves.

Christian, perhaps rev-news can help advertising Dennis's effort to
recruit like-minded souls?

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Christian Couder]

On Mon, Feb 27, 2017 at 9:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>
>> On Thu, 2017-02-23 at 23:18 -0500, Jeff King wrote:
>>> On Thu, Feb 23, 2017 at 08:11:11PM -0800, Junio C Hamano wrote:
>>>
>>> > > So I dunno. I could really go either way on it. Feel free to drop it, or
>>> > > even move it into a separate topic to be cooked longer.
>>> >
>>> > If it were 5 years ago, it would have been different, but I do not
>>> > think cooking it longer in 'next' would smoke out breakages in
>>> > obscure scripts any longer.  Git is used by too many people who have
>>> > never seen its source these days.
>>>
>>> Yeah, I have noticed that, too. I wonder if it would be interesting to
>>> cut "weeklies" or something of "master" or even "next" that people could
>>> install with a single click.
>>>
>>> Of course it's not like we have a binary installer in the first place,
>>> so I guess that's a prerequisite.
>>
>> I provide daily[*] snapshots of git's master and next tree as packages
>> for Ubuntu, Debian, Fedora and CentOS on launchpad and SuSE's
>> openbuildservice. If there's sufficient interest in this (I know of
>> only a few users), I can try to put more effort into this.
>
> That sounds handy for people who do not build from the source
> themselves.
>
> Christian, perhaps rev-news can help advertising Dennis's effort to
> recruit like-minded souls?

Yeah, I had already noticed this thread and now Jakub has mentioned it on:

https://github.com/git/git.github.io/issues/231

so yeah we will advertise it one way or another.