David Turner <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
