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