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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Feb 7, 2021, 21:13 UTC
Message-ID: <20210207211359.GD1015009@szeder.dev>
In-Reply-To: <16c37d2370cf4fd5fc309ac6dc8aa6443ffcf3d7.1612535453.git.gitgitgadget@gmail.com>

Show 10 quoted lines

> 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.