Re: What's cooking in git.git (Mar 2017, #02; Fri, 3)

[Lars Schneider <larsxschneider@gmail.com>]

> On 06 Mar 2017, at 22:08, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Lars Schneider <larsxschneider@gmail.com> writes:
> 
>>> On 04 Mar 2017, at 00:26, Junio C Hamano <gitster@pobox.com> 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