diff mbox series

[v3] blame: print unblamable and ignored commits in porcelain mode

Message ID 20250329-514-git-blame-1-s-porcelain-output-does-not-emit-unblamable-and-ignored-markers-v3-1-10f695ae519a@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v3] blame: print unblamable and ignored commits in porcelain mode | expand

Commit Message

Karthik Nayak March 29, 2025, 6:21 p.m. UTC
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 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 v2:

1:  3a4012718b ! 1:  a80353ab00 blame: print unblamable and ignored commits in porcelain mode
    @@ builtin/blame.c: static void emit_porcelain_details(struct blame_origin *suspect
     + * 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)
    ++static void emit_porcelain_per_line_details(struct blame_entry *ent)
     +{
     +	if (mark_unblamable_lines && ent->unblamable)
    -+		printf("unblamable\n");
    ++		puts("unblamable\n");
     +	if (mark_ignored_lines && ent->ignored)
    -+		printf("ignored\n");
    ++		puts("ignored\n");
     +}
     +
      static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
    @@ builtin/blame.c: static void emit_porcelain(struct blame_scoreboard *sb, struct
      	       ent->lno + 1,
      	       ent->num_lines);
      	emit_porcelain_details(suspect, repeat);
    -+	emit_per_line_details(ent);
    ++	emit_porcelain_per_line_details(ent);
      
      	cp = blame_nth_line(sb, ent->lno);
      	for (cnt = 0; cnt < ent->num_lines; cnt++) {
    @@ builtin/blame.c: static void emit_porcelain(struct blame_scoreboard *sb, struct
      			       ent->lno + 1 + cnt);
      			if (repeat)
      				emit_porcelain_details(suspect, 1);
    -+			emit_per_line_details(ent);
    ++			emit_porcelain_per_line_details(ent);
      		}
      		putchar('\t');
      		do {
    @@ 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=true blame $opt --ignore-rev Y file >actual &&
    @@ 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 line_porcelain' '
    ++	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 &&
---
 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

Comments

Junio C Hamano March 30, 2025, 4:56 a.m. UTC | #1
Karthik Nayak <karthik.188@gmail.com> writes:

> +static void emit_porcelain_per_line_details(struct blame_entry *ent)
> +{
> +	if (mark_unblamable_lines && ent->unblamable)
> +		puts("unblamable\n");
> +	if (mark_ignored_lines && ent->ignored)
> +		puts("ignored\n");
> +}

Doesn't puts(3), unlike fputs(3), add its own trailing newline to
stdout?
Phillip Wood March 30, 2025, 9:28 a.m. UTC | #2
On 30/03/2025 05:56, Junio C Hamano wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
> 
>> +static void emit_porcelain_per_line_details(struct blame_entry *ent)
>> +{
>> +	if (mark_unblamable_lines && ent->unblamable)
>> +		puts("unblamable\n");
>> +	if (mark_ignored_lines && ent->ignored)
>> +		puts("ignored\n");
>> +}
> 
> Doesn't puts(3), unlike fputs(3), add its own trailing newline to
> stdout?

Indeed. Perhaps the regression test would be more effective if it used 
test_cmp rather than

     test $(grep ^ignored actual | wc -l) -eq 2

which misses this bug and gives no output if it fails.

Best Wishes

Phillip
diff mbox series

Patch

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..8a73b7be4a 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\n");
+	if (mark_ignored_lines && ent->ignored)
+		puts("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_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..c5a7533232 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 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
+	'
+done
+
 # For ignored revs that added 'unblamable' lines and more recent commits changed
 # the blamable lines, mark the unblamable lines with a
 # '*'