Re: [PATCH v6 12/13] convert: add filter.<driver>.process option

[Junio C Hamano <gitster@pobox.com>]

Lars Schneider <larsxschneider@gmail.com> writes:

> On 30 Aug 2016, at 20:59, Junio C Hamano <gitster@pobox.com> wrote:
>> 
>>> "abort" could be ambiguous because it could be read as "abort only
>>> this file". "abort-all" would work, though. Would you prefer to see
>>> "error" replaced by "abort" and "error-all" by "abort-all"?
>> 
>> No.
>> 
>> I was primarily reacting to "-all" part, so anything that ends with
>> "-all" is equally ugly from my point of view and not an improvement.
>> 
>> As I said, "error-all" as long as other reviewers are happy with is
>> OK by me, too.
>
> Now, I see your point. How about "error-and-stop" or "error-stop"
> instead of "error-all"?

Not really.  I was primarily reacting to having "error" and
"error-all", that share the same prefix.  Changing "-all" suffix to
"-stop" does not make all that difference to me.  But in any case,
as long as other reviewers are happy with it, it is OK by me, too.

> Good argument. However, my intention here was to mimic the v1 filter
> mechanism.

I am not making any argument and in no way against the chosen
behaviour.  That "intention here" that was revealed after two
iterations is something we would want to see as the reasoning
behind the choice of the final behaviour recorded somewhere,
and now I drew it out of you, I achieved what I set out to do
initially ;-)

> I am not sure I understand your last sentence. Just to be clear:
> Would you prefer it, if Git would just close the pipe to the filter process
> on Git exit and leave the filter running?

As a potential filter writer, I would probably feel it the easiest
to handle if there is no signal involved.  My job would be to read
from the command pipe, react to it, and when the command pipe gives
me EOF, I am expected to exit myself.  That would be simple enough.

>> I meant it as primarily an example people can learn from when they
>> want to write their own.
>
> I think `t/t0021/rot13-filter.pl` (part of this patch) serves this purpose 
> already.

I would expect them to peek at contrib/, but do you seriously expect
people to comb through t/ directory?