Re: [PATCH] blame: make diff algorithm configurable

From: Antonin Delpeuch <antonin@delpeuch.eu>
Date: Oct 22, 2025, 09:37 UTC
Message-ID: <d59a2f97-1a69-44f6-924e-7419e36329a0@delpeuch.eu>
In-Reply-To: <xmqqldl51rtm.fsf@gitster.g>

On 20/10/2025 18:05, Junio C Hamano wrote:

Show 18 quoted lines

>>      If the general idea of this patch is judged worthwhile, I would be happy
>>      to add tests to demonstrate the impact of the diff algorithm on blame
>>      output.
> Do not ever say this here.
>
> I've seen from time to time people ask "I am thinking of doing this;
> will a patch be accepted?  If so, I'll work on it." before showing
> any work, and my response always has been:
>
>   (1) We don't know how useful and interesting your contribution would
>       be for our audience, until we see it; and
>
>   (2) If you truly believe in your work (find it useful, find writing
>       it fun, etc.), that would be incentive enough for you to work
>       on it, whether or not the result will land in my tree.  You
>       should instead aim for something so brilliant that we would
>       come to you begging for your permission to include it in our
>       project.

I am surprised by your reaction here, both by its substance and form.

My understanding is that gathering feedback on a proposal before carrying out the implementation work in its entirety is widely accepted as a good practice for contributions to open source projects. For instance, the following guide encourages to do so (in GitHub terms, by proposing an improvement as an issue first, and by opening a draft pull request if necessary):

https://opensource.guide/how-to-contribute/

While this is phrased in the context of GitHub, I think the general principle behind it is healthy. In projects I maintain, I feel bad for contributors who submit contributions that clearly required a significant effort, but that I can't accept for certain reasons. I wish they had got in touch ahead of investing all this work, because I care about their time.

In fact, I already did so for an earlier contribution to this very project, and on that occasion you did not seem to take offense at the fact that my proposal was done without an accompanying patch:

https://lore.kernel.org/git/8bb5e41e-4db9-4527-8492-3aca6a0f40bf@delpeuch.eu/

Has your position changed since? Or did I benefit from more of your kindness back then as a new contributor?

Your argument about my work being worthy on its own even if it's not integrated to your tree is an interesting one, but let me expand on my motivation for this patch. This change is not something I personally need, nor something that is particularly fun to write. I am working on this with the hope that it will eventually make it possible to switch the default to the histogram algorithm, for the benefit of many git users. I see no point for this patch if it is not integrated in your tree. It is a gift to you and to the git community: if the gift is to be declined, I'd rather not spend time crafting it.

Concerning the form, I feel obliged to let you know that from my cultural standpoint, your reply reads rather aggressive. Specifically, the sentence "Do not ever say this here." reads menacing to me, and the use of all caps later on in your reply reads aggressive to me. I'm doing my best to assume that it is not be the attitude you wanted to convey and hope that you can receive this feedback gratefully.

Show 17 quoted lines

>> +	*opt &= ~XDF_NEED_MINIMAL;
>> +	*opt &= ~XDF_DIFF_ALGORITHM_MASK;
>> +	*opt |= value;
>> +
>> +	return 0;
>> +}
>> +
>>   static int is_a_rev(const char *name)
>>   {
>>   	struct object_id oid;
>> @@ -908,13 +941,16 @@ int cmd_blame(int argc,
>>   		OPT_BIT('f', "show-name", &output_option, N_("show original filename (Default: auto)"), OUTPUT_SHOW_NAME),
>>   		OPT_BIT('n', "show-number", &output_option, N_("show original linenumber (Default: off)"), OUTPUT_SHOW_NUMBER),
>>   		OPT_BIT('p', "porcelain", &output_option, N_("show in a format designed for machine consumption"), OUTPUT_PORCELAIN),
>> -		OPT_BIT(0, "line-porcelain", &output_option, N_("show porcelain format with per-line commit information"), OUTPUT_PORCELAIN|OUTPUT_LINE_PORCELAIN),
>> +		OPT_BIT(0, "line-porcelain", &output_option, N_("show porcelain format with per-line commit information"), OUTPUT_PORCELAIN | OUTPUT_LINE_PORCELAIN),
> WHY?

In an attempt to conform to the coding style of this project, I ran `make style`, which generated this change. Given that it is on a line I hadn't touched, I pondered on whether to include it in my patch or not. In the past, I had submitted a patch which fixed a formatting issue in the documentation in passing, together with content changes further down, and you had been supportive of this:

https://lore.kernel.org/git/xmqq1qaeqtw7.fsf@gitster.g/

So I decided to include it this time as well. I am happy to remove it if you prefer not to have it.

I thank you for the rest of your comments and will take them into account for a new version of this patch, pending the discussion above.

Best wishes,

Antonin