Re: [PATCH v6 12/13] convert: add filter.<driver>.process option

From: Lars Schneider <larsxschneider@gmail.com>
Date: Aug 30, 2016, 16:27 UTC
Message-ID: <7A378528-58DE-4C17-9F00-9708947716DD@gmail.com>
In-Reply-To: <xmqqzinv6wtg.fsf@gitster.mtv.corp.google.com>

Show 24 quoted lines

> On 30 Aug 2016, at 00:21, Junio C Hamano <gitster@pobox.com> wrote:
> 
> larsxschneider@gmail.com writes:
> 
>> +In case the filter cannot or does not want to process the content,
>> +it is expected to respond with an "error" status. Depending on the
>> +`filter.<driver>.required` flag Git will interpret that as error
>> +but it will not stop or restart the filter process.
>> +------------------------
>> +packet:          git< status=error\n
>> +packet:          git< 0000
>> +------------------------
>> +
>> +In case the filter cannot or does not want to process the content
>> +as well as any future content for the lifetime of the Git process,
>> +it is expected to respond with an "error-all" status. Depending on
>> +the `filter.<driver>.required` flag Git will interpret that as error
>> +but it will not stop or restart the filter process.
>> +------------------------
>> +packet:          git< status=error-all\n
>> +packet:          git< 0000
>> +------------------------
> 
> This part of the document is well-written to help filter-writers.

Thanks!

Show 6 quoted lines

> One thing that was unclear from the above to me, when read as a
> potential filter-writer, is when I am supposed to exit(2).  After I
> tell Git with error-all (I would have called it "abort", but that's
> OK) that I desire no further communication, am I free to go?  Or do
> I wait until Git somehow disconnects (perhaps by closing the packet
> stream I have been reading)?

The filter can exit right after the "error-all". If the filter does not exit then Git will kill the filter. I'll add this to the docs.

"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"?

Show 8 quoted lines

>> +If the filter dies during the communication or does not adhere to
>> +the protocol then Git will stop the filter process and restart it
>> +with the next file that needs to be processed.
> 
> Hmph, is there a reason not to retry a half-converted-and-failed
> blob with the fresh process?  Note that this is not "you must do it
> that way", and it is not even "I think doing so may be a better
> idea".  I merely want to know the reason behind this decision.

A filter that dies during communication or does not adhere to the protocol is a faulty filter. Feeding the faulty filter after restart with the same blob would likely cause the same error.

There are legitimate reasons for retries. E.g. if the filter communicates with the network. In these cases I expect the filter to handle the retry logic. Git just writes to and reads from pipes. I don't expect frequent problems in that area. Plus the existing filter mechanism has no retry either.

Later on we could easily add a "retry" capability if we deem it necessary, though.

Show 14 quoted lines

>> +After the filter has processed a blob it is expected to wait for
>> +the next "key=value" list containing a command. When the Git process
>> +terminates, it will send a kill signal to the filter in that stage.
> 
> The "kill" may not be very nice.  As Git side _knows_ that the
> filter is waiting for the next command, having an explicit
> "shutdown" command would give the filter a chance to implement a
> clean exit--it may have some housekeeping tasks it wants to perform
> once it is done.  The "explicit shutdown" could just be "the pipe
> gets closed", so from the implementation point of view there may not
> be anything you need to further add to this patch (after all, when
> we exit, the pipes to them would be closed), but the shutdown
> protocol and the expectation on the behaviour of filter processes
> would need to be documented.

I implemented a shutdown command in v4 [1][2] but dropped it in v5 after a discussion with Peff [3].

[1] http://public-inbox.org/git/20160803164225.46355-8-larsxschneider@gmail.com/ [2] http://public-inbox.org/git/20160803164225.46355-13-larsxschneider@gmail.com/ [3] http://public-inbox.org/git/20160803225313.pk3tfe5ovz4y3i7l@sigill.intra.peff.net/

My main reasons to drop it:

A) There is no central place in Git that could execute code *after*
   all filter operations are complete and *before* Git exits. Therefore,
   I had to add a "clean_on_exit_handler()" to "run-command" [1]. This
   change made this series even larger and therefore harder to review.

B) If we communicate "shutdown" to the filter then we need to give the
   filter some time to perform the exit before the filter is killed on
   Git exit. I wasn't able to come up with a good answer how long Git 
   should wait for the exit.

Do you think I should resurrect the "shutdown" patch?

Show 7 quoted lines

>> +If a `filter.<driver>.clean` or `filter.<driver>.smudge` command
>> +is configured then these commands always take precedence over
>> +a configured `filter.<driver>.process` command.
> 
> It may make more sense to give precedence to the .process (which is
> a late-comer) if defined, ignoring .clean and .smudge, than the
> other way around.

I agree.

Show 8 quoted lines

>> +Please note that you cannot use an existing `filter.<driver>.clean`
>> +or `filter.<driver>.smudge` command with `filter.<driver>.process`
>> +because the former two use a different inter process communication
>> +protocol than the latter one.
> 
> Would it be a useful sample program we can ship in contrib/ if you
> created a "filter adapter" that reads these two configuration
> variables and act as a filter.<driver>.process?

You mean a v2 filter that would use v1 filters under the hood? If we would drop v1, then this would be useful. Otherwise I don't see any need for such a thing.

Show 5 quoted lines

> During an imaginary session of "git add .", I think I found where
> you start THE filter process upon the first path that needs to be
> filtered with one for the configured <driver>, and I think the same
> place is where you reuse THE filter process, but I am not sure where
> you are cleaning up by killing the filter once all paths are added.

I don't clean them up. The filter gets killed when Git exits.

> Wouldn't you need some hooks at strategic places after such bulk
> operation to tell the multi-file-filter machinery to walk all the
> entries in cmd_process_map and tell the remaining filter processes
> that they have no more tasks, or something?

That would be nice. I tried to find these strategic places but it turned out to be harder than I thought because Git can exit in a lot of places.

>  Are you relying on
> these processes to exit upon a read failure after we exit and the
> pipe going to the filter is severed?

Not really. The filter process is killed by the run-command machinery with the atexit child_cleanup_handler.

Thanks, Lars