threads / patch / 50554

patch, 8 partscommit-graph: segfault & other fixes for broken graphs

Subject: [PATCH 0/8] commit-graph: segfault & other fixes for broken graphs

## tl;dr

36 messages between Feb 21, 2019 and May 1, 2019. Diffs are folded; open one to read it.

replies: 35people: 6as markdown or json

Ævar Arnfjörð Bjarmason· Feb 21, 2019, 22:37 UTC · lore

This one isn't for 2.21.0, these issues are already in v2.20.0, I just spotted them when looking at & fixing the commit-graph test broken on NetBSD[1].

This series touches (among other things) some of the same test code, but merges cleanly with that "dd" fix, and it's not required for testing it (unless you're on NetBSD).

Instrumenting the commit-graph tests reveals that if you have a broken commit-graph git will either segfault or abort early with cryptic errors on such basic commands as "status".

Furthemore, if our graph is corrupt and we've set core.commitGraph=true we can't even write a new commit-graph, because writing a new one has grown to implicitly depend on reading the old one!

This series fixes all of that.
1. https://public-inbox.org/git/20190221192849.6581-3-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (8):
  commit-graph tests: split up corrupt_graph_and_verify()
  commit-graph tests: test a graph that's too small
  commit-graph: fix segfault on e.g. "git status"
  commit-graph: don't early exit(1) on e.g. "git status"
  commit-graph: don't pass filename to load_commit_graph_one_fd_st()
  commit-graph verify: detect inability to read the graph
  commit-graph write: don't die if the existing graph is corrupt
  commit-graph: improve & i18n error messages
 builtin/commit-graph.c  |  23 +++++--
 commit-graph.c          | 132 +++++++++++++++++++++++++++-------------
 commit-graph.h          |   4 ++
 commit.h                |   6 ++
 t/t5318-commit-graph.sh |  48 +++++++++++++--
 5 files changed, 160 insertions(+), 53 deletions(-)
-- 
2.21.0.rc0.258.g878e2cd30e
Ævar Arnfjörð Bjarmason· Feb 21, 2019, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 1/8] commit-graph tests: split up corrupt_graph_and_verify()

Split up the corrupt_graph_and_verify() function added in d9b9f8a6fd ("commit-graph: verify catches corrupt signature", 2018-06-27) into its logical components of setting up the test itself, doing the corruption in a particular way with "dd", and then finally testing that stderr is what we expect.

This allows for re-using everything except the now slimmer corrupt_graph_and_verify() to corrupt the graph in a way that doesn't involve inserting a given byte sequence at a given position, e.g. truncating it entirely to a custom value.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5318-commit-graph.sh | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)
Show changes to t/t5318-commit-graph.sh +16 −6
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index d4bd1522fe..725b7ce51f 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -366,6 +366,19 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \
 GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))
 GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
 
+corrupt_graph_setup() {
+	cd "$TRASH_DIRECTORY/full" &&
+	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
+	cp $objdir/info/commit-graph commit-graph-backup
+}
+
+corrupt_graph_verify() {
+	grepstr=$1
+	test_must_fail git commit-graph verify 2>test_err &&
+	grep -v "^+" test_err >err &&
+	test_i18ngrep "$grepstr" err
+}
+
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
 # Manipulates the commit-graph file at the position
 # by inserting the data, optionally zeroing the file
@@ -376,17 +389,14 @@ corrupt_graph_and_verify() {
 	pos=$1
 	data="${2:-\0}"
 	grepstr=$3
-	cd "$TRASH_DIRECTORY/full" &&
+	corrupt_graph_setup &&
 	orig_size=$(wc -c < $objdir/info/commit-graph) &&
 	zero_pos=${4:-${orig_size}} &&
-	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
-	cp $objdir/info/commit-graph commit-graph-backup &&
 	printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
 	dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 &&
 	generate_zero_bytes $(($orig_size - $zero_pos)) >>"$objdir/info/commit-graph" &&
-	test_must_fail git commit-graph verify 2>test_err &&
-	grep -v "^+" test_err >err &&
-	test_i18ngrep "$grepstr" err
+	corrupt_graph_verify "$grepstr"
+
 }
 
 test_expect_success 'detect bad signature' '

-- 
2.21.0.rc0.258.g878e2cd30e
Ævar Arnfjörð Bjarmason· Feb 21, 2019, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 2/8] commit-graph tests: test a graph that's too small

Use the recently split-up components of the corrupt_graph_and_verify() function to assert that we error on graphs that are too small. The error was added in 2a2e32bdc5 ("commit-graph: implement git commit-graph read", 2018-04-10), but there was no test for it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5318-commit-graph.sh | 6 ++++++
 1 file changed, 6 insertions(+)
Show changes to t/t5318-commit-graph.sh +6 −0
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 725b7ce51f..733be2ed30 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -399,6 +399,12 @@ corrupt_graph_and_verify() {
 
 }
 
+test_expect_success 'detect too small' '
+	corrupt_graph_setup &&
+	echo "a small graph" >$objdir/info/commit-graph &&
+	corrupt_graph_verify "too small"
+'
+
 test_expect_success 'detect bad signature' '
 	corrupt_graph_and_verify 0 "\0" \
 		"graph signature"

-- 
2.21.0.rc0.258.g878e2cd30e
Ævar Arnfjörð Bjarmason· Feb 21, 2019, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 3/8] commit-graph: fix segfault on e.g. "git status"

When core.commitGraph=true is set, various common commands now consult the commit graph. Because the commit-graph code is very trusting of its input data, it's possibly to construct a graph that'll cause an immediate segfault on e.g. "status" (and e.g. "log", "blame", ...). In some other cases where git immediately exits with a cryptic error about the graph being broken.

The root cause of this is that while the "commit-graph verify" sub-command exhaustively verifies the graph, other users of the graph simply trust the graph, and will e.g. deference data found at certain offsets as pointers, causing segfaults.

This change does the bare minimum to ensure that we don't segfault in the common fill_commit_in_graph() codepath called by e.g. setup_revisions(), to do this instrument the "commit-graph verify" tests to always check if "status" would subsequently segfault. This fixes the following tests which would previously segfault:

    not ok 50 - detect low chunk count
    not ok 51 - detect missing OID fanout chunk
    not ok 52 - detect missing OID lookup chunk
    not ok 53 - detect missing commit data chunk

Those happened because with the commit-graph enabled setup_revisions() would eventually call fill_commit_in_graph(), where e.g. g->chunk_commit_data is used early as an offset (and will be 0x0). With this change we get far enough to detect that the graph is broken, and show an error instead. E.g.:

    $ git status; echo $?
    error: commit-graph is missing the Commit Data chunk
    1

That also sucks, we should *warn* and not hard-fail "status" just because the commit-graph is corrupt, but fixing is left to a follow-up change.

A side-effect of changing the reporting from graph_report() to error() is that we now have an "error: " prefix for these even for "commit-graph verify". Pseudo-diff before/after:

    $ git commit-graph verify
    -commit-graph is missing the Commit Data chunk
    +error: commit-graph is missing the Commit Data chunk

Changing that is OK. Various errors it emits now early on are prefixed with "error: ", moving these over and changing the output doesn't break anything.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit-graph.c          | 43 ++++++++++++++++++++++++++++++++---------
 commit-graph.h          |  1 +
 t/t5318-commit-graph.sh |  3 ++-
 3 files changed, 37 insertions(+), 10 deletions(-)
Show changes to 3 files +37 −10

commit-graph.c, commit-graph.h, t/t5318-commit-graph.sh

diff --git a/commit-graph.c b/commit-graph.c
index 47e9be0a3a..980fbf47ea 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -233,6 +233,9 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 		last_chunk_offset = chunk_offset;
 	}
 
+	if (verify_commit_graph_lite(graph))
+		return NULL;
+
 	return graph;
 }
 
@@ -1075,6 +1078,36 @@ static void graph_report(const char *fmt, ...)
 #define GENERATION_ZERO_EXISTS 1
 #define GENERATION_NUMBER_EXISTS 2
 
+int verify_commit_graph_lite(struct commit_graph *g)
+{
+	/*
+	 * Basic validation shared between parse_commit_graph()
+	 * which'll be called every time the graph is used, and the
+	 * much more expensive verify_commit_graph() used by
+	 * "commit-graph verify".
+	 *
+	 * There should only be very basic checks here to ensure that
+	 * we don't e.g. segfault in fill_commit_in_graph(), but
+	 * because this is a very hot codepath nothing that e.g. loops
+	 * over g->num_commits, or runs a checksum on the commit-graph
+	 * itself.
+	 */
+	if (!g->chunk_oid_fanout) {
+		error("commit-graph is missing the OID Fanout chunk");
+		return 1;
+	}
+	if (!g->chunk_oid_lookup) {
+		error("commit-graph is missing the OID Lookup chunk");
+		return 1;
+	}
+	if (!g->chunk_commit_data) {
+		error("commit-graph is missing the Commit Data chunk");
+		return 1;
+	}
+
+	return 0;
+}
+
 int verify_commit_graph(struct repository *r, struct commit_graph *g)
 {
 	uint32_t i, cur_fanout_pos = 0;
@@ -1089,15 +1122,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		return 1;
 	}
 
-	verify_commit_graph_error = 0;
-
-	if (!g->chunk_oid_fanout)
-		graph_report("commit-graph is missing the OID Fanout chunk");
-	if (!g->chunk_oid_lookup)
-		graph_report("commit-graph is missing the OID Lookup chunk");
-	if (!g->chunk_commit_data)
-		graph_report("commit-graph is missing the Commit Data chunk");
-
+	verify_commit_graph_error = verify_commit_graph_lite(g);
 	if (verify_commit_graph_error)
 		return verify_commit_graph_error;
 
diff --git a/commit-graph.h b/commit-graph.h
index 096d8bac34..275f97d006 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -70,6 +70,7 @@ void write_commit_graph(const char *obj_dir,
 			struct string_list *commit_hex,
 			int append, int report_progress);
 
+int verify_commit_graph_lite(struct commit_graph *g);
 int verify_commit_graph(struct repository *r, struct commit_graph *g);
 
 void close_commit_graph(struct repository *);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 733be2ed30..b3f3e515fc 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -376,7 +376,8 @@ corrupt_graph_verify() {
 	grepstr=$1
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
-	test_i18ngrep "$grepstr" err
+	test_i18ngrep "$grepstr" err &&
+	test_might_fail git status --short
 }
 
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]

-- 
2.21.0.rc0.258.g878e2cd30e
Ævar Arnfjörð Bjarmason· Feb 21, 2019, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 4/8] commit-graph: don't early exit(1) on e.g. "git status"

Make the commit-graph loading code work as a library that returns an error code instead of calling exit(1) when the commit-graph is corrupt. This means that e.g. "status" will now report commit-graph corruption as an "error: [...]" at the top of its output, but then proceed to work normally.

This required splitting up the load_commit_graph_one() function so that the code that deals with open()-ing and stat()-ing the graph can now be called independently as open_commit_graph().

This is needed because "commit-graph verify" where the graph doesn't exist isn't an error. See the third paragraph in 283e68c72f ("commit-graph: add 'verify' subcommand", 2018-06-27). There's a bug in that logic where we conflate the intended ENOENT with other errno values (e.g. EACCES), but this change doesn't address that. That'll be addressed in a follow-up change.

I'm then splitting most of the logic out of load_commit_graph_one() into load_commit_graph_one_fd_st(), which allows for providing an existing file descriptor and stat information to the loading code. This isn't strictly needed, but it would be redundant and confusing to open() and stat() the file twice for some of the codepaths, this allows for calling open_commit_graph() followed by load_commit_graph_one_fd_st(). The "graph_file" still needs to be passed to that function for the the "graph file %s is too small" error message.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c  | 21 +++++++++++++++++----
 commit-graph.c          | 42 +++++++++++++++++++++++++++++------------
 commit-graph.h          |  3 +++
 t/t5318-commit-graph.sh |  2 +-
 4 files changed, 51 insertions(+), 17 deletions(-)
Show changes to 4 files +51 −17

