[PATCH] Trick to force setup of a specific configured E-Mail per repo

[PATCH] Trick to force setup of a specific configured E-Mail per repo

[Dan Aloni]

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 <alonid@gmail.com>
---
 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 000000000000..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 "test@ok.com" &&
+	EDITOR=: VISUAL=: git commit -a -m x
+'
+
+test_done
-- 
2.5.0

Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo

[Jeff King]

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

Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo

[Duy Nguyen]

On Wed, Feb 3, 2016 at 10:56 AM, Jeff King <peff@peff.net> 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

Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo

[Jeff King]

On Wed, Feb 03, 2016 at 12:19:20PM +0700, Duy Nguyen wrote:

> On Wed, Feb 3, 2016 at 10:56 AM, Jeff King <peff@peff.net> 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

Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo

[Duy Nguyen]

On Wed, Feb 3, 2016 at 12:22 PM, Jeff King <peff@peff.net> 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 <peff@peff.net> 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

Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo

[Jeff King]

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

Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo

[Dan Aloni]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

(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


[-- Attachment #2: 0001-Add-user.explicit-boolean-for-when-ident-shouldn-t-b.patch --]
[-- Type: text/plain, Size: 6320 bytes --]

>From 35d94d4a00af70eeaf6b291ec951f555b0bc99d3 Mon Sep 17 00:00:00 2001
From: Dan Aloni <alonid@gmail.com>
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 <alonid@gmail.com>
---
 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_explicitly_given & IDENT_MAIL_GIVEN)) {
+			die("requested explicitly given ident in config, "
+			    "but user.email is not set, or GIT_AUTHOR_EMAIL "
+			    "is not set");
+		}
+	}
+
 	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
 			 getenv("GIT_AUTHOR_EMAIL"),
 			 getenv("GIT_AUTHOR_DATE"),
@@ -417,6 +449,20 @@ const char *git_committer_info(int flag)
 		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
 	if (getenv("GIT_COMMITTER_EMAIL"))
 		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+
+	if (ident_explicit) {
+		if (!(committer_ident_explicitly_given & IDENT_NAME_GIVEN)) {
+			die("requested explicitly given ident in config, "
+			    "but user.name is not set, or GIT_COMMITTER_NAME "
+			    "is not set");
+		}
+		if (!(committer_ident_explicitly_given & IDENT_MAIL_GIVEN)) {
+			die("requested explicitly given ident in config, "
+			    "but user.email is not set, or GIT_COMMITTER_EMAIL "
+			    "is not set");
+		}
+	}
+
 	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
 			 getenv("GIT_COMMITTER_EMAIL"),
 			 getenv("GIT_COMMITTER_DATE"),
@@ -444,6 +490,11 @@ int author_ident_sufficiently_given(void)
 
 int git_ident_config(const char *var, const char *value, void *data)
 {
+	if (!strcmp(var, "user.explicit")) {
+		ident_explicit = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "user.name")) {
 		if (!value)
 			return config_error_nonbool(var);
diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh
new file mode 100755
index 000000000000..5876c9150ca6
--- /dev/null
+++ b/t/t9904-per-repo-email.sh
@@ -0,0 +1,36 @@
+#!/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_NAME &&
+	unset GIT_AUTHOR_EMAIL &&
+	(git config --global --unset user.name || true) &&
+	(git config --global --unset user.email || true) &&
+	(git config --unset user.name || true) &&
+	(git config --unset user.email || true) &&
+	git config --global user.explicit true &&
+	EDITOR=: VISUAL=: git commit -m msg
+
+'
+
+test_expect_success 'succeeds commiting if clone email is set' '
+	echo "Initial" >foo &&
+	git add foo &&
+	unset GIT_AUTHOR_EMAIL &&
+	(git config --global --unset user.name || true) &&
+	(git config --global --unset user.email || true) &&
+	git config --global user.explicit true &&
+	git config user.name "test" &&
+	git config user.email "test@ok.com" &&
+	EDITOR=: VISUAL=: git commit -m msg
+'
+
+test_done
-- 
2.5.0

Re: [PATCH] Trick to force setup of a specific configured E-Mail per repo

[Eric Sunshine]

On Wed, Feb 3, 2016 at 3:21 AM, Dan Aloni <alonid@gmail.com> 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.

[PATCH v3] Add user.explicit boolean for when ident shouldn't be guessed

[Dan Aloni]

On Wed, Feb 03, 2016 at 12:47:56PM -0500, Eric Sunshine wrote:
>[..]
> * The final paragraph of the commit message appears to be outdated
> since it still seems to be describing the approach taken by v1.

Revised.

> * Elsewhere, in the project, the spelling "email" is preferred over
> "E-Mail"; that's true even in the files your patch is touching.

Done.

> * In the documentation:s/mutiply/multiple/.

Done.

> * The documentation doesn't seem to mention the default value of the
> new config variable.

Done.

> * 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.

I wasn't sure about that, was waiting for input from Jeff. Kept it as
it is.

> * Don't initialize static variables to 0 (let the .bss section do that
> automatically).

