Re: [PATCH v2 09/11] merge-ort: add implementation of rename/delete conflicts

From: Derrick Stolee <stolee@gmail.com>
Date: Dec 15, 2020, 14:23 UTC
Message-ID: <05f52d13-d9ec-226d-370a-140937b12e7a@gmail.com>
In-Reply-To: <f017534243c967caa0137e6899f4e1c69ff02c2e.1607962900.git.gitgitgadget@gmail.com>

On 12/14/2020 11:21 AM, Elijah Newren via GitGitGadget wrote:

Show 36 quoted lines

> From: Elijah Newren <newren@gmail.com>
> 
> Implement rename/delete conflicts, i.e. one side renames a file and the
> other deletes the file.  This code replaces the following from
> merge-recurisve.c:
> 
>   * the code relevant to RENAME_DELETE in process_renames()
>   * the RENAME_DELETE case of process_entry()
>   * handle_rename_delete()
> 
> 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_change_delete()
>   * setup_rename_conflict_info()
> 
> The consolidation of five separate codepaths into one is made possible
> by a change in design: process_renames() tweaks the conflict_info
> entries within opt->priv->paths such that process_entry() can then
> handle all the non-rename conflict types (directory/file, modify/delete,
> etc.) orthogonally.  This means we're much less likely to miss special
> implementation of some kind of combination of conflict types (see
> commits brought in by 66c62eaec6 ("Merge branch 'en/merge-tests'",
> 2020-11-18), especially commit ef52778708 ("merge tests: expect improved
> directory/file conflict handling in ort", 2020-10-26) for more details).
> That, together with letting worktree/index updating be handled
> orthogonally in the merge_switch_to_result() function, dramatically
> simplifies the code for various special rename cases.
> 
> To be fair, there is a _slight_ tweak to process_entry() here, because
> rename/delete cases will also trigger the modify/delete codepath.
> However, we only want a modify/delete message to be printed for a
> rename/delete conflict if there is a content change in the renamed file
> in addition to the rename.  So process_renames() and process_entry()
> aren't quite fully orthogonal, but they are pretty close.

Thanks for adding this warning about the change to process_entry().

Show 5 quoted lines

> @@ -657,6 +657,7 @@ static int process_renames(struct merge_options *opt,
>  		unsigned int old_sidemask;
>  		int target_index, other_source_index;
>  		int source_deleted, collision, type_changed;
> +		const char *rename_branch = NULL, *delete_branch = NULL;

Ah, here they are!

Show 14 quoted lines

> +		if (source_deleted) {
> +			if (target_index == 1) {
> +				rename_branch = opt->branch1;
> +				delete_branch = opt->branch2;
> +			} else {
> +				rename_branch = opt->branch2;
> +				delete_branch = opt->branch1;
> +			}
>  		}
>  
>  		assert(source_deleted || oldinfo->filemask & old_sidemask);
> @@ -838,13 +847,26 @@ static int process_renames(struct merge_options *opt,
>  				   "to %s in %s, but deleted in %s."),
>  				 oldpath, newpath, rename_branch, delete_branch);

This context line is the previous use of rename_branch and delete_branch. Perhaps the declarations, initialization, and first-use here are worth their own patch?

Show 21 quoted lines

>  		} else {
> +			/*
> +			 * a few different cases...start by copying the
> +			 * existing stage(s) from oldinfo over the newinfo
> +			 * and update the pathname(s).
> +			 */
> +			memcpy(&newinfo->stages[0], &oldinfo->stages[0],
> +			       sizeof(newinfo->stages[0]));
> +			newinfo->filemask |= (1 << MERGE_BASE);
> +			newinfo->pathnames[0] = oldpath;
>  			if (type_changed) {
>  				/* rename vs. typechange */
>  				die("Not yet implemented");
>  			} else if (source_deleted) {
>  				/* rename/delete */
> +				newinfo->path_conflict = 1;
> +				path_msg(opt, newpath, 0,
> +					 _("CONFLICT (rename/delete): %s renamed"
> +					   " to %s in %s, but deleted in %s."),
> +					 oldpath, newpath,
> +					 rename_branch, delete_branch);

Since the primary purpose of rename_branch and delete_branch appears to be for these error messages, then likely the previous error message about a rename/delete should just be promoted into this patch instead of the previous.

In fact, the error messages are the exact same, but with slightly different lines due to wrapping:

			path_msg(opt, newpath, 0,
				 _("CONFLICT (rename/delete): %s renamed "
				   "to %s in %s, but deleted in %s."),
				 oldpath, newpath, rename_branch, delete_branch);

and

				path_msg(opt, newpath, 0,
					 _("CONFLICT (rename/delete): %s renamed"
					   " to %s in %s, but deleted in %s."),
					 oldpath, newpath,
					 rename_branch, delete_branch);

I wonder if there is a way to group these together? Perhaps the nested if/else if/else blocks could store a "conflict state" value that says which CONFLICT message to print after the complicated branching is done.

Alternatively, this message appears to be written in the following case:

	source_deleted && !type_changed

your if/else if/else block could be rearranged as follows:

	if (collision && !source_deleted)
		/* collision: rename/add or rename/rename(2to1) */
	else if (!type_change && source_deleted)
		/* rename/delete or rename/add/delete or rename/rename(2to1)/delete */
	else if (!collision)
		/* a few different cases */

Of course, the thing I am missing is that copy of oldinfo->stages[0] into newinfo->stages[0] along with changes to the filemask and pathnames! That is likely why you need the two different markers, because the cases truly are different in that subtle way.

Show 27 quoted lines

>  				/* normal rename */
>  				die("Not yet implemented");
> @@ -1380,12 +1402,21 @@ static void process_entry(struct merge_options *opt,
>  		modify_branch = (side == 1) ? opt->branch1 : opt->branch2;
>  		delete_branch = (side == 1) ? opt->branch2 : opt->branch1;
>  
> -		path_msg(opt, path, 0,
> -			 _("CONFLICT (modify/delete): %s deleted in %s "
> -			   "and modified in %s.  Version %s of %s left "
> -			   "in tree."),
> -			 path, delete_branch, modify_branch,
> -			 modify_branch, path);
> +		if (ci->path_conflict &&
> +		    oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {
> +			/*
> +			 * This came from a rename/delete; no action to take,
> +			 * but avoid printing "modify/delete" conflict notice
> +			 * since the contents were not modified.
> +			 */
> +		} else {
> +			path_msg(opt, path, 0,
> +				 _("CONFLICT (modify/delete): %s deleted in %s "
> +				   "and modified in %s.  Version %s of %s left "
> +				   "in tree."),
> +				 path, delete_branch, modify_branch,
> +				 modify_branch, path);
> +		}

Thanks, -Stolee