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

Reply via email to