builtin/commit-graph.c, commit-graph.c, commit-graph.h, t/t5318-commit-graph.sh

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 4ae502754c..32bcc63427 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -42,6 +42,9 @@ static int graph_verify(int argc, const char **argv)
 {
 	struct commit_graph *graph = NULL;
 	char *graph_name;
+	int open_ok;
+	int fd;
+	struct stat st;
 
 	static struct option builtin_commit_graph_verify_options[] = {
 		OPT_STRING(0, "object-dir", &opts.obj_dir,
@@ -58,11 +61,14 @@ static int graph_verify(int argc, const char **argv)
 		opts.obj_dir = get_object_directory();
 
 	graph_name = get_commit_graph_filename(opts.obj_dir);
-	graph = load_commit_graph_one(graph_name);
+	open_ok = open_commit_graph(graph_name, &fd, &st);
+	if (!open_ok)
+		return 0;
+	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
 	FREE_AND_NULL(graph_name);
 
 	if (!graph)
-		return 0;
+		return 1;
 
 	UNLEAK(graph);
 	return verify_commit_graph(the_repository, graph);
@@ -72,6 +78,9 @@ static int graph_read(int argc, const char **argv)
 {
 	struct commit_graph *graph = NULL;
 	char *graph_name;
+	int open_ok;
+	int fd;
+	struct stat st;
 
 	static struct option builtin_commit_graph_read_options[] = {
 		OPT_STRING(0, "object-dir", &opts.obj_dir,
@@ -88,10 +97,14 @@ static int graph_read(int argc, const char **argv)
 		opts.obj_dir = get_object_directory();
 
 	graph_name = get_commit_graph_filename(opts.obj_dir);
-	graph = load_commit_graph_one(graph_name);
 
+	open_ok = open_commit_graph(graph_name, &fd, &st);
+	if (!open_ok)
+		die_errno(_("Could not open commit-graph '%s'"), graph_name);
+
+	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
 	if (!graph)
-		die("graph file %s does not exist", graph_name);
+		return 1;
 
 	FREE_AND_NULL(graph_name);
 
diff --git a/commit-graph.c b/commit-graph.c
index 980fbf47ea..b1ba7a09cc 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -80,25 +80,31 @@ static int commit_graph_compatible(struct repository *r)
 	return 1;
 }
 
-struct commit_graph *load_commit_graph_one(const char *graph_file)
+int open_commit_graph(const char *graph_file, int *fd, struct stat *st)
+{
+	*fd = git_open(graph_file);
+	if (*fd < 0)
+		return 0;
+	if (fstat(*fd, st)) {
+		close(*fd);
+		return 0;
+	}
+	return 1;
+}
+
+struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
+						 int fd, struct stat *st)
 {
 	void *graph_map;
 	size_t graph_size;
-	struct stat st;
 	struct commit_graph *ret;
-	int fd = git_open(graph_file);
 
-	if (fd < 0)
-		return NULL;
-	if (fstat(fd, &st)) {
-		close(fd);
-		return NULL;
-	}
-	graph_size = xsize_t(st.st_size);
+	graph_size = xsize_t(st->st_size);
 
 	if (graph_size < GRAPH_MIN_SIZE) {
 		close(fd);
-		die(_("graph file %s is too small"), graph_file);
+		error(_("graph file %s is too small"), graph_file);
+		return 0;
 	}
 	graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	ret = parse_commit_graph(graph_map, fd, graph_size);
@@ -106,12 +112,24 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
 	if (!ret) {
 		munmap(graph_map, graph_size);
 		close(fd);
-		exit(1);
 	}
 
 	return ret;
 }
 
+struct commit_graph *load_commit_graph_one(const char *graph_file)
+{
+
+	struct stat st;
+	int fd;
+	int open_ok = open_commit_graph(graph_file, &fd, &st);
+
+	if (!open_ok)
+		return NULL;
+
+	return load_commit_graph_one_fd_st(graph_file, fd, &st);
+}
+
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 					size_t graph_size)
 {
diff --git a/commit-graph.h b/commit-graph.h
index 275f97d006..e4f17071e2 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -11,6 +11,7 @@
 struct commit;
 
 char *get_commit_graph_filename(const char *obj_dir);
+int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
 
 /*
  * Given a commit struct, try to fill the commit struct info, including:
@@ -53,6 +54,8 @@ struct commit_graph {
 };
 
 struct commit_graph *load_commit_graph_one(const char *graph_file);
+struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
+						 int fd, struct stat *st);
 
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 					size_t graph_size);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index b3f3e515fc..0d012f55e5 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -377,7 +377,7 @@ corrupt_graph_verify() {
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
 	test_i18ngrep "$grepstr" err &&
-	test_might_fail git status --short
+	git status --short
 }
 
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]

-- 
2.21.0.rc0.258.g878e2cd30e
Ævar Arnfjörð Bjarmason· Feb 21, 2019, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 6/8] commit-graph verify: detect inability to read the graph

Change "commit-graph verify" to error on open() failures other than ENOENT. As noted in the third paragraph of 283e68c72f ("commit-graph: add 'verify' subcommand", 2018-06-27) and the test it added it's intentional that "commit-graph verify" doesn't error out when the file doesn't exist.

But let's not be overly promiscuous in what we accept. If we can't read the file for other reasons, e.g. permission errors, bad file descriptor etc. we'd like to report an error to the user.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c  |  4 +++-
 t/t5318-commit-graph.sh | 12 ++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)
Show changes to 2 files +15 −1

builtin/commit-graph.c, t/t5318-commit-graph.sh

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 8196fdbe9c..537fdfd0f0 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -62,8 +62,10 @@ static int graph_verify(int argc, const char **argv)
 
 	graph_name = get_commit_graph_filename(opts.obj_dir);
 	open_ok = open_commit_graph(graph_name, &fd, &st);
-	if (!open_ok)
+	if (!open_ok && errno == ENOENT)
 		return 0;
+	if (!open_ok)
+		die_errno(_("Could not open commit-graph '%s'"), graph_name);
 	graph = load_commit_graph_one_fd_st(fd, &st);
 	FREE_AND_NULL(graph_name);
 
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 0d012f55e5..1ee00fa333 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -400,6 +400,18 @@ corrupt_graph_and_verify() {
 
 }
 
+test_expect_success 'detect permission problem' '
+	corrupt_graph_setup &&
+	chmod 000 $objdir/info/commit-graph &&
+
+	# Skip as root, or in other cases (odd fs or OS) where a
+	# "chmod 000 file" does not yield EACCES on e.g. "cat file"
+	if ! test -r $objdir/info/commit-graph
+	then
+		corrupt_graph_verify "Could not open"
+	fi
+'
+
 test_expect_success 'detect too small' '
 	corrupt_graph_setup &&
 	echo "a small graph" >$objdir/info/commit-graph &&

-- 
2.21.0.rc0.258.g878e2cd30e
SZEDER Gábor· Feb 21, 2019, 23:15 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 6/8] commit-graph verify: detect inability to read the graph

On Thu, Feb 21, 2019 at 11:37:51PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 11 quoted lines
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 0d012f55e5..1ee00fa333 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -400,6 +400,18 @@ corrupt_graph_and_verify() {
>  
>  }
>  
> +test_expect_success 'detect permission problem' '
> +	corrupt_graph_setup &&
> +	chmod 000 $objdir/info/commit-graph &&
This needs the POSIXPERM prereq.
> +
> +	# Skip as root, or in other cases (odd fs or OS) where a
> +	# "chmod 000 file" does not yield EACCES on e.g. "cat file"
> +	if ! test -r $objdir/info/commit-graph
And this condition would be unnecessary with the SANITY prereq.
Show 11 quoted lines
> +	then
> +		corrupt_graph_verify "Could not open"
> +	fi
> +'
> +
>  test_expect_success 'detect too small' '
>  	corrupt_graph_setup &&
>  	echo "a small graph" >$objdir/info/commit-graph &&
> -- 
> 2.21.0.rc0.258.g878e2cd30e
>
Ævar Arnfjörð Bjarmason· Feb 21, 2019, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 5/8] commit-graph: don't pass filename to load_commit_graph_one_fd_st()

An earlier change implemented load_commit_graph_one_fd_st() in a way that was bug-compatible with earlier code in terms of the "graph file %s is too small" error message printing out the path to the commit-graph (".git/objects/info/commit-graph").

But change that, because:
 * A function that takes an already-open file descriptor also needing
   the filename isn't very intuitive.
 * The vast majority of errors we might emit when loading the graph
   come from parse_commit_graph(), which doesn't report the
   filename. Let's not do that either in this case for consistency.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c | 4 ++--
 commit-graph.c         | 7 +++----
 commit-graph.h         | 3 +--
 3 files changed, 6 insertions(+), 8 deletions(-)
Show changes to 3 files +6 −8

builtin/commit-graph.c, commit-graph.c, commit-graph.h

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 32bcc63427..8196fdbe9c 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -64,7 +64,7 @@ static int graph_verify(int argc, const char **argv)
 	open_ok = open_commit_graph(graph_name, &fd, &st);
 	if (!open_ok)
 		return 0;
-	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
+	graph = load_commit_graph_one_fd_st(fd, &st);
 	FREE_AND_NULL(graph_name);
 
 	if (!graph)
@@ -102,7 +102,7 @@ static int graph_read(int argc, const char **argv)
 	if (!open_ok)
 		die_errno(_("Could not open commit-graph '%s'"), graph_name);
 
-	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
+	graph = load_commit_graph_one_fd_st(fd, &st);
 	if (!graph)
 		return 1;
 
diff --git a/commit-graph.c b/commit-graph.c
index b1ba7a09cc..d945e8f3e0 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -92,8 +92,7 @@ int open_commit_graph(const char *graph_file, int *fd, struct stat *st)
 	return 1;
 }
 
-struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
-						 int fd, struct stat *st)
+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st)
 {
 	void *graph_map;
 	size_t graph_size;
@@ -103,7 +102,7 @@ struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
 
 	if (graph_size < GRAPH_MIN_SIZE) {
 		close(fd);
-		error(_("graph file %s is too small"), graph_file);
+		error(_("commit-graph file is too small"));
 		return 0;
 	}
 	graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
@@ -127,7 +126,7 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
 	if (!open_ok)
 		return NULL;
 
-	return load_commit_graph_one_fd_st(graph_file, fd, &st);
+	return load_commit_graph_one_fd_st(fd, &st);
 }
 
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
diff --git a/commit-graph.h b/commit-graph.h
index e4f17071e2..36d8109901 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -54,8 +54,7 @@ struct commit_graph {
 };
 
 struct commit_graph *load_commit_graph_one(const char *graph_file);
-struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
-						 int fd, struct stat *st);
+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);
 
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 					size_t graph_size);

-- 
2.21.0.rc0.258.g878e2cd30e
Ævar Arnfjörð Bjarmason· Feb 21, 2019, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 7/8] commit-graph write: don't die if the existing graph is corrupt

When the commit-graph is written we end up calling parse_commit(). This will in turn invoke code that'll consult the existing commit-graph about the commit, if the graph is corrupted we die.

We thus get into a state where a failing "commit-graph verify" can't be followed-up with a "commit-graph write" if core.commitGraph=true is set, the graph either needs to be manually removed to proceed, or core.commitGraph needs to be set to "false".

Change the "commit-graph write" codepath to use a new parse_commit_no_graph() helper instead of parse_commit() to avoid this. The latter will call repo_parse_commit_internal() with use_commit_graph=1 as seen in 177722b344 ("commit: integrate commit graph with commit parsing", 2018-04-10).

Just fixing the current issue would be likely to result in code that's inadvertently broken in the future. New code might use the commit-graph at a distance. To detect such cases introduce a "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD" setting used when we do our corruption tests, and test that a "write/verify" combo works after every one of our current test cases where we now detect commit-graph corruption.

Some of the code changes here might be strictly unnecessary, e.g. I was unable to find cases where the parse_commit() called from write_graph_chunk_data() didn't exit early due to "item->object.parsed" being true in repo_parse_commit_internal() (before the use_commit_graph=1 has any effect). But let's also convert those cases for good measure, we do not have exhaustive tests for all possible types of commit-graph corruption.

This might need to be re-visited if we learn to write the commit-graph incrementally.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit-graph.c          | 10 +++++++---
 commit-graph.h          |  1 +
 commit.h                |  6 ++++++
 t/t5318-commit-graph.sh | 11 +++++++++--
 4 files changed, 23 insertions(+), 5 deletions(-)
Show changes to 4 files +23 −5

commit-graph.c, commit-graph.h, commit.h, t/t5318-commit-graph.sh

diff --git a/commit-graph.c b/commit-graph.c
index d945e8f3e0..6b3ade9496 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -281,6 +281,10 @@ static int prepare_commit_graph(struct repository *r)
 	struct object_directory *odb;
 	int config_value;
 
+	if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))
+		die("Dying as requested by the '%s' variable on commit-graph load!",
+		    GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);
+
 	if (r->objects->commit_graph_attempted)
 		return !!r->objects->commit_graph;
 	r->objects->commit_graph_attempted = 1;
@@ -545,7 +549,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
 		uint32_t packedDate[2];
 		display_progress(progress, ++*progress_cnt);
 
-		parse_commit(*list);
+		parse_commit_no_graph(*list);
 		hashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);
 
 		parent = (*list)->parents;
@@ -742,7 +746,7 @@ static void close_reachable(struct packed_oid_list *oids, int report_progress)
 		display_progress(progress, i + 1);
 		commit = lookup_commit(the_repository, &oids->list[i]);
 
-		if (commit && !parse_commit(commit))
+		if (commit && !parse_commit_no_graph(commit))
 			add_missing_parents(oids, commit);
 	}
 	stop_progress(&progress);
@@ -991,7 +995,7 @@ void write_commit_graph(const char *obj_dir,
 			continue;
 
 		commits.list[commits.nr] = lookup_commit(the_repository, &oids.list[i]);
-		parse_commit(commits.list[commits.nr]);
+		parse_commit_no_graph(commits.list[commits.nr]);
 
 		for (parent = commits.list[commits.nr]->parents;
 		     parent; parent = parent->next)
diff --git a/commit-graph.h b/commit-graph.h
index 36d8109901..6021ababa2 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -7,6 +7,7 @@
 #include "cache.h"
 
 #define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH"
+#define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD"
 
 struct commit;
 
diff --git a/commit.h b/commit.h
index 42728c2906..5d33477e78 100644
--- a/commit.h
+++ b/commit.h
@@ -89,6 +89,12 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)
 {
 	return repo_parse_commit_gently(r, item, 0);
 }
+
+static inline int parse_commit_no_graph(struct commit *commit)
+{
+	return repo_parse_commit_internal(the_repository, commit, 0, 0);
+}
+
 #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
 #define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use)
 #define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet)
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 1ee00fa333..6db658ed66 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -377,7 +377,13 @@ corrupt_graph_verify() {
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
 	test_i18ngrep "$grepstr" err &&
-	git status --short
+	if test -z "$NO_WRITE_TEST_BACKUP"
+	then
+		cp $objdir/info/commit-graph commit-graph-pre-write-test
+	fi &&
+	git status --short &&
+	GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&
+	git commit-graph verify
 }
 
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
@@ -408,7 +414,7 @@ test_expect_success 'detect permission problem' '
 	# "chmod 000 file" does not yield EACCES on e.g. "cat file"
 	if ! test -r $objdir/info/commit-graph
 	then
-		corrupt_graph_verify "Could not open"
+		NO_WRITE_TEST_BACKUP=1 corrupt_graph_verify "Could not open"
 	fi
 '
 
@@ -528,6 +534,7 @@ test_expect_success 'git fsck (checks commit-graph)' '
 	git fsck &&
 	corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
 		"incorrect checksum" &&
+	cp commit-graph-pre-write-test $objdir/info/commit-graph &&
 	test_must_fail git fsck
 '
 

-- 
2.21.0.rc0.258.g878e2cd30e
Junio C Hamano· Feb 22, 2019, 23:13 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 7/8] commit-graph write: don't die if the existing graph is corrupt

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> When the commit-graph is written we end up calling
> parse_commit(). This will in turn invoke code that'll consult the
> existing commit-graph about the commit, if the graph is corrupted we
> die.
Irony ;-).
Show 5 quoted lines
> Change the "commit-graph write" codepath to use a new
> parse_commit_no_graph() helper instead of parse_commit() to avoid
> this. The latter will call repo_parse_commit_internal() with
> use_commit_graph=1 as seen in 177722b344 ("commit: integrate commit
> graph with commit parsing", 2018-04-10).

That may slow down writing the graph but would be a sensible way to prevent an error in the existing commit-graph from spreading.

> This might need to be re-visited if we learn to write the commit-graph
> incrementally.

Probably yes. If we were doing "repack -a -d (without -f)" style of "incremental", then we do need to revisit this, which is a moral equivalent of saying "do not reuse delta" to "repack", and it would make it impossible to be incremental.

Hopefully a true "incremental" shouldn't even have to touch existing data, perhaps similar to "repack (without -a)", in which case the equation may be different. If we'd be computing reachability for only new things, there is nothing gained by allowing parse_commit() to peek into existing commit-graph file (by definition, these new things are not in there---that's the reason why we are incrementally extending the commit-graph by computing the reachability for them). I dunno.

Show 25 quoted lines
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 1ee00fa333..6db658ed66 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -377,7 +377,13 @@ corrupt_graph_verify() {
>  	test_must_fail git commit-graph verify 2>test_err &&
>  	grep -v "^+" test_err >err &&
>  	test_i18ngrep "$grepstr" err &&
> -	git status --short
> +	if test -z "$NO_WRITE_TEST_BACKUP"
> +	then
> +		cp $objdir/info/commit-graph commit-graph-pre-write-test
> +	fi &&
> +	git status --short &&
> +	GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&
> +	git commit-graph verify
>  }
>  
>  # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
> @@ -408,7 +414,7 @@ test_expect_success 'detect permission problem' '
>  	# "chmod 000 file" does not yield EACCES on e.g. "cat file"
>  	if ! test -r $objdir/info/commit-graph
>  	then
> -		corrupt_graph_verify "Could not open"
> +		NO_WRITE_TEST_BACKUP=1 corrupt_graph_verify "Could not open"

This would not work as you think it would; corrupt_graph_verify is a shell function, so you cannot VAR=VAL prefix to export an environment variable only for the duration of the command.

Show 10 quoted lines
>  	fi
>  '
>  
> @@ -528,6 +534,7 @@ test_expect_success 'git fsck (checks commit-graph)' '
>  	git fsck &&
>  	corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
>  		"incorrect checksum" &&
> +	cp commit-graph-pre-write-test $objdir/info/commit-graph &&
>  	test_must_fail git fsck
>  '
Eric Sunshine· Feb 23, 2019, 09:29 UTC · re: Junio C Hamano · lore

Re: [PATCH 7/8] commit-graph write: don't die if the existing graph is corrupt

On Fri, Feb 22, 2019 at 6:13 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> > @@ -408,7 +414,7 @@ test_expect_success 'detect permission problem' '
> >       # "chmod 000 file" does not yield EACCES on e.g. "cat file"
> >       if ! test -r $objdir/info/commit-graph
> >       then
> > -             corrupt_graph_verify "Could not open"
> > +             NO_WRITE_TEST_BACKUP=1 corrupt_graph_verify "Could not open"
>
> This would not work as you think it would; corrupt_graph_verify is a
> shell function, so you cannot VAR=VAL prefix to export an environment
> variable only for the duration of the command.

As of a0a630192d (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13), this sort of problem is correctly flagged by "make test-lint", which typically is run as part of "make test" or "make prove".

Ævar Arnfjörð Bjarmason· Feb 21, 2019, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 8/8] commit-graph: improve & i18n error messages

Change the error emitted when a commit-graph file is corrupt so that we actually mention the commit-graph, e.g. change errors like:

    error: improper chunk offset 0000000000385e0c
To:
    error: commit-graph improper chunk offset 0000000000385e0c

As discussed in the commits leading up to this one the commit-graph machinery is now used by common commands like "status". If the graph was corrupt we'd often emit some error that gave no indication what was wrong. Now some of them are still cryptic, but they'll at least mention "commit-graph" to give the user a hint as to where to look.

While I'm at it mark some of the strings that hadn't been marked for translation. It's clear from the commit history and the code that this was merely forgotten at the time, and wasn't intentional.p5

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit-graph.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)
Show changes to commit-graph.c +19 −19
diff --git a/commit-graph.c b/commit-graph.c
index 6b3ade9496..4cf7d6c294 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -150,21 +150,21 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 
 	graph_signature = get_be32(data);
 	if (graph_signature != GRAPH_SIGNATURE) {
-		error(_("graph signature %X does not match signature %X"),
+		error(_("commit-graph signature %X does not match signature %X"),
 		      graph_signature, GRAPH_SIGNATURE);
 		return NULL;
 	}
 
 	graph_version = *(unsigned char*)(data + 4);
 	if (graph_version != GRAPH_VERSION) {
-		error(_("graph version %X does not match version %X"),
+		error(_("commit-graph version %X does not match version %X"),
 		      graph_version, GRAPH_VERSION);
 		return NULL;
 	}
 
 	hash_version = *(unsigned char*)(data + 5);
 	if (hash_version != oid_version()) {
-		error(_("hash version %X does not match version %X"),
+		error(_("commit-graph hash version %X does not match version %X"),
 		      hash_version, oid_version());
 		return NULL;
 	}
@@ -187,7 +187,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 
 		if (data + graph_size - chunk_lookup <
 		    GRAPH_CHUNKLOOKUP_WIDTH) {
-			error(_("chunk lookup table entry missing; graph file may be incomplete"));
+			error(_("commit-graph chunk lookup table entry missing; file may be incomplete"));
 			free(graph);
 			return NULL;
 		}
@@ -198,7 +198,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 		chunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;
 
 		if (chunk_offset > graph_size - the_hash_algo->rawsz) {
-			error(_("improper chunk offset %08x%08x"), (uint32_t)(chunk_offset >> 32),
+			error(_("commit-graph improper chunk offset %08x%08x"), (uint32_t)(chunk_offset >> 32),
 			      (uint32_t)chunk_offset);
 			free(graph);
 			return NULL;
@@ -235,7 +235,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 		}
 
 		if (chunk_repeated) {
-			error(_("chunk id %08x appears multiple times"), chunk_id);
+			error(_("commit-graph chunk id %08x appears multiple times"), chunk_id);
 			free(graph);
 			return NULL;
 		}
@@ -1162,7 +1162,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
 
 		if (i && oidcmp(&prev_oid, &cur_oid) >= 0)
-			graph_report("commit-graph has incorrect OID order: %s then %s",
+			graph_report(_("commit-graph has incorrect OID order: %s then %s"),
 				     oid_to_hex(&prev_oid),
 				     oid_to_hex(&cur_oid));
 
@@ -1172,14 +1172,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 			uint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);
 
 			if (i != fanout_value)
-				graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",
+				graph_report(_("commit-graph has incorrect fanout value: fanout[%d] = %u != %u"),
 					     cur_fanout_pos, fanout_value, i);
 			cur_fanout_pos++;
 		}
 
 		graph_commit = lookup_commit(r, &cur_oid);
 		if (!parse_commit_in_graph_one(r, g, graph_commit))
-			graph_report("failed to parse %s from commit-graph",
+			graph_report(_("failed to parse commit %s from commit-graph"),
 				     oid_to_hex(&cur_oid));
 	}
 
@@ -1187,7 +1187,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		uint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);
 
 		if (g->num_commits != fanout_value)
-			graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",
+			graph_report(_("commit-graph has incorrect fanout value: fanout[%d] = %u != %u"),
 				     cur_fanout_pos, fanout_value, i);
 
 		cur_fanout_pos++;
@@ -1209,14 +1209,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		graph_commit = lookup_commit(r, &cur_oid);
 		odb_commit = (struct commit *)create_object(r, cur_oid.hash, alloc_commit_node(r));
 		if (parse_commit_internal(odb_commit, 0, 0)) {
-			graph_report("failed to parse %s from object database",
+			graph_report(_("failed to parse commit %s from object database for commit-graph"),
 				     oid_to_hex(&cur_oid));
 			continue;
 		}
 
 		if (!oideq(&get_commit_tree_in_graph_one(r, g, graph_commit)->object.oid,
 			   get_commit_tree_oid(odb_commit)))
-			graph_report("root tree OID for commit %s in commit-graph is %s != %s",
+			graph_report(_("root tree OID for commit %s in commit-graph is %s != %s"),
 				     oid_to_hex(&cur_oid),
 				     oid_to_hex(get_commit_tree_oid(graph_commit)),
 				     oid_to_hex(get_commit_tree_oid(odb_commit)));
@@ -1226,13 +1226,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 
 		while (graph_parents) {
 			if (odb_parents == NULL) {
-				graph_report("commit-graph parent list for commit %s is too long",
+				graph_report(_("commit-graph parent list for commit %s is too long"),
 					     oid_to_hex(&cur_oid));
 				break;
 			}
 
 			if (!oideq(&graph_parents->item->object.oid, &odb_parents->item->object.oid))
-				graph_report("commit-graph parent for %s is %s != %s",
+				graph_report(_("commit-graph parent for %s is %s != %s"),
 					     oid_to_hex(&cur_oid),
 					     oid_to_hex(&graph_parents->item->object.oid),
 					     oid_to_hex(&odb_parents->item->object.oid));
@@ -1245,16 +1245,16 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		}
 
 		if (odb_parents != NULL)
-			graph_report("commit-graph parent list for commit %s terminates early",
+			graph_report(_("commit-graph parent list for commit %s terminates early"),
 				     oid_to_hex(&cur_oid));
 
 		if (!graph_commit->generation) {
 			if (generation_zero == GENERATION_NUMBER_EXISTS)
-				graph_report("commit-graph has generation number zero for commit %s, but non-zero elsewhere",
+				graph_report(_("commit-graph has generation number zero for commit %s, but non-zero elsewhere"),
 					     oid_to_hex(&cur_oid));
 			generation_zero = GENERATION_ZERO_EXISTS;
 		} else if (generation_zero == GENERATION_ZERO_EXISTS)
-			graph_report("commit-graph has non-zero generation number for commit %s, but zero elsewhere",
+			graph_report(_("commit-graph has non-zero generation number for commit %s, but zero elsewhere"),
 				     oid_to_hex(&cur_oid));
 
 		if (generation_zero == GENERATION_ZERO_EXISTS)
@@ -1269,13 +1269,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 			max_generation--;
 
 		if (graph_commit->generation != max_generation + 1)
-			graph_report("commit-graph generation for commit %s is %u != %u",
+			graph_report(_("commit-graph generation for commit %s is %u != %u"),
 				     oid_to_hex(&cur_oid),
 				     graph_commit->generation,
 				     max_generation + 1);
 
 		if (graph_commit->date != odb_commit->date)
-			graph_report("commit date for commit %s in commit-graph is %"PRItime" != %"PRItime,
+			graph_report(_("commit date for commit %s in commit-graph is %"PRItime" != %"PRItime),
 				     oid_to_hex(&cur_oid),
 				     graph_commit->date,
 				     odb_commit->date);

-- 
2.21.0.rc0.258.g878e2cd30e
Ævar Arnfjörð Bjarmason· Mar 14, 2019, 21:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 0/8] commit-graph: segfault & other fixes for broken graphs

See the v1 cover letter for details: https://public-inbox.org/git/20190221223753.20070-1-avarab@gmail.com/

I'd forgotten this after 2.21 was released.

This addresses all the comments on v1 and rebases it. A range-diff is below. I also improved 7/8's commit message a bit.

I fixed a test to avoid the pattern a0a630192d (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13) tests for. The new pattern is more obvious.

As an aside I don't get how that doesn't work as intended from the commit message of that commit & its series.

    $ cat /tmp/x.sh 
    sayit() { echo "saying '$SAY'"; };
    SAY=hi sayit; sayit;
    $ sh /tmp/x.sh
    saying 'hi'
    saying ''

I get the same thing on bash, dash, NetBSD ksh, Solaris's shell & AIX's. I.e. it's explicitly not assigning $SAY for the duration of the shell as this would do:

    $ cat /tmp/y.sh 
    sayit() { echo "saying '$SAY'"; };
    SAY=hi; sayit; sayit;
    $ sh /tmp/y.sh
    saying 'hi'
    saying 'hi'
Which is the impression I get from the commit message.
Ævar Arnfjörð Bjarmason (8):
  commit-graph tests: split up corrupt_graph_and_verify()
  commit-graph tests: test a graph that's too small
  commit-graph: fix segfault on e.g. "git status"
  commit-graph: don't early exit(1) on e.g. "git status"
  commit-graph: don't pass filename to load_commit_graph_one_fd_st()
  commit-graph verify: detect inability to read the graph
  commit-graph write: don't die if the existing graph is corrupt
  commit-graph: improve & i18n error messages
 builtin/commit-graph.c  |  23 +++++--
 commit-graph.c          | 132 +++++++++++++++++++++++++++-------------
 commit-graph.h          |   4 ++
 commit.h                |   6 ++
 t/t5318-commit-graph.sh |  42 +++++++++++--
 5 files changed, 154 insertions(+), 53 deletions(-)
Range-diff:
1:  9d318d5106 ! 1:  2f8ba0adf8 commit-graph tests: split up corrupt_graph_and_verify()
    @@ -49,7 +49,7 @@
     -	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
     -	cp $objdir/info/commit-graph commit-graph-backup &&
      	printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
    - 	dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 &&
    + 	dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" if=/dev/null &&
      	generate_zero_bytes $(($orig_size - $zero_pos)) >>"$objdir/info/commit-graph" &&
     -	test_must_fail git commit-graph verify 2>test_err &&
     -	grep -v "^+" test_err >err &&
2:  73849add5e = 2:  800b17edde commit-graph tests: test a graph that's too small
3:  6bfce758e1 = 3:  7083ab81c7 commit-graph: fix segfault on e.g. "git status"
4:  ac07ff415e = 4:  d00564ae89 commit-graph: don't early exit(1) on e.g. "git status"
5:  b2dd394cc7 = 5:  25ee185bf7 commit-graph: don't pass filename to load_commit_graph_one_fd_st()
6:  9987149e5c ! 6:  7619b46987 commit-graph verify: detect inability to read the graph
    @@ -37,16 +37,10 @@
      
      }
      
    -+test_expect_success 'detect permission problem' '
    ++test_expect_success POSIXPERM,SANITY 'detect permission problem' '
     +	corrupt_graph_setup &&
     +	chmod 000 $objdir/info/commit-graph &&
    -+
    -+	# Skip as root, or in other cases (odd fs or OS) where a
    -+	# "chmod 000 file" does not yield EACCES on e.g. "cat file"
    -+	if ! test -r $objdir/info/commit-graph
    -+	then
    -+		corrupt_graph_verify "Could not open"
    -+	fi
    ++	corrupt_graph_verify "Could not open"
     +'
     +
      test_expect_success 'detect too small' '
7:  0e35b12a1a ! 7:  17ee4fc050 commit-graph write: don't die if the existing graph is corrupt
    @@ -18,6 +18,10 @@
         use_commit_graph=1 as seen in 177722b344 ("commit: integrate commit
         graph with commit parsing", 2018-04-10).
     
    +    Not using the old graph at all slows down the writing of the new graph
    +    by some small amount, but is a sensible way to prevent an error in the
    +    existing commit-graph from spreading.
    +
         Just fixing the current issue would be likely to result in code that's
         inadvertently broken in the future. New code might use the
         commit-graph at a distance. To detect such cases introduce a
    @@ -36,7 +40,12 @@
         corruption.
     
         This might need to be re-visited if we learn to write the commit-graph
    -    incrementally.
    +    incrementally, but probably not. Hopefully we'll just start by finding
    +    out what commits we have in total, then read the old graph(s) to see
    +    what they cover, and finally write a new graph file with everything
    +    that's missing. In that case the new graph writing code just needs to
    +    continue to use e.g. a parse_commit() that doesn't consult the
    +    existing commit-graphs.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ -119,7 +128,7 @@
      	grep -v "^+" test_err >err &&
      	test_i18ngrep "$grepstr" err &&
     -	git status --short
    -+	if test -z "$NO_WRITE_TEST_BACKUP"
    ++	if test "$2" != "no-copy"
     +	then
     +		cp $objdir/info/commit-graph commit-graph-pre-write-test
     +	fi &&
    @@ -130,14 +139,14 @@
      
      # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
     @@
    - 	# "chmod 000 file" does not yield EACCES on e.g. "cat file"
    - 	if ! test -r $objdir/info/commit-graph
    - 	then
    --		corrupt_graph_verify "Could not open"
    -+		NO_WRITE_TEST_BACKUP=1 corrupt_graph_verify "Could not open"
    - 	fi
    + test_expect_success POSIXPERM,SANITY 'detect permission problem' '
    + 	corrupt_graph_setup &&
    + 	chmod 000 $objdir/info/commit-graph &&
    +-	corrupt_graph_verify "Could not open"
    ++	corrupt_graph_verify "Could not open" "no-copy"
      '
      
    + test_expect_success 'detect too small' '
     @@
      	git fsck &&
      	corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
8:  a74d0f0f6f = 8:  29ab2895b7 commit-graph: improve & i18n error messages
-- 
2.21.0.360.g471c308f928
Eric Sunshine· Mar 15, 2019, 07:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2 0/8] commit-graph: segfault & other fixes for broken graphs

On Thu, Mar 14, 2019 at 5:47 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 17 quoted lines
> I fixed a test to avoid the pattern a0a630192d
> (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13)
> tests for. The new pattern is more obvious.
>
> As an aside I don't get how that doesn't work as intended from the
> commit message of that commit & its series.
>
>     $ cat /tmp/x.sh
>     sayit() { echo "saying '$SAY'"; };
>     SAY=hi sayit; sayit;
>     $ sh /tmp/x.sh
>     saying 'hi'
>     saying ''
>
> I get the same thing on bash, dash, NetBSD ksh, Solaris's shell &
> AIX's. I.e. it's explicitly not assigning $SAY for the duration of the
> shell [...]

The shells you tested may all behave "sanely" given that input, but not all shells do. For instance, on MacOS, the factory-supplied bash 3.2.57 behaves in the "broken" way in 'sh' compatibility mode:

$ cat /tmp/x.sh sayit() { echo "saying '$SAY'"; }; SAY=hi sayit; sayit;

$ /bin/sh /tmp/x.sh saying 'hi' saying 'hi'

$ /bin/bash /tmp/x.sh saying 'hi' saying ''

Ævar Arnfjörð Bjarmason· Mar 15, 2019, 10:39 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 0/8] commit-graph: segfault & other fixes for broken graphs

On Fri, Mar 15 2019, Eric Sunshine wrote:
Show 35 quoted lines
> On Thu, Mar 14, 2019 at 5:47 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> I fixed a test to avoid the pattern a0a630192d
>> (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13)
>> tests for. The new pattern is more obvious.
>>
>> As an aside I don't get how that doesn't work as intended from the
>> commit message of that commit & its series.
>>
>>     $ cat /tmp/x.sh
>>     sayit() { echo "saying '$SAY'"; };
>>     SAY=hi sayit; sayit;
>>     $ sh /tmp/x.sh
>>     saying 'hi'
>>     saying ''
>>
>> I get the same thing on bash, dash, NetBSD ksh, Solaris's shell &
>> AIX's. I.e. it's explicitly not assigning $SAY for the duration of the
>> shell [...]
>
> The shells you tested may all behave "sanely" given that input, but
> not all shells do. For instance, on MacOS, the factory-supplied bash
> 3.2.57 behaves in the "broken" way in 'sh' compatibility mode:
>
> $ cat /tmp/x.sh
> sayit() { echo "saying '$SAY'"; };
> SAY=hi sayit; sayit;
>
> $ /bin/sh /tmp/x.sh
> saying 'hi'
> saying 'hi'
>
> $ /bin/bash /tmp/x.sh
> saying 'hi'
> saying ''

Thanks. Also I found (with help from Freenode ##POSIX) that this part of the spec[1] talks about it:

BEGIN QUOTE
    Variable assignments shall be performed as follows:
    [...]
    * If the command name is a function that is not a standard utility
      implemented as a function, variable assignments shall affect the
      current execution environment during the execution of the
      function. It is unspecified:
      - Whether or not the variable assignments persist after the
        completion of the function
      - Whether or not the variables gain the export attribute during
        the execution of the function
      - Whether or not export attributes gained as a result of the
        variable assignments persist after the completion of the
        function (if variable assignments persist after the completion
        of the function)
END QUOTE

I.e. this is undefined behavior that just happened to work on the various shells I tested. Makes sense, was just wondering if I'd entirely missed out on what behavior was, turns out it's just fairly rare.

FWIW My bash 5.0.2 on Debian behaves as I noted in both sh and bash mode.

1. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
Ævar Arnfjörð Bjarmason· Mar 25, 2019, 12:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 0/8] commit-graph: segfault & other fixes for broken graphs

This v3 fixes issues raised by Ramsay in https://public-inbox.org/git/1908832c-8dd0-377e-917b-acb33b00273c@ramsayjones.plus.com/

While I was at it I changed a die("Whatevs") to die("whatevs"). I.e. start with lower-case as discussed in other places on-list recently.

The range-diff looks scarier than this v2..v3 really is. There's no code changes aside from the s/0/NULL/ in one place, but marking a couple of functions as "static" required moving them around, and removing their entry from the corresponding *.h file.

Ævar Arnfjörð Bjarmason (8):
  commit-graph tests: split up corrupt_graph_and_verify()
  commit-graph tests: test a graph that's too small
  commit-graph: fix segfault on e.g. "git status"
  commit-graph: don't early exit(1) on e.g. "git status"
  commit-graph: don't pass filename to load_commit_graph_one_fd_st()
  commit-graph verify: detect inability to read the graph
  commit-graph write: don't die if the existing graph is corrupt
  commit-graph: improve & i18n error messages
 builtin/commit-graph.c  |  23 +++++--
 commit-graph.c          | 132 +++++++++++++++++++++++++++-------------
 commit-graph.h          |   4 +-
 commit.h                |   6 ++
 t/t5318-commit-graph.sh |  42 +++++++++++--
 5 files changed, 153 insertions(+), 54 deletions(-)
Range-diff:
1:  2f8ba0adf8 = 1:  83ff92a39d commit-graph tests: split up corrupt_graph_and_verify()
2:  800b17edde = 2:  b9170c35e6 commit-graph tests: test a graph that's too small
3:  7083ab81c7 ! 3:  daf38a9af7 commit-graph: fix segfault on e.g. "git status"
    @@ -58,20 +58,10 @@
      --- a/commit-graph.c
      +++ b/commit-graph.c
     @@
    - 		last_chunk_offset = chunk_offset;
    - 	}
    - 
    -+	if (verify_commit_graph_lite(graph))
    -+		return NULL;
    -+
    - 	return graph;
    + 	return ret;
      }
      
    -@@
    - #define GENERATION_ZERO_EXISTS 1
    - #define GENERATION_NUMBER_EXISTS 2
    - 
    -+int verify_commit_graph_lite(struct commit_graph *g)
    ++static int verify_commit_graph_lite(struct commit_graph *g)
     +{
     +	/*
     +	 * Basic validation shared between parse_commit_graph()
    @@ -101,9 +91,19 @@
     +	return 0;
     +}
     +
    - int verify_commit_graph(struct repository *r, struct commit_graph *g)
    + struct commit_graph *parse_commit_graph(void *graph_map, int fd,
    + 					size_t graph_size)
      {
    - 	uint32_t i, cur_fanout_pos = 0;
    +@@
    + 		last_chunk_offset = chunk_offset;
    + 	}
    + 
    ++	if (verify_commit_graph_lite(graph))
    ++		return NULL;
    ++
    + 	return graph;
    + }
    + 
     @@
      		return 1;
      	}
    @@ -122,18 +122,6 @@
      		return verify_commit_graph_error;
      
     
    - diff --git a/commit-graph.h b/commit-graph.h
    - --- a/commit-graph.h
    - +++ b/commit-graph.h
    -@@
    - 			struct string_list *commit_hex,
    - 			int append, int report_progress);
    - 
    -+int verify_commit_graph_lite(struct commit_graph *g);
    - int verify_commit_graph(struct repository *r, struct commit_graph *g);
    - 
    - void close_commit_graph(struct repository *);
    -
      diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
      --- a/t/t5318-commit-graph.sh
      +++ b/t/t5318-commit-graph.sh
4:  d00564ae89 ! 4:  3d7d8c4deb commit-graph: don't early exit(1) on e.g. "git status"
    @@ -29,7 +29,15 @@
         passed to that function for the the "graph file %s is too small" error
         message.
     
    +    This leaves load_commit_graph_one() unused by everything except the
    +    internal prepare_commit_graph_one() function, so let's mark it as
    +    "static". If someone needs it in the future we can remove the "static"
    +    attribute. I could also rewrite its sole remaining
    +    user ("prepare_commit_graph_one()") to use
    +    load_commit_graph_one_fd_st() instead, but let's leave it at this.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +    Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
     
      diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
      --- a/builtin/commit-graph.c
    @@ -131,7 +139,7 @@
      		close(fd);
     -		die(_("graph file %s is too small"), graph_file);
     +		error(_("graph file %s is too small"), graph_file);
    -+		return 0;
    ++		return NULL;
      	}
      	graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
      	ret = parse_commit_graph(graph_map, fd, graph_size);
    @@ -143,9 +151,11 @@
      	}
      
      	return ret;
    +@@
    + 	return graph;
      }
      
    -+struct commit_graph *load_commit_graph_one(const char *graph_file)
    ++static struct commit_graph *load_commit_graph_one(const char *graph_file)
     +{
     +
     +	struct stat st;
    @@ -158,9 +168,9 @@
     +	return load_commit_graph_one_fd_st(graph_file, fd, &st);
     +}
     +
    - struct commit_graph *parse_commit_graph(void *graph_map, int fd,
    - 					size_t graph_size)
    + static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
      {
    + 	char *graph_name;
     
      diff --git a/commit-graph.h b/commit-graph.h
      --- a/commit-graph.h
    @@ -174,9 +184,10 @@
      /*
       * Given a commit struct, try to fill the commit struct info, including:
     @@
    + 	const unsigned char *chunk_extra_edges;
      };
      
    - struct commit_graph *load_commit_graph_one(const char *graph_file);
    +-struct commit_graph *load_commit_graph_one(const char *graph_file);
     +struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
     +						 int fd, struct stat *st);
      
5:  25ee185bf7 ! 5:  15687fb21f commit-graph: don't pass filename to load_commit_graph_one_fd_st()
    @@ -59,7 +59,7 @@
      		close(fd);
     -		error(_("graph file %s is too small"), graph_file);
     +		error(_("commit-graph file is too small"));
    - 		return 0;
    + 		return NULL;
      	}
      	graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
     @@
    @@ -70,15 +70,15 @@
     +	return load_commit_graph_one_fd_st(fd, &st);
      }
      
    - struct commit_graph *parse_commit_graph(void *graph_map, int fd,
    + static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
     
      diff --git a/commit-graph.h b/commit-graph.h
      --- a/commit-graph.h
      +++ b/commit-graph.h
     @@
    + 	const unsigned char *chunk_extra_edges;
      };
      
    - struct commit_graph *load_commit_graph_one(const char *graph_file);
     -struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
     -						 int fd, struct stat *st);
     +struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);
6:  7619b46987 = 6:  30145c2dca commit-graph verify: detect inability to read the graph
7:  17ee4fc050 ! 7:  51e3aa651b commit-graph write: don't die if the existing graph is corrupt
    @@ -57,7 +57,7 @@
      	int config_value;
      
     +	if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))
    -+		die("Dying as requested by the '%s' variable on commit-graph load!",
    ++		die("dying as requested by the '%s' variable on commit-graph load!",
     +		    GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);
     +
      	if (r->objects->commit_graph_attempted)
8:  29ab2895b7 = 8:  e7c801f73b commit-graph: improve & i18n error messages
-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 25, 2019, 12:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 1/8] commit-graph tests: split up corrupt_graph_and_verify()

Split up the corrupt_graph_and_verify() function added in d9b9f8a6fd ("commit-graph: verify catches corrupt signature", 2018-06-27) into its logical components of setting up the test itself, doing the corruption in a particular way with "dd", and then finally testing that stderr is what we expect.

This allows for re-using everything except the now slimmer corrupt_graph_and_verify() to corrupt the graph in a way that doesn't involve inserting a given byte sequence at a given position, e.g. truncating it entirely to a custom value.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5318-commit-graph.sh | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)
Show changes to t/t5318-commit-graph.sh +16 −6
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 561796f280..56a616831e 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -366,6 +366,19 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \
 GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))
 GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
 
+corrupt_graph_setup() {
+	cd "$TRASH_DIRECTORY/full" &&
+	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
+	cp $objdir/info/commit-graph commit-graph-backup
+}
+
+corrupt_graph_verify() {
+	grepstr=$1
+	test_must_fail git commit-graph verify 2>test_err &&
+	grep -v "^+" test_err >err &&
+	test_i18ngrep "$grepstr" err
+}
+
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
 # Manipulates the commit-graph file at the position
 # by inserting the data, optionally zeroing the file
@@ -376,17 +389,14 @@ corrupt_graph_and_verify() {
 	pos=$1
 	data="${2:-\0}"
 	grepstr=$3
-	cd "$TRASH_DIRECTORY/full" &&
+	corrupt_graph_setup &&
 	orig_size=$(wc -c < $objdir/info/commit-graph) &&
 	zero_pos=${4:-${orig_size}} &&
-	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
-	cp $objdir/info/commit-graph commit-graph-backup &&
 	printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
 	dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" if=/dev/null &&
 	generate_zero_bytes $(($orig_size - $zero_pos)) >>"$objdir/info/commit-graph" &&
-	test_must_fail git commit-graph verify 2>test_err &&
-	grep -v "^+" test_err >err &&
-	test_i18ngrep "$grepstr" err
+	corrupt_graph_verify "$grepstr"
+
 }
 
 test_expect_success 'detect bad signature' '

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 25, 2019, 12:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 2/8] commit-graph tests: test a graph that's too small

Use the recently split-up components of the corrupt_graph_and_verify() function to assert that we error on graphs that are too small. The error was added in 2a2e32bdc5 ("commit-graph: implement git commit-graph read", 2018-04-10), but there was no test for it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5318-commit-graph.sh | 6 ++++++
 1 file changed, 6 insertions(+)
Show changes to t/t5318-commit-graph.sh +6 −0
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 56a616831e..ce3459a6f5 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -399,6 +399,12 @@ corrupt_graph_and_verify() {
 
 }
 
+test_expect_success 'detect too small' '
+	corrupt_graph_setup &&
+	echo "a small graph" >$objdir/info/commit-graph &&
+	corrupt_graph_verify "too small"
+'
+
 test_expect_success 'detect bad signature' '
 	corrupt_graph_and_verify 0 "\0" \
 		"graph signature"

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 25, 2019, 12:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 3/8] commit-graph: fix segfault on e.g. "git status"

When core.commitGraph=true is set, various common commands now consult the commit graph. Because the commit-graph code is very trusting of its input data, it's possibly to construct a graph that'll cause an immediate segfault on e.g. "status" (and e.g. "log", "blame", ...). In some other cases where git immediately exits with a cryptic error about the graph being broken.

The root cause of this is that while the "commit-graph verify" sub-command exhaustively verifies the graph, other users of the graph simply trust the graph, and will e.g. deference data found at certain offsets as pointers, causing segfaults.

This change does the bare minimum to ensure that we don't segfault in the common fill_commit_in_graph() codepath called by e.g. setup_revisions(), to do this instrument the "commit-graph verify" tests to always check if "status" would subsequently segfault. This fixes the following tests which would previously segfault:

    not ok 50 - detect low chunk count
    not ok 51 - detect missing OID fanout chunk
    not ok 52 - detect missing OID lookup chunk
    not ok 53 - detect missing commit data chunk

Those happened because with the commit-graph enabled setup_revisions() would eventually call fill_commit_in_graph(), where e.g. g->chunk_commit_data is used early as an offset (and will be 0x0). With this change we get far enough to detect that the graph is broken, and show an error instead. E.g.:

    $ git status; echo $?
    error: commit-graph is missing the Commit Data chunk
    1

That also sucks, we should *warn* and not hard-fail "status" just because the commit-graph is corrupt, but fixing is left to a follow-up change.

A side-effect of changing the reporting from graph_report() to error() is that we now have an "error: " prefix for these even for "commit-graph verify". Pseudo-diff before/after:

    $ git commit-graph verify
    -commit-graph is missing the Commit Data chunk
    +error: commit-graph is missing the Commit Data chunk

Changing that is OK. Various errors it emits now early on are prefixed with "error: ", moving these over and changing the output doesn't break anything.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit-graph.c          | 43 ++++++++++++++++++++++++++++++++---------
 t/t5318-commit-graph.sh |  3 ++-
 2 files changed, 36 insertions(+), 10 deletions(-)
Show changes to 2 files +36 −10

commit-graph.c, t/t5318-commit-graph.sh

diff --git a/commit-graph.c b/commit-graph.c
index 47e9be0a3a..f8201d888b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -112,6 +112,36 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
 	return ret;
 }
 
+static int verify_commit_graph_lite(struct commit_graph *g)
+{
+	/*
+	 * Basic validation shared between parse_commit_graph()
+	 * which'll be called every time the graph is used, and the
+	 * much more expensive verify_commit_graph() used by
+	 * "commit-graph verify".
+	 *
+	 * There should only be very basic checks here to ensure that
+	 * we don't e.g. segfault in fill_commit_in_graph(), but
+	 * because this is a very hot codepath nothing that e.g. loops
+	 * over g->num_commits, or runs a checksum on the commit-graph
+	 * itself.
+	 */
+	if (!g->chunk_oid_fanout) {
+		error("commit-graph is missing the OID Fanout chunk");
+		return 1;
+	}
+	if (!g->chunk_oid_lookup) {
+		error("commit-graph is missing the OID Lookup chunk");
+		return 1;
+	}
+	if (!g->chunk_commit_data) {
+		error("commit-graph is missing the Commit Data chunk");
+		return 1;
+	}
+
+	return 0;
+}
+
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 					size_t graph_size)
 {
@@ -233,6 +263,9 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 		last_chunk_offset = chunk_offset;
 	}
 
+	if (verify_commit_graph_lite(graph))
+		return NULL;
+
 	return graph;
 }
 
@@ -1089,15 +1122,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		return 1;
 	}
 
