[BUG] GIT_AUTHOR_NAME was checked before prepare_fallback got called (ps/stash-in-c)

[BUG] GIT_AUTHOR_NAME was checked before prepare_fallback got called (ps/stash-in-c)

[Denton Liu]

Hello all,

I've been on "jch" for my daily use and I noticed today that git stash
isn't working. I managed to debug it down to "ps/stash-in-c".

To reproduce on git.git, it's simply the following:

	echo // >>dir.c
	git stash

This gives me the following error:

	$ git stash
	BUG: ident.c:511: GIT_AUTHOR_NAME was checked before prepare_fallback got called
	Aborted (core dumped)

I haven't read through the branch's code so I'm not too familiar with
the changes but please let me know if you need any other information or
if there's anything I can help with.

Thanks,

Denton

Re: [BUG] GIT_AUTHOR_NAME was checked before prepare_fallback got called (ps/stash-in-c)

[Jeff King]

On Wed, Mar 06, 2019 at 11:52:36AM -0800, Denton Liu wrote:

> Hello all,
> 
> I've been on "jch" for my daily use and I noticed today that git stash
> isn't working. I managed to debug it down to "ps/stash-in-c".
> 
> To reproduce on git.git, it's simply the following:
> 
> 	echo // >>dir.c
> 	git stash
> 
> This gives me the following error:
> 
> 	$ git stash
> 	BUG: ident.c:511: GIT_AUTHOR_NAME was checked before prepare_fallback got called
> 	Aborted (core dumped)
> 
> I haven't read through the branch's code so I'm not too familiar with
> the changes but please let me know if you need any other information or
> if there's anything I can help with.

Yeah, it seems like the code from fd5a58477c (ident: add the ability to
provide a "fallback identity", 2019-02-25) is over-eager:

  static void set_env_if(const char *key, const char *value, int *given, int bit)
  {
        if (*given & bit)
                BUG("%s was checked before prepare_fallback got called", key);
	...
  }

  void prepare_fallback_ident(const char *name, const char *email)
  {
        set_env_if("GIT_AUTHOR_NAME", name,
                   &author_ident_explicitly_given, IDENT_NAME_GIVEN);
	...
  }

If the ident comes from config, then those bits will be set already,
even if nobody ever looked at $GIT_AUTHOR_NAME. I think that BUG()
should actually just be a silent return.

-Peff

Re: [BUG] GIT_AUTHOR_NAME was checked before prepare_fallback got called (ps/stash-in-c)

[Thomas Gummerer]

On 03/06, Jeff King wrote:
> On Wed, Mar 06, 2019 at 11:52:36AM -0800, Denton Liu wrote:
> 
> > Hello all,
> > 
> > I've been on "jch" for my daily use and I noticed today that git stash
> > isn't working. I managed to debug it down to "ps/stash-in-c".
> > 
> > To reproduce on git.git, it's simply the following:
> > 
> > 	echo // >>dir.c
> > 	git stash
> > 
> > This gives me the following error:
> > 
> > 	$ git stash
> > 	BUG: ident.c:511: GIT_AUTHOR_NAME was checked before prepare_fallback got called
> > 	Aborted (core dumped)
> > 
> > I haven't read through the branch's code so I'm not too familiar with
> > the changes but please let me know if you need any other information or
> > if there's anything I can help with.
> 
> Yeah, it seems like the code from fd5a58477c (ident: add the ability to
> provide a "fallback identity", 2019-02-25) is over-eager:
> 
>   static void set_env_if(const char *key, const char *value, int *given, int bit)
>   {
>         if (*given & bit)
>                 BUG("%s was checked before prepare_fallback got called", key);
> 	...
>   }
> 
>   void prepare_fallback_ident(const char *name, const char *email)
>   {
>         set_env_if("GIT_AUTHOR_NAME", name,
>                    &author_ident_explicitly_given, IDENT_NAME_GIVEN);
> 	...
>   }
> 
> If the ident comes from config, then those bits will be set already,
> even if nobody ever looked at $GIT_AUTHOR_NAME. I think that BUG()
> should actually just be a silent return.

Eugh, I'm gonna go grab a brown paper bag.  This was actually working
correctly before the last update I sent, where I applied Junios
suggestions from [*1*].

Thanks for the report Denton, and thanks for digging into it Peff!

I was a bit puzzled why the test suite didn't catch this, but we only
run it with the environment variables set, not with the user.name and
user.email configs.  Even more awkwardly I did not use builtin stash
since I sent the last iteration.  Sorry about the mess :(

Here's a patch that can either be squashed into 4/27 of the
ps/stash-in-c series, applied on top, or I can re-send the series with
the fix.  Junio, please let me know which way you'd prefer.  

*1*: <xmqq4laxmmti.fsf@gitster-ct.c.googlers.com>

--- >8 ---
Subject: [PATCH] ident: don't require calling prepare_fallback_ident first

In fd5a58477c ("ident: add the ability to provide a "fallback
identity"", 2019-02-25) I made it a requirement to call
prepare_fallback_ident as the first function in the ident API.
However in stash we didn't actually end up following that.

This leads to a BUG if user.email and user.name are set.  It was not
caught in the test suite because we only rely on environment variables
for setting the user name and email instead of the config.

Instead of making it a bug to call other functions in the ident API
first, just return silently if the identity of a user was already set
up.

Reported-by: Denton Liu <liu.denton@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 cache.h          | 1 -
 ident.c          | 4 +---
 t/t3903-stash.sh | 6 ++++++
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 611e554dea..67e74b7f75 100644
--- a/cache.h
+++ b/cache.h
@@ -1493,7 +1493,6 @@ extern int is_terminal_dumb(void);
 extern int git_ident_config(const char *, const char *, void *);
 /*
  * Prepare an ident to fall back on if the user didn't configure it.
- * Must be called before any other function from the ident API.
  */
 void prepare_fallback_ident(const char *name, const char *email);
 extern void reset_ident_date(void);
diff --git a/ident.c b/ident.c
index f30bd623f0..bce20e8652 100644
--- a/ident.c
+++ b/ident.c
@@ -507,9 +507,7 @@ int git_ident_config(const char *var, const char *value, void *data)
 
 static void set_env_if(const char *key, const char *value, int *given, int bit)
 {
-	if (*given & bit)
-		BUG("%s was checked before prepare_fallback got called", key);
-	if (getenv(key))
+	if ((*given & bit) || getenv(key))
 		return; /* nothing to do */
 	setenv(key, value, 0);
 	*given |= bit;
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 7dfa3a8038..97cc71fbaf 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1164,6 +1164,12 @@ test_expect_success 'stash -- <subdir> works with binary files' '
 	test_path_is_file subdir/untracked
 '
 
+test_expect_success 'stash with user.name and user.email set works' '
+	test_config user.name "A U Thor" &&
+	test_config user.email "a.u@thor" &&
+	git stash
+'
+
 test_expect_success 'stash works when user.name and user.email are not set' '
 	git reset &&
 	>1 &&
-- 
2.20.1.32.g82735acce8.dirty

Re: [BUG] GIT_AUTHOR_NAME was checked before prepare_fallback got called (ps/stash-in-c)

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> Here's a patch that can either be squashed into 4/27 of the
> ps/stash-in-c series, applied on top, or I can re-send the series with
> the fix.  Junio, please let me know which way you'd prefer.  

Thanks for a quick turnaround.  Let me see how squashing it into
4/27 would make the resulting serues look like.