Re: [PATCH v7 2/2] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed
Jeff King 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/2] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed
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 > > Helped-by: Jeff King > > Signed-off-by: Junio C Hamano > > Cc: Eric Sunshine > > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/2] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed
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 ..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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/2] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed
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
> Helped-by: Jeff King
> Signed-off-by: Junio C Hamano
> Cc: Eric Sunshine
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=t...@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 "t...@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=t...@fail.com git commit --allow-empty -m msg
'
test_expect_success 'succeeds committing if clone email is set' '
git -c user.email=t...@ok.com commit --allow-empty -m msg
'
test_expect_success 'succeeds cloning if global email is not set' '
git clone . clone
'
test_done
--- 8< ---
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/2] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed
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 ..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=t...@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 "t...@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
'
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
