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

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

[Edmundo Carmona Antoranz]

When using ranges, use their 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.
$

Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
---
 builtin/blame.c  |  6 +++++-
 t/t8002-blame.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 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/t8002-blame.sh b/t/t8002-blame.sh
index ee4fdd8f18..151a6fddfd 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -129,6 +129,34 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' '
 	test_must_fail git blame --exclude-promisor-objects one
 '
 
+test_expect_success 'blame progress on a full file' '
+	cat >progress.txt <<-\EOF &&
+	a simple test file
+
+	no relevant content is expected here
+
+	If the file is too short, we cannot test ranges
+
+	EOF
+	git add progress.txt &&
+	git commit -m "add a file for testing progress" &&
+	GIT_PROGRESS_DELAY=0 \
+	git blame --progress progress.txt > /dev/null 2> full_progress.txt &&
+	grep "Blaming lines: 100% (6/6), done." full_progress.txt
+'
+
+test_expect_success 'blame progress on a single range' '
+	GIT_PROGRESS_DELAY=0 \
+	git blame --progress -L 2,5 progress.txt > /dev/null 2> range_progress.txt &&
+	grep "Blaming lines: 100% (4/4), done." range_progress.txt
+'
+
+test_expect_success 'blame progress on multiple ranges' '
+	GIT_PROGRESS_DELAY=0 \
+	git blame --progress -L 1,2 -L 4,6 progress.txt > /dev/null 2> range_progress.txt &&
+	grep "Blaming lines: 100% (5/5), done." range_progress.txt
+'
+
 test_expect_success 'blame with uncommitted edits in partial clone does not crash' '
 	git init server &&
 	echo foo >server/file.txt &&
-- 
2.35.1

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

[Edmundo Carmona Antoranz]

On Mon, Apr 4, 2022 at 8:21 PM Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
>
> When using ranges, use their sizes as the limit for progress
> instead of the size of the full file.
>
>  '
>
> +test_expect_success 'blame progress on a full file' '
> +       cat >progress.txt <<-\EOF &&
> +       a simple test file
> +
> +       no relevant content is expected here
> +
> +       If the file is too short, we cannot test ranges
> +
> +       EOF
> +       git add progress.txt &&
> +       git commit -m "add a file for testing progress" &&

I wonder if the preceding section should be kept in a
separate 'setup test'?

> +       GIT_PROGRESS_DELAY=0 \
> +       git blame --progress progress.txt > /dev/null 2> full_progress.txt &&
> +       grep "Blaming lines: 100% (6/6), done." full_progress.txt
> +'

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

[Junio C Hamano]

Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:

> On Mon, Apr 4, 2022 at 8:21 PM Edmundo Carmona Antoranz
> <eantoranz@gmail.com> wrote:
>>
>> When using ranges, use their sizes as the limit for progress
>> instead of the size of the full file.
>>
>>  '
>>
>> +test_expect_success 'blame progress on a full file' '
>> +       cat >progress.txt <<-\EOF &&
>> +       a simple test file
>> +
>> +       no relevant content is expected here
>> +
>> +       If the file is too short, we cannot test ranges
>> +
>> +       EOF
>> +       git add progress.txt &&
>> +       git commit -m "add a file for testing progress" &&
>
> I wonder if the preceding section should be kept in a
> separate 'setup test'?

I actually wonder why we need a *new* test file to run this test,
instead of reusing what we already use in the existing test.

>
>> +       GIT_PROGRESS_DELAY=0 \
>> +       git blame --progress progress.txt > /dev/null 2> full_progress.txt &&

Style:

	git blame --progress progress.txt >/dev/null 2>full_progress.txt &&

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

[Bagas Sanjaya]

On 05/04/22 01.21, Edmundo Carmona Antoranz wrote:
> When using ranges, use their sizes as the limit for progress
> instead of the size of the full file.
> 

The progress limit is defined by number of affected lines, right?

