| Message ID | 20250326-514-git-blame-1-s-porcelain-output-does-not-emit-unblamable-and-ignored-markers-v2-1-79037e17a74b@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] blame: print unblamable and ignored commits in porcelain mode | expand |
On Wed, Mar 26, 2025 at 5:07 PM Karthik Nayak <karthik.188@gmail.com> wrote: > The 'git-blame(1)' command allows users to ignore specific revisions via > the '--ignore-rev <rev>' and '--ignore-revs-file <file>' flags. These > flags are often combined with the 'blame.markIgnoredLines' and > 'blame.markUnblamableLines' config options. These config options prefix > ignored and unblamable lines with a '?' and '*', respectively. > > However, this option was never extended to the porcelain mode of > 'git-blame(1)'. Since the documentation does not indicate this > exclusion, it is a bug. > > Fix this by printing 'ignored' and 'unblamable' respectively for the > options when using the porcelain modes. > > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > --- > diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh > @@ -158,6 +158,16 @@ test_expect_success mark_unblamable_lines ' > +for opt in --porcelain --line-porcelain > +do > + test_expect_success 'mark_unblamable_lines with $opt' ' This test title is going to display literal "$opt" rather than the intended option. Fix this by replacing the single quotes with double quotes: test_expect_success "mark_unblamable_lines with $opt" ' > + sha=$(git rev-parse Y) && > + > + git -c blame.markUnblamableLines=true blame $opt --ignore-rev Y file >actual && > + test $(grep ^unblamable actual | wc -l) -eq 2 > + ' > +done > @@ -191,6 +201,16 @@ test_expect_success mark_ignored_lines ' > +for opt in --porcelain --line-porcelain > +do > + test_expect_success 'mark_ignored_lines line_porcelain' ' Similarly, this is going to display the same title for both cases, which isn't as helpful as it could be. Presumably, you instead wanted this (using double quotes): test_expect_success "mark_ignored_lines with $opt" ' > + sha=$(git rev-parse Y) && > + > + git -c blame.markIgnoredLines=true blame $opt --ignore-rev Z file >actual && > + test $(grep ^ignored actual | wc -l) -eq 2 > + '
Eric Sunshine <sunshine@sunshineco.com> writes: > On Wed, Mar 26, 2025 at 5:07 PM Karthik Nayak <karthik.188@gmail.com> wrote: >> The 'git-blame(1)' command allows users to ignore specific revisions via >> the '--ignore-rev <rev>' and '--ignore-revs-file <file>' flags. These >> flags are often combined with the 'blame.markIgnoredLines' and >> 'blame.markUnblamableLines' config options. These config options prefix >> ignored and unblamable lines with a '?' and '*', respectively. >> >> However, this option was never extended to the porcelain mode of >> 'git-blame(1)'. Since the documentation does not indicate this >> exclusion, it is a bug. >> >> Fix this by printing 'ignored' and 'unblamable' respectively for the >> options when using the porcelain modes. >> >> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> >> --- >> diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh >> @@ -158,6 +158,16 @@ test_expect_success mark_unblamable_lines ' >> +for opt in --porcelain --line-porcelain >> +do >> + test_expect_success 'mark_unblamable_lines with $opt' ' > > This test title is going to display literal "$opt" rather than the > intended option. Fix this by replacing the single quotes with double > quotes: > > test_expect_success "mark_unblamable_lines with $opt" ' > What a silly miss. Thanks for pointing out. >> + sha=$(git rev-parse Y) && >> + >> + git -c blame.markUnblamableLines=true blame $opt --ignore-rev Y file >actual && >> + test $(grep ^unblamable actual | wc -l) -eq 2 >> + ' >> +done >> @@ -191,6 +201,16 @@ test_expect_success mark_ignored_lines ' >> +for opt in --porcelain --line-porcelain >> +do >> + test_expect_success 'mark_ignored_lines line_porcelain' ' > > Similarly, this is going to display the same title for both cases, > which isn't as helpful as it could be. Presumably, you instead wanted > this (using double quotes): > > test_expect_success "mark_ignored_lines with $opt" ' > Yup, this needs to be fixed too. Thanks again. >> + sha=$(git rev-parse Y) && >> + >> + git -c blame.markIgnoredLines=true blame $opt --ignore-rev Z file >actual && >> + test $(grep ^ignored actual | wc -l) -eq 2 >> + '
On Wed, Mar 26, 2025 at 10:06:10PM +0100, Karthik Nayak wrote: > diff --git a/builtin/blame.c b/builtin/blame.c > index c470654c7e..528bfef249 100644 > --- a/builtin/blame.c > +++ b/builtin/blame.c > @@ -351,6 +351,19 @@ static void emit_porcelain_details(struct blame_origin *suspect, int repeat) > write_filename_info(suspect); > } > > +/* > + * Information which needs to be printed per-line goes here. Any > + * information which can be clubbed on a commit/file level, should > + * be printed via 'emit_one_suspect_detail()'. > + */ > +static void emit_per_line_details(struct blame_entry *ent) Tiny nit, feel free to ignore: should this something like `emit_porcelain_per_line_details()` to highlight that this is part of the porcelain format? > +{ > + if (mark_unblamable_lines && ent->unblamable) > + printf("unblamable\n"); > + if (mark_ignored_lines && ent->ignored) > + printf("ignored\n"); > +} > + Another tiny nit: you may use `puts()` instead of `printf()`. I don't mind it much though, both versions work equally well. > diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh > index 370b768149..306fc61057 100755 > --- a/t/t8013-blame-ignore-revs.sh > +++ b/t/t8013-blame-ignore-revs.sh > @@ -158,6 +158,16 @@ test_expect_success mark_unblamable_lines ' > test_cmp expect actual > ' > > +for opt in --porcelain --line-porcelain > +do > + test_expect_success 'mark_unblamable_lines with $opt' ' > + sha=$(git rev-parse Y) && > + > + git -c blame.markUnblamableLines=true blame $opt --ignore-rev Y file >actual && > + test $(grep ^unblamable actual | wc -l) -eq 2 > + ' > +done > + Okay, makes sense. We cannot batch the information on the first time we've seen the commit here because both the "unblamable" and "ignored" properties are properties of the line, not of the commit. So we'd expect to see the information per line in both modes. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Wed, Mar 26, 2025 at 10:06:10PM +0100, Karthik Nayak wrote: >> diff --git a/builtin/blame.c b/builtin/blame.c >> index c470654c7e..528bfef249 100644 >> --- a/builtin/blame.c >> +++ b/builtin/blame.c >> @@ -351,6 +351,19 @@ static void emit_porcelain_details(struct blame_origin *suspect, int repeat) >> write_filename_info(suspect); >> } >> >> +/* >> + * Information which needs to be printed per-line goes here. Any >> + * information which can be clubbed on a commit/file level, should >> + * be printed via 'emit_one_suspect_detail()'. >> + */ >> +static void emit_per_line_details(struct blame_entry *ent) > > Tiny nit, feel free to ignore: should this something like > `emit_porcelain_per_line_details()` to highlight that this is part of > the porcelain format? > That's a great point, will add that in. >> +{ >> + if (mark_unblamable_lines && ent->unblamable) >> + printf("unblamable\n"); >> + if (mark_ignored_lines && ent->ignored) >> + printf("ignored\n"); >> +} >> + > > Another tiny nit: you may use `puts()` instead of `printf()`. I don't > mind it much though, both versions work equally well. > Yeah, I think some compilers also do this translation. But I'll change it as anyways I'm going to push a new version. >> diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh >> index 370b768149..306fc61057 100755 >> --- a/t/t8013-blame-ignore-revs.sh >> +++ b/t/t8013-blame-ignore-revs.sh >> @@ -158,6 +158,16 @@ test_expect_success mark_unblamable_lines ' >> test_cmp expect actual >> ' >> >> +for opt in --porcelain --line-porcelain >> +do >> + test_expect_success 'mark_unblamable_lines with $opt' ' >> + sha=$(git rev-parse Y) && >> + >> + git -c blame.markUnblamableLines=true blame $opt --ignore-rev Y file >actual && >> + test $(grep ^unblamable actual | wc -l) -eq 2 >> + ' >> +done >> + > > Okay, makes sense. We cannot batch the information on the first time > we've seen the commit here because both the "unblamable" and "ignored" > properties are properties of the line, not of the commit. So we'd expect > to see the information per line in both modes. > > Patrick Thanks for the review. Karthik
Eric Sunshine <sunshine@sunshineco.com> writes: >> + test_expect_success 'mark_unblamable_lines with $opt' ' > > This test title is going to display literal "$opt" rather than the > intended option. Fix this by replacing the single quotes with double > quotes: > > test_expect_success "mark_unblamable_lines with $opt" ' Well spotted. Sorry for making all developers suffer the consequence of a mistake I made 20 years ago. In retrospect, I really should have anticipated that it would become a common pattern to have test_expect_success inside a loop, and made this part also be interpolated, to make it look similar to the next parameter.
diff --git a/Documentation/blame-options.adoc b/Documentation/blame-options.adoc index aa77406d4e..19ea187238 100644 --- a/Documentation/blame-options.adoc +++ b/Documentation/blame-options.adoc @@ -125,7 +125,8 @@ take effect. another commit will be marked with a `?` in the blame output. If the `blame.markUnblamableLines` config option is set, then those lines touched by an ignored commit that we could not attribute to another revision are - marked with a '*'. + marked with a '*'. In the porcelain modes, we print 'ignored' and + 'unblamable' on a newline respectively. --ignore-revs-file <file>:: Ignore revisions listed in `file`, which must be in the same format as an diff --git a/Documentation/git-blame.adoc b/Documentation/git-blame.adoc index f75ed44790..e438d28625 100644 --- a/Documentation/git-blame.adoc +++ b/Documentation/git-blame.adoc @@ -135,10 +135,11 @@ header elements later. The porcelain format generally suppresses commit information that has already been seen. For example, two lines that are blamed to the same commit will both be shown, but the details for that commit will be shown -only once. This is more efficient, but may require more state be kept by -the reader. The `--line-porcelain` option can be used to output full -commit information for each line, allowing simpler (but less efficient) -usage like: +only once. Information which is specific to individual lines will not be +grouped together, like revs to be marked 'ignored' or 'unblamable'. This +is more efficient, but may require more state be kept by the reader. The +`--line-porcelain` option can be used to output full commit information +for each line, allowing simpler (but less efficient) usage like: # count the number of lines attributed to each author git blame --line-porcelain file | diff --git a/builtin/blame.c b/builtin/blame.c index c470654c7e..528bfef249 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -351,6 +351,19 @@ static void emit_porcelain_details(struct blame_origin *suspect, int repeat) write_filename_info(suspect); } +/* + * Information which needs to be printed per-line goes here. Any + * information which can be clubbed on a commit/file level, should + * be printed via 'emit_one_suspect_detail()'. + */ +static void emit_per_line_details(struct blame_entry *ent) +{ + if (mark_unblamable_lines && ent->unblamable) + printf("unblamable\n"); + if (mark_ignored_lines && ent->ignored) + printf("ignored\n"); +} + static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent, int opt) { @@ -367,6 +380,7 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent, ent->lno + 1, ent->num_lines); emit_porcelain_details(suspect, repeat); + emit_per_line_details(ent); cp = blame_nth_line(sb, ent->lno); for (cnt = 0; cnt < ent->num_lines; cnt++) { @@ -377,6 +391,7 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent, ent->lno + 1 + cnt); if (repeat) emit_porcelain_details(suspect, 1); + emit_per_line_details(ent); } putchar('\t'); do { diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh index 370b768149..306fc61057 100755 --- a/t/t8013-blame-ignore-revs.sh +++ b/t/t8013-blame-ignore-revs.sh @@ -158,6 +158,16 @@ test_expect_success mark_unblamable_lines ' test_cmp expect actual ' +for opt in --porcelain --line-porcelain +do + test_expect_success 'mark_unblamable_lines with $opt' ' + sha=$(git rev-parse Y) && + + git -c blame.markUnblamableLines=true blame $opt --ignore-rev Y file >actual && + test $(grep ^unblamable actual | wc -l) -eq 2 + ' +done + # Commit Z will touch the first two lines. Y touched all four. # A--B--X--Y--Z # The blame output when ignoring Z should be: @@ -191,6 +201,16 @@ test_expect_success mark_ignored_lines ' ! test_cmp expect actual ' +for opt in --porcelain --line-porcelain +do + test_expect_success 'mark_ignored_lines line_porcelain' ' + sha=$(git rev-parse Y) && + + git -c blame.markIgnoredLines=true blame $opt --ignore-rev Z file >actual && + test $(grep ^ignored actual | wc -l) -eq 2 + ' +done + # For ignored revs that added 'unblamable' lines and more recent commits changed # the blamable lines, mark the unblamable lines with a # '*'
The 'git-blame(1)' command allows users to ignore specific revisions via the '--ignore-rev <rev>' and '--ignore-revs-file <file>' flags. These flags are often combined with the 'blame.markIgnoredLines' and 'blame.markUnblamableLines' config options. These config options prefix ignored and unblamable lines with a '?' and '*', respectively. However, this option was never extended to the porcelain mode of 'git-blame(1)'. Since the documentation does not indicate this exclusion, it is a bug. Fix this by printing 'ignored' and 'unblamable' respectively for the options when using the porcelain modes. Helped-by: Patrick Steinhardt <ps@pks.im> Helped-by: Toon Claes <toon@iotcl.com> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- Changes in v2: - Instead of printing the markers before the SHA in porcelain mode and breaking scripts and backward compatability, let's instead add a newline printing 'unblamable' or 'ignored'. This is printed per line in both the porcelain modes. - Link to v1: https://lore.kernel.org/r/20250321-514-git-blame-1-s-porcelain-output-does-not-emit-unblamable-and-ignored-markers-v1-1-44b562d9beb8@gmail.com --- Range-diff versus v1: 1: 9650db628f < -: ---------- blame: fix unblamable and ignored lines in porcelain mode -: ---------- > 1: 6bbfc0cbd2 blame: print unblamable and ignored commits in porcelain mode --- Documentation/blame-options.adoc | 3 ++- Documentation/git-blame.adoc | 9 +++++---- builtin/blame.c | 15 +++++++++++++++ t/t8013-blame-ignore-revs.sh | 20 ++++++++++++++++++++ 4 files changed, 42 insertions(+), 5 deletions(-) --- base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e change-id: 20250321-514-git-blame-1-s-porcelain-output-does-not-emit-unblamable-and-ignored-markers-4af46f02847e Thanks - Karthik