Re: [PATCH 3/4 v6] cache-tree: subdirectory tests
David Turner writes: > On Fri, 2014-07-11 at 08:40 -0700, Junio C Hamano wrote: > >> In the ideal world, I think whoever tries to compare two cache-trees >> (i.e. test-dump-cache-tree) should *not* care, because we are merely >> trying to show what the correct tree object name for the node would >> be, but this is only for testing, so the best way forward would be >> to: >> >> - Stop using DRY_RUN in test-dump-cache-tree.c; >> >> - Stop the code to support DRY_RUN from cache-tree.c (nobody but >>the test uses it); and >> >> - Drop the "-e '#(ref)/d'" from the above. >> >> I would think. > > Do you mean that I should do this in this patch set, or that it's a good > idea for the future? I have no strong preference either way. Removing DRY_RUN may simplify things in the code that gets used in the real life (as opposed to the code that is only used during the tests), so I do not mind it if it was done before the series as a preparation step. > Also, if we don't use DRY_RUN, won't test-dump-cache-tree add trees to > the actual ODB, which would be odd for a test program? I do not see it as odd at all; after all, nobody in the real-life uses dry-run and as you can see its use is broken, or at least is inconsistent with the rest of the system. -- 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 3/4 v6] cache-tree: subdirectory tests
On Fri, 2014-07-11 at 08:40 -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > >>> + sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p' > >>> >actual && > > > > Is the second one to remove "#(ref)", which appears for a good > > "reference" cache tree entry shown for comparison, necessary? Do > > they ever begin with "invalid"? If they ever begin with "invalid" > > that itself may even be a noteworthy breakage to catch, no? > > Answering to myself... > > Because test-dump-cache-tree uses DRY_RUN to create only an in-core > copy of tree object, and we notice that the reference cache-tree > created in the tests contains the object name of a tree that does > not yet exist in the object database. We get "invalid #(ref)" for > such node. > > In the ideal world, I think whoever tries to compare two cache-trees > (i.e. test-dump-cache-tree) should *not* care, because we are merely > trying to show what the correct tree object name for the node would > be, but this is only for testing, so the best way forward would be > to: > > - Stop using DRY_RUN in test-dump-cache-tree.c; > > - Stop the code to support DRY_RUN from cache-tree.c (nobody but >the test uses it); and > > - Drop the "-e '#(ref)/d'" from the above. > > I would think. Do you mean that I should do this in this patch set, or that it's a good idea for the future? Also, if we don't use DRY_RUN, won't test-dump-cache-tree add trees to the actual ODB, which would be odd for a test program? -- 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 3/4 v6] cache-tree: subdirectory tests
On Fri, 2014-07-11 at 08:27 -0700, Junio C Hamano wrote:
> Eric Sunshine writes:
>
> > On Thu, Jul 10, 2014 at 8:31 PM, David Turner
> > wrote:
> >> Add tests to confirm that invalidation of subdirectories neither over-
> >> nor under-invalidates.
> >>
> >> Signed-off-by: David Turner
> >> ---
> >> t/t0090-cache-tree.sh | 26 +++---
> >> 1 file changed, 23 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> >> index 98fb1ab..3a3342e 100755
> >> --- a/t/t0090-cache-tree.sh
> >> +++ b/t/t0090-cache-tree.sh
> >> @@ -22,9 +22,10 @@ test_shallow_cache_tree () {
> >> }
> >>
> >> test_invalid_cache_tree () {
> >> - echo "invalid (0 subtrees)"
> >> >expect &&
> >> - printf "SHA #(ref) (%d entries, 0 subtrees)\n" $(git ls-files|wc
> >> -l) >>expect &&
> >> - cmp_cache_tree expect
> >> + printf "invalid %s ()\n" "" "$@"
> >> >expect &&
>
> Hmm. This will always expect that the top-level is invalid, even
> when $# is 0. It is OK if you never need to use this to test that a
> cache-tree is fully valid, but is it something we want to check?
We have test_cache_tree to check that it's fully valid.
> Existing tests are mostly about "cache-tree is populated fully at a
> few strategic, well known and easy places and then it degrades over
> time", but after all your series is adding more places to that set
> of "a few places", so we may want to make sure that future breakages
> to the new code paths that "repair" the cache-tree are caught by
> these tests.
This patchset un-failed "initial commit has cache-tree", and added
"commit in child dir has cache-tree" and "partial commit gives
cache-tree". I've just added a test for interactive commit; when you
take a look at the next patchset, you can let me know if this seems
sufficient to you.
--
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 3/4 v6] cache-tree: subdirectory tests
Junio C Hamano writes: >>> + sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p' >>> >actual && > > Is the second one to remove "#(ref)", which appears for a good > "reference" cache tree entry shown for comparison, necessary? Do > they ever begin with "invalid"? If they ever begin with "invalid" > that itself may even be a noteworthy breakage to catch, no? Answering to myself... Because test-dump-cache-tree uses DRY_RUN to create only an in-core copy of tree object, and we notice that the reference cache-tree created in the tests contains the object name of a tree that does not yet exist in the object database. We get "invalid #(ref)" for such node. In the ideal world, I think whoever tries to compare two cache-trees (i.e. test-dump-cache-tree) should *not* care, because we are merely trying to show what the correct tree object name for the node would be, but this is only for testing, so the best way forward would be to: - Stop using DRY_RUN in test-dump-cache-tree.c; - Stop the code to support DRY_RUN from cache-tree.c (nobody but the test uses it); and - Drop the "-e '#(ref)/d'" from the above. I would think. -- 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 3/4 v6] cache-tree: subdirectory tests
Eric Sunshine writes:
> On Thu, Jul 10, 2014 at 8:31 PM, David Turner
> wrote:
>> Add tests to confirm that invalidation of subdirectories neither over-
>> nor under-invalidates.
>>
>> Signed-off-by: David Turner
>> ---
>> t/t0090-cache-tree.sh | 26 +++---
>> 1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
>> index 98fb1ab..3a3342e 100755
>> --- a/t/t0090-cache-tree.sh
>> +++ b/t/t0090-cache-tree.sh
>> @@ -22,9 +22,10 @@ test_shallow_cache_tree () {
>> }
>>
>> test_invalid_cache_tree () {
>> - echo "invalid (0 subtrees)"
>> >expect &&
>> - printf "SHA #(ref) (%d entries, 0 subtrees)\n" $(git ls-files|wc
>> -l) >>expect &&
>> - cmp_cache_tree expect
>> + printf "invalid %s ()\n" "" "$@"
>> >expect &&
Hmm. This will always expect that the top-level is invalid, even
when $# is 0. It is OK if you never need to use this to test that a
cache-tree is fully valid, but is it something we want to check?
Existing tests are mostly about "cache-tree is populated fully at a
few strategic, well known and easy places and then it degrades over
time", but after all your series is adding more places to that set
of "a few places", so we may want to make sure that future breakages
to the new code paths that "repair" the cache-tree are caught by
these tests.
>> + test-dump-cache-tree | \
>
> nit: unnecessary backslash
Good eyes ;-)
>> + sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p'
>> >actual &&
Is the second one to remove "#(ref)", which appears for a good
"reference" cache tree entry shown for comparison, necessary? Do
they ever begin with "invalid"? If they ever begin with "invalid"
that itself may even be a noteworthy breakage to catch, no?
>> + test_cmp expect actual
>> }
>>
>> test_no_cache_tree () {
>> @@ -49,6 +50,25 @@ test_expect_success 'git-add invalidates cache-tree' '
>> test_invalid_cache_tree
>> '
>>
>> +test_expect_success 'git-add in subdir invalidates cache-tree' '
>> + test_when_finished "git reset --hard; git read-tree HEAD" &&
>> + mkdir dirx &&
>> + echo "I changed this file" >dirx/foo &&
>> + git add dirx/foo &&
>> + test_invalid_cache_tree
>> +'
>> +
>> +test_expect_success 'git-add in subdir does not invalidate sibling
>> cache-tree' '
>> + git tag no-children &&
>> + test_when_finished "git reset --hard no-children; git read-tree
>> HEAD" &&
>> + mkdir dir1 dir2 &&
>> + test_commit dir1/a &&
>> + test_commit dir2/b &&
>> + echo "I changed this file" >dir1/a &&
>> + git add dir1/a &&
>> + test_invalid_cache_tree dir1/
>> +'
>> +
>> test_expect_success 'update-index invalidates cache-tree' '
>> test_when_finished "git reset --hard; git read-tree HEAD" &&
>> echo "I changed this file" >foo &&
>> --
>> 2.0.0.390.gcb682f8
--
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 3/4 v6] cache-tree: subdirectory tests
On Thu, Jul 10, 2014 at 8:31 PM, David Turner wrote:
> Add tests to confirm that invalidation of subdirectories neither over-
> nor under-invalidates.
>
> Signed-off-by: David Turner
> ---
> t/t0090-cache-tree.sh | 26 +++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 98fb1ab..3a3342e 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -22,9 +22,10 @@ test_shallow_cache_tree () {
> }
>
> test_invalid_cache_tree () {
> - echo "invalid (0 subtrees)" >expect
> &&
> - printf "SHA #(ref) (%d entries, 0 subtrees)\n" $(git ls-files|wc -l)
> >>expect &&
> - cmp_cache_tree expect
> + printf "invalid %s ()\n" "" "$@"
> >expect &&
> + test-dump-cache-tree | \
nit: unnecessary backslash
> + sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p'
> >actual &&
> + test_cmp expect actual
> }
>
> test_no_cache_tree () {
> @@ -49,6 +50,25 @@ test_expect_success 'git-add invalidates cache-tree' '
> test_invalid_cache_tree
> '
>
> +test_expect_success 'git-add in subdir invalidates cache-tree' '
> + test_when_finished "git reset --hard; git read-tree HEAD" &&
> + mkdir dirx &&
> + echo "I changed this file" >dirx/foo &&
> + git add dirx/foo &&
> + test_invalid_cache_tree
> +'
> +
> +test_expect_success 'git-add in subdir does not invalidate sibling
> cache-tree' '
> + git tag no-children &&
> + test_when_finished "git reset --hard no-children; git read-tree HEAD"
> &&
> + mkdir dir1 dir2 &&
> + test_commit dir1/a &&
> + test_commit dir2/b &&
> + echo "I changed this file" >dir1/a &&
> + git add dir1/a &&
> + test_invalid_cache_tree dir1/
> +'
> +
> test_expect_success 'update-index invalidates cache-tree' '
> test_when_finished "git reset --hard; git read-tree HEAD" &&
> echo "I changed this file" >foo &&
> --
> 2.0.0.390.gcb682f8
--
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 3/4 v6] cache-tree: subdirectory tests
Add tests to confirm that invalidation of subdirectories neither over-
nor under-invalidates.
Signed-off-by: David Turner
---
t/t0090-cache-tree.sh | 26 +++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 98fb1ab..3a3342e 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -22,9 +22,10 @@ test_shallow_cache_tree () {
}
test_invalid_cache_tree () {
- echo "invalid (0 subtrees)" >expect &&
- printf "SHA #(ref) (%d entries, 0 subtrees)\n" $(git ls-files|wc -l)
>>expect &&
- cmp_cache_tree expect
+ printf "invalid %s ()\n" "" "$@"
>expect &&
+ test-dump-cache-tree | \
+ sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p'
>actual &&
+ test_cmp expect actual
}
test_no_cache_tree () {
@@ -49,6 +50,25 @@ test_expect_success 'git-add invalidates cache-tree' '
test_invalid_cache_tree
'
+test_expect_success 'git-add in subdir invalidates cache-tree' '
+ test_when_finished "git reset --hard; git read-tree HEAD" &&
+ mkdir dirx &&
+ echo "I changed this file" >dirx/foo &&
+ git add dirx/foo &&
+ test_invalid_cache_tree
+'
+
+test_expect_success 'git-add in subdir does not invalidate sibling cache-tree'
'
+ git tag no-children &&
+ test_when_finished "git reset --hard no-children; git read-tree HEAD" &&
+ mkdir dir1 dir2 &&
+ test_commit dir1/a &&
+ test_commit dir2/b &&
+ echo "I changed this file" >dir1/a &&
+ git add dir1/a &&
+ test_invalid_cache_tree dir1/
+'
+
test_expect_success 'update-index invalidates cache-tree' '
test_when_finished "git reset --hard; git read-tree HEAD" &&
echo "I changed this file" >foo &&
--
2.0.0.390.gcb682f8
--
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
