> On 19 Jun 2017, at 19:18, Torsten Bögershausen <tboegi@xxxxxx> wrote:
>
> 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
OK, I'll change it in the next round.
>>>>
>>>> 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 ";"
True. However, I prefer the {} style here. Would that be OK with you?
Plus, this commit is not supposed to change code. I just want to move the
code to a different function.
>>> 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.
I agree. I'll add a comment!
>>
>>>> - } 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!
OK :)
>>> /*
>>> * 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.
No, at this point they are passed the negotiation phase. A problem actually
happened.
> 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" ?
Well, at this point it is too late because we don't retry.
Plus, I would prefer to not change code here as this commit is supposed
to just move existing code. Changing "abort" would change the protocol
that was released with Git 2.11.
>>
>>> # 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;
How about this:
The filter signaled a problem. Don't try to filter files with this capability
for the lifetime of the current Git process.
?
Thanks,
Lars