[RFC,v1,16/17] sequencer: use the "resolve" strategy without forking
diff mbox series

Message ID 20200625121953.16991-17-alban.gruin@gmail.com
State New
Headers show
Series
  • Rewrite the remaining merge strategies from shell to C
Related show

Commit Message

Alban Gruin June 25, 2020, 12:19 p.m. UTC
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(-)

Comments

Phillip Wood June 25, 2020, 4:11 p.m. UTC | #1
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);
>   	}
>
Alban Gruin July 12, 2020, 11:27 a.m. UTC | #2
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

Patch
diff mbox series

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);
 	}