> +test_expect_success 'blame progress on a full file' '
> +	cat >progress.txt <<-\EOF &&
> +	a simple test file
> +
> +	no relevant content is expected here
> +
> +	If the file is too short, we cannot test ranges
> +
> +	EOF
> +	git add progress.txt &&
> +	git commit -m "add a file for testing progress" &&
> +	GIT_PROGRESS_DELAY=0 \
> +	git blame --progress progress.txt > /dev/null 2> full_progress.txt &&
> +	grep "Blaming lines: 100% (6/6), done." full_progress.txt
> +'
> +
> +test_expect_success 'blame progress on a single range' '
> +	GIT_PROGRESS_DELAY=0 \
> +	git blame --progress -L 2,5 progress.txt > /dev/null 2> range_progress.txt &&
> +	grep "Blaming lines: 100% (4/4), done." range_progress.txt
> +'
> +
> +test_expect_success 'blame progress on multiple ranges' '
> +	GIT_PROGRESS_DELAY=0 \
> +	git blame --progress -L 1,2 -L 4,6 progress.txt > /dev/null 2> range_progress.txt &&
> +	grep "Blaming lines: 100% (5/5), done." range_progress.txt
> +'
> +

Why not using test_i18ngrep?

-- 
An old man doll... just what I always wanted! - Clara

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

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 05 2022, Bagas Sanjaya wrote:

> On 05/04/22 01.21, Edmundo Carmona Antoranz wrote:
>> When using ranges, use their sizes as the limit for progress
>> instead of the size of the full file.
>> 
>
> The progress limit is defined by number of affected lines, right?
>
>> +test_expect_success 'blame progress on a full file' '
>> +	cat >progress.txt <<-\EOF &&
>> +	a simple test file
>> +
>> +	no relevant content is expected here
>> +
>> +	If the file is too short, we cannot test ranges
>> +
>> +	EOF
>> +	git add progress.txt &&
>> +	git commit -m "add a file for testing progress" &&
>> +	GIT_PROGRESS_DELAY=0 \
>> +	git blame --progress progress.txt > /dev/null 2> full_progress.txt &&
>> +	grep "Blaming lines: 100% (6/6), done." full_progress.txt
>> +'
>> +
>> +test_expect_success 'blame progress on a single range' '
>> +	GIT_PROGRESS_DELAY=0 \
>> +	git blame --progress -L 2,5 progress.txt > /dev/null 2> range_progress.txt &&
>> +	grep "Blaming lines: 100% (4/4), done." range_progress.txt
>> +'
>> +
>> +test_expect_success 'blame progress on multiple ranges' '
>> +	GIT_PROGRESS_DELAY=0 \
>> +	git blame --progress -L 1,2 -L 4,6 progress.txt > /dev/null 2> range_progress.txt &&
>> +	grep "Blaming lines: 100% (5/5), done." range_progress.txt
>> +'
>> +
>
> Why not using test_i18ngrep?

Nothing should be using test_i18ngrep nowadays, just grep is better. We
no longer test with the gettext "poison" mode which necessitated it.

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

[Edmundo Carmona Antoranz]

On Tue, Apr 5, 2022 at 9:46 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
>
> Nothing should be using test_i18ngrep nowadays, just grep is better. We
> no longer test with the gettext "poison" mode which necessitated it.

Taking a closer look at the already defined tests/files. Thank you all
for your feedback.

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

[Philip Oakley]

On 05/04/2022 08:34, Bagas Sanjaya wrote:
> On 05/04/22 01.21, Edmundo Carmona Antoranz wrote:
>> When using ranges, use their sizes as the limit for progress
>> instead of the size of the full file.
>>
>
> The progress limit is defined by number of affected lines, right?

I'd also wondered about 'their', thinking it was 'the files', rather
than 'the ranges' [within those files].

perhaps: s/their/range/  

"When using ranges, use the range sizes as the limit for progress' ..

or maybe 'total range size'.
--
Philip

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

[Junio C Hamano]

Philip Oakley <philipoakley@iee.email> writes:

> On 05/04/2022 08:34, Bagas Sanjaya wrote:
>> On 05/04/22 01.21, Edmundo Carmona Antoranz wrote:
>>> When using ranges, use their sizes as the limit for progress
>>> instead of the size of the full file.
>>
>> The progress limit is defined by number of affected lines, right?
>
> I'd also wondered about 'their', thinking it was 'the files', rather
> than 'the ranges' [within those files].
>
> perhaps: s/their/range/

I actually think that it is obvious that "their" refers to the
ranges and not the file.  Between "the ranges" and "the file", only
the former is plural that "their" could possibly refer to.  Also,
"instead ... the full file" makes the sentence nonsensical if it
referred to the "file"---"we must use the number of lines in the
file, instead of the number of lines in the file" simply would not
make much sense.

