From: Phillip Wood Date: Fri, 04 Apr 2025 15:58:44 GMT Subject: Re: [PATCH v5] blame: print unblamable and ignored commits in porcelain mode Message-ID: <2fbecc76-c3d9-4a8e-9326-5b83db1ceb26@gmail.com> In-Reply-To: <20250403160326.120124-1-karthik.188@gmail.com> Hi Karthik On 03/04/2025 17:03, Karthik Nayak wrote: > The 'git-blame(1)' command allows users to ignore specific revisions via > the '--ignore-rev ' and '--ignore-revs-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. This looks good to me Best Wishes Phillip > Helped-by: Patrick Steinhardt > Helped-by: Toon Claes > Helped-by: Phillip Wood > Signed-off-by: Karthik Nayak > --- > Changes in v5: > - Fix the test to be more portable by not using '\n' in 'sed'. > - Link to v4: https://lore.kernel.org/all/20250330204339.191382-1-karthik.188@gmail.com/ > > Changes in v4: > - Remove extra newline in 'puts'. Modify the test to compare the > entire output, the earlier test missed the extraneous newline. > - Link to v3: > https://lore.kernel.org/r/20250329-514-git-blame-1-s-porcelain-output-does-not-emit-unblamable-and-ignored-markers-v3-1-10f695ae519a@gmail.com > > Changes in v3: > - Use double-qoutes in the test to ensure correct variable dereference. > - Fix incorrect test name. > - Rename the function from 'emit_per_line_details()' to > 'emit_porcelain_per_line_details()' to be more descriptive. > - Ues 'puts()' instead of 'printf()'. > - Link to v2: > https://lore.kernel.org/r/20250326-514-git-blame-1-s-porcelain-output-does-not-emit-unblamable-and-ignored-markers-v2-1-79037e17a74b@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 v4: > > 1: 5250fb436e ! 1: 43bc55bffe blame: print unblamable and ignored commits in porcelain mode > @@ Commit message > > Helped-by: Patrick Steinhardt > Helped-by: Toon Claes > + Helped-by: Phillip Wood > Signed-off-by: Karthik Nayak > > ## Documentation/blame-options.adoc ## > @@ t/t8013-blame-ignore-revs.sh: test_expect_success mark_unblamable_lines ' > > +for opt in --porcelain --line-porcelain > +do > -+ test_expect_success "mark_unblamable_lines with $opt" ' > ++ test_expect_success "mark_unblamable_lines with $opt" " > + sha=$(git rev-parse Y) && > + > + git -c blame.markUnblamableLines=false blame $opt --ignore-rev Y file >raw && > -+ sed -e "s/^\ty3/unblamable\n&/" raw >expect && > -+ cp expect raw && > -+ sed -e "s/^\ty4/unblamable\n&/" raw >expect && > ++ cat > sedscript <<- 'EOF' && > ++ /^ y3/i\\ > ++ unblamable > ++ /^ y4/i\\ > ++ unblamable > ++ EOF > ++ sed -f sedscript raw >expect && > + > + git -c blame.markUnblamableLines=true blame $opt --ignore-rev Y file >actual && > + test_cmp expect actual > -+ ' > ++ " > +done > + > # Commit Z will touch the first two lines. Y touched all four. > @@ t/t8013-blame-ignore-revs.sh: test_expect_success mark_ignored_lines ' > > +for opt in --porcelain --line-porcelain > +do > -+ test_expect_success "mark_ignored_lines with $opt" ' > ++ test_expect_success "mark_ignored_lines with $opt" " > + sha=$(git rev-parse Y) && > + > + git -c blame.markIgnoredLines=false blame $opt --ignore-rev Z file >raw && > -+ sed -e "s/^\tline-one-Z/ignored\n&/" raw >expect && > -+ cp expect raw && > -+ sed -e "s/^\tline-two-Z/ignored\n&/" raw >expect && > ++ cat > sedscript <<- 'EOF' && > ++ /^ line-one-Z/i\\ > ++ ignored > ++ /^ line-two-Z/i\\ > ++ ignored > ++ EOF > ++ sed -f sedscript raw >expect && > + > + git -c blame.markIgnoredLines=true blame $opt --ignore-rev Z file >actual && > + test_cmp expect actual > -+ ' > ++ " > +done > + > # For ignored revs that added 'unblamable' lines and more recent commits changed > > --- > Documentation/blame-options.adoc | 3 ++- > Documentation/git-blame.adoc | 9 ++++---- > builtin/blame.c | 15 +++++++++++++ > t/t8013-blame-ignore-revs.sh | 38 ++++++++++++++++++++++++++++++++ > 4 files changed, 60 insertions(+), 5 deletions(-) > > 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 :: > 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..9436f70aec 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_porcelain_per_line_details(struct blame_entry *ent) > +{ > + if (mark_unblamable_lines && ent->unblamable) > + puts("unblamable"); > + if (mark_ignored_lines && ent->ignored) > + puts("ignored"); > +} > + > 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_porcelain_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_porcelain_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..cace00ae8d 100755 > --- a/t/t8013-blame-ignore-revs.sh > +++ b/t/t8013-blame-ignore-revs.sh > @@ -158,6 +158,25 @@ 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=false blame $opt --ignore-rev Y file >raw && > + cat > sedscript <<- 'EOF' && > + /^ y3/i\\ > + unblamable > + /^ y4/i\\ > + unblamable > + EOF > + sed -f sedscript raw >expect && > + > + git -c blame.markUnblamableLines=true blame $opt --ignore-rev Y file >actual && > + test_cmp expect actual > + " > +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 +210,25 @@ test_expect_success mark_ignored_lines ' > ! test_cmp expect actual > ' > > +for opt in --porcelain --line-porcelain > +do > + test_expect_success "mark_ignored_lines with $opt" " > + sha=$(git rev-parse Y) && > + > + git -c blame.markIgnoredLines=false blame $opt --ignore-rev Z file >raw && > + cat > sedscript <<- 'EOF' && > + /^ line-one-Z/i\\ > + ignored > + /^ line-two-Z/i\\ > + ignored > + EOF > + sed -f sedscript raw >expect && > + > + git -c blame.markIgnoredLines=true blame $opt --ignore-rev Z file >actual && > + test_cmp expect actual > + " > +done > + > # For ignored revs that added 'unblamable' lines and more recent commits changed > # the blamable lines, mark the unblamable lines with a > # '*'