Done.

> * One space before && operator; not two.

Done.

> * Drop unnecessary braces.

Done.

> * Perhaps use test_config(), test_unconfig(), test_config_global() in
> the test script rather than the manual git-config invocations in
> subshells.

Done, although test_unconfig_global was missing, so I revised.

> * 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
>     '

Done. Also refactored both aspects of the test to a function.

> * Do these new tests really deserve a new test script, or would they
> fit into an existing script? (Genuine question.)

I am not sure. IMHO it makes sense to have a new test script for a new
feature.

> 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.

All changes from v2 to v3 listed above.

http://article.gmane.org/gmane.comp.version-control.git/285340

-- >8 --
Subject: 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 email address,
targeting different email 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 email
address.

This commit provides the same functionality by adding a new
configuration variable.

Signed-off-by: Dan Aloni <alonid@gmail.com>
---
 Documentation/config.txt  |  9 +++++++++
 ident.c                   | 45 +++++++++++++++++++++++++++++++++++++++++++++
 t/t9904-per-repo-email.sh | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+)
 create mode 100755 t/t9904-per-repo-email.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 877cbc875ec3..d329ec9ac8d1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2791,6 +2791,15 @@ 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 multiple email 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.
+	Defaults to `false`.
+
 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..c950b5e3490f 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;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"
@@ -373,6 +374,21 @@ 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 +421,18 @@ 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_explicitly_given & IDENT_MAIL_GIVEN))
+			die("requested explicitly given ident in config, "
+			    "but user.email is not set, or GIT_AUTHOR_EMAIL "
+			    "is not set");
+	}
+
 	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
 			 getenv("GIT_AUTHOR_EMAIL"),
 			 getenv("GIT_AUTHOR_DATE"),
@@ -417,6 +445,18 @@ const char *git_committer_info(int flag)
 		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
 	if (getenv("GIT_COMMITTER_EMAIL"))
 		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+
+	if (ident_explicit) {
+		if (!(committer_ident_explicitly_given & IDENT_NAME_GIVEN))
+			die("requested explicitly given ident in config, "
+			    "but user.name is not set, or GIT_COMMITTER_NAME "
+			    "is not set");
+		if (!(committer_ident_explicitly_given & IDENT_MAIL_GIVEN))
+			die("requested explicitly given ident in config, "
+			    "but user.email is not set, or GIT_COMMITTER_EMAIL "
+			    "is not set");
+	}
+
 	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
 			 getenv("GIT_COMMITTER_EMAIL"),
 			 getenv("GIT_COMMITTER_DATE"),
