Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> 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).
> 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 :)
