Re: [PATCH v3 10/10] convert: add filter.<driver>.process option

> On 01 Aug 2016, at 00:19, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> W dniu 30.07.2016 o 01:38, larsxschneider@gmail.com pisze:
> [...]
>> +Please note that you cannot use an existing filter.<driver>.clean
>> +or filter.<driver>.smudge command as filter.<driver>.process
>> +command.
> 
> I think it would be more readable and easier to understand to write:
> 
>  ... you cannot use an existing ... command with
>  filter.<driver>.process
> 
> About the style: wouldn't `filter.<driver>.process` be better?

>>             As soon as Git would detect a file that needs to be
>> +processed by this filter, it would stop responding.
> 
> This is quite convoluted, and hard to understand.  I would say
> that because `clean` and `smudge` filters are expected to read
> first, while Git expects `process` filter to say first, using
> `clean` or `smudge` filter without changes as `process` filter
> would lead to git command deadlocking / hanging / stopping
> responding.

>> +
>> +
>> Interaction between checkin/checkout attributes
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> 
>> diff --git a/convert.c b/convert.c
>> index 522e2c5..be6405c 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -3,6 +3,7 @@
>> #include "run-command.h"
>> #include "quote.h"
>> #include "sigchain.h"
>> +#include "pkt-line.h"
>> 
>> /*
>>  * convert.c - convert a file when checking it out and checking it in.
>> @@ -481,11 +482,355 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
>> 	return ret;
>> }
>> 
>> +static int multi_packet_read(int fd_in, struct strbuf *sb, size_t expected_bytes, int is_stream)
> 
> About name of this function: `multi_packet_read` is fine, though I wonder
> if `packet_read_in_full` with nearly the same parameters as `packet_read`,
> or `packet_read_till_flush`, or `read_in_full_packetized` would be better.

> Also, the problem is that while we know that what packet_read() stores
> would fit in memory (in size_t), it is not true for reading whole file,
> which might be very large - for example huge graphical assets like raw
> images or raw videos, or virtual machine images.  Isn't that the goal
> of git-LFS solutions, which need this feature?  Shouldn't we have then
> both `multi_packet_read_to_fd` and `multi_packet_read_to_buf`,
> or whatever?

>> +{
>> +	int bytes_read;
>> +	size_t total_bytes_read = 0;
> 
> Why `bytes_read` is int, while `total_bytes_read` is size_t? Ah, I see
> that packet_read() returns an int.  It should be ssize_t, just like
> read(), isn't it?  But we know that packet size is limited, and would
> fit in an int (or would it?).

>> +	if (expected_bytes == 0 && !is_stream)
>> +		return 0;
> 
> So in all cases *except* size = 0 we expect flush packet after the
> contents, but size = 0 is a corner case without flush packet?

>> +
>> +	if (is_stream)
>> +		strbuf_grow(sb, LARGE_PACKET_MAX);           // allocate space for at least one packet
>> +	else
>> +		strbuf_grow(sb, st_add(expected_bytes, 1));  // add one extra byte for the packet flush
>> +
>> +	do {
>> +		bytes_read = packet_read(
>> +			fd_in, NULL, NULL,
>> +			sb->buf + total_bytes_read, sb->len - total_bytes_read - 1,
>> +			PACKET_READ_GENTLE_ON_EOF
>> +		);
>> +		if (bytes_read < 0)
>> +			return 1;  // unexpected EOF
> 
> Don't we usually return negative numbers on error?  Ah, I see that the
> return is a bool, which allows to use boolean expression with 'return'.
> But I am still unsure if it is good API, this return value.

>> +
>> +		if (is_stream &&
>> +			bytes_read > 0 &&
>> +			sb->len - total_bytes_read - 1 <= 0)
>> +			strbuf_grow(sb, st_add(sb->len, LARGE_PACKET_MAX));
>> +		total_bytes_read += bytes_read;
>> +	}
>> +	while (
>> +		bytes_read > 0 &&                   // the last packet was no flush
>> +		sb->len - total_bytes_read - 1 > 0  // we still have space left in the buffer
> 
> Ah, so buffer is resized only in the 'is_stream' case.  Perhaps then
> use an "int options" instead of 'is_stream', and have one of flags
> tell if we should resize or not, that is if size parameter is hint
> or a strict limit.

>> +	);
>> +	strbuf_setlen(sb, total_bytes_read);
>> +	return (is_stream ? 0 : expected_bytes != total_bytes_read);
>> +}
>> +
>> +static int multi_packet_write_from_fd(const int fd_in, const int fd_out)
> 
> Is it equivalent of copy_fd() function, but where destination uses pkt-line
> and we need to pack data into pkt-lines?

>> +{
>> +	int did_fail = 0;
>> +	ssize_t bytes_to_write;
>> +	while (!did_fail) {
>> +		bytes_to_write = xread(fd_in, PKTLINE_DATA_START(packet_buffer), PKTLINE_DATA_LEN);
> 
> Using global variable packet_buffer makes this code thread-unsafe, isn't it?
> But perhaps that is not a problem, because other functions are also
> using this global variable.

> 
>> +		if (bytes_to_write < 0)
>> +			return 1;
>> +		if (bytes_to_write == 0)
>> +			break;
>> +		did_fail |= direct_packet_write(fd_out, packet_buffer, PKTLINE_HEADER_LEN + bytes_to_write, 1);
>> +	}
>> +	if (!did_fail)
>> +		did_fail = packet_flush_gentle(fd_out);
> 
> Shouldn't we try to flush even if there was an error?  Or is it
> that if there is an error writing, then there is some problem
> such that we know that flush would not work?

>> +	return did_fail;
> 
> Return true on fail?  Shouldn't we follow example of copy_fd()
> from copy.c, and return COPY_READ_ERROR, or COPY_WRITE_ERROR,
> or PKTLINE_WRITE_ERROR?

>> +}
>> +
>> +static int multi_packet_write_from_buf(const char *src, size_t len, int fd_out)
> 
> It is equivalent of write_in_full(), with different order of parameters,
> but where destination file descriptor expects pkt-line and we need to pack
> data into pkt-lines?

>> +{
>> +	int did_fail = 0;
>> +	size_t bytes_written = 0;
>> +	size_t bytes_to_write;
> 
> Note to self: bytes_to_write should fit in size_t, as it is limited to
> PKTLINE_DATA_LEN.  bytes_written should fit in size_t, as it is at most
> len, which is of type size_t.
> 
>> +	while (!did_fail) {
>> +		if ((len - bytes_written) > PKTLINE_DATA_LEN)
>> +			bytes_to_write = PKTLINE_DATA_LEN;
>> +		else
>> +			bytes_to_write = len - bytes_written;
>> +		if (bytes_to_write == 0)
>> +			break;
>> +		did_fail |= direct_packet_write_data(fd_out, src + bytes_written, bytes_to_write, 1);
>> +		bytes_written += bytes_to_write;
> 
> Ah, I see now why we need both direct_packet_write() and
> direct_packet_write_data().  Nice abstraction, makes for
> clear code.
> 
> The last parameter of '1' means 'gently', isn't it?

>> +	}
>> +	if (!did_fail)
>> +		did_fail = packet_flush_gentle(fd_out);
>> +	return did_fail;
>> +}
> 
> I think all three/four of those functions should be added in a separate
> commit, separate patch in patch series.

>  Namely:
> 
> - for git -> filter:
>    * read from fd,      write pkt-line to fd  (off_t)
>    * read from str+len, write pkt-line to fd  (size_t, ssize_t)
> - for filter -> git:
>    * read pkt-line from fd, write to fd       (off_t)

>> +
>> +#define FILTER_CAPABILITIES_STREAM   0x1
>> +#define FILTER_CAPABILITIES_CLEAN    0x2
>> +#define FILTER_CAPABILITIES_SMUDGE   0x4
>> +#define FILTER_CAPABILITIES_SHUTDOWN 0x8
>> +#define FILTER_SUPPORTS_STREAM(type) ((type) & FILTER_CAPABILITIES_STREAM)
>> +#define FILTER_SUPPORTS_CLEAN(type)  ((type) & FILTER_CAPABILITIES_CLEAN)
>> +#define FILTER_SUPPORTS_SMUDGE(type) ((type) & FILTER_CAPABILITIES_SMUDGE)
>> +#define FILTER_SUPPORTS_SHUTDOWN(type) ((type) & FILTER_CAPABILITIES_SHUTDOWN)
>> +
>> +struct cmd2process {
>> +	struct hashmap_entry ent; /* must be the first member! */
>> +	const char *cmd;
>> +	int supported_capabilities;
> 
> I wonder if switching from int (perhaps with field width of 1 to denote
> that it is boolean-like flag) to mask makes it more readable, or less.
> But I think it is.
> 
> 
> Reading Documentation/technical/api-hashmap.txt I found the following
> recommendation:
> 
>  `struct hashmap_entry`::
> 
>        An opaque structure representing an entry in the hash table, which must
>        be used as first member of user data structures. Ideally it should be
>        followed by an int-sized member to prevent unused memory on 64-bit
>        systems due to alignment.
> 
> Therefore it "int supported_capabilities" should precede
> "const char *cmd", I think.  Though it is not strictly necessary; it
> is not as if this hash table were large (maximum size is limited by
> the number of filter drivers configured), so we don't waste much space
> due to internal padding / due to alignment.

