Re: [PATCH v1 3/3] convert: add filter.<driver>.useProtocol option
- From
- Eric Wong <e@80x24.org>
- Date
- Jul 23, 2016, 08:14 UTC
- Message-ID
- <20160723081447.GA24318@starla>
- In-Reply-To
- <20160722154900.19477-4-larsxschneider@gmail.com>
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?
Show 18 quoted lines
> +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.
Show 7 quoted lines
> +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:
Show 23 quoted lines
> + 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
Show 11 quoted lines
> + 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) (and mixedCase is inconsistent with our style)
Show 5 quoted lines
> + 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";