Re: [PATCH 3/9] commit-graph: use MAX_NUM_CHUNKS

From: Jakub Narebski <jnareb@gmail.com>
Date: Jan 7, 2020, 12:19 UTC
Message-ID: <865zhnzcg5.fsf@gmail.com>
In-Reply-To: <a15f87fdcbea1a37a20a05135832b42f36f682f1.1576879520.git.gitgitgadget@gmail.com>

"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Garima Singh <garima.singh@microsoft.com>
>
> This is a minor cleanup to make it easier to change the
> number of chunks being written to the commit-graph in the future.

Very minor nit: in the whole commit message it is not stated explicitly what MAX_NUM_CHUNKS is for, though it is very easy to guess (from the name itself).

Show 15 quoted lines

>
> Signed-off-by: Garima Singh <garima.singh@microsoft.com>
> ---
>  commit-graph.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 61e60ff98a..8c4941eeaa 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -24,6 +24,7 @@
>  #define GRAPH_CHUNKID_DATA 0x43444154 /* "CDAT" */
>  #define GRAPH_CHUNKID_EXTRAEDGES 0x45444745 /* "EDGE" */
>  #define GRAPH_CHUNKID_BASE 0x42415345 /* "BASE" */
> +#define MAX_NUM_CHUNKS 5

Minor nit: MAX_NUM_CHUNKS or MAX_CHUNKS?

Show 11 quoted lines

>  
>  #define GRAPH_DATA_WIDTH (the_hash_algo->rawsz + 16)
>  
> @@ -1381,8 +1382,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
>  	int fd;
>  	struct hashfile *f;
>  	struct lock_file lk = LOCK_INIT;
> -	uint32_t chunk_ids[6];
> -	uint64_t chunk_offsets[6];
> +	uint32_t chunk_ids[MAX_NUM_CHUNKS + 1];
> +	uint64_t chunk_offsets[MAX_NUM_CHUNKS + 1];

Looks good. I guess we won't ever have more chunks than 5: OIDF, OIDL, CDAT, EDGE, BASE (and they cannot repeat, and last two are optional).

>  	const unsigned hashsz = the_hash_algo->rawsz;
>  	struct strbuf progress_title = STRBUF_INIT;
>  	int num_chunks = 3;

Good.

Looks good to me.

-- 
Jakub Narębski