[PATCH 2/3] test-dump-cache-tree: Improve output format and exit code

2014-07-01 Thread David Turner 
Make test-dump-cache-tree more useful for testing.  Do not treat known
invalid trees as errors (and do not produce non-zero exit code),
because we can fall back to the non-cache-tree codepath.

Signed-off-by: David Turner dtur...@twitter.com
---
 t/t0090-cache-tree.sh  | 28 +---
 test-dump-cache-tree.c | 24 
 2 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 98fb1ab..8437c5f 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -21,10 +21,13 @@ test_shallow_cache_tree () {
cmp_cache_tree expect
 }
 
+# Test that the cache-tree for a given directory is invalid.
+# If no directory is given, check that the root is invalid
 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
+   test-dump-cache-tree actual 
+   sed -e s/$_x40/SHA/ -e s/[0-9]* subtrees//g actual filtered 
+   expect=$(printf invalid  $1 ()\n) 
+   fgrep $expect filtered
 }
 
 test_no_cache_tree () {
@@ -49,6 +52,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 
diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index 47eab97..ad42ca1 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -6,12 +6,12 @@
 static void dump_one(struct cache_tree *it, const char *pfx, const char *x)
 {
if (it-entry_count  0)
-   printf(%-40s %s%s (%d subtrees)\n,
-  invalid, x, pfx, it-subtree_nr);
+   printf(%-40s %s (%d subtrees)%s\n,
+  invalid, pfx, it-subtree_nr, x);
else
-   printf(%s %s%s (%d entries, %d subtrees)\n,
-  sha1_to_hex(it-sha1), x, pfx,
-  it-entry_count, it-subtree_nr);
+   printf(%s %s (%d entries, %d subtrees)%s\n,
+  sha1_to_hex(it-sha1), pfx,
+  it-entry_count, it-subtree_nr, x);
 }
 
 static int dump_cache_tree(struct cache_tree *it,
@@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it,
/* missing in either */
return 0;
 
-   if (it-entry_count  0) {
+   if (it-entry_count  0)
+   /* invalid */
dump_one(it, pfx, );
-   dump_one(ref, pfx, #(ref) );
-   if (it-subtree_nr != ref-subtree_nr)
-   errs = 1;
-   }
else {
-   dump_one(it, pfx, );
if (hashcmp(it-sha1, ref-sha1) ||
ref-entry_count != it-entry_count ||
ref-subtree_nr != it-subtree_nr) {
-   dump_one(ref, pfx, #(ref) );
+   /* claims to be valid but is lying */
+   dump_one(ref, pfx,  #(error));
errs = 1;
+   } else {
+   /* is actually valid */
+   dump_one(it, pfx, );
}
}
 
-- 
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 2/3] test-dump-cache-tree: Improve output format and exit code

2014-07-01 Thread Junio C Hamano 
David Turner dtur...@twopensource.com writes:

 Make test-dump-cache-tree more useful for testing.  Do not treat known
 invalid trees as errors (and do not produce non-zero exit code),
 because we can fall back to the non-cache-tree codepath.

Under-explained.  more useful is subjective so I won't complain
about it being not explained enough, but I cannot quite parse and
understand the second sentence.

It is not we treat known invalid trees as errors.  I think what
you meant is we insist that a cache-tree entry, even if it is an
invalidated one, must record the correct number of subtrees and
signal errors otherwise, which is wrong.

I honestly cannot guess what you meant to say by because we can


 diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
 index 47eab97..ad42ca1 100644
 --- a/test-dump-cache-tree.c
 +++ b/test-dump-cache-tree.c
 @@ -6,12 +6,12 @@
  static void dump_one(struct cache_tree *it, const char *pfx, const char *x)
  {
   if (it-entry_count  0)
 - printf(%-40s %s%s (%d subtrees)\n,
 -invalid, x, pfx, it-subtree_nr);
 + printf(%-40s %s (%d subtrees)%s\n,
 +invalid, pfx, it-subtree_nr, x);
   else
 - printf(%s %s%s (%d entries, %d subtrees)\n,
 -sha1_to_hex(it-sha1), x, pfx,
 -it-entry_count, it-subtree_nr);
 + printf(%s %s (%d entries, %d subtrees)%s\n,
 +sha1_to_hex(it-sha1), pfx,
 +it-entry_count, it-subtree_nr, x);

I am guessing this is related to the more useful, but I cannot
offhand tell what this output shuffling is about.  It would be
better to illustrate in the log message to support the more useful
claim by showing how improved/readable the output got with this
change.

  }
  
  static int dump_cache_tree(struct cache_tree *it,
 @@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it,
   /* missing in either */
   return 0;
  
 - if (it-entry_count  0) {
 + if (it-entry_count  0)
 + /* invalid */
   dump_one(it, pfx, );
 - dump_one(ref, pfx, #(ref) );

Unfortunately this is not quite what we would want; this #(ref)
output is so that we can see what tree object we should be referring
to, while debugging, if this entry were not invalidated; losing that
does not Improve output---it loses information from the output.

 - if (it-subtree_nr != ref-subtree_nr)
 - errs = 1;

I am guessing that this is the change you did not explain quite
enough with do not treat ... as errors.  This code expects that
even an invalidated cache-tree entry knows how many subtrees it has,
which is unreasonable.  I do not recall offhand if we used to have
some code to ensure that such an invariant holds or not, but when
invalidating a directory (say t/) by adding a new subdirectory and
a new file in it (e.g. t/dir/file) to the index, we do not do
anything other than to invalidate t/ and t/dir/, and I do not
think the codepath recounts the number of subdirectories to adjust
subtree_nr in any way to match the reality, so removal of this check
is the right thing to do.

 - }
   else {
 - dump_one(it, pfx, );
   if (hashcmp(it-sha1, ref-sha1) ||
   ref-entry_count != it-entry_count ||
   ref-subtree_nr != it-subtree_nr) {
 - dump_one(ref, pfx, #(ref) );
 + /* claims to be valid but is lying */
 + dump_one(ref, pfx,  #(error));
   errs = 1;
 + } else {
 + /* is actually valid */
 + dump_one(it, pfx, );
   }
   }
--
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 2/3] test-dump-cache-tree: Improve output format and exit code

2014-06-30 Thread David Turner 
Make test-dump-cache-tree more useful for testing.  Do not treat known
invalid trees as errors (and do not produce non-zero exit code),
because we can fall back to the non-cache-tree codepath.

Signed-off-by: David Turner dtur...@twitter.com
---
 t/t0090-cache-tree.sh  | 28 +---
 test-dump-cache-tree.c | 24 
 2 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 7c60675..5383258 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -21,10 +21,13 @@ test_shallow_cache_tree () {
cmp_cache_tree expect
 }
 
+# Test that the cache-tree for a given directory is invalid.
+# If no directory is given, check that the root is invalid
 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
+   test-dump-cache-tree actual 
+   sed -e s/$_x40/SHA/ -e s/[0-9]* subtrees//g actual filtered 
+   expect=$(printf invalid  $1 ()\n) 
+   fgrep $expect filtered
 }
 
 test_no_cache_tree () {
@@ -49,6 +52,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 
diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index 47eab97..ad42ca1 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -6,12 +6,12 @@
 static void dump_one(struct cache_tree *it, const char *pfx, const char *x)
 {
if (it-entry_count  0)
-   printf(%-40s %s%s (%d subtrees)\n,
-  invalid, x, pfx, it-subtree_nr);
+   printf(%-40s %s (%d subtrees)%s\n,
+  invalid, pfx, it-subtree_nr, x);
else
-   printf(%s %s%s (%d entries, %d subtrees)\n,
-  sha1_to_hex(it-sha1), x, pfx,
-  it-entry_count, it-subtree_nr);
+   printf(%s %s (%d entries, %d subtrees)%s\n,
+  sha1_to_hex(it-sha1), pfx,
+  it-entry_count, it-subtree_nr, x);
 }
 
 static int dump_cache_tree(struct cache_tree *it,
@@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it,
/* missing in either */
return 0;
 
-   if (it-entry_count  0) {
+   if (it-entry_count  0)
+   /* invalid */
dump_one(it, pfx, );
-   dump_one(ref, pfx, #(ref) );
-   if (it-subtree_nr != ref-subtree_nr)
-   errs = 1;
-   }
else {
-   dump_one(it, pfx, );
if (hashcmp(it-sha1, ref-sha1) ||
ref-entry_count != it-entry_count ||
ref-subtree_nr != it-subtree_nr) {
-   dump_one(ref, pfx, #(ref) );
+   /* claims to be valid but is lying */
+   dump_one(ref, pfx,  #(error));
errs = 1;
+   } else {
+   /* is actually valid */
+   dump_one(it, pfx, );
}
}
 
-- 
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 2/3] test-dump-cache-tree: Improve output format and exit code

2014-06-30 Thread Eric Sunshine 
On Mon, Jun 30, 2014 at 8:13 PM, David Turner dtur...@twopensource.com wrote:
 Make test-dump-cache-tree more useful for testing.  Do not treat known
 invalid trees as errors (and do not produce non-zero exit code),
 because we can fall back to the non-cache-tree codepath.

 Signed-off-by: David Turner dtur...@twitter.com
 ---
  t/t0090-cache-tree.sh  | 28 +---
  test-dump-cache-tree.c | 24 
  2 files changed, 37 insertions(+), 15 deletions(-)

 diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
 index 7c60675..5383258 100755
 --- a/t/t0090-cache-tree.sh
 +++ b/t/t0090-cache-tree.sh
 @@ -21,10 +21,13 @@ test_shallow_cache_tree () {
 cmp_cache_tree expect
  }

 +# Test that the cache-tree for a given directory is invalid.
 +# If no directory is given, check that the root is invalid
  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
 +   test-dump-cache-tree actual 
 +   sed -e s/$_x40/SHA/ -e s/[0-9]* subtrees//g actual filtered 
 +   expect=$(printf invalid  $1 ()\n) 
 +   fgrep $expect filtered
  }

  test_no_cache_tree () {
 @@ -49,6 +52,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 

Style: drop whitespace after 

 +   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

Broken -chain.

 +   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 

Style: drop whitespace after 

 +   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 
 diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
 index 47eab97..ad42ca1 100644
 --- a/test-dump-cache-tree.c
 +++ b/test-dump-cache-tree.c
 @@ -6,12 +6,12 @@
  static void dump_one(struct cache_tree *it, const char *pfx, const char *x)
  {
 if (it-entry_count  0)
 -   printf(%-40s %s%s (%d subtrees)\n,
 -  invalid, x, pfx, it-subtree_nr);
 +   printf(%-40s %s (%d subtrees)%s\n,
 +  invalid, pfx, it-subtree_nr, x);
 else
 -   printf(%s %s%s (%d entries, %d subtrees)\n,
 -  sha1_to_hex(it-sha1), x, pfx,
 -  it-entry_count, it-subtree_nr);
 +   printf(%s %s (%d entries, %d subtrees)%s\n,
 +  sha1_to_hex(it-sha1), pfx,
 +  it-entry_count, it-subtree_nr, x);
  }

  static int dump_cache_tree(struct cache_tree *it,
 @@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it,
 /* missing in either */
 return 0;

 -   if (it-entry_count  0) {
 +   if (it-entry_count  0)
 +   /* invalid */
 dump_one(it, pfx, );
 -   dump_one(ref, pfx, #(ref) );
 -   if (it-subtree_nr != ref-subtree_nr)
 -   errs = 1;
 -   }
 else {
 -   dump_one(it, pfx, );
 if (hashcmp(it-sha1, ref-sha1) ||
 ref-entry_count != it-entry_count ||
 ref-subtree_nr != it-subtree_nr) {
 -   dump_one(ref, pfx, #(ref) );
 +   /* claims to be valid but is lying */
 +   dump_one(ref, pfx,  #(error));
 errs = 1;
 +   } else {
 +   /* is actually valid */
 +   dump_one(it, pfx, );
 }
 }

 --
 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
--
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 2/3] test-dump-cache-tree: Improve output format and exit code

2014-06-27 Thread David Turner 
Make test-dump-cache-tree more useful for testing.  Do not treat known
invalid trees as errors (and do not produce non-zero exit code),
because we can fall back to the non-cache-tree codepath.

Signed-off-by: David Turner dtur...@twitter.com
---
 t/t0090-cache-tree.sh  | 28 +---
 test-dump-cache-tree.c | 24 
 2 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 7c60675..5383258 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -21,10 +21,13 @@ test_shallow_cache_tree () {
cmp_cache_tree expect
 }
 
+# Test that the cache-tree for a given directory is invalid.
+# If no directory is given, check that the root is invalid
 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
+   test-dump-cache-tree actual 
+   sed -e s/$_x40/SHA/ -e s/[0-9]* subtrees//g actual filtered 
+   expect=$(printf invalid  $1 ()\n) 
+   fgrep $expect filtered
 }
 
 test_no_cache_tree () {
@@ -49,6 +52,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 
diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index 47eab97..ad42ca1 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -6,12 +6,12 @@
 static void dump_one(struct cache_tree *it, const char *pfx, const char *x)
 {
if (it-entry_count  0)
-   printf(%-40s %s%s (%d subtrees)\n,
-  invalid, x, pfx, it-subtree_nr);
+   printf(%-40s %s (%d subtrees)%s\n,
+  invalid, pfx, it-subtree_nr, x);
else
-   printf(%s %s%s (%d entries, %d subtrees)\n,
-  sha1_to_hex(it-sha1), x, pfx,
-  it-entry_count, it-subtree_nr);
+   printf(%s %s (%d entries, %d subtrees)%s\n,
+  sha1_to_hex(it-sha1), pfx,
+  it-entry_count, it-subtree_nr, x);
 }
 
 static int dump_cache_tree(struct cache_tree *it,
@@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it,
/* missing in either */
return 0;
 
-   if (it-entry_count  0) {
+   if (it-entry_count  0)
+   /* invalid */
dump_one(it, pfx, );
-   dump_one(ref, pfx, #(ref) );
-   if (it-subtree_nr != ref-subtree_nr)
-   errs = 1;
-   }
else {
-   dump_one(it, pfx, );
if (hashcmp(it-sha1, ref-sha1) ||
ref-entry_count != it-entry_count ||
ref-subtree_nr != it-subtree_nr) {
-   dump_one(ref, pfx, #(ref) );
+   /* claims to be valid but is lying */
+   dump_one(ref, pfx,  #(error));
errs = 1;
+   } else {
+   /* is actually valid */
+   dump_one(it, pfx, );
}
}
 
-- 
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