> On 27 Jun 2017, at 21:00, Junio C Hamano <[email protected]> wrote:
>
> Lars Schneider <[email protected]> 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.
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.
> ...
>
>> +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.
>> + 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?
>> +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
