I'm more than happy to fix these, but could use some guidance on how to format this without going over the 80 column limit. Should I move the first argument to the next line and indent all continuation lines by four spaces? Apologies if this is explained on the coding standards page, I couldn't find it.

Moving the first argument to the next line and indenting by 4 is correct. For example, in this case:

repo = Repository.objects.create(
    name='Test HG',
    path='scmtools/testdata/hg_repo.bundle',
    tool=Tool.objects.get(name='Mercurial'))

I didn't know about evolution files - I did wonder how database upgrade worked, should have read up on it before submitting. I'll try and get time to do this and the reformatting changes in the next few days.

[bump]

Just wondering if anyone had had a chance to look at this yet? It would really help us to get this fix in.

Hi Colin,

I think it looks fine (we'l need to test this more in the wild to make sure it doesn't break more corner cases). However, it doesn't merge anymore. Mind fixing that up? We'll then get it into a release and test it out.

Christian -

I suggest holding off on incorporating this patch until https://code.google.com/p/reviewboard/issues/detail?id=2971 is fixed, as that fix may impact this one.

Can you let me know which unit test failed? When I run them a lot of them get skipped or have errors, presumably due to me not having all of the required dependencies on my system. Of the ones that were able to run, I got the same number of failures before and after the change so I'm assuming that the failure is in one of the ones I couldn't run.

As for the f.origFile to f.newFile change; this is pretty crucial to fixing the issue at hand. Every way I look at it the changed version is correct - but I'm ready to accept that there's a way of looking at it that I'm not seeing. :)

As far as I can tell my patch doesn't change what is being *stored* (at least in the database), it just changes how we match up files in the parent diff with files in the posted diff (excuse my made-up terminology).

Basically we have "files", which is the set of files in the posted diff, and "parent_files", which is the set of files in the parent diff. Each element of both has an "origFile" field, which is the name of the file before the diff, and a "newFile" field, which is the name of the file after the diff.

So if we have a file that is renamed from A to B in the parent diff, and then from B to C in the posted diff, in order to match them up correctly we have to compare parent_file.newFile with posted_file.origFile. The unpatched code compares parent_file.origFile with posted_file.origFile, which is clearly going to fail in this scenario. (Either version of the code will work if the file was not renamed in the parent diff.)

Hi Colin,

I believe it was UploadDiffFormTests.test_parent_diff_filtering that was failing.

Thanks for the detailed explanation. I think you're right in that it's the correct solution. Just hoping there's no major corner cases. We'll find out, though :)