threads / patch / 41878

patch, 2 partsident: make the useConfigOnly error messages more informative

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

## tl;dr

10 messages between Mar 30, 2016 and Apr 12, 2016. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Marios Titas· Mar 30, 2016, 19:29 UTC · lore

[PATCH 1/2] 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. 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(-)
Show changes to ident.c +6 −6
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
Marios Titas· Mar 30, 2016, 19:29 UTC · re: Marios Titas · lore

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(-)
Show changes to ident.c +8 −4
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
Junio C Hamano· Mar 30, 2016, 22:27 UTC · re: Marios Titas · lore

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

Marios Titas <redneb@gmx.com> writes:
Show 5 quoted lines
> -			    && !(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.

Marios Titas· Mar 30, 2016, 23:25 UTC · re: Junio C Hamano · lore

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

On Wed, Mar 30, 2016 at 03:27:05PM -0700, Junio C Hamano wrote:
Show 22 quoted lines
>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.

Junio C Hamano· Apr 1, 2016, 22:04 UTC · re: Marios Titas · lore

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

Marios Titas <redneb@gmx.com> writes:
Show 5 quoted lines
> 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(-)
Show changes to ident.c +8 −4
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
Junio C Hamano· Apr 12, 2016, 23:19 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> 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.
Show 50 quoted lines
> -- >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);
Jeff King· Mar 31, 2016, 14:40 UTC · re: Marios Titas · lore

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

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?

Show 16 quoted lines
> @@ -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
Marios Titas· Mar 31, 2016, 15:01 UTC · re: Jeff King · lore

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

On Thu, Mar 31, 2016 at 10:40:03AM -0400, Jeff King wrote:
Show 9 quoted lines
>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.

Jeff King· Mar 31, 2016, 16:31 UTC · re: Marios Titas · lore

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

On Thu, Mar 31, 2016 at 06:01:09PM +0300, Marios Titas wrote:
Show 26 quoted lines
> 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
Junio C Hamano· Apr 1, 2016, 22:00 UTC · re: Marios Titas · lore

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

Marios Titas <redneb@gmx.com> writes:
Show 26 quoted lines
> 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(-)
Show changes to ident.c +6 −6
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

← back to recent threads