SHA-256: Update commit-graph and multi-pack-index formats

As discussed [1], there is some concern around binary file formats requiring the context of the repository config in order to infer hash lengths. Two formats that were designed with the hash transition in mind (commit-graph and multi-pack-index) have bytes available to indicate the hash algorithm used. Let's actually update these formats to be more self-contained with the two hash algorithms being available.

[1]
https://lore.kernel.org/git/CAN0heSp024=Kyy7gdQ2VSetk_5iVhj_qdT8CMVPcry_AwWrhHQ@mail.gmail.com/

This merges cleanly with tb/bloom-improvements, but both that branch and this patch series have merge conflicts with the corrected commit date patch series [2].

[2]
https://lore.kernel.org/git/[email protected]/

In particular, the following conflict can be resolved in the "obvioius" way:

<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< HEAD

    header: 43475048 1 $OID_VERSION 3 $NUM_BASE
================================
    header: 43475048 1 1 4 $NUM_BASE

Instead use:

    header: 43475048 1 $OID_VERSION 4 $NUM_BASE

But, it also needs the following fix to actually work with this series:

index 211ec625d2..09f133792c 100755
@@ -464,7 +464,7 @@ test_expect_success 'setup repo for mixed generation commit-graph-chain' '
        GIT_TEST_COMMIT_GRAPH_NO_GDAT=1 git commit-graph write --reachable --split=no-merge &&
        test-tool read-graph >output &&
        cat >expect <<-EOF &&
-       header: 43475048 1 1 4 1
+       header: 43475048 1 $OID_VERSION 4 1
        num_commits: 2
        chunks: oid_fanout oid_lookup commit_metadata
        EOF

@@ -482,7 +482,7 @@ test_expect_success 'does not write generation data chunk if not present on exis
        git commit-graph write --reachable --split=no-merge &&
        test-tool read-graph >output &&
        cat >expect <<-EOF &&
-       header: 43475048 1 1 4 2
+       header: 43475048 1 $OID_VERSION 4 2
        num_commits: 3
        chunks: oid_fanout oid_lookup commit_metadata
        EOF

If this is the way we want to go with the formats, then I'll assist coordinating these textual and semantic merge conflicts.

Thanks, -Stolee

Derrick Stolee (3):
t/README: document GIT_TEST_DEFAULT_HASH
commit-graph: use the hash version byte
multi-pack-index: use hash version byte

 .../technical/commit-graph-format.txt         |  9 +++-
 Documentation/technical/pack-format.txt       |  7 ++-
 commit-graph.c                                |  6 ++-
 midx.c                                        | 32 +++++++++++---
 t/README                                      |  3 ++
 t/helper/test-read-midx.c                     |  8 +++-
 t/t4216-log-bloom.sh                          |  8 +++-
 t/t5318-commit-graph.sh                       | 37 +++++++++++++++-
 t/t5319-multi-pack-index.sh                   | 43 +++++++++++++++++--
 t/t5324-split-commit-graph.sh                 |  8 +++-
 10 files changed, 142 insertions(+), 19 deletions(-)

base-commit: 878e727637ec5815ccb3301eb994a54df95b21b8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-703%2Fderrickstolee%2Fcommit-graph-256-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-703/derrickstolee/commit-graph-256-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/703

gitgitgadget

From: Derrick Stolee <[email protected]>

---
 t/README | 3 +++
 1 file changed, 3 insertions(+)

index 70ec61cf88..ecf8c7291d 100644
@@ -421,6 +421,9 @@ GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is
 the default when running tests), errors out when an abbreviated option
 is used.
 
+GIT_TEST_DEFAULT_HASH=<sha1|sha256> specifies which hash algorithm to use
+in the test scripts.
+
 Naming Tests
 ------------
 

gitgitgadget

Very helpful. Will queue.

Sounds good!

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

The commit-graph format reserved a byte among the header of the file to store a "hash version". During the SHA-256 work, this was not modified because file formats are not necessarily intended to work across hash versions. If a repository has SHA-256 as its hash algorithm, it automatically up-shifts the lengths of object names in all necessary formats.

However, since we have this byte available for adjusting the version, we can make the file formats more obviously incompatible instead of relying on other context from the repository.

Update the oid_version() method in commit-graph.c to add a new value, 2, for sha-256. This automatically writes the new value in a SHA-256 repository _and_ verifies the value is correct. This is a breaking change relative to the current 'master' branch since 092b677 (Merge branch 'bc/sha-256-cvs-svn-updates', 2020-08-13) but it is not breaking relative to any released version of Git.

The test impact is relatively minor: the output of 'test-tool read-graph' lists the header information, so those instances of '1' need to be replaced with a variable determined by GIT_TEST_DEFAULT_HASH. A more careful test is added that specifically creates a repository of each type then swaps the commit-graph files. The important value here is that the "git log" command succeeds while writing a message to stderr.

---
 .../technical/commit-graph-format.txt         |  9 ++++-
 commit-graph.c                                |  6 ++-
 t/t4216-log-bloom.sh                          |  8 +++-
 t/t5318-commit-graph.sh                       | 37 ++++++++++++++++++-
 t/t5324-split-commit-graph.sh                 |  8 +++-
 5 files changed, 62 insertions(+), 6 deletions(-)

