[1/2] commit-graph: rename 'num_extra_edges' variable to 'num_large_edges'
diff mbox series

Message ID 20181121012532.26878-1-szeder.dev@gmail.com
State New
Headers show
Series
  • [1/2] commit-graph: rename 'num_extra_edges' variable to 'num_large_edges'
Related show

Commit Message

SZEDER Gábor Nov. 21, 2018, 1:25 a.m. UTC
The commit graph file format describes an optional 'Large Edge List'
chunk, and the function writing out this chunk is called
write_graph_chunk_large_edges().  Then there are two functions in
'commit-graph.c', namely write_graph_chunk_data() and
write_commit_graph(), which have a local variable called
'num_extra_edges'.

It can be confusing on first sight whether large edges and extra edges
refer to the same thing or not, but they do, so let's rename those
variables to 'num_large_edges'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

I rename these variables to 'num_large_edges', because the commit
graph file format speaks about the 'Large Edge List' chunk.

However, I do find that the term 'extra' makes much more sense and
fits the concept better (i.e. extra commit graph edges resulting from
the extra parents or octopus merges; after a s/extra/large/g the
previous phrase would make no sense), and notice that the term 'large'
doesn't come up in the file format itseld (the chunk's magic is {'E',
'D', 'G', 'E'}, there is no 'L' in there), but only in the
specification text and a couple of variable and function names in the
code.

Would it make sense to do the rename in the other direction?

 commit-graph.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Junio C Hamano Nov. 21, 2018, 3:29 a.m. UTC | #1
SZEDER Gábor <szeder.dev@gmail.com> writes:

> I rename these variables to 'num_large_edges', because the commit
> graph file format speaks about the 'Large Edge List' chunk.
>
> However, I do find that the term 'extra' makes much more sense and
> fits the concept better (i.e. extra commit graph edges resulting from
> the extra parents or octopus merges; after a s/extra/large/g the
> previous phrase would make no sense), and notice that the term 'large'
> doesn't come up in the file format itseld (the chunk's magic is {'E',
> 'D', 'G', 'E'}, there is no 'L' in there), but only in the
> specification text and a couple of variable and function names in the
> code.
>
> Would it make sense to do the rename in the other direction?

So edges that are involved in octopus merges are counted with
num_extra_edges and written to the large edges table?

I tend to agree that "large edge" is a misnomer.  These edges that
point at third and subsequent parents are no larger than the edges
that point at the first or the second parents---they are the same
size.  What is larger than usual is the size of the list of edges
(i.e. the number of parents), because the commit has extra (compared
to the majority of commits) number of edges.  So from the point of
view, I agree with you that "extra" makes a lot more sense than
"large".

And the magic number being "EDGE" without "L" is probably a good
thing, as a graph whose commits are all without any extra edge does
not need the "EDGE" chunk, so presence of the chunk by itself is a
sign that extra things are involved.  Which means that there isn't
any need to update the magic number, if we wanted to get rid of
"large" and replace it with "extra".  The only thing needed to
update the documentation, variable names and in-code comment.

And while at it, GRAPH_OCTOPUS_EDGES_NEEDED may also want to be
renamed with s/OCTOPUS/EXTRA/;

