On Mon, Nov 9, 2020 at 2:09 PM Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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 :-) :-)
