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

From: Junio C Hamano <[email-masked]>
Date: Fri, 05 Feb 2016 11:31:34 -0800

Dan Aloni <[email-masked]> writes:

> +user.useConfigOnly::
> +	This instruct Git to avoid trying to guess defaults for 'user.email'
> +	and 'user.name' other than strictly from environment or config.

OK.

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

The first sentence mentioned both name and email, but here the
example is only about email.  A first time reader might be led into
thinking this is only about email and not name, but I am assuming
that is not the intention (i.e. this is merely showing just one use
case).

> +	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..9bd6ac69bfe8 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");

I can read the split expression either with && hanging at the end of
line or && leading the next line just fine, but you'd want to be
consistent especially when you are writing two almost identical
things.

> diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh
> new file mode 100755
> index 000000000000..0430f2e38434
> --- /dev/null
> +++ b/t/t9904-per-repo-email.sh
> @@ -0,0 +1,58 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2016 Dan Aloni
> +#
> +
> +test_description='per-repo forced setting of email address'
> +
> +. ./test-lib.sh
> +
> +prepare () {
> +	# Have a non-empty repository
> +	rm -fr .git
> +	git init

Hmm, do we do something drastic like this in any of our existing
tests?

When your test script starts by dot-sourcing test-lib.sh, you will
be given an initial repository with an empty history, so I'd expect
that you would be able to use that without calling "prepare" over
and over again.  The usual convention is to do this kind of setup
to establish a reasonable baseline in the first test titled 'setup'.
I think the part up to where you unset the environment variables (by
the way, don't you need to unset GIT_COMMITTER_* variables, too?)
belongs to the 'setup' that is done only once at the beginning of
this script.

Each of your tests will make changes to the state by setting or
unsetting configuration variables and possibly making commits, that
would affect the state of the repository that will be used by the
next and subsequent tests.  test_when_finished helper can register
the clean-up procedure to revert these possible state changes, and
you can further avoid code duplication by calling that same clean-up
procedure at the end of the setup test.

So if this followed the style of a typical existing test, we would
probably do something along these lines:

	reprepare () {
		git reset --hard initial &&
                echo Second >foo &&
                git add foo
	}

	test_expect_success setup '
		echo Initial >foo &&
                git add foo &&
                git commit -m foo &&
                git tag initial &&

		sane_unset GIT_AUTHOR_NAME GIT_COMMITTER_NAME ... &&
                git config --global user.name test &&
                git config --global user.useConfigOnly true &&
		reprepare
        '

	test_expect_success 'fail without email anywhere' '
        	test_when_finished reprepare &&
                test_must_fail git commit -m msg
	'

	test_expect_success 'suceed with config' '
        	test_when_finished reprepare &&
		test_config user.email [email-masked] &&
                test_must_fail git commit -m msg
	'

Note that you do not need "test_unconfig user.email" in reprepare,
as the variable is set in one test with test_config, which uses
test_when_finished to arrange the variable to be removed after
running the test.

> +	echo "Initial" >foo &&
> +	git add foo &&
> +	git commit -m foo &&
> +
> +	# Setup a likely user.useConfigOnly use case
> +	sane_unset GIT_AUTHOR_NAME &&
> +	sane_unset GIT_AUTHOR_EMAIL &&
> +	test_unconfig --global user.name &&
> +	test_unconfig --global user.email &&
> +	test_config user.name "test" &&
> +	test_unconfig user.email &&
> +	test_config_global user.useConfigOnly true
> +}
> +
> +about_to_commit () {
> +	echo "Second" >>foo &&
> +	git add foo
> +}

> +test_expect_success 'fails committing if clone email is not set' '
> +	prepare && about_to_commit &&
> +
> +	test_must_fail git commit -m msg
> +'

> +test_expect_success 'fails committing if clone email is not set, but EMAIL set' '
> +	prepare && about_to_commit &&
> +
> +	test_must_fail env EMAIL=[email-masked] git commit -m msg
> +'
> +
> +test_expect_success 'succeeds committing if clone email is set' '
> +	prepare && about_to_commit &&
> +
> +	test_config user.email "[email-masked]" &&
> +	git commit -m msg
> +'
> +
> +test_expect_success 'succeeds cloning if global email is not set' '
> +	prepare &&
> +
> +	git clone . clone
> +'
> +
> +test_done