Re: [PATCH 04/12] commit-graph: verify fanout and lookup table
On 5/10/2018 2:29 PM, Martin Ågren wrote:
On 10 May 2018 at 19:34, Derrick Stolee 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
---
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
Re: [PATCH 04/12] commit-graph: verify fanout and lookup table
On 10 May 2018 at 19:34, Derrick Stolee 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
> ---
> 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?
> + 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?
Martin
[PATCH 04/12] commit-graph: verify fanout and lookup table
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
---
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"),
+
+ oid_to_hex(&prev_oid),
+ oid_to_hex(&cur_oid));
+ 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);
+
+ 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);
+
+ cur_fanout_pos++;
+ }
+
return verify_commit_graph_error;
}
--
2.16.2.329.gfb62395de6
