> On 04 Oct 2016, at 21:04, Jakub Narębski <jnareb@xxxxxxxxx> wrote: > > W dniu 03.10.2016 o 19:13, Lars Schneider pisze: >>> On 01 Oct 2016, at 22:48, Jakub Narębski <jnareb@xxxxxxxxx> wrote: >>> W dniu 01.10.2016 o 20:59, Lars Schneider pisze: >>>> On 29 Sep 2016, at 23:27, Junio C Hamano <gitster@xxxxxxxxx> wrote: >>>>> Lars Schneider <larsxschneider@xxxxxxxxx> 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 am on the fence with this one. @Junio/Peff: Where would you prefer to see a "Waiting for filter 'XYZ'... " message? On stderr or via GIT_TRACE? > >>>>> 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. Correct. >> [...] 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). That's what we do. EOF does signal the multi-filter to shutdown. > For full patch, you would need also to add to Documentation/config.txt Why config.txt? Thanks, Lars