On Wed, Nov 11, 2020 at 12:01 PM Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
>
> > +struct directory_versions {
> > + struct string_list versions;
>
> Maybe comment that this is an unordered list of basenames to <whatever
> the type of ci->merged.result is>.
There actually is an order, and it's important. It's reverse
lexicographic order of full pathnames (the ordering comes from the
fact that process_entries() iterates paths in that order). The
reasons for that ordering are (1) all the basenames within a directory
are adjacent so that I can write out a tree for a directory as soon as
it is done, and (2) paths within a directory are listed before the
directory itself so that I get the necessary info for subtrees before
trying to write out their parent trees.
It's not until later patches that I take advantage of this ordering
(and when I do I have a very long commit message to describe it all),
but I can add a comment that this is a list of basenames to
merge_info.
>
> > @@ -442,6 +464,7 @@ static void process_entries(struct merge_options *opt,
> > struct strmap_entry *e;
> > struct string_list plist = STRING_LIST_INIT_NODUP;
> > struct string_list_item *entry;
> > + struct directory_versions dir_metadata;
> >
> > if (strmap_empty(&opt->priv->paths)) {
> > oidcpy(result_oid, opt->repo->hash_algo->empty_tree);
> > @@ -458,6 +481,9 @@ static void process_entries(struct merge_options *opt,
> > plist.cmp = string_list_df_name_compare;
> > string_list_sort(&plist);
> >
> > + /* other setup */
> > + string_list_init(&dir_metadata.versions, 0);
> > +
>
> Might be clearer to just initialize dir_metadata as {
> STRING_LIST_INIT_NODUP }.
It'll eventually grow to { STRING_LIST_INIT_NODUP,
STRING_LIST_INIT_NODUP, NULL, 0 }, which is a tad long, but if the
initializer is clearer I'm happy to switch over to it.
> The rest makes sense.
