Hi Elijah,
On Thu, 25 Jul 2019, Elijah Newren wrote:
> We had a rule to enforce that the index matches head, but it was found
> at the beginning of merge_trees() and would only trigger when
> opt->call_depth was 0. Since merge_recursive() doesn't call
> merge_trees() until after returning from recursing, this meant that the
> check wasn't triggered by merge_recursive() until it had first finished
> all the intermediate merges to create virtual merge bases. That is a
> potentially huge amount of computation (and writing of intermediate
> merge results into the .git/objects directory) before it errors out and
> says, in effect, "Sorry, I can't do any merging because you have some
> local changes that would be overwritten."
>
> Further, not enforcing this requirement earlier allowed other bugs (such
> as an unintentional unconditional dropping and reloading of the index in
> merge_recursive() even when no recursion was necessary), to mask bugs in
> other callers (which were fixed in the commit prior to this one).
>
> Make sure we do the index == head check at the beginning of the merge,
> and error out immediately if it fails.
Very clear commit message.
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 37bb94fb4d..b762ecd7bd 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -3381,21 +3381,14 @@ static int process_entry(struct merge_options *opt,
> return clean_merge;
> }
>
> -int merge_trees(struct merge_options *opt,
> - struct tree *head,
> - struct tree *merge,
> - struct tree *common,
> - struct tree **result)
> +static int merge_trees_internal(struct merge_options *opt,
In other, similar cases, we seem to use the suffix `_1`. Not sure
whether you want to change that here.
> + struct tree *head,
> + struct tree *merge,
> + struct tree *common,
> + struct tree **result)
> {
> struct index_state *istate = opt->repo->index;
> int code, clean;
> - struct strbuf sb = STRBUF_INIT;
> -
> - if (!opt->call_depth && repo_index_has_changes(opt->repo, head, &sb)) {
> - err(opt, _("Your local changes to the following files would be overwritten by merge:\n %s"),
> - sb.buf);
> - return -1;
> - }
>
> if (opt->subtree_shift) {
> merge = shift_tree_object(opt->repo, head, merge, opt->subtree_shift);
> @@ -3499,11 +3492,11 @@ static struct commit_list *reverse_commit_list(struct commit_list *list)
> * Merge the commits h1 and h2, return the resulting virtual
> * commit object and a flag indicating the cleanness of the merge.
> */
> -int merge_recursive(struct merge_options *opt,
> - struct commit *h1,
> - struct commit *h2,
> - struct commit_list *ca,
> - struct commit **result)
> +static int merge_recursive_internal(struct merge_options *opt,
Same here.
> + struct commit *h1,
> + struct commit *h2,
> + struct commit_list *ca,
> + struct commit **result)
> {
> struct commit_list *iter;
> struct commit *merged_common_ancestors;
> [...]
> @@ -3596,6 +3589,58 @@ int merge_recursive(struct merge_options *opt,
> return clean;
> }
>
> +static int merge_start(struct merge_options *opt, struct tree *head)
I would prefer to call this something like `check_invariants()` or
something similar.
> +{
> + struct strbuf sb = STRBUF_INIT;
> +
> + assert(opt->branch1 && opt->branch2);
> +
> + if (repo_index_has_changes(opt->repo, head, &sb)) {
> + err(opt, _("Your local changes to the following files would be overwritten by merge:\n %s"),
> + sb.buf);
I know that you did not introduce this leak, but maybe we could slip an
`strbuf_release(&sb);` in at this point?
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static void merge_finalize(struct merge_options *opt)
> +{
> + /* Common code for wrapping up merges will be added here later */
> +}
I was about to comment that this complicates this here diff and that we
should do this when we need it, but I just peeked into the next patch,
and it uses it, so I leave this here paragraph only to show that I
actually reviewed this part, too.
And of course, if we have a `merge_finalize()`, then a `merge_start()`
does not sound all that bad, either.
The rest looks good to me.
Thanks,
Dscho
> +
> +int merge_trees(struct merge_options *opt,
> + struct tree *head,
> + struct tree *merge,
> + struct tree *common,
> + struct tree **result)
> +{
> + int ret;
> +
> + if (merge_start(opt, head))
> + return -1;
> + ret = merge_trees_internal(opt, head, merge, common, result);
> + merge_finalize(opt);
> +
> + return ret;
> +}
> +
> +int merge_recursive(struct merge_options *opt,
> + struct commit *h1,
> + struct commit *h2,
> + struct commit_list *ca,
> + struct commit **result)
> +{
> + int ret;
> +
> + if (merge_start(opt, repo_get_commit_tree(opt->repo, h1)))
> + return -1;
> + ret = merge_recursive_internal(opt, h1, h2, ca, result);
> + merge_finalize(opt);
> +
> + return ret;
> +}
> +
> static struct commit *get_ref(struct repository *repo, const struct object_id *oid,
> const char *name)
> {
> --
> 2.22.0.559.g28a8880890.dirty
>
>
