| Message ID | 20200607193237.699335-4-abhishekkumar8222@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GSOC,v2,1/4] commit-graph: introduce commit_graph_data_slab | expand |
Abhishek Kumar <abhishekkumar8222@gmail.com> writes: > commit_graph_generation() returns GENERATION_NUMBER_INFINITY if the > graph position for commit is COMMIT_NOT_FROM_GRAPH. > > While this is true when reading from a commit graph, no graph positions > are associated with a commit when writing a commit graph. Therefore, the > helper incorrectly returns GENERATION_NUMBER_INFINITY despite having a > finite generation number. > > Let's fix this by using generation number directly when writing a commit > graph. I think that to avoid having non-working patch (which can cause problems when bisecting), it would be a better idea to switch the order of patches 2 and 3. This way we won't have incorrect behaviour. > > Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> > --- > commit-graph.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/commit-graph.c b/commit-graph.c > index f7cca4def4..0dc79e7c90 100644 > --- a/commit-graph.c > +++ b/commit-graph.c > @@ -1070,7 +1070,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len, > else > packedDate[0] = 0; > > - packedDate[0] |= htonl(commit_graph_generation((*list)) << 2); > + packedDate[0] |= htonl(commit_graph_data_at(*list)->generation << 2); > All right. > packedDate[1] = htonl((*list)->date); > hashwrite(f, packedDate, 8); > @@ -1301,9 +1301,11 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx) > _("Computing commit graph generation numbers"), > ctx->commits.nr); > for (i = 0; i < ctx->commits.nr; i++) { > + uint32_t generation = commit_graph_data_at(ctx->commits.list[i])->generation; > + > display_progress(ctx->progress, i + 1); > - if (commit_graph_generation(ctx->commits.list[i]) != GENERATION_NUMBER_INFINITY && > - commit_graph_generation(ctx->commits.list[i]) != GENERATION_NUMBER_ZERO) > + if (generation != GENERATION_NUMBER_INFINITY && > + generation != GENERATION_NUMBER_ZERO) > continue; > All right; this also introduces local variable to avoid accessing the slab twice^W four times... > commit_list_insert(ctx->commits.list[i], &list); > @@ -1314,8 +1316,9 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx) > uint32_t max_generation = 0; > > for (parent = current->parents; parent; parent = parent->next) { > - if (commit_graph_generation(parent->item) == GENERATION_NUMBER_INFINITY || > - commit_graph_generation(parent->item) == GENERATION_NUMBER_ZERO) { > + > + if (generation == GENERATION_NUMBER_INFINITY || > + generation == GENERATION_NUMBER_ZERO) { > all_parents_computed = 0; > commit_list_insert(parent->item, &list); > break; ... which is then used here. Best,
On Mon, Jun 08, 2020 at 06:31:49PM +0200, Jakub Narębski wrote: > Abhishek Kumar <abhishekkumar8222@gmail.com> writes: > > > commit_graph_generation() returns GENERATION_NUMBER_INFINITY if the > > graph position for commit is COMMIT_NOT_FROM_GRAPH. > > > > While this is true when reading from a commit graph, no graph positions > > are associated with a commit when writing a commit graph. Therefore, the > > helper incorrectly returns GENERATION_NUMBER_INFINITY despite having a > > finite generation number. > > > > Let's fix this by using generation number directly when writing a commit > > graph. > > I think that to avoid having non-working patch (which can cause problems > when bisecting), it would be a better idea to switch the order of > patches 2 and 3. This way we won't have incorrect behaviour. Yup... agreed (with the additional caveat that the unused function in the first patch also be moved into this new--larger--patch). > > > > Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> > > --- > > commit-graph.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/commit-graph.c b/commit-graph.c > > index f7cca4def4..0dc79e7c90 100644 > > --- a/commit-graph.c > > +++ b/commit-graph.c > > @@ -1070,7 +1070,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len, > > else > > packedDate[0] = 0; > > > > - packedDate[0] |= htonl(commit_graph_generation((*list)) << 2); > > + packedDate[0] |= htonl(commit_graph_data_at(*list)->generation << 2); > > > > All right. > > > packedDate[1] = htonl((*list)->date); > > hashwrite(f, packedDate, 8); > > @@ -1301,9 +1301,11 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx) > > _("Computing commit graph generation numbers"), > > ctx->commits.nr); > > for (i = 0; i < ctx->commits.nr; i++) { > > + uint32_t generation = commit_graph_data_at(ctx->commits.list[i])->generation; > > + > > display_progress(ctx->progress, i + 1); > > - if (commit_graph_generation(ctx->commits.list[i]) != GENERATION_NUMBER_INFINITY && > > - commit_graph_generation(ctx->commits.list[i]) != GENERATION_NUMBER_ZERO) > > + if (generation != GENERATION_NUMBER_INFINITY && > > + generation != GENERATION_NUMBER_ZERO) > > continue; > > > > All right; this also introduces local variable to avoid accessing the > slab twice^W four times... > > > commit_list_insert(ctx->commits.list[i], &list); > > @@ -1314,8 +1316,9 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx) > > uint32_t max_generation = 0; > > > > for (parent = current->parents; parent; parent = parent->next) { > > - if (commit_graph_generation(parent->item) == GENERATION_NUMBER_INFINITY || > > - commit_graph_generation(parent->item) == GENERATION_NUMBER_ZERO) { > > + > > + if (generation == GENERATION_NUMBER_INFINITY || > > + generation == GENERATION_NUMBER_ZERO) { > > all_parents_computed = 0; > > commit_list_insert(parent->item, &list); > > break; > > ... which is then used here. > > Best, > -- > Jakub Narębski Thanks, Taylor
diff --git a/commit-graph.c b/commit-graph.c index f7cca4def4..0dc79e7c90 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1070,7 +1070,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len, else packedDate[0] = 0; - packedDate[0] |= htonl(commit_graph_generation((*list)) << 2); + packedDate[0] |= htonl(commit_graph_data_at(*list)->generation << 2); packedDate[1] = htonl((*list)->date); hashwrite(f, packedDate, 8); @@ -1301,9 +1301,11 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx) _("Computing commit graph generation numbers"), ctx->commits.nr); for (i = 0; i < ctx->commits.nr; i++) { + uint32_t generation = commit_graph_data_at(ctx->commits.list[i])->generation; + display_progress(ctx->progress, i + 1); - if (commit_graph_generation(ctx->commits.list[i]) != GENERATION_NUMBER_INFINITY && - commit_graph_generation(ctx->commits.list[i]) != GENERATION_NUMBER_ZERO) + if (generation != GENERATION_NUMBER_INFINITY && + generation != GENERATION_NUMBER_ZERO) continue; commit_list_insert(ctx->commits.list[i], &list); @@ -1314,8 +1316,9 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx) uint32_t max_generation = 0; for (parent = current->parents; parent; parent = parent->next) { - if (commit_graph_generation(parent->item) == GENERATION_NUMBER_INFINITY || - commit_graph_generation(parent->item) == GENERATION_NUMBER_ZERO) { + + if (generation == GENERATION_NUMBER_INFINITY || + generation == GENERATION_NUMBER_ZERO) { all_parents_computed = 0; commit_list_insert(parent->item, &list); break;
commit_graph_generation() returns GENERATION_NUMBER_INFINITY if the graph position for commit is COMMIT_NOT_FROM_GRAPH. While this is true when reading from a commit graph, no graph positions are associated with a commit when writing a commit graph. Therefore, the helper incorrectly returns GENERATION_NUMBER_INFINITY despite having a finite generation number. Let's fix this by using generation number directly when writing a commit graph. Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> --- commit-graph.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)