From: Jonathan Tan <jonathantanmy@google.com> To: newren@gmail.com Cc: git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com> Subject: Re: [PATCH v2 01/20] merge-ort: setup basic internal data structures Date: Fri, 6 Nov 2020 14:05:21 -0800 [thread overview] Message-ID: <20201106220521.757698-1-jonathantanmy@google.com> (raw) In-Reply-To: <20201102204344.342633-2-newren@gmail.com> I'm not very familiar with the merge machinery, but I'll attempt a review of at least the first 10 patches. > merged_info contains all relevant information for a non-conflicted > entry. conflict_info contains a merged_info, plus any additional > information about a conflict such as the higher orders stages involved > and the names of the paths those came from (handy once renames get > involved). If an entry remains conflicted, the merged_info portion of a > conflict_info will later be filled with whatever version of the file > should be placed in the working directory (e.g. an as-merged-as-possible > variation that contains conflict markers). I think that this information should be in the .c file. > diff --git a/merge-ort.c b/merge-ort.c > index b487901d3e..9d5ea0930d 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -17,6 +17,46 @@ > #include "cache.h" > #include "merge-ort.h" > > +#include "strmap.h" > + > +struct merge_options_internal { > + struct strmap paths; /* maps path -> (merged|conflict)_info */ > + struct strmap unmerged; /* maps path -> conflict_info */ > + const char *current_dir_name; > + int call_depth; > +}; Maybe document if the path is from the root of the directory or just the filename as it appears in a tree object? I would have expected "unmerged" to be a "strset", but I guess it's more convenient for it to be a map itself. Maybe just document it as "all mappings in paths wherein the value is a struct conflict_info". 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? 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. > +struct version_info { > + struct object_id oid; > + unsigned short mode; > +}; OK. > +struct merged_info { > + struct version_info result; > + unsigned is_null:1; > + unsigned clean:1; > + size_t basename_offset; > + /* > + * Containing directory name. Note that we assume directory_name is > + * constructed such that > + * strcmp(dir1_name, dir2_name) == 0 iff dir1_name == dir2_name, > + * i.e. string equality is equivalent to pointer equality. For this > + * to hold, we have to be careful setting directory_name. > + */ > + const char *directory_name; > +}; I'm not sure how most of the fields in this struct are to be used, but perhaps that will be clearer once I read the subsequent code. > +struct conflict_info { > + struct merged_info merged; > + struct version_info stages[3]; > + const char *pathnames[3]; Why would these be different across stages? (Rename detection?) > + unsigned df_conflict:1; OK. > + unsigned path_conflict:1; This doesn't seem to be assigned anywhere in this patch set? > + unsigned filemask:3; > + unsigned dirmask:3; I wonder if this needs to be documented that the least significant bit corresponds to stages[0], and so forth. > + unsigned match_mask:3; I think this can be derived by just looking at the stages array? Maybe document as: Optimization to track which stages match. Either 0 or at least 2 bits are set; if at least 2 bits are set, their corresponding stages match. > +};
next prev parent reply other threads:[~2020-11-06 22:05 UTC|newest] Thread overview: 84+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-02 20:43 [PATCH v2 00/20] fundamentals of merge-ort implementation Elijah Newren 2020-11-02 20:43 ` [PATCH v2 01/20] merge-ort: setup basic internal data structures Elijah Newren 2020-11-06 22:05 ` Jonathan Tan [this message] 2020-11-06 22:45 ` Elijah Newren 2020-11-09 20:55 ` Jonathan Tan 2020-11-02 20:43 ` [PATCH v2 02/20] merge-ort: add some high-level algorithm structure Elijah Newren 2020-11-02 20:43 ` [PATCH v2 03/20] merge-ort: port merge_start() from merge-recursive Elijah Newren 2020-11-11 13:52 ` Derrick Stolee 2020-11-11 16:22 ` Elijah Newren 2020-11-02 20:43 ` [PATCH v2 04/20] merge-ort: use histogram diff Elijah Newren 2020-11-11 13:54 ` Derrick Stolee 2020-11-11 16:47 ` Elijah Newren 2020-11-11 16:51 ` Derrick Stolee 2020-11-11 17:03 ` Elijah Newren 2020-11-02 20:43 ` [PATCH v2 05/20] merge-ort: add an err() function similar to one from merge-recursive Elijah Newren 2020-11-11 13:58 ` Derrick Stolee 2020-11-11 17:07 ` Elijah Newren 2020-11-11 17:10 ` Derrick Stolee 2020-11-02 20:43 ` [PATCH v2 06/20] merge-ort: implement a very basic collect_merge_info() Elijah Newren 2020-11-06 22:19 ` Jonathan Tan 2020-11-06 23:10 ` Elijah Newren 2020-11-09 20:59 ` Jonathan Tan 2020-11-11 14:38 ` Derrick Stolee 2020-11-11 17:02 ` Elijah Newren 2020-11-02 20:43 ` [PATCH v2 07/20] merge-ort: avoid repeating fill_tree_descriptor() on the same tree Elijah Newren 2020-11-11 14:51 ` Derrick Stolee 2020-11-11 17:13 ` Elijah Newren 2020-11-11 17:21 ` Eric Sunshine 2020-11-02 20:43 ` [PATCH v2 08/20] merge-ort: compute a few more useful fields for collect_merge_info Elijah Newren 2020-11-06 22:52 ` Jonathan Tan 2020-11-06 23:41 ` Elijah Newren 2020-11-09 22:04 ` Jonathan Tan 2020-11-09 23:05 ` Elijah Newren 2020-11-02 20:43 ` [PATCH v2 09/20] merge-ort: record stage and auxiliary info for every path Elijah Newren 2020-11-06 22:58 ` Jonathan Tan 2020-11-07 0:26 ` Elijah Newren 2020-11-09 22:09 ` Jonathan Tan 2020-11-09 23:08 ` Elijah Newren 2020-11-11 15:26 ` Derrick Stolee 2020-11-11 18:16 ` Elijah Newren 2020-11-11 22:06 ` Elijah Newren 2020-11-12 18:23 ` Derrick Stolee 2020-11-12 18:39 ` Derrick Stolee 2020-11-02 20:43 ` [PATCH v2 10/20] merge-ort: avoid recursing into identical trees Elijah Newren 2020-11-11 15:31 ` Derrick Stolee 2020-11-02 20:43 ` [PATCH v2 11/20] merge-ort: add a preliminary simple process_entries() implementation Elijah Newren 2020-11-11 19:51 ` Jonathan Tan 2020-11-12 1:48 ` Elijah Newren 2020-11-02 20:43 ` [PATCH v2 12/20] merge-ort: have process_entries operate in a defined order Elijah Newren 2020-11-11 16:09 ` Derrick Stolee 2020-11-11 18:58 ` Elijah Newren 2020-11-02 20:43 ` [PATCH v2 13/20] merge-ort: step 1 of tree writing -- record basenames, modes, and oids Elijah Newren 2020-11-11 20:01 ` Jonathan Tan 2020-11-11 20:24 ` Elijah Newren 2020-11-12 20:39 ` Jonathan Tan 2020-11-02 20:43 ` [PATCH v2 14/20] merge-ort: step 2 of tree writing -- function to create tree object Elijah Newren 2020-11-11 20:47 ` Jonathan Tan 2020-11-11 21:21 ` Elijah Newren 2020-11-02 20:43 ` [PATCH v2 15/20] merge-ort: step 3 of tree writing -- handling subdirectories as we go Elijah Newren 2020-11-12 20:15 ` Jonathan Tan 2020-11-12 22:30 ` Elijah Newren 2020-11-24 20:19 ` Elijah Newren 2020-11-25 2:07 ` Jonathan Tan 2020-11-26 18:13 ` Elijah Newren 2020-11-30 18:41 ` Jonathan Tan 2020-11-02 20:43 ` [PATCH v2 16/20] merge-ort: basic outline for merge_switch_to_result() Elijah Newren 2020-11-02 20:43 ` [PATCH v2 17/20] merge-ort: add implementation of checkout() Elijah Newren 2020-11-02 20:43 ` [PATCH v2 18/20] tree: enable cmp_cache_name_compare() to be used elsewhere Elijah Newren 2020-11-02 20:43 ` [PATCH v2 19/20] merge-ort: add implementation of record_unmerged_index_entries() Elijah Newren 2020-11-02 20:43 ` [PATCH v2 20/20] merge-ort: free data structures in merge_finalize() Elijah Newren 2020-11-03 14:49 ` [PATCH v2 00/20] fundamentals of merge-ort implementation Derrick Stolee 2020-11-03 16:36 ` Elijah Newren 2020-11-07 6:06 ` Elijah Newren 2020-11-07 15:02 ` Derrick Stolee 2020-11-07 19:39 ` Elijah Newren 2020-11-09 12:30 ` Derrick Stolee 2020-11-09 17:13 ` Elijah Newren 2020-11-09 19:51 ` Derrick Stolee 2020-11-09 22:44 ` Elijah Newren 2020-11-11 17:08 ` Derrick Stolee 2020-11-11 18:35 ` Elijah Newren 2020-11-11 20:48 ` Derrick Stolee 2020-11-11 21:18 ` Elijah Newren 2020-11-29 7:43 [PATCH " Elijah Newren via GitGitGadget 2020-12-04 20:47 ` [PATCH v2 " Elijah Newren via GitGitGadget 2020-12-04 20:47 ` [PATCH v2 01/20] merge-ort: setup basic internal data structures Elijah Newren via GitGitGadget
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201106220521.757698-1-jonathantanmy@google.com \ --to=jonathantanmy@google.com \ --cc=git@vger.kernel.org \ --cc=newren@gmail.com \ --subject='Re: [PATCH v2 01/20] merge-ort: setup basic internal data structures' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).