@@ -444,6 +484,11 @@ int author_ident_sufficiently_given(void)
 
 int git_ident_config(const char *var, const char *value, void *data)
 {
+	if (!strcmp(var, "user.explicit")) {
+		ident_explicit = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "user.name")) {
 		if (!value)
 			return config_error_nonbool(var);
diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh
new file mode 100755
index 000000000000..f6f42288a10c
--- /dev/null
+++ b/t/t9904-per-repo-email.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+#
+# Copyright (c) 2016 Dan Aloni
+#
+
+test_description='per-repo forced setting of email address'
+
+. ./test-lib.sh
+
+prepare () {
+	echo "Initial" >foo &&
+	git add foo &&
+	unset GIT_AUTHOR_NAME &&
+	unset GIT_AUTHOR_EMAIL &&
+	test_unconfig --global user.name &&
+	test_unconfig --global user.email &&
+	test_unconfig user.name &&
+	test_unconfig user.email &&
+	test_config_global user.explicit true
+}
+
+test_expect_success 'fails commiting if clone email is not set' '
+        prepare &&
+
+	EDITOR=: VISUAL=: test_must_fail git commit -m msg
+
+'
+
+test_expect_success 'succeeds commiting if clone email is set' '
+        prepare &&
+
+	test_config user.name "test" &&
+	test_config user.email "test@ok.com" &&
+	EDITOR=: VISUAL=: git commit -m msg
+'
+
+test_done
-- 
2.5.0



-- 
Dan Aloni

Re: [PATCH v3] Add user.explicit boolean for when ident shouldn't be guessed

[Jeff King]

On Wed, Feb 03, 2016 at 09:22:27PM +0200, Dan Aloni wrote:

> > * 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.
> 
> I wasn't sure about that, was waiting for input from Jeff. Kept it as
> it is.

I can't think of an objective reason one is better than the other. I
agree with Eric that mine is more clear, but that is probably because I
am the one who thought of it. :)

Though see below...

> > * Perhaps use test_config(), test_unconfig(), test_config_global() in
> > the test script rather than the manual git-config invocations in
> > subshells.
> 
> Done, although test_unconfig_global was missing, so I revised.

As a side note, we recently taught test_config() to handle "-C" for
setting config in another directory. I wonder if test_config_global
should be implemented the same way.

Not a blocker for your patch, obviously.

> > * Do these new tests really deserve a new test script, or would they
> > fit into an existing script? (Genuine question.)
> 
> I am not sure. IMHO it makes sense to have a new test script for a new
> feature.

I hope we never have more than 9999 features, then. :)

On to the patch itself...

> @@ -373,6 +374,21 @@ 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");
> +	}
> +

I'm not sure why this block is effectively repeated here, in
git_author_info(), and in git_committer_info(). Don't the latter two
just call fmt_ident?

To be honest, I had expected something much simpler, like this (I
omitted the config parsing for brevity):

diff --git a/ident.c b/ident.c
index 9dd3ae3..e4ee0e5 100644
--- a/ident.c
+++ b/ident.c
@@ -153,6 +154,8 @@ static void copy_email(const struct passwd *pw, struct strbuf *email,
 const char *ident_default_name(void)
 {
 	if (!git_default_name.len) {
+		if (ident_explicit)
+			die("user.explicit is set, but no name given");
 		copy_gecos(xgetpwuid_self(&default_name_is_bogus), &git_default_name);
 		strbuf_trim(&git_default_name);
 	}
@@ -168,9 +171,12 @@ const char *ident_default_email(void)
 			strbuf_addstr(&git_default_email, email);
 			committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 			author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		} else
+		} else {
+			if (ident_explicit)
+				die("user.explicit is set, but no email given");
 			copy_email(xgetpwuid_self(&default_email_is_bogus),
 				   &git_default_email, &default_email_is_bogus);
+		}
 		strbuf_trim(&git_default_email);
 	}
 	return git_default_email.buf;


And then I think you can see why I thought of the name "guessIdent"; I
expected to literally intercept the guessing stage and die rather than
guess. But I'm not sure it's so simple, and nor is your patch.

When placing that second conditional, where should we die()? Above, I do
so only when we are going to look up the username and hostname to
generate the email. But the block right above it handles the $EMAIL
environment variable.

What should happen with:

  git config --global --unset user.email
  git config --global user.explicit true
  export EMAIL=me@example.com
  git commit -m foo

? We usually count an $EMAIL variable as explicit (at least for the
purposes of complaining for "lookups"), so this is OK in both my code
and yours (because you check the explicitly_given flags). But for
your use case, I'm not sure that holds. Your goal is to complain unless
there is per-repo config, and $EMAIL is not that.

