| Message ID | 20190726155258.28561-7-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup merge API | expand |
Elijah Newren <newren@gmail.com> writes: > Alternatively, you can view this as "make the merge functions behave > more similarly." merge-recursive has three different entry points: > merge_trees(), merge_recursive(), and merge_recursive_generic(). Two of > these would call diff_warn_rename_limit(), but merge_trees() didn't. > This lead to callers of merge_trees() needing to manually call > diff_warn_rename_limit() themselves. Move this to the new > merge_finalize() function to make sure that all three entry points run > this function. Interesting. It seems that b520abf1c8f did a suboptimal jobs but this step cleans it up quite nicely. Are there callers of merge_trees() in other codepaths, and do they want this change? There is only one, builtin/checkout.c::merge_working_tree(), which is used when switching to a different commit. I think it would not hurt to give the same warning (but I also think it would not hurt to simply disable rename detection in that context); we should record the whatever decision we make in the log message. > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > merge-recursive.c | 7 +++---- > sequencer.c | 1 - > 2 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/merge-recursive.c b/merge-recursive.c > index 4a481c3929..308e350ff1 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -3583,9 +3583,6 @@ static int merge_recursive_internal(struct merge_options *opt, > flush_output(opt); > if (!opt->call_depth && opt->buffer_output < 2) > strbuf_release(&opt->obuf); > - if (show(opt, 2)) > - diff_warn_rename_limit("merge.renamelimit", > - opt->needed_rename_limit, 0); > return clean; > } > > @@ -3607,7 +3604,9 @@ static int merge_start(struct merge_options *opt, struct tree *head) > > static void merge_finalize(struct merge_options *opt) > { > - /* Common code for wrapping up merges will be added here later */ > + if (show(opt, 2)) > + diff_warn_rename_limit("merge.renamelimit", > + opt->needed_rename_limit, 0); > } > > int merge_trees(struct merge_options *opt, > diff --git a/sequencer.c b/sequencer.c > index c4ed30f1b4..094a4dd03d 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -617,7 +617,6 @@ static int do_recursive_merge(struct repository *r, > if (is_rebase_i(opts) && clean <= 0) > fputs(o.obuf.buf, stdout); > strbuf_release(&o.obuf); > - diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0); > if (clean < 0) { > rollback_lock_file(&index_lock); > return clean;
On Fri, Jul 26, 2019 at 12:57 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > Alternatively, you can view this as "make the merge functions behave > > more similarly." merge-recursive has three different entry points: > > merge_trees(), merge_recursive(), and merge_recursive_generic(). Two of > > these would call diff_warn_rename_limit(), but merge_trees() didn't. > > This lead to callers of merge_trees() needing to manually call > > diff_warn_rename_limit() themselves. Move this to the new > > merge_finalize() function to make sure that all three entry points run > > this function. > > Interesting. It seems that b520abf1c8f did a suboptimal jobs but > this step cleans it up quite nicely. > > Are there callers of merge_trees() in other codepaths, and do they > want this change? > > There is only one, builtin/checkout.c::merge_working_tree(), which > is used when switching to a different commit. I think it would not > hurt to give the same warning (but I also think it would not hurt to > simply disable rename detection in that context); we should record > the whatever decision we make in the log message. I was surprised to read this because I thought I did that...but I guess that was in patch 15 for a slightly different case. Yeah, I can add some words to the commit message about this.
diff --git a/merge-recursive.c b/merge-recursive.c index 4a481c3929..308e350ff1 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3583,9 +3583,6 @@ static int merge_recursive_internal(struct merge_options *opt, flush_output(opt); if (!opt->call_depth && opt->buffer_output < 2) strbuf_release(&opt->obuf); - if (show(opt, 2)) - diff_warn_rename_limit("merge.renamelimit", - opt->needed_rename_limit, 0); return clean; } @@ -3607,7 +3604,9 @@ static int merge_start(struct merge_options *opt, struct tree *head) static void merge_finalize(struct merge_options *opt) { - /* Common code for wrapping up merges will be added here later */ + if (show(opt, 2)) + diff_warn_rename_limit("merge.renamelimit", + opt->needed_rename_limit, 0); } int merge_trees(struct merge_options *opt, diff --git a/sequencer.c b/sequencer.c index c4ed30f1b4..094a4dd03d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -617,7 +617,6 @@ static int do_recursive_merge(struct repository *r, if (is_rebase_i(opts) && clean <= 0) fputs(o.obuf.buf, stdout); strbuf_release(&o.obuf); - diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0); if (clean < 0) { rollback_lock_file(&index_lock); return clean;
Alternatively, you can view this as "make the merge functions behave more similarly." merge-recursive has three different entry points: merge_trees(), merge_recursive(), and merge_recursive_generic(). Two of these would call diff_warn_rename_limit(), but merge_trees() didn't. This lead to callers of merge_trees() needing to manually call diff_warn_rename_limit() themselves. Move this to the new merge_finalize() function to make sure that all three entry points run this function. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-recursive.c | 7 +++---- sequencer.c | 1 - 2 files changed, 3 insertions(+), 5 deletions(-)