> 
>> +	struct child_process process;
>> +};
>> +
>> +static int cmd_process_map_initialized = 0;
>> +static struct hashmap cmd_process_map;
> 
> Reading Documentation/technical/api-hashmap.txt I see that:
> 
>  `tablesize` is the allocated size of the hash table. A non-0 value indicates
>  that the hashmap is initialized.
> 
> So cmd_process_map_initialized is not really needed, is it?

>> +
>> +static int cmd2process_cmp(const struct cmd2process *e1,
>> +							const struct cmd2process *e2,
>> +							const void *unused)
>> +{
>> +	return strcmp(e1->cmd, e2->cmd);
>> +}
> 
> Well, to be exact (which is decidely not needed!) two commands might
> be equivalent not being identical as strings (e.g. extra space between
> parameters).  But it is something the user should care about, not Git.
> 
>> +
>> +static struct cmd2process *find_protocol2_filter_entry(struct hashmap *hashmap, const char *cmd)
> 
> I'm not sure if *_protocol2_* is needed; those functions are static,
> local to convert.c.

>> +{
>> +	struct cmd2process k;
> 
> Does this name of variable 'k' follow established convention?
> 'key' would be more descriptive, but it's not as if this function
> was long; so 'k' is all right, I think.

> 
>> +	hashmap_entry_init(&k, strhash(cmd));
>> +	k.cmd = cmd;
>> +	return hashmap_get(hashmap, &k, NULL);
>> +}
>> +
>> +static void kill_protocol2_filter(struct hashmap *hashmap, struct cmd2process *entry) {
> 
> Programming style: the opening brace should be on separate line,
> that is:
> 
>  +static void kill_protocol2_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);
>> +	child_process_clear(&entry->process);
>> +	hashmap_remove(hashmap, entry, NULL);
>> +	free(entry);
>> +}
> 
> All those, from #define FILTER_CAPABILITIES_ to here could be put
> in a separate patch, to reduce size of this one.  But I am less
> sure that it is worth it for this case.
> 
>> +
>> +void shutdown_protocol2_filter(pid_t pid)
>> +{
> [...]
> 
> In my opinion this should be postponed to a separate commit.

> 
>> +}
>> +
>> +static struct cmd2process *start_protocol2_filter(struct hashmap *hashmap, const char *cmd)
> 
> This has some parts in common with existing filter_buffer_or_fd().
> I wonder if it would be worth to extract those common parts.
> 
> But perhaps it would be better to leave such refactoring for later.
> 
>> +{
>> +	int did_fail;
>> +	struct cmd2process *entry;
>> +	struct child_process *process;
>> +	const char *argv[] = { cmd, NULL };
>> +	struct string_list capabilities = STRING_LIST_INIT_NODUP;
>> +	char *capabilities_buffer;
>> +	int i;
>> +
>> +	entry = xmalloc(sizeof(*entry));
>> +	hashmap_entry_init(entry, strhash(cmd));
>> +	entry->cmd = cmd;
>> +	entry->supported_capabilities = 0;
>> +	process = &entry->process;
>> +
>> +	child_process_init(process);
> 
> filter_buffer_or_fd() uses instead
> 
>  struct child_process child_process = CHILD_PROCESS_INIT;
> 
> But I see that you need to access &entry->process anyway, so you
> need to have it here, and in this case child_process_init() is
> equivalent.
> 
> I wonder if it would be worth it to use strbuf for cmd.

>> +	process->argv = argv;
>> +	process->use_shell = 1;
>> +	process->in = -1;
>> +	process->out = -1;
>> +	process->clean_on_exit = 1;
>> +	process->clean_on_exit_handler = shutdown_protocol2_filter;
> 
> These two lines are new, and related to the "shutdown" capability, isn't it?

> 
>> +
>> +	if (start_command(process)) {
>> +		error("cannot fork to run external filter '%s'", cmd);
>> +		kill_protocol2_filter(hashmap, entry);
> 
> I guess the alternative solution of adding filter to the hashmap only
> after starting the process would be racy?
> 
> Ah, disregard that. I see that this pattern is a common way to error
> out in this function (for process-related errors).
> 
>> +		return NULL;
>> +	}
>> +
>> +	sigchain_push(SIGPIPE, SIG_IGN);
>> +	did_fail = strcmp(packet_read_line(process->out, NULL), "git-filter-protocol");
>> +	if (!did_fail)
>> +		did_fail |= strcmp(packet_read_line(process->out, NULL), "version 2");
>> +	if (!did_fail)
>> +		capabilities_buffer = packet_read_line(process->out, NULL);
>> +	else
>> +		capabilities_buffer = NULL;
>> +	sigchain_pop(SIGPIPE);
>> +
>> +	if (!did_fail && capabilities_buffer) {
>> +		string_list_split_in_place(&capabilities, capabilities_buffer, ' ', -1);
>> +		if (capabilities.nr > 1 &&
>> +			!strcmp(capabilities.items[0].string, "capabilities")) {
>> +			for (i = 1; i < capabilities.nr; i++) {
>> +				const char *requested = capabilities.items[i].string;
>> +				if (!strcmp(requested, "stream")) {
>> +					entry->supported_capabilities |= FILTER_CAPABILITIES_STREAM;
>> +				} else if (!strcmp(requested, "clean")) {
>> +					entry->supported_capabilities |= FILTER_CAPABILITIES_CLEAN;
>> +				} else if (!strcmp(requested, "smudge")) {
>> +					entry->supported_capabilities |= FILTER_CAPABILITIES_SMUDGE;
>> +				} else if (!strcmp(requested, "shutdown")) {
>> +					entry->supported_capabilities |= FILTER_CAPABILITIES_SHUTDOWN;
>> +				} else {
>> +					warning(
>> +						"external filter '%s' requested unsupported filter capability '%s'",
>> +						cmd, requested
>> +					);
>> +				}
>> +			}
>> +		} else {
>> +			error("filter capabilities not found");
>> +			did_fail = 1;
>> +		}
>> +		string_list_clear(&capabilities, 0);
>> +	}
> 
> I wonder if the above conditional wouldn't be better to be put in
> a separate function, parse_filter_capabilities(capabilities_buffer),
> returning a mask, or having mask as an out parameter, and returning
> an error condition.

>> +
>> +	if (did_fail) {
>> +		error("initialization for external filter '%s' failed", cmd);
> 
> More detailed information not needed, because one can use GIT_PACKET_TRACE.
> Would it be worth add this information as a kind of advice, or put it
> in the documentation of the `process` option?

> 
>> +		kill_protocol2_filter(hashmap, entry);
>> +		return NULL;
>> +	}
>> +
>> +	hashmap_add(hashmap, entry);
>> +	return entry;
>> +}
>> +
>> +static int apply_protocol2_filter(const char *path, const char *src, size_t len,
>> +						int fd, struct strbuf *dst, const char *cmd,
>> +						const int wanted_capability)
> 
> apply_protocol2_filter, or apply_process_filter?  Or rather,
> s/_protocol2_/_process_/g ?

