[PATCH v7] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

[PATCH v7] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

[Dan Aloni]

Changes between v6 -> v7:

* Dropped patch: ident: cleanup wrt ident's source
* Revised the documentation of the feature according to comments.
* Revised the test according to comments.
* Styling fix.

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

[PATCH v7 1/2] fmt_ident: refactor strictness checks

[Dan Aloni]

From: Jeff King <peff@peff.net>

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>
---
 ident.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/ident.c b/ident.c
index 3da555634290..f3a431f738cc 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.5.0

[PATCH v7 2/2] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

[Dan Aloni]

It used to be that:

   git config --global user.email "(none)"

was a viable way for people to force themselves to set user.email in
each repository.  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.

A recent change, 19ce497c (ident: keep a flag for bogus
default_email, 2015-12-10), however declared that an explicitly
configured user.email is not bogus, no matter what its value is, so
this hack no longer works.

Provide the same functionality by adding a new configuration
variable user.useConfigOnly; when this variable is set, the
user must explicitly set user.email configuration.

Signed-off-by: Dan Aloni <alonid@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Cc: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/config.txt  | 10 +++++++++
 ident.c                   | 16 ++++++++++++++
 t/t9904-per-repo-email.sh | 55 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+)
 create mode 100755 t/t9904-per-repo-email.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 02bcde6bb596..0d168d92fd79 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2821,6 +2821,16 @@ user.name::
 	Can be overridden by the 'GIT_AUTHOR_NAME' and 'GIT_COMMITTER_NAME'
 	environment variables.  See linkgit:git-commit-tree[1].
 
+user.useConfigOnly::
+	This instructs Git to avoid trying to guess defaults for 'user.email'
+	and 'user.name' other than strictly from config. For example, if
+	you have multiple email addresses and would like to use a different
+	one for each repository, then with this configuration option set
+	to `true` in the global config along with a name, Git would prompt
+	for you for setting up an email upon making new commits in a newly
+	cloned repository.
+	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 f3a431f738cc..6e125821f056 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_use_config_only;
+
 #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_use_config_only
+			    && !(ident_config_given & IDENT_NAME_GIVEN))
+				die("user.useConfigOnly set but no name given");
 		}
 		if (!*name) {
 			struct passwd *pw;
@@ -373,6 +379,9 @@ 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_use_config_only
+		    && !(ident_config_given & IDENT_MAIL_GIVEN))
+			die("user.useConfigOnly set but no mail given");
 	}
 
 	strbuf_reset(&ident);
@@ -446,6 +455,11 @@ int author_ident_sufficiently_given(void)
 
 int git_ident_config(const char *var, const char *value, void *data)
 {
+	if (!strcmp(var, "user.useconfigonly")) {
+		ident_use_config_only = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "user.name")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -453,6 +467,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 +478,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;
 	}
 
diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh
new file mode 100755
index 000000000000..f2b33881e46b
--- /dev/null
+++ b/t/t9904-per-repo-email.sh
@@ -0,0 +1,55 @@
+#!/bin/sh
+#
+# Copyright (c) 2016 Dan Aloni
+#
+
+test_description='per-repo forced setting of email address'
+
+. ./test-lib.sh
+
+reprepare () {
+	git reset --hard initial
+}
+
+test_expect_success setup '
+	# Initial repo state
+	echo "Initial" >foo &&
+	git add foo &&
+	git commit -m foo &&
+	git tag initial &&
+
+	# Setup a likely user.useConfigOnly use case
+	sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
+	sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
+	git config user.name "test" &&
+	git config --global user.useConfigOnly true &&
+
+	reprepare
+'
+
+test_expect_success 'fails committing if clone email is not set' '
+	test_when_finished reprepare &&
+
+	test_must_fail git commit --allow-empty -m msg
+'
+
+test_expect_success 'fails committing if clone email is not set, but EMAIL set' '
+	test_when_finished reprepare &&
+
+	test_must_fail env EMAIL=test@fail.com git commit --allow-empty -m msg
+'
+
+test_expect_success 'succeeds committing if clone email is set' '
+	test_when_finished reprepare &&
+
+	test_config user.email "test@ok.com" &&
+	git commit --allow-empty -m msg
+'
+
+test_expect_success 'succeeds cloning if global email is not set' '
+	test_when_finished reprepare &&
+
+	git clone . clone
+'
+
+test_done
-- 
2.5.0

