Re: [PATCH v5 04/16] commit-graph: load commit-graph chains

[Junio C Hamano <gitster@pobox.com>]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +	fp = fopen(chain_name, "r");
> +	stat_res = stat(chain_name, &st);
> +	free(chain_name);
> +
> +	if (!fp ||
> +	    stat_res ||
> +	    st.st_size <= the_hash_algo->hexsz)
> +		return NULL;
> +
> +	count = st.st_size / (the_hash_algo->hexsz + 1);
> +	oids = xcalloc(count, sizeof(struct object_id));
> +
> +	for (i = 0; i < count && valid; i++) {
> +		char *graph_name;
> +		struct commit_graph *g;
> +
> +		if (strbuf_getline_lf(&line, fp) == EOF)
> +			break;
> +
> +		if (get_oid_hex(line.buf, &oids[i])) {
> +			warning(_("invalid commit-graph chain: line '%s' not a hash"),
> +				line.buf);
> +			valid = 0;
> +			break;
> +		}
> +
> +		graph_name = get_split_graph_filename(obj_dir, line.buf);
> +		g = load_commit_graph_one(graph_name);
> +		free(graph_name);
> +
> +		if (g && add_graph_to_chain(g, graph_chain, oids, i))
> +			graph_chain = g;
> +		else
> +			valid = 0;
> +	}

At this point, if 'i' is smaller than 'count', that would indicate
that the file was corrupt (we hit one of the 'break' in the loop).

How would we handle such an error?  It appears that the strategy
taken in this loop is to "read as many as possible without an error
and then give up upon the first error---keep whatever we have read
so far", so from that point of view, the only thing that is missing
may be a warning() after the loop.

> +
> +	free(oids);
> +	fclose(fp);
> +
> +	return graph_chain;
> +}
> +
> +static struct commit_graph *read_commit_graph_one(struct repository *r, const char *obj_dir)
> +{
> +	struct commit_graph *g = load_commit_graph_v1(r, obj_dir);
> +
> +	if (!g)
> +		g = load_commit_graph_chain(r, obj_dir);
> +
> +	return g;
> +}
> +
>  static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
>  {
> -	char *graph_name;
>  
>  	if (r->objects->commit_graph)
>  		return;
>  
> -	graph_name = get_commit_graph_filename(obj_dir);
> -	r->objects->commit_graph =
> -		load_commit_graph_one(graph_name);
> -
> -	FREE_AND_NULL(graph_name);
> +	r->objects->commit_graph = read_commit_graph_one(r, obj_dir);
>  }
>  
>  /*