[GSoC,2/3] commit: convert commit->generation to a slab
diff mbox series

Message ID 20200604072759.19142-3-abhishekkumar8222@gmail.com
State New
Headers show
Series
  • Move generation, graph_pos to a slab
Related show

Commit Message

Abhishek Kumar June 4, 2020, 7:27 a.m. UTC
In this commit, we will use the generation slab helpers introduced in
last commit and replace existing uses of commit->generation using
'contrib/coccinelle/generation.cocci'

Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---
 alloc.c                             |  1 -
 blame.c                             |  2 +-
 commit-graph.c                      | 39 +++++++++++-----------
 commit-reach.c                      | 50 ++++++++++++++---------------
 commit.c                            |  4 +--
 commit.h                            |  1 -
 contrib/coccinelle/generation.cocci | 12 +++++++
 revision.c                          | 16 ++++-----
 8 files changed, 68 insertions(+), 57 deletions(-)
 create mode 100644 contrib/coccinelle/generation.cocci

Comments

Derrick Stolee June 4, 2020, 2:27 p.m. UTC | #1
On 6/4/2020 3:27 AM, Abhishek Kumar wrote:
> In this commit, we will use the generation slab helpers introduced in
> last commit and replace existing uses of commit->generation using
> 'contrib/coccinelle/generation.cocci'
> @@ -1048,7 +1048,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
>  		else
>  			packedDate[0] = 0;
>  
> -		packedDate[0] |= htonl((*list)->generation << 2);
> +		packedDate[0] |= htonl(generation((*list)) << 2);

nit: We no longer need the extra parens around *list.

> @@ -3414,8 +3414,8 @@ static void init_topo_walk(struct rev_info *revs)
>  		test_flag_and_insert(&info->explore_queue, c, TOPO_WALK_EXPLORED);
>  		test_flag_and_insert(&info->indegree_queue, c, TOPO_WALK_INDEGREE);
>  
> -		if (c->generation < info->min_generation)
> -			info->min_generation = c->generation;
> +		if (generation(c) < info->min_generation)
> +			info->min_generation = generation(c);

A pattern I've noticed in several places is that the struct
member is accessed multiple times in the same method body,
and this is auto-converted to multiple method calls. However,
these values are fixed, so it would be better to store the
value as a local variable and reuse that variable instead.

This is one of the shortcomings of the Coccinelle transformation,
so you'll need to manually inspect each of the diff fragments to
see if we can reduce the number of method calls. It might be
helpful to do that as a follow-up, so we can see that this patch
is generated by the Coccinelle script, and then a later patch can
be scrutinized more carefully when you are doing manual code
manipulation.

Thanks,
-Stolee
Junio C Hamano June 4, 2020, 5:49 p.m. UTC | #2
Abhishek Kumar <abhishekkumar8222@gmail.com> writes:

> In this commit, we will use the generation slab helpers introduced in
> last commit and replace existing uses of commit->generation using
> 'contrib/coccinelle/generation.cocci'
>
> Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
> ---
>  alloc.c                             |  1 -
>  blame.c                             |  2 +-
>  commit-graph.c                      | 39 +++++++++++-----------
>  commit-reach.c                      | 50 ++++++++++++++---------------
>  commit.c                            |  4 +--
>  commit.h                            |  1 -
>  contrib/coccinelle/generation.cocci | 12 +++++++
>  revision.c                          | 16 ++++-----
>  8 files changed, 68 insertions(+), 57 deletions(-)
>  create mode 100644 contrib/coccinelle/generation.cocci
>
> diff --git a/alloc.c b/alloc.c
> index 1c64c4dd16..cbed187094 100644
> --- a/alloc.c
> +++ b/alloc.c
> @@ -109,7 +109,6 @@ void init_commit_node(struct repository *r, struct commit *c)
>  	c->object.type = OBJ_COMMIT;
>  	c->index = alloc_commit_index(r);
>  	c->graph_pos = COMMIT_NOT_FROM_GRAPH;
> -	c->generation = GENERATION_NUMBER_INFINITY;
>  }
>  
>  void *alloc_commit_node(struct repository *r)
> diff --git a/blame.c b/blame.c
> index da7e28800e..50e6316076 100644
> --- a/blame.c
> +++ b/blame.c
> @@ -1272,7 +1272,7 @@ static int maybe_changed_path(struct repository *r,
>  	if (!bd)
>  		return 1;
>  
> -	if (origin->commit->generation == GENERATION_NUMBER_INFINITY)
> +	if (generation(origin->commit) == GENERATION_NUMBER_INFINITY)

Hmmmm, as C is not all that object-oriented that lets us say "commit
objects have generation() method", a plain vanilla function whose
name is generation() is a bit overly vague.  The field name this
helper function replaces, .generation, is very localized to a commit
"object" and does not have such a problem.

We probably need to choose a better name in the previous step to fix
it.
Jakub Narębski June 6, 2020, 10:03 p.m. UTC | #3
Abhishek Kumar <abhishekkumar8222@gmail.com> writes:

> In this commit, we will use the generation slab helpers introduced in
> last commit and replace existing uses of commit->generation using
> 'contrib/coccinelle/generation.cocci'
>
> Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
> ---
>  alloc.c                             |  1 -
>  blame.c                             |  2 +-
>  commit-graph.c                      | 39 +++++++++++-----------
>  commit-reach.c                      | 50 ++++++++++++++---------------
>  commit.c                            |  4 +--
>  commit.h                            |  1 -
>  contrib/coccinelle/generation.cocci | 12 +++++++
>  revision.c                          | 16 ++++-----
>  8 files changed, 68 insertions(+), 57 deletions(-)
>  create mode 100644 contrib/coccinelle/generation.cocci
>

For easier review I have changed the order of files, grouping together
different categories of changes.


First category is removing commit->generation and related changes:

> diff --git a/commit.h b/commit.h
> index cc610400d5..01e1c4c3eb 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -34,7 +34,6 @@ struct commit {
>  	 */
>  	struct tree *maybe_tree;
>  	uint32_t graph_pos;
> -	uint32_t generation;
>  	unsigned int index;
>  };
>  

This is quite straightforward.

> diff --git a/alloc.c b/alloc.c
> index 1c64c4dd16..cbed187094 100644
> --- a/alloc.c
> +++ b/alloc.c
> @@ -109,7 +109,6 @@ void init_commit_node(struct repository *r, struct commit *c)
>  	c->object.type = OBJ_COMMIT;
>  	c->index = alloc_commit_index(r);
>  	c->graph_pos = COMMIT_NOT_FROM_GRAPH;
> -	c->generation = GENERATION_NUMBER_INFINITY;
>  }
>  
>  void *alloc_commit_node(struct repository *r)

