Re: [PATCH 07/11] merge-ort: add implementation of both sides renaming differently
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Dec 11, 2020, 03:39 UTC
- Message-ID
- <5b9f5db0-b4d5-f60d-b5c5-2f8376d4bf12@gmail.com>
- In-Reply-To
- <d4595397052568ea6ea0cf46190374c74140fed7.1607542887.git.gitgitgadget@gmail.com>
On 12/9/2020 2:41 PM, Elijah Newren via GitGitGadget wrote:
Show 19 quoted lines
> From: Elijah Newren <newren@gmail.com> > > Implement rename/rename(1to2) handling, i.e. both sides of history > renaming a file and rename it differently. This code replaces the > following from merge-recurisve.c: > > * all the 1to2 code in process_renames() > * the RENAME_ONE_FILE_TO_TWO case of process_entry() > * handle_rename_rename_1to2() > > Also, there is some shared code from merge-recursive.c for multiple > different rename cases which we will no longer need for this case (or > other rename cases): > > * handle_file_collision() > * setup_rename_conflict_info() > > The consolidation of five separate codepaths into one is made possible > by a change in design:
Excellent!
Show 10 quoted lines
> /* This is a rename/rename(1to2) */
> - die("Not yet implemented");
> + clean_merge = handle_content_merge(opt,
> + pair->one->path,
> + &base->stages[0],
> + &side1->stages[1],
> + &side2->stages[2],
> + pathnames,
> + 1 + 2 * opt->priv->call_depth,
> + &merged);(this method currently die()s. ok)
Show 5 quoted lines
> + if (!clean_merge &&
> + merged.mode == side1->stages[1].mode &&
> + oideq(&merged.oid, &side1->stages[1].oid)) {
> + was_binary_blob = 1;
> + }nit: Extraneous braces?
Show 34 quoted lines
> + memcpy(&side1->stages[1], &merged, sizeof(merged));
> + if (was_binary_blob) {
> + /*
> + * Getting here means we were attempting to
> + * merge a binary blob.
> + *
> + * Since we can't merge binaries,
> + * handle_content_merge() just takes one
> + * side. But we don't want to copy the
> + * contents of one side to both paths. We
> + * used the contents of side1 above for
> + * side1->stages, let's use the contents of
> + * side2 for side2->stages below.
> + */
> + oidcpy(&merged.oid, &side2->stages[2].oid);
> + merged.mode = side2->stages[2].mode;
> + }
> + memcpy(&side2->stages[2], &merged, sizeof(merged));
> +
> + side1->path_conflict = 1;
> + side2->path_conflict = 1;
> + /*
> + * TODO: For renames we normally remove the path at the
> + * old name. It would thus seem consistent to do the
> + * same for rename/rename(1to2) cases, but we haven't
> + * done so traditionally and a number of the regression
> + * tests now encode an expectation that the file is
> + * left there at stage 1. If we ever decide to change
> + * this, add the following two lines here:
> + * base->merged.is_null = 1;
> + * base->merged.clean = 1;
> + * and remove the setting of base->path_conflict to 1.
> + */
> + base->path_conflict = 1;I'm getting the point of the review/evening where I'm starting to gloss over these important details. Time to take a break (after this patch).
Show 6 quoted lines
> + path_msg(opt, oldpath, 0,
> + _("CONFLICT (rename/rename): %s renamed to "
> + "%s in %s and to %s in %s."),
> + pathnames[0],
> + pathnames[1], opt->branch1,
> + pathnames[2], opt->branch2);This output differs a bit from handle_rename_rename_1to2() in merge-recursive.c:
output(opt, 1, _("CONFLICT (rename/rename): "
"Rename \"%s\"->\"%s\" in branch \"%s\" "
"rename \"%s\"->\"%s\" in \"%s\"%s"),
o->path, a->path, ci->ren1->branch,
o->path, b->path, ci->ren2->branch,
opt->priv->call_depth ? _(" (left unresolved)") : "");How much do we want to have _exact_ output matches between the two strategies, at least in the short term?
Show 13 quoted lines
> @@ -1257,13 +1309,13 @@ static void process_entry(struct merge_options *opt,
> int side = (ci->filemask == 4) ? 2 : 1;
> ci->merged.result.mode = ci->stages[side].mode;
> oidcpy(&ci->merged.result.oid, &ci->stages[side].oid);
> - ci->merged.clean = !ci->df_conflict;
> + ci->merged.clean = !ci->df_conflict && !ci->path_conflict;
> } else if (ci->filemask == 1) {
> /* Deleted on both sides */
> ci->merged.is_null = 1;
> ci->merged.result.mode = 0;
> oidcpy(&ci->merged.result.oid, &null_oid);
> - ci->merged.clean = 1;
> + ci->merged.clean = !ci->path_conflict;These exist because this is the first time we assign path_conflict. Sure.
Thanks, -Stolee