Re: [PATCH v2 06/12] commit: add tests of commit races
On 02/12/2015 08:36 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh >> new file mode 100755 >> index 000..08e6a6c >> --- /dev/null >> +++ b/t/t7516-commit-races.sh >> @@ -0,0 +1,33 @@ >> +#!/bin/sh >> + >> +test_description='git commit races' >> +. ./test-lib.sh >> + >> +test_tick >> + >> +test_expect_success 'set up editor' ' >> +write_script editor <<-\EOF >> +git commit --allow-empty -m hare >> +echo tortoise >"$1" >> +EOF >> +' >> + >> +test_expect_failure 'race to create orphan commit' ' >> +test_must_fail env EDITOR=./editor git commit --allow-empty && >> +git show -s --pretty=format:%s >subject && >> +grep -q hare subject && > > Why "grep -q" in the test? Normal invocation of the tests will hide > the output anyway, no? > > Wouldn't letting "sh t-name-of-test.sh -v" show the output > better for those who are hunting for breakages to see at which step > of the &&-chain things break? Good point. I will remove the "-q" from the two grep invocations in this file. >> +test -z "$(git show -s --pretty=format:%P)" >> +' > >> +test_expect_success 'race to create non-orphan commit' ' >> +git checkout --orphan branch && >> +git commit --allow-empty -m base && >> +git rev-parse HEAD >base && >> +test_must_fail env EDITOR=./editor git commit --allow-empty && >> +git show -s --pretty=format:%s >subject && >> +grep -q hare subject && > > Can we use a token different from hare and tortoise here? If the > previous one worked correctly, the main "commit" process would have > failed to add 'tortoise' on top of 'hare' that raced from sideways > (which is simulated by making 'hare' from the editor), so the tip of > the history would be 'hare' when this test starts. Expecting 'hare' > here makes it unclear if you are expecting _both_ of the competing > processes to fail (i.e. the main 'commit' fails to add 'tortoise' > and the racing 'commit' fails to do 'hare'), leaving the 'hare' the > previous test left at the tip of the history, or if you are expecting > that the competing one that tries to create the second 'hare' on top > of the existing 'hare' to win. Yes, you're right. I will change the second test to use different tokens. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 v2 06/12] commit: add tests of commit races
On 02/12/2015 07:13 PM, Stefan Beller wrote: > On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty > wrote: >> Committing involves the following steps: >> [...] >> diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh >> new file mode 100755 >> index 000..08e6a6c >> --- /dev/null >> +++ b/t/t7516-commit-races.sh >> @@ -0,0 +1,33 @@ >> +#!/bin/sh >> + >> +test_description='git commit races' >> +. ./test-lib.sh >> + >> +test_tick > > So I am wondering why we need to have a test_tick here. > In case we need to pass simulation time after loading the > test-lib, we should rather have it inside the test-lib. You're right; I don't think it's necessary. I will remove it. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 v2 06/12] commit: add tests of commit races
Michael Haggerty writes: > diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh > new file mode 100755 > index 000..08e6a6c > --- /dev/null > +++ b/t/t7516-commit-races.sh > @@ -0,0 +1,33 @@ > +#!/bin/sh > + > +test_description='git commit races' > +. ./test-lib.sh > + > +test_tick > + > +test_expect_success 'set up editor' ' > + write_script editor <<-\EOF > + git commit --allow-empty -m hare > + echo tortoise >"$1" > + EOF > +' > + > +test_expect_failure 'race to create orphan commit' ' > + test_must_fail env EDITOR=./editor git commit --allow-empty && > + git show -s --pretty=format:%s >subject && > + grep -q hare subject && Why "grep -q" in the test? Normal invocation of the tests will hide the output anyway, no? Wouldn't letting "sh t-name-of-test.sh -v" show the output better for those who are hunting for breakages to see at which step of the &&-chain things break? > + test -z "$(git show -s --pretty=format:%P)" > +' > +test_expect_success 'race to create non-orphan commit' ' > + git checkout --orphan branch && > + git commit --allow-empty -m base && > + git rev-parse HEAD >base && > + test_must_fail env EDITOR=./editor git commit --allow-empty && > + git show -s --pretty=format:%s >subject && > + grep -q hare subject && Can we use a token different from hare and tortoise here? If the previous one worked correctly, the main "commit" process would have failed to add 'tortoise' on top of 'hare' that raced from sideways (which is simulated by making 'hare' from the editor), so the tip of the history would be 'hare' when this test starts. Expecting 'hare' here makes it unclear if you are expecting _both_ of the competing processes to fail (i.e. the main 'commit' fails to add 'tortoise' and the racing 'commit' fails to do 'hare'), leaving the 'hare' the previous test left at the tip of the history, or if you are expecting that the competing one that tries to create the second 'hare' on top of the existing 'hare' to win. > + git rev-parse HEAD^ >parent && > + test_cmp base parent > +' > + > +test_done -- 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 v2 06/12] commit: add tests of commit races
On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty wrote: > Committing involves the following steps: > > 1. Determine the current value of HEAD (if any). > 2. Create the new commit object. > 3. Update HEAD. > > Please note that step 2 can take arbitrarily long, because it might > involve the user editing a commit message. > > If a second process sneaks in a commit during step 2, then the first > commit process should fail. This is usually done correctly, because > step 3 verifies that HEAD still points at the same commit that it > pointed to during step 1. > > However, if there is a race when creating an *orphan* commit, then the > test in step 3 is skipped. > > Add tests for proper handling of such races. One of the new tests > fails. It will be fixed in a moment. > > Signed-off-by: Michael Haggerty > --- > t/t7516-commit-races.sh | 33 + > 1 file changed, 33 insertions(+) > create mode 100755 t/t7516-commit-races.sh > > diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh > new file mode 100755 > index 000..08e6a6c > --- /dev/null > +++ b/t/t7516-commit-races.sh > @@ -0,0 +1,33 @@ > +#!/bin/sh > + > +test_description='git commit races' > +. ./test-lib.sh > + > +test_tick So I am wondering why we need to have a test_tick here. In case we need to pass simulation time after loading the test-lib, we should rather have it inside the test-lib. If we need to pass time specifically for this test (I just don't understand why at this point of execution), then we should move it more to the relevant place inside the &&-chain as all other tests have test_tick inside a test chained up with &&. > + > +test_expect_success 'set up editor' ' > + write_script editor <<-\EOF > + git commit --allow-empty -m hare > + echo tortoise >"$1" > + EOF > +' > + > +test_expect_failure 'race to create orphan commit' ' > + test_must_fail env EDITOR=./editor git commit --allow-empty && > + git show -s --pretty=format:%s >subject && > + grep -q hare subject && > + test -z "$(git show -s --pretty=format:%P)" > +' > + > +test_expect_success 'race to create non-orphan commit' ' > + git checkout --orphan branch && > + git commit --allow-empty -m base && > + git rev-parse HEAD >base && > + test_must_fail env EDITOR=./editor git commit --allow-empty && > + git show -s --pretty=format:%s >subject && > + grep -q hare subject && > + git rev-parse HEAD^ >parent && > + test_cmp base parent > +' > + > +test_done > -- > 2.1.4 > -- 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
[PATCH v2 06/12] commit: add tests of commit races
Committing involves the following steps: 1. Determine the current value of HEAD (if any). 2. Create the new commit object. 3. Update HEAD. Please note that step 2 can take arbitrarily long, because it might involve the user editing a commit message. If a second process sneaks in a commit during step 2, then the first commit process should fail. This is usually done correctly, because step 3 verifies that HEAD still points at the same commit that it pointed to during step 1. However, if there is a race when creating an *orphan* commit, then the test in step 3 is skipped. Add tests for proper handling of such races. One of the new tests fails. It will be fixed in a moment. Signed-off-by: Michael Haggerty --- t/t7516-commit-races.sh | 33 + 1 file changed, 33 insertions(+) create mode 100755 t/t7516-commit-races.sh diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh new file mode 100755 index 000..08e6a6c --- /dev/null +++ b/t/t7516-commit-races.sh @@ -0,0 +1,33 @@ +#!/bin/sh + +test_description='git commit races' +. ./test-lib.sh + +test_tick + +test_expect_success 'set up editor' ' + write_script editor <<-\EOF + git commit --allow-empty -m hare + echo tortoise >"$1" + EOF +' + +test_expect_failure 'race to create orphan commit' ' + test_must_fail env EDITOR=./editor git commit --allow-empty && + git show -s --pretty=format:%s >subject && + grep -q hare subject && + test -z "$(git show -s --pretty=format:%P)" +' + +test_expect_success 'race to create non-orphan commit' ' + git checkout --orphan branch && + git commit --allow-empty -m base && + git rev-parse HEAD >base && + test_must_fail env EDITOR=./editor git commit --allow-empty && + git show -s --pretty=format:%s >subject && + grep -q hare subject && + git rev-parse HEAD^ >parent && + test_cmp base parent +' + +test_done -- 2.1.4 -- 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
