> On 30 Aug 2016, at 22:46, Jakub Narębski <jnareb@xxxxxxxxx> wrote: > >>> 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. >> >> OK. > > Is it explicit kill, or implicit kill: close pipe and end process? I close the pipe and call "finish_command". >>> "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"? >> >> No. >> >> I was primarily reacting to "-all" part, so anything that ends with >> "-all" is equally ugly from my point of view and not an improvement. >> >> As I said, "error-all" as long as other reviewers are happy with is >> OK by me, too. > > I'm rather partial to "abort" instead of "error-all", or "quit"/"exit" > (but I prefer "abort" or "bail-out"), as it better reflects what this > response is about - ending filter process. After some thinking I agree with "abort" instead of "error-all". >>> 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. >> >> Why does Git restart it and continue with the rest of the blobs >> then? Is there a reason to believe it may produce correct results >> for other blobs if you run it again? > > I guess the idea is that single filter can be run on different types > of blobs, and it could fail on some types (some files) and not others. > Like LFS-type filter failing on files with size larger than 2 GiB, > or iconv-like filter to convert UTF-16 to UTF-8 failing on invalid > byte sequences. This mimics the v1 behavior and I will explain that in the documentation. >>> 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. >> >> Would that be an issue? A filter is buggy if told to shutdown, >> ignores the request and hangs around forever. I do not think we >> even need to kill it. It is not Git's problem. > > I think the idea was for Git to request shutdown, and filter to tell > when it finished (or just exiting, and Git getting SIGCHLD, I guess). > It is hard to tell how much to wait, for example for filter process > to send "sync" command, or finish unmounting, or something... I like Junios approach because then we don't need to wait at all... >> I personally would be reluctant to become a filter process writer if >> the system it will be talking to can kill it without giving it a >> chance to do a graceful exit, but perhaps that is just me. I don't >> know if closing the pipe going there where you are having Git to >> kill the process on the other end is any more involved than what you >> have without extra patches. > > Isn't it the same problem with v1 filters being killed on Git process > exit? Unless v2 filter wants to do something _after_ writing output > to Git, it should be safe... unless Git process is killed, but it > is not different from filter being stray-killed. Yes, it should be safe. However, I think it is probably nicer if the filter process can shutdown gracefully instead of being killed. >>>>> +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. >> >> I meant it as primarily an example people can learn from when they >> want to write their own. > > Errr... if it were any v1 filter, it would be useless (as bad or > worse performance than ordinary v1 clean or smudge filter). It > might make sense if v1 filter and v2 wrapper were written in the > same [dynamic] language, and wrapper could just load / require > filter as a function / procedure, [compile it], and use it. > For example smudge/clean filter in Perl (e.g. rot13), and v2 wrapper > in Perl too. I'll provide a simple Perl rot13 v2 filter in contrib. Thanks, Lars