Re: [PATCH v5 4/5] convert: move multiple file filter error handling to separate function
From: Torsten Bögershausen <hidden>
Date: 2017-06-18 07:21:30
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'.
quoted hunk ↗ jump to hunk
Signed-off-by: Lars Schneider <redacted> --- 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 ?- } 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(&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;
}