>  commit-graph.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 40c855f185..7b4e3a02cf 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -475,7 +475,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
>  {
>  	struct commit **list = commits;
>  	struct commit **last = commits + nr_commits;
> -	uint32_t num_extra_edges = 0;
> +	uint32_t num_large_edges = 0;
>  
>  	while (list < last) {
>  		struct commit_list *parent;
> @@ -507,7 +507,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
>  		if (!parent)
>  			edge_value = GRAPH_PARENT_NONE;
>  		else if (parent->next)
> -			edge_value = GRAPH_OCTOPUS_EDGES_NEEDED | num_extra_edges;
> +			edge_value = GRAPH_OCTOPUS_EDGES_NEEDED | num_large_edges;
>  		else {
>  			edge_value = sha1_pos(parent->item->object.oid.hash,
>  					      commits,
> @@ -521,7 +521,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
>  
>  		if (edge_value & GRAPH_OCTOPUS_EDGES_NEEDED) {
>  			do {
> -				num_extra_edges++;
> +				num_large_edges++;
>  				parent = parent->next;
>  			} while (parent);
>  		}
> @@ -761,7 +761,7 @@ void write_commit_graph(const char *obj_dir,
>  	uint32_t chunk_ids[5];
>  	uint64_t chunk_offsets[5];
>  	int num_chunks;
> -	int num_extra_edges;
> +	int num_large_edges;
>  	struct commit_list *parent;
>  	struct progress *progress = NULL;
>  
> @@ -871,7 +871,7 @@ void write_commit_graph(const char *obj_dir,
>  	commits.alloc = count_distinct;
>  	ALLOC_ARRAY(commits.list, commits.alloc);
>  
> -	num_extra_edges = 0;
> +	num_large_edges = 0;
>  	for (i = 0; i < oids.nr; i++) {
>  		int num_parents = 0;
>  		if (i > 0 && oideq(&oids.list[i - 1], &oids.list[i]))
> @@ -885,11 +885,11 @@ void write_commit_graph(const char *obj_dir,
>  			num_parents++;
>  
>  		if (num_parents > 2)
> -			num_extra_edges += num_parents - 1;
> +			num_large_edges += num_parents - 1;
>  
>  		commits.nr++;
>  	}
> -	num_chunks = num_extra_edges ? 4 : 3;
> +	num_chunks = num_large_edges ? 4 : 3;
>  
>  	if (commits.nr >= GRAPH_PARENT_MISSING)
>  		die(_("too many commits to write graph"));
> @@ -916,7 +916,7 @@ void write_commit_graph(const char *obj_dir,
>  	chunk_ids[0] = GRAPH_CHUNKID_OIDFANOUT;
>  	chunk_ids[1] = GRAPH_CHUNKID_OIDLOOKUP;
>  	chunk_ids[2] = GRAPH_CHUNKID_DATA;
> -	if (num_extra_edges)
> +	if (num_large_edges)
>  		chunk_ids[3] = GRAPH_CHUNKID_LARGEEDGES;
>  	else
>  		chunk_ids[3] = 0;
> @@ -926,7 +926,7 @@ void write_commit_graph(const char *obj_dir,
>  	chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
>  	chunk_offsets[2] = chunk_offsets[1] + GRAPH_OID_LEN * commits.nr;
>  	chunk_offsets[3] = chunk_offsets[2] + (GRAPH_OID_LEN + 16) * commits.nr;
> -	chunk_offsets[4] = chunk_offsets[3] + 4 * num_extra_edges;
> +	chunk_offsets[4] = chunk_offsets[3] + 4 * num_large_edges;
>  
>  	for (i = 0; i <= num_chunks; i++) {
>  		uint32_t chunk_write[3];
Derrick Stolee Nov. 21, 2018, 11:32 a.m. UTC | #2
On 11/20/2018 10:29 PM, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> I rename these variables to 'num_large_edges', because the commit
>> graph file format speaks about the 'Large Edge List' chunk.
>>
>> However, I do find that the term 'extra' makes much more sense
>>
>> Would it make sense to do the rename in the other direction?
> I tend to agree that "large edge" is a misnomer.

I agree with you both. "Extra" is better.

Thanks,

-Stolee

Patch
diff mbox series

diff --git a/commit-graph.c b/commit-graph.c
index 40c855f185..7b4e3a02cf 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -475,7 +475,7 @@  static void write_graph_chunk_data(struct hashfile *f, int hash_len,
 {
 	struct commit **list = commits;
 	struct commit **last = commits + nr_commits;
-	uint32_t num_extra_edges = 0;
+	uint32_t num_large_edges = 0;
 
 	while (list < last) {
 		struct commit_list *parent;
@@ -507,7 +507,7 @@  static void write_graph_chunk_data(struct hashfile *f, int hash_len,
 		if (!parent)
 			edge_value = GRAPH_PARENT_NONE;
 		else if (parent->next)
-			edge_value = GRAPH_OCTOPUS_EDGES_NEEDED | num_extra_edges;
+			edge_value = GRAPH_OCTOPUS_EDGES_NEEDED | num_large_edges;
 		else {
 			edge_value = sha1_pos(parent->item->object.oid.hash,
 					      commits,
@@ -521,7 +521,7 @@  static void write_graph_chunk_data(struct hashfile *f, int hash_len,
 
 		if (edge_value & GRAPH_OCTOPUS_EDGES_NEEDED) {
 			do {
-				num_extra_edges++;
+				num_large_edges++;
 				parent = parent->next;
 			} while (parent);
 		}
@@ -761,7 +761,7 @@  void write_commit_graph(const char *obj_dir,
 	uint32_t chunk_ids[5];
 	uint64_t chunk_offsets[5];
 	int num_chunks;
-	int num_extra_edges;
+	int num_large_edges;
 	struct commit_list *parent;
 	struct progress *progress = NULL;
 
@@ -871,7 +871,7 @@  void write_commit_graph(const char *obj_dir,
 	commits.alloc = count_distinct;
 	ALLOC_ARRAY(commits.list, commits.alloc);
 
-	num_extra_edges = 0;
+	num_large_edges = 0;
 	for (i = 0; i < oids.nr; i++) {
 		int num_parents = 0;
 		if (i > 0 && oideq(&oids.list[i - 1], &oids.list[i]))
@@ -885,11 +885,11 @@  void write_commit_graph(const char *obj_dir,
 			num_parents++;
 
 		if (num_parents > 2)
-			num_extra_edges += num_parents - 1;
+			num_large_edges += num_parents - 1;
 
 		commits.nr++;
 	}
-	num_chunks = num_extra_edges ? 4 : 3;
+	num_chunks = num_large_edges ? 4 : 3;
 
 	if (commits.nr >= GRAPH_PARENT_MISSING)
 		die(_("too many commits to write graph"));
@@ -916,7 +916,7 @@  void write_commit_graph(const char *obj_dir,
 	chunk_ids[0] = GRAPH_CHUNKID_OIDFANOUT;
 	chunk_ids[1] = GRAPH_CHUNKID_OIDLOOKUP;
 	chunk_ids[2] = GRAPH_CHUNKID_DATA;
-	if (num_extra_edges)
+	if (num_large_edges)
 		chunk_ids[3] = GRAPH_CHUNKID_LARGEEDGES;
 	else
 		chunk_ids[3] = 0;
@@ -926,7 +926,7 @@  void write_commit_graph(const char *obj_dir,
 	chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
 	chunk_offsets[2] = chunk_offsets[1] + GRAPH_OID_LEN * commits.nr;
 	chunk_offsets[3] = chunk_offsets[2] + (GRAPH_OID_LEN + 16) * commits.nr;
-	chunk_offsets[4] = chunk_offsets[3] + 4 * num_extra_edges;
+	chunk_offsets[4] = chunk_offsets[3] + 4 * num_large_edges;
 
 	for (i = 0; i <= num_chunks; i++) {
 		uint32_t chunk_write[3];