From: Elijah Newren <firstname.lastname@example.org> To: Jonathan Tan <email@example.com> Cc: Git Mailing List <firstname.lastname@example.org> Subject: Re: [PATCH v2 09/20] merge-ort: record stage and auxiliary info for every path Date: Mon, 9 Nov 2020 15:08:58 -0800 [thread overview] Message-ID: <CABPp-BE=4uTRyxsuBUz7iB7+kqKjtM1-J3qiSi4q1nvwC1Odvg@mail.gmail.com> (raw) In-Reply-To: <email@example.com> On Mon, Nov 9, 2020 at 2:09 PM Jonathan Tan <firstname.lastname@example.org> wrote: > > > > So these are placed in paths but not unmerged. I'm starting to wonder if > > > struct merge_options_internal should be called merge_options_state or > > > something, and each field having documentation about when they're used > > > (or better yet, have functions like collect_merge_info() return their > > > calculations in return values (which may be "out" parameters) instead of > > > in this struct). > > > > Right, unmerged is only those paths that remain unmerged after all > > steps. record_unmerged_index_entries() could simply walk over all > > entries in paths and pick out the ones that were unmerged, but > > process_entries() has to walk over all paths, determine whether they > > can be merged, and determine what to record for the resulting tree for > > each path. So, having it stash away the unmerged stuff is a simple > > optimization. > > > > Renaming to merge_options_state or even just merge_state would be fine > > -- but any renaming done here will also affect merge-recursive.[ch]. > > See the definition of merge_options in merge-recursive. (For history, > > merge-recursive.h stuffed state into merge_options, which risked funny > > misusage patterns and made the API unnecessarily complex...and made it > > suggest that alternative algorithms needed to have the same state. > > So, the state was moved to a merge_options_internal struct. That's > > not to say we can't rename, but it does need to be done in > > merge-recursive as well.) > > Ah, I see. > > > As for having collect_merge_info() return their calculations in return > > values, would that just end with me returning a struct > > merge_options_internal? Or did you want each return value added to > > the function signature? Each return value in the function signature > > makes sense right now for this super-simplified initial 20 patches, > > but what about when this data structure gains all kind of > > rename-related state that is collected, updated, and passed between > > these areas? I'd have a huge number of "out" and "in" fields to every > > function. Eventually, merge_options_internal (or whatever it might be > > renamed to) expands to the following, where I have to first define an > > extra enum and two extra structs so that you know the definitions of > > new types that show up in merge_options_internal: > > [snip enums and structs] > > Good point. I should have realized that there would be much more to > track. > > > > > + result->string = fullpath; > > > > + result->util = path_info; > > > > +} > > > > + > > > > static int collect_merge_info_callback(int n, > > > > unsigned long mask, > > > > unsigned long dirmask, > > > > @@ -91,10 +136,12 @@ static int collect_merge_info_callback(int n, > > > > */ > > > > struct merge_options *opt = info->data; > > > > struct merge_options_internal *opti = opt->priv; > > > > - struct conflict_info *ci; > > > > + struct string_list_item pi; /* Path Info */ > > > > + struct conflict_info *ci; /* pi.util when there's a conflict */ > > > > > > Looking ahead to patch 10, this seems more like "pi.util unless we know > > > for sure that there's no conflict". > > > > That's too long for the line to remain at 80 characters; it's 16 > > characters over the limit. ;-) > > Well, you could move the description onto its own line :-) :-)
next prev parent reply other threads:[~2020-11-09 23:09 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 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 [this message] 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 09/20] merge-ort: record stage and auxiliary info for every path 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 List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CABPp-BE=4uTRyxsuBUz7iB7+kqKjtM1-J3qiSi4q1nvwC1Odvg@mail.gmail.com' \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH v2 09/20] merge-ort: record stage and auxiliary info for every path' \ /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
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git 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 read-only IMAP folder(s) and NNTP newsgroup(s).