[PATCH 1/2] ident: check for useConfigOnly before auto-detection of name/email

[PATCH 1/2] ident: check for useConfigOnly before auto-detection of name/email

[Marios Titas]

If user.useConfigOnly is set, it does not make sense to try to
auto-detect the name and/or the email. So it's better to do the
useConfigOnly checks first.

Signed-off-by: Marios Titas <redneb@gmx.com>
---
 ident.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/ident.c b/ident.c
index 6e12582..74b2663 100644
--- a/ident.c
+++ b/ident.c
@@ -351,15 +351,15 @@ const char *fmt_ident(const char *name, const char *email,
 	if (want_name) {
 		int using_default = 0;
 		if (!name) {
+			if (strict && ident_use_config_only
+			    && !(ident_config_given & IDENT_NAME_GIVEN))
+				die("user.useConfigOnly set but no name given");
 			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);
 			}
-			if (strict && ident_use_config_only
-			    && !(ident_config_given & IDENT_NAME_GIVEN))
-				die("user.useConfigOnly set but no name given");
 		}
 		if (!*name) {
 			struct passwd *pw;
@@ -374,14 +374,14 @@ const char *fmt_ident(const char *name, const char *email,
 	}
 
 	if (!email) {
+		if (strict && ident_use_config_only
+		    && !(ident_config_given & IDENT_MAIL_GIVEN))
+			die("user.useConfigOnly set but no mail given");
 		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 (strict && ident_use_config_only
-		    && !(ident_config_given & IDENT_MAIL_GIVEN))
-			die("user.useConfigOnly set but no mail given");
 	}
 
 	strbuf_reset(&ident);
-- 
2.8.0

[PATCH 2/2] ident: make the useConfigOnly error messages more informative

[Marios Titas]

The env_hint message applies perfectly to the case when
user.useConfigOnly is set and at least one of the user.name and the
user.email are not provided. Additionally, use a more descriptive error
message when that happens.

Signed-off-by: Marios Titas <redneb@gmx.com>
---
 ident.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/ident.c b/ident.c
index 74b2663..4fd82d1 100644
--- a/ident.c
+++ b/ident.c
@@ -352,8 +352,10 @@ const char *fmt_ident(const char *name, const char *email,
 		int using_default = 0;
 		if (!name) {
 			if (strict && ident_use_config_only
-			    && !(ident_config_given & IDENT_NAME_GIVEN))
-				die("user.useConfigOnly set but no name given");
+			    && !(ident_config_given & IDENT_NAME_GIVEN)) {
+				fputs(env_hint, stderr);
+				die("no name was given and auto-detection is disabled");
+			}
 			name = ident_default_name();
 			using_default = 1;
 			if (strict && default_name_is_bogus) {
@@ -375,8 +377,10 @@ const char *fmt_ident(const char *name, const char *email,
 
 	if (!email) {
 		if (strict && ident_use_config_only
-		    && !(ident_config_given & IDENT_MAIL_GIVEN))
-			die("user.useConfigOnly set but no mail given");
+		    && !(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);
-- 
2.8.0

Re: [PATCH 2/2] ident: make the useConfigOnly error messages more informative

[Junio C Hamano]

Marios Titas <redneb@gmx.com> writes:

> -			    && !(ident_config_given & IDENT_NAME_GIVEN))
> -				die("user.useConfigOnly set but no name given");
> +			    && !(ident_config_given & IDENT_NAME_GIVEN)) {
> +				fputs(env_hint, stderr);
> +				die("no name was given and auto-detection is disabled

Hmph.  I do not think that this is making the message "more
informative".

When a user hits this error, the old message allowed the user to
easily see how to toggle the "disable auto-detection" bit off to let
the code continue by telling the name of the configuration, but the
updated message hides that name, making it harder for the user to
disable the disabling of auto-detection.

I can buy the argument that this change helps the user by making the
message "less" informative, though.  By discouraging the users from
toggling the user.useConfigOnly bit off, it indirectly makes the
other option to work around this error condition, i.e. giving a name
more explicitly, more appetizing.

Re: [PATCH 2/2] ident: make the useConfigOnly error messages more informative

[Marios Titas]

On Wed, Mar 30, 2016 at 03:27:05PM -0700, Junio C Hamano wrote:
>Marios Titas <redneb@gmx.com> writes:
>
>> -			    && !(ident_config_given & IDENT_NAME_GIVEN))
>> -				die("user.useConfigOnly set but no name given");
>> +			    && !(ident_config_given & IDENT_NAME_GIVEN)) {
>> +				fputs(env_hint, stderr);
>> +				die("no name was given and auto-detection is disabled
>
>Hmph.  I do not think that this is making the message "more
>informative".
>
>When a user hits this error, the old message allowed the user to
>easily see how to toggle the "disable auto-detection" bit off to let
>the code continue by telling the name of the configuration, but the
>updated message hides that name, making it harder for the user to
>disable the disabling of auto-detection.
>
>I can buy the argument that this change helps the user by making the
>message "less" informative, though.  By discouraging the users from
>toggling the user.useConfigOnly bit off, it indirectly makes the
>other option to work around this error condition, i.e. giving a name
>more explicitly, more appetizing.

Yeah, maybe informative is not the right word. What I meant is that it 
directs the user to do the "git config user.name" thing, which is likely 
the most appropriate course of action in this situation. In any event, I 
think printing the env_hint message would be really helpful in this 
case.

Re: [PATCH 2/2] ident: make the useConfigOnly error messages more informative

[Junio C Hamano]

Marios Titas <redneb@gmx.com> writes:

> Yeah, maybe informative is not the right word. What I meant is that it
> directs the user to do the "git config user.name" thing, which is
> likely the most appropriate course of action in this situation. In any
> event, I think printing the env_hint message would be really helpful
> in this case.

OK, let's do this, then.

Thanks.

-- >8 --
From: Marios Titas <redneb@gmx.com>
Date: Wed, 30 Mar 2016 22:29:43 +0300
Subject: [PATCH] ident: give "please tell me" message upon useConfigOnly error

The env_hint message applies perfectly to the case when
user.useConfigOnly is set and at least one of the user.name and the
user.email are not provided.

Additionally, use a less descriptive error message to discourage
users from disabling user.useConfigOnly configuration variable to
work around this error condition.  We want to encourage them to set
user.name or user.email instead.

Signed-off-by: Marios Titas <redneb@gmx.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ident.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/ident.c b/ident.c
index b2521ff..c766127 100644
--- a/ident.c
+++ b/ident.c
@@ -352,8 +352,10 @@ const char *fmt_ident(const char *name, const char *email,
 		int using_default = 0;
 		if (!name) {
 			if (strict && ident_use_config_only
-			    && !(ident_config_given & IDENT_NAME_GIVEN))
-				die("user.useConfigOnly set but no name given");
+			    && !(ident_config_given & IDENT_NAME_GIVEN)) {
+				fputs(env_hint, stderr);
+				die("no name was given and auto-detection is disabled");
+			}
 			name = ident_default_name();
 			using_default = 1;
 			if (strict && default_name_is_bogus) {
@@ -375,8 +377,10 @@ const char *fmt_ident(const char *name, const char *email,
 
 	if (!email) {
 		if (strict && ident_use_config_only
-		    && !(ident_config_given & IDENT_MAIL_GIVEN))
-			die("user.useConfigOnly set but no mail given");
+		    && !(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);
-- 
2.8.0-246-g1783343

Re: [PATCH 2/2] ident: make the useConfigOnly error messages more informative

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Marios Titas <redneb@gmx.com> writes:
>
>> Yeah, maybe informative is not the right word. What I meant is that it
>> directs the user to do the "git config user.name" thing, which is
>> likely the most appropriate course of action in this situation. In any
>> event, I think printing the env_hint message would be really helpful
>> in this case.
>
> OK, let's do this, then.
>
> Thanks.

A friendly ping asking for an Ack.

> -- >8 --
> From: Marios Titas <redneb@gmx.com>
> Date: Wed, 30 Mar 2016 22:29:43 +0300
> Subject: [PATCH] ident: give "please tell me" message upon useConfigOnly error
>
> The env_hint message applies perfectly to the case when
> user.useConfigOnly is set and at least one of the user.name and the
> user.email are not provided.
>
> Additionally, use a less descriptive error message to discourage
> users from disabling user.useConfigOnly configuration variable to
> work around this error condition.  We want to encourage them to set
> user.name or user.email instead.
>
> Signed-off-by: Marios Titas <redneb@gmx.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  ident.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/ident.c b/ident.c
> index b2521ff..c766127 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -352,8 +352,10 @@ const char *fmt_ident(const char *name, const char *email,
>  		int using_default = 0;
>  		if (!name) {
>  			if (strict && ident_use_config_only
> -			    && !(ident_config_given & IDENT_NAME_GIVEN))
> -				die("user.useConfigOnly set but no name given");
> +			    && !(ident_config_given & IDENT_NAME_GIVEN)) {
> +				fputs(env_hint, stderr);
> +				die("no name was given and auto-detection is disabled");
> +			}
>  			name = ident_default_name();
>  			using_default = 1;
>  			if (strict && default_name_is_bogus) {
> @@ -375,8 +377,10 @@ const char *fmt_ident(const char *name, const char *email,
>  
>  	if (!email) {
>  		if (strict && ident_use_config_only
> -		    && !(ident_config_given & IDENT_MAIL_GIVEN))
> -			die("user.useConfigOnly set but no mail given");
> +		    && !(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);

Re: [PATCH 1/2] ident: check for useConfigOnly before auto-detection of name/email

[Jeff King]

On Wed, Mar 30, 2016 at 10:29:42PM +0300, Marios Titas wrote:

> If user.useConfigOnly is set, it does not make sense to try to
> auto-detect the name and/or the email. So it's better to do the
> useConfigOnly checks first.

It might be nice to explain how it is better here. I'd guess it is
because we may fail during xgetpwuid(), giving a message that is much
less informative?

> @@ -374,14 +374,14 @@ const char *fmt_ident(const char *name, const char *email,
>  	}
>  
>  	if (!email) {
> +		if (strict && ident_use_config_only
> +		    && !(ident_config_given & IDENT_MAIL_GIVEN))
> +			die("user.useConfigOnly set but no mail given");
>  		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 (strict && ident_use_config_only
> -		    && !(ident_config_given & IDENT_MAIL_GIVEN))
> -			die("user.useConfigOnly set but no mail given");
>  	}

I wondered on this hunk whether ident_default_email() could ever set the
IDENT_MAIL_GIVEN flag. It _does_ set it, but only for
"explicitly_given", not for "config_given", which makes sense.

So I think this is doing the right thing.

-Peff

Re: [PATCH 1/2] ident: check for useConfigOnly before auto-detection of name/email

[Marios Titas]

On Thu, Mar 31, 2016 at 10:40:03AM -0400, Jeff King wrote:
>On Wed, Mar 30, 2016 at 10:29:42PM +0300, Marios Titas wrote:
>
>> If user.useConfigOnly is set, it does not make sense to try to
>> auto-detect the name and/or the email. So it's better to do the
>> useConfigOnly checks first.
>
>It might be nice to explain how it is better here. I'd guess it is
>because we may fail during xgetpwuid(), giving a message that is much
>less informative?

Oops sorry, my bad, I should have included an example in the commit 
message. So with git 2.8.0, if you provide a name and set useConfigOnly 
to true in your ~/.gitconfig file, then if try to commit something in a 
new git repo, it will fail with the following message:

    *** Please tell me who you are.
    
    Run
    
      git config --global user.email "you@example.com"
      git config --global user.name "Your Name"
    
    to set your account's default identity.
    Omit --global to set the identity only in this repository.
    
    fatal: unable to auto-detect email address (got 'XXX@YYY.(none)')

(provided of course that auto-detection of email fails). This wrong, 
because auto-detection is disabled anyway.

Re: [PATCH 1/2] ident: check for useConfigOnly before auto-detection of name/email

[Jeff King]

On Thu, Mar 31, 2016 at 06:01:09PM +0300, Marios Titas wrote:

> On Thu, Mar 31, 2016 at 10:40:03AM -0400, Jeff King wrote:
> >On Wed, Mar 30, 2016 at 10:29:42PM +0300, Marios Titas wrote:
> >
> >>If user.useConfigOnly is set, it does not make sense to try to
> >>auto-detect the name and/or the email. So it's better to do the
> >>useConfigOnly checks first.
> >
> >It might be nice to explain how it is better here. I'd guess it is
> >because we may fail during xgetpwuid(), giving a message that is much
> >less informative?
> 
> Oops sorry, my bad, I should have included an example in the commit message.
> So with git 2.8.0, if you provide a name and set useConfigOnly to true in
> your ~/.gitconfig file, then if try to commit something in a new git repo,
> it will fail with the following message:
> 
>    *** Please tell me who you are.
>    Run
>      git config --global user.email "you@example.com"
>      git config --global user.name "Your Name"
>    to set your account's default identity.
>    Omit --global to set the identity only in this repository.
>    fatal: unable to auto-detect email address (got 'XXX@YYY.(none)')
> 
> (provided of course that auto-detection of email fails). This wrong, because
> auto-detection is disabled anyway.

Ah, right. We used to die in xgetpwuid, but now we just set
default_name_is_bogus. So I think bumping the use_config_only check
above the name_is_bogus check would be sufficient. Where you put it
(above ident_default_name) is fine, though it would be a problem if we
later lazily loaded the config in that function (I don't have any
particular plans to do so, though).

-Peff

Re: [PATCH 1/2] ident: check for useConfigOnly before auto-detection of name/email

[Junio C Hamano]

Marios Titas <redneb@gmx.com> writes:

> On Thu, Mar 31, 2016 at 10:40:03AM -0400, Jeff King wrote:
>>On Wed, Mar 30, 2016 at 10:29:42PM +0300, Marios Titas wrote:
>>
>>> If user.useConfigOnly is set, it does not make sense to try to
>>> auto-detect the name and/or the email. So it's better to do the
>>> useConfigOnly checks first.
>>
>>It might be nice to explain how it is better here. I'd guess it is
>>because we may fail during xgetpwuid(), giving a message that is much
>>less informative?
>
> Oops sorry, my bad, I should have included an example in the commit
> message. So with git 2.8.0, if you provide a name and set
> useConfigOnly to true in your ~/.gitconfig file, then if try to commit
> something in a new git repo, it will fail with the following message:
>
>    *** Please tell me who you are.
>     Run
>       git config --global user.email "you@example.com"
>      git config --global user.name "Your Name"
>     to set your account's default identity.
>    Omit --global to set the identity only in this repository.
>     fatal: unable to auto-detect email address (got 'XXX@YYY.(none)')
>
> (provided of course that auto-detection of email fails). This wrong,
> because auto-detection is disabled anyway.

OK, let's do this, then.

-- >8 --
From: Marios Titas <redneb@gmx.com>
Date: Wed, 30 Mar 2016 22:29:42 +0300
Subject: [PATCH] ident: check for useConfigOnly before auto-detection of
 name/email

If user.useConfigOnly is set, it does not make sense to try to
auto-detect the name and/or the email.  The auto-detection may
even result in a bogus name and trigger an error message.

Check if the use-config-only is set and die if no explicit name was
given, before attempting to auto-detect, to correct this.

Signed-off-by: Marios Titas <redneb@gmx.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ident.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/ident.c b/ident.c
index 4bd8084..b2521ff 100644
--- a/ident.c
+++ b/ident.c
@@ -351,15 +351,15 @@ const char *fmt_ident(const char *name, const char *email,
 	if (want_name) {
 		int using_default = 0;
 		if (!name) {
+			if (strict && ident_use_config_only
+			    && !(ident_config_given & IDENT_NAME_GIVEN))
+				die("user.useConfigOnly set but no name given");
 			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);
 			}
-			if (strict && ident_use_config_only
-			    && !(ident_config_given & IDENT_NAME_GIVEN))
-				die("user.useConfigOnly set but no name given");
 		}
 		if (!*name) {
 			struct passwd *pw;
@@ -374,14 +374,14 @@ const char *fmt_ident(const char *name, const char *email,
 	}
 
 	if (!email) {
+		if (strict && ident_use_config_only
+		    && !(ident_config_given & IDENT_MAIL_GIVEN))
+			die("user.useConfigOnly set but no mail given");
 		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 (strict && ident_use_config_only
-		    && !(ident_config_given & IDENT_MAIL_GIVEN))
-			die("user.useConfigOnly set but no mail given");
 	}
 
 	strbuf_reset(&ident);
-- 
2.8.0-246-g1783343