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

> On 2/7/2021 4:13 PM, SZEDER Gábor wrote:
> >> +#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?),
> 
> Chunk-based files don't have a minimum size unless we know the header
> size and a minimum number of required chunks. I suppose that we could
> add this in the future to further simplify consumers of the API.
> 
> > 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.
> 
> More descriptive, for sure.
> 
> > 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.
> 
> Thanks. The following patch can be added on top of this series
> to clean up these dangling macros.

> Thanks,
> -Stolee
> 
> --- >8 ---
> 
> From 839b880ccee65eac63e8b77b12fab6531acc55b0 Mon Sep 17 00:00:00 2001
> From: Derrick Stolee <dstolee@microsoft.com>
> Date: Mon, 8 Feb 2021 08:38:47 -0500
> Subject: [PATCH] chunk-format: remove outdated macro constants
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The following macros were needed by midx.c and commit-graph.c to handle
> their independent implementations of the chunk-based file format, but
> now the chunk-format API makes them obsolete:
> 
> * MAX_NUM_CHUNKS
> * MIDX_MAX_CHUNKS
> * MIX_CHUNKLOOKUP_WIDTH
> 
> Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  commit-graph.c | 1 -
>  midx.c         | 2 --
>  2 files changed, 3 deletions(-)
> 
> diff --git a/commit-graph.c b/commit-graph.c
> index 32cf5091d2f..3b5a8767269 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -45,7 +45,6 @@ void git_test_write_commit_graph_or_die(void)
>  #define GRAPH_CHUNKID_BLOOMINDEXES 0x42494458 /* "BIDX" */
>  #define GRAPH_CHUNKID_BLOOMDATA 0x42444154 /* "BDAT" */
>  #define GRAPH_CHUNKID_BASE 0x42415345 /* "BASE" */
> -#define MAX_NUM_CHUNKS 9
>  
>  #define GRAPH_DATA_WIDTH (the_hash_algo->rawsz + 16)
>  
> diff --git a/midx.c b/midx.c
> index 95648a1f368..5c7f2ed2333 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -22,14 +22,12 @@
>  #define MIDX_HEADER_SIZE 12
>  #define MIDX_MIN_SIZE (MIDX_HEADER_SIZE + the_hash_algo->rawsz)
>  
> -#define MIDX_MAX_CHUNKS 5
>  #define MIDX_CHUNK_ALIGNMENT 4
>  #define MIDX_CHUNKID_PACKNAMES 0x504e414d /* "PNAM" */
>  #define MIDX_CHUNKID_OIDFANOUT 0x4f494446 /* "OIDF" */
>  #define MIDX_CHUNKID_OIDLOOKUP 0x4f49444c /* "OIDL" */
>  #define MIDX_CHUNKID_OBJECTOFFSETS 0x4f4f4646 /* "OOFF" */
>  #define MIDX_CHUNKID_LARGEOFFSETS 0x4c4f4646 /* "LOFF" */
> -#define MIDX_CHUNKLOOKUP_WIDTH (sizeof(uint32_t) + sizeof(uint64_t))
>  #define MIDX_CHUNK_FANOUT_SIZE (sizeof(uint32_t) * 256)
>  #define MIDX_CHUNK_OFFSET_WIDTH (2 * sizeof(uint32_t))
>  #define MIDX_CHUNK_LARGE_OFFSET_WIDTH (sizeof(uint64_t))
> -- 
> 2.30.0.vfs.0.0.exp
>