So I think that would argue for something more like my patch above,
where the check is pushed down to ident_default_*, and we can tell the
difference between "this came from config" and everything else.

But I don't think that's quite right, either. The other complication is
that sometimes we want a "strict" ident (for making commits, etc) and
sometimes we do not care as much (e.g., when writing a reflog entry).
And I think we cannot have this kick in for a non-strict context, as
otherwise you cannot clone. The newly formed clone does not yet have
your user.name and user.email variables, yet it wants to write a reflog
entry for the cloned branch.

In a sense, that encourages a nice workflow for your intended feature.
You have to do:

  git clone -c user.name=... -c user.email=... clone ...

to set up your ident in the newly-cloned repository, or else clone will
yell at you. But it's a little unfriendly. If you are just cloning to
view and not make commits, you don't need your ident set up. And worse,
if you forget to add your "-c" ident, clone will go through the trouble
to copy all of the objects, and only then complain about your ident.

So I'd argue that this should only kick in for the strict case. Which
means the check _has_ to go into fmt_ident, and we have to somehow
inform fmt_ident of the four cases:

  1. this ident came from git config

  2. this ident came from the environment, but from explicit variables

  3. this ident came from the environment; we guessed but the results
     look pretty good

  4. this ident came from the environment; it's probably bogus

Right now we can identify type 4, with the *_is_bogus flags. We can
identify 3, because EXPLICITLY_GIVEN flags won't be set. But we can't
tell the difference between types 1 and 2.

I suppose there is also a type 0: "this ident was from GIT_COMMITTER_*
and we did not bother to look up ident at all". But I think we agree
that strictness and explicitness don't even come into play there.

So I think we can make this work by adding another variable to
communicate the difference between types 1 and 2, and then act on it in
fmt_ident only when "strict" is set. And I think "user.explicit" is not
quite the right config name there. "user.guessIdent" is perhaps closer,
but what we are really saying is "the ident must come from git config".
So maybe "user.useConfigOnly" or something?

I also wonder if we could simply expose the 4 levels of above in a
variable, and default it to type-3. That would let people loosen or
tighten as they see fit. But it would be a more complicated patch, so if
nobody really cares about it beyond this use case, it may be overkill.

-Peff

Re: [PATCH v3] Add user.explicit boolean for when ident shouldn't be guessed

[Jeff King]

On Wed, Feb 03, 2016 at 11:01:11PM -0500, Jeff King wrote:

> > +	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");
> > +	}
> > +

By the way, while reading fmt_ident, I found it to be pretty convoluted,
and the comparisons to the global strbuf are a bit gross. Maybe this
should be a good preparatory cleanup?

-- >8 --
Subject: [PATCH] fmt_ident: refactor strictness checks

This function has evolved quite a bit over time, and as a
result, the logic for "is this an OK ident" has been
sprinkled throughout. This ends up with a lot of redundant
conditionals, like checking want_name repeatedly. Worse,
we want to know in many cases whether we are using the
"default" ident, and we do so by comparing directly to the
global strbuf, which violates the abstraction of the
ident_default_* functions.

Let's reorganize the function into a hierarchy of
conditionals to handle similar cases together. The only
case that doesn't just work naturally for this is that of an
empty name, where our advice is different based on whether
we came from ident_default_name() or not. We can use a
simple flag to cover this case.

Signed-off-by: Jeff King <peff@peff.net>
---
Another alternative for the last paragraph would be to break the
empty-name-handling out to its own function and call it from two places.

 ident.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/ident.c b/ident.c
