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

[Jeff King <peff@peff.net>]

On Mon, May 04, 2020 at 07:13:49PM -0600, Taylor Blau wrote:

> When given a list of commits, the commit-graph machinery calls
> 'lookup_commit_reference_gently()' on each element in the set and treats
> the resulting set of OIDs as the base over which to close for
> reachability.
> 
> In an earlier collection of commits, the 'git commit-graph write
> --reachable' case made the inner-most call to
> 'lookup_commit_reference_gently()' by peeling references before they
> were passed over to the commit-graph internals.
> 
> Do the analog for 'git commit-graph write --stdin-commits' by calling
> 'lookup_commit_reference_gently()' outside of the commit-graph
> machinery, making the inner-most call a noop.

Yep, I think this is a good direction.

> @@ -148,7 +151,15 @@ static int read_one_commit(struct oidset *commits, char *hash)
>  		return 1;
>  	}
>  
> -	oidset_insert(commits, &oid);
> +	display_progress(progress, oidset_size(commits) + 1);

Most of our meters increment progress _after_ doing work.

This is especially important for meters with percentages (if we knew we
had 1 commit, we'd print "100%" and _then_ start to peel it, which is
silly).

For this instance we don't know the total number, so we're just counting
up. But I think we should be consistent about when we update meters.
Plus it's shorter to say:

  display_progress(progress, oidset_size(commits));

after having done the work. ;)

> +	result = lookup_commit_reference_gently(the_repository, &oid, 1);

Would we want to pass quiet==0 here? If we see an error we're going to
bail loudly below, so getting more details from the low-level functions
is helpful (I think the only one you'd really get is "I'm looking for a
commit, but it's a tree" or similar).

lookup_commit_reference_gently() is pretty aggressive about parsing
objects. We'll have to parse commits eventually, but we could possibly
do so using their graph representations. It may not be worth optimizing,
because it would only matter if you fed a lot of --stdin-commits inputs
that were already graphed. (And if it is worth optimizing, it should
probably come in a separate commit anyway; this is just about moving the
existing peeling).

> +	if (result)
> +		oidset_insert(commits, &result->object.oid);
> +	else {
> +		error(_("invalid commit object id: %s"), hash);
> +		return 1;
> +	}

If you follow my "return -1" suggestion from earlier, this would need
it, too.

>  		while (strbuf_getline(&buf, stdin) != EOF) {
>  			char *line = strbuf_detach(&buf, NULL);
>  			if (opts.stdin_commits) {
> -				int result = read_one_commit(&commits, line);
> +				int result = read_one_commit(&commits, progress,
> +							     line);
>  				if (result)
>  					return result;
>  			} else
>  				string_list_append(&pack_indexes, line);
>  		}
>  
> +		if (progress)
> +			stop_progress(&progress);

If we return early in the loop, we'd leave this progress meter hanging.
It might be worth converting that return to a break or goto that handles
cleanup (it also needs to handle releasing the strbuf).

-Peff