| Message ID | 20200625121953.16991-17-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rewrite the remaining merge strategies from shell to C | expand |
Hi Alban On 25/06/2020 13:19, Alban Gruin wrote: > This teaches the sequencer to invoke the "resolve" strategy with a > function call instead of forking. This is a good idea, however we should check the existing tests that use this strategy to see if they are doing so to test the try_merge_command() code path. I've got some patches in seen that use '--strategy=resolve' to exercise the "non merge-recursive" code path, so I'll update them to use a proper custom merge strategy. Is it worth optimizing do_merge() to take advantage of resolve and octopus being builtin as well? Best Wishes Phil > Signed-off-by: Alban Gruin <alban.gruin@gmail.com> > --- > sequencer.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index fd7701c88a..ea8dc58108 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -33,6 +33,7 @@ > #include "commit-reach.h" > #include "rebase-interactive.h" > #include "reset.h" > +#include "merge-strategies.h" > > #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" > > @@ -1922,9 +1923,15 @@ static int do_pick_commit(struct repository *r, > > commit_list_insert(base, &common); > commit_list_insert(next, &remotes); > - res |= try_merge_command(r, opts->strategy, > - opts->xopts_nr, (const char **)opts->xopts, > - common, oid_to_hex(&head), remotes); > + > + if (!strcmp(opts->strategy, "resolve")) { > + repo_read_index(r); > + res |= merge_strategies_resolve(r, common, oid_to_hex(&head), remotes); > + } else > + res |= try_merge_command(r, opts->strategy, > + opts->xopts_nr, (const char **)opts->xopts, > + common, oid_to_hex(&head), remotes); > + > free_commit_list(common); > free_commit_list(remotes); > } >
Hi Phillip, Phillip Wood (phillip.wood123@gmail.com) a écrit : > Hi Alban > > On 25/06/2020 13:19, Alban Gruin wrote: > > This teaches the sequencer to invoke the "resolve" strategy with a > > function call instead of forking. > > This is a good idea, however we should check the existing tests that use this > strategy to see if they are doing so to test the try_merge_command() code > path. I've got some patches in seen that use '--strategy=resolve' to exercise > the "non merge-recursive" code path, so I'll update them to use a proper > custom merge strategy. > > Is it worth optimizing do_merge() to take advantage of resolve and octopus > being builtin as well? > Hmm, I see that do_merge() doesn't call directly the strategies, and delegates this work to git-merge. If calling the new APIs does not imply to copy/paste too much code from merge.c, then my answer is yes. > Best Wishes > > Phil > Cheers, Alban
diff --git a/sequencer.c b/sequencer.c index fd7701c88a..ea8dc58108 100644 --- a/sequencer.c +++ b/sequencer.c @@ -33,6 +33,7 @@ #include "commit-reach.h" #include "rebase-interactive.h" #include "reset.h" +#include "merge-strategies.h" #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" @@ -1922,9 +1923,15 @@ static int do_pick_commit(struct repository *r, commit_list_insert(base, &common); commit_list_insert(next, &remotes); - res |= try_merge_command(r, opts->strategy, - opts->xopts_nr, (const char **)opts->xopts, - common, oid_to_hex(&head), remotes); + + if (!strcmp(opts->strategy, "resolve")) { + repo_read_index(r); + res |= merge_strategies_resolve(r, common, oid_to_hex(&head), remotes); + } else + res |= try_merge_command(r, opts->strategy, + opts->xopts_nr, (const char **)opts->xopts, + common, oid_to_hex(&head), remotes); + free_commit_list(common); free_commit_list(remotes); }
This teaches the sequencer to invoke the "resolve" strategy with a function call instead of forking. Signed-off-by: Alban Gruin <alban.gruin@gmail.com> --- sequencer.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)