| Message ID | 20220404182129.33992-1-eantoranz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] blame: report correct number of lines in progress when using ranges | expand |
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 &&
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?
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.
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.
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 = π > 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 &&
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
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' ..
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
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.
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 = π 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 &&
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(-)