Christian Couder <christian.cou...@gmail.com> writes: > On Thu, May 12, 2016 at 10:43 PM, Junio C Hamano <gits...@pobox.com> wrote: >> Junio C Hamano <gits...@pobox.com> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
