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:

> @@ -560,6 +563,9 @@ static int add_packed_commits(const struct object_id *oid,
>  	off_t offset = nth_packed_object_offset(pack, pos);
>  	struct object_info oi = OBJECT_INFO_INIT;
>  
> +	if (list->progress)
> +		display_progress(list->progress, ++list->progress_done);

Note that add_packed_commits() is used as a callback function for
for_each_object_in_pack() (with '--stdin-packs') or
for_each_packed_object() (no options), i.e. this will count the number
of objects, not commits:

  $ git rev-list --all |wc -l
  768524
  $ git rev-list --objects --all |wc -l
  6130295
  # '--count --objects' together didn't work as expected.
  $ time ~/src/git/git commit-graph write
  Finding commits for commit graph: 6130295, done.
  Annotating commits in commit graph: 2305572, done.
  Computing commit graph generation numbers: 100% (768524/768524), done.

(Now I also see the 3x difference in the "Annotating commits" counter
that you mentioned.)

I see two options:

  - Provide a different title for this progress counter, e.g.
    "Scanning objects for c-g", or "Processing objects...", or
    something else that says "objects" instead of "commits".

  - Move this condition and display_progress() call to the end of the
    function, so it will only count commits, not any other objects.
    (As far as I understand both for_each_object_in_pack() and
    for_each_packed_object() iterate in pack .idx order, i.e. it's
    essentially random.  This means that commit objects should be
    distributed evenly among other kinds of objects, so we don't have
    to worry about the counter stalling for a long stretch of
    consecutive non-commit objects.  At least in theory.)