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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2019, 21:47 UTC
Message-ID: <xmqqy329kt9j.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <fc3423046b9b2fc43bf8323dd41281f0a29b9d0a.1559932699.git.gitgitgadget@gmail.com>

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

Show 35 quoted lines

> +	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.

Show 33 quoted lines

> +
> +	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);
>  }
>  
>  /*