Re: [PATCH v2 09/20] merge-ort: record stage and auxiliary info for every path

[Elijah Newren <newren@gmail.com>]

On Mon, Nov 9, 2020 at 2:09 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > > So these are placed in paths but not unmerged. I'm starting to wonder if
> > > struct merge_options_internal should be called merge_options_state or
> > > something, and each field having documentation about when they're used
> > > (or better yet, have functions like collect_merge_info() return their
> > > calculations in return values (which may be "out" parameters) instead of
> > > in this struct).
> >
> > Right, unmerged is only those paths that remain unmerged after all
> > steps.  record_unmerged_index_entries() could simply walk over all
> > entries in paths and pick out the ones that were unmerged, but
> > process_entries() has to walk over all paths, determine whether they
> > can be merged, and determine what to record for the resulting tree for
> > each path.  So, having it stash away the unmerged stuff is a simple
> > optimization.
> >
> > Renaming to merge_options_state or even just merge_state would be fine
> > -- but any renaming done here will also affect merge-recursive.[ch].
> > See the definition of merge_options in merge-recursive.  (For history,
> > merge-recursive.h stuffed state into merge_options, which risked funny
> > misusage patterns and made the API unnecessarily complex...and made it
> > suggest that alternative algorithms needed to have the same state.
> > So, the state was moved to a merge_options_internal struct.  That's
> > not to say we can't rename, but it does need to be done in
> > merge-recursive as well.)
>
> Ah, I see.
>
> > As for having collect_merge_info() return their calculations in return
> > values, would that just end with me returning a struct
> > merge_options_internal?  Or did you want each return value added to
> > the function signature?  Each return value in the function signature
> > makes sense right now for this super-simplified initial 20 patches,
> > but what about when this data structure gains all kind of
> > rename-related state that is collected, updated, and passed between
> > these areas?  I'd have a huge number of "out" and "in" fields to every
> > function.  Eventually, merge_options_internal (or whatever it might be
> > renamed to) expands to the following, where I have to first define an
> > extra enum and two extra structs so that you know the definitions of
> > new types that show up in merge_options_internal:
>
> [snip enums and structs]
>
> Good point. I should have realized that there would be much more to
> track.
>
> > > > +     result->string = fullpath;
> > > > +     result->util = path_info;
> > > > +}
> > > > +
> > > >  static int collect_merge_info_callback(int n,
> > > >                                      unsigned long mask,
> > > >                                      unsigned long dirmask,
> > > > @@ -91,10 +136,12 @@ static int collect_merge_info_callback(int n,
> > > >        */
> > > >       struct merge_options *opt = info->data;
> > > >       struct merge_options_internal *opti = opt->priv;
> > > > -     struct conflict_info *ci;
> > > > +     struct string_list_item pi;  /* Path Info */
> > > > +     struct conflict_info *ci; /* pi.util when there's a conflict */
> > >
> > > Looking ahead to patch 10, this seems more like "pi.util unless we know
> > > for sure that there's no conflict".
> >
> > That's too long for the line to remain at 80 characters; it's 16
> > characters over the limit.  ;-)
>
> Well, you could move the description onto its own line :-)

:-)