Re: [PATCH v3 02/20] commit-graph: fix GRAPH_MIN_SIZE

From: Jakub Narebski <jnareb@gmail.com>
Date: May 26, 2018, 18:46 UTC
Message-ID: <868t861ci6.fsf@gmail.com>
In-Reply-To: <20180524162504.158394-3-dstolee@microsoft.com>

Derrick Stolee <dstolee@microsoft.com> writes:

Show 22 quoted lines

> The GRAPH_MIN_SIZE macro should be the smallest size of a parsable
> commit-graph file. However, the minimum number of chunks was wrong.
> It is possible to write a commit-graph file with zero commits, and
> that violates this macro's value.
>
> Rewrite the macro, and use extra macros to better explain the magic
> constants.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  commit-graph.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index a8c337dd77..82295f0975 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -33,10 +33,11 @@
>  
>  #define GRAPH_LAST_EDGE 0x80000000
>  
> +#define GRAPH_HEADER_SIZE 8

Nice.

Show 6 quoted lines

>  #define GRAPH_FANOUT_SIZE (4 * 256)
>  #define GRAPH_CHUNKLOOKUP_WIDTH 12
> -#define GRAPH_MIN_SIZE (5 * GRAPH_CHUNKLOOKUP_WIDTH + GRAPH_FANOUT_SIZE + \
> -			GRAPH_OID_LEN + 8)
> +#define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \
> +			+ GRAPH_FANOUT_SIZE + GRAPH_OID_LEN)

So in this case we have file header (4-byte signature, 1-byte version number, 1-byte oid/hash version, 1-byte number of chunks, 1-byte reserved: 4+1+1+1+1 = 8 bytes), chunk lookup: 3 required chunks plus terminating label = 4 entries, constant-size fanout chunks, and checksum. Two remaining required chunks (OID Lookup and Commit Data) can have length of 0.

One issue: in the future when Git moves to NewHash, it could encounter then both commit-graph files using SHA-1 and using NewHash. What about GRPH_OID_LEN then: for one of those it would be incorrect. Unless it is about minimal length of checksum, that is we assume that NewHash would be longer than SHA-1, but ten why name it GRAPH_OID_LEN?

This may be going too much in the future; there is no need to borrow trouble now, where we have only SHA-1 supported as OID. Still...

>  
>  char *get_commit_graph_filename(const char *obj_dir)
>  {

Best,

-- 
Jakub Narębski