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

On Fri, Nov 6, 2020 at 2:58 PM Jonathan Tan <jonathantanmy@google.com> wrote:

I guess this goes back to the question on patch 6 where you suggested I mark some unsigned variables (derived from bit math on other unsigned quantities) instead be int. I guess it could, but I have the same question; since "boolean" isn't available in C, does int vs. unsigned matter?

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

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:

enum relevance {
    RELEVANT_NO_MORE = 0,
    RELEVANT_CONTENT = 1,
    RELEVANT_LOCATION = 2,
    RELEVANT_BOTH = 3
};

struct traversal_callback_data {
    unsigned long mask;
    unsigned long dirmask;
    struct name_entry names[3];
};

struct rename_info {
    /* For the next six vars, the 0th entry is ignored and unused */
    struct diff_queue_struct pairs[3]; /* input to & output from
diffcore_rename */
    struct strintmap relevant_sources[3];  /* filepath => enum relevance */
    struct strintmap dirs_removed[3];      /* directory => bool */
    struct strmap dir_rename_count[3];     /* old_dir => {new_dir => int} */
    struct strintmap possible_trivial_merges[3]; /* dirname->dir_rename_mask */
    struct strset target_dirs[3];             /* set of directory paths */
    unsigned trivial_merges_okay[3];          /* 0 = no, 1 = maybe */
    /*
     * dir_rename_mask:
     *   0: optimization removing unmodified potential rename source okay
     *   2 or 4: optimization okay, but must check for files added to dir
     *   7: optimization forbidden; need rename source in case of dir rename
     */
    unsigned dir_rename_mask:3;

    /*
     * dir_rename_mask needs to be coupled with a traversal through trees
     * that iterates over all files in a given tree before all immediate
     * subdirectories within that tree.  Since traverse_trees() doesn't do
     * that naturally, we have a traverse_trees_wrapper() that stores any
     * immediate subdirectories while traversing files, then traverses the
     * immediate subdirectories later.
     */
    struct traversal_callback_data *callback_data;
    int callback_data_nr, callback_data_alloc;
    char *callback_data_traverse_path;

    /*
     * When doing repeated merges, we can re-use renaming information from
     * previous merges under special circumstances;
     */
    struct tree *merge_trees[3];
    int cached_pairs_valid_side;
    struct strmap cached_pairs[3];   /* fullnames -> {rename_path or NULL} */
    struct strset cached_irrelevant[3]; /* fullnames */
    struct strset cached_target_names[3]; /* set of target fullnames */
    /*
     * And sometimes it pays to detect renames, and then restart the merge
     * with the renames cached so that we can do trivial tree merging.
     * Values: 0 = don't bother, 1 = let's do it, 2 = we already did it.
     */
    unsigned redo_after_renames;
};

struct merge_options_internal {
    struct strmap paths;    /* maps path -> (merged|conflict)_info */
    struct strmap unmerged; /* maps path -> conflict_info */
#if USE_MEMORY_POOL
    struct mem_pool pool;
#else
    struct string_list paths_to_free; /* list of strings to free */
#endif
    struct rename_info *renames;
    struct index_state attr_index; /* renormalization weirdly needs one... */
    struct strmap output;  /* maps path -> conflict messages */
    const char *current_dir_name;
    char *toplevel_dir; /* see merge_info.directory_name comment */
    int call_depth;
    int needed_rename_limit;
};

That's too long for the line to remain at 80 characters; it's 16 characters over the limit. ;-)