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