Re: [BUG] All files in folder are moved when cherry-picking commit that moves fewer files

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 6, 2019, 04:43 UTC
Message-ID: <xmqqh8cgr4tf.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190306002744.14418-1-newren@gmail.com>

Elijah Newren <newren@gmail.com> writes:

Show 10 quoted lines

> Note that there is also a third possibility here:
>
>   C) There are different answers depending on the context and content
>      that cannot be determined by git, so this is a conflict.  Use a
>      higher stage in the index to record the conflict and notify the
>      user of the potential issue instead of silently selecting a
>      resolution for them.
>
> Add an option for users to specify their preference for whether to use
> directory rename detection, and default to (C).

Yeah, (c) is the sanest default that can give the best of both worlds, I would have to say.

Sometimes the heuristics guess that the new one may want to go to the same place as all the neighbours, and other times the new one may want to stay. By using stage #2 and stage #3 appropriately, the user can see where the final result wants to be in. Because by definition there won't be any auto-resolvable content-level merge for such paths (after all, one side is adding it as a new file), I think the contents for these two higher-stage entries would actually be the same?

Show 7 quoted lines

> +	if (mark_conflicted)
> +		output(o, 1, _("CONFLICT (directory possibly renamed): %s "
> +			       "added in %s, but that directory was renamed "
> +			       "in %s suggesting it should perhaps be moved "
> +			       "to %s."),
> +		       pair->one->path, other_branch, rename_branch,
> +		       dest->path);

What confused the end user is that it is not always the case, and that is because the only thing we know at this point is that the other branch moved all the files in that directory in their branch to the other place, not that they "moved the directory".

In other words, if "that directory was renamed" in this message were definitive and agreed with end-user perception, we won't be issuing this conflict with a label that says "possibly" renamed.

Perhaps something along this line?

	%(pair->one->path)s added in %(other_branch)s, but the other
	branch %(rename_branch)s moved all the files in the same
	directory to %(dirname(dest->path))s, so this file may also
	want to move there, too.

Show 27 quoted lines

>  	if (!o->call_depth && would_lose_untracked(o, dest->path)) {
> -		char *alt_path = unique_path(o, dest->path, rename_branch);
> -
> +		mark_conflicted = 1;
> +		file_path = unique_path(o, dest->path, rename_branch);
>  		output(o, 1, _("Error: Refusing to lose untracked file at %s; "
>  			       "writing to %s instead."),
> -		       dest->path, alt_path);
> +		       dest->path, file_path);
> +	}
> +
> +	if (mark_conflicted) {
>  		/*
> -		 * Write the file in worktree at alt_path, but not in the
> -		 * index.  Instead, write to dest->path for the index but
> -		 * only at the higher appropriate stage.
> +		 * Write the file in worktree at file_path.  In the index,
> +		 * only record the file at dest->path in the appropriate
> +		 * higher stage.
>  		 */
> -		if (update_file(o, 0, &dest->oid, dest->mode, alt_path))
> +		if (update_file(o, 0, &dest->oid, dest->mode, file_path))
>  			return -1;
> +		if (update_stages(o, dest->path, NULL,
> +				  rename_branch == o->branch1 ? dest : NULL,
> +				  rename_branch == o->branch1 ? NULL : dest))
> +			return -1;

This one does not issue "may want to stay here" comment of its own, so the earlier "CONFLICT (directory possibly renamed)" message would need to cover both of these two higher-stage entries. I think the message mentions the two paths, so it would be good already.

The earlier "CONFLICT (directory possibly renamed)" message may be sufficient to explain the situation, but I wonder if the user would need hints in resolving either of the two possible ways. I do not know if it should be in the same message, or in the manpage update, but first let's make sure what we want the end users to understand. IIUC, the hint for resolving in favor of either would be to do these two commands?

	git checkout --ours $path_the_user_wants_to_use
	git rm --cached $the_other_path

If the path the user wants to use matches what the branch being merged has, then replace "--ours" above with "--theirs".

Would that be a good explanation?

Regarding the implementation (not the use of stages, but how "mark_conflicted" variable gets used), I am not sure if I agree that only choice (c) should give the hint. Regardless of the settings, we know both paths this file may want to end up with, and I suspect that the alternative a/b/c you gave are how by default the tool automatically places the result, giving a chance to the end user to correct mistakes. I wonder if it is possible to somehow show "the other possibilty" in the merge log even when choice (a) or (b) are in effect? E.g. "path X/B moved to Y/B because the other side moved X/A to Y/A" when choice (b) is in use, and "path X/B kept there but the other side moved neighbouring A/X to Y/A" when choice (a) is in use.

Another thing that makes me wonder is if this should be treated similar to rerere.autoupdate; even when rerere kicks in to resolve otherwise conflicting tricky merge, without being explicitly allowed with the config, it does not register the resolution to the index at stage #0. Either choices (a) that ignores neighbouring files' movement or (b) that follows the movement lets this tricky heuristics not just suggest the end result, but allows it to register the result at stage #0, which somehow feels too aggressive (which was what triggered my suggestion to go with (c)). With that attitude, we may be able to get rid of choice (a) altogether, which may make things a tad simpler. IOW, we assume the heuristics would suggest the right solution most of the time, just like we assume rerere gives a reasonable resolution most of the time, with a knob to accept the result blindly, together with hints to recover when heuristics makes mistakes.