[PATCH v3] blame: report correct number of lines in progress when using ranges

[PATCH v3] blame: report correct number of lines in progress when using ranges

[Edmundo Carmona Antoranz]

When using ranges, use the range sizes as the limit for progress
instead of the size of the full file.

Before:
$ git blame --progress builtin/blame.c > /dev/null
Blaming lines: 100% (1210/1210), done.
$ git blame --progress -L 100,120 -L 200,300 builtin/blame.c > /dev/null
Blaming lines:  10% (122/1210), done.
$

After:
$ ./git blame --progress builtin/blame.c > /dev/null
Blaming lines: 100% (1210/1210), done.
$ ./git blame --progress -L 100,120 -L 200,300 builtin/blame.c > /dev/null
Blaming lines: 100% (122/122), done.
$

Note: Shamelessly copied show_cr from t0500-progress-display.sh
Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
---
 builtin/blame.c     |  6 ++++-
 t/annotate-tests.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 8d15b68afc..e33372c56b 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -898,6 +898,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	unsigned int range_i;
 	long anchor;
 	const int hexsz = the_hash_algo->hexsz;
+	long num_lines = 0;
 
 	setup_default_color_by_age();
 	git_config(git_blame_config, &output_option);
@@ -1129,7 +1130,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	for (range_i = ranges.nr; range_i > 0; --range_i) {
 		const struct range *r = &ranges.ranges[range_i - 1];
 		ent = blame_entry_prepend(ent, r->start, r->end, o);
+		num_lines += (r->end - r->start);
 	}
+	if (!num_lines)
+		num_lines = sb.num_lines;
 
 	o->suspects = ent;
 	prio_queue_put(&sb.commits, o->commit);
@@ -1158,7 +1162,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	sb.found_guilty_entry = &found_guilty_entry;
 	sb.found_guilty_entry_data = &pi;
 	if (show_progress)
-		pi.progress = start_delayed_progress(_("Blaming lines"), sb.num_lines);
+		pi.progress = start_delayed_progress(_("Blaming lines"), num_lines);
 
 	assign_blame(&sb, opt);
 
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 09e86f9ba0..90932e304b 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -12,6 +12,10 @@ else
   }
 fi
 
