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

[Lars Schneider]

> On 19 Jun 2017, at 19:18, Torsten Bögershausen  wrote:
> 
> On 2017-06-18 13:47, Lars Schneider wrote:
>> 
>>> On 18 Jun 2017, at 09:20, Torsten Bögershausen  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 
 ---
 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(_map, >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 

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

[Torsten Bögershausen]

On 2017-06-18 13:47, Lars Schneider wrote:
> 
>> On 18 Jun 2017, at 09:20, Torsten Bögershausen  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 
>>> ---
>>> 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(_map, >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

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

[Lars Schneider]

> On 18 Jun 2017, at 09:20, Torsten Bögershausen  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..."?


>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> 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(_map, >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.


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


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

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

[Torsten Bögershausen]

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

> 
> Signed-off-by: Lars Schneider 
> ---
>  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(_map, >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 ?
Question: What should/will happen to the file ?
Will Git complain later ? Retry later ?
> - } 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.

/*
 * The filter signaled a missing capabilty. Don't try to filter
 * files with the same command for the lifetime of the current
 * Git process.
 */

 # And the there is a question why the answer is "abort" and not
 # "unsupported"
> - } else {
> - /*
> -  * Something went wrong with the protocol filter.
> -  * Force shutdown and restart if another blob requires 
> filtering.
> -  */
> - error("external filter '%s' failed", cmd);
> - subprocess_stop(_map, >subprocess);
> - free(entry);
> - }
> - } else {
> + if (err)
> + handle_filter_error(_status, entry, wanted_capability);
> + else
>   strbuf_swap(dst, );
> - }
>   strbuf_release();
>   return !err;
>  }
> 

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

[Lars Schneider]

This is useful for the subsequent patch 'convert: add "status=delayed" to
filter process protocol'.

Signed-off-by: Lars Schneider 
---
 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(_map, >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. */
-   } 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;
-   } else {
-   /*
-* Something went wrong with the protocol filter.
-* Force shutdown and restart if another blob requires 
filtering.
-*/
-   error("external filter '%s' failed", cmd);
-   subprocess_stop(_map, >subprocess);
-   free(entry);
-   }
-   } else {
+   if (err)
+   handle_filter_error(_status, entry, wanted_capability);
+   else
strbuf_swap(dst, );
-   }
strbuf_release();
return !err;
 }
-- 
2.13.0