Re: [PATCH v3 5/8] builtin/commit-graph.c: dereference tags in builtin

From: Jeff King <peff@peff.net>
Date: May 14, 2020, 18:01 UTC
Message-ID: <20200514180150.GB2430834@coredump.intra.peff.net>
In-Reply-To: <56403dd377b996d21a4da1e01ffd6e691ac120bd.1589407014.git.me@ttaylorr.com>

On Wed, May 13, 2020 at 03:59:44PM -0600, Taylor Blau wrote:

Show 20 quoted lines

> -static int read_one_commit(struct oidset *commits, const char *hash)
> +static int read_one_commit(struct oidset *commits, struct progress *progress,
> +			   const char *hash)
>  {
> +	struct commit *result;
>  	struct object_id oid;
>  	const char *end;
>  
>  	if (parse_oid_hex(hash, &oid, &end))
>  		return error(_("unexpected non-hex object ID: %s"), hash);
>  
> -	oidset_insert(commits, &oid);
> +	result = lookup_commit_reference_gently(the_repository, &oid, 1);
> +	if (result)
> +		oidset_insert(commits, &result->object.oid);
> +	else
> +		return error(_("invalid commit object id: %s"), hash);
> +
> +	display_progress(progress, oidset_size(commits));
> +

I expected this to switch to deref_tag() here, but it looks like you do it in the final commit. That makes sense, since this step is really just copying the existing logic.

Show 7 quoted lines

> @@ -249,6 +265,8 @@ static int graph_write(int argc, const char **argv)
>  cleanup:
>  	UNLEAK(pack_indexes);
>  	strbuf_release(&buf);
> +	if (progress)
> +		stop_progress(&progress);
>  	return result;

Really minor nit, but stop_progress(), like display_progress(), handles NULL already. So you can lose the "if" here.

-Peff