+show_cr () {
+	tr '\015' Q | sed -e "s/Q/<CR>\\$LF/g"
+}
+
 check_count () {
 	head= &&
 	file='file' &&
@@ -604,3 +608,52 @@ test_expect_success 'blame -L X,-N (non-numeric N)' '
 test_expect_success 'blame -L ,^/RE/' '
 	test_must_fail $PROG -L1,^/99/ file
 '
+
+test_expect_success 'blame progress on a full file' '
+	cat >expect <<-\EOF &&
+	Blaming lines:  10% (1/10)<CR>
+	Blaming lines:  50% (5/10)<CR>
+	Blaming lines:  60% (6/10)<CR>
+	Blaming lines:  90% (9/10)<CR>
+	Blaming lines: 100% (10/10)<CR>
+	Blaming lines: 100% (10/10), done.
+	EOF
+
+	GIT_PROGRESS_DELAY=0 \
+	git blame --progress hello.c 2>stderr &&
+
+	show_cr <stderr >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'blame progress on a single range' '
+	cat >expect <<-\EOF &&
+	Blaming lines:  25% (1/4)<CR>
+	Blaming lines:  75% (3/4)<CR>
+	Blaming lines: 100% (4/4)<CR>
+	Blaming lines: 100% (4/4), done.
+	EOF
+
+	GIT_PROGRESS_DELAY=0 \
+	git blame --progress -L 3,6 hello.c 2>stderr &&
+
+	show_cr <stderr >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'blame progress on multiple ranges' '
+	cat >expect <<-\EOF &&
+	Blaming lines:  42% (3/7)<CR>
+	Blaming lines:  57% (4/7)<CR>
+	Blaming lines:  85% (6/7)<CR>
+	Blaming lines: 100% (7/7)<CR>
+	Blaming lines: 100% (7/7), done.
+	EOF
+
+	GIT_PROGRESS_DELAY=0 \
+	git blame --progress -L 3,6 -L 8,10 hello.c 2>stderr &&
+
+	cp stderr /home/antoranz/proyectos/git/git/stderr &&
+	show_cr <stderr >actual &&
+	test_cmp expect actual
+'
-- 
2.35.1

Re: [PATCH v3] blame: report correct number of lines in progress when using ranges

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 05 2022, Edmundo Carmona Antoranz wrote:

Hint: use --in-reply-to on re-rolls, this is in reply to v2 here:
https://lore.kernel.org/git/20220404182129.33992-1-eantoranz@gmail.com/

Also the --range-diff option to git-format-patch is really helpful,
it'll make a diff between v2 and this v3 and attach it after "--".

Anyway...

> Note: Shamelessly copied show_cr from t0500-progress-display.sh
> Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>

Formatting: \n before the Signed-off-by.

> ---
>  builtin/blame.c     |  6 ++++-
>  t/annotate-tests.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 8d15b68afc..e33372c56b 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -898,6 +898,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  	unsigned int range_i;
>  	long anchor;
>  	const int hexsz = the_hash_algo->hexsz;
> +	long num_lines = 0;
>  
>  	setup_default_color_by_age();
>  	git_config(git_blame_config, &output_option);
> @@ -1129,7 +1130,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  	for (range_i = ranges.nr; range_i > 0; --range_i) {
>  		const struct range *r = &ranges.ranges[range_i - 1];
>  		ent = blame_entry_prepend(ent, r->start, r->end, o);
> +		num_lines += (r->end - r->start);
>  	}
> +	if (!num_lines)
> +		num_lines = sb.num_lines;
>  
>  	o->suspects = ent;
>  	prio_queue_put(&sb.commits, o->commit);
> @@ -1158,7 +1162,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  	sb.found_guilty_entry = &found_guilty_entry;
>  	sb.found_guilty_entry_data = &pi;
>  	if (show_progress)
> -		pi.progress = start_delayed_progress(_("Blaming lines"), sb.num_lines);
> +		pi.progress = start_delayed_progress(_("Blaming lines"), num_lines);
>  
>  	assign_blame(&sb, opt);
>  
> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
> index 09e86f9ba0..90932e304b 100644
> --- a/t/annotate-tests.sh
> +++ b/t/annotate-tests.sh
> @@ -12,6 +12,10 @@ else
>    }
>  fi
>  
> +show_cr () {
> +	tr '\015' Q | sed -e "s/Q/<CR>\\$LF/g"
> +}
> +
>  check_count () {
>  	head= &&
>  	file='file' &&
> @@ -604,3 +608,52 @@ test_expect_success 'blame -L X,-N (non-numeric N)' '
>  test_expect_success 'blame -L ,^/RE/' '
>  	test_must_fail $PROG -L1,^/99/ file
>  '
> +
> +test_expect_success 'blame progress on a full file' '
> +	cat >expect <<-\EOF &&
> +	Blaming lines:  10% (1/10)<CR>
> +	Blaming lines:  50% (5/10)<CR>
> +	Blaming lines:  60% (6/10)<CR>
> +	Blaming lines:  90% (9/10)<CR>
> +	Blaming lines: 100% (10/10)<CR>
> +	Blaming lines: 100% (10/10), done.
> +	EOF
> +
> +	GIT_PROGRESS_DELAY=0 \
> +	git blame --progress hello.c 2>stderr &&
> +
> +	show_cr <stderr >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'blame progress on a single range' '
> +	cat >expect <<-\EOF &&
> +	Blaming lines:  25% (1/4)<CR>
> +	Blaming lines:  75% (3/4)<CR>
> +	Blaming lines: 100% (4/4)<CR>
> +	Blaming lines: 100% (4/4), done.
> +	EOF
> +
> +	GIT_PROGRESS_DELAY=0 \
> +	git blame --progress -L 3,6 hello.c 2>stderr &&
> +
> +	show_cr <stderr >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'blame progress on multiple ranges' '
> +	cat >expect <<-\EOF &&
> +	Blaming lines:  42% (3/7)<CR>
> +	Blaming lines:  57% (4/7)<CR>
> +	Blaming lines:  85% (6/7)<CR>
> +	Blaming lines: 100% (7/7)<CR>
> +	Blaming lines: 100% (7/7), done.
> +	EOF
> +
> +	GIT_PROGRESS_DELAY=0 \
> +	git blame --progress -L 3,6 -L 8,10 hello.c 2>stderr &&
> +
> +	cp stderr /home/antoranz/proyectos/git/git/stderr &&
> +	show_cr <stderr >actual &&
> +	test_cmp expect actual
> +'

We had a small thread that I notice now was off-list in reply to
https://lore.kernel.org/git/220405.86o81flve1.gmgdl@evledraar.gmail.com/. Quoted
below. I assume that was both of our mistakes:

	On Tue, Apr 05 2022, Ævar Arnfjörð Bjarmason wrote:
	
	> On Tue, Apr 05 2022, Edmundo Carmona Antoranz wrote:
	>
	>> On Tue, Apr 5, 2022 at 11:41 AM Ævar Arnfjörð Bjarmason
	>> <avarab@gmail.com> wrote:
	>>>
	>>>
	>>> Let's use test_cmp here instead, as we expect nothing else on stderr,
	>>> and with grep one wonders why it's not ^$ anchored, but just:
	>>>
	>>>     echo "Blaming lines: 100% (6/6), done." >expect &&
	>>>     git blame ... 2>actual &&
	>>>     test_cmp expect actual
	>>>
	>>> is better, both because it's more exhaustive as a test, and because
	>>> it'll give better debug (diff) output on failure than grep will (just no
	>>> output at all).
	>>>
	>>
	>> The problem is that progress output is using CRs to write each line
	>> so, when checking the output, if you tried with ^$ with the last line,
	>> it wouldn't match anyway. I switched to match progress output as a
	>> whole using the same technique that is used in
	>> t0500-progress-display.sh.
	>>
	>> v3 is already out there.
	>
	> Ah yes, I forgot about that. Nevermind

