On Thu, May 15, 2025 at 4:18 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > K Jayatheerth <jayatheerthkulkarni2005@xxxxxxxxx> writes: > > > Add helper `submodule_active_matches_path()` so we can > > re-implement the old “is this path already covered by > > submodule.active?” logic without re-reading the config twice. > > Having duplicated code to implement what is supposed to be the same > thing is a bug waiting to happen by them diverging from each other. > > Isn't the fact that our configuration reading code reads things just > once and the caches the result good enough for the purpose of this > code path? Do we have a measurement that tells us that the extra > complexity is worth the maintenance headache? > Well when I first sent this patch I didn't quite understand why test 4137 was failing then I read the tests and I didn't have a lot of idea of the code. But I think it' better to remove helper as you said I will send a new patch with a different approach as I've spent some time understanding this now. > > @@ -3443,7 +3452,11 @@ static int module_add(int argc, const char **argv, const char *prefix, > > int force = 0, quiet = 0, progress = 0, dissociate = 0; > > struct add_data add_data = ADD_DATA_INIT; > > const char *ref_storage_format = NULL; > > + const struct submodule *existing; > > char *to_free = NULL; > > + struct strbuf buf = STRBUF_INIT; > > + int i; > > + int allocated_sm_name = 0; > > A separate flag is not wrong per-se, but the idiom used in this > project more often is to have an extra pointer variable that points > at an allocated piece of memory (or NULL), and free the piece of > memory unconditionally. > > "git grep -e to_free" to see the idiom in action. Even better yet, > this codepath already uses the idiom. > > By doing so > > > + if (allocated_sm_name) > > + free((char *)add_data.sm_name); > > becomes > > free(sm_name_to_free); > > and we can keep the "add_data.sm_name is pointing at a borrowed > piece of memory, and we will _never_ free anything through that > pointer" memory ownership rule. We were borrowing from a separate > variable sm_name_to_free, and we may free it when add_data is > getting destroyed, or we may be borrowing from the .sm_path string, > which we would free it when add_data is getting destroyed. > Interesting, this is good, I'm going to copy this : ) Thank you, -Jayatheerth
