On Mon, Oct 10, 2016 at 03:43:13PM -0700, Junio C Hamano wrote: > Stefan Beller <sbeller@xxxxxxxxxx> writes: > > >> +static struct sha1_array *get_sha1s_from_list(struct string_list *submodules, > >> + const char *path) > > > > So this will take the stringlist `submodules` and insert the path into it, > > if it wasn't already in there. In case it is newly inserted, add a sha1_array > > as util, so each inserted path has it's own empty array. > > > > So it is both init of the data structures as well as retrieving them. I was > > initially confused by the name as I assumed it would give you sha1s out > > of a string list (e.g. transform strings to internal sha1 things). > > Maybe it's just > > me having a hard time to understand that, but I feel like the name could be > > improved. > > > > lookup_sha1_list_by_path, > > insert_path_and_return_sha1_list ? > > I do not think either the name or the "find if exists otherwise > initialize one" behaviour is particularly confusing, but I do not > think "maintain a set of sha1_arrays keyed with a string" is a so > widely reusable general concept/construct. As can be seen easily in > the names of parameters, this function is about maintaining a set of > sha1_arrays keyed by paths to submodules, and I also assume that the > array indexed by path is not meant to be a general purpose "we can > use it to store any 40-hex thing" but to store something specific. > > What is that specific thing? The names of commit objects in the > submodule repository? > > I'd prefer to see that exact thing used to construct the function > name for a helper function with specific usage in mind, i.e. > get_commit_object_names_for_submodule_path() or something along that > line. I did not name this function too precisely to keep it's name short since everything specific was quite long, like the suggestion from Junio. Since this is a static function local to the submodule file I was assuming anyone interested would just look up the usage and immediately see the purpose. If I look into submodule-cache.c where I have a similar functionality we used 'lookup_or_create' for this create on demand functionality. So a function name would be: lookup_or_create_commit_objects_for_submodule_path(... Which seems quite extensively long for a static function so how about we shorten it a bit and add a comment: /* lookup or create commit object list for submodule */ get_commit_objects_for_submodule_path(... ? Cheers Heiko