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

Junio C Hamano [off-list ref] 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.

Thanks.

On Thu, May 12, 2016 at 10:43 PM, Junio C Hamano [off-list ref] wrote:

Junio C Hamano [off-list ref] writes:

quoted

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.

Thanks,
Christian.