Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 9, 2014, 22:40 UTC
Message-ID: <xmqq8ujkath2.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqlhnkb5bf.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 19 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Christian Couder <chriscool@tuxfamily.org> writes:
>>
>>> Yeah, it won't be as efficient as using only one strbuf and only byte
>>> oriented functions, and it looks much less manly too :-) But over time in
>>> Git we have developed a number of less efficient but quite clean
>>> abstractions like strbuf, argv_array, sha1_array and so on, that we
>>> are quite happy with.
>>
>> Actually, all these examples you gave are fairly efficient and clean
>> abstractions.  I find it insulting to pretend that the "one line per
>> strbuf" is in the same league.  It isn't.
>>
>> And it is not about manliness.
>
> By the way, I do not mean to say that all of strbuf (which is a
> rather full API) is uniformly efficient and cleanly abstracted.
> ...

Having said all that, please notice that I said "might need to be rethought before the code becomes ready for production."

After all, it would have perfectly been OK to experiment with this new topic in a script; e.g. this topic being text processing, it might have happened as a Perl script while we get the semantics and desirable features nailed down before rewriting the real thing in C.

And if that were what happened here, I wouldn't have been talking about data structures and unnecessary complexity having to have one strbuf per line only to join them into one string (or printf() out to a stream one-after-another) later.

In other words, I did not say "this code is too ugly to live and we should clean it up as soon as possible or we should revert it from 'master'---it was a mistake to merge it".

I consider the "trailer" code in 'master' today as experimental (in other words, something I might have written in a scripting language as a mock-up if I were doing it) that needs to be worked on further to still get the features and semantics right. And the patch series in this thread are hopefully taking us in the right direction to get them right [*1*].

You could have just said "Yeah, I realize that the code is way suboptimal, but lets get it feature complete first and then clean it up", or something, instead of getting defensive with unnecessary excuses.

[Footnote]

*1* ... even though this step exposes why the approach taken, splitting the input lines first into individual lines without even looking at them, is a wrong one---if you started from a single buffer that is not yet split, it would have been much easier to find where the trailer block is, and that is why you are re-joining the buffer you have already split into lines (i.e. what I called "impedance mismatch"), which shows that the initial splitting is done prematurely. Hopefully you would realize that by now ;-).