Re: [PATCH v7 2/2] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

[Jeff King]

On Fri, Feb 05, 2016 at 11:29:06PM +0200, Dan Aloni wrote:

> diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh
> new file mode 100755
> index 000000000000..f2b33881e46b
> --- /dev/null
> +++ b/t/t9904-per-repo-email.sh

Is t9904 the right place for this? Usually t99xx is for very separate
components.

This is sort-of about "commit", which would put it in the t75xx range.
But in some ways, it is even more fundamental than that. We don't seem
to have a lot of tests for ident stuff. The closest is the strict ident
stuff in t0007.

> +reprepare () {
> +	git reset --hard initial
> +}

Do we need this reprepare stuff at all now? The tests don't care which
commit we're at when they start.

> +test_expect_success setup '
> +	# Initial repo state
> +	echo "Initial" >foo &&
> +	git add foo &&
> +	git commit -m foo &&
> +	git tag initial &&

A shorter way of saying this is "test_commit foo".

I almost thought we could get rid of this part entirely; the commit
tests don't care. But we do still need _a_ commit for the clone test,
since we want to make sure a reflog is written. It would be nice to push
it down there, but our test environment doesn't allow creating commits,
because of of useConfigOnly. So it's probably fine to leave it here.

Technically, the final "commit" test does make a commit for us to push,
but we do generally try to avoid unnecessary dependencies between the
individual tests.

So all together, maybe:

diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh
index f2b3388..5694b84 100755
--- a/t/t9904-per-repo-email.sh
+++ b/t/t9904-per-repo-email.sh
@@ -7,48 +7,31 @@ test_description='per-repo forced setting of email address'
 
 . ./test-lib.sh
 
-reprepare () {
-	git reset --hard initial
-}
-
-test_expect_success setup '
-	# Initial repo state
-	echo "Initial" >foo &&
-	git add foo &&
-	git commit -m foo &&
-	git tag initial &&
-
-	# Setup a likely user.useConfigOnly use case
+test_expect_success 'setup a likely user.useConfigOnly use case' '
+	# we want to make sure a reflog is written, since that needs
+	# a non-strict ident. So be sure we have an actual commit.
+	test_commit foo &&
+
 	sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
 	sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
 	git config user.name "test" &&
-	git config --global user.useConfigOnly true &&
-
-	reprepare
+	git config --global user.useConfigOnly true
 '
 
 test_expect_success 'fails committing if clone email is not set' '
-	test_when_finished reprepare &&
-
 	test_must_fail git commit --allow-empty -m msg
 '
 
 test_expect_success 'fails committing if clone email is not set, but EMAIL set' '
-	test_when_finished reprepare &&
-
 	test_must_fail env EMAIL=test@fail.com git commit --allow-empty -m msg
 '
 
 test_expect_success 'succeeds committing if clone email is set' '
-	test_when_finished reprepare &&
-
 	test_config user.email "test@ok.com" &&
 	git commit --allow-empty -m msg
 '
 
 test_expect_success 'succeeds cloning if global email is not set' '
-	test_when_finished reprepare &&
-
 	git clone . clone
 '
 

Re: [PATCH v7 2/2] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

[Dan Aloni]

On Fri, Feb 05, 2016 at 04:48:33PM -0500, Jeff King wrote:
> On Fri, Feb 05, 2016 at 11:29:06PM +0200, Dan Aloni wrote:
> 
> > diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh
> > new file mode 100755
> > index 000000000000..f2b33881e46b
> > --- /dev/null
> > +++ b/t/t9904-per-repo-email.sh
> 
> Is t9904 the right place for this? Usually t99xx is for very separate
> components.
> 
> This is sort-of about "commit", which would put it in the t75xx range.
> But in some ways, it is even more fundamental than that. We don't seem
> to have a lot of tests for ident stuff. The closest is the strict ident
> stuff in t0007.