-	verify_commit_graph_error = 0;
-
-	if (!g->chunk_oid_fanout)
-		graph_report("commit-graph is missing the OID Fanout chunk");
-	if (!g->chunk_oid_lookup)
-		graph_report("commit-graph is missing the OID Lookup chunk");
-	if (!g->chunk_commit_data)
-		graph_report("commit-graph is missing the Commit Data chunk");
-
+	verify_commit_graph_error = verify_commit_graph_lite(g);
 	if (verify_commit_graph_error)
 		return verify_commit_graph_error;
 
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index ce3459a6f5..ad3a695f76 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -376,7 +376,8 @@ corrupt_graph_verify() {
 	grepstr=$1
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
-	test_i18ngrep "$grepstr" err
+	test_i18ngrep "$grepstr" err &&
+	test_might_fail git status --short
 }
 
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 25, 2019, 12:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 4/8] commit-graph: don't early exit(1) on e.g. "git status"

Make the commit-graph loading code work as a library that returns an error code instead of calling exit(1) when the commit-graph is corrupt. This means that e.g. "status" will now report commit-graph corruption as an "error: [...]" at the top of its output, but then proceed to work normally.

This required splitting up the load_commit_graph_one() function so that the code that deals with open()-ing and stat()-ing the graph can now be called independently as open_commit_graph().

