From: Elijah Newren <newren@gmail.com> To: Jonathan Tan <jonathantanmy@google.com> Cc: Git Mailing List <git@vger.kernel.org> Subject: Re: [PATCH v2 06/20] merge-ort: implement a very basic collect_merge_info() Date: Fri, 6 Nov 2020 15:10:07 -0800 [thread overview] Message-ID: <CABPp-BFkAu6oO5V0jRh=ExuPxBXma1F17zaq-zEaqMagc72Ccw@mail.gmail.com> (raw) In-Reply-To: <20201106221934.764672-1-jonathantanmy@google.com> On Fri, Nov 6, 2020 at 2:19 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > > diff --git a/merge-ort.c b/merge-ort.c > > index 537da9f6df..626eb9713e 100644 > > --- a/merge-ort.c > > +++ b/merge-ort.c > > @@ -77,13 +77,130 @@ static int err(struct merge_options *opt, const char *err, ...) > > return -1; > > } > > > > +static int collect_merge_info_callback(int n, > > + unsigned long mask, > > + unsigned long dirmask, > > + struct name_entry *names, > > + struct traverse_info *info) > > +{ > > [snip] > > > + unsigned mbase_null = !(mask & 1); > > + unsigned side1_null = !(mask & 2); > > + unsigned side2_null = !(mask & 4); > > Should these be "int"? Does the type matter, particularly since "boolean" isn't available? > > + /* > > + * A bunch of sanity checks verifying that traverse_trees() calls > > + * us the way I expect. Could just remove these at some point, > > + * though maybe they are helpful to future code readers. > > + */ > > + assert(mbase_null == is_null_oid(&names[0].oid)); > > + assert(side1_null == is_null_oid(&names[1].oid)); > > + assert(side2_null == is_null_oid(&names[2].oid)); > > + assert(!mbase_null || !side1_null || !side2_null); > > + assert(mask > 0 && mask < 8); > > These were helpful to me. > > > + /* Other invariant checks, mostly for documentation purposes. */ > > + assert(mask == (dirmask | filemask)); > > But not this - filemask was computed in this function, so I need not > look elsewhere to see that this is correct. > > > + /* > > + * TODO: record information about the path other than all zeros, > > + * so we can resolve later in process_entries. > > + */ > > + ci = xcalloc(1, sizeof(struct conflict_info)); > > + strmap_put(&opti->paths, fullpath, ci); > > OK - so each entry is a full-size conflict_info to store all relevant > information. Presumably some of these will be converted later into what > is effectively a struct merged_info (so, the extra struct conflict_info > fields are unused but memory is still occupied). > > I do see that in patch 10, there is an optimization that directly > allocates the smaller struct merged_info when it is known at this point > that there is no conflict. Yep. :-) > [snip rest of function] > > > static int collect_merge_info(struct merge_options *opt, > > struct tree *merge_base, > > struct tree *side1, > > struct tree *side2) > > { > > - /* TODO: Implement this using traverse_trees() */ > > - die("Not yet implemented."); > > + int ret; > > + struct tree_desc t[3]; > > + struct traverse_info info; > > + char *toplevel_dir_placeholder = ""; > > + > > + opt->priv->current_dir_name = toplevel_dir_placeholder; > > + setup_traverse_info(&info, toplevel_dir_placeholder); > > I thought that this was written like this (instead of inlining the 2 > double-quotes) to ensure that the string-equality-is-pointer-equality > characteristic holds, but I see that that characteristic is for > directory_name in struct merged_info, not current_dir_name in struct > merge_options_internal. Any reason for not inlining ""? You're really digging in; I love it. From setup_path_info(), the directory_name is set from the current_dir_name: path_info->merged.directory_name = current_dir_name; (and if you follow where the current_dir_name parameter gets its value from, you find that it came indirectly from opt->priv->current_dir_name), so current_dir_name must meet all the requirements on merge_info's directory_name field. Perhaps there's still some kind of additional simplification possible here, but directory rename detection is an area that has to take some special care around this requirement. I simplified the code a little bit in this area as I was trying to break off a good first 20 patches to submit, but even if we can simplify it more, the structure is just going to come back later.
next prev parent reply other threads:[~2020-11-06 23:10 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 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 [this message] 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 06/20] merge-ort: implement a very basic collect_merge_info() 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='CABPp-BFkAu6oO5V0jRh=ExuPxBXma1F17zaq-zEaqMagc72Ccw@mail.gmail.com' \ --to=newren@gmail.com \ --cc=git@vger.kernel.org \ --cc=jonathantanmy@google.com \ --subject='Re: [PATCH v2 06/20] merge-ort: implement a very basic collect_merge_info()' \ /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).