On 06 Mar 2017, at 22:08, Junio C Hamano [off-list ref] wrote:

Lars Schneider [off-list ref] writes:

quoted

quoted

On 04 Mar 2017, at 00:26, Junio C Hamano [off-list ref] wrote:


* ls/filter-process-delayed (2017-01-08) 1 commit
. convert: add "status=delayed" to filter process protocol

Sorry for the delayed response. I am still pursuing this topic but
unfortunately I wasn't able to spend time on it recently.

For example, your async_convert_to_working_tree() returns Success or
Delayed [*2*] and the callers of write_entry() cannot tell which the
paths on the filesystem needs a call to checkout_delayed_entries()
to finish them before they can safely tell the outside world that
these paths are safe to use.  

It seems to me that any caller that calls checkout_entry() needs to
essentially do:

	- compute which entries in the index need to be checked out
         to the filesystem;

	- for each of them:
		- call checkout_entry()

	- call checkout_delayed_entries(), because among the
         checkout_entry() calls we did in the above loop, some of
         them may have "delayed", but we do not know which one(s).

Agreed. Would it be OK to store the "delayed" bit in the cache
entry itself? The extended ce_flags are stored on disk which is not
necessary I think. Would a new member in the cache_entry struct be
an acceptable option?

Output from "git grep -e checkout_delayed_entries -e checkout_entry"
seems to tell me that at least builtin/apply.c and
builtin/checkout-index.c forget the last step.

For all these "single" checkout entry places the delayed checkout
makes no sense I think. Would you prefer something likes this:

checkout_entry(); finish_checkout();

or this:

checkout_entry(..., DISABLE_DELAYED_CHECKOUT);

in all these places?

I'd understand the design better if the delayed-item list were a
part of the "struct checkout" state structure, and write_entry(),

This works [1], however I had to remove "const" from 
"const struct checkout *state" in a few places. OK?

when delaying the write, returned enough information (not just "this
has been delayed") that can be used to later instruct the
long-running filter process things like "you gave me this 'delayed'
token earlier; I want the result for it now!", "are you finished
processing my earlier request, to which you gave me this 'delayed'
token?", etc.  One of these instructions could be "here is the
path. write the result out for the earlier request of mine you gave
me this 'delayed' token for.  I do not need the result in-core.  And
take as much time as you need--I do not mind blocking here at this
point."  In a future, a new instruction may be added to ask "please
give me the result in-core, as if you returned the result to my
earlier original async_convert_to_working_tree() call without
delaying the request."

Within such a framework, your checkout_delayed_entries() would be a
special case for finalizing a "struct checkout" that has been in
use.  By enforcing that any "struct checkout" begins its life by a
"state = CHECKOUT_INIT" initialization and finishes its life by a
"finish_checkout(&state)" call, we will reduce risks to forget
making necessary call to checkout_delayed_entries(), I would think.

Agreed. How would you want to enforce "finish_checkout(&state)", though?
By convention or by different means?

*2* By the way, the code in write_entry() should have a final else
clause that diagnoses an error return from
async_convert_to_working_tree() and act on it---an unexpected return
will fall through to the code that opens output fd and

It is ok for async_convert_to_working_tree() to fail if the filter is not
required. That's how it is currently implemented upstream.


I will post a new round to the mailing list soon. Here is a preview if
you want to look at the const changes etc:
https://github.com/larsxschneider/git/commit/a0132e564faaf5bca76af0ccc4ec6fe900be739a?w=1


Thanks,
Lars