On 2017-06-18 13:47, Lars Schneider wrote:
>
>> On 18 Jun 2017, at 09:20, Torsten Bögershausen <tboegi@xxxxxx> wrote:
>>
>>
>> On 2017-06-01 10:22, Lars Schneider wrote:
>>> This is useful for the subsequent patch 'convert: add "status=delayed" to
>>> filter process protocol'.
>>
>> May be
>> Collecting all filter error handling is useful for the subsequent patch
>> 'convert: add "status=delayed" to filter process protocol'.
>
> I think I get your point. However, I feel "Collecting" is not the right word.
>
> How about:
> "Refactoring filter error handling is useful..."?
OK
>
>>>
>>> Signed-off-by: Lars Schneider <larsxschneider@xxxxxxxxx>
>>> ---
>>> convert.c | 47 ++++++++++++++++++++++++++---------------------
>>> 1 file changed, 26 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/convert.c b/convert.c
>>> index f1e168bc30..a5e09bb0e8 100644
>>> --- a/convert.c
>>> +++ b/convert.c
>>> @@ -565,6 +565,29 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
>>> return err;
>>> }
>>>
>>> +static void handle_filter_error(const struct strbuf *filter_status,
>>> + struct cmd2process *entry,
>>> + const unsigned int wanted_capability) {
>>> + if (!strcmp(filter_status->buf, "error")) {
>>> + /* The filter signaled a problem with the file. */
>>> + } else if (!strcmp(filter_status->buf, "abort") && wanted_capability) {
>>> + /*
>>> + * The filter signaled a permanent problem. Don't try to filter
>>> + * files with the same command for the lifetime of the current
>>> + * Git process.
>>> + */
>>> + entry->supported_capabilities &= ~wanted_capability;
>>> + } else {
>>> + /*
>>> + * Something went wrong with the protocol filter.
>>> + * Force shutdown and restart if another blob requires filtering.
>>> + */
>>> + error("external filter '%s' failed", entry->subprocess.cmd);
>>> + subprocess_stop(&subprocess_map, &entry->subprocess);
>>> + free(entry);
>>> + }
>>> +}
>>> +
>>> static int apply_multi_file_filter(const char *path, const char *src, size_t len,
>>> int fd, struct strbuf *dst, const char *cmd,
>>> const unsigned int wanted_capability)
>>> @@ -656,28 +679,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
>>> done:
>>> sigchain_pop(SIGPIPE);
>>>
>>> - if (err) {
>>> - if (!strcmp(filter_status.buf, "error")) {
>>> - /* The filter signaled a problem with the file. */
>> Note1: Do we need a line with a single ";" here ?
>
> The single ";" wouldn't hurt but I don't think it is helpful either.
> I can't find anything in the coding guidelines about this...
OK about that. I was thinking to remove the {}, and the we -need- the ";"
>
>
>> Question: What should/will happen to the file ?
>> Will Git complain later ? Retry later ?
>
> The file is not filtered and Git does not try, again.
> That might be OK for certain filters:
> If "filter.foo.required = false" then this would be OK.
> If "filter.foo.required = true" then this would cause an error.
Does it make sense to add it as a comment ?
I know, everything is documented otherwise, but when looking at the source
code only, the context may be useful, it may be not.
>
>
>>> - } else if (!strcmp(filter_status.buf, "abort")) {
>>> - /*
>>> - * The filter signaled a permanent problem. Don't try to filter
>>> - * files with the same command for the lifetime of the current
>>> - * Git process.
>>> - */
>>> - entry->supported_capabilities &= ~wanted_capability;
>> Hm, could this be clarified somewhat ?
>> Mapping "abort" to "permanent problem" makes sense as
>> such, but the only action that is done is to reset
>> a capablity.
>
> I am not sure what you mean with "reset capability". We disable the capability here.
> That means Git will not use the capability for the active session.
Sorry, my wrong - reset is a bad word here.
"Git will not use the capability for the active session" is perfect!
>
>
>> /*
>> * The filter signaled a missing capabilty. Don't try to filter
>> * files with the same command for the lifetime of the current
>> * Git process.
>> */
>
> I like the original version better because the capability is not "missing".
> There is "a permanent problem" and the filter wants to signal Git to not use
> this capability for the current session anymore.
Git and the filter are in a negotiation phase to find out which side is able
to do what.So I don't think there is a "problem" (in the sense that I understand
it) at all.
And back to the "abort":
I still think that the word feels to harsh...
"abort" in my understanding smells too much "a program is terminated".
If it is not too late, does it may sense to use something like "nack" ?
>
>
>> # And the there is a question why the answer is "abort" and not
>> # "unsupported"
>
> Because the filter supports the capability. There is just some kind of failure that
> that causes the capability to not work as expected. Again, depending on the filter
> "required" flag this is an error or not.
>
May be I misunderstood the whole sequence, and abort is the right thing.
If yes, how about this ?
} else if (!strcmp(filter_status->buf, "abort") && wanted_capability) {
/*
* Don't try to filter files with this capability for lifetime
* of the current Git process.
*/
entry->supported_capabilities &= ~wanted_capability;