49154 2010-11-07 19:57:39 -0800 patches uploaded from git (using webkit-patch) do not include moved files 2011-04-18 19:08:21 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC OS X 10.5 RESOLVED DUPLICATE 48075 P2 Normal --- 1 eric webkit-unassigned abarth aroben dbates ddkilzer evan mihaip rolandsteiner oldest_to_newest 305946 0 eric 2010-11-07 19:57:39 -0800 patches uploaded from git (using webkit-patch) do not include moved files I ran into this while trying to work on bug 49153. 305948 1 eric 2010-11-07 19:59:31 -0800 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! 305950 2 eric 2010-11-07 20:00:45 -0800 Maybe we should be using some other command to generate the git diff? 305957 3 ddkilzer 2010-11-07 20:17:57 -0800 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. 352595 4 eric 2011-02-16 21:00:35 -0800 We seem to run into this a lot, but I'm not sure I have good suggestions as to how to fix. 352947 5 ddkilzer 2011-02-17 08:56:04 -0800 (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 353113 6 eric 2011-02-17 12:48:15 -0800 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?). 353121 7 aroben 2011-02-17 12:55:02 -0800 (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? 353123 8 abarth 2011-02-17 12:56:25 -0800 This might be a problem with the changed_files optimization. 355444 9 tony 2011-02-22 11:35:39 -0800 *** Bug 54854 has been marked as a duplicate of this bug. *** 358089 10 mihaip 2011-02-25 11:36:43 -0800 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. 358182 11 abarth 2011-02-25 14:22:16 -0800 (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. 388222 12 eric 2011-04-18 19:08:21 -0700 *** This bug has been marked as a duplicate of bug 48075 ***