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@xxxxxxxxx/ 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx/. 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@xxxxxxxxx> 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.
