Re: [PATCH] blame: fix unblamable and ignored lines in porcelain mode

From

Patrick Steinhardt <ps@pks.im>

Date

Mar 24, 2025, 10:16 UTC

Message-ID

<Z-Ew_12NWX_5qATN@pks.im>

In-Reply-To

<xmqqzfhbrb78.fsf@gitster.g>

On Sun, Mar 23, 2025 at 08:58:03AM -0700, Junio C Hamano wrote:

> Karthik Nayak <karthik.188@gmail.com> writes:
> 
> > 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.
> 
> I agree it is a bug when people added ignore or unblamable support
> that they did not _consider_ what to do with their new pieces of
> information to help porcelain writers.  It is not a bug in the code
> per-se, but it is a bug in the brain of these people ;-)
> 
> But prefixing random garbage to the commit object name line in the
> porcelain mode output does not sound like the right solution to the
> bug, either.
> 
> When enhancing an existing output format, make sure that your
> changes will have minimum empact to existing parsers that do not
> know about your extension.  It is reasonably expected that existing
> Porcelain scripts reading from --porcelain mode output works by
> 
>  - Recognizing a line that match "^[0-9a-f]{40} \d+ \d+ \d+$" and
>    take it as the beginning of a new record;
> 
>  - Collect all info lines before the payload line.  Lines that
>    describe per-commit information are not repeated if it is already
>    shown, so remember them when you see the commit for the first
>    time, and recall them when you recognize the commit you already
>    saw.
> 
>  - A payload line is indented with HT and terminates the record.
> 
> If you start to add unrecognizable garbage to the line with very
> well known fixed format that is used as record delimiter, you would
> break the existing parsers, which is not a very nice thing to do.
> Are there other and better ways you can think of to add new pieces
> of information like this in a way with less severe damage?

Thanks!

Patrick

diff --git a/builtin/blame.c b/builtin/blame.c
index c470654c7ec..cd8322e2619 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -255,7 +255,8 @@ static void write_filename_info(struct blame_origin *suspect)
  * the first time each commit appears in the output (unless the
  * user has specifically asked for us to repeat).
  */
-static int emit_one_suspect_detail(struct blame_origin *suspect, int repeat)
+static int emit_one_suspect_detail(struct blame_entry *ent,
+				   struct blame_origin *suspect, int repeat)
 {
 	struct commit_info ci = COMMIT_INFO_INIT;
 
@@ -275,6 +276,10 @@ static int emit_one_suspect_detail(struct blame_origin *suspect, int repeat)
 	printf("summary %s\n", ci.summary.buf);
 	if (suspect->commit->object.flags & UNINTERESTING)
 		printf("boundary\n");
+	if (mark_unblamable_lines && ent->unblamable)
+		printf("unblamable\n");
+	if (mark_ignored_lines && ent->ignored)
+		printf("ignored\n");
 
 	commit_info_destroy(&ci);
 
@@ -295,7 +300,7 @@ static void found_guilty_entry(struct blame_entry *ent, void *data)
 		printf("%s %d %d %d\n",
 		       oid_to_hex(&suspect->commit->object.oid),
 		       ent->s_lno + 1, ent->lno + 1, ent->num_lines);
-		emit_one_suspect_detail(suspect, 0);
+		emit_one_suspect_detail(ent, suspect, 0);
 		write_filename_info(suspect);
 		maybe_flush_or_die(stdout, "stdout");
 	}
@@ -344,9 +349,10 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 #define OUTPUT_COLOR_LINE           (1U<<10)
 #define OUTPUT_SHOW_AGE_WITH_COLOR  (1U<<11)
 
-static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
+static void emit_porcelain_details(struct blame_entry *ent,
+				   struct blame_origin *suspect, int repeat)
 {
-	if (emit_one_suspect_detail(suspect, repeat) ||
+	if (emit_one_suspect_detail(ent, suspect, repeat) ||
 	    (suspect->commit->object.flags & MORE_THAN_ONE_PATH))
 		write_filename_info(suspect);
 }
@@ -366,7 +372,7 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
 	       ent->s_lno + 1,
 	       ent->lno + 1,
 	       ent->num_lines);
-	emit_porcelain_details(suspect, repeat);
+	emit_porcelain_details(ent, suspect, repeat);
 
 	cp = blame_nth_line(sb, ent->lno);
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
@@ -376,7 +382,7 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
 			       ent->s_lno + 1 + cnt,
 			       ent->lno + 1 + cnt);
 			if (repeat)
-				emit_porcelain_details(suspect, 1);
+				emit_porcelain_details(ent, suspect, 1);
 		}
 		putchar('\t');
 		do {