From: Junio C Hamano <gitster@pobox•com>
To: David Turner <dturner@twopensource•com>
Cc: git@vger•kernel.org, David Turner <dturner@twitter•com>
Subject: Re: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code
Date: Tue, 01 Jul 2014 14:42:43 -0700 [thread overview]
Message-ID: <xmqqpphovjqk.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1404242075-7068-2-git-send-email-dturner@twitter.com> (David Turner's message of "Tue, 1 Jul 2014 12:14:34 -0700")
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
...".
> 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, "");
> }
> }
next prev parent reply other threads:[~2014-07-01 21:42 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-01 19:14 [PATCH 1/3] cache-tree: Create/update cache-tree on checkout David Turner
2014-07-01 19:14 ` [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code David Turner
2014-07-01 21:42 ` Junio C Hamano [this message]
2014-07-01 19:14 ` [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit David Turner
2014-07-01 22:45 ` Junio C Hamano
2014-07-01 22:58 ` David Turner
2014-07-01 21:08 ` [PATCH 1/3] cache-tree: Create/update cache-tree on checkout Junio C Hamano
-- strict thread matches above, loose matches on Subject: below --
2014-07-01 0:13 David Turner
2014-07-01 0:13 ` [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code David Turner
2014-07-01 4:43 ` Eric Sunshine
2014-06-28 0:20 [PATCH 1/3] cache-tree: Create/update cache-tree on checkout David Turner
2014-06-28 0:20 ` [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code David Turner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqpphovjqk.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=dturner@twitter$(echo .)com \
--cc=dturner@twopensource$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox