Patrick Steinhardt <ps@xxxxxx> writes:
> On Sun, Mar 23, 2025 at 08:58:03AM -0700, Junio C Hamano wrote:
>> Karthik Nayak <karthik.188@xxxxxxxxx> 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.
>
This indeed looks like a much better way of doing this. Let me
> 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 {
Thanks Patrick, I will send in the new version with this and modified
tests.
Attachment:
signature.asc
Description: PGP signature
