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

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Mon, Feb 08, 2021 at 08:44:06AM -0500, Derrick Stolee wrote:
> 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.

It would be better to squash this into the patches that removed the
last uses of each of those constants.

And it still leaves the magic number '12' duplicated in
'commit-graph.c' and 'chunk-format.c'.

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