[10/11] pack-bitmap-write.c: don't return without stop_progress()

Commit Message

Taylor Blau Oct. 21, 2021, 3:40 a.m. UTC

From: Ævar Arnfjörð Bjarmason <[e-mail]>

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 <[e-mail]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
Signed-off-by: Taylor Blau <[e-mail]>
---
 pack-bitmap-write.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Eric Sunshine Oct. 21, 2021, 5:12 a.m. UTC | #1

On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau <[e-mail]> wrote:
> From: Ævar Arnfjörð Bjarmason <[e-mail]>
>
> 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.

Nit: There is no subsequent commit in this series which "BUGS out", so
this comment is confusing.

> Suggested-by: SZEDER Gábor <[e-mail]>
> Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
> Signed-off-by: Taylor Blau <[e-mail]>

Ævar Arnfjörð Bjarmason Oct. 21, 2021, 11:31 a.m. UTC | #2

On Wed, Oct 20 2021, Taylor Blau wrote:

> From: Ævar Arnfjörð Bjarmason <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
> Signed-off-by: Taylor Blau <[e-mail]>
> ---
>  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 9c55c1531e..cab3eaa2ac 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);
> +
>  	for (;;) {
>  		struct commit *chosen = NULL;

Looks good :) Just a note that this is in
"ab/only-single-progress-at-once" already in case you didn't spot it.

That series is marked for "next?" (with a question mark) by Junio in the
latest what's cooking, so *hint* *hint* for any last minute review :)

Junio C Hamano Oct. 21, 2021, 6:39 p.m. UTC | #3

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

> Looks good :) Just a note that this is in
> "ab/only-single-progress-at-once" already in case you didn't spot it.
>
> That series is marked for "next?" (with a question mark) by Junio in the
> latest what's cooking, so *hint* *hint* for any last minute review :)

I wonder if it would help contributors if we give them a simple rule
to follow before sending their patches out:

 - You develop on an appropriate base (either on maint, or master,
   or a merge of selected topics in flight you absolutely have to
   depend on) as usual.

 - You test merge the result to 'seen' and to 'next'.  If you do not
   get heavy conflicts and the tests pass, you are OK.

 - Otherwise, you study what is conflicting, and should review it
   before you submit your topic.

Without understanding what others are doing in the same area, it is
hard to make productive use of your time to develop new things, and
if you are reading others' work in the same area anyway, it would be
a minimum additional cost to write about what you learned in their
work.

Taylor Blau Oct. 22, 2021, 4:32 a.m. UTC | #4

On Thu, Oct 21, 2021 at 11:39:34AM -0700, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <[e-mail]> writes:
>
> > Looks good :) Just a note that this is in
> > "ab/only-single-progress-at-once" already in case you didn't spot it.
> >
> > That series is marked for "next?" (with a question mark) by Junio in the
> > latest what's cooking, so *hint* *hint* for any last minute review :)
>
> I wonder if it would help contributors if we give them a simple rule
> to follow before sending their patches out:
>
>  - You develop on an appropriate base (either on maint, or master,
>    or a merge of selected topics in flight you absolutely have to
>    depend on) as usual.

That seems like sensible advice to me. When I sent the series out last
night, I figured that it would be preferable to cherry-pick the commit
from Ævar's series, since it wasn't clear whether or not all or parts of
that series were actively moving forward (besides being marked as
"next?" in your What's Cooking email).

But I'll send the next iteration based on a combined merge of my topic
and Ævar's, and note your branch names for both of them clearly. To
encourage both of our topics to move forward, I'll throw Ævar's onto my
review queue.

Thanks,
Taylor

Junio C Hamano Oct. 23, 2021, 8:28 p.m. UTC | #5

Junio C Hamano <[e-mail]> writes:

>> Looks good :) Just a note that this is in
>> "ab/only-single-progress-at-once" already in case you didn't spot it.
>>
>> That series is marked for "next?" (with a question mark) by Junio in the
>> latest what's cooking, so *hint* *hint* for any last minute review :)
>
> I wonder if it would help contributors if we give them a simple rule
> to follow before sending their patches out:
>
>  - You develop on an appropriate base (either on maint, or master,
>    or a merge of selected topics in flight you absolutely have to
>    depend on) as usual.
>
>  - You test merge the result to 'seen' and to 'next'.  If you do not
>    get heavy conflicts and the tests pass, you are OK.
>
>  - Otherwise, you study what is conflicting, and should review it
>    before you submit your topic.

Addendum to this third step.

     ... Also, if you chose to depend on other topics when you
     decided on the base in the first step, giving them your review
     would be a good way to show others that _you_ understand the
     code that you are building on top of, which in turn may give
     others confidence in your topic.

Thanks.

Patch

diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 9c55c1531e..cab3eaa2ac 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);
+
 	for (;;) {
 		struct commit *chosen = NULL;