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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 9, 2020, 20:55 UTC
Message-ID: <20201109205525.2521806-1-jonathantanmy@google.com>
In-Reply-To: <CABPp-BHx4w1_JAen0fupCzosu0NfoTsT9d8dEve7n5VJZ0EvBQ@mail.gmail.com>

Show 17 quoted lines

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

Show 11 quoted lines

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

Show 6 quoted lines

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