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

> 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?
>
> I think the porcelain mode is already built so that it can be extended
> with arbitrary new information, no? In `emit_one_suspect_detail()` we
> end up printing one line per info we want to display. I would have
> expected that we can extend that function to also print information
> around unblamable or ignored commits, like we already do for boundary
> commits. E.g. something like the patch further down.
>

> 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 {