> 
> This is equivalent to
> 
>   static int apply_filter(const char *path, const char *src, size_t len, int fd,
>                           struct strbuf *dst, const char *cmd)
> 
> Could we have extended that one instead?

>> +{
>> +	int ret = 1;
>> +	struct cmd2process *entry;
>> +	struct child_process *process;
>> +	struct stat file_stat;
>> +	struct strbuf nbuf = STRBUF_INIT;
>> +	size_t expected_bytes = 0;
>> +	char *strtol_end;
>> +	char *strbuf;
>> +	char *filter_type;
>> +	char *filter_result = NULL;
>> +
> 
>> +	if (!cmd || !*cmd)
>> +		return 0;
>> +
>> +	if (!dst)
>> +		return 1;
> 
> This is the same as in apply_filter().
> 
>> +
>> +	if (!cmd_process_map_initialized) {
>> +		cmd_process_map_initialized = 1;
>> +		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
>> +		entry = NULL;
>> +	} else {
>> +		entry = find_protocol2_filter_entry(&cmd_process_map, cmd);
>> +	}
> 
> Here we try to find existing process, rather than starting new
> as in apply_filter()
> 
>> +
>> +	fflush(NULL);
> 
> This is the same as in apply_filter(), but I wonder what it is for.

> 
>> +
>> +	if (!entry) {
>> +		entry = start_protocol2_filter(&cmd_process_map, cmd);
>> +		if (!entry) {
>> +			return 0;
>> +		}
> 
> Style; we prefer:
> 
>  +		if (!entry)
>  +			return 0;

> This is very similar to apply_filter(), but the latter uses start_async()
> from "run-command.h", with filter_buffer_or_fd() as asynchronous process,
> which gets passed command to run in struct filter_params.  In this
> function start_protocol2_filter() runs start_command(), synchronous API.
> 
> Why the difference?