Re: [PATCH 1/4] submodule-config: add submodules_of_tree() helper

From: Glen Choo <chooglen@google.com>
Date: Nov 23, 2021, 18:35 UTC
Message-ID: <kl6lk0gyk9ej.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <211123.86r1b7uoil.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 24 quoted lines

> Having skimmed through this topic isn't this in 4/4 the only resulting caller:
> 	
> 	+void create_submodule_branches(struct repository *r, const char *name,
> 	+			       const char *start_name, int force, int reflog,
> 	+			       int quiet, enum branch_track track)
> 	+{
> 	+	int i = 0;
> 	+	char *branch_point = NULL;
> 	+	struct repository *subrepos;
> 	+	struct submodule *submodules;
> 	+	struct object_id super_oid;
> 	+	struct submodule_entry_list *submodule_entry_list;
> 	+	char *err_msg = NULL;
> 	+
> 	+	validate_branch_start(r, start_name, track, &super_oid, &branch_point);
> 	+
> 	+	submodule_entry_list = submodules_of_tree(r, &super_oid);
> 	+	CALLOC_ARRAY(subrepos, submodule_entry_list->entry_nr);
> 	+	CALLOC_ARRAY(submodules, submodule_entry_list->entry_nr);
> 	+
> 	+	for (i = 0; i < submodule_entry_list->entry_nr; i++) {
>
> I think it would be better to just intorduce this function at the same
> time as its (only?) user, which also makes it clear how it's used.

Yes that makes sense. That is the only user (for now).

Show 16 quoted lines

> In this case this seems like quite a bit of over-allocation. I.e. we
> return a malloc'd pointer, and iterate with tree_entry(), the caller
> then needs to loop over that and do its own allocations of "struct
> repository *" and "struct submodule *".
>
> Wouldn't it be better just to have this new submodule_entry_list contain
> a list of not "struct name_entry", but:
>
>     struct new_thingy {
>         struct name_entry *entry;
>         struct repository *repo;
>         struct submodule *submodule;
>     }
>
> Then have the caller allocate the container on the stack, pass it to
> this function.

I thought about it as well. "struct new_thingy" is obviously the right struct for create_submodule_branches(), but I'm not sure if it is the right thing for other future callers e.g. "struct submodule" is only used to give a submodule name to users in help messages.

But chances are, any caller that needs 'submodules of a tree' will need very similar pieces of information, so it seems reasonable to do what you said instead of over-allocating in all of the callers.

> Maybe not, just musings while doing some light reading. I was surprised
> at what are effectively two loops over the same data, first allocating
> 1/3, then the other doing the other 2/3...

The first loop validates all submodules before creating any branches (and also happens to allocate). If we didn't have the validation step, allocation + creating branches could just be one loop :)