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

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

On 25 Jul 2016, at 01:22, Jakub Narębski [off-list ref] wrote:

quoted

W dniu 2016-07-25 o 00:36, Ramsay Jones pisze:

quoted

On 24/07/16 18:16, Lars Schneider wrote:

quoted

quoted

quoted

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 [off-list ref] wrote:

quoted

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.

quoted

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

[clarified]

-- 
Jakub Narębski