Lars Schneider [off-list ref] writes:
quoted
This part of the document is well-written to help filter-writers.
Thanks!
Don't thank me; thank yourself and reviewers of the previous rounds.
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.
"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.
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?
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 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.
quoted
quoted
+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.
On 30 Aug 2016, at 20:59, Junio C Hamano [off-list ref] wrote:
quoted
"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.
Now, I see your point. How about "error-and-stop" or "error-stop"
instead of "error-all"?
quoted
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?
Good argument. However, my intention here was to mimic the v1 filter
mechanism. If a filter fails then Git will run the same v1 filter on the
next file that needs to be filtered. If `filter.<driver>.required=false`
then a failure is even a legitimate result.
quoted
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 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.
I am not sure I understand your last sentence. Just to be clear:
Would you prefer it, if Git would just close the pipe to the filter process
on Git exit and leave the filter running? I think I wouldn't even need
to close the pipe explicitly. The pipe would be closed automatically when
git exits or dies, right? The filter could detect EOF and exit on its own.
That would be OK with me.
quoted
quoted
quoted
+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.
I think `t/t0021/rot13-filter.pl` (part of this patch) serves this purpose
already.
Thanks,
Lars
Hello,
W dniu 30.08.2016 o 20:59, Junio C Hamano pisze:
Lars Schneider [off-list ref] writes:
[...]
quoted
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?
quoted
"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.
quoted
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.
quoted
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 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.
quoted
quoted
quoted
+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.
Best,
--
Jakub Narębski
On 30 Aug 2016, at 22:46, Jakub Narębski [off-list ref] wrote:
quoted
quoted
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".
quoted
quoted
"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".
quoted
quoted
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.
quoted
quoted
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...
quoted
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.
quoted
quoted
quoted
quoted
+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