threads / patch / 57658

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

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

## tl;dr

11 messages between Apr 4, 2022 and Apr 8, 2022. Diffs are folded; open one to read it.

replies: 10people: 5as markdown or json

Edmundo Carmona Antoranz· Apr 4, 2022, 18:21 UTC · lore

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

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(-)
Show changes to 2 files +33 −1

builtin/blame.c, t/t8002-blame.sh

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
Edmundo Carmona Antoranz· Apr 4, 2022, 18:25 UTC · re: Edmundo Carmona Antoranz · lore

On Mon, Apr 4, 2022 at 8:21 PM Edmundo Carmona Antoranz <eantoranz@gmail.com> wrote:

Show 17 quoted lines
>
> 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
> +'
Junio C Hamano· Apr 4, 2022, 19:32 UTC · re: Edmundo Carmona Antoranz · lore
Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:
Show 22 quoted lines
> 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 &&
Bagas Sanjaya· Apr 5, 2022, 07:34 UTC · re: Edmundo Carmona Antoranz · lore
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?
Show 28 quoted lines
> +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
Ævar Arnfjörð Bjarmason· Apr 5, 2022, 07:46 UTC · re: Bagas Sanjaya · lore
On Tue, Apr 05 2022, Bagas Sanjaya wrote:
Show 37 quoted lines
> 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.

Edmundo Carmona Antoranz· Apr 5, 2022, 07:55 UTC · re: Ævar Arnfjörð Bjarmason · lore
On Tue, Apr 5, 2022 at 9:46 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 5 quoted lines
>
>
>
> 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.

Philip Oakley· Apr 5, 2022, 09:42 UTC · re: Bagas Sanjaya · lore
On 05/04/2022 08:34, Bagas Sanjaya wrote:
Show 6 quoted lines
> 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

Junio C Hamano· Apr 6, 2022, 15:14 UTC · re: Philip Oakley · lore
Philip Oakley <philipoakley@iee.email> writes:
Show 11 quoted lines
> 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' ..
Philip Oakley· Apr 8, 2022, 08:03 UTC · re: Junio C Hamano · lore
On 06/04/2022 16:14, Junio C Hamano wrote:
Show 18 quoted lines
> 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

Junio C Hamano· Apr 8, 2022, 18:16 UTC · re: Philip Oakley · lore
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.
Ævar Arnfjörð Bjarmason· Apr 5, 2022, 09:36 UTC · re: Edmundo Carmona Antoranz · lore
On Mon, Apr 04 2022, Edmundo Carmona Antoranz wrote:
Show 54 quoted lines
> 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.
Show 19 quoted lines
> 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).

Show 11 quoted lines
> +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 &&

← back to recent threads