Re: [PATCH 1/2] commit-graph: rename 'num_extra_edges' variable to 'num_large_edges'
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 21, 2018, 03:29 UTC
- Message-ID
- <xmqqwop785wh.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20181121012532.26878-1-szeder.dev@gmail.com>
SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 13 quoted lines
> 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/;
Show 84 quoted lines
> 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];