* [PATCH v2] blame: report correct number of lines in progress when using ranges @ 2022-04-04 18:21 Edmundo Carmona Antoranz 2022-04-04 18:25 ` Edmundo Carmona Antoranz ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Edmundo Carmona Antoranz @ 2022-04-04 18:21 UTC (permalink / raw) To: gitster, whydoubt, git; +Cc: 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 = π 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] blame: report correct number of lines in progress when using ranges 2022-04-04 18:21 [PATCH v2] blame: report correct number of lines in progress when using ranges Edmundo Carmona Antoranz @ 2022-04-04 18:25 ` Edmundo Carmona Antoranz 2022-04-04 19:32 ` Junio C Hamano 2022-04-05 7:34 ` Bagas Sanjaya 2022-04-05 9:36 ` Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 11+ messages in thread From: Edmundo Carmona Antoranz @ 2022-04-04 18:25 UTC (permalink / raw) To: Junio C Hamano, whydoubt, Git List 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 > +' ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] blame: report correct number of lines in progress when using ranges 2022-04-04 18:25 ` Edmundo Carmona Antoranz @ 2022-04-04 19:32 ` Junio C Hamano 0 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2022-04-04 19:32 UTC (permalink / raw) To: Edmundo Carmona Antoranz; +Cc: whydoubt, Git List 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 && ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] blame: report correct number of lines in progress when using ranges 2022-04-04 18:21 [PATCH v2] blame: report correct number of lines in progress when using ranges Edmundo Carmona Antoranz 2022-04-04 18:25 ` Edmundo Carmona Antoranz @ 2022-04-05 7:34 ` Bagas Sanjaya 2022-04-05 7:46 ` Ævar Arnfjörð Bjarmason 2022-04-05 9:42 ` Philip Oakley 2022-04-05 9:36 ` Ævar Arnfjörð Bjarmason 2 siblings, 2 replies; 11+ messages in thread From: Bagas Sanjaya @ 2022-04-05 7:34 UTC (permalink / raw) To: Edmundo Carmona Antoranz, gitster, whydoubt, git 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] blame: report correct number of lines in progress when using ranges 2022-04-05 7:34 ` Bagas Sanjaya @ 2022-04-05 7:46 ` Ævar Arnfjörð Bjarmason 2022-04-05 7:55 ` Edmundo Carmona Antoranz 2022-04-05 9:42 ` Philip Oakley 1 sibling, 1 reply; 11+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-04-05 7:46 UTC (permalink / raw) To: Bagas Sanjaya; +Cc: Edmundo Carmona Antoranz, gitster, whydoubt, git 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] blame: report correct number of lines in progress when using ranges 2022-04-05 7:46 ` Ævar Arnfjörð Bjarmason @ 2022-04-05 7:55 ` Edmundo Carmona Antoranz 0 siblings, 0 replies; 11+ messages in thread From: Edmundo Carmona Antoranz @ 2022-04-05 7:55 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Bagas Sanjaya, Junio C Hamano, whydoubt, Git List 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] blame: report correct number of lines in progress when using ranges 2022-04-05 7:34 ` Bagas Sanjaya 2022-04-05 7:46 ` Ævar Arnfjörð Bjarmason @ 2022-04-05 9:42 ` Philip Oakley 2022-04-06 15:14 ` Junio C Hamano 1 sibling, 1 reply; 11+ messages in thread From: Philip Oakley @ 2022-04-05 9:42 UTC (permalink / raw) To: Bagas Sanjaya, Edmundo Carmona Antoranz, gitster, whydoubt, git 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] blame: report correct number of lines in progress when using ranges 2022-04-05 9:42 ` Philip Oakley @ 2022-04-06 15:14 ` Junio C Hamano 2022-04-08 8:03 ` Philip Oakley 0 siblings, 1 reply; 11+ messages in thread From: Junio C Hamano @ 2022-04-06 15:14 UTC (permalink / raw) To: Philip Oakley; +Cc: Bagas Sanjaya, Edmundo Carmona Antoranz, whydoubt, git 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' .. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] blame: report correct number of lines in progress when using ranges 2022-04-06 15:14 ` Junio C Hamano @ 2022-04-08 8:03 ` Philip Oakley 2022-04-08 18:16 ` Junio C Hamano 0 siblings, 1 reply; 11+ messages in thread From: Philip Oakley @ 2022-04-08 8:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: Bagas Sanjaya, Edmundo Carmona Antoranz, whydoubt, git 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] blame: report correct number of lines in progress when using ranges 2022-04-08 8:03 ` Philip Oakley @ 2022-04-08 18:16 ` Junio C Hamano 0 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2022-04-08 18:16 UTC (permalink / raw) To: Philip Oakley; +Cc: Bagas Sanjaya, Edmundo Carmona Antoranz, whydoubt, git 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] blame: report correct number of lines in progress when using ranges 2022-04-04 18:21 [PATCH v2] blame: report correct number of lines in progress when using ranges Edmundo Carmona Antoranz 2022-04-04 18:25 ` Edmundo Carmona Antoranz 2022-04-05 7:34 ` Bagas Sanjaya @ 2022-04-05 9:36 ` Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 11+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-04-05 9:36 UTC (permalink / raw) To: Edmundo Carmona Antoranz; +Cc: gitster, whydoubt, git 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 && ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-04-08 18:16 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-04 18:21 [PATCH v2] blame: report correct number of lines in progress when using ranges Edmundo Carmona Antoranz 2022-04-04 18:25 ` Edmundo Carmona Antoranz 2022-04-04 19:32 ` Junio C Hamano 2022-04-05 7:34 ` Bagas Sanjaya 2022-04-05 7:46 ` Ævar Arnfjörð Bjarmason 2022-04-05 7:55 ` Edmundo Carmona Antoranz 2022-04-05 9:42 ` Philip Oakley 2022-04-06 15:14 ` Junio C Hamano 2022-04-08 8:03 ` Philip Oakley 2022-04-08 18:16 ` Junio C Hamano 2022-04-05 9:36 ` Ævar Arnfjörð Bjarmason
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).