Re: [PATCH v1 3/3] convert: add filter.<driver>.useProtocol option

From: Jakub Narębski <jnareb@gmail.com>
Date: Jul 26, 2016, 10:58 UTC
Message-ID: <57974240.5030003@gmail.com>
In-Reply-To: <7D0AD199-F077-4EB2-83A5-58EB603973CA@gmail.com>

W dniu 2016-07-25 o 22:32, Lars Schneider pisze:

> On 25 Jul 2016, at 01:22, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 2016-07-25 o 00:36, Ramsay Jones pisze:
>>> On 24/07/16 18:16, Lars Schneider wrote:

Show 25 quoted lines

>>>> It was a conscious decision to have the `filter` talk first.
>>>> My reasoning was:
>>>> 
>>>> (1) I want a reliable way to distinguish the existing filter 
>>>> protocol ("single-shot invocation") from the new one ("long 
>>>> running"). I don't think there would be a situation where the 
>>>> existing protocol would talk first. Therefore the users would 
>>>> not accidentally mix them with a possibly half working, 
>>>> undetermined, outcome.
>>> 
>>> If an 'single-shot' filter were incorrectly configured, instead 
>>> of a new one, then the interaction could last a little while - 
>>> since it would result in deadlock! ;-)
>>> 
>>> [If Git talks first instead, configuring a 'single-shot' filter 
>>> _may_ still result in a deadlock - depending on pipe size, etc.]
>> 
>> Would it be possible to do an equivalent of sending empty file to 
>> the filter? If it is misconfigured old-style script, it would exit 
>> after possibly empty output; if not, we would start new-style 
>> interaction.
> 
> I think we would need to close the pipe to communicate "end" to the 
> filter, no? I would prefer to define the protocol explicitly as this 
> is clearly easier.

Well, we could always close stdin of a script, check if it quits, then reopen. Or close stdin, and send commands via file descriptor 4. Or send SIGPIPE. But I don't know if it is a good idea.

> On 25 Jul 2016, at 00:36, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:

Show 10 quoted lines

>> If an 'single-shot' filter were incorrectly configured, instead of
>> a new one, then the interaction could last a little while - since
>> it would result in deadlock! ;-)
>> 
>> [If Git talks first instead, configuring a 'single-shot' filter
>> _may_ still result in a deadlock - depending on pipe size, etc.]
> 
> Do you think this is an issue that needs to be addressed in the first
> version? If yes, I would probably look into "select" to specify a
> timeout for the filter.

This might be a better idea. Additionally, it would make it possible to detect buggy v2 filter scripts.

>                         However, wouldn't the current "single-shot"
> clean/smudge filter block in the same way if they don't write
> anything?

Hmmm... so deadlocking (waiting for user to press ^C) might be an acceptable solution. It would be good to tell him or her why there was a deadlock (catch SIGINT), that Git was waiting for specific command in a specific filter driver, for a specific file.

On the other hand v2 protocol has an additional problem: users switching to v2, while using old one-shot filters (that worked correctly). So in my opinion you need to ensure two things:

(1) name things in such way that it is easy to see that you need to write filter script specifically for the v2 protocol,

(2) if possible, do not hang but warn the user if he or she wants to use v1 filter (per-file) with v2 protocol (per-command), or at least help diagnose the issue.

>> This should be tested, if we agree that detecting misconfigured 
>> filters is a good thing.

[clarified]

-- 
Jakub Narębski