Re: [PATCH 3/6] commit-graph: consolidate fill_commit_graph_info
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jul 28, 2020, 13:14 UTC
- Message-ID
- <a9d50995-566d-cad2-ff67-8b8604b52eed@gmail.com>
- In-Reply-To
- <701f5912369c0fcc07cf604c3129cb5017a125ce.1595927632.git.gitgitgadget@gmail.com>
On 7/28/2020 5:13 AM, Abhishek Kumar via GitGitGadget wrote:
Show 12 quoted lines
> From: Abhishek Kumar <abhishekkumar8222@gmail.com> > > Both fill_commit_graph_info() and fill_commit_in_graph() parse > information present in commit data chunk. Let's simplify the > implementation by calling fill_commit_graph_info() within > fill_commit_in_graph(). > > The test 'generate tar with future mtime' creates a commit with commit > time of (2 ^ 36 + 1) seconds since EPOCH. The commit time overflows into > generation number and has undefined behavior. The test used to pass as > fill_commit_in_graph() did not read commit time from commit graph, > reading commit date from odb instead.
I was first confused as to why fill_commit_graph_info() did not load the timestamp, but the reason is that it is only used by two methods:
1. fill_commit_in_graph(): this actually leaves the commit in a "parsed" state, so the date must be correct. Thus, it parses the date out of the commit-graph.
2. load_commit_graph_info(): this only helps to guarantee we know the graph_pos and generation number values.
Perhaps add this extra context: you will _need_ the commit date from the commit-graph in order to populate the generation number v2 in fill_commit_graph_info().
> Let's fix that by setting commit time of (2 ^ 34 - 1) seconds.
The timestamp limit placed in the commit-graph is more restrictive than 64-bit timestamps, but as your test points out, the maximum timestamp allowed takes place in the year 2514. That is far enough away for all real data.
Show 49 quoted lines
> Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
> ---
> commit-graph.c | 31 ++++++++++++-------------------
> t/t5000-tar-tree.sh | 4 ++--
> 2 files changed, 14 insertions(+), 21 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 5d3c9bd23c..204eb454b2 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -735,15 +735,24 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,
> const unsigned char *commit_data;
> struct commit_graph_data *graph_data;
> uint32_t lex_index;
> + uint64_t date_high, date_low;
>
> while (pos < g->num_commits_in_base)
> g = g->base_graph;
>
> + if (pos >= g->num_commits + g->num_commits_in_base)
> + die(_("invalid commit position. commit-graph is likely corrupt"));
> +
> lex_index = pos - g->num_commits_in_base;
> commit_data = g->chunk_commit_data + GRAPH_DATA_WIDTH * lex_index;
>
> graph_data = commit_graph_data_at(item);
> graph_data->graph_pos = pos;
> +
> + date_high = get_be32(commit_data + g->hash_len + 8) & 0x3;
> + date_low = get_be32(commit_data + g->hash_len + 12);
> + item->date = (timestamp_t)((date_high << 32) | date_low);
> +
> graph_data->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
> }
>
> @@ -758,38 +767,22 @@ static int fill_commit_in_graph(struct repository *r,
> {
> uint32_t edge_value;
> uint32_t *parent_data_ptr;
> - uint64_t date_low, date_high;
> struct commit_list **pptr;
> - struct commit_graph_data *graph_data;
> const unsigned char *commit_data;
> uint32_t lex_index;
>
> + fill_commit_graph_info(item, g, pos);
> +
> while (pos < g->num_commits_in_base)
> g = g->base_graph;This 'while' loop happens in both implementations, so you could save a miniscule amount of time by placing the call to fill_commit_graph_info() after the while loop.
> - if (pos >= g->num_commits + g->num_commits_in_base)
> - die(_("invalid commit position. commit-graph is likely corrupt"));Show 10 quoted lines
> - /* > - * Store the "full" position, but then use the > - * "local" position for the rest of the calculation. > - */ > - graph_data = commit_graph_data_at(item); > - graph_data->graph_pos = pos; > lex_index = pos - g->num_commits_in_base; > - > - commit_data = g->chunk_commit_data + (g->hash_len + 16) * lex_index; > + commit_data = g->chunk_commit_data + GRAPH_DATA_WIDTH * lex_index;
I was about to complain about this change, but GRAPH_DATA_WIDTH is a macro that does an equivalent thing (except the_hash_algo->rawsz instead of g->hash_len).
Show 37 quoted lines
> > item->object.parsed = 1; > > set_commit_tree(item, NULL); > > - date_high = get_be32(commit_data + g->hash_len + 8) & 0x3; > - date_low = get_be32(commit_data + g->hash_len + 12); > - item->date = (timestamp_t)((date_high << 32) | date_low); > - > - graph_data->generation = get_be32(commit_data + g->hash_len + 8) >> 2; > - > pptr = &item->parents; > > edge_value = get_be32(commit_data + g->hash_len); > diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh > index 37655a237c..1986354fc3 100755 > --- a/t/t5000-tar-tree.sh > +++ b/t/t5000-tar-tree.sh > @@ -406,7 +406,7 @@ test_expect_success TIME_IS_64BIT 'set up repository with far-future commit' ' > rm -f .git/index && > echo content >file && > git add file && > - GIT_COMMITTER_DATE="@68719476737 +0000" \ > + GIT_COMMITTER_DATE="@17179869183 +0000" \ > git commit -m "tempori parendum" > ' > > @@ -415,7 +415,7 @@ test_expect_success TIME_IS_64BIT 'generate tar with future mtime' ' > ' > > test_expect_success TAR_HUGE,TIME_IS_64BIT,TIME_T_IS_64BIT 'system tar can read our future mtime' ' > - echo 4147 >expect && > + echo 2514 >expect && > tar_info future.tar | cut -d" " -f2 >actual && > test_cmp expect actual > ' >
Thanks, -Stolee