This is needed because "commit-graph verify" where the graph doesn't exist isn't an error. See the third paragraph in 283e68c72f ("commit-graph: add 'verify' subcommand", 2018-06-27). There's a bug in that logic where we conflate the intended ENOENT with other errno values (e.g. EACCES), but this change doesn't address that. That'll be addressed in a follow-up change.

I'm then splitting most of the logic out of load_commit_graph_one() into load_commit_graph_one_fd_st(), which allows for providing an existing file descriptor and stat information to the loading code. This isn't strictly needed, but it would be redundant and confusing to open() and stat() the file twice for some of the codepaths, this allows for calling open_commit_graph() followed by load_commit_graph_one_fd_st(). The "graph_file" still needs to be passed to that function for the the "graph file %s is too small" error message.

This leaves load_commit_graph_one() unused by everything except the internal prepare_commit_graph_one() function, so let's mark it as "static". If someone needs it in the future we can remove the "static" attribute. I could also rewrite its sole remaining user ("prepare_commit_graph_one()") to use load_commit_graph_one_fd_st() instead, but let's leave it at this.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 builtin/commit-graph.c  | 21 +++++++++++++++++----
 commit-graph.c          | 42 +++++++++++++++++++++++++++++------------
 commit-graph.h          |  4 +++-
 t/t5318-commit-graph.sh |  2 +-
 4 files changed, 51 insertions(+), 18 deletions(-)
Show changes to 4 files +51 −18

