Re: [PATCH v2 48/94] builtin/apply: rename 'prefix_' parameter to 'prefix'

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]