I.e. the test_cmp here is now, given what you mentioned I'd have been
fine with the grep, but stealing the show_cr also works.

I suppose it's also abetter as a targeted fix, since the point of this
patch is specifically to fix a bug where we wouldn't do the right
"steps" in-between with the progress bar, in addition to the end-state
not being correct.

Are the small number of missing steps above expected? E.g. 1-2/7 and 5/7
in the last tets above, ditto the rest?

Mm, yes, looking at assign_blame() in blame.c we'll "skip" some.

So if we ever change that algorithm we'll need to adjust these, but it's
probably good to notice that then, even if the test_cmp here does
implicitly encode a bit of internal implementation details, i.e. when
exactly we update the progress bar.

Re: [PATCH v3] blame: report correct number of lines in progress when using ranges

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Are the small number of missing steps above expected? E.g. 1-2/7 and 5/7
> in the last tets above, ditto the rest?
>
> Mm, yes, looking at assign_blame() in blame.c we'll "skip" some.
>
> So if we ever change that algorithm we'll need to adjust these, but it's
> probably good to notice that then, even if the test_cmp here does
> implicitly encode a bit of internal implementation details, i.e. when
> exactly we update the progress bar.

I do not think that is something we want to test, or distract the
developers by forcing them to update the skipping hardcoded in the
test.

Is it easy to grab the last progress message from the output?  What
we care, and have cared in the past, is that we expect that the 100%
is paired with the number of lines in the ranges, and the progress
actually ends at 100% (we had different bugs that did not bring the
meter to 100% at the end), so checking that end result would be what
matters most, e.g.

    tr '\015' '\012' output | tail -n 1

or something?

Re: [PATCH v3] blame: report correct number of lines in progress when using ranges

[Edmundo Carmona Antoranz]

On Wed, Apr 6, 2022 at 6:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>
>
> I do not think that is something we want to test, or distract the
> developers by forcing them to update the skipping hardcoded in the
> test.
>
> Is it easy to grab the last progress message from the output?  What
> we care, and have cared in the past, is that we expect that the 100%
> is paired with the number of lines in the ranges, and the progress
> actually ends at 100% (we had different bugs that did not bring the
> meter to 100% at the end), so checking that end result would be what
> matters most, e.g.
>
>     tr '\015' '\012' output | tail -n 1
>
> or something?
>

Let me send the next version of the patch that will hopefully get the best
of all the ideas we have been throwing around.

Re: [PATCH v3] blame: report correct number of lines in progress when using ranges

[Edmundo Carmona Antoranz]

On Wed, Apr 6, 2022 at 10:46 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> Hint: use --in-reply-to on re-rolls, this is in reply to v2 here:
> https://lore.kernel.org/git/20220404182129.33992-1-eantoranz@gmail.com/
>
> Also the --range-diff option to git-format-patch is really helpful,
> it'll make a diff between v2 and this v3 and attach it after "--".
>
> Anyway...
>

Thanks for the tips. I realized I left a garbage line in the v3 patch
that I would like to never see moving forward so I will send a clean
patch for v4 just like the previous ones, hope that's not a problem. I
will keep the tips in mind for a future time.