Re: [PATCH 04/12] commit-graph: verify fanout and lookup table
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- May 10, 2018, 18:29 UTC
- Message-ID
- <CAN0heSoaTdkb5xcrEE4dY7aUpcTGCROyUbZ-HUUKFBHJ9zTGew@mail.gmail.com>
- In-Reply-To
- <20180510173345.40577-5-dstolee@microsoft.com>
On 10 May 2018 at 19:34, Derrick Stolee <dstolee@microsoft.com> wrote:
Show 32 quoted lines
> 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. ;-)
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?
> + > + oid_to_hex(&prev_oid), > + oid_to_hex(&cur_oid));
Hmm, these two lines do not actually achieve anything?
Show 7 quoted lines
> + 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.
Show 11 quoted lines
> +
> + 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?
Martin