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

[Taylor Blau <me@ttaylorr.com>]

On Thu, May 14, 2020 at 02:01:50PM -0400, Jeff King wrote:
> On Wed, May 13, 2020 at 03:59:44PM -0600, Taylor Blau wrote:
>
> > -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.

I went back and forth on this. I figure that the behavior change should
come in the final commit, so this is keeping things consistent at this
point in the series.

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

Mm. If there's nothing else (besides the review of 4/8 which I already
sent a replacement patch for), I'd prefer to leave this as-is to avoid
another reroll. Hopefully since this isn't hurting anything with the
extra if statement, it shouldn't matter too much.

If you feel strongly, I'm happy to re-send this series, or this patch.

> -Peff

Thanks,
Taylor