Re: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 1, 2014, 21:42 UTC
- Message-ID
- <xmqqpphovjqk.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1404242075-7068-2-git-send-email-dturner@twitter.com>
David Turner <dturner@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 ...".
Show 19 quoted lines
> 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.
Show 12 quoted lines
> }
>
> 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.
Show 15 quoted lines
> - }
> 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, "");
> }
> }