Thread (83 messages) 83 messages, 4 authors, 2020-11-30

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help