On Wed, Sep 05 2018, Derrick Stolee wrote:
> On 9/4/2018 6:07 PM, Junio C Hamano wrote:
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>>
>>> With --stdin-packs we don't show any estimation of how much is left to
>>> do. This is because we might be processing more than one pack. We
>>> could be less lazy here and show progress, either detect by detecting
>>> that we're only processing one pack, or by first looping over the
>>> packs to discover how many commits they have. I don't see the point in
>> I do not know if there is no point, but if we were to do it, I think
>> slurping the list of packs and computing the number of objects is
>> not all that bad.
>
> If you want to do that, I have nothing against it. However, I don't
> expect users to use that option directly. That option is used by VFS
> for Git to compute the commit-graph in the background after receiving
> a pack of commits and trees, but not by 'git gc' which I expect is how
> most users will compute commit-graphs.
Yeah, I suspected only one guy at Microsoft would potentially benefit
from this, but added it just so we'd have progress regardless of entry
point :)
>>> static void compute_generation_numbers(struct packed_commit_list* commits)
>>> {
>>> int i;
>>> struct commit_list *list = NULL;
>>> + struct progress *progress = NULL;
>>> + progress = start_progress(
>>> + _("Computing commit graph generation numbers"), commits->nr);
>>> for (i = 0; i < commits->nr; i++) {
>>> + display_progress(progress, i);
>>> if (commits->list[i]->generation != GENERATION_NUMBER_INFINITY &&
>>> commits->list[i]->generation != GENERATION_NUMBER_ZERO)
>>> continue;
>> I am wondering if the progress call should be moved after this
>> conditional continue; would we want to count the entry whose
>> generation is already known here? Of course, as we give commits->nr
>> as the 100% ceiling, we cannot avoid doing so, but it somehow smells
>> wrong.
>
> If we wanted to be completely right, we would count the commits in the
> list that do not have a generation number and report that as the 100%
> ceiling.
>
> Something like the diff below would work. I tested it in Linux by
> first deleting my commit-graph and running the following:
>
> stolee@stolee-linux:~/linux$ rm .git/objects/info/commit-graph
> stolee@stolee-linux:~/linux$ git rev-parse v4.6 | ~/git/git
> commit-graph write --stdin-commits
> Annotating commits in commit graph: 1180333, done.
> Computing commit graph generation numbers: 100% (590166/590166), done.
> stolee@stolee-linux:~/linux$ ~/git/git commit-graph write --reachable
> Annotating commits in commit graph: 1564087, done.
> Computing commit graph generation numbers: 100% (191590/191590), done.
>
> -->8--
>
> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> Date: Wed, 5 Sep 2018 11:55:42 +0000
> Subject: [PATCH] fixup! commit-graph write: add progress output
>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
> commit-graph.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 1a02fe019a..b933bc9f00 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -634,14 +634,20 @@ static void close_reachable(struct
> packed_oid_list *oids)
>
> static void compute_generation_numbers(struct packed_commit_list* commits)
> {
> - int i;
> + int i, count_uncomputed = 0;
> struct commit_list *list = NULL;
> struct progress *progress = NULL;
>
> + for (i = 0; i < commits->nr; i++)
> + if (commits->list[i]->generation ==
> GENERATION_NUMBER_INFINITY ||
> + commits->list[i]->generation == GENERATION_NUMBER_ZERO)
> + count_uncomputed++;
> +
> progress = start_progress(
> - _("Computing commit graph generation numbers"),
> commits->nr);
> + _("Computing commit graph generation numbers"),
> count_uncomputed);
> + count_uncomputed = 0;
> +
> for (i = 0; i < commits->nr; i++) {
> - display_progress(progress, i);
> if (commits->list[i]->generation !=
> GENERATION_NUMBER_INFINITY &&
> commits->list[i]->generation != GENERATION_NUMBER_ZERO)
> continue;
> @@ -670,10 +676,11 @@ static void compute_generation_numbers(struct
> packed_commit_list* commits)
>
> if (current->generation >
> GENERATION_NUMBER_MAX)
> current->generation =
> GENERATION_NUMBER_MAX;
> +
> + display_progress(progress,
> ++count_uncomputed);
> }
> }
> }
> - display_progress(progress, i);
> stop_progress(&progress);
> }
Thanks! That looks good, and you obviously know this code a lot
better. I'll squash this into v2 pending further feedback I'll need to
address.
