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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 27, 2017, 21:30 UTC
Message-ID: <xmqqlgodyv7k.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <77DBCB49-2A44-42BB-9060-23FA79C1CF57@gmail.com>

Lars Schneider <larsxschneider@gmail.com> writes:

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

Sure. I was just being curious what happened; I agree that it is a good idea to treat this as a lower priority clean-up item and leave it to later.

Show 13 quoted lines

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

Actually I should be asking the "OK?" question---the above was what I had in mind when I commented, but you are a lot more familiar with the way how this code is supposed to work, and I didn't know if the emptyness of the list can be a 100% substitute for that bit. If you think they are equivalent, that is a great news.

Show 9 quoted lines

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

If that is the reason behind the assert, I am OK with either (1) moving it _after_ checkout_entry() returns (to ensure that the function no longer mucks with the state, or (2) removing it.

Leaving it at the current position does not make much sense to me.