Fix fsck --name-objects bug

From: Johannes Schindelin <[email protected]>

In 7b35efd734e (fsck_walk(): optionally name objects on the go, 2016-07-17), the fsck machinery learned to optionally name the objects, so that it is easier to see what part of the repository is in a bad shape, say, when objects are missing.

To save on complexity, this machinery uses a parser to determine the name of a parent given a commit's name: any ~<n> suffix is parsed and the parent's name is formed from the prefix together with ~<n+1>.

However, this parser has a bug: if it finds a suffix <n> that is _not_ ~<n>, it will mistake the empty string for the prefix and <n> for the generation number. In other words, it will generate a name of the form ~<bogus-number>.

Let's fix this.

Signed-off-by Johannes Schindelin <[email protected]>

---
 fsck.c          |  5 +++++
 t/t1450-fsck.sh | 10 ++++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

fsck.c +5 -0

index 73f30773f28a..83d727c6fe33 100644
@@ -461,6 +461,11 @@ static int fsck_walk_commit(struct commit *commit, void *data, struct fsck_optio
 				generation += power * (name[--len] - '0');
 			if (power > 1 && len && name[len - 1] == '~')
 				name_prefix_len = len - 1;
+			else {
+				/* Maybe a non-first parent, e.g. HEAD^2 */
+				generation = 0;
+				name_prefix_len = len;
+			}
 		}
 	}
 

t/t1450-fsck.sh +6 -4

index 779f700ac4a0..bfa3588f37ab 100755
@@ -607,13 +607,15 @@ test_expect_success 'fsck --name-objects' '
 	git init name-objects &&
 	(
 		cd name-objects &&
+		git config core.logAllRefUpdates false &&
 		test_commit julius caesar.t &&
-		test_commit augustus &&
-		test_commit caesar &&
+		test_commit augustus44 &&
+		test_commit caesar  &&
 		remove_object $(git rev-parse julius:caesar.t) &&
-		test_must_fail git fsck --name-objects >out &&
 		tree=$(git rev-parse --verify julius:) &&
-		test_i18ngrep "$tree (refs/tags/julius:" out
+		git tag -d julius &&
+		test_must_fail git fsck --name-objects >out &&
+		test_i18ngrep "$tree (refs/tags/augustus44\\^:" out
 	)
 '

"Johannes Schindelin via GitGitGadget" <[email protected]>

t/t1450-fsck.sh test_expect_success 'HEAD is part of refs, valid objects appear valid' ' · line 46

Quoted 9 lines · writes:

t/t1450-fsck.sh +3 -6

-test_expect_success 'setup: helpers for corruption tests' '
-	sha1_file() {
-		remainder=${1#??} &&
-		firsttwo=${1%$remainder} &&
-		echo ".git/objects/$firsttwo/$remainder"
-	} &&
+sha1_file () {
+	git rev-parse --git-path objects/$(test_oid_to_path "$1")
+}

Yeah, back when 90cf590f (fsck: optionally show more helpful info for broken links, 2016-07-17) originally introduced this pattern, we didn't have nicely abstracted helper, but now we do, and there is no reason not to use it. Nice.

Quoted 17 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>

t/t1450-fsck.sh test_expect_success 'HEAD is part of refs, valid objects appear valid' ' · line 46

Quoted 9 lines · writes:

t/t1450-fsck.sh +3 -6

-test_expect_success 'setup: helpers for corruption tests' '
-	sha1_file() {
-		remainder=${1#??} &&
-		firsttwo=${1%$remainder} &&
-		echo ".git/objects/$firsttwo/$remainder"
-	} &&
+sha1_file () {
+	git rev-parse --git-path objects/$(test_oid_to_path "$1")
+}

Yeah, back when 90cf590f (fsck: optionally show more helpful info for broken links, 2016-07-17) originally introduced this pattern, we didn't have nicely abstracted helper, but now we do, and there is no reason not to use it. Nice.

This has nothing to do with this series, but I do notice a number of other uses of test_oid_to_path that are doing this exact thing. In fact, many of them don't use "git rev-parse --git-path", which would be better.

I wonder if it's worth a clean-up on top to consolidate all of those "combine the loose object path of the object with xyz OID and the $GIT_DIR/objects directory".

In either case -- and I think I'm pretty clearly being pedantic at this point -- do you suppose it's worthwhile to rename sha1_file to something that doesn't have sha1 in it?