But this change might need a more detailed write-up in the commit
message.

If I understand it correctly, this is function is used by generic commit
allocator, and given commit object may not need generation number. That
is why the default value of GENERATION_NUMBER_INFINITY is handled by new
"getters" and "setters", i.e. generation() and set_generation() -- which
are called only if commit-graph is present, I think.

The generation() returns GENERATION_NUMBER_INFINITY for commits not in
generation_slab, assuming that for all commits in the commit graph it
would be filled by commit-graph parsing -- just like init_commit_node()
would initialize commit->generation to this value.

Because <slabname>_at() (including generation_slab_at()) extends array
if necessary, we want all data on generation_slab to be correctly
initialized to GENERATION_NUMBER_INFINITY, just like init_commit_node()
would do it, because generation() "getter" cannot.  The commit might be
present on generation_slab just because allocation is done in chunks
(slabs).


Second category is semantic patch that generates the rest of changes
(which could be adjusted manually in subsequent commits).

> diff --git a/contrib/coccinelle/generation.cocci b/contrib/coccinelle/generation.cocci
> new file mode 100644
> index 0000000000..da13c44856
> --- /dev/null
> +++ b/contrib/coccinelle/generation.cocci
> @@ -0,0 +1,12 @@
> +@@
> +struct commit *c;
> +expression E;
> +@@
> +- c->generation = E
> ++ set_generation(c, E)
> +
> +@@
> +struct commit *c;
> +@@
> +- c->generation
> ++ generation(c)

I wonder if Coccinelle is able to automatically discard extra
parentheses (the problem noticed by Stolee in his reply) with the
following chunk:

  +@@
  +struct commit *c;
  +@@
  +- (c)->generation
  ++ generation(c)


Third category is all the changes that are just straight mechanical
changes being the result of applying the Coccinelle patch.

