Re: [PATCH v2 01/20] merge-ort: setup basic internal data structures

[Jonathan Tan <jonathantanmy@google.com>]

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

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

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