larsxschneider@xxxxxxxxx writes:
> +#define FILTER_CAPABILITIES_CLEAN (1u<<0)
> +#define FILTER_CAPABILITIES_SMUDGE (1u<<1)
> +#define FILTER_SUPPORTS_CLEAN(type) ((type) & FILTER_CAPABILITIES_CLEAN)
> +#define FILTER_SUPPORTS_SMUDGE(type) ((type) & FILTER_CAPABILITIES_SMUDGE)
I would expect a lot shorter names as these are file-local;
CAP_CLEAN and CAP_SMUDGE, perhaps, _WITHOUT_ "supports BLAH" macros?
if (FILTER_SUPPORTS_CLEAN(type))
is not all that more readable than
if (CAP_CLEAN & type)
> +struct cmd2process {
> + struct hashmap_entry ent; /* must be the first member! */
> + int supported_capabilities;
> + const char *cmd;
> + struct child_process process;
> +};
> +
> +static int cmd_process_map_initialized = 0;
> +static struct hashmap cmd_process_map;
Don't initialize statics to 0 or NULL.
> +static int cmd2process_cmp(const struct cmd2process *e1,
> + const struct cmd2process *e2,
> + const void *unused)
> +{
> + return strcmp(e1->cmd, e2->cmd);
> +}
> +
> +static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)
> +{
> + struct cmd2process key;
> + hashmap_entry_init(&key, strhash(cmd));
> + key.cmd = cmd;
> + return hashmap_get(hashmap, &key, NULL);
> +}
> +
> +static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *entry)
> +{
> + if (!entry)
> + return;
> + sigchain_push(SIGPIPE, SIG_IGN);
> + close(entry->process.in);
> + close(entry->process.out);
> + sigchain_pop(SIGPIPE);
> + finish_command(&entry->process);
I wonder if we want to diagnose failures from close(), which is a
lot more interesting than usual because these are connected to
pipes.
> +static int apply_multi_file_filter(const char *path, const char *src, size_t len,
> + int fd, struct strbuf *dst, const char *cmd,
> + const int wanted_capability)
> +{
> + int ret = 1;
> + ...
> + if (!(wanted_capability & entry->supported_capabilities))
> + return 1; // it is OK if the wanted capability is not supported
No // comment please.
> + filter_result = packet_read_line(process->out, NULL);
> + ret = !strcmp(filter_result, "result=success");
> +
> +done:
> + if (ret) {
> + strbuf_swap(dst, &nbuf);
> + } else {
> + if (!filter_result || strcmp(filter_result, "result=reject")) {
> + // Something went wrong with the protocol filter. Force shutdown!
> + error("external filter '%s' failed", cmd);
> + kill_multi_file_filter(&cmd_process_map, entry);
> + }
> + }
> + strbuf_release(&nbuf);
> + return ret;
> +}
I think this was already pointed out in the previous review by Peff,
but a variable "ret" that says "0 is bad" somehow makes it hard to
follow the code. Perhaps rename it to "int error", flip the meaning,
and if the caller wants this function to return non-zero on success
flip the polarity in the return statement itself, i.e. "return !errors",
may make it easier to follow?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html