index 440541045d..6ddbceba15 100644
@@ -42,8 +42,13 @@ 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
+      We infer the hash length (H) from this value:
+	1 => SHA-1
+	2 => SHA-256
+      If the hash type does not match the repository's hash algorithm, the
+      commit-graph file should be ignored with a warning presented to the
+      user.
 
   1-byte number (C) of "chunks"
 

index e51c91dd5b..d03328d64c 100644
@@ -179,7 +179,11 @@ static char *get_chain_filename(struct object_directory *odb)
 
 static uint8_t oid_version(void)
 {
-	return 1;
+	if (the_hash_algo->rawsz == GIT_SHA1_RAWSZ)
+		return 1;
+	if (the_hash_algo->rawsz == GIT_SHA256_RAWSZ)
+		return 2;
+	die(_("invalid hash version"));
 }
 
 static struct commit_graph *alloc_commit_graph(void)

index c21cc160f3..906af2799d 100755
@@ -6,6 +6,12 @@ test_description='git log for a path with Bloom filters'
 GIT_TEST_COMMIT_GRAPH=0
 GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
 
+OID_VERSION=1
+if [ "$GIT_DEFAULT_HASH" = "sha256" ]
+then
+	OID_VERSION=2
+fi
+
 test_expect_success 'setup test - repo, commits, commit graph, log outputs' '
 	git init &&
 	mkdir A A/B A/B/C &&

