Thread (191 messages) flat view 191 messages, 12 authors, 2022-02-03

Re: [PATCH v3 08/10] pack-bitmap-write.c: don't return without stop_progress()

From: Taylor Blau <hidden>
Date: 2021-10-22 22:47:23

On Thu, Oct 14, 2021 at 12:28:24AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted hunk ↗ jump to hunk
Fix a bug that's been here since 7cc8f971085 (pack-objects: implement
bitmap writing, 2013-12-21), we did not call stop_progress() if we
reached the early exit in this function.

We could call stop_progress() before we return, but better yet is to
defer calling start_progress() until we need it.

This will matter in a subsequent commit where we BUG(...) out if this
happens, and matters now e.g. because we don't have a corresponding
"region_end" for the progress trace2 event.

Suggested-by: SZEDER Gábor <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 pack-bitmap-write.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 9c55c1531e1..cab3eaa2acd 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -575,15 +575,15 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,

 	QSORT(indexed_commits, indexed_commits_nr, date_compare);

-	if (writer.show_progress)
-		writer.progress = start_progress("Selecting bitmap commits", 0);
-
 	if (indexed_commits_nr < 100) {
 		for (i = 0; i < indexed_commits_nr; ++i)
 			push_bitmapped_commit(indexed_commits[i]);
 		return;
 	}

+	if (writer.show_progress)
+		writer.progress = start_progress("Selecting bitmap commits", 0);
+
Ah, this is the change that you and I discovered independently. For what
it's worth, I wrote something slightly differently, which was to jump to
a "cleanup" label and then call stop_progress() there instead of waiting
to call start_progress().

But thinking on it more, I think that difference highlights a compelling
reason to get rid of this progress meter entirely. That's because this
rule of "if there are fewer than 100 commits, select all of them"
doesn't even update the progress meter!

And the loop that it is timing below is so fast that we end up just
showing the final count, which doesn't provide any useful information
beyond how many commits *could* have been selected.

So I think that your fix is OK, and I'd be happy to see this get picked
up as-is, but I'd be equally happy to drop this progress meter
altogether.
 	for (;;) {
 		struct commit *chosen = NULL;

--
2.33.1.1346.g48288c3c089
Thanks,
Taylor
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help