| Message ID | 20201102204344.342633-4-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fundamentals of merge-ort implementation | expand |
On 11/2/2020 3:43 PM, Elijah Newren wrote: > merge_start() basically does a bunch of sanity checks, then allocates > and initializes opt->priv -- a struct merge_options_internal. > > Most the sanity checks are usable as-is. The allocation/intialization s/Most the/Most of the/ > The weirdest part here is that merge-ort and merge-recursive use the > same struct merge_options, even though merge_options has a number of > fields that are oddly specific to merge-recursive's internal > implementation and don't even make sense with merge-ort's high-level > design (e.g. buffer_output, which merge-ort has to always do). I reused > the same data structure because: > * most the fields made sense to both merge algorithms > * making a new struct would have required making new enums or somehow > externalizing them, and that was getting messy. > * it simplifies converting the existing callers by not having to > have different code paths for merge_options setup. I think this is appropriate. The other option would be to split the struct into "common options" and "specific options" but that starts to get messy if we add yet another merge strategy that changes what should be "common". Hopefully we can group options within the struct merge_options definition to assist with this? For now, the assertions are a good approach. > I also marked detect_renames as ignored. We can revisit that later, but > in short: merge-recursive allowed turning off rename detection because > it was sometimes glacially slow. When you speed something up by a few > orders of magnitude, it's worth revisiting whether that justification is > still relevant. Besides, if folks find it's still too slow, perhaps > they have a better scaling case than I could find and maybe it turns up > some more optimizations we can add. If it still is needed as an option, > it is easy to add later. As long as it is easy to add later, I don't see much of a problem. Usually adding a knob to disable a feature is necessary to mitigate risk, and here we can simply adjust config to use the non-ort algorithm if we notice a data shape where rename detection makes the algorithm slow/unusable. > static void merge_start(struct merge_options *opt, struct merge_result *result) > { > - die("Not yet implemented."); > + /* Sanity checks on opt */ > + assert(opt->repo); > + > + assert(opt->branch1 && opt->branch2); > + > + assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE && > + opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE); > + assert(opt->rename_limit >= -1); > + assert(opt->rename_score >= 0 && opt->rename_score <= MAX_SCORE); > + assert(opt->show_rename_progress >= 0 && opt->show_rename_progress <= 1); > + > + assert(opt->xdl_opts >= 0); > + assert(opt->recursive_variant >= MERGE_VARIANT_NORMAL && > + opt->recursive_variant <= MERGE_VARIANT_THEIRS); > + > + /* > + * detect_renames, verbosity, buffer_output, and obuf are ignored > + * fields that were used by "recursive" rather than "ort" -- but > + * sanity check them anyway. > + */ > + assert(opt->detect_renames >= -1 && > + opt->detect_renames <= DIFF_DETECT_COPY); > + assert(opt->verbosity >= 0 && opt->verbosity <= 5); > + assert(opt->buffer_output <= 2); > + assert(opt->obuf.len == 0); > + > + assert(opt->priv == NULL); > + > + /* Initialization of opt->priv, our internal merge data */ > + opt->priv = xcalloc(1, sizeof(*opt->priv)); nit: I would insert an empty line between this code and the multi-line comment below. > + /* > + * Although we initialize opt->priv->paths with strdup_strings=0, > + * that's just to avoid making yet another copy of an allocated > + * string. Putting the entry into paths means we are taking > + * ownership, so we will later free it. > + * > + * In contrast, unmerged just has a subset of keys from paths, so > + * we don't want to free those (it'd be a duplicate free). > + */ > + strmap_init_with_options(&opt->priv->paths, NULL, 0); > + strmap_init_with_options(&opt->priv->unmerged, NULL, 0); > } This approach looks fine to me. Thanks, -Stolee
Thanks for all the reviews and suggestions! I'll avoid commenting on the simple fixes that I'm just going to apply, and instead concentrate on the bigger questions you have in my reply. On Wed, Nov 11, 2020 at 5:53 AM Derrick Stolee <stolee@gmail.com> wrote: > > On 11/2/2020 3:43 PM, Elijah Newren wrote: > > The weirdest part here is that merge-ort and merge-recursive use the > > same struct merge_options, even though merge_options has a number of > > fields that are oddly specific to merge-recursive's internal > > implementation and don't even make sense with merge-ort's high-level > > design (e.g. buffer_output, which merge-ort has to always do). I reused > > the same data structure because: > > * most the fields made sense to both merge algorithms > > * making a new struct would have required making new enums or somehow > > externalizing them, and that was getting messy. > > * it simplifies converting the existing callers by not having to > > have different code paths for merge_options setup. > > I think this is appropriate. The other option would be to split the > struct into "common options" and "specific options" but that starts > to get messy if we add yet another merge strategy that changes what > should be "common". Hopefully we can group options within the struct > merge_options definition to assist with this? I think we should plan on merge-recursive.[ch] being deleted before a third merge strategy comes along, so the common options might make sense. But then again, it sounds like work towards simultaneously supporting two backends in perpetuity, which isn't at all the current plan[1]. As far as grouping and other cleanups, see the series included with merge commit 280bd44551 ("Merge branch 'en/merge-recursive-cleanup'", 2019-10-15), particularly commits ff1bfa2cd5 ("merge-recursive: use common name for ancestors/common/base_list", 2019-08-17), a779fb829b ("merge-recursive: comment and reorder the merge_options fields", 2019-08-17), and 8599ab4574 ("merge-recursive: consolidate unnecessary fields in merge_options", 2019-08-17). I guess we could change from grouping by option similarity and instead group by which are in use by which merge backends, but since the plan is to [eventually] kill merge-recursive and then to just drop the unused or ignored fields, I think helping users understand the purpose of the options (which grouping-by-similarity aids with) is more important than grouping for the purpose of reminding myself which ones to remove later. [1] https://lore.kernel.org/git/xmqqk1ydkbx0.fsf@gitster.mtv.corp.google.com/ > For now, the assertions are a good approach. > > > I also marked detect_renames as ignored. We can revisit that later, but > > in short: merge-recursive allowed turning off rename detection because > > it was sometimes glacially slow. When you speed something up by a few > > orders of magnitude, it's worth revisiting whether that justification is > > still relevant. Besides, if folks find it's still too slow, perhaps > > they have a better scaling case than I could find and maybe it turns up > > some more optimizations we can add. If it still is needed as an option, > > it is easy to add later. > > As long as it is easy to add later, I don't see much of a problem. Usually > adding a knob to disable a feature is necessary to mitigate risk, and here > we can simply adjust config to use the non-ort algorithm if we notice a data > shape where rename detection makes the algorithm slow/unusable. Yes, it should be pretty easy to add later. > > static void merge_start(struct merge_options *opt, struct merge_result *result) > > { > > - die("Not yet implemented."); > > + /* Sanity checks on opt */ > > + assert(opt->repo); > > + > > + assert(opt->branch1 && opt->branch2); > > + > > + assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE && > > + opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE); > > + assert(opt->rename_limit >= -1); > > + assert(opt->rename_score >= 0 && opt->rename_score <= MAX_SCORE); > > + assert(opt->show_rename_progress >= 0 && opt->show_rename_progress <= 1); > > + > > + assert(opt->xdl_opts >= 0); > > + assert(opt->recursive_variant >= MERGE_VARIANT_NORMAL && > > + opt->recursive_variant <= MERGE_VARIANT_THEIRS); > > + > > + /* > > + * detect_renames, verbosity, buffer_output, and obuf are ignored > > + * fields that were used by "recursive" rather than "ort" -- but > > + * sanity check them anyway. > > + */ > > + assert(opt->detect_renames >= -1 && > > + opt->detect_renames <= DIFF_DETECT_COPY); > > + assert(opt->verbosity >= 0 && opt->verbosity <= 5); > > + assert(opt->buffer_output <= 2); > > + assert(opt->obuf.len == 0); > > + > > + assert(opt->priv == NULL); > > + > > + /* Initialization of opt->priv, our internal merge data */ > > + opt->priv = xcalloc(1, sizeof(*opt->priv)); > > nit: I would insert an empty line between this code and the > multi-line comment below. > > > + /* > > + * Although we initialize opt->priv->paths with strdup_strings=0, > > + * that's just to avoid making yet another copy of an allocated > > + * string. Putting the entry into paths means we are taking > > + * ownership, so we will later free it. > > + * > > + * In contrast, unmerged just has a subset of keys from paths, so > > + * we don't want to free those (it'd be a duplicate free). > > + */ > > + strmap_init_with_options(&opt->priv->paths, NULL, 0); > > + strmap_init_with_options(&opt->priv->unmerged, NULL, 0); > > } > > This approach looks fine to me. > > Thanks, > -Stolee
diff --git a/merge-ort.c b/merge-ort.c index b53cd80104..f5460a8a52 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -17,6 +17,8 @@ #include "cache.h" #include "merge-ort.h" +#include "diff.h" +#include "diffcore.h" #include "strmap.h" #include "tree.h" @@ -107,7 +109,47 @@ void merge_finalize(struct merge_options *opt, static void merge_start(struct merge_options *opt, struct merge_result *result) { - die("Not yet implemented."); + /* Sanity checks on opt */ + assert(opt->repo); + + assert(opt->branch1 && opt->branch2); + + assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE && + opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE); + assert(opt->rename_limit >= -1); + assert(opt->rename_score >= 0 && opt->rename_score <= MAX_SCORE); + assert(opt->show_rename_progress >= 0 && opt->show_rename_progress <= 1); + + assert(opt->xdl_opts >= 0); + assert(opt->recursive_variant >= MERGE_VARIANT_NORMAL && + opt->recursive_variant <= MERGE_VARIANT_THEIRS); + + /* + * detect_renames, verbosity, buffer_output, and obuf are ignored + * fields that were used by "recursive" rather than "ort" -- but + * sanity check them anyway. + */ + assert(opt->detect_renames >= -1 && + opt->detect_renames <= DIFF_DETECT_COPY); + assert(opt->verbosity >= 0 && opt->verbosity <= 5); + assert(opt->buffer_output <= 2); + assert(opt->obuf.len == 0); + + assert(opt->priv == NULL); + + /* Initialization of opt->priv, our internal merge data */ + opt->priv = xcalloc(1, sizeof(*opt->priv)); + /* + * Although we initialize opt->priv->paths with strdup_strings=0, + * that's just to avoid making yet another copy of an allocated + * string. Putting the entry into paths means we are taking + * ownership, so we will later free it. + * + * In contrast, unmerged just has a subset of keys from paths, so + * we don't want to free those (it'd be a duplicate free). + */ + strmap_init_with_options(&opt->priv->paths, NULL, 0); + strmap_init_with_options(&opt->priv->unmerged, NULL, 0); } /*
merge_start() basically does a bunch of sanity checks, then allocates and initializes opt->priv -- a struct merge_options_internal. Most the sanity checks are usable as-is. The allocation/intialization is a bit different since merge-ort has a very different merge_options_internal than merge-recursive, but the idea is the same. The weirdest part here is that merge-ort and merge-recursive use the same struct merge_options, even though merge_options has a number of fields that are oddly specific to merge-recursive's internal implementation and don't even make sense with merge-ort's high-level design (e.g. buffer_output, which merge-ort has to always do). I reused the same data structure because: * most the fields made sense to both merge algorithms * making a new struct would have required making new enums or somehow externalizing them, and that was getting messy. * it simplifies converting the existing callers by not having to have different code paths for merge_options setup. I also marked detect_renames as ignored. We can revisit that later, but in short: merge-recursive allowed turning off rename detection because it was sometimes glacially slow. When you speed something up by a few orders of magnitude, it's worth revisiting whether that justification is still relevant. Besides, if folks find it's still too slow, perhaps they have a better scaling case than I could find and maybe it turns up some more optimizations we can add. If it still is needed as an option, it is easy to add later. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-ort.c | 44 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-)