Re: [PATCH v5 4/5] convert: move multiple file filter error handling to separate function

[Lars Schneider <larsxschneider@gmail.com>]

> On 19 Jun 2017, at 19:18, Torsten Bögershausen <tboegi@web.de> wrote:
> 
> On 2017-06-18 13:47, Lars Schneider wrote:
>> 
>>> On 18 Jun 2017, at 09:20, Torsten Bögershausen <tboegi@web.de> 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@gmail.com>
>>>> ---
>>>> 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