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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Jun 18, 2017, 11:47 UTC
Message-ID: <28D2B758-7E5D-4EEA-A33D-57871960D5E1@gmail.com>
In-Reply-To: <be787e74-5f37-8429-9976-abdf8725737c@web.de>

Show 10 quoted lines

> 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..."?

Show 48 quoted lines

>> 
>> 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...

>                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.

Show 11 quoted lines

>> -		} 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.

Show 5 quoted lines

> 		/*
> 		 * 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.

>                 # 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.

Thanks for the review, Lars