possible bug: inconsistent CLI behaviour for empty user.name

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.

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

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

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

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(-)

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(+)

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(-)

Jeff King <peff@peff.net> writes:

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.

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

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).

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

Jeff King <peff@peff.net> writes:

Ah, that changes the equation somewhat ;-)

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.

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

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

On Thu, 2017-02-23 at 23:18 -0500, Jeff King wrote:

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 <dennis@kaarsemaker.net> writes:

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?

On Mon, Feb 27, 2017 at 9:42 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.