Re: [PATCH 5/5] commit-graph-format.txt: fix "Hash Version" description

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 14, 2020, 12:37 UTC
Message-ID: <e216492c-d11a-6733-db64-720d027d3d42@gmail.com>
In-Reply-To: <bbcd97db8a2c94343950bb0cf8cbd5c9c01b4577.1597406877.git.martin.agren@gmail.com>

On 8/14/2020 8:21 AM, Martin Ågren wrote:

Show 17 quoted lines

> We say that value 1 means "SHA-1", but in fact, it means "whatever
> the_hash_algo is", see commit c166599862 ("commit-graph: convert to
> using the_hash_algo", 2018-11-14).
> 
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  If we want to be more fine-grained in the future, we'll need to say,
>  e.g., "2 means SHA-1, 3 means SHA-256" or, perhaps preferrably, bump the
>  version number.
> 
>  I wonder: Should we instead say "1 means SHA-1, 2 means SHA-256"? It
>  could be implemented as "easily" as "if (value_from_header !=
>  value_from_the_hash_algo) die(...);" for now. Might that pay off in the
>  long run?
> 
>  This relates to Stolee's "in a vacuum" comment [1] ... so maybe we're
>  fine.

I think that was the intention of the byte, but that is not what ended up happening. If we want that to be the case, then we should do that work as part of the 2.29 cycle before we release with the ability to create SHA-256 repos (which will lock the commit-graph format for these repos).

(By "we" I mean that I would try to do this work in a way that minimizes conflicts with the current commit-graph work in flight [1] [2].)

[1] https://lore.kernel.org/git/pull.676.v2.git.1596941624.gitgitgadget@gmail.com/

[2] https://lore.kernel.org/git/cover.1597178914.git.me@ttaylorr.com/

Show 17 quoted lines

>  [1] https://lore.kernel.org/git/da077fb0-14bb-b84f-c526-d759ebc9f5eb@gmail.com/
> 
>  Documentation/technical/commit-graph-format.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/technical/commit-graph-format.txt b/Documentation/technical/commit-graph-format.txt
> index 440541045d..3535426d32 100644
> --- a/Documentation/technical/commit-graph-format.txt
> +++ b/Documentation/technical/commit-graph-format.txt
> @@ -42,8 +42,8 @@ HEADER:
>    1-byte version number:
>        Currently, the only valid version is 1.
>  
> -  1-byte Hash Version (1 = SHA-1)
> -      We infer the hash length (H) from this value.
> +  1-byte Hash Version (1 = SHA-1 in SHA-1 repo, SHA-256 in SHA-256 repo)
> +      We infer the hash length (H) from the hash algo derived from this value.

If we are _not_ changing the format to have a meaningful value in this byte, then this documentation should be updated to state that this byte must always have value 1, as it does not provide any information.

Thanks, -Stolee