Re: [PATCH v6 12/13] convert: add filter.<driver>.process option
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 30, 2016, 22:23 UTC
- Message-ID
- <xmqqmvjt3nht.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <4020D8E2-77D9-46AE-95DC-F644F7C6FDAF@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
Show 16 quoted lines
> 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.
Show 5 quoted lines
>> 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?