Re: [PATCH v5 4/5] convert: move multiple file filter error handling to separate function
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'.
Show 47 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 ?
Question: What should/will happen to the file ?
Will Git complain later ? Retry later ?Show 7 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. /*
* 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"Show 19 quoted lines
> - } 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(&subprocess_map, &entry->subprocess);
> - free(entry);
> - }
> - } else {
> + if (err)
> + handle_filter_error(&filter_status, entry, wanted_capability);
> + else
> strbuf_swap(dst, &nbuf);
> - }
> strbuf_release(&nbuf);
> return !err;
> }
> Torsten Bögershausen <tboegi@web.de>