But I do not object to being more explicit.

> "When using ranges, use the range sizes as the limit for progress' ..

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

[Philip Oakley]

On 06/04/2022 16:14, Junio C Hamano wrote:
> Philip Oakley <philipoakley@iee.email> writes:
>
>> On 05/04/2022 08:34, Bagas Sanjaya wrote:
>>> On 05/04/22 01.21, Edmundo Carmona Antoranz wrote:
>>>> When using ranges, use their sizes as the limit for progress
>>>> instead of the size of the full file.
>>> The progress limit is defined by number of affected lines, right?
>> I'd also wondered about 'their', thinking it was 'the files', rather
>> than 'the ranges' [within those files].
>>
>> perhaps: s/their/range/
> I actually think that it is obvious that "their" refers to the
> ranges and not the file.  Between "the ranges" and "the file", only
> the former is plural that "their" could possibly refer to.  Also,
> "instead ... the full file" makes the sentence nonsensical if it
> referred to the "file"---"we must use the number of lines in the
> file, instead of the number of lines in the file" simply would not
> make much sense.
I'm on the 'context and guidelines' side of English comprehension, so it
was all about files being blamed.

>
> But I do not object to being more explicit.

The core point though was that it can be misunderstood, thus avoiding
the indirection, as you say, makes it more explicit for the reader.
>
>> "When using ranges, use the range sizes as the limit for progress' ..
--
Philip

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

[Junio C Hamano]

Philip Oakley <philipoakley@iee.email> writes:

>> But I do not object to being more explicit.
>
> The core point though was that it can be misunderstood, thus avoiding
> the indirection, as you say, makes it more explicit for the reader.

Yup.  FWIW, I was saying that what the author wrote was not _wrong_
per-se.  I agree that being explicit here (instead of hiding behind
a pronoun) is an improvement.

Thanks.

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

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 04 2022, Edmundo Carmona Antoranz wrote:

> When using ranges, use their 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.
> $
>
> Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
> ---
>  builtin/blame.c  |  6 +++++-
>  t/t8002-blame.sh | 28 ++++++++++++++++++++++++++++
>  2 files changed, 33 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);

Looking good.

> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
> index ee4fdd8f18..151a6fddfd 100755
> --- a/t/t8002-blame.sh
> +++ b/t/t8002-blame.sh
> @@ -129,6 +129,34 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' '
>  	test_must_fail git blame --exclude-promisor-objects one
>  '
>  
> +test_expect_success 'blame progress on a full file' '
> +	cat >progress.txt <<-\EOF &&
> +	a simple test file
> +
> +	no relevant content is expected here
> +
> +	If the file is too short, we cannot test ranges
> +
> +	EOF
> +	git add progress.txt &&
> +	git commit -m "add a file for testing progress" &&

Let's just skip this then and use existing test setup. A quick glance at
the state after this test shows that e.g. the "hello.c" we already
created would be a good candidate.

> +	GIT_PROGRESS_DELAY=0 \
> +	git blame --progress progress.txt > /dev/null 2> full_progress.txt &&
> +	grep "Blaming lines: 100% (6/6), done." full_progress.txt

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).

> +test_expect_success 'blame progress on a single range' '
> +	GIT_PROGRESS_DELAY=0 \
> +	git blame --progress -L 2,5 progress.txt > /dev/null 2> range_progress.txt &&
> +	grep "Blaming lines: 100% (4/4), done." range_progress.txt
> +'
> +
> +test_expect_success 'blame progress on multiple ranges' '
> +	GIT_PROGRESS_DELAY=0 \
> +	git blame --progress -L 1,2 -L 4,6 progress.txt > /dev/null 2> range_progress.txt &&
> +	grep "Blaming lines: 100% (5/5), done." range_progress.txt
> +'

Style nit, no space after ">", so e.g. 2>err.

Also shorter names are easier to read, so just:

    [...] 2>err

Or "actual" per the suggestion above.

And no need to redirect stdout to /dev/null, it's helpful to see it by
default in the verbose test output, we let that take care of suppressing
all of it ornot.


>  test_expect_success 'blame with uncommitted edits in partial clone does not crash' '
>  	git init server &&
>  	echo foo >server/file.txt &&