{"thread":{"id":"41327","subject":"[PATCH v4] Add user.useConfigOnly boolean for when ident shouldn't be guessed","startedAt":"2016-02-04T09:12:37Z","lastAt":"2016-02-05T06:47:54Z","messageCount":9,"participants":["Dan Aloni","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"277408","messageId":"1454577160-24484-1-git-send-email-alonid@gmail.com","threadId":"41327","inReplyTo":null,"subject":"[PATCH v4] Add user.useConfigOnly boolean for when ident shouldn't be guessed","fromName":"Dan Aloni","fromEmail":"alonid@gmail.com","sentAt":"2016-02-04T09:12:37Z","receivedAt":"2016-02-04T09:12:37Z","isPatch":true,"sender":{"key":"alonid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/321273?v=4"},"body":"\nv3->v4 changes:\n\n * Added cleanup patch for ident.c\n * Added Jeff's refactoring to fmt_indent\n * Renamed configuration option to useConfigOnly.\n * Extended tests to include a clone, EMAIL env.\n\nAlso, ran the entire Git test suite to make sure nothing's broken.\n"},{"id":"277409","messageId":"1454577160-24484-2-git-send-email-alonid@gmail.com","threadId":"41327","inReplyTo":"1454577160-24484-1-git-send-email-alonid@gmail.com","subject":"[PATCH v4 1/3] fmt_ident: refactor strictness checks","fromName":"Dan Aloni","fromEmail":"alonid@gmail.com","sentAt":"2016-02-04T09:12:38Z","receivedAt":"2016-02-04T09:12:38Z","isPatch":true,"sender":{"key":"alonid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/321273?v=4"},"body":"From: Jeff King \n\nThis function has evolved quite a bit over time, and as a\nresult, the logic for \"is this an OK ident\" has been\nsprinkled throughout. This ends up with a lot of redundant\nconditionals, like checking want_name repeatedly. Worse,\nwe want to know in many cases whether we are using the\n\"default\" ident, and we do so by comparing directly to the\nglobal strbuf, which violates the abstraction of the\nident_default_* functions.\n\nLet's reorganize the function into a hierarchy of\nconditionals to handle similar cases together. The only\ncase that doesn't just work naturally for this is that of an\nempty name, where our advice is different based on whether\nwe came from ident_default_name() or not. We can use a\nsimple flag to cover this case.\n\nSigned-off-by: Jeff King \n---\n ident.c | 46 ++++++++++++++++++++++++----------------------\n 1 file changed, 24 insertions(+), 22 deletions(-)\n\ndiff --git a/ident.c b/ident.c\nindex 3da555634290..f3a431f738cc 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -345,32 +345,34 @@ const char *fmt_ident(const char *name, const char *email,\n \tint want_date = !(flag & IDENT_NO_DATE);\n \tint want_name = !(flag & IDENT_NO_NAME);\n \n-\tif (want_name && !name)\n-\t\tname = ident_default_name();\n-\tif (!email)\n-\t\temail = ident_default_email();\n-\n-\tif (want_name && !*name) {\n-\t\tstruct passwd *pw;\n-\n-\t\tif (strict) {\n-\t\t\tif (name == git_default_name.buf)\n+\tif (want_name) {\n+\t\tint using_default = 0;\n+\t\tif (!name) {\n+\t\t\tname = ident_default_name();\n+\t\t\tusing_default = 1;\n+\t\t\tif (strict && default_name_is_bogus) {\n \t\t\t\tfputs(env_hint, stderr);\n-\t\t\tdie(\"empty ident name (for <%s>) not allowed\", email);\n+\t\t\t\tdie(\"unable to auto-detect name (got '%s')\", name);\n+\t\t\t}\n+\t\t}\n+\t\tif (!*name) {\n+\t\t\tstruct passwd *pw;\n+\t\t\tif (strict) {\n+\t\t\t\tif (using_default)\n+\t\t\t\t\tfputs(env_hint, stderr);\n+\t\t\t\tdie(\"empty ident name (for <%s>) not allowed\", email);\n+\t\t\t}\n+\t\t\tpw = xgetpwuid_self(NULL);\n+\t\t\tname = pw->pw_name;\n \t\t}\n-\t\tpw = xgetpwuid_self(NULL);\n-\t\tname = pw->pw_name;\n-\t}\n-\n-\tif (want_name && strict &&\n-\t name == git_default_name.buf && default_name_is_bogus) {\n-\t\tfputs(env_hint, stderr);\n-\t\tdie(\"unable to auto-detect name (got '%s')\", name);\n \t}\n \n-\tif (strict && email == git_default_email.buf && default_email_is_bogus) {\n-\t\tfputs(env_hint, stderr);\n-\t\tdie(\"unable to auto-detect email address (got '%s')\", email);\n+\tif (!email) {\n+\t\temail = ident_default_email();\n+\t\tif (strict && default_email_is_bogus) {\n+\t\t\tfputs(env_hint, stderr);\n+\t\t\tdie(\"unable to auto-detect email address (got '%s')\", email);\n+\t\t}\n \t}\n \n \tstrbuf_reset(&ident);\n-- \n2.5.0\n"},{"id":"277410","messageId":"1454577160-24484-3-git-send-email-alonid@gmail.com","threadId":"41327","inReplyTo":"1454577160-24484-1-git-send-email-alonid@gmail.com","subject":"[PATCH v4 2/3] Add user.useConfigOnly boolean for when ident shouldn't be guessed","fromName":"Dan Aloni","fromEmail":"alonid@gmail.com","sentAt":"2016-02-04T09:12:39Z","receivedAt":"2016-02-04T09:12:39Z","isPatch":true,"sender":{"key":"alonid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/321273?v=4"},"body":"Previously, before 5498c57cdd63, many people did the following:\n\n git config --global user.email \"(none)\"\n\nThis was helpful for people with more than one email address,\ntargeting different email addresses for different clones.\nas it barred git from creating commit unless the user.email\nconfig was set in the per-repo config to the correct email\naddress.\n\nThis commit provides the same functionality by adding a new\nconfiguration variable.\n\nSigned-off-by: Dan Aloni \nCc: Eric Sunshine \nCc: Jeff King \nCc: Git List \n---\n Documentation/config.txt | 9 ++++++++\n ident.c | 15 ++++++++++++\n t/t9904-per-repo-email.sh | 58 +++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 82 insertions(+)\n create mode 100755 t/t9904-per-repo-email.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 02bcde6bb596..25cf7ce4e83a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2821,6 +2821,15 @@ user.name::\n \tCan be overridden by the 'GIT_AUTHOR_NAME' and 'GIT_COMMITTER_NAME'\n \tenvironment variables. See linkgit:git-commit-tree[1].\n \n+user.useConfigOnly::\n+\tThis instruct Git to avoid trying to guess defaults for 'user.email'\n+\tand 'user.name' other than strictly from environment or config.\n+\tIf you have multiple email addresses that you would like to set\n+\tup per repository, you may want to set this to 'true' in the global\n+\tconfig, and then Git would prompt you to set user.email separately,\n+\tin each of the cloned repositories.\n+\tDefaults to `false`.\n+\n user.signingKey::\n \tIf linkgit:git-tag[1] or linkgit:git-commit[1] is not selecting the\n \tkey you want it to automatically when creating a signed tag or\ndiff --git a/ident.c b/ident.c\nindex f3a431f738cc..1216079d0b0d 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -13,11 +13,14 @@ static struct strbuf git_default_date = STRBUF_INIT;\n static int default_email_is_bogus;\n static int default_name_is_bogus;\n \n+static int ident_use_config_only;\n+\n #define IDENT_NAME_GIVEN 01\n #define IDENT_MAIL_GIVEN 02\n #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n static int committer_ident_explicitly_given;\n static int author_ident_explicitly_given;\n+static int ident_config_given;\n \n #ifdef NO_GECOS_IN_PWENT\n #define get_gecos(ignored) \"&\"\n@@ -354,6 +357,9 @@ const char *fmt_ident(const char *name, const char *email,\n \t\t\t\tfputs(env_hint, stderr);\n \t\t\t\tdie(\"unable to auto-detect name (got '%s')\", name);\n \t\t\t}\n+\t\t\tif (strict && ident_use_config_only &&\n+\t\t\t !(ident_config_given & IDENT_NAME_GIVEN))\n+\t\t\t\tdie(\"user.useConfigOnly set but no name given\");\n \t\t}\n \t\tif (!*name) {\n \t\t\tstruct passwd *pw;\n@@ -373,6 +379,8 @@ const char *fmt_ident(const char *name, const char *email,\n \t\t\tfputs(env_hint, stderr);\n \t\t\tdie(\"unable to auto-detect email address (got '%s')\", email);\n \t\t}\n+\t\tif (strict && ident_use_config_only && !(ident_config_given & IDENT_MAIL_GIVEN))\n+\t\t\tdie(\"user.useConfigOnly set but no mail given\");\n \t}\n \n \tstrbuf_reset(&ident);\n@@ -446,6 +454,11 @@ int author_ident_sufficiently_given(void)\n \n int git_ident_config(const char *var, const char *value, void *data)\n {\n+\tif (!strcmp(var, \"user.useconfigonly\")) {\n+\t\tident_use_config_only = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"user.name\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n@@ -453,6 +466,7 @@ int git_ident_config(const char *var, const char *value, void *data)\n \t\tstrbuf_addstr(&git_default_name, value);\n \t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\t\tident_config_given |= IDENT_NAME_GIVEN;\n \t\treturn 0;\n \t}\n \n@@ -463,6 +477,7 @@ int git_ident_config(const char *var, const char *value, void *data)\n \t\tstrbuf_addstr(&git_default_email, value);\n \t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\tident_config_given |= IDENT_MAIL_GIVEN;\n \t\treturn 0;\n \t}\n \ndiff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh\nnew file mode 100755\nindex 000000000000..9522a640951b\n--- /dev/null\n+++ b/t/t9904-per-repo-email.sh\n@@ -0,0 +1,58 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2016 Dan Aloni\n+#\n+\n+test_description='per-repo forced setting of email address'\n+\n+. ./test-lib.sh\n+\n+prepare () {\n+ # Have a non-empty repository\n+ rm -fr .git\n+ git init\n+\techo \"Initial\" >foo &&\n+ git add foo &&\n+ EDITOR=: VISUAL=: git commit -m foo &&\n+\n+\t# Setup a likely user.useConfigOnly use case\n+\tunset GIT_AUTHOR_NAME &&\n+\tunset GIT_AUTHOR_EMAIL &&\n+\ttest_unconfig --global user.name &&\n+\ttest_unconfig --global user.email &&\n+\ttest_config user.name \"test\" &&\n+\ttest_unconfig user.email &&\n+\ttest_config_global user.useConfigOnly true\n+}\n+\n+about_to_commit () {\n+\techo \"Second\" >>foo &&\n+\tgit add foo\n+}\n+\n+test_expect_success 'fails committing if clone email is not set' '\n+ prepare && about_to_commit &&\n+\n+\tEDITOR=: VISUAL=: test_must_fail git commit -m msg\n+'\n+\n+test_expect_success 'fails committing if clone email is not set, but EMAIL set' '\n+ prepare && about_to_commit &&\n+\n+\tEMAIL=test@fail.com EDITOR=: VISUAL=: test_must_fail git commit -m msg\n+'\n+\n+test_expect_success 'succeeds committing if clone email is set' '\n+ prepare && about_to_commit &&\n+\n+\ttest_config user.email \"test@ok.com\" &&\n+\tEDITOR=: VISUAL=: git commit -m msg\n+'\n+\n+test_expect_success 'succeeds cloning if global email is not set' '\n+ prepare &&\n+\n+ git clone . clone\n+'\n+\n+test_done\n-- \n2.5.0\n"},{"id":"277411","messageId":"1454577160-24484-4-git-send-email-alonid@gmail.com","threadId":"41327","inReplyTo":"1454577160-24484-1-git-send-email-alonid@gmail.com","subject":"[PATCH v4 3/3] ident.c: cleanup wrt ident's source","fromName":"Dan Aloni","fromEmail":"alonid@gmail.com","sentAt":"2016-02-04T09:12:40Z","receivedAt":"2016-02-04T09:12:40Z","isPatch":true,"sender":{"key":"alonid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/321273?v=4"},"body":" * Condense the variables that tells where we got the user's\n ident into single enum, instead of a collection of booleans.\n * Have {committer,author}_ident_sufficiently_given directly\n probe the environment and the afformentioned enum instead of\n relying on git_{committer,author}_info to do so.\n\nSigned-off-by: Dan Aloni \n---\n ident.c | 122 ++++++++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 77 insertions(+), 45 deletions(-)\n\ndiff --git a/ident.c b/ident.c\nindex 1216079d0b0d..b9aad38e0621 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -10,17 +10,19 @@\n static struct strbuf git_default_name = STRBUF_INIT;\n static struct strbuf git_default_email = STRBUF_INIT;\n static struct strbuf git_default_date = STRBUF_INIT;\n-static int default_email_is_bogus;\n-static int default_name_is_bogus;\n+\n+enum ident_source {\n+\tIDENT_SOURCE_UNKNOWN = 0,\n+\tIDENT_SOURCE_CONFIG,\n+\tIDENT_SOURCE_ENVIRONMENT,\n+\tIDENT_SOURCE_GUESSED,\n+\tIDENT_SOURCE_GUESSED_BOGUS,\n+};\n \n static int ident_use_config_only;\n \n-#define IDENT_NAME_GIVEN 01\n-#define IDENT_MAIL_GIVEN 02\n-#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n-static int committer_ident_explicitly_given;\n-static int author_ident_explicitly_given;\n-static int ident_config_given;\n+static enum ident_source source_of_default_email = IDENT_SOURCE_UNKNOWN;\n+static enum ident_source source_of_default_name = IDENT_SOURCE_UNKNOWN;\n \n #ifdef NO_GECOS_IN_PWENT\n #define get_gecos(ignored) \"&\"\n@@ -28,7 +30,7 @@ static int ident_config_given;\n #define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)\n #endif\n \n-static struct passwd *xgetpwuid_self(int *is_bogus)\n+static struct passwd *xgetpwuid_self(enum ident_source *source)\n {\n \tstruct passwd *pw;\n \n@@ -41,9 +43,13 @@ static struct passwd *xgetpwuid_self(int *is_bogus)\n \t\tfallback.pw_gecos = \"Unknown\";\n #endif\n \t\tpw = &fallback;\n-\t\tif (is_bogus)\n-\t\t\t*is_bogus = 1;\n+\t\tif (source)\n+\t\t\t*source = IDENT_SOURCE_GUESSED_BOGUS;\n+\t} else {\n+\t\tif (source)\n+\t\t\t*source = IDENT_SOURCE_GUESSED;\n \t}\n+\n \treturn pw;\n }\n \n@@ -120,26 +126,26 @@ static int canonical_name(const char *host, struct strbuf *out)\n \treturn status;\n }\n \n-static void add_domainname(struct strbuf *out, int *is_bogus)\n+static void add_domainname(struct strbuf *out, enum ident_source *source)\n {\n \tchar buf[1024];\n \n \tif (gethostname(buf, sizeof(buf))) {\n \t\twarning(\"cannot get host name: %s\", strerror(errno));\n \t\tstrbuf_addstr(out, \"(none)\");\n-\t\t*is_bogus = 1;\n+\t\t*source = IDENT_SOURCE_GUESSED_BOGUS;\n \t\treturn;\n \t}\n \tif (strchr(buf, '.'))\n \t\tstrbuf_addstr(out, buf);\n \telse if (canonical_name(buf, out) < 0) {\n \t\tstrbuf_addf(out, \"%s.(none)\", buf);\n-\t\t*is_bogus = 1;\n+\t\t*source = IDENT_SOURCE_GUESSED_BOGUS;\n \t}\n }\n \n static void copy_email(const struct passwd *pw, struct strbuf *email,\n-\t\t int *is_bogus)\n+\t\t enum ident_source *source)\n {\n \t/*\n \t * Make up a fake email address\n@@ -150,13 +156,13 @@ static void copy_email(const struct passwd *pw, struct strbuf *email,\n \n \tif (!add_mailname_host(email))\n \t\treturn;\t/* read from \"/etc/mailname\" (Debian) */\n-\tadd_domainname(email, is_bogus);\n+\tadd_domainname(email, source);\n }\n \n const char *ident_default_name(void)\n {\n \tif (!git_default_name.len) {\n-\t\tcopy_gecos(xgetpwuid_self(&default_name_is_bogus), &git_default_name);\n+\t\tcopy_gecos(xgetpwuid_self(&source_of_default_name), &git_default_name);\n \t\tstrbuf_trim(&git_default_name);\n \t}\n \treturn git_default_name.buf;\n@@ -169,11 +175,12 @@ const char *ident_default_email(void)\n \n \t\tif (email && email[0]) {\n \t\t\tstrbuf_addstr(&git_default_email, email);\n-\t\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n-\t\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n-\t\t} else\n-\t\t\tcopy_email(xgetpwuid_self(&default_email_is_bogus),\n-\t\t\t\t &git_default_email, &default_email_is_bogus);\n+\t\t\tsource_of_default_email = IDENT_SOURCE_ENVIRONMENT;\n+\t\t} else {\n+\t\t\tcopy_email(xgetpwuid_self(&source_of_default_email),\n+\t\t\t\t &git_default_email, &source_of_default_email);\n+\t\t}\n+\n \t\tstrbuf_trim(&git_default_email);\n \t}\n \treturn git_default_email.buf;\n@@ -353,12 +360,13 @@ const char *fmt_ident(const char *name, const char *email,\n \t\tif (!name) {\n \t\t\tname = ident_default_name();\n \t\t\tusing_default = 1;\n-\t\t\tif (strict && default_name_is_bogus) {\n+\t\t\tif (strict &&\n+\t\t\t source_of_default_name == IDENT_SOURCE_GUESSED_BOGUS) {\n \t\t\t\tfputs(env_hint, stderr);\n \t\t\t\tdie(\"unable to auto-detect name (got '%s')\", name);\n \t\t\t}\n \t\t\tif (strict && ident_use_config_only &&\n-\t\t\t !(ident_config_given & IDENT_NAME_GIVEN))\n+\t\t\t source_of_default_name != IDENT_SOURCE_CONFIG)\n \t\t\t\tdie(\"user.useConfigOnly set but no name given\");\n \t\t}\n \t\tif (!*name) {\n@@ -375,11 +383,12 @@ const char *fmt_ident(const char *name, const char *email,\n \n \tif (!email) {\n \t\temail = ident_default_email();\n-\t\tif (strict && default_email_is_bogus) {\n+\t\tif (strict && source_of_default_email == IDENT_SOURCE_GUESSED_BOGUS) {\n \t\t\tfputs(env_hint, stderr);\n \t\t\tdie(\"unable to auto-detect email address (got '%s')\", email);\n \t\t}\n-\t\tif (strict && ident_use_config_only && !(ident_config_given & IDENT_MAIL_GIVEN))\n+\t\tif (strict && ident_use_config_only &&\n+\t\t source_of_default_email != IDENT_SOURCE_CONFIG)\n \t\t\tdie(\"user.useConfigOnly set but no mail given\");\n \t}\n \n@@ -411,10 +420,6 @@ const char *fmt_name(const char *name, const char *email)\n \n const char *git_author_info(int flag)\n {\n-\tif (getenv(\"GIT_AUTHOR_NAME\"))\n-\t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n-\tif (getenv(\"GIT_AUTHOR_EMAIL\"))\n-\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \treturn fmt_ident(getenv(\"GIT_AUTHOR_NAME\"),\n \t\t\t getenv(\"GIT_AUTHOR_EMAIL\"),\n \t\t\t getenv(\"GIT_AUTHOR_DATE\"),\n@@ -423,33 +428,64 @@ const char *git_author_info(int flag)\n \n const char *git_committer_info(int flag)\n {\n-\tif (getenv(\"GIT_COMMITTER_NAME\"))\n-\t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n-\tif (getenv(\"GIT_COMMITTER_EMAIL\"))\n-\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \treturn fmt_ident(getenv(\"GIT_COMMITTER_NAME\"),\n \t\t\t getenv(\"GIT_COMMITTER_EMAIL\"),\n \t\t\t getenv(\"GIT_COMMITTER_DATE\"),\n \t\t\t flag);\n }\n \n-static int ident_is_sufficient(int user_ident_explicitly_given)\n+enum ident_person {\n+\tIDENT_PERSON_COMMITTER,\n+\tIDENT_PERSON_AUTHOR,\n+};\n+\n+static int ident_source_is_sufficient(enum ident_source source)\n+{\n+\tswitch (source) {\n+\tcase IDENT_SOURCE_CONFIG:\n+\tcase IDENT_SOURCE_ENVIRONMENT:\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int ident_is_sufficient(enum ident_person person)\n {\n+\tconst char *str_name, *str_email;\n+\tint name, email;\n+\n+\tswitch (person) {\n+\tcase IDENT_PERSON_COMMITTER:\n+\t\tstr_name = getenv(\"GIT_COMMITTER_NAME\");\n+\t\tstr_email = getenv(\"GIT_COMMITTER_EMAIL\");\n+\t\tbreak;\n+\tcase IDENT_PERSON_AUTHOR:\n+\t\tstr_name = getenv(\"GIT_AUTHOR_NAME\");\n+\t\tstr_email = getenv(\"GIT_AUTHOR_EMAIL\");\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"invalid parameter to ident_is_sufficient()\");\n+\t}\n+\n+\tname = !!str_name || ident_source_is_sufficient(source_of_default_name);\n+\temail = !!str_email || ident_source_is_sufficient(source_of_default_email);\n+\n #ifndef WINDOWS\n-\treturn (user_ident_explicitly_given & IDENT_MAIL_GIVEN);\n+\treturn email;\n #else\n-\treturn (user_ident_explicitly_given == IDENT_ALL_GIVEN);\n+\treturn name && email;\n #endif\n }\n \n int committer_ident_sufficiently_given(void)\n {\n-\treturn ident_is_sufficient(committer_ident_explicitly_given);\n+\treturn ident_is_sufficient(IDENT_PERSON_COMMITTER);\n }\n \n int author_ident_sufficiently_given(void)\n {\n-\treturn ident_is_sufficient(author_ident_explicitly_given);\n+\treturn ident_is_sufficient(IDENT_PERSON_AUTHOR);\n }\n \n int git_ident_config(const char *var, const char *value, void *data)\n@@ -464,9 +500,7 @@ int git_ident_config(const char *var, const char *value, void *data)\n \t\t\treturn config_error_nonbool(var);\n \t\tstrbuf_reset(&git_default_name);\n \t\tstrbuf_addstr(&git_default_name, value);\n-\t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n-\t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n-\t\tident_config_given |= IDENT_NAME_GIVEN;\n+\t\tsource_of_default_name = IDENT_SOURCE_CONFIG;\n \t\treturn 0;\n \t}\n \n@@ -475,9 +509,7 @@ int git_ident_config(const char *var, const char *value, void *data)\n \t\t\treturn config_error_nonbool(var);\n \t\tstrbuf_reset(&git_default_email);\n \t\tstrbuf_addstr(&git_default_email, value);\n-\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n-\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n-\t\tident_config_given |= IDENT_MAIL_GIVEN;\n+\t\tsource_of_default_email = IDENT_SOURCE_CONFIG;\n \t\treturn 0;\n \t}\n \n-- \n2.5.0\n"},{"id":"277462","messageId":"xmqqd1scgnyw.fsf@gitster.mtv.corp.google.com","threadId":"41327","inReplyTo":"1454577160-24484-4-git-send-email-alonid@gmail.com","subject":"Re: [PATCH v4 3/3] ident.c: cleanup wrt ident's source","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-04T21:33:27Z","receivedAt":"2016-02-04T21:33:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dan Aloni writes:\n\n> * Condense the variables that tells where we got the user's\n> ident into single enum, instead of a collection of booleans.\n> * Have {committer,author}_ident_sufficiently_given directly\n> probe the environment and the afformentioned enum instead of\n> relying on git_{committer,author}_info to do so.\n\nThat looks quite different from how we write our proposed log\nmessages.\n\n>\n> Signed-off-by: Dan Aloni \n> ---\n> ident.c | 122 ++++++++++++++++++++++++++++++++++++++++------------------------\n> 1 file changed, 77 insertions(+), 45 deletions(-)\n>\n> diff --git a/ident.c b/ident.c\n> index 1216079d0b0d..b9aad38e0621 100644\n> --- a/ident.c\n> +++ b/ident.c\n> @@ -10,17 +10,19 @@\n> static struct strbuf git_default_name = STRBUF_INIT;\n> static struct strbuf git_default_email = STRBUF_INIT;\n> static struct strbuf git_default_date = STRBUF_INIT;\n> -static int default_email_is_bogus;\n> -static int default_name_is_bogus;\n> +\n> +enum ident_source {\n> +\tIDENT_SOURCE_UNKNOWN = 0,\n> +\tIDENT_SOURCE_CONFIG,\n> +\tIDENT_SOURCE_ENVIRONMENT,\n> +\tIDENT_SOURCE_GUESSED,\n> +\tIDENT_SOURCE_GUESSED_BOGUS,\n> +};\n\nNo trailing comma after the last enum (some compliers choke on this\nIIRC).\n\nI skimmed the remainder of the patch but I am no the fence--I cannot\nquite see how this improves the readability of the result.\n\nThanks.\n"},{"id":"277464","messageId":"xmqq8u30gn1m.fsf@gitster.mtv.corp.google.com","threadId":"41327","inReplyTo":"1454577160-24484-3-git-send-email-alonid@gmail.com","subject":"Re: [PATCH v4 2/3] Add user.useConfigOnly boolean for when ident shouldn't be guessed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-04T21:53:25Z","receivedAt":"2016-02-04T21:53:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dan Aloni writes:\n\n> Previously, before 5498c57cdd63, many people did the following:\n>\n> git config --global user.email \"(none)\"\n>\n> This was helpful for people with more than one email address,\n> targeting different email addresses for different clones.\n> as it barred git from creating commit unless the user.email\n> config was set in the per-repo config to the correct email\n> address.\n>\n> This commit provides the same functionality by adding a new\n> configuration variable.\n\nThanks. I'd rather cite an individual commit, not a merge of a\ntopic, without forcing people to run \"git show\" only to see the\ntitle of the commit. Also I'd avoid \"was it that really that many\npeople did so?\" discussion by not saying \"many people\" altogether.\n\n\"by adding a new configuration variable\" is a bit weak. Help the\nreader by mentioning what it is called and what it does in the same\nsentence.\n\nPerhaps like this?\n\n-- >8 --\n ident: add user.useConfigOnly boolean for when ident shouldn't be guessed\n \n It used to be that:\n \n git config --global user.email \"(none)\"\n \n was a viable way for people to force themselves to set user.email in\n each repository. This was helpful for people with more than one\n email address, targeting different email addresses for different\n clones, as it barred git from creating commit unless the user.email\n config was set in the per-repo config to the correct email address.\n \n A recent change, 19ce497c (ident: keep a flag for bogus\n default_email, 2015-12-10), however declared that an explicitly\n configured user.email is not bogus, no matter what its value is, so\n this hack no longer works.\n \n Provide the same functionality by adding a new configuration\n variable user.useConfigOnly; when this variable is set, the\n user must explicitly set user.email configuration.\n \n Signed-off-by: Dan Aloni \n Helped-by: Jeff King \n Signed-off-by: Junio C Hamano \n-- 8< --\n\nThe logic in the patch is very cleanly written, partly thanks to the\nprevious step that was a real clean-up.\n\n> @@ -354,6 +357,9 @@ const char *fmt_ident(const char *name, const char *email,\n> \t\t\t\tfputs(env_hint, stderr);\n> \t\t\t\tdie(\"unable to auto-detect name (got '%s')\", name);\n> \t\t\t}\n> +\t\t\tif (strict && ident_use_config_only &&\n> +\t\t\t !(ident_config_given & IDENT_NAME_GIVEN))\n> +\t\t\t\tdie(\"user.useConfigOnly set but no name given\");\n> \t\t}\n> \t\tif (!*name) {\n> \t\t\tstruct passwd *pw;\n> @@ -373,6 +379,8 @@ const char *fmt_ident(const char *name, const char *email,\n> \t\t\tfputs(env_hint, stderr);\n> \t\t\tdie(\"unable to auto-detect email address (got '%s')\", email);\n> \t\t}\n> +\t\tif (strict && ident_use_config_only && !(ident_config_given & IDENT_MAIL_GIVEN))\n> +\t\t\tdie(\"user.useConfigOnly set but no mail given\");\n> \t}\n\nBy folding the line just like you did for \"name\" above, you do not\nhave to worry about an overlong line here.\n\n> diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh\n> new file mode 100755\n> index 000000000000..9522a640951b\n> --- /dev/null\n> +++ b/t/t9904-per-repo-email.sh\n> @@ -0,0 +1,58 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2016 Dan Aloni\n> +#\n> +\n> +test_description='per-repo forced setting of email address'\n> +\n> +. ./test-lib.sh\n> +\n> +prepare () {\n> + # Have a non-empty repository\n> + rm -fr .git\n> + git init\n> +\techo \"Initial\" >foo &&\n\nBy attempting to reply to a patch, one discovers that the patch has\nmixed indentation style ;-) Indent with tabs.\n\n> + git add foo &&\n> + EDITOR=: VISUAL=: git commit -m foo &&\n\nWhat is the point of these one-shot assignments to the environment\nvariables?\n\n\"git commit -m \" does not invoke the editor unless given \"-e\",\nand EDITOR=: is done early in test-lib.sh already, so I am puzzled.\n\nBesides, if you are worried about some stray environment variable,\noverriding EDITOR and VISUAL would not guard you against a stray\nGIT_EDITOR, which takes the precedence, I think.\n\n> +\t# Setup a likely user.useConfigOnly use case\n> +\tunset GIT_AUTHOR_NAME &&\n> +\tunset GIT_AUTHOR_EMAIL &&\n\nDoesn't unset fail when the variable is not set (we have sane_unset\nhelper for that)?\n\n> +\ttest_unconfig --global user.name &&\n> +\ttest_unconfig --global user.email &&\n> +\ttest_config user.name \"test\" &&\n> +\ttest_unconfig user.email &&\n> +\ttest_config_global user.useConfigOnly true\n> +}\n> +\n> +about_to_commit () {\n> +\techo \"Second\" >>foo &&\n> +\tgit add foo\n> +}\n> +\n> +test_expect_success 'fails committing if clone email is not set' '\n> + prepare && about_to_commit &&\n> +\n> +\tEDITOR=: VISUAL=: test_must_fail git commit -m msg\n\ntest_must_fail, being a shell function, does not follow the usual\n\"With 'VAR1=VAL2 VAR2=VAL2 thing', $VAR1 and $VAR2 get temporary\nvalues only during the execution of the thing\". If you really need\nto, you would need to do this like so\n\n\ttest_must_fail env EDITOR=: git commit -m msg\n\nBut again, I do not think you need to override EDITOR/VISUAL here,\nso...\n\n> +test_expect_success 'fails committing if clone email is not set, but EMAIL set' '\n> + prepare && about_to_commit &&\n> +\n> +\tEMAIL=test@fail.com EDITOR=: VISUAL=: test_must_fail git commit -m msg\n\nThis is a good place to use the \"test_must_fail env\" pattern, i.e.\n\n\ttest_must_fail env EMAIL=test@fail.com git commit -m msg\n\nI would think.\n\nThanks.\n"},{"id":"277479","messageId":"xmqqvb64f66o.fsf@gitster.mtv.corp.google.com","threadId":"41327","inReplyTo":"1454577160-24484-4-git-send-email-alonid@gmail.com","subject":"Re: [PATCH v4 3/3] ident.c: cleanup wrt ident's source","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-04T22:42:55Z","receivedAt":"2016-02-04T22:42:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dan Aloni writes:\n\n> +static int ident_source_is_sufficient(enum ident_source source)\n> +{\n> +\tswitch (source) {\n> +\tcase IDENT_SOURCE_CONFIG:\n> +\tcase IDENT_SOURCE_ENVIRONMENT:\n> +\t\treturn 1;\n\nWithout adding these two lines here:\n\n\tdefault:\n \tbreak;\n\nI get this build failure:\n\nident.c: In function 'ident_source_is_sufficient':\nident.c:444:2: error: enumeration value 'IDENT_SOURCE_UNKNOWN' not handled in switch [-Werror=switch]\n switch (source) {\n ^\nident.c:444:2: error: enumeration value 'IDENT_SOURCE_GUESSED' not handled in switch [-Werror=switch]\nident.c:444:2: error: enumeration value 'IDENT_SOURCE_GUESSED_BOGUS' not handled in switch [-Werror=switch]\n\n> +static int ident_is_sufficient(enum ident_person person)\n> {\n> +\tconst char *str_name, *str_email;\n> +\tint name, email;\n> +\n> +\tswitch (person) {\n> +\tcase IDENT_PERSON_COMMITTER:\n> +\t\tstr_name = getenv(\"GIT_COMMITTER_NAME\");\n> +\t\tstr_email = getenv(\"GIT_COMMITTER_EMAIL\");\n> +\t\tbreak;\n> +\tcase IDENT_PERSON_AUTHOR:\n> +\t\tstr_name = getenv(\"GIT_AUTHOR_NAME\");\n> +\t\tstr_email = getenv(\"GIT_AUTHOR_EMAIL\");\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tdie(\"invalid parameter to ident_is_sufficient()\");\n> +\t}\n> +\n> +\tname = !!str_name || ident_source_is_sufficient(source_of_default_name);\n> +\temail = !!str_email || ident_source_is_sufficient(source_of_default_email);\n> +\n> #ifndef WINDOWS\n> -\treturn (user_ident_explicitly_given & IDENT_MAIL_GIVEN);\n> +\treturn email;\n> #else\n> -\treturn (user_ident_explicitly_given == IDENT_ALL_GIVEN);\n> +\treturn name && email;\n> #endif\n> }\n\nIt appears that str_name and name are unconditionally computed even\nthough it does not affect the outcome of the whole thing. When\nbuilding for !WINDOWS, I get\n\nident.c: In function 'ident_is_sufficient':\nident.c:456:6: error: variable 'name' set but not used [-Werror=unused-but-set-variable]\n int name, email;\n ^\ncc1: all warnings being treated as errors\nmake: *** [ident.o] Error 1\n\nas the result of this.\n"},{"id":"277509","messageId":"20160205064055.GA15392@gmail.com","threadId":"41327","inReplyTo":"xmqqvb64f66o.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 3/3] ident.c: cleanup wrt ident's source","fromName":"Dan Aloni","fromEmail":"alonid@gmail.com","sentAt":"2016-02-05T06:40:55Z","receivedAt":"2016-02-05T06:40:55Z","isPatch":true,"sender":{"key":"alonid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/321273?v=4"},"body":"On Thu, Feb 04, 2016 at 02:42:55PM -0800, Junio C Hamano wrote:\n> Dan Aloni writes:\n> \n> > +static int ident_source_is_sufficient(enum ident_source source)\n> > +{\n> > +\tswitch (source) {\n> > +\tcase IDENT_SOURCE_CONFIG:\n> > +\tcase IDENT_SOURCE_ENVIRONMENT:\n> > +\t\treturn 1;\n> \n> Without adding these two lines here:\n> \n> \tdefault:\n> \tbreak;\n> \n> I get this build failure:\n> \n> ident.c: In function 'ident_source_is_sufficient':\n> ident.c:444:2: error: enumeration value 'IDENT_SOURCE_UNKNOWN' not handled in switch [-Werror=switch]\n> switch (source) {\n> ^\n> ident.c:444:2: error: enumeration value 'IDENT_SOURCE_GUESSED' not handled in switch [-Werror=switch]\n> ident.c:444:2: error: enumeration value 'IDENT_SOURCE_GUESSED_BOGUS' not handled in switch [-Werror=switch]\n\nMy fault - must have left over CFLAGS=\"-O0 -g\" while debugging, so it\ngot missed. Will fix.\n\n>[..]\n> ident.c: In function 'ident_is_sufficient':\n> ident.c:456:6: error: variable 'name' set but not used [-Werror=unused-but-set-variable]\n> int name, email;\n> ^\n> cc1: all warnings being treated as errors\n> make: *** [ident.o] Error 1\n\nDitto.\n\n-- \nDan Aloni\n"},{"id":"277510","messageId":"20160205064754.GB15392@gmail.com","threadId":"41327","inReplyTo":"xmqq8u30gn1m.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/3] Add user.useConfigOnly boolean for when ident shouldn't be guessed","fromName":"Dan Aloni","fromEmail":"alonid@gmail.com","sentAt":"2016-02-05T06:47:54Z","receivedAt":"2016-02-05T06:47:54Z","isPatch":true,"sender":{"key":"alonid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/321273?v=4"},"body":"On Thu, Feb 04, 2016 at 01:53:25PM -0800, Junio C Hamano wrote:\n>[..]\n> \"by adding a new configuration variable\" is a bit weak. Help the\n> reader by mentioning what it is called and what it does in the same\n> sentence.\n> \n> Perhaps like this?\n> \n> -- >8 --\n>[..]\n>\n\nLooks good, I'll just take that :)\n\n> ident: add user.useConfigOnly boolean for when ident shouldn't be guessed\n>[..]\n> > \t\t}\n> > +\t\tif (strict && ident_use_config_only && !(ident_config_given & IDENT_MAIL_GIVEN))\n> > +\t\t\tdie(\"user.useConfigOnly set but no mail given\");\n> > \t}\n> \n> By folding the line just like you did for \"name\" above, you do not\n> have to worry about an overlong line here.\n\nConsistency is important. Will fix here too, though it got fixed later\nin the cleanup.\n\n>[..]\n> > + git add foo &&\n> > + EDITOR=: VISUAL=: git commit -m foo &&\n> \n> What is the point of these one-shot assignments to the environment\n> variables?\n> \n> \"git commit -m \" does not invoke the editor unless given \"-e\",\n> and EDITOR=: is done early in test-lib.sh already, so I am puzzled.\n> \n> Besides, if you are worried about some stray environment variable,\n> overriding EDITOR and VISUAL would not guard you against a stray\n> GIT_EDITOR, which takes the precedence, I think.\n\nBeing new to this testing framework, I tried learning the trade from\nother tests. Maybe I goofed, or the other tests need cleaning?\n\n> \n> > +\t# Setup a likely user.useConfigOnly use case\n> > +\tunset GIT_AUTHOR_NAME &&\n> > +\tunset GIT_AUTHOR_EMAIL &&\n> \n> Doesn't unset fail when the variable is not set (we have sane_unset\n> helper for that)?\n\nSure.\n\n>[..] \n> > +test_expect_success 'fails committing if clone email is not set, but EMAIL set' '\n> > + prepare && about_to_commit &&\n> > +\n> > +\tEMAIL=test@fail.com EDITOR=: VISUAL=: test_must_fail git commit -m msg\n> \n> This is a good place to use the \"test_must_fail env\" pattern, i.e.\n> \n> \ttest_must_fail env EMAIL=test@fail.com git commit -m msg\n> \n> I would think.\n\nYes, and the fixed test still passes. Will resubmit the patches.\n\n-- \nDan Aloni\n"}]}