Re: [PATCH v1 3/3] convert: add filter.<driver>.useProtocol option

> larsxschneider@gmail.com wrote:
>> Please note that the protocol filters do not support stream processing
>> with this implemenatation because the filter needs to know the length of
>> the result in advance. A protocol version 2 could address this in a
>> future patch.
> 
> Would it be prudent to reuse pkt-line for this?

>> +static void stop_protocol_filter(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);
>> +	child_process_clear(&entry->process);
>> +	hashmap_remove(&cmd_process_map, entry, NULL);
>> +	free(entry);
>> +}
>> +
>> +static struct cmd2process *start_protocol_filter(const char *cmd)
>> +{
>> +	int ret = 1;
>> +	struct cmd2process *entry = NULL;
>> +	struct child_process *process = NULL;
> 
> These are unconditionally set below, so initializing to NULL
> may hide future bugs.

>> +	struct strbuf nbuf = STRBUF_INIT;
>> +	struct string_list split = STRING_LIST_INIT_NODUP;
>> +	const char *argv[] = { NULL, NULL };
>> +	const char *header = "git-filter-protocol\nversion";
> 
> 	static const char header[] = "git-filter-protocol\nversion";
> 
> ...might be smaller by avoiding the extra pointer
> (but compilers ought to be able to optimize it)

>> +	entry = xmalloc(sizeof(*entry));
>> +	hashmap_entry_init(entry, strhash(cmd));
>> +	entry->cmd = cmd;
>> +	process = &entry->process;
> 
> <snip>
> 
>> +	ret &= strncmp(header, split.items[0].string, strlen(header)) == 0;
> 
> starts_with() is probably more readable, here.

>> +static int apply_protocol_filter(const char *path, const char *src, size_t len,
>> +						int fd, struct strbuf *dst, const char *cmd,
>> +						const char *filter_type)
>> +{
>> +	int ret = 1;
>> +	struct cmd2process *entry = NULL;
>> +	struct child_process *process = NULL;
> 
> I would leave process initialized, here, since it should
> always be set below:

>> +	struct stat fileStat;
>> +	struct strbuf nbuf = STRBUF_INIT;
>> +	size_t nbuf_len;
>> +	char *strtol_end;
>> +	char c;
>> +
>> +	if (!cmd || !*cmd)
>> +		return 0;
>> +
>> +	if (!dst)
>> +		return 1;
>> +
>> +	if (!cmd_process_map_init) {
>> +		cmd_process_map_init = 1;
>> +		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
>> +	} else {
>> +		entry = find_protocol_filter_entry(cmd);
>> +	}
>> +
>> +	if (!entry){
>> +		entry = start_protocol_filter(cmd);
>> +		if (!entry) {
>> +			stop_protocol_filter(entry);
> 
> stop_protocol_filter is a no-op, here, since entry is NULL

>> +			return 0;
>> +		}
>> +	}
>> +	process = &entry->process;
>> +
>> +	sigchain_push(SIGPIPE, SIG_IGN);
>> +	switch (entry->protocol) {
>> +		case 1:
>> +			if (fd >= 0 && !src) {
>> +				ret &= fstat(fd, &fileStat) != -1;
>> +				len = fileStat.st_size;
> 
> There's a truncation bug when sizeof(size_t) < sizeof(off_t)

>> +    my $filelen  = <STDIN>;
>> +    chomp $filelen;
>> +    print $debug " $filelen";
>> +
>> +    $filelen = int($filelen);
> 
> Calling int() here is unnecessary and may hide bugs if you
> forget to check $debug.   Perhaps a regexp check is safer:
> 
> 	$filelen =~ /\A\d+\z/ or die "bad filelen: $filelen\n";