{"thread":{"id":"50673","subject":"[BUG] GIT_AUTHOR_NAME was checked before prepare_fallback got called (ps/stash-in-c)","startedAt":"2019-03-06T19:52:42Z","lastAt":"2019-03-07T00:40:08Z","messageCount":4,"participants":["Denton Liu","Jeff King","Thomas Gummerer","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"370822","messageId":"20190306195236.GA22872@dev-l","threadId":"50673","inReplyTo":null,"subject":"[BUG] GIT_AUTHOR_NAME was checked before prepare_fallback got called (ps/stash-in-c)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-03-06T19:52:36Z","receivedAt":"2019-03-06T19:52:42Z","isPatch":false,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hello all,\n\nI've been on \"jch\" for my daily use and I noticed today that git stash\nisn't working. I managed to debug it down to \"ps/stash-in-c\".\n\nTo reproduce on git.git, it's simply the following:\n\n\techo // >>dir.c\n\tgit stash\n\nThis gives me the following error:\n\n\t$ git stash\n\tBUG: ident.c:511: GIT_AUTHOR_NAME was checked before prepare_fallback got called\n\tAborted (core dumped)\n\nI haven't read through the branch's code so I'm not too familiar with\nthe changes but please let me know if you need any other information or\nif there's anything I can help with.\n\nThanks,\n\nDenton\n"},{"id":"370824","messageId":"20190306200049.GA23315@sigill.intra.peff.net","threadId":"50673","inReplyTo":"20190306195236.GA22872@dev-l","subject":"Re: [BUG] GIT_AUTHOR_NAME was checked before prepare_fallback got called (ps/stash-in-c)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-06T20:00:49Z","receivedAt":"2019-03-06T20:00:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 06, 2019 at 11:52:36AM -0800, Denton Liu wrote:\n\n> Hello all,\n> \n> I've been on \"jch\" for my daily use and I noticed today that git stash\n> isn't working. I managed to debug it down to \"ps/stash-in-c\".\n> \n> To reproduce on git.git, it's simply the following:\n> \n> \techo // >>dir.c\n> \tgit stash\n> \n> This gives me the following error:\n> \n> \t$ git stash\n> \tBUG: ident.c:511: GIT_AUTHOR_NAME was checked before prepare_fallback got called\n> \tAborted (core dumped)\n> \n> I haven't read through the branch's code so I'm not too familiar with\n> the changes but please let me know if you need any other information or\n> if there's anything I can help with.\n\nYeah, it seems like the code from fd5a58477c (ident: add the ability to\nprovide a \"fallback identity\", 2019-02-25) is over-eager:\n\n  static void set_env_if(const char *key, const char *value, int *given, int bit)\n  {\n        if (*given & bit)\n                BUG(\"%s was checked before prepare_fallback got called\", key);\n\t...\n  }\n\n  void prepare_fallback_ident(const char *name, const char *email)\n  {\n        set_env_if(\"GIT_AUTHOR_NAME\", name,\n                   &author_ident_explicitly_given, IDENT_NAME_GIVEN);\n\t...\n  }\n\nIf the ident comes from config, then those bits will be set already,\neven if nobody ever looked at $GIT_AUTHOR_NAME. I think that BUG()\nshould actually just be a silent return.\n\n-Peff\n"},{"id":"370834","messageId":"20190306220911.GZ6085@hank.intra.tgummerer.com","threadId":"50673","inReplyTo":"20190306200049.GA23315@sigill.intra.peff.net","subject":"Re: [BUG] GIT_AUTHOR_NAME was checked before prepare_fallback got called (ps/stash-in-c)","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-06T22:09:11Z","receivedAt":"2019-03-06T22:09:17Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/06, Jeff King wrote:\n> On Wed, Mar 06, 2019 at 11:52:36AM -0800, Denton Liu wrote:\n> \n> > Hello all,\n> > \n> > I've been on \"jch\" for my daily use and I noticed today that git stash\n> > isn't working. I managed to debug it down to \"ps/stash-in-c\".\n> > \n> > To reproduce on git.git, it's simply the following:\n> > \n> > \techo // >>dir.c\n> > \tgit stash\n> > \n> > This gives me the following error:\n> > \n> > \t$ git stash\n> > \tBUG: ident.c:511: GIT_AUTHOR_NAME was checked before prepare_fallback got called\n> > \tAborted (core dumped)\n> > \n> > I haven't read through the branch's code so I'm not too familiar with\n> > the changes but please let me know if you need any other information or\n> > if there's anything I can help with.\n> \n> Yeah, it seems like the code from fd5a58477c (ident: add the ability to\n> provide a \"fallback identity\", 2019-02-25) is over-eager:\n> \n>   static void set_env_if(const char *key, const char *value, int *given, int bit)\n>   {\n>         if (*given & bit)\n>                 BUG(\"%s was checked before prepare_fallback got called\", key);\n> \t...\n>   }\n> \n>   void prepare_fallback_ident(const char *name, const char *email)\n>   {\n>         set_env_if(\"GIT_AUTHOR_NAME\", name,\n>                    &author_ident_explicitly_given, IDENT_NAME_GIVEN);\n> \t...\n>   }\n> \n> If the ident comes from config, then those bits will be set already,\n> even if nobody ever looked at $GIT_AUTHOR_NAME. I think that BUG()\n> should actually just be a silent return.\n\nEugh, I'm gonna go grab a brown paper bag.  This was actually working\ncorrectly before the last update I sent, where I applied Junios\nsuggestions from [*1*].\n\nThanks for the report Denton, and thanks for digging into it Peff!\n\nI was a bit puzzled why the test suite didn't catch this, but we only\nrun it with the environment variables set, not with the user.name and\nuser.email configs.  Even more awkwardly I did not use builtin stash\nsince I sent the last iteration.  Sorry about the mess :(\n\nHere's a patch that can either be squashed into 4/27 of the\nps/stash-in-c series, applied on top, or I can re-send the series with\nthe fix.  Junio, please let me know which way you'd prefer.  \n\n*1*: <xmqq4laxmmti.fsf@gitster-ct.c.googlers.com>\n\n--- >8 ---\nSubject: [PATCH] ident: don't require calling prepare_fallback_ident first\n\nIn fd5a58477c (\"ident: add the ability to provide a \"fallback\nidentity\"\", 2019-02-25) I made it a requirement to call\nprepare_fallback_ident as the first function in the ident API.\nHowever in stash we didn't actually end up following that.\n\nThis leads to a BUG if user.email and user.name are set.  It was not\ncaught in the test suite because we only rely on environment variables\nfor setting the user name and email instead of the config.\n\nInstead of making it a bug to call other functions in the ident API\nfirst, just return silently if the identity of a user was already set\nup.\n\nReported-by: Denton Liu <liu.denton@gmail.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n cache.h          | 1 -\n ident.c          | 4 +---\n t/t3903-stash.sh | 6 ++++++\n 3 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 611e554dea..67e74b7f75 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1493,7 +1493,6 @@ extern int is_terminal_dumb(void);\n extern int git_ident_config(const char *, const char *, void *);\n /*\n  * Prepare an ident to fall back on if the user didn't configure it.\n- * Must be called before any other function from the ident API.\n  */\n void prepare_fallback_ident(const char *name, const char *email);\n extern void reset_ident_date(void);\ndiff --git a/ident.c b/ident.c\nindex f30bd623f0..bce20e8652 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -507,9 +507,7 @@ int git_ident_config(const char *var, const char *value, void *data)\n \n static void set_env_if(const char *key, const char *value, int *given, int bit)\n {\n-\tif (*given & bit)\n-\t\tBUG(\"%s was checked before prepare_fallback got called\", key);\n-\tif (getenv(key))\n+\tif ((*given & bit) || getenv(key))\n \t\treturn; /* nothing to do */\n \tsetenv(key, value, 0);\n \t*given |= bit;\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 7dfa3a8038..97cc71fbaf 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1164,6 +1164,12 @@ test_expect_success 'stash -- <subdir> works with binary files' '\n \ttest_path_is_file subdir/untracked\n '\n \n+test_expect_success 'stash with user.name and user.email set works' '\n+\ttest_config user.name \"A U Thor\" &&\n+\ttest_config user.email \"a.u@thor\" &&\n+\tgit stash\n+'\n+\n test_expect_success 'stash works when user.name and user.email are not set' '\n \tgit reset &&\n \t>1 &&\n-- \n2.20.1.32.g82735acce8.dirty\n\n"},{"id":"370853","messageId":"xmqqimwvo6u4.fsf@gitster-ct.c.googlers.com","threadId":"50673","inReplyTo":"20190306220911.GZ6085@hank.intra.tgummerer.com","subject":"Re: [BUG] GIT_AUTHOR_NAME was checked before prepare_fallback got called (ps/stash-in-c)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-07T00:40:03Z","receivedAt":"2019-03-07T00:40:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> Here's a patch that can either be squashed into 4/27 of the\n> ps/stash-in-c series, applied on top, or I can re-send the series with\n> the fix.  Junio, please let me know which way you'd prefer.  \n\nThanks for a quick turnaround.  Let me see how squashing it into\n4/27 would make the resulting serues look like.\n\n"}]}