Re: [PATCH v3 1/2] commit-graph write: add progress output

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Mon, Sep 17, 2018 at 03:33:35PM +0000, Ævar Arnfjörð Bjarmason wrote:
>     $ git -c gc.writeCommitGraph=true gc
>     [...]
>     Annotating commits in commit graph: 1565573, done.
>     Computing commit graph generation numbers: 100% (782484/782484), done.

While poking around 'commit-graph.c' in my Bloom filter experiment, I
saw similar numbers like above, and was confused by the much higher
than expected number of annotated commits.  It's about twice as much
as the number of commits in the repository, or the number shown on the
very next line.

> diff --git a/commit-graph.c b/commit-graph.c
> index 8a1bec7b8a..2c5d996194 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> -static void close_reachable(struct packed_oid_list *oids)
> +static void close_reachable(struct packed_oid_list *oids, int report_progress)
>  {
>  	int i;
>  	struct commit *commit;
> +	struct progress *progress = NULL;
> +	int j = 0;
>  
> +	if (report_progress)
> +		progress = start_delayed_progress(
> +			_("Annotating commits in commit graph"), 0);
>  	for (i = 0; i < oids->nr; i++) {
> +		display_progress(progress, ++j);
>  		commit = lookup_commit(the_repository, &oids->list[i]);
>  		if (commit)
>  			commit->object.flags |= UNINTERESTING;
> @@ -604,6 +616,7 @@ static void close_reachable(struct packed_oid_list *oids)
>  	 * closure.
>  	 */
>  	for (i = 0; i < oids->nr; i++) {
> +		display_progress(progress, ++j);
>  		commit = lookup_commit(the_repository, &oids->list[i]);
>  
>  		if (commit && !parse_commit(commit))
> @@ -611,19 +624,28 @@ static void close_reachable(struct packed_oid_list *oids)
>  	}

The above loops have already counted all the commits, and, more
importantly, did all the hard work that takes time and makes the
progress indicator useful.

>  	for (i = 0; i < oids->nr; i++) {
> +		display_progress(progress, ++j);

This display_progress() call, however, doesn't seem to be necessary.
First, it counts all commits for a second time, resulting in the ~2x
difference compared to the actual number of commits, and then causing
my confusion.  Second, all what this loop is doing is setting a flag
in commits that were already looked up and parsed in the above loops.
IOW this loop is very fast, and the progress indicator jumps from
~780k right to 1.5M, even on my tiny laptop, so it doesn't need a
progress indicator at all.

>  		commit = lookup_commit(the_repository, &oids->list[i]);
>  
>  		if (commit)
>  			commit->object.flags &= ~UNINTERESTING;
>  	}
> +	stop_progress(&progress);
>  }