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

[Junio C Hamano <gitster@pobox.com>]

Elijah Newren <newren@gmail.com> writes:

> 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?

> +	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.

>  	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.