index 3da5556..f3a431f 100644
--- a/ident.c
+++ b/ident.c
@@ -345,32 +345,34 @@ const char *fmt_ident(const char *name, const char *email,
 	int want_date = !(flag & IDENT_NO_DATE);
 	int want_name = !(flag & IDENT_NO_NAME);
 
-	if (want_name && !name)
-		name = ident_default_name();
-	if (!email)
-		email = ident_default_email();
-
-	if (want_name && !*name) {
-		struct passwd *pw;
-
-		if (strict) {
-			if (name == git_default_name.buf)
+	if (want_name) {
+		int using_default = 0;
+		if (!name) {
+			name = ident_default_name();
+			using_default = 1;
+			if (strict && default_name_is_bogus) {
 				fputs(env_hint, stderr);
-			die("empty ident name (for <%s>) not allowed", email);
+				die("unable to auto-detect name (got '%s')", name);
+			}
+		}
+		if (!*name) {
+			struct passwd *pw;
+			if (strict) {
+				if (using_default)
+					fputs(env_hint, stderr);
+				die("empty ident name (for <%s>) not allowed", email);
+			}
+			pw = xgetpwuid_self(NULL);
+			name = pw->pw_name;
 		}
-		pw = xgetpwuid_self(NULL);
-		name = pw->pw_name;
-	}
-
-	if (want_name && strict &&
-	    name == git_default_name.buf && default_name_is_bogus) {
-		fputs(env_hint, stderr);
-		die("unable to auto-detect name (got '%s')", name);
 	}
 
-	if (strict && email == git_default_email.buf && default_email_is_bogus) {
-		fputs(env_hint, stderr);
-		die("unable to auto-detect email address (got '%s')", email);
+	if (!email) {
+		email = ident_default_email();
+		if (strict && default_email_is_bogus) {
+			fputs(env_hint, stderr);
+			die("unable to auto-detect email address (got '%s')", email);
+		}
 	}
 
 	strbuf_reset(&ident);
-- 
2.7.0.513.g5aae4e5

Re: [PATCH v3] Add user.explicit boolean for when ident shouldn't be guessed

[Jeff King]

On Wed, Feb 03, 2016 at 11:19:09PM -0500, Jeff King wrote:

> On Wed, Feb 03, 2016 at 11:01:11PM -0500, Jeff King wrote:
> 
> > > +	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");
> > > +	}
> > > +
> 
> By the way, while reading fmt_ident, I found it to be pretty convoluted,
> and the comparisons to the global strbuf are a bit gross. Maybe this
> should be a good preparatory cleanup?

And then your feature on top would look something like this (taking into
account the points from my previous mail, but leaving the option name
and exact wording of messages up for grabs):

---
diff --git a/ident.c b/ident.c
index f3a431f..d3dc4a9 100644
--- a/ident.c
+++ b/ident.c
@@ -13,11 +13,14 @@ static struct strbuf git_default_date = STRBUF_INIT;
 static int default_email_is_bogus;
 static int default_name_is_bogus;
 
+static int ident_explicit;
+
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
 #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_config_given;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"
@@ -354,6 +357,9 @@ const char *fmt_ident(const char *name, const char *email,
 				fputs(env_hint, stderr);
 				die("unable to auto-detect name (got '%s')", name);
 			}
+			if (strict && ident_explicit &&
+			    !(ident_config_given & IDENT_NAME_GIVEN))
+				die("user.explicit set but no name given");
 		}
 		if (!*name) {
 			struct passwd *pw;
@@ -373,6 +379,8 @@ const char *fmt_ident(const char *name, const char *email,
 			fputs(env_hint, stderr);
 			die("unable to auto-detect email address (got '%s')", email);
 		}
+		if (strict && ident_explicit && !(ident_config_given & IDENT_MAIL_GIVEN))
+			die("user.explicit set but no mail given");
 	}
 
 	strbuf_reset(&ident);
@@ -446,6 +454,11 @@ int author_ident_sufficiently_given(void)
 
 int git_ident_config(const char *var, const char *value, void *data)
 {
+	if (!strcmp(var, "user.explicit")) {
+		ident_explicit = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "user.name")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -453,6 +466,7 @@ int git_ident_config(const char *var, const char *value, void *data)
 		strbuf_addstr(&git_default_name, value);
 		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		ident_config_given |= IDENT_NAME_GIVEN;
 		return 0;
 	}
 
@@ -463,6 +477,7 @@ int git_ident_config(const char *var, const char *value, void *data)
 		strbuf_addstr(&git_default_email, value);
 		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		ident_config_given |= IDENT_MAIL_GIVEN;
 		return 0;
 	}
 

And the tests should probably cover a few more cases, including cloning
with the option on, and seeing the impact of $EMAIL (both of which
should work with this, I think).

-Peff

Re: [PATCH v3] Add user.explicit boolean for when ident shouldn't be guessed

[Dan Aloni]

On Wed, Feb 03, 2016 at 11:01:11PM -0500, Jeff King wrote:
>[..]
> > > * Do these new tests really deserve a new test script, or would they
> > > fit into an existing script? (Genuine question.)
> > 
> > I am not sure. IMHO it makes sense to have a new test script for a new
> > feature.
> 
> I hope we never have more than 9999 features, then. :)
> 
> On to the patch itself...
> 
> > @@ -373,6 +374,21 @@ 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");
> > +	}
> > +
> 
> I'm not sure why this block is effectively repeated here, in
> git_author_info(), and in git_committer_info(). Don't the latter two
> just call fmt_ident?

