From: Derrick Stolee <stolee@gmail.com>
Date: Fri, 11 May 2018 15:17:26 GMT
Subject: Re: [PATCH 04/12] commit-graph: verify fanout and lookup table
Message-ID: <62c7d8c3-87e3-f8e3-a83a-fb53f956f89d@gmail.com>
In-Reply-To: <CAN0heSoaTdkb5xcrEE4dY7aUpcTGCROyUbZ-HUUKFBHJ9zTGew@mail.gmail.com>

On 5/10/2018 2:29 PM, Martin Ågren wrote:
> On 10 May 2018 at 19:34, Derrick Stolee <dstolee@microsoft.com> wrote:
>> While running 'git commit-graph verify', verify that the object IDs
>> are listed in lexicographic order and that the fanout table correctly
>> navigates into that list of object IDs.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>   commit-graph.c | 33 +++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/commit-graph.c b/commit-graph.c
>> index ce11af1d20..b4c146c423 100644
>> --- a/commit-graph.c
>> +++ b/commit-graph.c
>> @@ -839,6 +839,9 @@ static int verify_commit_graph_error;
>>
>>   int verify_commit_graph(struct commit_graph *g)
>>   {
>> +       uint32_t i, cur_fanout_pos = 0;
>> +       struct object_id prev_oid, cur_oid;
>> +
>>          if (!g) {
>>                  graph_report(_("no commit-graph file loaded"));
>>                  return 1;
>> @@ -853,5 +856,35 @@ int verify_commit_graph(struct commit_graph *g)
>>          if (!g->chunk_commit_data)
>>                  graph_report(_("commit-graph is missing the Commit Data chunk"));
>>
>> +       for (i = 0; i < g->num_commits; i++) {
>> +               hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
>> +
>> +               if (i > 0 && oidcmp(&prev_oid, &cur_oid) >= 0)
>> +                       graph_report(_("commit-graph has incorrect oid order: %s then %s"),
> Minor: I think our style would prefer s/i > 0/i/.
>
> I suppose the second check should be s/>=/>/, but it's not like it
> should matter. ;-)

It shouldn't, but a duplicate commit is still an incorrect oid order.

> I wonder if this is a message that would virtually never make sense to a
> user, but more to a developer. Leave it untranslated to make sure any
> bug reports to the list are readable to us?

Yeah, I'll make all of the errors untranslated since they are for 
developer debugging, not end-user information.

>> +
>> +               oid_to_hex(&prev_oid),
>> +               oid_to_hex(&cur_oid));
> Hmm, these two lines do not actually achieve anything?

It's a formatting bug: They complete the statement starting with 
'graph_report(' above.

>
>> +               oidcpy(&prev_oid, &cur_oid);
>> +
>> +               while (cur_oid.hash[0] > cur_fanout_pos) {
>> +                       uint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);
>> +                       if (i != fanout_value)
>> +                               graph_report(_("commit-graph has incorrect fanout value: fanout[%d] = %u != %u"),
>> +                                            cur_fanout_pos, fanout_value, i);
> Same though re `_()`, even more so because of the more technical
> notation.
>
>> +
>> +                       cur_fanout_pos++;
>> +               }
>> +       }
>> +
>> +       while (cur_fanout_pos < 256) {
>> +               uint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);
>> +
>> +               if (g->num_commits != fanout_value)
>> +                       graph_report(_("commit-graph has incorrect fanout value: fanout[%d] = %u != %u"),
>> +                                    cur_fanout_pos, fanout_value, i);
> Same here. Or maybe these should just give a translated user-readable
> basic idea of what is wrong and skip the details?

As someone who is responsible for probably inserting these problems, I 
think the details are important.

Thanks,
-Stolee