On Mon, Mar 25 2019, Ævar Arnfjörð Bjarmason wrote:
> 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@xxxxxxxxx>
> Signed-off-by: Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxx/
