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

From: Dan Aloni <alonid@gmail.com>
Date: Feb 5, 2016, 06:47 UTC
Message-ID: <20160205064754.GB15392@gmail.com>
In-Reply-To: <xmqq8u30gn1m.fsf@gitster.mtv.corp.google.com>

On Thu, Feb 04, 2016 at 01:53:25PM -0800, Junio C Hamano wrote:

Show 10 quoted lines

>[..]
> "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 --
>[..]
>

Looks good, I'll just take that :)

Show 9 quoted lines

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

Consistency is important. Will fix here too, though it got fixed later in the cleanup.

Show 13 quoted lines

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

Being new to this testing framework, I tried learning the trade from other tests. Maybe I goofed, or the other tests need cleaning?

Show 7 quoted lines

> 
> > +	# 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)?

Sure.

Show 11 quoted lines

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

Yes, and the fixed test still passes. Will resubmit the patches.

-- 
Dan Aloni