[PATCH] Trick to force setup of a specific configured E-Mail per repo
Previously, before 5498c57cdd63, many people did the following: git config --global user.email "(none)" This was helpful for people with more than one E-Mail address, targeting different E-Mail addresses for different clones. as it barred git from creating commit unless the user.email config was set in the per-clone config to the correct E-Mail address. Now, since the original 'bug' was fixed, and practically every string is acceptable for user.email and user.name, it is best to reimplement the feature not as an exploit of a bug, but as an actual feature. Signed-off-by: Dan Aloni --- Documentation/config.txt | 3 +++ ident.c | 5 + t/t9904-per-repo-email.sh | 26 ++ 3 files changed, 34 insertions(+) create mode 100755 t/t9904-per-repo-email.sh diff --git a/Documentation/config.txt b/Documentation/config.txt index f61788668e89..f9712e7c7752 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2769,6 +2769,9 @@ user.email:: Your email address to be recorded in any newly created commits. Can be overridden by the 'GIT_AUTHOR_EMAIL', 'GIT_COMMITTER_EMAIL', and 'EMAIL' environment variables. See linkgit:git-commit-tree[1]. + For people who seek setting different E-Mail addresses depending + on the clone, set to '(per-repo)' on the global configuration, + and Git will prompt you to set the E-Mail address in the clone. user.name:: Your full name to be recorded in any newly created commits. diff --git a/ident.c b/ident.c index daf7e1ea8370..0e07d45f8ff3 100644 --- a/ident.c +++ b/ident.c @@ -373,6 +373,11 @@ const char *fmt_ident(const char *name, const char *email, die("unable to auto-detect email address (got '%s')", email); } + if (strict && email && !strcmp(email, "(per-repo)")) { + die("email is '(per-repo)', suggesting to set specific email " + "for the current repo"); + } + strbuf_reset(&ident); if (want_name) { strbuf_addstr_without_crud(&ident, name); diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh new file mode 100755 index ..c085ba671b85 --- /dev/null +++ b/t/t9904-per-repo-email.sh @@ -0,0 +1,26 @@ +#!/bin/sh +# +# Copyright (c) 2016 Dan Aloni +# + +test_description='per-repo forced setting of E-Mail address' + +. ./test-lib.sh + +test_expect_failure 'fails commiting if clone email is not set' ' + echo "Initial" >foo && + git add foo && + unset GIT_AUTHOR_EMAIL && + git config --global user.email "(per-repo)" && + EDITOR=: VISUAL=: git commit -a -m x +' + +test_expect_success 'succeeds commiting if clone email is set' ' + echo "Initial" >foo && + git add foo && + git config --global user.email "(per-repo)" && + git config user.email "t...@ok.com" && + EDITOR=: VISUAL=: git commit -a -m x +' + +test_done -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo
On Tue, Feb 02, 2016 at 09:54:21PM +0200, Dan Aloni wrote: > Previously, before 5498c57cdd63, many people did the following: > >git config --global user.email "(none)" > > This was helpful for people with more than one E-Mail address, > targeting different E-Mail addresses for different clones. > as it barred git from creating commit unless the user.email > config was set in the per-clone config to the correct E-Mail > address. > > Now, since the original 'bug' was fixed, and practically every > string is acceptable for user.email and user.name, it is best > to reimplement the feature not as an exploit of a bug, but as > an actual feature. Just when I dare to think "somebody cannot possibly be relying on this arcane behavior", I am proven wrong. :) The motivating case for your patch makes sense to me. In the implementation, though: > + if (strict && email && !strcmp(email, "(per-repo)")) { > + die("email is '(per-repo)', suggesting to set specific email " > + "for the current repo"); > + } I find it disappointing that we go back to looking for magic sequences in the string. Could we perhaps do this more cleanly with a new config option? Like a "user.guessIdent" which defaults to true, but people can set to false. And without that, we do not do any automagic at all; we get the values from the GIT_COMMITTER_* environment or the user.{name,email} config variables, or we die(). I think that should allow your use case (and extend the same feature to user.name). It wouldn't work on older versions of git, but nor would your fix here (the only way to do that is to re-instate "(none)" as magical). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo
On Wed, Feb 3, 2016 at 10:56 AM, Jeff King wrote: > I find it disappointing that we go back to looking for magic sequences > in the string. Could we perhaps do this more cleanly with a new config > option? Like a "user.guessIdent" which defaults to true, but people can > set to false. And without that, we do not do any automagic at all; we > get the values from the GIT_COMMITTER_* environment or the > user.{name,email} config variables, or we die(). > > I think that should allow your use case (and extend the same feature to > user.name). It wouldn't work on older versions of git, but nor would > your fix here (the only way to do that is to re-instate "(none)" as > magical). Should we generalize this use case, i.e. define a list of configuration variables that must be (re-)defined per-repo? Maybe not worth it, I don't know. I can't think of any other variable that should behave this way off the top of my head. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo
On Wed, Feb 03, 2016 at 12:19:20PM +0700, Duy Nguyen wrote: > On Wed, Feb 3, 2016 at 10:56 AM, Jeff King wrote: > > I find it disappointing that we go back to looking for magic sequences > > in the string. Could we perhaps do this more cleanly with a new config > > option? Like a "user.guessIdent" which defaults to true, but people can > > set to false. And without that, we do not do any automagic at all; we > > get the values from the GIT_COMMITTER_* environment or the > > user.{name,email} config variables, or we die(). > > > > I think that should allow your use case (and extend the same feature to > > user.name). It wouldn't work on older versions of git, but nor would > > your fix here (the only way to do that is to re-instate "(none)" as > > magical). > > Should we generalize this use case, i.e. define a list of > configuration variables that must be (re-)defined per-repo? Maybe not > worth it, I don't know. I can't think of any other variable that > should behave this way off the top of my head. That's an interesting thought, but I'm not sure how it would work. The ident variables are special in that people are often unhappy with the fallback. What would it mean for somebody to say "do not proceed if diff.renameLimit is not set", and where would we enforce that? -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo
On Wed, Feb 3, 2016 at 12:22 PM, Jeff King wrote: > On Wed, Feb 03, 2016 at 12:19:20PM +0700, Duy Nguyen wrote: > >> On Wed, Feb 3, 2016 at 10:56 AM, Jeff King wrote: >> > I find it disappointing that we go back to looking for magic sequences >> > in the string. Could we perhaps do this more cleanly with a new config >> > option? Like a "user.guessIdent" which defaults to true, but people can >> > set to false. And without that, we do not do any automagic at all; we >> > get the values from the GIT_COMMITTER_* environment or the >> > user.{name,email} config variables, or we die(). >> > >> > I think that should allow your use case (and extend the same feature to >> > user.name). It wouldn't work on older versions of git, but nor would >> > your fix here (the only way to do that is to re-instate "(none)" as >> > magical). >> >> Should we generalize this use case, i.e. define a list of >> configuration variables that must be (re-)defined per-repo? Maybe not >> worth it, I don't know. I can't think of any other variable that >> should behave this way off the top of my head. > > That's an interesting thought, but I'm not sure how it would work. The > ident variables are special in that people are often unhappy with the > fallback. What would it mean for somebody to say "do not proceed if > diff.renameLimit is not set", and where would we enforce that? Enforcing it at git-init and git-clone, interactively asking the values of these variables, may be too much. If only we can catch if a variable is used, then we probably can catch it there. But that may require some more changes in config api. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo
On Wed, Feb 03, 2016 at 12:26:36PM +0700, Duy Nguyen wrote: > >> Should we generalize this use case, i.e. define a list of > >> configuration variables that must be (re-)defined per-repo? Maybe not > >> worth it, I don't know. I can't think of any other variable that > >> should behave this way off the top of my head. > > > > That's an interesting thought, but I'm not sure how it would work. The > > ident variables are special in that people are often unhappy with the > > fallback. What would it mean for somebody to say "do not proceed if > > diff.renameLimit is not set", and where would we enforce that? > > Enforcing it at git-init and git-clone, interactively asking the > values of these variables, may be too much. If only we can catch if a > variable is used, then we probably can catch it there. But that may > require some more changes in config api. Yeah, maybe. I do not want to say "definitely not", but I do think this is sufficiently more complicated than something like user.guessIdent that we should not make it a dependency. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo
Jeff King writes: > Just when I dare to think "somebody cannot possibly be relying on this > arcane behavior", I am proven wrong. :) My thoughts, exactly. This is the kind of thing that makes this project uncomfortably "interesting"; we cannot make improvements without risking breaking existing users. > Could we perhaps do this more cleanly with a new config > option? Like a "user.guessIdent" which defaults to true, but people can > set to false. And without that, we do not do any automagic at all; we > get the values from the GIT_COMMITTER_* environment or the > user.{name,email} config variables, or we die(). That sounds like an excellent and clean way to do this. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo
(resend - my mailer was misconfigured) On Tue, Feb 02, 2016 at 10:56:48PM -0500, Jeff King wrote: > On Tue, Feb 02, 2016 at 09:54:21PM +0200, Dan Aloni wrote: >[..] > > + if (strict && email && !strcmp(email, "(per-repo)")) { > > + die("email is '(per-repo)', suggesting to set specific email " > > + "for the current repo"); > > + } > > I find it disappointing that we go back to looking for magic sequences > in the string. Could we perhaps do this more cleanly with a new config > option? Like a "user.guessIdent" which defaults to true, but people can > set to false. And without that, we do not do any automagic at all; we > get the values from the GIT_COMMITTER_* environment or the > user.{name,email} config variables, or we die(). >[..] Agreed. New patch attached, feel free to amend. -- Dan Aloni >From 35d94d4a00af70eeaf6b291ec951f555b0bc99d3 Mon Sep 17 00:00:00 2001 From: Dan Aloni Date: Wed, 3 Feb 2016 10:09:40 +0200 Subject: [PATCH] Add user.explicit boolean for when ident shouldn't be guessed Previously, before 5498c57cdd63, many people did the following: git config --global user.email "(none)" This was helpful for people with more than one E-Mail address, targeting different E-Mail addresses for different clones. as it barred git from creating commit unless the user.email config was set in the per-repo config to the correct E-Mail address. Now, since the original 'bug' was fixed, and practically every string is acceptable for user.email and user.name, it is best to reimplement the feature not as an exploit of a bug, but as an actual feature. Signed-off-by: Dan Aloni --- Documentation/config.txt | 8 ident.c | 51 +++ t/t9904-per-repo-email.sh | 36 + 3 files changed, 95 insertions(+) create mode 100755 t/t9904-per-repo-email.sh diff --git a/Documentation/config.txt b/Documentation/config.txt index 877cbc875ec3..4c99f8f1de4f 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2791,6 +2791,14 @@ user.name:: Can be overridden by the 'GIT_AUTHOR_NAME' and 'GIT_COMMITTER_NAME' environment variables. See linkgit:git-commit-tree[1]. +user.explicit:: + This instruct Git to avoid trying to guess defaults for 'user.email' + and 'user.name' other than strictly from environment or config. + If you have multiply E-Mail addresses that you would like to set + up per repository, you may want to set this to 'true' in the global + config, and then Git would prompt you to set user.email separately, + in each of the cloned repositories. + user.signingKey:: If linkgit:git-tag[1] or linkgit:git-commit[1] is not selecting the key you want it to automatically when creating a signed tag or diff --git a/ident.c b/ident.c index 9dd3ae345255..305dc32a8eaf 100644 --- a/ident.c +++ b/ident.c @@ -18,6 +18,7 @@ static int default_name_is_bogus; #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN) static int committer_ident_explicitly_given; static int author_ident_explicitly_given; +static int ident_explicit = 0; #ifdef NO_GECOS_IN_PWENT #define get_gecos(ignored) "&" @@ -373,6 +374,23 @@ const char *fmt_ident(const char *name, const char *email, die("unable to auto-detect email address (got '%s')", email); } + if (ident_explicit) { + if (name == git_default_name.buf && + !(committer_ident_explicitly_given & IDENT_NAME_GIVEN) && + !(author_ident_explicitly_given & IDENT_NAME_GIVEN)) { + die("requested explicitly given ident in config, " + "but user.name is not set, or environment is " + "not set"); + } + if (email == git_default_email.buf && + !(committer_ident_explicitly_given & IDENT_MAIL_GIVEN) && + !(author_ident_explicitly_given & IDENT_MAIL_GIVEN)) { + die("requested explicitly given ident in config, " + "but user.email is not set, or environment is " + "not set"); + } + } + strbuf_reset(&ident); if (want_name) { strbuf_addstr_without_crud(&ident, name); @@ -405,6 +423,20 @@ const char *git_author_info(int flag) author_ident_explicitly_given |= IDENT_NAME_GIVEN; if (getenv("GIT_AUTHOR_EMAIL")) author_ident_explicitly_given |= IDENT_MAIL_GIVEN; + + if (ident_explicit) { + if (!(author_ident_explicitly_given & IDENT_NAME_GIVEN)) { + die("requested explicitly given ident in config, " + "but user.name is not set, or GIT_AUTHOR_NAME " + "is not set"); + } + if (!(author_ident_ex
Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo
On Wed, Feb 3, 2016 at 3:21 AM, Dan Aloni wrote: > On Tue, Feb 02, 2016 at 10:56:48PM -0500, Jeff King wrote: >> On Tue, Feb 02, 2016 at 09:54:21PM +0200, Dan Aloni wrote: >> > + if (strict && email && !strcmp(email, "(per-repo)")) { >> > + die("email is '(per-repo)', suggesting to set specific email " >> > + "for the current repo"); >> > + } >> >> I find it disappointing that we go back to looking for magic sequences >> in the string. Could we perhaps do this more cleanly with a new config >> option? Like a "user.guessIdent" which defaults to true, but people can >> set to false. And without that, we do not do any automagic at all; we >> get the values from the GIT_COMMITTER_* environment or the >> user.{name,email} config variables, or we die(). > > Agreed. New patch attached, feel free to amend. Please re-send with the patch inline since reviewers will want to comment on on specific bits of code inline as well. When the patch is an attachment, this process becomes too onerous for reviewers, and most will simply ignore the patch. Thanks. Before sending v3 (inline), perhaps take note of the few issues below which I noticed while quickly scanning the attachment: * The final paragraph of the commit message appears to be outdated since it still seems to be describing the approach taken by v1. * Elsewhere, in the project, the spelling "email" is preferred over "E-Mail"; that's true even in the files your patch is touching. * In the documentation:s/mutiply/multiple/. * The documentation doesn't seem to mention the default value of the new config variable. * The new config variable "user.explicit" has a more nebulous name than Peff's suggestion of "user.guessIdent", which may make its intent harder to determine without consulting documentation. * Don't initialize static variables to 0 (let the .bss section do that automatically). * One space before && operator; not two. * Drop unnecessary braces. * Perhaps use test_config(), test_unconfig(), test_config_global() in the test script rather than the manual git-config invocations in subshells. * test_expect_failure() is for indicating that a test will fail because some feature is known to be broken, not for when you expect a git command to fail in a controlled fashion. Instead, use test_expect_success, and then use test_must_fail() within the body of the test. test_expect_success '...' ' ... && test_must_fail git commit -m msg ' * Do these new tests really deserve a new test script, or would they fit into an existing script? (Genuine question.) It's also reviewer-friendly to indicate the patch revision in the subject "[PATCH v3]", and to describe what changed since the previous version of the patch. Providing a gmane link to the previous version is also very helpful. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html