> diff --git a/blame.c b/blame.c
> index da7e28800e..50e6316076 100644
> --- a/blame.c
> +++ b/blame.c
> @@ -1272,7 +1272,7 @@ static int maybe_changed_path(struct repository *r,
>  	if (!bd)
>  		return 1;
>  
> -	if (origin->commit->generation == GENERATION_NUMBER_INFINITY)
> +	if (generation(origin->commit) == GENERATION_NUMBER_INFINITY)
>  		return 1;
>  
>  	filter = get_bloom_filter(r, origin->commit, 0);
> diff --git a/commit-graph.c b/commit-graph.c
> index 63f419048d..9ce7d4acb1 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -120,9 +120,9 @@ static int commit_gen_cmp(const void *va, const void *vb)
>  	const struct commit *b = *(const struct commit **)vb;
>  
>  	/* lower generation commits first */
> -	if (a->generation < b->generation)
> +	if (generation(a) < generation(b))
>  		return -1;
> -	else if (a->generation > b->generation)
> +	else if (generation(a) > generation(b))
>  		return 1;
>  
>  	/* use date as a heuristic when generations are equal */
> @@ -712,7 +712,7 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,
>  	lex_index = pos - g->num_commits_in_base;
>  	commit_data = g->chunk_commit_data + GRAPH_DATA_WIDTH * lex_index;
>  	item->graph_pos = pos;
> -	item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
> +	set_generation(item, get_be32(commit_data + g->hash_len + 8) >> 2);
>  }
>  
>  static inline void set_commit_tree(struct commit *c, struct tree *t)
> @@ -754,7 +754,7 @@ static int fill_commit_in_graph(struct repository *r,
>  	date_low = get_be32(commit_data + g->hash_len + 12);
>  	item->date = (timestamp_t)((date_high << 32) | date_low);
>  
> -	item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
> +	set_generation(item, get_be32(commit_data + g->hash_len + 8) >> 2);
>  
>  	pptr = &item->parents;
>  
> @@ -1048,7 +1048,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
>  		else
>  			packedDate[0] = 0;
>  
> -		packedDate[0] |= htonl((*list)->generation << 2);
> +		packedDate[0] |= htonl(generation((*list)) << 2);
>  
>  		packedDate[1] = htonl((*list)->date);
>  		hashwrite(f, packedDate, 8);
> @@ -1280,8 +1280,8 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
>  					ctx->commits.nr);
>  	for (i = 0; i < ctx->commits.nr; i++) {
>  		display_progress(ctx->progress, i + 1);
> -		if (ctx->commits.list[i]->generation != GENERATION_NUMBER_INFINITY &&
> -		    ctx->commits.list[i]->generation != GENERATION_NUMBER_ZERO)
> +		if (generation(ctx->commits.list[i]) != GENERATION_NUMBER_INFINITY &&
> +		    generation(ctx->commits.list[i]) != GENERATION_NUMBER_ZERO)
>  			continue;
>  
>  		commit_list_insert(ctx->commits.list[i], &list);
> @@ -1292,22 +1292,23 @@ 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 (parent->item->generation == GENERATION_NUMBER_INFINITY ||
> -				    parent->item->generation == GENERATION_NUMBER_ZERO) {
> +				if (generation(parent->item) == GENERATION_NUMBER_INFINITY ||
> +				    generation(parent->item) == GENERATION_NUMBER_ZERO) {
>  					all_parents_computed = 0;
>  					commit_list_insert(parent->item, &list);
>  					break;
> -				} else if (parent->item->generation > max_generation) {
> -					max_generation = parent->item->generation;
> +				} else if (generation(parent->item) > max_generation) {
> +					max_generation = generation(parent->item);
>  				}
>  			}

Here generation(parent->item) is called three times, which is probably
cost effective to save the value to the local variable (as Stolee
noticed).

>  
>  			if (all_parents_computed) {
> -				current->generation = max_generation + 1;
> +				set_generation(current, max_generation + 1);
>  				pop_commit(&list);
>  
> -				if (current->generation > GENERATION_NUMBER_MAX)
> -					current->generation = GENERATION_NUMBER_MAX;
> +				if (generation(current) > GENERATION_NUMBER_MAX)
> +					set_generation(current,
> +						       GENERATION_NUMBER_MAX);
>  			}
>  		}
>  	}
> @@ -2314,8 +2315,8 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)
>  					     oid_to_hex(&graph_parents->item->object.oid),
>  					     oid_to_hex(&odb_parents->item->object.oid));
>  
> -			if (graph_parents->item->generation > max_generation)
> -				max_generation = graph_parents->item->generation;
> +			if (generation(graph_parents->item) > max_generation)
> +				max_generation = generation(graph_parents->item);
>  
>  			graph_parents = graph_parents->next;
>  			odb_parents = odb_parents->next;
> @@ -2325,7 +2326,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)
>  			graph_report(_("commit-graph parent list for commit %s terminates early"),
>  				     oid_to_hex(&cur_oid));
>  
> -		if (!graph_commit->generation) {
> +		if (!generation(graph_commit)) {
>  			if (generation_zero == GENERATION_NUMBER_EXISTS)
>  				graph_report(_("commit-graph has generation number zero for commit %s, but non-zero elsewhere"),
>  					     oid_to_hex(&cur_oid));
> @@ -2345,10 +2346,10 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)
>  		if (max_generation == GENERATION_NUMBER_MAX)
>  			max_generation--;
>  
> -		if (graph_commit->generation != max_generation + 1)
> +		if (generation(graph_commit) != max_generation + 1)
>  			graph_report(_("commit-graph generation for commit %s is %u != %u"),
>  				     oid_to_hex(&cur_oid),
> -				     graph_commit->generation,
> +				     generation(graph_commit),
>  				     max_generation + 1);
>  
>  		if (graph_commit->date != odb_commit->date)
> diff --git a/commit-reach.c b/commit-reach.c
> index 4ca7e706a1..77c980054a 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -59,13 +59,13 @@ static struct commit_list *paint_down_to_common(struct repository *r,
>  		struct commit_list *parents;
>  		int flags;
>  
> -		if (min_generation && commit->generation > last_gen)
> +		if (min_generation && generation(commit) > last_gen)
>  			BUG("bad generation skip %8x > %8x at %s",
> -			    commit->generation, last_gen,
> +			    generation(commit), last_gen,
>  			    oid_to_hex(&commit->object.oid));
> -		last_gen = commit->generation;
> +		last_gen = generation(commit);
>  
> -		if (commit->generation < min_generation)
> +		if (generation(commit) < min_generation)
>  			break;

Here generation(commit) is called three times (two times in the
exceptional case).

>  
>  		flags = commit->object.flags & (PARENT1 | PARENT2 | STALE);
> @@ -176,7 +176,7 @@ static int remove_redundant(struct repository *r, struct commit **array, int cnt
>  		repo_parse_commit(r, array[i]);
>  	for (i = 0; i < cnt; i++) {
>  		struct commit_list *common;
> -		uint32_t min_generation = array[i]->generation;
> +		uint32_t min_generation = generation(array[i]);
>  
>  		if (redundant[i])
>  			continue;
> @@ -186,8 +186,8 @@ static int remove_redundant(struct repository *r, struct commit **array, int cnt
>  			filled_index[filled] = j;
>  			work[filled++] = array[j];
>  
> -			if (array[j]->generation < min_generation)
> -				min_generation = array[j]->generation;
> +			if (generation(array[j]) < min_generation)
> +				min_generation = generation(array[j]);
>  		}
>  		common = paint_down_to_common(r, array[i], filled,
>  					      work, min_generation);
> @@ -323,16 +323,16 @@ int repo_in_merge_bases_many(struct repository *r, struct commit *commit,
>  	for (i = 0; i < nr_reference; i++) {
>  		if (repo_parse_commit(r, reference[i]))
>  			return ret;
> -		if (reference[i]->generation < min_generation)
> -			min_generation = reference[i]->generation;
> +		if (generation(reference[i]) < min_generation)
> +			min_generation = generation(reference[i]);
>  	}
>  
> -	if (commit->generation > min_generation)
> +	if (generation(commit) > min_generation)
>  		return ret;
>  
>  	bases = paint_down_to_common(r, commit,
>  				     nr_reference, reference,
> -				     commit->generation);
> +				     generation(commit));
>  	if (commit->object.flags & PARENT2)
>  		ret = 1;
>  	clear_commit_marks(commit, all_flags);
> @@ -467,7 +467,7 @@ static enum contains_result contains_test(struct commit *candidate,
>  	/* Otherwise, we don't know; prepare to recurse */
>  	parse_commit_or_die(candidate);
>  
> -	if (candidate->generation < cutoff)
> +	if (generation(candidate) < cutoff)
>  		return CONTAINS_NO;
>  
>  	return CONTAINS_UNKNOWN;
> @@ -492,8 +492,8 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
>  	for (p = want; p; p = p->next) {
>  		struct commit *c = p->item;
>  		load_commit_graph_info(the_repository, c);
> -		if (c->generation < cutoff)
> -			cutoff = c->generation;
> +		if (generation(c) < cutoff)
> +			cutoff = generation(c);
>  	}
>  
>  	result = contains_test(candidate, want, cache, cutoff);
> @@ -544,9 +544,9 @@ static int compare_commits_by_gen(const void *_a, const void *_b)
>  	const struct commit *a = *(const struct commit * const *)_a;
>  	const struct commit *b = *(const struct commit * const *)_b;
>  
> -	if (a->generation < b->generation)
> +	if (generation(a) < generation(b))
>  		return -1;
> -	if (a->generation > b->generation)
> +	if (generation(a) > generation(b))
>  		return 1;
>  	return 0;
>  }
> @@ -585,7 +585,7 @@ int can_all_from_reach_with_flag(struct object_array *from,
>  
>  		list[nr_commits] = (struct commit *)from_one;
>  		if (parse_commit(list[nr_commits]) ||
> -		    list[nr_commits]->generation < min_generation) {
> +		    generation(list[nr_commits]) < min_generation) {
>  			result = 0;
>  			goto cleanup;
>  		}
> @@ -621,7 +621,7 @@ int can_all_from_reach_with_flag(struct object_array *from,
>  
>  					if (parse_commit(parent->item) ||
>  					    parent->item->date < min_commit_date ||
> -					    parent->item->generation < min_generation)
> +					    generation(parent->item) < min_generation)
>  						continue;
>  
>  					commit_list_insert(parent->item, &stack);
> @@ -665,8 +665,8 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to,
>  			if (from_iter->item->date < min_commit_date)
>  				min_commit_date = from_iter->item->date;
>  
> -			if (from_iter->item->generation < min_generation)
> -				min_generation = from_iter->item->generation;
> +			if (generation(from_iter->item) < min_generation)
> +				min_generation = generation(from_iter->item);
>  		}
>  
>  		from_iter = from_iter->next;
> @@ -677,8 +677,8 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to,
>  			if (to_iter->item->date < min_commit_date)
>  				min_commit_date = to_iter->item->date;
>  
> -			if (to_iter->item->generation < min_generation)
> -				min_generation = to_iter->item->generation;
> +			if (generation(to_iter->item) < min_generation)
> +				min_generation = generation(to_iter->item);
>  		}
>  
>  		to_iter->item->object.flags |= PARENT2;
> @@ -721,8 +721,8 @@ struct commit_list *get_reachable_subset(struct commit **from, int nr_from,
>  		struct commit *c = *item;
>  
>  		parse_commit(c);
> -		if (c->generation < min_generation)
> -			min_generation = c->generation;
> +		if (generation(c) < min_generation)
> +			min_generation = generation(c);
>  
>  		if (!(c->object.flags & PARENT1)) {
>  			c->object.flags |= PARENT1;
> @@ -755,7 +755,7 @@ struct commit_list *get_reachable_subset(struct commit **from, int nr_from,
>  
>  			parse_commit(p);
>  
> -			if (p->generation < min_generation)
> +			if (generation(p) < min_generation)
>  				continue;
>  
>  			if (p->object.flags & PARENT2)
> diff --git a/commit.c b/commit.c
> index 87686a7055..8dad0f8446 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -731,9 +731,9 @@ int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void
>  	const struct commit *a = a_, *b = b_;
>  
>  	/* newer commits first */
> -	if (a->generation < b->generation)
> +	if (generation(a) < generation(b))
>  		return 1;
> -	else if (a->generation > b->generation)
> +	else if (generation(a) > generation(b))
>  		return -1;
>  
>  	/* use date as a heuristic when generations are equal */
> diff --git a/revision.c b/revision.c
> index 60cca8c0b9..d76382007c 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -720,7 +720,7 @@ static int check_maybe_different_in_bloom_filter(struct rev_info *revs,
>  	if (!revs->repo->objects->commit_graph)
>  		return -1;
>  
> -	if (commit->generation == GENERATION_NUMBER_INFINITY)
> +	if (generation(commit) == GENERATION_NUMBER_INFINITY)
>  		return -1;
>  
>  	filter = get_bloom_filter(revs->repo, commit, 0);
> @@ -3314,7 +3314,7 @@ static void explore_to_depth(struct rev_info *revs,
>  	struct topo_walk_info *info = revs->topo_walk_info;
>  	struct commit *c;
>  	while ((c = prio_queue_peek(&info->explore_queue)) &&
> -	       c->generation >= gen_cutoff)
> +	       generation(c) >= gen_cutoff)
>  		explore_walk_step(revs);
>  }
>  
> @@ -3330,7 +3330,7 @@ static void indegree_walk_step(struct rev_info *revs)
>  	if (parse_commit_gently(c, 1) < 0)
>  		return;
>  
> -	explore_to_depth(revs, c->generation);
> +	explore_to_depth(revs, generation(c));
>  
>  	for (p = c->parents; p; p = p->next) {
>  		struct commit *parent = p->item;
> @@ -3354,7 +3354,7 @@ static void compute_indegrees_to_depth(struct rev_info *revs,
>  	struct topo_walk_info *info = revs->topo_walk_info;
>  	struct commit *c;
>  	while ((c = prio_queue_peek(&info->indegree_queue)) &&
> -	       c->generation >= gen_cutoff)
> +	       generation(c) >= gen_cutoff)
>  		indegree_walk_step(revs);
>  }
>  
> @@ -3414,8 +3414,8 @@ static void init_topo_walk(struct rev_info *revs)
>  		test_flag_and_insert(&info->explore_queue, c, TOPO_WALK_EXPLORED);
>  		test_flag_and_insert(&info->indegree_queue, c, TOPO_WALK_INDEGREE);
>  
> -		if (c->generation < info->min_generation)
> -			info->min_generation = c->generation;
> +		if (generation(c) < info->min_generation)
> +			info->min_generation = generation(c);
>  
>  		*(indegree_slab_at(&info->indegree, c)) = 1;
>  
> @@ -3473,8 +3473,8 @@ static void expand_topo_walk(struct rev_info *revs, struct commit *commit)
>  		if (parse_commit_gently(parent, 1) < 0)
>  			continue;
>  
> -		if (parent->generation < info->min_generation) {
> -			info->min_generation = parent->generation;
> +		if (generation(parent) < info->min_generation) {
> +			info->min_generation = generation(parent);
>  			compute_indegrees_to_depth(revs, info->min_generation);
>  		}

Best,
--
Jakub Narębski

Patch
diff mbox series

diff --git a/alloc.c b/alloc.c
index 1c64c4dd16..cbed187094 100644
--- a/alloc.c
+++ b/alloc.c
@@ -109,7 +109,6 @@  void init_commit_node(struct repository *r, struct commit *c)
 	c->object.type = OBJ_COMMIT;
 	c->index = alloc_commit_index(r);
 	c->graph_pos = COMMIT_NOT_FROM_GRAPH;
-	c->generation = GENERATION_NUMBER_INFINITY;
 }
 
 void *alloc_commit_node(struct repository *r)
diff --git a/blame.c b/blame.c
index da7e28800e..50e6316076 100644
--- a/blame.c
+++ b/blame.c
@@ -1272,7 +1272,7 @@  static int maybe_changed_path(struct repository *r,
 	if (!bd)
 		return 1;
 
-	if (origin->commit->generation == GENERATION_NUMBER_INFINITY)
+	if (generation(origin->commit) == GENERATION_NUMBER_INFINITY)
 		return 1;
 
 	filter = get_bloom_filter(r, origin->commit, 0);
diff --git a/commit-graph.c b/commit-graph.c
index 63f419048d..9ce7d4acb1 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -120,9 +120,9 @@  static int commit_gen_cmp(const void *va, const void *vb)
 	const struct commit *b = *(const struct commit **)vb;
 
 	/* lower generation commits first */
-	if (a->generation < b->generation)
+	if (generation(a) < generation(b))
 		return -1;
-	else if (a->generation > b->generation)
+	else if (generation(a) > generation(b))
 		return 1;
 
 	/* use date as a heuristic when generations are equal */
@@ -712,7 +712,7 @@  static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,
 	lex_index = pos - g->num_commits_in_base;
 	commit_data = g->chunk_commit_data + GRAPH_DATA_WIDTH * lex_index;
 	item->graph_pos = pos;
-	item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
+	set_generation(item, get_be32(commit_data + g->hash_len + 8) >> 2);
 }
 
 static inline void set_commit_tree(struct commit *c, struct tree *t)
@@ -754,7 +754,7 @@  static int fill_commit_in_graph(struct repository *r,
 	date_low = get_be32(commit_data + g->hash_len + 12);
 	item->date = (timestamp_t)((date_high << 32) | date_low);
 
-	item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
+	set_generation(item, get_be32(commit_data + g->hash_len + 8) >> 2);
 
 	pptr = &item->parents;
 
@@ -1048,7 +1048,7 @@  static void write_graph_chunk_data(struct hashfile *f, int hash_len,
 		else
 			packedDate[0] = 0;
 
-		packedDate[0] |= htonl((*list)->generation << 2);
+		packedDate[0] |= htonl(generation((*list)) << 2);
 
 		packedDate[1] = htonl((*list)->date);
 		hashwrite(f, packedDate, 8);
@@ -1280,8 +1280,8 @@  static void compute_generation_numbers(struct write_commit_graph_context *ctx)
 					ctx->commits.nr);
 	for (i = 0; i < ctx->commits.nr; i++) {
 		display_progress(ctx->progress, i + 1);
-		if (ctx->commits.list[i]->generation != GENERATION_NUMBER_INFINITY &&
-		    ctx->commits.list[i]->generation != GENERATION_NUMBER_ZERO)
+		if (generation(ctx->commits.list[i]) != GENERATION_NUMBER_INFINITY &&
+		    generation(ctx->commits.list[i]) != GENERATION_NUMBER_ZERO)
 			continue;
 
 		commit_list_insert(ctx->commits.list[i], &list);
@@ -1292,22 +1292,23 @@  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 (parent->item->generation == GENERATION_NUMBER_INFINITY ||
-				    parent->item->generation == GENERATION_NUMBER_ZERO) {
+				if (generation(parent->item) == GENERATION_NUMBER_INFINITY ||
+				    generation(parent->item) == GENERATION_NUMBER_ZERO) {
 					all_parents_computed = 0;
 					commit_list_insert(parent->item, &list);
 					break;
-				} else if (parent->item->generation > max_generation) {
-					max_generation = parent->item->generation;
+				} else if (generation(parent->item) > max_generation) {
+					max_generation = generation(parent->item);
 				}
 			}
 
 			if (all_parents_computed) {
-				current->generation = max_generation + 1;
+				set_generation(current, max_generation + 1);
 				pop_commit(&list);
 
-				if (current->generation > GENERATION_NUMBER_MAX)
-					current->generation = GENERATION_NUMBER_MAX;
+				if (generation(current) > GENERATION_NUMBER_MAX)
+					set_generation(current,
+						       GENERATION_NUMBER_MAX);
 			}
 		}
 	}
@@ -2314,8 +2315,8 @@  int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)
 					     oid_to_hex(&graph_parents->item->object.oid),
 					     oid_to_hex(&odb_parents->item->object.oid));
 
-			if (graph_parents->item->generation > max_generation)
-				max_generation = graph_parents->item->generation;
+			if (generation(graph_parents->item) > max_generation)
+				max_generation = generation(graph_parents->item);
 
 			graph_parents = graph_parents->next;
 			odb_parents = odb_parents->next;
@@ -2325,7 +2326,7 @@  int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)
 			graph_report(_("commit-graph parent list for commit %s terminates early"),
 				     oid_to_hex(&cur_oid));
 
-		if (!graph_commit->generation) {
+		if (!generation(graph_commit)) {
 			if (generation_zero == GENERATION_NUMBER_EXISTS)
 				graph_report(_("commit-graph has generation number zero for commit %s, but non-zero elsewhere"),
 					     oid_to_hex(&cur_oid));
@@ -2345,10 +2346,10 @@  int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)
 		if (max_generation == GENERATION_NUMBER_MAX)
 			max_generation--;
 
-		if (graph_commit->generation != max_generation + 1)
+		if (generation(graph_commit) != max_generation + 1)
 			graph_report(_("commit-graph generation for commit %s is %u != %u"),
 				     oid_to_hex(&cur_oid),
-				     graph_commit->generation,
+				     generation(graph_commit),
 				     max_generation + 1);
 
 		if (graph_commit->date != odb_commit->date)
diff --git a/commit-reach.c b/commit-reach.c
index 4ca7e706a1..77c980054a 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -59,13 +59,13 @@  static struct commit_list *paint_down_to_common(struct repository *r,
 		struct commit_list *parents;
 		int flags;
 
-		if (min_generation && commit->generation > last_gen)
+		if (min_generation && generation(commit) > last_gen)
 			BUG("bad generation skip %8x > %8x at %s",
-			    commit->generation, last_gen,
+			    generation(commit), last_gen,
 			    oid_to_hex(&commit->object.oid));
-		last_gen = commit->generation;
+		last_gen = generation(commit);
 
-		if (commit->generation < min_generation)
+		if (generation(commit) < min_generation)
 			break;
 
 		flags = commit->object.flags & (PARENT1 | PARENT2 | STALE);
@@ -176,7 +176,7 @@  static int remove_redundant(struct repository *r, struct commit **array, int cnt
 		repo_parse_commit(r, array[i]);
 	for (i = 0; i < cnt; i++) {
 		struct commit_list *common;
-		uint32_t min_generation = array[i]->generation;
+		uint32_t min_generation = generation(array[i]);
 
 		if (redundant[i])
 			continue;
@@ -186,8 +186,8 @@  static int remove_redundant(struct repository *r, struct commit **array, int cnt
 			filled_index[filled] = j;
 			work[filled++] = array[j];
 
-			if (array[j]->generation < min_generation)
-				min_generation = array[j]->generation;
+			if (generation(array[j]) < min_generation)
+				min_generation = generation(array[j]);
 		}
 		common = paint_down_to_common(r, array[i], filled,
 					      work, min_generation);
@@ -323,16 +323,16 @@  int repo_in_merge_bases_many(struct repository *r, struct commit *commit,
 	for (i = 0; i < nr_reference; i++) {
 		if (repo_parse_commit(r, reference[i]))
 			return ret;
-		if (reference[i]->generation < min_generation)
-			min_generation = reference[i]->generation;
+		if (generation(reference[i]) < min_generation)
+			min_generation = generation(reference[i]);
 	}
 
-	if (commit->generation > min_generation)
+	if (generation(commit) > min_generation)
 		return ret;
 
 	bases = paint_down_to_common(r, commit,
 				     nr_reference, reference,
-				     commit->generation);
+				     generation(commit));
 	if (commit->object.flags & PARENT2)
 		ret = 1;
 	clear_commit_marks(commit, all_flags);
@@ -467,7 +467,7 @@  static enum contains_result contains_test(struct commit *candidate,
 	/* Otherwise, we don't know; prepare to recurse */
 	parse_commit_or_die(candidate);
 
-	if (candidate->generation < cutoff)
+	if (generation(candidate) < cutoff)
 		return CONTAINS_NO;
 
 	return CONTAINS_UNKNOWN;
@@ -492,8 +492,8 @@  static enum contains_result contains_tag_algo(struct commit *candidate,
 	for (p = want; p; p = p->next) {
 		struct commit *c = p->item;
 		load_commit_graph_info(the_repository, c);
-		if (c->generation < cutoff)
-			cutoff = c->generation;
+		if (generation(c) < cutoff)
+			cutoff = generation(c);
 	}
 
 	result = contains_test(candidate, want, cache, cutoff);
@@ -544,9 +544,9 @@  static int compare_commits_by_gen(const void *_a, const void *_b)
 	const struct commit *a = *(const struct commit * const *)_a;
 	const struct commit *b = *(const struct commit * const *)_b;
 
-	if (a->generation < b->generation)
+	if (generation(a) < generation(b))
 		return -1;
-	if (a->generation > b->generation)
+	if (generation(a) > generation(b))
 		return 1;
 	return 0;
 }
@@ -585,7 +585,7 @@  int can_all_from_reach_with_flag(struct object_array *from,
 
 		list[nr_commits] = (struct commit *)from_one;
 		if (parse_commit(list[nr_commits]) ||
-		    list[nr_commits]->generation < min_generation) {
+		    generation(list[nr_commits]) < min_generation) {
 			result = 0;
 			goto cleanup;
 		}
@@ -621,7 +621,7 @@  int can_all_from_reach_with_flag(struct object_array *from,
 
 					if (parse_commit(parent->item) ||
 					    parent->item->date < min_commit_date ||
-					    parent->item->generation < min_generation)
+					    generation(parent->item) < min_generation)
 						continue;
 
 					commit_list_insert(parent->item, &stack);
@@ -665,8 +665,8 @@  int can_all_from_reach(struct commit_list *from, struct commit_list *to,
 			if (from_iter->item->date < min_commit_date)
 				min_commit_date = from_iter->item->date;
 
-			if (from_iter->item->generation < min_generation)
-				min_generation = from_iter->item->generation;
+			if (generation(from_iter->item) < min_generation)
+				min_generation = generation(from_iter->item);
 		}
 
 		from_iter = from_iter->next;
@@ -677,8 +677,8 @@  int can_all_from_reach(struct commit_list *from, struct commit_list *to,
 			if (to_iter->item->date < min_commit_date)
 				min_commit_date = to_iter->item->date;
 
-			if (to_iter->item->generation < min_generation)
-				min_generation = to_iter->item->generation;
+			if (generation(to_iter->item) < min_generation)
+				min_generation = generation(to_iter->item);
 		}
 
 		to_iter->item->object.flags |= PARENT2;
@@ -721,8 +721,8 @@  struct commit_list *get_reachable_subset(struct commit **from, int nr_from,
 		struct commit *c = *item;
 
 		parse_commit(c);
-		if (c->generation < min_generation)
-			min_generation = c->generation;
+		if (generation(c) < min_generation)
+			min_generation = generation(c);
 
 		if (!(c->object.flags & PARENT1)) {
 			c->object.flags |= PARENT1;
@@ -755,7 +755,7 @@  struct commit_list *get_reachable_subset(struct commit **from, int nr_from,
 
 			parse_commit(p);
 
-			if (p->generation < min_generation)
+			if (generation(p) < min_generation)
 				continue;
 
 			if (p->object.flags & PARENT2)
diff --git a/commit.c b/commit.c
index 87686a7055..8dad0f8446 100644
--- a/commit.c
+++ b/commit.c
@@ -731,9 +731,9 @@  int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void
 	const struct commit *a = a_, *b = b_;
 
 	/* newer commits first */
-	if (a->generation < b->generation)
+	if (generation(a) < generation(b))
 		return 1;
-	else if (a->generation > b->generation)
+	else if (generation(a) > generation(b))
 		return -1;
 
 	/* use date as a heuristic when generations are equal */
diff --git a/commit.h b/commit.h
index cc610400d5..01e1c4c3eb 100644
--- a/commit.h
+++ b/commit.h
@@ -34,7 +34,6 @@  struct commit {
 	 */
 	struct tree *maybe_tree;
 	uint32_t graph_pos;
-	uint32_t generation;
 	unsigned int index;
 };
 
diff --git a/contrib/coccinelle/generation.cocci b/contrib/coccinelle/generation.cocci
new file mode 100644
index 0000000000..da13c44856
--- /dev/null
+++ b/contrib/coccinelle/generation.cocci
@@ -0,0 +1,12 @@ 
+@@
+struct commit *c;
+expression E;
+@@
+- c->generation = E
++ set_generation(c, E)
+
+@@
+struct commit *c;
+@@
+- c->generation
++ generation(c)
diff --git a/revision.c b/revision.c
index 60cca8c0b9..d76382007c 100644
--- a/revision.c
+++ b/revision.c
@@ -720,7 +720,7 @@  static int check_maybe_different_in_bloom_filter(struct rev_info *revs,
 	if (!revs->repo->objects->commit_graph)
 		return -1;
 
-	if (commit->generation == GENERATION_NUMBER_INFINITY)
+	if (generation(commit) == GENERATION_NUMBER_INFINITY)
 		return -1;
 
 	filter = get_bloom_filter(revs->repo, commit, 0);
@@ -3314,7 +3314,7 @@  static void explore_to_depth(struct rev_info *revs,
 	struct topo_walk_info *info = revs->topo_walk_info;
 	struct commit *c;
 	while ((c = prio_queue_peek(&info->explore_queue)) &&
-	       c->generation >= gen_cutoff)
+	       generation(c) >= gen_cutoff)
 		explore_walk_step(revs);
 }
 
@@ -3330,7 +3330,7 @@  static void indegree_walk_step(struct rev_info *revs)
 	if (parse_commit_gently(c, 1) < 0)
 		return;
 
-	explore_to_depth(revs, c->generation);
+	explore_to_depth(revs, generation(c));
 
 	for (p = c->parents; p; p = p->next) {
 		struct commit *parent = p->item;
@@ -3354,7 +3354,7 @@  static void compute_indegrees_to_depth(struct rev_info *revs,
 	struct topo_walk_info *info = revs->topo_walk_info;
 	struct commit *c;
 	while ((c = prio_queue_peek(&info->indegree_queue)) &&
-	       c->generation >= gen_cutoff)
+	       generation(c) >= gen_cutoff)
 		indegree_walk_step(revs);
 }
 
@@ -3414,8 +3414,8 @@  static void init_topo_walk(struct rev_info *revs)
 		test_flag_and_insert(&info->explore_queue, c, TOPO_WALK_EXPLORED);
 		test_flag_and_insert(&info->indegree_queue, c, TOPO_WALK_INDEGREE);
 
-		if (c->generation < info->min_generation)
-			info->min_generation = c->generation;
+		if (generation(c) < info->min_generation)
+			info->min_generation = generation(c);
 
 		*(indegree_slab_at(&info->indegree, c)) = 1;
 
@@ -3473,8 +3473,8 @@  static void expand_topo_walk(struct rev_info *revs, struct commit *commit)
 		if (parse_commit_gently(parent, 1) < 0)
 			continue;
 
-		if (parent->generation < info->min_generation) {
-			info->min_generation = parent->generation;
+		if (generation(parent) < info->min_generation) {
+			info->min_generation = generation(parent);
 			compute_indegrees_to_depth(revs, info->min_generation);
 		}