@@ -35,7 +41,7 @@ test_expect_success 'setup test - repo, commits, commit graph, log outputs' '
 graph_read_expect () {
 	NUM_CHUNKS=5
 	cat >expect <<- EOF
-	header: 43475048 1 1 $NUM_CHUNKS 0
+	header: 43475048 1 $OID_VERSION $NUM_CHUNKS 0
 	num_commits: $1
 	chunks: oid_fanout oid_lookup commit_metadata bloom_indexes bloom_data
 	EOF

index 044cf8a3de..5b65017676 100755
@@ -5,6 +5,12 @@ test_description='commit graph'
 
 GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
 
+OID_VERSION=1
+if [ "$GIT_DEFAULT_HASH" = "sha256" ]
+then
+	OID_VERSION=2
+fi
+
 test_expect_success 'setup full repo' '
 	mkdir full &&
 	cd "$TRASH_DIRECTORY/full" &&

@@ -77,7 +83,7 @@ graph_read_expect() {
 		NUM_CHUNKS=$((3 + $(echo "$2" | wc -w)))
 	fi
 	cat >expect <<- EOF
-	header: 43475048 1 1 $NUM_CHUNKS 0
+	header: 43475048 1 $OID_VERSION $NUM_CHUNKS 0
 	num_commits: $1
 	chunks: oid_fanout oid_lookup commit_metadata$OPTIONAL
 	EOF

@@ -412,6 +418,35 @@ test_expect_success 'replace-objects invalidates commit-graph' '
 	)
 '
 
+test_expect_success 'warn on improper hash version' '
+	git init --object-format=sha1 sha1 &&
+	(
+		cd sha1 &&
+		test_commit 1 &&
+		git commit-graph write --reachable &&
+		mv .git/objects/info/commit-graph ../cg-sha1
+	) &&
+	git init --object-format=sha256 sha256 &&
+	(
+		cd sha256 &&
+		test_commit 1 &&
+		git commit-graph write --reachable &&
+		mv .git/objects/info/commit-graph ../cg-sha256
+	) &&
+	(
+		cd sha1 &&
+		mv ../cg-sha256 .git/objects/info/commit-graph &&
+		git log -1 2>err &&
+		test_i18ngrep "commit-graph hash version 2 does not match version 1" err
+	) &&
+	(
+		cd sha256 &&
+		mv ../cg-sha1 .git/objects/info/commit-graph &&
+		git log -1 2>err &&
+		test_i18ngrep "commit-graph hash version 1 does not match version 2" err
+	)
+'
+
 # the verify tests below expect the commit-graph to contain
 # exactly the commits reachable from the commits/8 branch.
 # If the file changes the set of commits in the list, then the

index ea28d522b8..6f1a324f4f 100755
@@ -6,6 +6,12 @@ test_description='split commit graph'
 GIT_TEST_COMMIT_GRAPH=0
 GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
 
+OID_VERSION=1
+if [ "$GIT_DEFAULT_HASH" = "sha256" ]
+then
+	OID_VERSION=2
+fi
+
 test_expect_success 'setup repo' '
 	git init &&
 	git config core.commitGraph true &&

@@ -28,7 +34,7 @@ graph_read_expect() {
 		NUM_BASE=$2
 	fi
 	cat >expect <<- EOF
-	header: 43475048 1 1 3 $NUM_BASE
+	header: 43475048 1 $OID_VERSION 3 $NUM_BASE
 	num_commits: $1
 	chunks: oid_fanout oid_lookup commit_metadata
 	EOF

gitgitgadget

Very good idea.

That is perfectly fine. I think any file and on-wire protocol that uses anything but SHA-1 without identifying what it uses is a bug.

Will queue. Thanks.

Great. I have nothing to add other than my 'ack' that this is a good idea.

Thanks,
Taylor

Excellent.

I appreciate the suggested improvements here. I'm happy to do something more similar to other places in the code.

These will be part of my v2 to be re-rolled early next week.

Thanks,
-Stolee

I have nothing to add to brian's great suggestions. I'll be very happy to see this patch instead of my patch 5/5 'commit-graph-format.txt: fix "Hash Version" description'.

Martin

From: Derrick Stolee <[email protected]>

Similar to the commit-graph format, the multi-pack-index format has a byte in the header intended to track the hash version used to write the file. This allows one to interpret the hash length without having the context of the repository config specifying the hash length. This was not modified as part of the SHA-256 work because the hash length was automatically up-shifted due to that config.

Since we have this byte available, we can make the file formats more obviously incompatible instead of relying on other context from the repository.

Add a new oid_version() method in midx.c similar to the one in commit-graph.c. This is specifically made separate from that implementation to avoid artificially linking the formats.

The test impact requires a few more things than the corresponding change in the commit-graph format. Specifically, 'test-tool read-midx' was not writing anything about this header value to output. Since the value available in 'struct multi_pack_index' is hash_len instead of a version value, we output "20" or "32" instead of "1" or "2".

Since we want a user to not have their Git commands fail if their multi-pack-index has the incorrect hash version compared to the repository's hash version, we relax the die() to an error() in load_multi_pack_index(). This has some effect on 'git multi-pack-index verify' as we need to check that a failed parse of a file that exists is actually a verify error. For that test that checks the hash version matches, we change the corrupted byte from "2" to "3" to ensure the test fails for both hash algorithms.

---
 Documentation/technical/pack-format.txt |  7 +++-
 midx.c                                  | 32 ++++++++++++++----
 t/helper/test-read-midx.c               |  8 +++--
 t/t5319-multi-pack-index.sh             | 43 ++++++++++++++++++++++---
 4 files changed, 77 insertions(+), 13 deletions(-)

index d3a142c652..16cf7e83aa 100644
@@ -273,7 +273,12 @@ HEADER:
 	    Git only writes or recognizes version 1.
 
 	1-byte Object Id Version
-	    Git only writes or recognizes version 1 (SHA1).
+	    We infer the length of object IDs (OIDs) from this value:
+		1 => SHA-1
+		2 => SHA-256
+	    If the hash type does not match the repository's hash algorithm,
+	    the multi-pack-index file should be ignored with a warning
+	    presented to the user.
 
 	1-byte number of "chunks"
 

index a5fb797ede..0c165a40f5 100644
@@ -17,7 +17,6 @@
 #define MIDX_BYTE_HASH_VERSION 5
 #define MIDX_BYTE_NUM_CHUNKS 6
 #define MIDX_BYTE_NUM_PACKS 8
-#define MIDX_HASH_VERSION 1
 #define MIDX_HEADER_SIZE 12
 #define MIDX_MIN_SIZE (MIDX_HEADER_SIZE + the_hash_algo->rawsz)
 

@@ -36,6 +35,15 @@
 
 #define PACK_EXPIRED UINT_MAX
 
+static uint8_t oid_version(void)
+{
+	if (the_hash_algo->rawsz == GIT_SHA1_RAWSZ)
+		return 1;
+	if (the_hash_algo->rawsz == GIT_SHA256_RAWSZ)
+		return 2;
+	die(_("invalid hash version"));
+}
+
 static char *get_midx_filename(const char *object_dir)
 {
 	return xstrfmt("%s/pack/multi-pack-index", object_dir);

@@ -90,8 +98,11 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
 		      m->version);
 
 	hash_version = m->data[MIDX_BYTE_HASH_VERSION];
-	if (hash_version != MIDX_HASH_VERSION)
-		die(_("hash version %u does not match"), hash_version);
+	if (hash_version != oid_version()) {
+		error(_("multi-pack-index hash version %u does not match version %u"),
+		      hash_version, oid_version());
+		goto cleanup_fail;
+	}
 	m->hash_len = the_hash_algo->rawsz;
 
 	m->num_chunks = m->data[MIDX_BYTE_NUM_CHUNKS];

@@ -418,7 +429,7 @@ static size_t write_midx_header(struct hashfile *f,
 
 	hashwrite_be32(f, MIDX_SIGNATURE);
 	byte_values[0] = MIDX_VERSION;
-	byte_values[1] = MIDX_HASH_VERSION;
+	byte_values[1] = oid_version();
 	byte_values[2] = num_chunks;
 	byte_values[3] = 0; /* unused */
 	hashwrite(f, byte_values, sizeof(byte_values));

@@ -1105,8 +1116,17 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
 	struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
 	verify_midx_error = 0;
 
-	if (!m)
-		return 0;
+	if (!m) {
+		int result = 0;
+		struct stat sb;
+		char *filename = get_midx_filename(object_dir);
+		if (!stat(filename, &sb)) {
+			error(_("multi-pack-index file exists, but failed to parse"));
+			result = 1;
+		}
+		free(filename);
+		return result;
+	}
 
 	if (flags & MIDX_PROGRESS)
 		progress = start_progress(_("Looking for referenced packfiles"),

index 831b586d02..2430880f78 100644
@@ -7,14 +7,18 @@
 static int read_midx_file(const char *object_dir)
 {
 	uint32_t i;
-	struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
+	struct multi_pack_index *m;
+
+	setup_git_directory();
+	m = load_multi_pack_index(object_dir, 1);
 
 	if (!m)
 		return 1;
 
-	printf("header: %08x %d %d %d\n",
+	printf("header: %08x %d %d %d %d\n",
 	       m->signature,
 	       m->version,
+	       m->hash_len,
 	       m->num_chunks,
 	       m->num_packs);
 

gitgitgadget

I agree that the files should be self-identifying, but have these changes tested without sha256 hash?

All of the test scripts pass with and without GIT_TEST_DEFAULT_HASH=sha256, and this test in t5318 (and a similar one in t5319) are explicit about testing both options:

+test_expect_success 'warn on improper hash version' '
+ git init --object-format=sha1 sha1 &&
+ (
+ cd sha1 &&
+ test_commit 1 &&
+ git commit-graph write --reachable &&
+ mv .git/objects/info/commit-graph ../cg-sha1
+ ) &&
+ git init --object-format=sha256 sha256 &&
+ (
+ cd sha256 &&
+ test_commit 1 &&
+ git commit-graph write --reachable &&
+ mv .git/objects/info/commit-graph ../cg-sha256
+ ) &&
+ (
+ cd sha1 &&
+ mv ../cg-sha256 .git/objects/info/commit-graph &&
+ git log -1 2>err &&
+ test_i18ngrep "commit-graph hash version 2 does not match version 1" err
+ ) &&
+ (
+ cd sha256 &&
+ mv ../cg-sha1 .git/objects/info/commit-graph &&
+ git log -1 2>err &&
+ test_i18ngrep "commit-graph hash version 1 does not match version 2" err
+ )
+'
+

Since this tests exactly that the "hash version" byte is the same in a SHA-1 repo, this checks that the new version of Git writes backwards- compatible data in SHA-1 repos.

Or are you hinting at a more subtle test scenario that I missed?

Thanks,
-Stolee

No, I was just wondering how ready we are, as the four tests looked too easy ;-)

As discussed [1], there is some concern around binary file formats requiring the context of the repository config in order to infer hash lengths. Two formats that were designed with the hash transition in mind (commit-graph and multi-pack-index) have bytes available to indicate the hash algorithm used. Let's actually update these formats to be more self-contained with the two hash algorithms being available.

[1]
https://lore.kernel.org/git/CAN0heSp024=Kyy7gdQ2VSetk_5iVhj_qdT8CMVPcry_AwWrhHQ@mail.gmail.com/

This merges cleanly with tb/bloom-improvements, but both that branch and this patch series have merge conflicts with the corrected commit date patch series [2].

[2]
https://lore.kernel.org/git/[email protected]/

In particular, the following conflict can be resolved in the "obvioius" way:

<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< HEAD

    header: 43475048 1 $(test_oid oid_version) 3 $NUM_BASE
================================
    header: 43475048 1 1 4 $NUM_BASE

Instead use:

    header: 43475048 1 $(test_oid oid_version) 4 $NUM_BASE

But, it also needs the following fix to actually work with this series:

index 211ec625d2..09f133792c 100755
@@ -464,7 +464,7 @@ test_expect_success 'setup repo for mixed generation commit-graph-chain' '
        GIT_TEST_COMMIT_GRAPH_NO_GDAT=1 git commit-graph write --reachable --split=no-merge &&
        test-tool read-graph >output &&
        cat >expect <<-EOF &&
-       header: 43475048 1 1 4 1
+       header: 43475048 1 $(test_oid oid_version) 4 1
        num_commits: 2
        chunks: oid_fanout oid_lookup commit_metadata
        EOF

@@ -482,7 +482,7 @@ test_expect_success 'does not write generation data chunk if not present on exis
        git commit-graph write --reachable --split=no-merge &&
        test-tool read-graph >output &&
        cat >expect <<-EOF &&
-       header: 43475048 1 1 4 2
+       header: 43475048 1 $(test_oid oid_version) 4 2
        num_commits: 3
        chunks: oid_fanout oid_lookup commit_metadata
        EOF

If this is the way we want to go with the formats, then I'll assist coordinating these textual and semantic merge conflicts.

UPDATES IN V2
=============

* Documentation is updated, thanks to Eric's suggestion.
* The implementation of oid_version() and the way we access it in the test scripts is improved, thanks to Brian's suggestion.
* I use "mv" instead of "cp" in the cross-version tests because of a subtlety on macOS when overwriting these files.

Thanks, -Stolee

Derrick Stolee (3):
t/README: document GIT_TEST_DEFAULT_HASH
commit-graph: use the "hash version" byte
multi-pack-index: use hash version byte

 .../technical/commit-graph-format.txt         |  9 +++-
 Documentation/technical/pack-format.txt       |  7 ++-
 commit-graph.c                                |  9 +++-
 midx.c                                        | 35 ++++++++++++---
 t/README                                      |  4 ++
 t/helper/test-read-midx.c                     |  8 +++-
 t/t4216-log-bloom.sh                          |  9 +++-
 t/t5318-commit-graph.sh                       | 38 +++++++++++++++-
 t/t5319-multi-pack-index.sh                   | 43 +++++++++++++++++--
 t/t5324-split-commit-graph.sh                 |  5 ++-
 10 files changed, 146 insertions(+), 21 deletions(-)

base-commit: 878e727637ec5815ccb3301eb994a54df95b21b8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-703%2Fderrickstolee%2Fcommit-graph-256-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-703/derrickstolee/commit-graph-256-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/703

Range-diff vs v1:

1: 242a44b63c ! 1: 62e7247bad t/README: document GIT_TEST_DEFAULT_HASH
@@ Metadata
## Commit message ##
t/README: document GIT_TEST_DEFAULT_HASH

+ Helped-by: Eric Sunshine <[email protected]>

## t/README ##
@@ t/README: GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is
the default when running tests), errors out when an abbreviated option is used.

-+GIT_TEST_DEFAULT_HASH=<sha1|sha256> specifies which hash algorithm to use -+in the test scripts.
++GIT_TEST_DEFAULT_HASH=<hash-algo> specifies which hash algorithm to ++use in the test scripts. Recognized values for <hash-algo> are "sha1" ++and "sha256".
+
Naming Tests
------------
2: 4bbfd345d1 ! 2: 8d481f3b22 commit-graph: use the hash version byte

     @@ Metadata
      Author: Derrick Stolee <[email protected]>
      
       ## Commit message ##
     -    commit-graph: use the hash version byte
     +    commit-graph: use the "hash version" byte
      
          The commit-graph format reserved a byte among the header of the file to
          store a "hash version". During the SHA-256 work, this was not modified
     @@ Commit message
          each type then swaps the commit-graph files. The important value here is
          that the "git log" command succeeds while writing a message to stderr.
      
     +    Helped-by: brian m. carlson <[email protected]>
          Signed-off-by: Derrick Stolee <[email protected]>
      
       ## Documentation/technical/commit-graph-format.txt ##
     @@ commit-graph.c: static char *get_chain_filename(struct object_directory *odb)
       static uint8_t oid_version(void)
       {
      -	return 1;
     -+	if (the_hash_algo->rawsz == GIT_SHA1_RAWSZ)
     ++	switch (hash_algo_by_ptr(the_hash_algo)) {
     ++	case GIT_HASH_SHA1:
      +		return 1;
     -+	if (the_hash_algo->rawsz == GIT_SHA256_RAWSZ)
     ++	case GIT_HASH_SHA256:
      +		return 2;
     -+	die(_("invalid hash version"));
     ++	default:
     ++		die(_("invalid hash version"));
     ++	}
       }
       
       static struct commit_graph *alloc_commit_graph(void)
      
       ## t/t4216-log-bloom.sh ##
     -@@ t/t4216-log-bloom.sh: test_description='git log for a path with Bloom filters'
     - GIT_TEST_COMMIT_GRAPH=0
     - GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
     - 
     -+OID_VERSION=1
     -+if [ "$GIT_DEFAULT_HASH" = "sha256" ]
     -+then
     -+	OID_VERSION=2
     -+fi
     -+
     - test_expect_success 'setup test - repo, commits, commit graph, log outputs' '
     - 	git init &&
     - 	mkdir A A/B A/B/C &&
      @@ t/t4216-log-bloom.sh: test_expect_success 'setup test - repo, commits, commit graph, log outputs' '
     + 	rm file_to_be_deleted &&
     + 	git add . &&
     + 	git commit -m "file removed" &&
     +-	git commit-graph write --reachable --changed-paths
     ++	git commit-graph write --reachable --changed-paths &&
     ++
     ++	test_oid_cache <<-EOF
     ++	oid_version sha1:1
     ++	oid_version sha256:2
     ++	EOF
     + '
       graph_read_expect () {
       	NUM_CHUNKS=5
       	cat >expect <<- EOF
      -	header: 43475048 1 1 $NUM_CHUNKS 0
     -+	header: 43475048 1 $OID_VERSION $NUM_CHUNKS 0
     ++	header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0
       	num_commits: $1
       	chunks: oid_fanout oid_lookup commit_metadata bloom_indexes bloom_data
       	EOF
      
       ## t/t5318-commit-graph.sh ##
     -@@ t/t5318-commit-graph.sh: test_description='commit graph'
     - 
     - GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
     - 
     -+OID_VERSION=1
     -+if [ "$GIT_DEFAULT_HASH" = "sha256" ]
     -+then
     -+	OID_VERSION=2
     -+fi
     -+
     - test_expect_success 'setup full repo' '
     - 	mkdir full &&
     +@@ t/t5318-commit-graph.sh: test_expect_success 'setup full repo' '
       	cd "$TRASH_DIRECTORY/full" &&
     + 	git init &&
     + 	git config core.commitGraph true &&
     +-	objdir=".git/objects"
     ++	objdir=".git/objects" &&
     ++
     ++	test_oid_cache <<-EOF
     ++	oid_version sha1:1
     ++	oid_version sha256:2
     ++	EOF
     + '
     + 
     + test_expect_success POSIXPERM 'tweak umask for modebit tests' '
      @@ t/t5318-commit-graph.sh: graph_read_expect() {
       		NUM_CHUNKS=$((3 + $(echo "$2" | wc -w)))
       	fi
       	cat >expect <<- EOF
      -	header: 43475048 1 1 $NUM_CHUNKS 0
     -+	header: 43475048 1 $OID_VERSION $NUM_CHUNKS 0
     ++	header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0
       	num_commits: $1
       	chunks: oid_fanout oid_lookup commit_metadata$OPTIONAL
       	EOF
     @@ t/t5318-commit-graph.sh: test_expect_success 'replace-objects invalidates commit
       # If the file changes the set of commits in the list, then the
      
       ## t/t5324-split-commit-graph.sh ##
     -@@ t/t5324-split-commit-graph.sh: test_description='split commit graph'
     - GIT_TEST_COMMIT_GRAPH=0
     - GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
     +@@ t/t5324-split-commit-graph.sh: test_expect_success 'setup repo' '
       
     -+OID_VERSION=1
     -+if [ "$GIT_DEFAULT_HASH" = "sha256" ]
     -+then
     -+	OID_VERSION=2
     -+fi
     + 	base sha1:1376
     + 	base sha256:1496
      +
     - test_expect_success 'setup repo' '
     - 	git init &&
     - 	git config core.commitGraph true &&
     ++	oid_version sha1:1
     ++	oid_version sha256:2
     + 	EOM
     + '
     + 
      @@ t/t5324-split-commit-graph.sh: graph_read_expect() {
       		NUM_BASE=$2
       	fi
       	cat >expect <<- EOF
      -	header: 43475048 1 1 3 $NUM_BASE
     -+	header: 43475048 1 $OID_VERSION 3 $NUM_BASE
     ++	header: 43475048 1 $(test_oid oid_version) 3 $NUM_BASE
       	num_commits: $1
       	chunks: oid_fanout oid_lookup commit_metadata
       	EOF
 3:  b4645789ad ! 3:  822e46868f multi-pack-index: use hash version byte
     @@ Commit message
          matches, we change the corrupted byte from "2" to "3" to ensure the test
          fails for both hash algorithms.
      
     +    Helped-by: brian m. carlson <[email protected]>
          Signed-off-by: Derrick Stolee <[email protected]>
      
       ## Documentation/technical/pack-format.txt ##
     @@ midx.c
       
      +static uint8_t oid_version(void)
      +{
     -+	if (the_hash_algo->rawsz == GIT_SHA1_RAWSZ)
     ++	switch (hash_algo_by_ptr(the_hash_algo)) {
     ++	case GIT_HASH_SHA1:
      +		return 1;
     -+	if (the_hash_algo->rawsz == GIT_SHA256_RAWSZ)
     ++	case GIT_HASH_SHA256:
      +		return 2;
     -+	die(_("invalid hash version"));
     ++	default:
     ++		die(_("invalid hash version"));
     ++	}
      +}
      +
       static char *get_midx_filename(const char *object_dir)

gitgitgadget

From: Derrick Stolee <[email protected]>

---
 t/README | 4 ++++
 1 file changed, 4 insertions(+)

index 70ec61cf88..2adaf7c2d2 100644
@@ -421,6 +421,10 @@ GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is
 the default when running tests), errors out when an abbreviated option
 is used.
 
+GIT_TEST_DEFAULT_HASH=<hash-algo> specifies which hash algorithm to
+use in the test scripts. Recognized values for <hash-algo> are "sha1"
+and "sha256".
+
 Naming Tests
 ------------
 

gitgitgadget

From: Derrick Stolee <[email protected]>

The commit-graph format reserved a byte among the header of the file to store a "hash version". During the SHA-256 work, this was not modified because file formats are not necessarily intended to work across hash versions. If a repository has SHA-256 as its hash algorithm, it automatically up-shifts the lengths of object names in all necessary formats.

However, since we have this byte available for adjusting the version, we can make the file formats more obviously incompatible instead of relying on other context from the repository.

Update the oid_version() method in commit-graph.c to add a new value, 2, for sha-256. This automatically writes the new value in a SHA-256 repository _and_ verifies the value is correct. This is a breaking change relative to the current 'master' branch since 092b677 (Merge branch 'bc/sha-256-cvs-svn-updates', 2020-08-13) but it is not breaking relative to any released version of Git.

The test impact is relatively minor: the output of 'test-tool read-graph' lists the header information, so those instances of '1' need to be replaced with a variable determined by GIT_TEST_DEFAULT_HASH. A more careful test is added that specifically creates a repository of each type then swaps the commit-graph files. The important value here is that the "git log" command succeeds while writing a message to stderr.

---
 .../technical/commit-graph-format.txt         |  9 ++++-
 commit-graph.c                                |  9 ++++-
 t/t4216-log-bloom.sh                          |  9 ++++-
 t/t5318-commit-graph.sh                       | 38 ++++++++++++++++++-
 t/t5324-split-commit-graph.sh                 |  5 ++-
 5 files changed, 62 insertions(+), 8 deletions(-)

index 440541045d..6ddbceba15 100644
@@ -42,8 +42,13 @@ 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
+      We infer the hash length (H) from this value:
+	1 => SHA-1
+	2 => SHA-256
+      If the hash type does not match the repository's hash algorithm, the
+      commit-graph file should be ignored with a warning presented to the
+      user.
 
   1-byte number (C) of "chunks"
 

index e51c91dd5b..0ed003e218 100644
@@ -179,7 +179,14 @@ static char *get_chain_filename(struct object_directory *odb)
 
 static uint8_t oid_version(void)
 {
-	return 1;
+	switch (hash_algo_by_ptr(the_hash_algo)) {
+	case GIT_HASH_SHA1:
+		return 1;
+	case GIT_HASH_SHA256:
+		return 2;
+	default:
+		die(_("invalid hash version"));
+	}
 }
 
 static struct commit_graph *alloc_commit_graph(void)

index c21cc160f3..4bb9e9dbe2 100755
@@ -30,12 +30,17 @@ test_expect_success 'setup test - repo, commits, commit graph, log outputs' '
 	rm file_to_be_deleted &&
 	git add . &&
 	git commit -m "file removed" &&
-	git commit-graph write --reachable --changed-paths
+	git commit-graph write --reachable --changed-paths &&
+
+	test_oid_cache <<-EOF
+	oid_version sha1:1
+	oid_version sha256:2
+	EOF
 '
 graph_read_expect () {
 	NUM_CHUNKS=5
 	cat >expect <<- EOF
-	header: 43475048 1 1 $NUM_CHUNKS 0
+	header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0
 	num_commits: $1
 	chunks: oid_fanout oid_lookup commit_metadata bloom_indexes bloom_data
 	EOF

index 044cf8a3de..2ed0c1544d 100755
@@ -10,7 +10,12 @@ test_expect_success 'setup full repo' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git init &&
 	git config core.commitGraph true &&
-	objdir=".git/objects"
+	objdir=".git/objects" &&
+
+	test_oid_cache <<-EOF
+	oid_version sha1:1
+	oid_version sha256:2
+	EOF
 '
 
 test_expect_success POSIXPERM 'tweak umask for modebit tests' '

@@ -77,7 +82,7 @@ graph_read_expect() {
 		NUM_CHUNKS=$((3 + $(echo "$2" | wc -w)))
 	fi
 	cat >expect <<- EOF
-	header: 43475048 1 1 $NUM_CHUNKS 0
+	header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0
 	num_commits: $1
 	chunks: oid_fanout oid_lookup commit_metadata$OPTIONAL
 	EOF

@@ -412,6 +417,35 @@ test_expect_success 'replace-objects invalidates commit-graph' '
 	)
 '
 
+test_expect_success 'warn on improper hash version' '
+	git init --object-format=sha1 sha1 &&
+	(
+		cd sha1 &&
+		test_commit 1 &&
+		git commit-graph write --reachable &&
+		mv .git/objects/info/commit-graph ../cg-sha1
+	) &&
+	git init --object-format=sha256 sha256 &&
+	(
+		cd sha256 &&
+		test_commit 1 &&
+		git commit-graph write --reachable &&
+		mv .git/objects/info/commit-graph ../cg-sha256
+	) &&
+	(
+		cd sha1 &&
+		mv ../cg-sha256 .git/objects/info/commit-graph &&
+		git log -1 2>err &&
+		test_i18ngrep "commit-graph hash version 2 does not match version 1" err
+	) &&
+	(
+		cd sha256 &&
+		mv ../cg-sha1 .git/objects/info/commit-graph &&
+		git log -1 2>err &&
+		test_i18ngrep "commit-graph hash version 1 does not match version 2" err
+	)
+'
+
 # the verify tests below expect the commit-graph to contain
 # exactly the commits reachable from the commits/8 branch.
 # If the file changes the set of commits in the list, then the

index ea28d522b8..18216463c7 100755
@@ -18,6 +18,9 @@ test_expect_success 'setup repo' '
 
 	base sha1:1376
 	base sha256:1496
+
+	oid_version sha1:1
+	oid_version sha256:2
 	EOM
 '
 

@@ -28,7 +31,7 @@ graph_read_expect() {
 		NUM_BASE=$2
 	fi
 	cat >expect <<- EOF
-	header: 43475048 1 1 3 $NUM_BASE
+	header: 43475048 1 $(test_oid oid_version) 3 $NUM_BASE
 	num_commits: $1
 	chunks: oid_fanout oid_lookup commit_metadata
 	EOF

gitgitgadget

From: Derrick Stolee <[email protected]>

Similar to the commit-graph format, the multi-pack-index format has a byte in the header intended to track the hash version used to write the file. This allows one to interpret the hash length without having the context of the repository config specifying the hash length. This was not modified as part of the SHA-256 work because the hash length was automatically up-shifted due to that config.

Since we have this byte available, we can make the file formats more obviously incompatible instead of relying on other context from the repository.

Add a new oid_version() method in midx.c similar to the one in commit-graph.c. This is specifically made separate from that implementation to avoid artificially linking the formats.

The test impact requires a few more things than the corresponding change in the commit-graph format. Specifically, 'test-tool read-midx' was not writing anything about this header value to output. Since the value available in 'struct multi_pack_index' is hash_len instead of a version value, we output "20" or "32" instead of "1" or "2".

Since we want a user to not have their Git commands fail if their multi-pack-index has the incorrect hash version compared to the repository's hash version, we relax the die() to an error() in load_multi_pack_index(). This has some effect on 'git multi-pack-index verify' as we need to check that a failed parse of a file that exists is actually a verify error. For that test that checks the hash version matches, we change the corrupted byte from "2" to "3" to ensure the test fails for both hash algorithms.

---
 Documentation/technical/pack-format.txt |  7 +++-
 midx.c                                  | 35 ++++++++++++++++----
 t/helper/test-read-midx.c               |  8 +++--
 t/t5319-multi-pack-index.sh             | 43 ++++++++++++++++++++++---
 4 files changed, 80 insertions(+), 13 deletions(-)

index d3a142c652..16cf7e83aa 100644
@@ -273,7 +273,12 @@ HEADER:
 	    Git only writes or recognizes version 1.
 
 	1-byte Object Id Version
-	    Git only writes or recognizes version 1 (SHA1).
+	    We infer the length of object IDs (OIDs) from this value:
+		1 => SHA-1
+		2 => SHA-256
+	    If the hash type does not match the repository's hash algorithm,
+	    the multi-pack-index file should be ignored with a warning
+	    presented to the user.
 
 	1-byte number of "chunks"
 

index a5fb797ede..551a30b907 100644
@@ -17,7 +17,6 @@
 #define MIDX_BYTE_HASH_VERSION 5
 #define MIDX_BYTE_NUM_CHUNKS 6
 #define MIDX_BYTE_NUM_PACKS 8
-#define MIDX_HASH_VERSION 1
 #define MIDX_HEADER_SIZE 12
 #define MIDX_MIN_SIZE (MIDX_HEADER_SIZE + the_hash_algo->rawsz)
 

@@ -36,6 +35,18 @@
 
 #define PACK_EXPIRED UINT_MAX
 
+static uint8_t oid_version(void)
+{
+	switch (hash_algo_by_ptr(the_hash_algo)) {
+	case GIT_HASH_SHA1:
+		return 1;
+	case GIT_HASH_SHA256:
+		return 2;
+	default:
+		die(_("invalid hash version"));
+	}
+}
+
 static char *get_midx_filename(const char *object_dir)
 {
 	return xstrfmt("%s/pack/multi-pack-index", object_dir);

@@ -90,8 +101,11 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
 		      m->version);
 
 	hash_version = m->data[MIDX_BYTE_HASH_VERSION];
-	if (hash_version != MIDX_HASH_VERSION)
-		die(_("hash version %u does not match"), hash_version);
+	if (hash_version != oid_version()) {
+		error(_("multi-pack-index hash version %u does not match version %u"),
+		      hash_version, oid_version());
+		goto cleanup_fail;
+	}
 	m->hash_len = the_hash_algo->rawsz;
 
 	m->num_chunks = m->data[MIDX_BYTE_NUM_CHUNKS];

@@ -418,7 +432,7 @@ static size_t write_midx_header(struct hashfile *f,
 
 	hashwrite_be32(f, MIDX_SIGNATURE);
 	byte_values[0] = MIDX_VERSION;
-	byte_values[1] = MIDX_HASH_VERSION;
+	byte_values[1] = oid_version();
 	byte_values[2] = num_chunks;
 	byte_values[3] = 0; /* unused */
 	hashwrite(f, byte_values, sizeof(byte_values));

@@ -1105,8 +1119,17 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
 	struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
 	verify_midx_error = 0;
 
-	if (!m)
-		return 0;
+	if (!m) {
+		int result = 0;
+		struct stat sb;
+		char *filename = get_midx_filename(object_dir);
+		if (!stat(filename, &sb)) {
+			error(_("multi-pack-index file exists, but failed to parse"));
+			result = 1;
+		}
+		free(filename);
+		return result;
+	}
 
 	if (flags & MIDX_PROGRESS)
 		progress = start_progress(_("Looking for referenced packfiles"),

index 831b586d02..2430880f78 100644
@@ -7,14 +7,18 @@
 static int read_midx_file(const char *object_dir)
 {
 	uint32_t i;
-	struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
+	struct multi_pack_index *m;
+
+	setup_git_directory();
+	m = load_multi_pack_index(object_dir, 1);
 
 	if (!m)
 		return 1;
 
-	printf("header: %08x %d %d %d\n",
+	printf("header: %08x %d %d %d %d\n",
 	       m->signature,
 	       m->version,
+	       m->hash_len,
 	       m->num_chunks,
 	       m->num_packs);
 

gitgitgadget

This series looked sane to me. Thanks for the patches, and feel free to have my

brian m. carlson: Houston, Texas, US