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

From: Derrick Stolee <stolee@gmail.com>
Date: Feb 8, 2021, 13:44 UTC
Message-ID: <96d4f87d-eb6e-faec-1d22-5f6ae307b624@gmail.com>
In-Reply-To: <20210207211359.GD1015009@szeder.dev>

On 2/7/2021 4:13 PM, SZEDER Gábor wrote:

Show 10 quoted lines

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

Show 14 quoted lines

> 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