> > 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 :-)