Will move to t7517. IMHO it's better to verify the commit operation
itself before running further tests that rely on its proper function.

>[..]
> > +reprepare () {
> > +	git reset --hard initial
> > +}
> 
> Do we need this reprepare stuff at all now? The tests don't care which
> commit we're at when they start.
> 
> > +test_expect_success setup '
> > +	# Initial repo state
> > +	echo "Initial" >foo &&
> > +	git add foo &&
> > +	git commit -m foo &&
> > +	git tag initial &&
> 
> A shorter way of saying this is "test_commit foo".
> 
> I almost thought we could get rid of this part entirely; the commit
> tests don't care. But we do still need _a_ commit for the clone test,
> since we want to make sure a reflog is written. It would be nice to push
> it down there, but our test environment doesn't allow creating commits,
> because of of useConfigOnly. So it's probably fine to leave it here.
> 
> Technically, the final "commit" test does make a commit for us to push,
> but we do generally try to avoid unnecessary dependencies between the
> individual tests.
> 
> So all together, maybe:
>[..]

Yes, shorted is better.

I'm squashing in these changes and adding you as Signed-off for v8.

-- 
Dan Aloni

Re: [PATCH v7 2/2] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This is sort-of about "commit", which would put it in the t75xx range.
> But in some ways, it is even more fundamental than that. We don't seem
> to have a lot of tests for ident stuff. The closest is the strict ident
> stuff in t0007.

Good point.

>> +reprepare () {
>> +	git reset --hard initial
>> +}
>
> Do we need this reprepare stuff at all now? The tests don't care which
> commit we're at when they start.

Ah, I thought the function was needed to make sure we have something
to commit, but I agree that you do not need it, if you are going to
use "commit --allow-empty" in the tests.

> So all together, maybe:

Sounds sensible.

Thanks.

Re: [PATCH v7 2/2] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

[Eric Sunshine]

On Fri, Feb 05, 2016 at 11:29:06PM +0200, Dan Aloni wrote:
> It used to be that:
> 
>    git config --global user.email "(none)"
> 
> was a viable way for people to force themselves to set user.email in
> each repository.  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

Either: s/commit/a commit/ or s/commit/commits/

> config was set in the per-repo config to the correct email address.
> 
> A recent change, 19ce497c (ident: keep a flag for bogus
> default_email, 2015-12-10), however declared that an explicitly

s/however/&,/

> configured user.email is not bogus, no matter what its value is, so
> this hack no longer works.
> 
> Provide the same functionality by adding a new configuration
> variable user.useConfigOnly; when this variable is set, the
> user must explicitly set user.email configuration.
> 
> Signed-off-by: Dan Aloni <alonid@gmail.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Cc: Eric Sunshine <sunshine@sunshineco.com>

You'd generally place your sign-off last.

> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -2821,6 +2821,16 @@ user.name::
> +user.useConfigOnly::
> +	This instructs Git to avoid trying to guess defaults for 'user.email'

Perhaps: s/This instructs/Instruct/

> +	and 'user.name' other than strictly from config. For example, if

The way this is phrased, it sounds almost as if Git also "guesses"
the value from config. Perhaps rephrase like this:

    Instruct Git to avoid trying to guess defaults for 'user.email'
    and 'user.name', and instead retrieve the values only from
    configuration.

> +	you have multiple email addresses and would like to use a different
> +	one for each repository, then with this configuration option set
> +	to `true` in the global config along with a name, Git would prompt

s/would/will/

> +	for you for setting up an email upon making new commits in a newly

s/for you for setting/you to set/

More below...

