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

On 27 Jun 2017, at 21:00, Junio C Hamano [off-list ref] wrote:

Lars Schneider [off-list ref] writes:

quoted

@@ -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.

I treated that as low-prio as I got the impression that you are generally OK with
the current implementation. I promise to send a patch with this change. 
Either as part of this series or as a follow up patch.

...

quoted

+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.
        */

Oops. Lesson learned.

quoted

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

Yes. I guess that was a premature optimization on my end.
This is how "write_entry()" in entry.c would change:

-                               dco->is_delayed = 0;
                                ret = async_convert_to_working_tree(
                                        ce->name, new, size, &buf, dco);
-                               if (ret && dco->is_delayed) {
+                               if (ret && string_list_has_string(&dco->paths, ce->name)) {
                                        free(new);
                                        goto finish;
                                }

OK?

quoted

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

At this point I want to ensure that checkout_entry() is called
with the CE_RETRY state. Because checkout_entry() needs to pass 
that flag to the convert machinery.

This assert was useful when checkout_entry() changed dco->state
between CAN_DELAY and DELAYED. In the current implementation the
state should not be changed anymore.

Would you prefer if I remove it? (with or without is both fine with me)


Thanks,
Lars