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

[Lars Schneider <larsxschneider@gmail.com>]

> On 03 Aug 2016, at 22:29, Junio C Hamano <gitster@pobox.com> wrote:
> 
> 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)

OK. I will change that.


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

OK, statics are initialized implicitly to 0.
I will fix it.


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

In this particular case we kill the filter. That means some error 
already happened, therefore the result wouldn't be of interest
anymore, I think. Wrong?

The other case is the proper shutdown (see 12/12). However, in
that case Git is already exiting and therefore I wonder what
we would do with a "close" error?


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

OK!


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

This follows the existing filter function. Please see Peff's later
reply here:

"So I'm not sure if changing them is a good idea. I agree with you that
it's probably inviting confusion to have the two sets of filter
functions have opposite return codes. So I think I retract my
suggestion. :)"

http://public-inbox.org/git/20160728133523.GB21311%40sigill.intra.peff.net/

That's why I kept it the way it is. If you prefer the "!errors" approach
then I will change that.


Thanks for looking at the patch,
Lars