Re: [PATCH v8 00/11] Git filter protocol

> On 04 Oct 2016, at 21:04, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> W dniu 03.10.2016 o 19:13, Lars Schneider pisze: 
>>> On 01 Oct 2016, at 22:48, Jakub Narębski <jnareb@gmail.com> wrote:
>>> W dniu 01.10.2016 o 20:59, Lars Schneider pisze: 
>>>> On 29 Sep 2016, at 23:27, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> Lars Schneider <larsxschneider@gmail.com> writes:
>>>>> 
>>>>> If the filter process refuses to die forever when Git told it to
>>>>> shutdown (by closing the pipe to it, for example), that filter
>>>>> process is simply buggy.  I think we want users to become aware of
>>>>> that, instead of Git leaving it behind, which essentially is to
>>>>> sweep the problem under the rug.
>>> 
>>> Well, it would be good to tell users _why_ Git is hanging, see below.
>> 
>> Agreed. Do you think it is OK to write the message to stderr?
> 
> On the other hand, this is why GIT_TRACE (and GIT_TRACE_PERFORMANCE)
> was invented for.  We do not signal troubles with single-shot filters,
> so I guess doing it for multi-file filters is not needed.

> 
>>>>> I agree with what Peff said elsewhere in the thread; if a filter
>>>>> process wants to take time to clean things up while letting Git
>>>>> proceed, it can do its own process management, but I think it is
>>>>> sensible for Git to wait the filter process it directly spawned.
>>>> 
>>>> To realize the approach above I prototyped the run-command patch below:
>>>> 
>>>> I added an "exit_timeout" variable to the "child_process" struct.
>>>> On exit, Git will close the pipe to the process and wait "exit_timeout" 
>>>> seconds until it kills the child process. If "exit_timeout" is negative
>>>> then Git will wait until the process is done.
>>> 
>>> That might be good approach.  Probably the default would be to wait.
>> 
>> I think I would prefer a 2sec timeout or something as default. This way
>> we can ensure Git would not wait indefinitely for a buggy filter by default.
> 
> Actually this waiting for multi-file filter is only about waiting for
> the shutdown process of the filter.  The filter could still hang during
> processing a file, and git would hang too, if I understand it correctly.

>> [...] this function is also used with the async struct... 
> 
> Hmmm... now I wonder if it is a good idea (similar treatment for
> single-file async-invoked filter, and multi-file pkt-line filters).
> 
> For single-file one-shot filter (correct me if I am wrong):
> 
> - git sends contents to filter, signals end with EOF
>   (after process is started)
> - in an async process:
>   - process is started
>   - git reads contents from filter, until EOF
>   - if process did not end, it is killed
> 
> 
> For multi-process pkt-line based filter (simplified):
> 
> - process is started
> - handshake
> - for each file
>   - file is send to filter process over pkt-line,
>     end signalled with flush packet
>   - git reads from filter from pkt-line, until flush
> - ...
> 
> 
> See how single-shot filter is sent EOF, though in different part
> of code.  We need to signal multi-file filter that no more files
> will be coming.  Simplest solution is to send EOF (we could send
> "command=shutdown" for example...) to filter, and wait for EOF
> from filter (or for "status=finished" and EOF).