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

> On 27 Jun 2017, at 21:00, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Lars Schneider <larsxschneider@gmail.com> writes:
> 
>> @@ -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.

> ...
> 
>> +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"?

>> +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().