Re: Bugs with detection of renames that are also overwrites
On Sat, Feb 13, 2010 at 06:46:55PM -0500, Anders Kaseorg wrote:
Show 10 quoted lines
> One can create a rename that also overwrites an existing file, for example
> with ‘git mv -f’:
>
> $ git init
> $ seq 100 200 > a; seq 300 400 > b; git add a b
> $ git commit -m foo; git tag foo
> $ git mv -f a b
> $ git commit -m bar; git tag bar
>
> Git does not ordinarily detect this as a rename, even with -M.
Right. Git looks at only a subset of the files when looking for renames. For straight renames, the set of possible sources is the list of deleted files, and the possible destinations are the new files.
Finding copies ("-C") is similar, except that we also consider files that were modified but not deleted. And --find-copies-harder will look at _all_ files as sources, not just modified ones.
But what you are asking for is to expand the possible destination list to include files that were modified but are not new. I don't think there is currently a way to do that explicitly.
As you discovered, though, if either the source or destination file has changed significantly, we should break them apart using "-B":
Show 5 quoted lines
> But it can (sometimes*) be forced to detect the rename with -M -B.
>
> $ git diff --stat -M -B foo bar
> a => b | 0
> 1 files changed, 0 insertions(+), 0 deletions(-)
In which case the rename engine sees the deletion and addition separately (they just happen to have the same path name), and therefore the path gets added to the source and destination lists.
Show 14 quoted lines
> However, the resulting patch incorrectly omits the deletion of the
> overwritten file!
>
> $ git diff -M -B foo bar
> diff --git a/a b/b
> similarity index 100%
> rename from a
> rename to b
>
> Therefore, the patch can’t be applied to its own source tree.
>
> $ git checkout foo
> $ git diff -M -B foo bar | git apply
> error: b: already exists in working directory
Hmm. I think this is a conflict between what the user wants to see and what apply wants to see. From the user's perspective, thinking about the diff representing a change, "b" was not really deleted. It was simply overwritten. But from apply's perspective, the diff is a set of instructions, and those instructions do not mention that "b" previously existed and was overwritten. So it is right to be cautious and declare a conflict.
I'm not sure just throwing a "delete" line in there would be the best thing, though. The instructions for apply really need to be "if 'b' has this sha1, then it is safe to delete and rename a into place. Otherwise it is a conflict". And I'm not sure how we would represent that (I guess with a full diff and an "index" line).
> Also, a question: is it possible to get ‘git merge’ to recognize this
> rename?
No, I don't think there is a way currently. You would need to patch git to set opts.break_opt in merge-recursive.c:get_renames, I think.
Show 5 quoted lines
> (* I say “sometimes” because -B only detects the rewrite if it deems the
> renamed file to be sufficiently different than the overwritten file. If
> you use 190 and 390 instead of 200 and 400 above, the rewrite and rename
> will not be detected, even though the rename would be detected if it was
> not an overwrite. This also feels like a bug.)
I think you are getting into a philosophical discussion of what is a rename, here. If "a" and "b" are very similar, "a" is removed, and "b" has the same (or similar) content as "a", was it a rename from "a", or was it simply that "b" was changed, possibly to incorporate the duplicated items in "a"?
So I don't think it is a bug. But that isn't to say you can't come up with a case where it would be nice, during a diff or a merge, to show things that way. I mentioned at the beginning of the message that what you wanted was to expand the list of destination possibilities. You could have a "--find-overwrites" which puts all of the modified files into the destination list. You would not want it on by default, though, I think, as it would add a lot of computation time to find a somewhat rare case.
-Peff
Anders Kaseorg
