Re: [PATCH v2 01/20] merge-ort: setup basic internal data structures
From: Jonathan Tan <hidden>
Date: 2020-11-09 20:55:30
quoted
There seems to be 2 ways of referring to something that we couldn't merge - "conflicted" (or "having a conflict") and "unmerged". Should we stick to one of them?Uhm...perhaps, but it feels like I'm going to miss some while looking over it. Also, there are some semantic differences at play. Since the whole algorithm is divided around multiple stages -- collect_merge_info(), detect_and_process_renames(), process_entries(), as of a given early stage we might just know that we couldn't merge it *yet*. For example, both sides modified the entry, or one side modified and the other side is missing ("did they delete it or rename it?"). After rename detection and three-way content merge, something that had not been automatically mergeable as of an earlier step might end up being so. But we need names for stuff in the interim state. But it's also possible for us to know at an early state that thing are definitely going to be a conflict regardless of what later stages do (e.g. both sides rename a path, but rename it differently).
In that case, maybe "possibly conflicted" and "unconflicted"? Or maybe someone else will come up with a better name.
quoted
Also, looking ahead, I see that current_dir_name is used as a temporary variable in the recursive calls to collect_merge_info_callback(). I would prefer if current_dir_name went in the cbdata to that function instead, but if that's not possible, maybe document here that current_dir_name is only used in collect_merge_info_callback(), and temporarily at that.Yeah, not possible. collect_merge_info_callback() has to be of traverse_callback_t type (from tree-walk.h), which provides no extra parameters for extra callback data. I can add a documentation comment.
But traverse_callback_t provides a data field, which you use to store a struct merge_options in patch 6 - in theory, you could instead create another struct that contains a pointer to struct merge_options and store that in data instead. That does sound like it will make the code more complicated, though, so maybe current_dir_name here is the way.
quoted
I wonder if this needs to be documented that the least significant bit corresponds to stages[0], and so forth.Maybe I should just put a comment to look at tree-walk.h? The struct traverse_info has a "fn" member with a big comment above it describing mask & dirmask; filemask is just mask & ~dirmask.
That makes sense. I understood a lot more once I saw that it was iterating over a variable number of trees - hence the arrays.