Suspected that. Originally I started changing git_committer_info() and
git_author_info(), and only changed fmt_ident() later when I saw it
covers more cases. But the flow was confusing enough so I wasn't sure
whether to keep it.

> To be honest, I had expected something much simpler, like this (I
> omitted the config parsing for brevity):
>[..]
> In a sense, that encourages a nice workflow for your intended feature.
> You have to do:
> 
>   git clone -c user.name=... -c user.email=... clone ...
> 
> to set up your ident in the newly-cloned repository, or else clone will
> yell at you. But it's a little unfriendly. If you are just cloning to
> view and not make commits, you don't need your ident set up. And worse,
> if you forget to add your "-c" ident, clone will go through the trouble
> to copy all of the objects, and only then complain about your ident.

I think that forcing to give the configuration in 'git clone' could be
problematic for automated tools (e.g. o build) that invoke 'git clone'
just for building purposes (i.e. read-only) to a tool-managed directory.
And what about sub-modules clones? It would be hard to distinguish manual
clones and automatic clones anyway.

> So I'd argue that this should only kick in for the strict case. Which
> means the check _has_ to go into fmt_ident, and we have to somehow
> inform fmt_ident of the four cases:
> 
>   1. this ident came from git config
> 
>   2. this ident came from the environment, but from explicit variables
> 
>   3. this ident came from the environment; we guessed but the results
>      look pretty good
> 
>   4. this ident came from the environment; it's probably bogus
> 
> Right now we can identify type 4, with the *_is_bogus flags. We can
> identify 3, because EXPLICITLY_GIVEN flags won't be set. But we can't
> tell the difference between types 1 and 2.
> 
> I suppose there is also a type 0: "this ident was from GIT_COMMITTER_*
> and we did not bother to look up ident at all". But I think we agree
> that strictness and explicitness don't even come into play there.

Looks like an enum type would be better here instead of a set of booleans.

> So I think we can make this work by adding another variable to
> communicate the difference between types 1 and 2, and then act on it in
> fmt_ident only when "strict" is set. And I think "user.explicit" is not
> quite the right config name there. "user.guessIdent" is perhaps closer,
> but what we are really saying is "the ident must come from git config".
> So maybe "user.useConfigOnly" or something?

Yes, seems to me that useConfigOnly is better than both so far.

> I also wonder if we could simply expose the 4 levels of above in a
> variable, and default it to type-3. That would let people loosen or
> tighten as they see fit. But it would be a more complicated patch, so if
> nobody really cares about it beyond this use case, it may be overkill.

I get the impression from this and your later E-Mails that there are
much more cases to cover when testing this feature (and I would not
like to break stuff implementing this, obviously).

The code should be cleaned up anyway. I only delved into that code for
the first time two days ago, so it would take me more time to come up
with a new one (though reading your overview here of the cases is going
to be helpful, thanks).

-- 
Dan Aloni

Re: [PATCH v3] Add user.explicit boolean for when ident shouldn't be guessed

[Jeff King]

On Thu, Feb 04, 2016 at 07:36:46AM +0200, Dan Aloni wrote:

