Re: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code
David Turner 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
Re: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code
On Mon, Jun 30, 2014 at 8:13 PM, David Turner 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
> ---
> 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" 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
