Re: [PATCH v2 1/3] serialize collection of changed submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 10, 2016, 22:43 UTC
Message-ID: <xmqqvawzbzb2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kZiY56-84aThH1F02E_HzCTAK3KSYLbyP1D5GUAt892cw@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 16 quoted lines

>> +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.