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

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

quoted

+#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 <redacted>
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 <redacted>
Signed-off-by: Derrick Stolee <redacted>
---
 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