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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2016, 21:53 UTC
Message-ID: <xmqq8u30gn1m.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1454577160-24484-3-git-send-email-alonid@gmail.com>

Dan Aloni <alonid@gmail.com> writes:

Show 12 quoted lines

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

Thanks. I'd rather cite an individual commit, not a merge of a topic, without forcing people to run "git show" only to see the title of the commit. Also I'd avoid "was it that really that many people did so?" discussion by not saying "many people" altogether.

"by adding a new configuration variable" is a bit weak. Help the reader by mentioning what it is called and what it does in the same sentence.

Perhaps like this?

-- >8 --
    ident: add user.useConfigOnly boolean for when ident shouldn't be guessed
    
    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>
-- 8< --

The logic in the patch is very cleanly written, partly thanks to the previous step that was a real clean-up.

Show 17 quoted lines

> @@ -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,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_use_config_only && !(ident_config_given & IDENT_MAIL_GIVEN))
> +			die("user.useConfigOnly set but no mail given");
>  	}

By folding the line just like you did for "name" above, you do not have to worry about an overlong line here.

Show 20 quoted lines

> diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh
> new file mode 100755
> index 000000000000..9522a640951b
> --- /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
> +	echo "Initial" >foo &&

By attempting to reply to a patch, one discovers that the patch has mixed indentation style ;-) Indent with tabs.

> +        git add foo &&
> +        EDITOR=: VISUAL=: git commit -m foo &&

What is the point of these one-shot assignments to the environment variables?

"git commit -m <msg>" does not invoke the editor unless given "-e", and EDITOR=: is done early in test-lib.sh already, so I am puzzled.

Besides, if you are worried about some stray environment variable, overriding EDITOR and VISUAL would not guard you against a stray GIT_EDITOR, which takes the precedence, I think.

> +	# Setup a likely user.useConfigOnly use case
> +	unset GIT_AUTHOR_NAME &&
> +	unset GIT_AUTHOR_EMAIL &&

Doesn't unset fail when the variable is not set (we have sane_unset helper for that)?

Show 16 quoted lines

> +	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 &&
> +
> +	EDITOR=: VISUAL=: test_must_fail git commit -m msg

test_must_fail, being a shell function, does not follow the usual "With 'VAR1=VAL2 VAR2=VAL2 thing', $VAR1 and $VAR2 get temporary values only during the execution of the thing". If you really need to, you would need to do this like so

	test_must_fail env EDITOR=: git commit -m msg

But again, I do not think you need to override EDITOR/VISUAL here, so...

> +test_expect_success 'fails committing if clone email is not set, but EMAIL set' '
> +        prepare && about_to_commit &&
> +
> +	EMAIL=test@fail.com EDITOR=: VISUAL=: test_must_fail git commit -m msg

This is a good place to use the "test_must_fail env" pattern, i.e.

	test_must_fail env EMAIL=test@fail.com git commit -m msg

I would think.

Thanks.