> > In a sense, that encourages a nice workflow for your intended feature.
> > You have to do:
> > 
> >   git clone -c user.name=... -c user.email=... clone ...
> > 
> > to set up your ident in the newly-cloned repository, or else clone will
> > yell at you. But it's a little unfriendly. If you are just cloning to
> > view and not make commits, you don't need your ident set up. And worse,
> > if you forget to add your "-c" ident, clone will go through the trouble
> > to copy all of the objects, and only then complain about your ident.
> 
> I think that forcing to give the configuration in 'git clone' could be
> problematic for automated tools (e.g. o build) that invoke 'git clone'
> just for building purposes (i.e. read-only) to a tool-managed directory.
> And what about sub-modules clones? It would be hard to distinguish manual
> clones and automatic clones anyway.

Yeah. I sort of assumed that people with automated tools _wouldn't_ set
user.explicit. But even with that assumption, I think it's too
unfriendly to continue.

> > So I'd argue that this should only kick in for the strict case. Which
> > means the check _has_ to go into fmt_ident, and we have to somehow
> > inform fmt_ident of the four cases:
> [...]
> 
> Looks like an enum type would be better here instead of a set of booleans.

Yeah, if we can actually split the state space into the 4 cases, I agree
an enum would be easier to follow. I'm not 100% sure that my cases map
completely to what the code does now, though.

> > I also wonder if we could simply expose the 4 levels of above in a
> > variable, and default it to type-3. That would let people loosen or
> > tighten as they see fit. But it would be a more complicated patch, so if
> > nobody really cares about it beyond this use case, it may be overkill.
> 
> I get the impression from this and your later E-Mails that there are
> much more cases to cover when testing this feature (and I would not
> like to break stuff implementing this, obviously).
> 
> The code should be cleaned up anyway. I only delved into that code for
> the first time two days ago, so it would take me more time to come up
> with a new one (though reading your overview here of the cases is going
> to be helpful, thanks).

Feel free to look into this direction, but having pushed a little
further towards the "simple" approach (with the 2 patches I just sent),
I think that does what you want without too much complication. I'd be
fine, too, if you wanted to pick those up[1] and put the finishing
touches on the second one.

-Peff

[1] To clarify, since you are new to the git.git workflow: I'd expect
    you to use `git am` to pick up my two patches. Leave me as the
    author of the first cleanup patch. Squash your additions onto the
    second one using `cherry-pick`, `commit --amend`, or whatever, and
    make sure to `commit --reset-author` so that you're the author. Post
    both as part of the v4 re-roll.

    But that's just "here is what I meant", not "what you have to do". :)

Re: [PATCH v3] Add user.explicit boolean for when ident shouldn't be guessed

[Dan Aloni]

On Thu, Feb 04, 2016 at 12:50:35AM -0500, Jeff King wrote:
> On Thu, Feb 04, 2016 at 07:36:46AM +0200, Dan Aloni wrote:
>[..]
> > The code should be cleaned up anyway. I only delved into that code for
> > the first time two days ago, so it would take me more time to come up
> > with a new one (though reading your overview here of the cases is going
> > to be helpful, thanks).
> 
> Feel free to look into this direction, but having pushed a little
> further towards the "simple" approach (with the 2 patches I just sent),
> I think that does what you want without too much complication. I'd be
> fine, too, if you wanted to pick those up[1] and put the finishing
> touches on the second one.
> 
> -Peff
> 
> [1] To clarify, since you are new to the git.git workflow: I'd expect
>     you to use `git am` to pick up my two patches. Leave me as the
>     author of the first cleanup patch. Squash your additions onto the
>     second one using `cherry-pick`, `commit --amend`, or whatever, and
>     make sure to `commit --reset-author` so that you're the author. Post
>     both as part of the v4 re-roll.
> 
>     But that's just "here is what I meant", not "what you have to do". :)

Thanks. Being familiar with Linux kernel patch submission process, good to
be focused about git.git's idiosyncrasies too.

The cleanup was easy thanks to your patches. Going to post v4.

-- 
Dan Aloni