> +	cloned repository.
> +	Defaults to `false`.
> diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh
> @@ -0,0 +1,55 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2016 Dan Aloni
> +#
> +
> +test_description='per-repo forced setting of email address'
> +
> +. ./test-lib.sh
> +
> +reprepare () {
> +	git reset --hard initial
> +}
> +
> +test_expect_success setup '
> +	# Initial repo state
> +	echo "Initial" >foo &&
> +	git add foo &&
> +	git commit -m foo &&
> +	git tag initial &&
> +
> +	# Setup a likely user.useConfigOnly use case
> +	sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
> +	sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
> +	git config user.name "test" &&
> +	git config --global user.useConfigOnly true &&
> +
> +	reprepare
> +'
> +
> +test_expect_success 'fails committing if clone email is not set' '
> +	test_when_finished reprepare &&
> +
> +	test_must_fail git commit --allow-empty -m msg
> +'
> +
> +test_expect_success 'fails committing if clone email is not set, but EMAIL set' '
> +	test_when_finished reprepare &&
> +
> +	test_must_fail env EMAIL=test@fail.com git commit --allow-empty -m msg
> +'
> +
> +test_expect_success 'succeeds committing if clone email is set' '
> +	test_when_finished reprepare &&
> +
> +	test_config user.email "test@ok.com" &&
> +	git commit --allow-empty -m msg
> +'
> +
> +test_expect_success 'succeeds cloning if global email is not set' '
> +	test_when_finished reprepare &&
> +
> +	git clone . clone
> +'
> +
> +test_done

This test script still has a fair amount of unnecessary cruft in it
which obscures the important bits showing what you are really
testing. Below is a more concise version with the unnecessary stuff
removed:

--- 8< ---
#!/bin/sh
#
# Copyright (c) 2016 Dan Aloni
#

test_description='per-repo forced setting of email address'

. ./test-lib.sh

test_expect_success setup '
	sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
	sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
	git config user.name "test" &&
	git config --global user.useConfigOnly true
'

test_expect_success 'fails committing if clone email is not set' '
	test_must_fail git commit --allow-empty -m msg
'

test_expect_success 'fails committing if clone email is not set, but EMAIL set' '
	test_must_fail env EMAIL=test@fail.com git commit --allow-empty -m msg
'

test_expect_success 'succeeds committing if clone email is set' '
	git -c user.email=test@ok.com commit --allow-empty -m msg
'

test_expect_success 'succeeds cloning if global email is not set' '
	git clone . clone
'

test_done
--- 8< ---

Re: [PATCH v7 2/2] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

[Dan Aloni]

On Fri, Feb 05, 2016 at 04:59:52PM -0500, Eric Sunshine wrote:
> On Fri, Feb 05, 2016 at 11:29:06PM +0200, Dan Aloni wrote:
> > It used to be that:
> > 
> >    git config --global user.email "(none)"
> > 
> > was a viable way for people to force themselves to set user.email in
> > each repository.  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
> 
> Either: s/commit/a commit/ or s/commit/commits/

Thanks for all the proofing in your reply.

>[..]
> > config was set in the per-repo config to the correct email address.
> > 
> > A recent change, 19ce497c (ident: keep a flag for bogus
> > default_email, 2015-12-10), however declared that an explicitly
> 
> s/however/&,/
> 
> > configured user.email is not bogus, no matter what its value is, so
> > this hack no longer works.
> > 
> > Provide the same functionality by adding a new configuration
> > variable user.useConfigOnly; when this variable is set, the
> > user must explicitly set user.email configuration.
> > 
> > Signed-off-by: Dan Aloni <alonid@gmail.com>
> > Helped-by: Jeff King <peff@peff.net>
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > Cc: Eric Sunshine <sunshine@sunshineco.com>
> 
> You'd generally place your sign-off last.
> [..]

Good to know :)

> This test script still has a fair amount of unnecessary cruft in it
> which obscures the important bits showing what you are really
> testing. Below is a more concise version with the unnecessary stuff
> removed:

Thanks, though I'll stick to what Jeff suggested. Also, perhaps better
to keep 'test_config' as it is instead of using '-c', to better mimick
the tested use case.

-- 
Dan Aloni