Re: [PATCH] Split out merge_recursive() to merge-recursive.c

[Stephan Beyer <s-beyer@gmx.net>]

Hi,

Miklos Vajna wrote:
> Move most of the of code from builtin-merge-recursive.c to a new file
> merge-recursive.c and introduce merge_recursive_setup() in there so that
> builtin-merge-recursive and other builtins call it.

So, according to that change my "revert" patch is the same plus this
interdiff:

--8<--
diff --git a/builtin-revert.c b/builtin-revert.c
index dcee181..941b875 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -281,7 +281,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	const char *message, *encoding;
 	const char *defmsg = xstrdup(git_path("MERGE_MSG"));
 
-	git_config(git_default_config, NULL);
+	git_config(merge_recursive_config, NULL);
+	merge_recursive_setup(0);
 	me = action == REVERT ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
 	parse_args(argc, argv);
-->8--

I'm fine with that, but my hope(?) was that we could have some more
generic function that takes SHAs ("unsigned char *") instead of commits.
I don't know if this is bad for builtin-merge, but from the "revert" and
"sequencer" point of view this is all I need.

Hmm, I think it takes less time to implement it based on your patch than
explaining :-)

One further comment:
> -static int merge_config(const char *var, const char *value, void *cb)
> +int merge_recursive_config(const char *var, const char *value, void *cb)
>  {
>  	if (!strcasecmp(var, "merge.verbosity")) {
> -		verbosity = git_config_int(var, value);
> +		merge_recursive_verbosity = git_config_int(var, value);
[...]
> diff --git a/merge-recursive.h b/merge-recursive.h
> index f37630a..73e4413 100644
> --- a/merge-recursive.h
> +++ b/merge-recursive.h
> @@ -14,7 +14,11 @@ int merge_trees(struct tree *head,
>  		const char *branch1,
>  		const char *branch2,
>  		struct tree **result);
> -
> +struct commit *make_virtual_commit(struct tree *tree, const char *comment);
> +int merge_recursive_config(const char *var, const char *value, void *cb);
> +void merge_recursive_setup(int is_subtree_merge);
>  struct tree *write_tree_from_memory(void);
>  

> +extern int merge_recursive_verbosity;

Why this?

So we have:
 1. "merge.verbosity" config value
 2. GIT_MERGE_VERBOSITY environment
 3. merge_recursive_verbosity variable

I wonder if 3 is really necessary.

Kind regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F