Subject: Re: [PATCH v3 02/17] chunk-format: create chunk format write API

From: SZEDER Gábor <[email-masked]>
Date: Sun, 7 Feb 2021 22:13:59 +0100

> diff --git a/chunk-format.c b/chunk-format.c
> new file mode 100644
> index 000000000000..6e0f1900213e
> --- /dev/null
> +++ b/chunk-format.c
> @@ -0,0 +1,91 @@
> +#include "cache.h"
> +#include "chunk-format.h"
> +#include "csum-file.h"
> +#define CHUNK_LOOKUP_WIDTH 12

As this macro is defined in 'chunk-format.c' it's not part of the
chunkfile API.  However, at the end of this patch series
'commit-graph.c' still contains:

  #define GRAPH_CHUNKLOOKUP_WIDTH 12

and uses it in a couple of safety checks (that didn't became part of
the common chunkfile module; why?), while 'midx.c' contains:

  #define MIDX_CHUNKLOOKUP_WIDTH (sizeof(uint32_t) + sizeof(uint64_t))

though it's not used anymore.

I think we should have only one such constant as part of the chunkfile
API; and preferably use the definition from 'midx.c' as it is more
informative than yet another magic number.

Furthermore, being called 'CHUNK_LOOKUP_WIDTH', I had to look up the
places where this constant is used to make sure that it indeed means
what I suspect it means.  Perhaps CHUNK_TOC_ENTRY_SIZE would be a more
descriptive name for this constant.

On a somewhat related note: 'commit-graph.c' and 'midx.c' still
contains the constants MAX_NUM_CHUNKS and MIDX_MAX_CHUNKS,
respecticely, but neither of them is used anymore.