Re: [PATCH] builtin-revert.c: Make use of merge_recursive()
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 11, 2008, 15:47 UTC
- Message-ID
- <alpine.DEB.1.00.0808111737270.24820@pacific.mpi-cbg.de.mpi-cbg.de>
- In-Reply-To
- <1218467003-14591-1-git-send-email-s-beyer@gmx.net>
Hi,
On Mon, 11 Aug 2008, Stephan Beyer wrote:
Show 10 quoted lines
> 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.
Show 26 quoted lines
> 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