builtin/commit-graph.c, commit-graph.c, commit-graph.h, t/t5318-commit-graph.sh

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 4ae502754c..32bcc63427 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -42,6 +42,9 @@ static int graph_verify(int argc, const char **argv)
 {
 	struct commit_graph *graph = NULL;
 	char *graph_name;
+	int open_ok;
+	int fd;
+	struct stat st;
 
 	static struct option builtin_commit_graph_verify_options[] = {
 		OPT_STRING(0, "object-dir", &opts.obj_dir,
@@ -58,11 +61,14 @@ static int graph_verify(int argc, const char **argv)
 		opts.obj_dir = get_object_directory();
 
 	graph_name = get_commit_graph_filename(opts.obj_dir);
-	graph = load_commit_graph_one(graph_name);
+	open_ok = open_commit_graph(graph_name, &fd, &st);
+	if (!open_ok)
+		return 0;
+	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
 	FREE_AND_NULL(graph_name);
 
 	if (!graph)
-		return 0;
+		return 1;
 
 	UNLEAK(graph);
 	return verify_commit_graph(the_repository, graph);
@@ -72,6 +78,9 @@ static int graph_read(int argc, const char **argv)
 {
 	struct commit_graph *graph = NULL;
 	char *graph_name;
+	int open_ok;
+	int fd;
+	struct stat st;
 
 	static struct option builtin_commit_graph_read_options[] = {
 		OPT_STRING(0, "object-dir", &opts.obj_dir,
@@ -88,10 +97,14 @@ static int graph_read(int argc, const char **argv)
 		opts.obj_dir = get_object_directory();
 
 	graph_name = get_commit_graph_filename(opts.obj_dir);
-	graph = load_commit_graph_one(graph_name);
 
+	open_ok = open_commit_graph(graph_name, &fd, &st);
+	if (!open_ok)
+		die_errno(_("Could not open commit-graph '%s'"), graph_name);
+
+	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
 	if (!graph)
-		die("graph file %s does not exist", graph_name);
+		return 1;
 
 	FREE_AND_NULL(graph_name);
 
diff --git a/commit-graph.c b/commit-graph.c
index f8201d888b..3acc032c1b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -80,25 +80,31 @@ static int commit_graph_compatible(struct repository *r)
 	return 1;
 }
 
-struct commit_graph *load_commit_graph_one(const char *graph_file)
+int open_commit_graph(const char *graph_file, int *fd, struct stat *st)
+{
+	*fd = git_open(graph_file);
+	if (*fd < 0)
+		return 0;
+	if (fstat(*fd, st)) {
+		close(*fd);
+		return 0;
+	}
+	return 1;
+}
+
+struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
+						 int fd, struct stat *st)
 {
 	void *graph_map;
 	size_t graph_size;
-	struct stat st;
 	struct commit_graph *ret;
-	int fd = git_open(graph_file);
 
-	if (fd < 0)
-		return NULL;
-	if (fstat(fd, &st)) {
-		close(fd);
-		return NULL;
-	}
-	graph_size = xsize_t(st.st_size);
+	graph_size = xsize_t(st->st_size);
 
 	if (graph_size < GRAPH_MIN_SIZE) {
 		close(fd);
-		die(_("graph file %s is too small"), graph_file);
+		error(_("graph file %s is too small"), graph_file);
+		return NULL;
 	}
 	graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	ret = parse_commit_graph(graph_map, fd, graph_size);
@@ -106,7 +112,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
 	if (!ret) {
 		munmap(graph_map, graph_size);
 		close(fd);
-		exit(1);
 	}
 
 	return ret;
@@ -269,6 +274,19 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 	return graph;
 }
 
+static struct commit_graph *load_commit_graph_one(const char *graph_file)
+{
+
+	struct stat st;
+	int fd;
+	int open_ok = open_commit_graph(graph_file, &fd, &st);
+
+	if (!open_ok)
+		return NULL;
+
+	return load_commit_graph_one_fd_st(graph_file, fd, &st);
+}
+
 static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
 {
 	char *graph_name;
diff --git a/commit-graph.h b/commit-graph.h
index 096d8bac34..77cc739bc0 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -11,6 +11,7 @@
 struct commit;
 
 char *get_commit_graph_filename(const char *obj_dir);
+int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
 
 /*
  * Given a commit struct, try to fill the commit struct info, including:
@@ -52,7 +53,8 @@ struct commit_graph {
 	const unsigned char *chunk_extra_edges;
 };
 
-struct commit_graph *load_commit_graph_one(const char *graph_file);
+struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
+						 int fd, struct stat *st);
 
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 					size_t graph_size);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index ad3a695f76..71d775e313 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -377,7 +377,7 @@ corrupt_graph_verify() {
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
 	test_i18ngrep "$grepstr" err &&
-	test_might_fail git status --short
+	git status --short
 }
 
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Apr 27, 2019, 13:06 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v3 4/8] commit-graph: don't early exit(1) on e.g. "git status"

On Mon, Mar 25 2019, Ævar Arnfjörð Bjarmason wrote:
Show 211 quoted lines
> Make the commit-graph loading code work as a library that returns an
> error code instead of calling exit(1) when the commit-graph is
> corrupt. This means that e.g. "status" will now report commit-graph
> corruption as an "error: [...]" at the top of its output, but then
> proceed to work normally.
>
> This required splitting up the load_commit_graph_one() function so
> that the code that deals with open()-ing and stat()-ing the graph can
> now be called independently as open_commit_graph().
>
> This is needed because "commit-graph verify" where the graph doesn't
> exist isn't an error. See the third paragraph in
> 283e68c72f ("commit-graph: add 'verify' subcommand",
> 2018-06-27). There's a bug in that logic where we conflate the
> intended ENOENT with other errno values (e.g. EACCES), but this change
> doesn't address that. That'll be addressed in a follow-up change.
>
> I'm then splitting most of the logic out of load_commit_graph_one()
> into load_commit_graph_one_fd_st(), which allows for providing an
> existing file descriptor and stat information to the loading
> code. This isn't strictly needed, but it would be redundant and
> confusing to open() and stat() the file twice for some of the
> codepaths, this allows for calling open_commit_graph() followed by
> load_commit_graph_one_fd_st(). The "graph_file" still needs to be
> passed to that function for the the "graph file %s is too small" error
> message.
>
> This leaves load_commit_graph_one() unused by everything except the
> internal prepare_commit_graph_one() function, so let's mark it as
> "static". If someone needs it in the future we can remove the "static"
> attribute. I could also rewrite its sole remaining
> user ("prepare_commit_graph_one()") to use
> load_commit_graph_one_fd_st() instead, but let's leave it at this.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>  builtin/commit-graph.c  | 21 +++++++++++++++++----
>  commit-graph.c          | 42 +++++++++++++++++++++++++++++------------
>  commit-graph.h          |  4 +++-
>  t/t5318-commit-graph.sh |  2 +-
>  4 files changed, 51 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 4ae502754c..32bcc63427 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -42,6 +42,9 @@ static int graph_verify(int argc, const char **argv)
>  {
>  	struct commit_graph *graph = NULL;
>  	char *graph_name;
> +	int open_ok;
> +	int fd;
> +	struct stat st;
>
>  	static struct option builtin_commit_graph_verify_options[] = {
>  		OPT_STRING(0, "object-dir", &opts.obj_dir,
> @@ -58,11 +61,14 @@ static int graph_verify(int argc, const char **argv)
>  		opts.obj_dir = get_object_directory();
>
>  	graph_name = get_commit_graph_filename(opts.obj_dir);
> -	graph = load_commit_graph_one(graph_name);
> +	open_ok = open_commit_graph(graph_name, &fd, &st);
> +	if (!open_ok)
> +		return 0;
> +	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
>  	FREE_AND_NULL(graph_name);
>
>  	if (!graph)
> -		return 0;
> +		return 1;
>
>  	UNLEAK(graph);
>  	return verify_commit_graph(the_repository, graph);
> @@ -72,6 +78,9 @@ static int graph_read(int argc, const char **argv)
>  {
>  	struct commit_graph *graph = NULL;
>  	char *graph_name;
> +	int open_ok;
> +	int fd;
> +	struct stat st;
>
>  	static struct option builtin_commit_graph_read_options[] = {
>  		OPT_STRING(0, "object-dir", &opts.obj_dir,
> @@ -88,10 +97,14 @@ static int graph_read(int argc, const char **argv)
>  		opts.obj_dir = get_object_directory();
>
>  	graph_name = get_commit_graph_filename(opts.obj_dir);
> -	graph = load_commit_graph_one(graph_name);
>
> +	open_ok = open_commit_graph(graph_name, &fd, &st);
> +	if (!open_ok)
> +		die_errno(_("Could not open commit-graph '%s'"), graph_name);
> +
> +	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
>  	if (!graph)
> -		die("graph file %s does not exist", graph_name);
> +		return 1;
>
>  	FREE_AND_NULL(graph_name);
>
> diff --git a/commit-graph.c b/commit-graph.c
> index f8201d888b..3acc032c1b 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -80,25 +80,31 @@ static int commit_graph_compatible(struct repository *r)
>  	return 1;
>  }
>
> -struct commit_graph *load_commit_graph_one(const char *graph_file)
> +int open_commit_graph(const char *graph_file, int *fd, struct stat *st)
> +{
> +	*fd = git_open(graph_file);
> +	if (*fd < 0)
> +		return 0;
> +	if (fstat(*fd, st)) {
> +		close(*fd);
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
> +						 int fd, struct stat *st)
>  {
>  	void *graph_map;
>  	size_t graph_size;
> -	struct stat st;
>  	struct commit_graph *ret;
> -	int fd = git_open(graph_file);
>
> -	if (fd < 0)
> -		return NULL;
> -	if (fstat(fd, &st)) {
> -		close(fd);
> -		return NULL;
> -	}
> -	graph_size = xsize_t(st.st_size);
> +	graph_size = xsize_t(st->st_size);
>
>  	if (graph_size < GRAPH_MIN_SIZE) {
>  		close(fd);
> -		die(_("graph file %s is too small"), graph_file);
> +		error(_("graph file %s is too small"), graph_file);
> +		return NULL;
>  	}
>  	graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
>  	ret = parse_commit_graph(graph_map, fd, graph_size);
> @@ -106,7 +112,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
>  	if (!ret) {
>  		munmap(graph_map, graph_size);
>  		close(fd);
> -		exit(1);
>  	}
>
>  	return ret;
> @@ -269,6 +274,19 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
>  	return graph;
>  }
>
> +static struct commit_graph *load_commit_graph_one(const char *graph_file)
> +{
> +
> +	struct stat st;
> +	int fd;
> +	int open_ok = open_commit_graph(graph_file, &fd, &st);
> +
> +	if (!open_ok)
> +		return NULL;
> +
> +	return load_commit_graph_one_fd_st(graph_file, fd, &st);
> +}
> +
>  static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
>  {
>  	char *graph_name;
> diff --git a/commit-graph.h b/commit-graph.h
> index 096d8bac34..77cc739bc0 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -11,6 +11,7 @@
>  struct commit;
>
>  char *get_commit_graph_filename(const char *obj_dir);
> +int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
>
>  /*
>   * Given a commit struct, try to fill the commit struct info, including:
> @@ -52,7 +53,8 @@ struct commit_graph {
>  	const unsigned char *chunk_extra_edges;
>  };
>
> -struct commit_graph *load_commit_graph_one(const char *graph_file);
> +struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
> +						 int fd, struct stat *st);
>
>  struct commit_graph *parse_commit_graph(void *graph_map, int fd,
>  					size_t graph_size);
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index ad3a695f76..71d775e313 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -377,7 +377,7 @@ corrupt_graph_verify() {
>  	test_must_fail git commit-graph verify 2>test_err &&
>  	grep -v "^+" test_err >err &&
>  	test_i18ngrep "$grepstr" err &&
> -	test_might_fail git status --short
> +	git status --short
>  }
>
>  # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
There's still cases left where we'll exit early, e.g. if you do:
    $ git diff -U1
    diff --git a/commit-graph.c b/commit-graph.c
    index 66865acbd7..63773764ce 100644
    --- a/commit-graph.c
    +++ b/commit-graph.c
    @@ -1074,3 +1074,3 @@ void write_commit_graph(const char *obj_dir,
            chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
    -       chunk_offsets[2] = chunk_offsets[1] + hashsz * commits.nr;
    +       chunk_offsets[2] = chunk_offsets[0] + hashsz * commits.nr;
            chunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * commits.nr;

Which is obviously bad, but something I encounterd while hacking up [1] we'll still hard die as before this patch on:

    $ git status
    fatal: invalid parent position 1734910766
    $
Stacktrace:
    (gdb) bt
    #0  die (err=0x5555558008cd "invalid parent position %lu") at usage.c:153
    #1  0x000055555565e88c in insert_parent_or_die (r=0x5555558d6320 <the_repo>, g=0x5555559d4ff0, pos=1734910766, pptr=0x5555559c0f00) at commit-graph.c:383
    #2  0x000055555565eae0 in fill_commit_in_graph (r=0x5555558d6320 <the_repo>, item=0x5555559c0ed0, g=0x5555559d4ff0, pos=102470) at commit-graph.c:426
    #3  0x000055555565ec56 in parse_commit_in_graph_one (r=0x5555558d6320 <the_repo>, g=0x5555559d4ff0, item=0x5555559c0ed0) at commit-graph.c:469
    #4  0x000055555565eca5 in parse_commit_in_graph (r=0x5555558d6320 <the_repo>, item=0x5555559c0ed0) at commit-graph.c:478
    #5  0x000055555565aaf4 in repo_parse_commit_internal (r=0x5555558d6320 <the_repo>, item=0x5555559c0ed0, quiet_on_missing=0, use_commit_graph=1) at commit.c:465
    #6  0x000055555565ac32 in repo_parse_commit_gently (r=0x5555558d6320 <the_repo>, item=0x5555559c0ed0, quiet_on_missing=0) at commit.c:490
    #7  0x000055555573347f in repo_parse_commit (r=0x5555558d6320 <the_repo>, item=0x5555559c0ed0) at commit.h:90
    #8  0x0000555555734764 in get_reference (revs=0x7fffffffcf50, name=0x55555582ceb8 "HEAD", oid=0x7fffffffce60, flags=0) at revision.c:365
    #9  0x000055555573b0e2 in setup_revisions (argc=0, argv=0x0, revs=0x7fffffffcf50, opt=0x7fffffffcf30) at revision.c:2611
    #10 0x00005555557a6985 in wt_status_collect_changes_index (s=0x5555558a3de0 <s>) at wt-status.c:618
    #11 0x00005555557a70a5 in wt_status_collect (s=0x5555558a3de0 <s>) at wt-status.c:761
    #12 0x0000555555595ede in cmd_status (argc=0, argv=0x7fffffffdda8, prefix=0x0) at builtin/commit.c:1408
    #13 0x000055555556f8a2 in run_builtin (p=0x5555558931a8 <commands+2568>, argc=1, argv=0x7fffffffdda8) at git.c:444
    #14 0x000055555556fbe3 in handle_builtin (argc=1, argv=0x7fffffffdda8) at git.c:676
    #15 0x000055555556fe4d in run_argv (argcp=0x7fffffffdc4c, argv=0x7fffffffdc40) at git.c:743
    #16 0x000055555557014b in cmd_main (argc=1, argv=0x7fffffffdda8) at git.c:877
    #17 0x000055555562d6d2 in main (argc=3, argv=0x7fffffffdd98) at common-main.c:50

I might (or not) get to this some other time, meanwhile sending an E-Mail report...

1. https://public-inbox.org/git/87zhobr4fl.fsf@evledraar.gmail.com/
Derrick Stolee· Apr 29, 2019, 12:48 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v3 4/8] commit-graph: don't early exit(1) on e.g. "git status"

On 4/27/2019 9:06 AM, Ævar Arnfjörð Bjarmason wrote:
Show 20 quoted lines
> 
> There's still cases left where we'll exit early, e.g. if you do:
> 
>     $ git diff -U1
>     diff --git a/commit-graph.c b/commit-graph.c
>     index 66865acbd7..63773764ce 100644
>     --- a/commit-graph.c
>     +++ b/commit-graph.c
>     @@ -1074,3 +1074,3 @@ void write_commit_graph(const char *obj_dir,
>             chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
>     -       chunk_offsets[2] = chunk_offsets[1] + hashsz * commits.nr;
>     +       chunk_offsets[2] = chunk_offsets[0] + hashsz * commits.nr;
>             chunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * commits.nr;
> 
> Which is obviously bad, but something I encounterd while hacking up [1]
> we'll still hard die as before this patch on:
> 
>     $ git status
>     fatal: invalid parent position 1734910766
>     $

I really appreciate you digging in deep into these kinds of issues. You seem to be hitting corrupted commit-graph files more often than we are (in VFS for Git world). However, we should be _very careful_ when turning some of these errors to warnings. At the very least, we should do some high-level planning for how to handle this case.

The biggest issue is that we have some logic that is run after a call to generation_numbers_enabled(), such as the `git rev-list --topo-order` logic, that relies on the commit-graph for correctness. If we output a warning and then stop using the commit-graph, then we will start having commits with finite generation pointing to commits with infinite generation.

Perhaps, with some care, we can alert the algorithm to change the "minimum generation" that limits how far we dequeue the priority-queue. Changing it to zero will cause the algorithm to behave like the old algorithm.

But, having an algorithm that usually takes 0.1 seconds suddenly take 10+ seconds also violates some expectations.

Q: How should we handle a detectably-invalid commit-graph?

I think most of your patches have done a good job so far of detecting an invalid header, and responding by ignoring the commit-graph. This case of a detectable error in the chunk data itself is not something we can check on the first load without serious performance issues.

I hope we can decide on a good solution.

Thanks, -Stolee

Ævar Arnfjörð Bjarmason· Apr 29, 2019, 14:30 UTC · re: Derrick Stolee · lore

Re: [PATCH v3 4/8] commit-graph: don't early exit(1) on e.g. "git status"

On Mon, Apr 29 2019, Derrick Stolee wrote:
Show 25 quoted lines
> On 4/27/2019 9:06 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> There's still cases left where we'll exit early, e.g. if you do:
>>
>>     $ git diff -U1
>>     diff --git a/commit-graph.c b/commit-graph.c
>>     index 66865acbd7..63773764ce 100644
>>     --- a/commit-graph.c
>>     +++ b/commit-graph.c
>>     @@ -1074,3 +1074,3 @@ void write_commit_graph(const char *obj_dir,
>>             chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
>>     -       chunk_offsets[2] = chunk_offsets[1] + hashsz * commits.nr;
>>     +       chunk_offsets[2] = chunk_offsets[0] + hashsz * commits.nr;
>>             chunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * commits.nr;
>>
>> Which is obviously bad, but something I encounterd while hacking up [1]
>> we'll still hard die as before this patch on:
>>
>>     $ git status
>>     fatal: invalid parent position 1734910766
>>     $
>
> I really appreciate you digging in deep into these kinds of issues. You
> seem to be hitting corrupted commit-graph files more often than we are
> (in VFS for Git world).

FWIW I've never encountered any of these in the wild. I just started poking at this in 2ac138d568 ("commit-graph: fix segfault on e.g. "git status"", 2019-03-25) because I was looking at the commit graph, running its tests with -d, and we'd segfault previously on e.g. "git status" in our own graph corruption tests.

> However, we should be _very careful_ when turning some of these errors
> to warnings. At the very least, we should do some high-level planning
> for how to handle this case.

Indeed. I should have been explicit, I don't think it's sane to do anything except return NULL up the stack and say "the graph is screwed, we can't use it" when initially parsing it/headers, but reading on...

Show 14 quoted lines
> The biggest issue is that we have some logic that is run after a call to
> generation_numbers_enabled(), such as the `git rev-list --topo-order`
> logic, that relies on the commit-graph for correctness. If we output a
> warning and then stop using the commit-graph, then we will start having
> commits with finite generation pointing to commits with infinite generation.
>
> Perhaps, with some care, we can alert the algorithm to change the "minimum
> generation" that limits how far we dequeue the priority-queue. Changing it
> to zero will cause the algorithm to behave like the old algorithm.
>
> But, having an algorithm that usually takes 0.1 seconds suddenly take 10+
> seconds also violates some expectations.
>
> Q: How should we handle a detectably-invalid commit-graph?

...I don't think we need to do any paranoid algorithm fallback in general. As you point out that's going to be a PITA as we read the actual data in the graph in some cases.

Show 6 quoted lines
> I think most of your patches have done a good job so far of detecting
> an invalid header, and responding by ignoring the commit-graph. This case
> of a detectable error in the chunk data itself is not something we can
> check on the first load without serious performance issues.
>
> I hope we can decide on a good solution.

...OK, so this is one of those PITA cases. I think it's fine to just leave it.

Although maybe we'd still want to be more paranoid with O(n) cases like "--contains" or "--topo-order" and cases that are surely just "look up my immediate commit data", like "status" dying on this particular error.

But I think it's fine to just decide to nothing about this. I mainly just wanted to make a note to myself & CC the list in case there was interest...

Jeff King· May 1, 2019, 18:31 UTC · re: Derrick Stolee · lore

Re: [PATCH v3 4/8] commit-graph: don't early exit(1) on e.g. "git status"

On Mon, Apr 29, 2019 at 08:48:51AM -0400, Derrick Stolee wrote:
Show 8 quoted lines
> Q: How should we handle a detectably-invalid commit-graph?
> 
> I think most of your patches have done a good job so far of detecting
> an invalid header, and responding by ignoring the commit-graph. This case
> of a detectable error in the chunk data itself is not something we can
> check on the first load without serious performance issues.
> 
> I hope we can decide on a good solution.

I think it's OK to die() unceremoniously in these cases. We already have similar behavior when dealing with packfiles. While we try to just return an error for bit-flips in data (so we can try finding the object elsewhere), if there are things like nonsensical file offsets, we generally just give up.

So I think it makes sense to start with the _safe_ thing, which is just giving up on the operation and informing the user. If somebody wants to then loosen things up on a case by case basis (after seeing that it wouldn't produce incorrect results to do so), that can be future work.

For the commit-graph, though, and given that we're talking about file corruption, I don't know that it's even worth the effort. The solution is basically always going to be "delete the commit-graph file and regenerate it". It's perhaps slightly inconvenient compared to falling back, but this just isn't something we'd expect to happen often.

-Peff
Ævar Arnfjörð Bjarmason· Mar 25, 2019, 12:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 5/8] commit-graph: don't pass filename to load_commit_graph_one_fd_st()

An earlier change implemented load_commit_graph_one_fd_st() in a way that was bug-compatible with earlier code in terms of the "graph file %s is too small" error message printing out the path to the commit-graph (".git/objects/info/commit-graph").

But change that, because:
 * A function that takes an already-open file descriptor also needing
   the filename isn't very intuitive.
 * The vast majority of errors we might emit when loading the graph
   come from parse_commit_graph(), which doesn't report the
   filename. Let's not do that either in this case for consistency.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c | 4 ++--
 commit-graph.c         | 7 +++----
 commit-graph.h         | 3 +--
 3 files changed, 6 insertions(+), 8 deletions(-)
Show changes to 3 files +6 −8

builtin/commit-graph.c, commit-graph.c, commit-graph.h

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 32bcc63427..8196fdbe9c 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -64,7 +64,7 @@ static int graph_verify(int argc, const char **argv)
 	open_ok = open_commit_graph(graph_name, &fd, &st);
 	if (!open_ok)
 		return 0;
-	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
+	graph = load_commit_graph_one_fd_st(fd, &st);
 	FREE_AND_NULL(graph_name);
 
 	if (!graph)
@@ -102,7 +102,7 @@ static int graph_read(int argc, const char **argv)
 	if (!open_ok)
 		die_errno(_("Could not open commit-graph '%s'"), graph_name);
 
-	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
+	graph = load_commit_graph_one_fd_st(fd, &st);
 	if (!graph)
 		return 1;
 
diff --git a/commit-graph.c b/commit-graph.c
index 3acc032c1b..a26d266663 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -92,8 +92,7 @@ int open_commit_graph(const char *graph_file, int *fd, struct stat *st)
 	return 1;
 }
 
-struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
-						 int fd, struct stat *st)
+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st)
 {
 	void *graph_map;
 	size_t graph_size;
@@ -103,7 +102,7 @@ struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
 
 	if (graph_size < GRAPH_MIN_SIZE) {
 		close(fd);
-		error(_("graph file %s is too small"), graph_file);
+		error(_("commit-graph file is too small"));
 		return NULL;
 	}
 	graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
@@ -284,7 +283,7 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file)
 	if (!open_ok)
 		return NULL;
 
-	return load_commit_graph_one_fd_st(graph_file, fd, &st);
+	return load_commit_graph_one_fd_st(fd, &st);
 }
 
 static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
diff --git a/commit-graph.h b/commit-graph.h
index 77cc739bc0..ada7aea9ed 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -53,8 +53,7 @@ struct commit_graph {
 	const unsigned char *chunk_extra_edges;
 };
 
-struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
-						 int fd, struct stat *st);
+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);
 
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 					size_t graph_size);

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 25, 2019, 12:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 8/8] commit-graph: improve & i18n error messages

Change the error emitted when a commit-graph file is corrupt so that we actually mention the commit-graph, e.g. change errors like:

    error: improper chunk offset 0000000000385e0c
To:
    error: commit-graph improper chunk offset 0000000000385e0c

As discussed in the commits leading up to this one the commit-graph machinery is now used by common commands like "status". If the graph was corrupt we'd often emit some error that gave no indication what was wrong. Now some of them are still cryptic, but they'll at least mention "commit-graph" to give the user a hint as to where to look.

While I'm at it mark some of the strings that hadn't been marked for translation. It's clear from the commit history and the code that this was merely forgotten at the time, and wasn't intentional.p5

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit-graph.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)
Show changes to commit-graph.c +19 −19
diff --git a/commit-graph.c b/commit-graph.c
index 34ecaaf857..66865acbd7 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -167,21 +167,21 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 
 	graph_signature = get_be32(data);
 	if (graph_signature != GRAPH_SIGNATURE) {
-		error(_("graph signature %X does not match signature %X"),
+		error(_("commit-graph signature %X does not match signature %X"),
 		      graph_signature, GRAPH_SIGNATURE);
 		return NULL;
 	}
 
 	graph_version = *(unsigned char*)(data + 4);
 	if (graph_version != GRAPH_VERSION) {
-		error(_("graph version %X does not match version %X"),
+		error(_("commit-graph version %X does not match version %X"),
 		      graph_version, GRAPH_VERSION);
 		return NULL;
 	}
 
 	hash_version = *(unsigned char*)(data + 5);
 	if (hash_version != oid_version()) {
-		error(_("hash version %X does not match version %X"),
+		error(_("commit-graph hash version %X does not match version %X"),
 		      hash_version, oid_version());
 		return NULL;
 	}
@@ -204,7 +204,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 
 		if (data + graph_size - chunk_lookup <
 		    GRAPH_CHUNKLOOKUP_WIDTH) {
-			error(_("chunk lookup table entry missing; graph file may be incomplete"));
+			error(_("commit-graph chunk lookup table entry missing; file may be incomplete"));
 			free(graph);
 			return NULL;
 		}
@@ -215,7 +215,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 		chunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;
 
 		if (chunk_offset > graph_size - the_hash_algo->rawsz) {
-			error(_("improper chunk offset %08x%08x"), (uint32_t)(chunk_offset >> 32),
+			error(_("commit-graph improper chunk offset %08x%08x"), (uint32_t)(chunk_offset >> 32),
 			      (uint32_t)chunk_offset);
 			free(graph);
 			return NULL;
@@ -252,7 +252,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 		}
 
 		if (chunk_repeated) {
-			error(_("chunk id %08x appears multiple times"), chunk_id);
+			error(_("commit-graph chunk id %08x appears multiple times"), chunk_id);
 			free(graph);
 			return NULL;
 		}
@@ -1162,7 +1162,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
 
 		if (i && oidcmp(&prev_oid, &cur_oid) >= 0)
-			graph_report("commit-graph has incorrect OID order: %s then %s",
+			graph_report(_("commit-graph has incorrect OID order: %s then %s"),
 				     oid_to_hex(&prev_oid),
 				     oid_to_hex(&cur_oid));
 
@@ -1172,14 +1172,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 			uint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);
 
 			if (i != fanout_value)
-				graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",
+				graph_report(_("commit-graph has incorrect fanout value: fanout[%d] = %u != %u"),
 					     cur_fanout_pos, fanout_value, i);
 			cur_fanout_pos++;
 		}
 
 		graph_commit = lookup_commit(r, &cur_oid);
 		if (!parse_commit_in_graph_one(r, g, graph_commit))
-			graph_report("failed to parse %s from commit-graph",
+			graph_report(_("failed to parse commit %s from commit-graph"),
 				     oid_to_hex(&cur_oid));
 	}
 
@@ -1187,7 +1187,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		uint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);
 
 		if (g->num_commits != fanout_value)
-			graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",
+			graph_report(_("commit-graph has incorrect fanout value: fanout[%d] = %u != %u"),
 				     cur_fanout_pos, fanout_value, i);
 
 		cur_fanout_pos++;
@@ -1209,14 +1209,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		graph_commit = lookup_commit(r, &cur_oid);
 		odb_commit = (struct commit *)create_object(r, cur_oid.hash, alloc_commit_node(r));
 		if (parse_commit_internal(odb_commit, 0, 0)) {
-			graph_report("failed to parse %s from object database",
+			graph_report(_("failed to parse commit %s from object database for commit-graph"),
 				     oid_to_hex(&cur_oid));
 			continue;
 		}
 
 		if (!oideq(&get_commit_tree_in_graph_one(r, g, graph_commit)->object.oid,
 			   get_commit_tree_oid(odb_commit)))
-			graph_report("root tree OID for commit %s in commit-graph is %s != %s",
+			graph_report(_("root tree OID for commit %s in commit-graph is %s != %s"),
 				     oid_to_hex(&cur_oid),
 				     oid_to_hex(get_commit_tree_oid(graph_commit)),
 				     oid_to_hex(get_commit_tree_oid(odb_commit)));
@@ -1226,13 +1226,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 
 		while (graph_parents) {
 			if (odb_parents == NULL) {
-				graph_report("commit-graph parent list for commit %s is too long",
+				graph_report(_("commit-graph parent list for commit %s is too long"),
 					     oid_to_hex(&cur_oid));
 				break;
 			}
 
 			if (!oideq(&graph_parents->item->object.oid, &odb_parents->item->object.oid))
-				graph_report("commit-graph parent for %s is %s != %s",
+				graph_report(_("commit-graph parent for %s is %s != %s"),
 					     oid_to_hex(&cur_oid),
 					     oid_to_hex(&graph_parents->item->object.oid),
 					     oid_to_hex(&odb_parents->item->object.oid));
@@ -1245,16 +1245,16 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		}
 
 		if (odb_parents != NULL)
-			graph_report("commit-graph parent list for commit %s terminates early",
+			graph_report(_("commit-graph parent list for commit %s terminates early"),
 				     oid_to_hex(&cur_oid));
 
 		if (!graph_commit->generation) {
 			if (generation_zero == GENERATION_NUMBER_EXISTS)
-				graph_report("commit-graph has generation number zero for commit %s, but non-zero elsewhere",
+				graph_report(_("commit-graph has generation number zero for commit %s, but non-zero elsewhere"),
 					     oid_to_hex(&cur_oid));
 			generation_zero = GENERATION_ZERO_EXISTS;
 		} else if (generation_zero == GENERATION_ZERO_EXISTS)
-			graph_report("commit-graph has non-zero generation number for commit %s, but zero elsewhere",
+			graph_report(_("commit-graph has non-zero generation number for commit %s, but zero elsewhere"),
 				     oid_to_hex(&cur_oid));
 
 		if (generation_zero == GENERATION_ZERO_EXISTS)
@@ -1269,13 +1269,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 			max_generation--;
 
 		if (graph_commit->generation != max_generation + 1)
-			graph_report("commit-graph generation for commit %s is %u != %u",
+			graph_report(_("commit-graph generation for commit %s is %u != %u"),
 				     oid_to_hex(&cur_oid),
 				     graph_commit->generation,
 				     max_generation + 1);
 
 		if (graph_commit->date != odb_commit->date)
-			graph_report("commit date for commit %s in commit-graph is %"PRItime" != %"PRItime,
+			graph_report(_("commit date for commit %s in commit-graph is %"PRItime" != %"PRItime),
 				     oid_to_hex(&cur_oid),
 				     graph_commit->date,
 				     odb_commit->date);

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 25, 2019, 12:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 7/8] commit-graph write: don't die if the existing graph is corrupt

When the commit-graph is written we end up calling parse_commit(). This will in turn invoke code that'll consult the existing commit-graph about the commit, if the graph is corrupted we die.

We thus get into a state where a failing "commit-graph verify" can't be followed-up with a "commit-graph write" if core.commitGraph=true is set, the graph either needs to be manually removed to proceed, or core.commitGraph needs to be set to "false".

Change the "commit-graph write" codepath to use a new parse_commit_no_graph() helper instead of parse_commit() to avoid this. The latter will call repo_parse_commit_internal() with use_commit_graph=1 as seen in 177722b344 ("commit: integrate commit graph with commit parsing", 2018-04-10).

Not using the old graph at all slows down the writing of the new graph by some small amount, but is a sensible way to prevent an error in the existing commit-graph from spreading.

Just fixing the current issue would be likely to result in code that's inadvertently broken in the future. New code might use the commit-graph at a distance. To detect such cases introduce a "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD" setting used when we do our corruption tests, and test that a "write/verify" combo works after every one of our current test cases where we now detect commit-graph corruption.

Some of the code changes here might be strictly unnecessary, e.g. I was unable to find cases where the parse_commit() called from write_graph_chunk_data() didn't exit early due to "item->object.parsed" being true in repo_parse_commit_internal() (before the use_commit_graph=1 has any effect). But let's also convert those cases for good measure, we do not have exhaustive tests for all possible types of commit-graph corruption.

This might need to be re-visited if we learn to write the commit-graph incrementally, but probably not. Hopefully we'll just start by finding out what commits we have in total, then read the old graph(s) to see what they cover, and finally write a new graph file with everything that's missing. In that case the new graph writing code just needs to continue to use e.g. a parse_commit() that doesn't consult the existing commit-graphs.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit-graph.c          | 10 +++++++---
 commit-graph.h          |  1 +
 commit.h                |  6 ++++++
 t/t5318-commit-graph.sh | 11 +++++++++--
 4 files changed, 23 insertions(+), 5 deletions(-)
Show changes to 4 files +23 −5

commit-graph.c, commit-graph.h, commit.h, t/t5318-commit-graph.sh

diff --git a/commit-graph.c b/commit-graph.c
index a26d266663..34ecaaf857 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -311,6 +311,10 @@ static int prepare_commit_graph(struct repository *r)
 	struct object_directory *odb;
 	int config_value;
 
+	if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))
+		die("dying as requested by the '%s' variable on commit-graph load!",
+		    GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);
+
 	if (r->objects->commit_graph_attempted)
 		return !!r->objects->commit_graph;
 	r->objects->commit_graph_attempted = 1;
@@ -575,7 +579,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
 		uint32_t packedDate[2];
 		display_progress(progress, ++*progress_cnt);
 
-		parse_commit(*list);
+		parse_commit_no_graph(*list);
 		hashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);
 
 		parent = (*list)->parents;
@@ -772,7 +776,7 @@ static void close_reachable(struct packed_oid_list *oids, int report_progress)
 		display_progress(progress, i + 1);
 		commit = lookup_commit(the_repository, &oids->list[i]);
 
-		if (commit && !parse_commit(commit))
+		if (commit && !parse_commit_no_graph(commit))
 			add_missing_parents(oids, commit);
 	}
 	stop_progress(&progress);
@@ -1021,7 +1025,7 @@ void write_commit_graph(const char *obj_dir,
 			continue;
 
 		commits.list[commits.nr] = lookup_commit(the_repository, &oids.list[i]);
-		parse_commit(commits.list[commits.nr]);
+		parse_commit_no_graph(commits.list[commits.nr]);
 
 		for (parent = commits.list[commits.nr]->parents;
 		     parent; parent = parent->next)
diff --git a/commit-graph.h b/commit-graph.h
index ada7aea9ed..7dfb8c896f 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -7,6 +7,7 @@
 #include "cache.h"
 
 #define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH"
+#define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD"
 
 struct commit;
 
diff --git a/commit.h b/commit.h
index 42728c2906..5d33477e78 100644
--- a/commit.h
+++ b/commit.h
@@ -89,6 +89,12 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)
 {
 	return repo_parse_commit_gently(r, item, 0);
 }
+
+static inline int parse_commit_no_graph(struct commit *commit)
+{
+	return repo_parse_commit_internal(the_repository, commit, 0, 0);
+}
+
 #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
 #define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use)
 #define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet)
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 1cb0355c7e..d146cf4982 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -377,7 +377,13 @@ corrupt_graph_verify() {
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
 	test_i18ngrep "$grepstr" err &&
-	git status --short
+	if test "$2" != "no-copy"
+	then
+		cp $objdir/info/commit-graph commit-graph-pre-write-test
+	fi &&
+	git status --short &&
+	GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&
+	git commit-graph verify
 }
 
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
@@ -403,7 +409,7 @@ corrupt_graph_and_verify() {
 test_expect_success POSIXPERM,SANITY 'detect permission problem' '
 	corrupt_graph_setup &&
 	chmod 000 $objdir/info/commit-graph &&
-	corrupt_graph_verify "Could not open"
+	corrupt_graph_verify "Could not open" "no-copy"
 '
 
 test_expect_success 'detect too small' '
@@ -522,6 +528,7 @@ test_expect_success 'git fsck (checks commit-graph)' '
 	git fsck &&
 	corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
 		"incorrect checksum" &&
+	cp commit-graph-pre-write-test $objdir/info/commit-graph &&
 	test_must_fail git fsck
 '
 

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 25, 2019, 12:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 6/8] commit-graph verify: detect inability to read the graph

Change "commit-graph verify" to error on open() failures other than ENOENT. As noted in the third paragraph of 283e68c72f ("commit-graph: add 'verify' subcommand", 2018-06-27) and the test it added it's intentional that "commit-graph verify" doesn't error out when the file doesn't exist.

But let's not be overly promiscuous in what we accept. If we can't read the file for other reasons, e.g. permission errors, bad file descriptor etc. we'd like to report an error to the user.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c  | 4 +++-
 t/t5318-commit-graph.sh | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)
Show changes to 2 files +9 −1

builtin/commit-graph.c, t/t5318-commit-graph.sh

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 8196fdbe9c..537fdfd0f0 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -62,8 +62,10 @@ static int graph_verify(int argc, const char **argv)
 
 	graph_name = get_commit_graph_filename(opts.obj_dir);
 	open_ok = open_commit_graph(graph_name, &fd, &st);
-	if (!open_ok)
+	if (!open_ok && errno == ENOENT)
 		return 0;
+	if (!open_ok)
+		die_errno(_("Could not open commit-graph '%s'"), graph_name);
 	graph = load_commit_graph_one_fd_st(fd, &st);
 	FREE_AND_NULL(graph_name);
 
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 71d775e313..1cb0355c7e 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -400,6 +400,12 @@ corrupt_graph_and_verify() {
 
 }
 
+test_expect_success POSIXPERM,SANITY 'detect permission problem' '
+	corrupt_graph_setup &&
+	chmod 000 $objdir/info/commit-graph &&
+	corrupt_graph_verify "Could not open"
+'
+
 test_expect_success 'detect too small' '
 	corrupt_graph_setup &&
 	echo "a small graph" >$objdir/info/commit-graph &&

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 14, 2019, 21:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 1/8] commit-graph tests: split up corrupt_graph_and_verify()

Split up the corrupt_graph_and_verify() function added in d9b9f8a6fd ("commit-graph: verify catches corrupt signature", 2018-06-27) into its logical components of setting up the test itself, doing the corruption in a particular way with "dd", and then finally testing that stderr is what we expect.

This allows for re-using everything except the now slimmer corrupt_graph_and_verify() to corrupt the graph in a way that doesn't involve inserting a given byte sequence at a given position, e.g. truncating it entirely to a custom value.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5318-commit-graph.sh | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)
Show changes to t/t5318-commit-graph.sh +16 −6
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 561796f280..56a616831e 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -366,6 +366,19 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \
 GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))
 GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
 
+corrupt_graph_setup() {
+	cd "$TRASH_DIRECTORY/full" &&
+	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
+	cp $objdir/info/commit-graph commit-graph-backup
+}
+
+corrupt_graph_verify() {
+	grepstr=$1
+	test_must_fail git commit-graph verify 2>test_err &&
+	grep -v "^+" test_err >err &&
+	test_i18ngrep "$grepstr" err
+}
+
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
 # Manipulates the commit-graph file at the position
 # by inserting the data, optionally zeroing the file
@@ -376,17 +389,14 @@ corrupt_graph_and_verify() {
 	pos=$1
 	data="${2:-\0}"
 	grepstr=$3
-	cd "$TRASH_DIRECTORY/full" &&
+	corrupt_graph_setup &&
 	orig_size=$(wc -c < $objdir/info/commit-graph) &&
 	zero_pos=${4:-${orig_size}} &&
-	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
-	cp $objdir/info/commit-graph commit-graph-backup &&
 	printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
 	dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" if=/dev/null &&
 	generate_zero_bytes $(($orig_size - $zero_pos)) >>"$objdir/info/commit-graph" &&
-	test_must_fail git commit-graph verify 2>test_err &&
-	grep -v "^+" test_err >err &&
-	test_i18ngrep "$grepstr" err
+	corrupt_graph_verify "$grepstr"
+
 }
 
 test_expect_success 'detect bad signature' '

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 14, 2019, 21:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 2/8] commit-graph tests: test a graph that's too small

Use the recently split-up components of the corrupt_graph_and_verify() function to assert that we error on graphs that are too small. The error was added in 2a2e32bdc5 ("commit-graph: implement git commit-graph read", 2018-04-10), but there was no test for it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5318-commit-graph.sh | 6 ++++++
 1 file changed, 6 insertions(+)
Show changes to t/t5318-commit-graph.sh +6 −0
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 56a616831e..ce3459a6f5 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -399,6 +399,12 @@ corrupt_graph_and_verify() {
 
 }
 
+test_expect_success 'detect too small' '
+	corrupt_graph_setup &&
+	echo "a small graph" >$objdir/info/commit-graph &&
+	corrupt_graph_verify "too small"
+'
+
 test_expect_success 'detect bad signature' '
 	corrupt_graph_and_verify 0 "\0" \
 		"graph signature"

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 14, 2019, 21:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 3/8] commit-graph: fix segfault on e.g. "git status"

When core.commitGraph=true is set, various common commands now consult the commit graph. Because the commit-graph code is very trusting of its input data, it's possibly to construct a graph that'll cause an immediate segfault on e.g. "status" (and e.g. "log", "blame", ...). In some other cases where git immediately exits with a cryptic error about the graph being broken.

The root cause of this is that while the "commit-graph verify" sub-command exhaustively verifies the graph, other users of the graph simply trust the graph, and will e.g. deference data found at certain offsets as pointers, causing segfaults.

This change does the bare minimum to ensure that we don't segfault in the common fill_commit_in_graph() codepath called by e.g. setup_revisions(), to do this instrument the "commit-graph verify" tests to always check if "status" would subsequently segfault. This fixes the following tests which would previously segfault:

    not ok 50 - detect low chunk count
    not ok 51 - detect missing OID fanout chunk
    not ok 52 - detect missing OID lookup chunk
    not ok 53 - detect missing commit data chunk

Those happened because with the commit-graph enabled setup_revisions() would eventually call fill_commit_in_graph(), where e.g. g->chunk_commit_data is used early as an offset (and will be 0x0). With this change we get far enough to detect that the graph is broken, and show an error instead. E.g.:

    $ git status; echo $?
    error: commit-graph is missing the Commit Data chunk
    1

That also sucks, we should *warn* and not hard-fail "status" just because the commit-graph is corrupt, but fixing is left to a follow-up change.

A side-effect of changing the reporting from graph_report() to error() is that we now have an "error: " prefix for these even for "commit-graph verify". Pseudo-diff before/after:

    $ git commit-graph verify
    -commit-graph is missing the Commit Data chunk
    +error: commit-graph is missing the Commit Data chunk

Changing that is OK. Various errors it emits now early on are prefixed with "error: ", moving these over and changing the output doesn't break anything.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit-graph.c          | 43 ++++++++++++++++++++++++++++++++---------
 commit-graph.h          |  1 +
 t/t5318-commit-graph.sh |  3 ++-
 3 files changed, 37 insertions(+), 10 deletions(-)
Show changes to 3 files +37 −10

commit-graph.c, commit-graph.h, t/t5318-commit-graph.sh

diff --git a/commit-graph.c b/commit-graph.c
index 47e9be0a3a..980fbf47ea 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -233,6 +233,9 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 		last_chunk_offset = chunk_offset;
 	}
 
+	if (verify_commit_graph_lite(graph))
+		return NULL;
+
 	return graph;
 }
 
@@ -1075,6 +1078,36 @@ static void graph_report(const char *fmt, ...)
 #define GENERATION_ZERO_EXISTS 1
 #define GENERATION_NUMBER_EXISTS 2
 
+int verify_commit_graph_lite(struct commit_graph *g)
+{
+	/*
+	 * Basic validation shared between parse_commit_graph()
+	 * which'll be called every time the graph is used, and the
+	 * much more expensive verify_commit_graph() used by
+	 * "commit-graph verify".
+	 *
+	 * There should only be very basic checks here to ensure that
+	 * we don't e.g. segfault in fill_commit_in_graph(), but
+	 * because this is a very hot codepath nothing that e.g. loops
+	 * over g->num_commits, or runs a checksum on the commit-graph
+	 * itself.
+	 */
+	if (!g->chunk_oid_fanout) {
+		error("commit-graph is missing the OID Fanout chunk");
+		return 1;
+	}
+	if (!g->chunk_oid_lookup) {
+		error("commit-graph is missing the OID Lookup chunk");
+		return 1;
+	}
+	if (!g->chunk_commit_data) {
+		error("commit-graph is missing the Commit Data chunk");
+		return 1;
+	}
+
+	return 0;
+}
+
 int verify_commit_graph(struct repository *r, struct commit_graph *g)
 {
 	uint32_t i, cur_fanout_pos = 0;
@@ -1089,15 +1122,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		return 1;
 	}
 
-	verify_commit_graph_error = 0;
-
-	if (!g->chunk_oid_fanout)
-		graph_report("commit-graph is missing the OID Fanout chunk");
-	if (!g->chunk_oid_lookup)
-		graph_report("commit-graph is missing the OID Lookup chunk");
-	if (!g->chunk_commit_data)
-		graph_report("commit-graph is missing the Commit Data chunk");
-
+	verify_commit_graph_error = verify_commit_graph_lite(g);
 	if (verify_commit_graph_error)
 		return verify_commit_graph_error;
 
diff --git a/commit-graph.h b/commit-graph.h
index 096d8bac34..275f97d006 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -70,6 +70,7 @@ void write_commit_graph(const char *obj_dir,
 			struct string_list *commit_hex,
 			int append, int report_progress);
 
+int verify_commit_graph_lite(struct commit_graph *g);
 int verify_commit_graph(struct repository *r, struct commit_graph *g);
 
 void close_commit_graph(struct repository *);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index ce3459a6f5..ad3a695f76 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -376,7 +376,8 @@ corrupt_graph_verify() {
 	grepstr=$1
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
-	test_i18ngrep "$grepstr" err
+	test_i18ngrep "$grepstr" err &&
+	test_might_fail git status --short
 }
 
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 14, 2019, 21:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 5/8] commit-graph: don't pass filename to load_commit_graph_one_fd_st()

An earlier change implemented load_commit_graph_one_fd_st() in a way that was bug-compatible with earlier code in terms of the "graph file %s is too small" error message printing out the path to the commit-graph (".git/objects/info/commit-graph").

But change that, because:
 * A function that takes an already-open file descriptor also needing
   the filename isn't very intuitive.
 * The vast majority of errors we might emit when loading the graph
   come from parse_commit_graph(), which doesn't report the
   filename. Let's not do that either in this case for consistency.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c | 4 ++--
 commit-graph.c         | 7 +++----
 commit-graph.h         | 3 +--
 3 files changed, 6 insertions(+), 8 deletions(-)
Show changes to 3 files +6 −8

builtin/commit-graph.c, commit-graph.c, commit-graph.h

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 32bcc63427..8196fdbe9c 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -64,7 +64,7 @@ static int graph_verify(int argc, const char **argv)
 	open_ok = open_commit_graph(graph_name, &fd, &st);
 	if (!open_ok)
 		return 0;
-	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
+	graph = load_commit_graph_one_fd_st(fd, &st);
 	FREE_AND_NULL(graph_name);
 
 	if (!graph)
@@ -102,7 +102,7 @@ static int graph_read(int argc, const char **argv)
 	if (!open_ok)
 		die_errno(_("Could not open commit-graph '%s'"), graph_name);
 
-	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
+	graph = load_commit_graph_one_fd_st(fd, &st);
 	if (!graph)
 		return 1;
 
diff --git a/commit-graph.c b/commit-graph.c
index b1ba7a09cc..d945e8f3e0 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -92,8 +92,7 @@ int open_commit_graph(const char *graph_file, int *fd, struct stat *st)
 	return 1;
 }
 
-struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
-						 int fd, struct stat *st)
+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st)
 {
 	void *graph_map;
 	size_t graph_size;
@@ -103,7 +102,7 @@ struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
 
 	if (graph_size < GRAPH_MIN_SIZE) {
 		close(fd);
-		error(_("graph file %s is too small"), graph_file);
+		error(_("commit-graph file is too small"));
 		return 0;
 	}
 	graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
@@ -127,7 +126,7 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
 	if (!open_ok)
 		return NULL;
 
-	return load_commit_graph_one_fd_st(graph_file, fd, &st);
+	return load_commit_graph_one_fd_st(fd, &st);
 }
 
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
diff --git a/commit-graph.h b/commit-graph.h
index e4f17071e2..36d8109901 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -54,8 +54,7 @@ struct commit_graph {
 };
 
 struct commit_graph *load_commit_graph_one(const char *graph_file);
-struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
-						 int fd, struct stat *st);
+struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);
 
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 					size_t graph_size);

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 14, 2019, 21:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 4/8] commit-graph: don't early exit(1) on e.g. "git status"

Make the commit-graph loading code work as a library that returns an error code instead of calling exit(1) when the commit-graph is corrupt. This means that e.g. "status" will now report commit-graph corruption as an "error: [...]" at the top of its output, but then proceed to work normally.

This required splitting up the load_commit_graph_one() function so that the code that deals with open()-ing and stat()-ing the graph can now be called independently as open_commit_graph().

This is needed because "commit-graph verify" where the graph doesn't exist isn't an error. See the third paragraph in 283e68c72f ("commit-graph: add 'verify' subcommand", 2018-06-27). There's a bug in that logic where we conflate the intended ENOENT with other errno values (e.g. EACCES), but this change doesn't address that. That'll be addressed in a follow-up change.

I'm then splitting most of the logic out of load_commit_graph_one() into load_commit_graph_one_fd_st(), which allows for providing an existing file descriptor and stat information to the loading code. This isn't strictly needed, but it would be redundant and confusing to open() and stat() the file twice for some of the codepaths, this allows for calling open_commit_graph() followed by load_commit_graph_one_fd_st(). The "graph_file" still needs to be passed to that function for the the "graph file %s is too small" error message.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c  | 21 +++++++++++++++++----
 commit-graph.c          | 42 +++++++++++++++++++++++++++++------------
 commit-graph.h          |  3 +++
 t/t5318-commit-graph.sh |  2 +-
 4 files changed, 51 insertions(+), 17 deletions(-)
