Hi,
On Mon, 11 Aug 2008, Stephan Beyer wrote:
> diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
> index 09aa830..d8bd21f 100644
> --- a/builtin-merge-recursive.c
> +++ b/builtin-merge-recursive.c
> @@ -1327,7 +1327,7 @@ static const char *better_branch_name(const char *branch)
> return name ? name : branch;
> }
>
> -static struct commit *get_ref(const char *ref)
> +struct commit *get_ref(const char *ref)
The name get_ref() is way too generic to be non-static. But I have a
hunch that peel_to_type() does a lot of what we want here, if not all of
it.
> diff --git a/builtin-revert.c b/builtin-revert.c
> index 27881e9..c54cf8a 100644
> --- a/builtin-revert.c
> +++ b/builtin-revert.c
> @@ -219,15 +216,21 @@ static int merge_recursive(const char *base_sha1,
> * and $prev on top of us (when reverting), or the change between
> * $prev and $commit on top of us (when cherry-picking or replaying).
> */
> - argv[i++] = "merge-recursive";
> - if (base_sha1)
> - argv[i++] = base_sha1;
> - argv[i++] = "--";
> - argv[i++] = head_sha1;
> - argv[i++] = next_sha1;
> - argv[i++] = NULL;
> -
> - return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD);
> + if (base_sha1) {
> + struct commit *base = get_ref(base_sha1);
> + commit_list_insert(base, &ca);
> + }
> + h1 = get_ref(head_sha1);
> + h2 = get_ref(next_sha1);
> +
> + index_fd = hold_locked_index(lock, 1);
> + clean = merge_recursive(h1, h2, head_name, next_name, ca, &result);
h1 and h2 are not expressive. head_commit and next_commit would be.
Rest looks good to me -- even if I had to spend too much time (therefore
being not really thorough in the end) verifying that merge_recursive()
does not lock the index itself, and that GITHEAD_* definitions are not
necessary anymore, since merge_recursive() takes the arguments directly;
you might want to make it easier for this reviewer in the future, if you
want this reviewer to review your patches, that is.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
