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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Mar 21, 2017, 08:28 UTC
Message-ID: <BA6E546F-3594-4673-A43B-7E4D4CEA8F68@gmail.com>
In-Reply-To: <xmqq60jmnmef.fsf@junio-linux.mtv.corp.google.com>

Show 9 quoted lines

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

Show 18 quoted lines

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

Show 20 quoted lines

> 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