Show changes to 4 files +51 −17

builtin/commit-graph.c, commit-graph.c, commit-graph.h, t/t5318-commit-graph.sh

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 4ae502754c..32bcc63427 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -42,6 +42,9 @@ static int graph_verify(int argc, const char **argv)
 {
 	struct commit_graph *graph = NULL;
 	char *graph_name;
+	int open_ok;
+	int fd;
+	struct stat st;
 
 	static struct option builtin_commit_graph_verify_options[] = {
 		OPT_STRING(0, "object-dir", &opts.obj_dir,
@@ -58,11 +61,14 @@ static int graph_verify(int argc, const char **argv)
 		opts.obj_dir = get_object_directory();
 
 	graph_name = get_commit_graph_filename(opts.obj_dir);
-	graph = load_commit_graph_one(graph_name);
+	open_ok = open_commit_graph(graph_name, &fd, &st);
+	if (!open_ok)
+		return 0;
+	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
 	FREE_AND_NULL(graph_name);
 
 	if (!graph)
-		return 0;
+		return 1;
 
 	UNLEAK(graph);
 	return verify_commit_graph(the_repository, graph);
@@ -72,6 +78,9 @@ static int graph_read(int argc, const char **argv)
 {
 	struct commit_graph *graph = NULL;
 	char *graph_name;
+	int open_ok;
+	int fd;
+	struct stat st;
 
 	static struct option builtin_commit_graph_read_options[] = {
 		OPT_STRING(0, "object-dir", &opts.obj_dir,
@@ -88,10 +97,14 @@ static int graph_read(int argc, const char **argv)
 		opts.obj_dir = get_object_directory();
 
 	graph_name = get_commit_graph_filename(opts.obj_dir);
-	graph = load_commit_graph_one(graph_name);
 
+	open_ok = open_commit_graph(graph_name, &fd, &st);
+	if (!open_ok)
+		die_errno(_("Could not open commit-graph '%s'"), graph_name);
+
+	graph = load_commit_graph_one_fd_st(graph_name, fd, &st);
 	if (!graph)
-		die("graph file %s does not exist", graph_name);
+		return 1;
 
 	FREE_AND_NULL(graph_name);
 
diff --git a/commit-graph.c b/commit-graph.c
index 980fbf47ea..b1ba7a09cc 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -80,25 +80,31 @@ static int commit_graph_compatible(struct repository *r)
 	return 1;
 }
 
-struct commit_graph *load_commit_graph_one(const char *graph_file)
+int open_commit_graph(const char *graph_file, int *fd, struct stat *st)
+{
+	*fd = git_open(graph_file);
+	if (*fd < 0)
+		return 0;
+	if (fstat(*fd, st)) {
+		close(*fd);
+		return 0;
+	}
+	return 1;
+}
+
+struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
+						 int fd, struct stat *st)
 {
 	void *graph_map;
 	size_t graph_size;
-	struct stat st;
 	struct commit_graph *ret;
-	int fd = git_open(graph_file);
 
-	if (fd < 0)
-		return NULL;
-	if (fstat(fd, &st)) {
-		close(fd);
-		return NULL;
-	}
-	graph_size = xsize_t(st.st_size);
+	graph_size = xsize_t(st->st_size);
 
 	if (graph_size < GRAPH_MIN_SIZE) {
 		close(fd);
-		die(_("graph file %s is too small"), graph_file);
+		error(_("graph file %s is too small"), graph_file);
+		return 0;
 	}
 	graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	ret = parse_commit_graph(graph_map, fd, graph_size);
@@ -106,12 +112,24 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
 	if (!ret) {
 		munmap(graph_map, graph_size);
 		close(fd);
-		exit(1);
 	}
 
 	return ret;
 }
 
+struct commit_graph *load_commit_graph_one(const char *graph_file)
+{
+
+	struct stat st;
+	int fd;
+	int open_ok = open_commit_graph(graph_file, &fd, &st);
+
+	if (!open_ok)
+		return NULL;
+
+	return load_commit_graph_one_fd_st(graph_file, fd, &st);
+}
+
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 					size_t graph_size)
 {
diff --git a/commit-graph.h b/commit-graph.h
index 275f97d006..e4f17071e2 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -11,6 +11,7 @@
 struct commit;
 
 char *get_commit_graph_filename(const char *obj_dir);
+int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
 
 /*
  * Given a commit struct, try to fill the commit struct info, including:
@@ -53,6 +54,8 @@ struct commit_graph {
 };
 
 struct commit_graph *load_commit_graph_one(const char *graph_file);
+struct commit_graph *load_commit_graph_one_fd_st(const char *graph_file,
+						 int fd, struct stat *st);
 
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 					size_t graph_size);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index ad3a695f76..71d775e313 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -377,7 +377,7 @@ corrupt_graph_verify() {
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
 	test_i18ngrep "$grepstr" err &&
-	test_might_fail git status --short
+	git status --short
 }
 
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 14, 2019, 21:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 6/8] commit-graph verify: detect inability to read the graph

Change "commit-graph verify" to error on open() failures other than ENOENT. As noted in the third paragraph of 283e68c72f ("commit-graph: add 'verify' subcommand", 2018-06-27) and the test it added it's intentional that "commit-graph verify" doesn't error out when the file doesn't exist.

But let's not be overly promiscuous in what we accept. If we can't read the file for other reasons, e.g. permission errors, bad file descriptor etc. we'd like to report an error to the user.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c  | 4 +++-
 t/t5318-commit-graph.sh | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)
Show changes to 2 files +9 −1

builtin/commit-graph.c, t/t5318-commit-graph.sh

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 8196fdbe9c..537fdfd0f0 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -62,8 +62,10 @@ static int graph_verify(int argc, const char **argv)
 
 	graph_name = get_commit_graph_filename(opts.obj_dir);
 	open_ok = open_commit_graph(graph_name, &fd, &st);
-	if (!open_ok)
+	if (!open_ok && errno == ENOENT)
 		return 0;
+	if (!open_ok)
+		die_errno(_("Could not open commit-graph '%s'"), graph_name);
 	graph = load_commit_graph_one_fd_st(fd, &st);
 	FREE_AND_NULL(graph_name);
 
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 71d775e313..1cb0355c7e 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -400,6 +400,12 @@ corrupt_graph_and_verify() {
 
 }
 
+test_expect_success POSIXPERM,SANITY 'detect permission problem' '
+	corrupt_graph_setup &&
+	chmod 000 $objdir/info/commit-graph &&
+	corrupt_graph_verify "Could not open"
+'
+
 test_expect_success 'detect too small' '
 	corrupt_graph_setup &&
 	echo "a small graph" >$objdir/info/commit-graph &&

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 14, 2019, 21:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 7/8] commit-graph write: don't die if the existing graph is corrupt

When the commit-graph is written we end up calling parse_commit(). This will in turn invoke code that'll consult the existing commit-graph about the commit, if the graph is corrupted we die.

We thus get into a state where a failing "commit-graph verify" can't be followed-up with a "commit-graph write" if core.commitGraph=true is set, the graph either needs to be manually removed to proceed, or core.commitGraph needs to be set to "false".

Change the "commit-graph write" codepath to use a new parse_commit_no_graph() helper instead of parse_commit() to avoid this. The latter will call repo_parse_commit_internal() with use_commit_graph=1 as seen in 177722b344 ("commit: integrate commit graph with commit parsing", 2018-04-10).

Not using the old graph at all slows down the writing of the new graph by some small amount, but is a sensible way to prevent an error in the existing commit-graph from spreading.

Just fixing the current issue would be likely to result in code that's inadvertently broken in the future. New code might use the commit-graph at a distance. To detect such cases introduce a "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD" setting used when we do our corruption tests, and test that a "write/verify" combo works after every one of our current test cases where we now detect commit-graph corruption.

Some of the code changes here might be strictly unnecessary, e.g. I was unable to find cases where the parse_commit() called from write_graph_chunk_data() didn't exit early due to "item->object.parsed" being true in repo_parse_commit_internal() (before the use_commit_graph=1 has any effect). But let's also convert those cases for good measure, we do not have exhaustive tests for all possible types of commit-graph corruption.

This might need to be re-visited if we learn to write the commit-graph incrementally, but probably not. Hopefully we'll just start by finding out what commits we have in total, then read the old graph(s) to see what they cover, and finally write a new graph file with everything that's missing. In that case the new graph writing code just needs to continue to use e.g. a parse_commit() that doesn't consult the existing commit-graphs.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit-graph.c          | 10 +++++++---
 commit-graph.h          |  1 +
 commit.h                |  6 ++++++
 t/t5318-commit-graph.sh | 11 +++++++++--
 4 files changed, 23 insertions(+), 5 deletions(-)
Show changes to 4 files +23 −5

commit-graph.c, commit-graph.h, commit.h, t/t5318-commit-graph.sh

diff --git a/commit-graph.c b/commit-graph.c
index d945e8f3e0..6b3ade9496 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -281,6 +281,10 @@ static int prepare_commit_graph(struct repository *r)
 	struct object_directory *odb;
 	int config_value;
 
+	if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))
+		die("Dying as requested by the '%s' variable on commit-graph load!",
+		    GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);
+
 	if (r->objects->commit_graph_attempted)
 		return !!r->objects->commit_graph;
 	r->objects->commit_graph_attempted = 1;
@@ -545,7 +549,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
 		uint32_t packedDate[2];
 		display_progress(progress, ++*progress_cnt);
 
-		parse_commit(*list);
+		parse_commit_no_graph(*list);
 		hashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);
 
 		parent = (*list)->parents;
@@ -742,7 +746,7 @@ static void close_reachable(struct packed_oid_list *oids, int report_progress)
 		display_progress(progress, i + 1);
 		commit = lookup_commit(the_repository, &oids->list[i]);
 
-		if (commit && !parse_commit(commit))
+		if (commit && !parse_commit_no_graph(commit))
 			add_missing_parents(oids, commit);
 	}
 	stop_progress(&progress);
@@ -991,7 +995,7 @@ void write_commit_graph(const char *obj_dir,
 			continue;
 
 		commits.list[commits.nr] = lookup_commit(the_repository, &oids.list[i]);
-		parse_commit(commits.list[commits.nr]);
+		parse_commit_no_graph(commits.list[commits.nr]);
 
 		for (parent = commits.list[commits.nr]->parents;
 		     parent; parent = parent->next)
diff --git a/commit-graph.h b/commit-graph.h
index 36d8109901..6021ababa2 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -7,6 +7,7 @@
 #include "cache.h"
 
 #define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH"
+#define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD"
 
 struct commit;
 
diff --git a/commit.h b/commit.h
index 42728c2906..5d33477e78 100644
--- a/commit.h
+++ b/commit.h
@@ -89,6 +89,12 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)
 {
 	return repo_parse_commit_gently(r, item, 0);
 }
+
+static inline int parse_commit_no_graph(struct commit *commit)
+{
+	return repo_parse_commit_internal(the_repository, commit, 0, 0);
+}
+
 #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
 #define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use)
 #define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet)
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 1cb0355c7e..d146cf4982 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -377,7 +377,13 @@ corrupt_graph_verify() {
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
 	test_i18ngrep "$grepstr" err &&
-	git status --short
+	if test "$2" != "no-copy"
+	then
+		cp $objdir/info/commit-graph commit-graph-pre-write-test
+	fi &&
+	git status --short &&
+	GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&
+	git commit-graph verify
 }
 
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
@@ -403,7 +409,7 @@ corrupt_graph_and_verify() {
 test_expect_success POSIXPERM,SANITY 'detect permission problem' '
 	corrupt_graph_setup &&
 	chmod 000 $objdir/info/commit-graph &&
-	corrupt_graph_verify "Could not open"
+	corrupt_graph_verify "Could not open" "no-copy"
 '
 
 test_expect_success 'detect too small' '
@@ -522,6 +528,7 @@ test_expect_success 'git fsck (checks commit-graph)' '
 	git fsck &&
 	corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
 		"incorrect checksum" &&
+	cp commit-graph-pre-write-test $objdir/info/commit-graph &&
 	test_must_fail git fsck
 '
 

-- 
2.21.0.360.g471c308f928
Ævar Arnfjörð Bjarmason· Mar 14, 2019, 21:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 8/8] commit-graph: improve & i18n error messages

Change the error emitted when a commit-graph file is corrupt so that we actually mention the commit-graph, e.g. change errors like:

    error: improper chunk offset 0000000000385e0c
To:
    error: commit-graph improper chunk offset 0000000000385e0c

As discussed in the commits leading up to this one the commit-graph machinery is now used by common commands like "status". If the graph was corrupt we'd often emit some error that gave no indication what was wrong. Now some of them are still cryptic, but they'll at least mention "commit-graph" to give the user a hint as to where to look.

While I'm at it mark some of the strings that hadn't been marked for translation. It's clear from the commit history and the code that this was merely forgotten at the time, and wasn't intentional.p5

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit-graph.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)
Show changes to commit-graph.c +19 −19
diff --git a/commit-graph.c b/commit-graph.c
index 6b3ade9496..4cf7d6c294 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -150,21 +150,21 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 
 	graph_signature = get_be32(data);
 	if (graph_signature != GRAPH_SIGNATURE) {
-		error(_("graph signature %X does not match signature %X"),
+		error(_("commit-graph signature %X does not match signature %X"),
 		      graph_signature, GRAPH_SIGNATURE);
 		return NULL;
 	}
 
 	graph_version = *(unsigned char*)(data + 4);
 	if (graph_version != GRAPH_VERSION) {
-		error(_("graph version %X does not match version %X"),
+		error(_("commit-graph version %X does not match version %X"),
 		      graph_version, GRAPH_VERSION);
 		return NULL;
 	}
 
 	hash_version = *(unsigned char*)(data + 5);
 	if (hash_version != oid_version()) {
-		error(_("hash version %X does not match version %X"),
+		error(_("commit-graph hash version %X does not match version %X"),
 		      hash_version, oid_version());
 		return NULL;
 	}
@@ -187,7 +187,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 
 		if (data + graph_size - chunk_lookup <
 		    GRAPH_CHUNKLOOKUP_WIDTH) {
-			error(_("chunk lookup table entry missing; graph file may be incomplete"));
+			error(_("commit-graph chunk lookup table entry missing; file may be incomplete"));
 			free(graph);
 			return NULL;
 		}
@@ -198,7 +198,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 		chunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;
 
 		if (chunk_offset > graph_size - the_hash_algo->rawsz) {
-			error(_("improper chunk offset %08x%08x"), (uint32_t)(chunk_offset >> 32),
+			error(_("commit-graph improper chunk offset %08x%08x"), (uint32_t)(chunk_offset >> 32),
 			      (uint32_t)chunk_offset);
 			free(graph);
 			return NULL;
@@ -235,7 +235,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 		}
 
 		if (chunk_repeated) {
-			error(_("chunk id %08x appears multiple times"), chunk_id);
+			error(_("commit-graph chunk id %08x appears multiple times"), chunk_id);
 			free(graph);
 			return NULL;
 		}
@@ -1162,7 +1162,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
 
 		if (i && oidcmp(&prev_oid, &cur_oid) >= 0)
-			graph_report("commit-graph has incorrect OID order: %s then %s",
+			graph_report(_("commit-graph has incorrect OID order: %s then %s"),
 				     oid_to_hex(&prev_oid),
 				     oid_to_hex(&cur_oid));
 
@@ -1172,14 +1172,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 			uint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);
 
 			if (i != fanout_value)
-				graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",
+				graph_report(_("commit-graph has incorrect fanout value: fanout[%d] = %u != %u"),
 					     cur_fanout_pos, fanout_value, i);
 			cur_fanout_pos++;
 		}
 
 		graph_commit = lookup_commit(r, &cur_oid);
 		if (!parse_commit_in_graph_one(r, g, graph_commit))
-			graph_report("failed to parse %s from commit-graph",
+			graph_report(_("failed to parse commit %s from commit-graph"),
 				     oid_to_hex(&cur_oid));
 	}
 
@@ -1187,7 +1187,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		uint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);
 
 		if (g->num_commits != fanout_value)
-			graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",
+			graph_report(_("commit-graph has incorrect fanout value: fanout[%d] = %u != %u"),
 				     cur_fanout_pos, fanout_value, i);
 
 		cur_fanout_pos++;
@@ -1209,14 +1209,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		graph_commit = lookup_commit(r, &cur_oid);
 		odb_commit = (struct commit *)create_object(r, cur_oid.hash, alloc_commit_node(r));
 		if (parse_commit_internal(odb_commit, 0, 0)) {
-			graph_report("failed to parse %s from object database",
+			graph_report(_("failed to parse commit %s from object database for commit-graph"),
 				     oid_to_hex(&cur_oid));
 			continue;
 		}
 
 		if (!oideq(&get_commit_tree_in_graph_one(r, g, graph_commit)->object.oid,
 			   get_commit_tree_oid(odb_commit)))
-			graph_report("root tree OID for commit %s in commit-graph is %s != %s",
+			graph_report(_("root tree OID for commit %s in commit-graph is %s != %s"),
 				     oid_to_hex(&cur_oid),
 				     oid_to_hex(get_commit_tree_oid(graph_commit)),
 				     oid_to_hex(get_commit_tree_oid(odb_commit)));
@@ -1226,13 +1226,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 
 		while (graph_parents) {
 			if (odb_parents == NULL) {
-				graph_report("commit-graph parent list for commit %s is too long",
+				graph_report(_("commit-graph parent list for commit %s is too long"),
 					     oid_to_hex(&cur_oid));
 				break;
 			}
 
 			if (!oideq(&graph_parents->item->object.oid, &odb_parents->item->object.oid))
-				graph_report("commit-graph parent for %s is %s != %s",
+				graph_report(_("commit-graph parent for %s is %s != %s"),
 					     oid_to_hex(&cur_oid),
 					     oid_to_hex(&graph_parents->item->object.oid),
 					     oid_to_hex(&odb_parents->item->object.oid));
@@ -1245,16 +1245,16 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 		}
 
 		if (odb_parents != NULL)
-			graph_report("commit-graph parent list for commit %s terminates early",
+			graph_report(_("commit-graph parent list for commit %s terminates early"),
 				     oid_to_hex(&cur_oid));
 
 		if (!graph_commit->generation) {
 			if (generation_zero == GENERATION_NUMBER_EXISTS)
-				graph_report("commit-graph has generation number zero for commit %s, but non-zero elsewhere",
+				graph_report(_("commit-graph has generation number zero for commit %s, but non-zero elsewhere"),
 					     oid_to_hex(&cur_oid));
 			generation_zero = GENERATION_ZERO_EXISTS;
 		} else if (generation_zero == GENERATION_ZERO_EXISTS)
-			graph_report("commit-graph has non-zero generation number for commit %s, but zero elsewhere",
+			graph_report(_("commit-graph has non-zero generation number for commit %s, but zero elsewhere"),
 				     oid_to_hex(&cur_oid));
 
 		if (generation_zero == GENERATION_ZERO_EXISTS)
@@ -1269,13 +1269,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 			max_generation--;
 
 		if (graph_commit->generation != max_generation + 1)
-			graph_report("commit-graph generation for commit %s is %u != %u",
+			graph_report(_("commit-graph generation for commit %s is %u != %u"),
 				     oid_to_hex(&cur_oid),
 				     graph_commit->generation,
 				     max_generation + 1);
 
 		if (graph_commit->date != odb_commit->date)
-			graph_report("commit date for commit %s in commit-graph is %"PRItime" != %"PRItime,
+			graph_report(_("commit date for commit %s in commit-graph is %"PRItime" != %"PRItime),
 				     oid_to_hex(&cur_oid),
 				     graph_commit->date,
 				     odb_commit->date);

-- 
2.21.0.360.g471c308f928

← back to recent threads