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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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@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/