On Sat, May 14, 2016 at 8:27 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Christian Couder <christian.couder@xxxxxxxxx> writes:
>
>> On Thu, May 12, 2016 at 10:43 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>> Junio C Hamano <gitster@xxxxxxxxx> writes:
>>>
>>>> Up to this point, the conversion looks quite sensible, even though I
>>>> think the organization of fields in apply_state do not look logical.
>>>
>>> I'd stop here for now, as everything before this step looks
>>> uncontroversial. Anybody whose tasked to move the global state for
>>> these variables into a structure would reach the samestate after
>>> applying these 48 patches, modulo minor differences in the way the
>>> comments would say things, how the patches are split and how the
>>> fields in apply_state() are organized.
>>>
>>> One thing that is missing is a counterpart of init_apply_state().
>>> In the early part of patches where it added only "const char *"
>>> borrowed from the caller and fields of intregral type, the lack of
>>> clear_apply_state() did not mattter, but with a few fields with
>>> "string_list" type, anybody who want to make repeated call into the
>>> apply machinery would want a way to release the resource the
>>> structure holds.
>>>
>>> Because 49/94 is a step to add an unfreeable resource, this is a
>>> good place to stop and then add the clean_apply_state() before that
>>> happens, I would think. After that, I think both the contributor
>>> and the reviewers would benefit if these early patches are merged
>>> early without waiting for the review of the remainder.
>>
>> Ok, I will add add the clean_apply_state() and resend the patches up
>> to that point soon, so that they can be merged early.
>
> Two more comments:
>
> - Just like the change between v1 and v2, if we were to introduce
> clear_apply_state(), it would be nicer if it were done early in
> the series, ideally at the same time as init_apply_state() gets
> introduced,
Yeah, I have done that in v3.
> or at least no later than the first field that holds
> resource that needs releasing is added.
>
> - I didn't double check if 50 and later still had changes that
> belong to this "early batch that moves things into a struct" (aka
> cc/apply-introduce-state) topic, as I stopped at 49 and saw the
> need for clear_apply_state(). So 48 may not be the ideal place
> to stop. From a cursory read of their titles, perhaps 49, 50, 56
> and possibly 60 should be in this early series? While 60 does
> sound like an immediate follow-up to 09/94, it depends on a few
> "die elimination" changes, so I do NOT think it must be in the
> early batch.
For v3 I stopped at patch 50 from v2.
Patch 56 moves 'struct apply_state' into apply.h and it can logically
belong to another patch series, as anyway 'struct apply_state' is not
usable by other code as long as the related functions
(init_apply_state(), clear_apply_state(), check_apply_state(), ...)
are not also moved to apply.{c,h}.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html