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

From: Derrick Stolee <stolee@gmail.com>
To: "Junio C Hamano" <gitster@pobox.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/2] commit-graph write: add progress output
Date: Wed, 5 Sep 2018 07:58:12 -0400	[thread overview]
Message-ID: <c6960252-c095-fb2b-e0bc-b1e6bb261614@gmail.com> (raw)
In-Reply-To: <xmqqin3k7w8b.fsf@gitster-ct.c.googlers.com>

On 9/4/2018 6:07 PM, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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.

>>   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@microsoft.com>
Date: Wed, 5 Sep 2018 11:55:42 +0000
Subject: [PATCH] fixup! commit-graph write: add progress output

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
  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);
  }

--
2.19.0.rc2

