Re: [PATCH v2 13/20] merge-ort: step 1 of tree writing -- record basenames, modes, and oids
- From
Elijah Newren <newren@gmail.com>
- Date
- Nov 11, 2020, 20:24 UTC
- Message-ID
- <CABPp-BGr4kpjyZc3Rbh+8StPbWWqo36Kg0P=dgqctwX0SCGPxw@mail.gmail.com>
- In-Reply-To
- <20201111200157.3118955-1-jonathantanmy@google.com>
On Wed, Nov 11, 2020 at 12:01 PM Jonathan Tan <jonathantanmy@google.com> wrote:
Show 6 quoted lines
>
> > +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.
Show 19 quoted lines
>
> > @@ -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.