patches uploaded from git (using webkit-patch) do not include moved files

[Eric Seidel (no email)]

patches uploaded from git (using webkit-patch) do not include moved files

I ran into this while trying to work on bug 49153.

[Eric Seidel (no email)]

The command we're using is:

    def create_patch(self, git_commit=None, changed_files=[]):
...
        return self.run(['git', 'diff', '--binary', "--no-ext-diff", "--full-index", "-M", self.merge_base(git_commit), "--"] + changed_files, decode_output=False)

It's possible this is recently broken by Adam's changed_files optimization, but I think git diff just doesn't include moved files!

[Eric Seidel (no email)]

Maybe we should be using some other command to generate the git diff?

[David Kilzer (:ddkilzer)]

If there are no changes to the renamed (moved) file, the diff content is blank, but there is a diff header noting the rename does exist in the patch.  This differs from svn-create-patch, though, in that the (unchanged) contents of the file aren't included in the patch itself.

[Eric Seidel (no email)]

We seem to run into this a lot, but I'm not sure I have good suggestions as to how to fix.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> We seem to run into this a lot, but I'm not sure I have good suggestions as to how to fix.

git detects renames through content only.  Is the real issue that committing the patch back to svn (via "git svn dcommit") doesn't represent the moved file like it would if "svn mv" were used?  If so, you may want to add this to the .git/config being used on the commit queue bots:

[diff]
	renames = true

If the bots are using an svn repo (not a git repo), then this issue becomes harder (unless everyone uses -M to make their patches so git includes "rename" metadata near the top of the patch.

This is completely orthogonal, but this should also be set (so that empty directories aren't left in svn by the bots):

[svn]
	rmdir = true

[Eric Seidel (no email)]

Sounds like we should definitely add those to the git config page.

However I don't think this is a commit-side problem.  It seems that the patches uploaded have no move information.  Or at least if they do I don't remember seeing it in prettypatch and it must be ignored by svn-apply (maybe that's what the diff.rename=true option fixes?).

[Adam Roben (:aroben)]

(In reply to comment #6)
> Sounds like we should definitely add those to the git config page.
> 
> However I don't think this is a commit-side problem.  It seems that the patches uploaded have no move information.  Or at least if they do I don't remember seeing it in prettypatch and it must be ignored by svn-apply (maybe that's what the diff.rename=true option fixes?).

PrettyPatch will show file moves Do you have an example where the wrong thing happened for us to look at?

[Adam Barth]

This might be a problem with the changed_files optimization.

[Tony Chang]

*** Bug 54854 has been marked as a duplicate of this bug. ***

[Mihai Parparita]

I don't know if it's related to this, but ChangeLogs generated by webkit-patch land-cowboy also don't list moved files correctly, but prepare-ChangeLog does.

[Adam Barth]

(In reply to comment #10)
> I don't know if it's related to this, but ChangeLogs generated by webkit-patch land-cowboy also don't list moved files correctly, but prepare-ChangeLog does.

That's likely to be the same bug.  The difference is we keep around a list of changed files, which we use to avoid stating the whole file system.  It seems likely that moved files aren't represented correctly in that list.

[Eric Seidel (no email)]

*** This bug has been marked as a duplicate of bug 48075 ***