Re: [PATCH v2] blame: print unblamable and ignored commits in porcelain mode

From: Karthik Nayak <karthik.188@gmail.com>
Date: Mar 29, 2025, 10:26 UTC
Message-ID: <CAOLa=ZRgAQ+FKznxOaOe0ou_6vRPOA7SorhxwvGeBZ07SasgqA@mail.gmail.com>
In-Reply-To: <Z-ZI-M2jBf5pqMoA@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 20 quoted lines

> On Wed, Mar 26, 2025 at 10:06:10PM +0100, Karthik Nayak wrote:
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> index c470654c7e..528bfef249 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_per_line_details(struct blame_entry *ent)
>
> Tiny nit, feel free to ignore: should this something like
> `emit_porcelain_per_line_details()` to highlight that this is part of
> the porcelain format?
>

That's a great point, will add that in.

Show 11 quoted lines

>> +{
>> +	if (mark_unblamable_lines && ent->unblamable)
>> +		printf("unblamable\n");
>> +	if (mark_ignored_lines && ent->ignored)
>> +		printf("ignored\n");
>> +}
>> +
>
> Another tiny nit: you may use `puts()` instead of `printf()`. I don't
> mind it much though, both versions work equally well.
>

Yeah, I think some compilers also do this translation. But I'll change it as anyways I'm going to push a new version.

Show 25 quoted lines

>> diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh
>> index 370b768149..306fc61057 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
>> +
>
> Okay, makes sense. We cannot batch the information on the first time
> we've seen the commit here because both the "unblamable" and "ignored"
> properties are properties of the line, not of the commit. So we'd expect
> to see the information per line in both modes.
>
> Patrick

Thanks for the review. Karthik