Re: [PATCH v4 11/12] convert: add filter.<driver>.process option

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 3, 2016, 20:29 UTC
Message-ID: <xmqq8twd8uld.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160803164225.46355-12-larsxschneider@gmail.com>

larsxschneider@gmail.com 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)

Show 9 quoted lines

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

Show 24 quoted lines

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

Show 8 quoted lines

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

Show 16 quoted lines

> +	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?