From: "Torsten Bögershausen" <tboegi@web.de>
To: Lars Schneider <larsxschneider@gmail.com>, git@vger.kernel.org
Cc: gitster@pobox.com, peff@peff.net, e@80x24.org,
ttaylorr@github.com, peartben@gmail.com
Subject: Re: [PATCH v5 4/5] convert: move multiple file filter error handling to separate function
Date: Sun, 18 Jun 2017 09:20:49 +0200 [thread overview]
Message-ID: <be787e74-5f37-8429-9976-abdf8725737c@web.de> (raw)
In-Reply-To: <20170601082203.50397-5-larsxschneider@gmail.com>
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 <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 ?
> - } 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;
> }
>
next prev parent reply other threads:[~2017-06-18 7:21 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-01 8:21 [PATCH v5 0/5] convert: add "status=delayed" to filter process protocol Lars Schneider
2017-06-01 8:21 ` [PATCH v5 1/5] t0021: keep filter log files on comparison Lars Schneider
2017-06-01 8:22 ` [PATCH v5 2/5] t0021: make debug log file name configurable Lars Schneider
2017-06-01 8:22 ` [PATCH v5 3/5] t0021: write "OUT" only on success Lars Schneider
2017-06-01 8:22 ` [PATCH v5 4/5] convert: move multiple file filter error handling to separate function Lars Schneider
2017-06-18 7:20 ` Torsten Bögershausen [this message]
2017-06-18 11:47 ` Lars Schneider
2017-06-19 17:18 ` Torsten Bögershausen
2017-06-19 17:47 ` Lars Schneider
2017-06-01 8:22 ` [PATCH v5 5/5] convert: add "status=delayed" to filter process protocol Lars Schneider
2017-06-02 2:21 ` Junio C Hamano
2017-06-05 11:36 ` Lars Schneider
2017-06-24 14:19 ` Jeff King
2017-06-24 17:22 ` Lars Schneider
2017-06-24 18:51 ` Junio C Hamano
2017-06-24 20:36 ` Jeff King
2017-06-24 20:32 ` Jeff King
2017-06-01 9:44 ` [PATCH v5 0/5] " Junio C Hamano
2017-06-02 2:06 ` Junio C Hamano
2017-06-24 14:23 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=be787e74-5f37-8429-9976-abdf8725737c@web.de \
--to=tboegi@web.de \
--cc=e@80x24.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=larsxschneider@gmail.com \
--cc=peartben@gmail.com \
--cc=peff@peff.net \
--cc=ttaylorr@github.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.