git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v7 6/6] convert: add "status=delayed" to filter process protocol

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 27, 2017, 19:00 UTC
Message-ID
<xmqqbmp91cjk.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<20170627121027.99209-7-larsxschneider@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
Show 19 quoted lines
> @@ -533,7 +534,8 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
>  	if (err)
>  		goto done;
>  
> -	err = packet_writel(process->in, "capability=clean", "capability=smudge", NULL);
> +	err = packet_writel(process->in,
> +		"capability=clean", "capability=smudge", "capability=delay", NULL);
>  
>  	for (;;) {
>  		cap_buf = packet_read_line(process->out, NULL);
> @@ -549,6 +551,8 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
>  			entry->supported_capabilities |= CAP_CLEAN;
>  		} else if (!strcmp(cap_name, "smudge")) {
>  			entry->supported_capabilities |= CAP_SMUDGE;
> +		} else if (!strcmp(cap_name, "delay")) {
> +			entry->supported_capabilities |= CAP_DELAY;
>  		} else {
>  			warning(
>  				"external filter '%s' requested unsupported filter capability '%s'",

I thought you said something about attempting to make this more table-driven; did the attempt produce a better result? Just being curious.

Show 64 quoted lines
> @@ -590,9 +594,11 @@ static void handle_filter_error(const struct strbuf *filter_status,
>  
>  static int apply_multi_file_filter(const char *path, const char *src, size_t len,
>  				   int fd, struct strbuf *dst, const char *cmd,
> -				   const unsigned int wanted_capability)
> +				   const unsigned int wanted_capability,
> +				   struct delayed_checkout *dco)
>  {
>  	int err;
> +	int can_delay = 0;
>  	struct cmd2process *entry;
>  	struct child_process *process;
>  	struct strbuf nbuf = STRBUF_INIT;
> @@ -647,6 +653,14 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
>  	if (err)
>  		goto done;
>  
> +	if ((entry->supported_capabilities & CAP_DELAY) &&
> +	    dco && dco->state == CE_CAN_DELAY) {
> +		can_delay = 1;
> +		err = packet_write_fmt_gently(process->in, "can-delay=1\n");
> +		if (err)
> +			goto done;
> +	}
> +
>  	err = packet_flush_gently(process->in);
>  	if (err)
>  		goto done;
> @@ -662,14 +676,74 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
>  	if (err)
>  		goto done;
>  
> -	err = strcmp(filter_status.buf, "success");
> +	if (can_delay && !strcmp(filter_status.buf, "delayed")) {
> +		dco->is_delayed = 1;
> +		string_list_insert(&dco->filters, cmd);
> +		string_list_insert(&dco->paths, path);
> +	} else {
> +		/* The filter got the blob and wants to send us a response. */
> +		err = strcmp(filter_status.buf, "success");
> +		if (err)
> +			goto done;
> +
> +		err = read_packetized_to_strbuf(process->out, &nbuf) < 0;
> +		if (err)
> +			goto done;
> +
> +		err = subprocess_read_status(process->out, &filter_status);
> +		if (err)
> +			goto done;
> +
> +		err = strcmp(filter_status.buf, "success");
> +	}
> +
> +done:
> +	sigchain_pop(SIGPIPE);
> +
> +	if (err)
> +		handle_filter_error(&filter_status, entry, wanted_capability);
> +	else
> +		strbuf_swap(dst, &nbuf);
> +	strbuf_release(&nbuf);
> +	return !err;
> +}
This I can understand better than the previous round ;-)
Show 22 quoted lines
> diff --git a/convert.h b/convert.h
> index 82871a11d5..cdb91ab99a 100644
> --- a/convert.h
> +++ b/convert.h
> @@ -4,6 +4,8 @@
>  #ifndef CONVERT_H
>  #define CONVERT_H
>  
> +#include "string-list.h"
> +
>  enum safe_crlf {
>  	SAFE_CRLF_FALSE = 0,
>  	SAFE_CRLF_FAIL = 1,
> @@ -32,6 +34,27 @@ enum eol {
>  #endif
>  };
>  
> +enum ce_delay_state {
> +	CE_NO_DELAY = 0,
> +	CE_CAN_DELAY = 1,
> +	CE_RETRY = 2
> +};

This feels more natural and makes it easy to imagine the state transition diagram. enable-delay will take us from no-delay to can-delay, and we tell the filter that it is OK to delay while making the initial pass of the requests, and then after that we go into retry state to collect the delayed reponses.

Show 7 quoted lines
> +struct delayed_checkout {
> +	/* State of the currently processed cache entry. If the state is
> +	 * CE_CAN_DELAY, then the filter can change the 'is_delayed' flag
> +	 * to signal that the current cache entry was delayed. If the state is
> +	 * CE_RETRY, then this signals the filter that the cache entry was
> +	 * requested before.
> +	 */
        /*
         * Our multi-line comment look like this; slash-aster 
         * and aster-slash that opens and closes the block are
         * on their own lines.
         */
> +	enum ce_delay_state state;
> +	int is_delayed;
Hmph, I do not terribly mind but is this thing really needed?

Wouldn't filters and paths being non-empty be a good enough sign that the backend said "ok, I am allowed to give a delayed response so I acknowledge this path but would not give a result right away"?

Show 56 quoted lines
> +int finish_delayed_checkout(struct checkout *state)
> +{
> +	int errs = 0;
> +	struct string_list_item *filter, *path;
> +	struct delayed_checkout *dco = state->delayed_checkout;
> +
> +	if (!state->delayed_checkout)
> +		return errs;
> +
> +	dco->state = CE_RETRY;
> +	while (dco->filters.nr > 0) {
> +		for_each_string_list_item(filter, &dco->filters) {
> +			struct string_list available_paths = STRING_LIST_INIT_NODUP;
> +
> +			if (!async_query_available_blobs(filter->string, &available_paths)) {
> +				/* Filter reported an error */
> +				errs = 1;
> +				filter->string = "";
> +				continue;
> +			}
> +			if (available_paths.nr <= 0) {
> +				/* Filter responded with no entries. That means
> +				 * the filter is done and we can remove the
> +				 * filter from the list (see
> +				 * "string_list_remove_empty_items" call below).
> +				 */
> +				filter->string = "";
> +				continue;
> +			}
> +
> +			/* In dco->paths we store a list of all delayed paths.
> +			 * The filter just send us a list of available paths.
> +			 * Remove them from the list.
> +			 */
> +			filter_string_list(&dco->paths, 0,
> +				&remove_available_paths, &available_paths);
> +
> +			for_each_string_list_item(path, &available_paths) {
> +				struct cache_entry* ce;
> +
> +				if (!path->util) {
> +					error("external filter '%s' signaled that '%s' "
> +					      "is now available although it has not been "
> +					      "delayed earlier",
> +					      filter->string, path->string);
> +					errs |= 1;
> +
> +					/* Do not ask the filter for available blobs,
> +					 * again, as the filter is likely buggy.
> +					 */
> +					filter->string = "";
> +					continue;
> +				}
> +				ce = index_file_exists(state->istate, path->string,
> +						       strlen(path->string), 0);
> +				assert(dco->state == CE_RETRY);

Can anything futz with dco->state at this late in the game? You entered into CE_RETRY state at the beginning of this function, and this loop is going through each delayed ones. At this point, you are going to make , but not yet have made, a request to the backend via another call to checkout_entry() again.

Just wondering what kind of programming errors you are protecting yourself against. I briefly wondered perhaps you are afraid of a bug in checkout_entry() that may flip the state back, but it that is the case then the assert() would be after checkout_entry().

> +				errs |= (ce ? checkout_entry(ce, state, NULL) : 1);
Show 18 quoted lines
> +			}
> +		}
> +		string_list_remove_empty_items(&dco->filters, 0);
> +	}
> +	string_list_clear(&dco->filters, 0);
> +
> +	/* At this point we should not have any delayed paths anymore. */
> +	errs |= dco->paths.nr;
> +	for_each_string_list_item(path, &dco->paths) {
> +		error("'%s' was not filtered properly", path->string);
> +	}
> +	string_list_clear(&dco->paths, 0);
> +
> +	free(dco);
> +	state->delayed_checkout = NULL;
> +
> +	return errs;
> +}
Thanks.
Previous: Lars SchneiderNext: Lars Schneider
Message 9 of 15 in “convert: add "status=delayed" to filter process protocol”
  1. 0/6 convert: add "status=delayed" to filter process protocolLars Schneider, Jun 27, 2017
  2. 1/6 t0021: keep filter log files on comparisonLars Schneider, Jun 27, 2017
  3. 2/6 t0021: make debug log file name configurableLars Schneider, Jun 27, 2017
  4. 3/6 t0021: write "OUT <size>" only on successLars Schneider, Jun 27, 2017
  5. Junio C HamanoJun 27, 2017
  6. Lars SchneiderJun 27, 2017
  7. Junio C HamanoJun 27, 2017
  8. 6/6 convert: add "status=delayed" to filter process protocolLars Schneider, Jun 27, 2017
  9. Junio C HamanoJun 27, 2017
  10. Lars SchneiderJun 27, 2017
  11. Junio C HamanoJun 27, 2017
  12. Lars SchneiderJun 27, 2017
  13. Lars SchneiderJun 27, 2017
  14. 5/6 convert: move multiple file filter error handling to separate functionLars Schneider, Jun 27, 2017
  15. 4/6 convert: put the flags field before the flag itself for consistent styleLars Schneider, Jun 27, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.