Re: [PATCH 07/20] test-read-cache: print cache entries with --table

From: Derrick Stolee <stolee@gmail.com>
To: Elijah Newren <newren@gmail.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Nguyễn Thái Ngọc" <pclouds@gmail.com>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Derrick Stolee" <dstolee@microsoft.com>
Subject: Re: [PATCH 07/20] test-read-cache: print cache entries with --table
Date: Tue, 9 Mar 2021 16:00:57 -0500	[thread overview]
Message-ID: <27f113c8-313e-6812-0646-5073891df448@gmail.com> (raw)
In-Reply-To: <CABPp-BGspVmXFNpAoF-8wcb4eyjyivM4Cpcy4wRfjL6kMxcu6Q@mail.gmail.com>

On 2/25/2021 2:02 AM, Elijah Newren wrote:
> On Tue, Feb 23, 2021 at 12:14 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> This table is helpful for discovering data in the index to ensure it is
>> being written correctly, especially as we build and test the
>> sparse-index. This table includes an output format similar to 'git
>> ls-tree', but should not be compared to that directly. The biggest
>> reasons are that 'git ls-tree' includes a tree entry for every
>> subdirectory, even those that would not appear as a sparse directory in
>> a sparse-index. Further, 'git ls-tree' does not use a trailing directory
>> separator for its tree rows.
>>
>> This does not print the stat() information for the blobs. That could be
>> added in a future change with another option. The tests that are added
>> in the next few changes care only about the object types and IDs.
>>
>> To make the option parsing slightly more robust, wrap the string
>> comparisons in a loop adapted from test-dir-iterator.c.
>>
>> Care must be taken with the final check for the 'cnt' variable. We
>> continue the expectation that the numerical value is the final argument.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>  t/helper/test-read-cache.c | 50 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 40 insertions(+), 10 deletions(-)
>>
>> diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
>> index 244977a29bdf..e4c3492f7d3e 100644
>> --- a/t/helper/test-read-cache.c
>> +++ b/t/helper/test-read-cache.c
>> @@ -2,35 +2,65 @@
>>  #include "cache.h"
>>  #include "config.h"
>>
>> +static void print_cache_entry(struct cache_entry *ce)
>> +{
>> +       printf("%06o ", ce->ce_mode & 0777777);
> 
> This constant is curious.  I think it's because you want to strip off
> the special in-memory bits of the ce_mode where git stores extra data,
> which would be everything beyond the first 16 bits (as noted in a
> comment near the beginning of cache.h).  But here you keep the first
> 18 bits.  Is CE_UPDATE and CE_REMOVE just 0 in the cases you've viewed
> so this works (but you really should use 0177777 or 0xFFFF), or am I
> off in my guess of what you're trying to do and you do want to see
> those two flags?

You're right, 0177777 is what I want. I'm focusing only on the
file permissions bits that are reported by ls-tree.

> It also seems surprising to me that this constant isn't already
> defined somewhere in cache.h or as some variant of S_IFMT, though I'm
> not finding it.

I'm not, either.

>> +
>> +       if (S_ISSPARSEDIR(ce->ce_mode))
>> +               printf("tree ");
>> +       else if (S_ISGITLINK(ce->ce_mode))
>> +               printf("commit ");
>> +       else
>> +               printf("blob ");
> 
> Perhaps make use of the tree_type, commit_type, and blob_type global constants?

Today I Learned...

>> +
>> +       printf("%s\t%s\n",
>> +              oid_to_hex(&ce->oid),
>> +              ce->name);
>> +}
>> +
>> +static void print_cache(struct index_state *cache)
>> +{
>> +       int i;
>> +       for (i = 0; i < the_index.cache_nr; i++)
>> +               print_cache_entry(the_index.cache[i]);
> 
> Why are you passing cache as a parameter, then ignoring it and using the_index?

Good catch!

Thanks,
-Stolee