3067: Parent diffs with mercurial are still broken

glin****@dynamicco********** (Google Code) (Is this you? Claim this profile.)
Jan. 18, 2014
What version are you running?
RB 1.7.13

What's the URL of the page containing the problem?

What steps will reproduce the problem?
1. create new local repo with three changeset, see attachment
2. create new empty central repository, linked to RB
3. push up revision 0 to central repository (don't push up r1 and r2)
4. my intention is to use RB to review changes between rev 1 and rev 2:
5. create parent diff: hg diff -r0 -r1>parent.diff
6. create diff to review: hg diff -r1 -r2>target diff
7. create new review in RB, upload parent.diff & target.diff
8. When clicking "Create Review Request" RB responds with error:
The file 'f1' (r5f714bed7dcc) could not be found in the repository

What is the expected output? What do you see instead?
- I expect RB to create a new review, instead it returns a fault. RB starts looking for file 'f1' (r5f714bed7dcc) in the central repository, but it doesn't exist. This is logical because the listed revision is not in th central repository.

What operating system are you using? What browser?
- tried it from Windows XP and Debian Linux, both give the same result
- browser is Iceweasel

Please provide any additional information below.
- I created the parent and target diff manually to exclude any tool involvement
#1 david
I'm not sure that revision 0 was properly pushed to the repository.

According to this:

$ hg diff -c 1
diff -r 5f714bed7dcc -r ec32bb492cac f1
--- a/f1	Thu Aug 22 07:12:46 2013 +1200
+++ b/f1	Thu Aug 22 07:14:14 2013 +1200
@@ -0,0 +1,1 @@

revision 1 starts with f1 at 5f714bed7dcc and ends at revision ec32bb492cac. I'm not sure how to get hg to tell me what revision the files are at which changes, but it seems to me like 5f714bed7dcc was introduced at revision 0.

#2 glin****@dynamicco********** (Google Code) (Is this you? Claim this profile.)
I made a mistake when creating a new review request. I used the wrong main repository :-(

When I use the correct repository, RB creates the review request and it goes wrong when I want to look at the diff. The diff for f1 is OK, the diff for f2 gives an error: Error: The file 'f2' (rec32bb492cac) could not be found in the repository.

See images attached.

#3 david
  • +Parent diffs with mercurial are still broken
#4 Scher******@gmai***** (Google Code) (Is this you? Claim this profile.)
This bug is extremely aggravating for us. It appears to happen whenever the parent diff does not contain the file that is being changed in the diff to be reviewed.

For instance, parent diff made changes to files A, B
Uploaded diff makes changes to files B, C
B will display correctly, C will throw an error.

We figured out how to work around it, by using a parent diff that goes from 0:desireddiff-1; for example:

we want to upload revision 5, then we would do:
hg export 5 > diff.patch
hg diff -r 0:4 > parent.patch (or null:4)

and upload those, and that always seems to work. Unfortunately, this means we are unable to use postreview in most cases.
#5 glin****@dynamicco********** (Google Code) (Is this you? Claim this profile.)
Thanks for that. Two remarks on your work-around:
- you can still use postreview. Just use both the --master and --parent options
- when using an earlier changeset for the parent, the changeset can become very big. When using changeset zero, it includes the whole change history.

Another option is to manually patch the changeset (diff.patch). I you change the changeset id (in the diffs that go wrong) to the id of the last common parent, it works. Reviewboard can find this changeset and perform the diff correctly. This works because the file hasn't changed in the changesets between the actual parent and the last common parent.
#6 jeme****@gmai***** (Google Code) (Is this you? Claim this profile.)
For postreview, I'm using the -o option in combination with -g and it is enough to work around this bug. 
-o will use a parent diff based on the upstream repo
-g submits the review in a git format

For some reason using the git format fixes the "Diff currently unavailable" bug for me.
#7 Scher******@gmai***** (Google Code) (Is this you? Claim this profile.)
I can't seem to figure out a permutation of --master and --parent to get it to work.

file added in revision 0, trying to review revision 3.

master parent result
0      2      issue present
2      0      abort: The file was not found in the repository (207)
0      0      gives the appearance that all the files in the repo are new

However, the recently posted -o -g work-around does appear to work for me as well.
#9 Scher******@gmai***** (Google Code) (Is this you? Claim this profile.)
Possible related issue, but it may be a different bug. I attempted to use the -o -g workaround and have encountered another edge case that fails:

r4 [draft] : add file A (unrelated in content, but sharing the same name)
r3 [draft] : miscelaneous
r2 [draft] : remove file A
r1 [public] : file A present

hg postreview r4 -o -g -i 'foo'

File A will show 'diff unavailable'

My original workaround (diff -r 0:r3) still worked in this scenario. I have not tested to see if r3 is necessary to the test. Since the -o -g workaround seemed to work because it was using git diffs (and thus likely a different code path), this case should probably be tested with a git repository as well to ensure that it doesn't exist there.
#10 patric******@gmai***** (Google Code) (Is this you? Claim this profile.)
I'm using RB 1.7.12 and I had a slightly different problem with a simpler solution (when using hg-style diffs).

The old code allowed the 'origChangesetId' from any file in the parent diff to become the source_rev for every file in the child diff; however it only worked if the parent diff had any files in common with the child. My attached patch fixes this. While the code in 1.7.12 was not 100% correct, this effectively solved the issue. If you try with 1.7.12 and the attached patch, the problem should be solved. I also had no issues when removing files and re-adding them back--this should be handled by the "f.origInfo != PRE_CREATION" check. Perhaps it is a bug with git-style diffs.

I have yet to test in RB 1.7.13, but looking at the commit log, it looks like the code changed significantly between 1.7.12 and 1.7.13 with https://reviews.reviewboard.org/r/4121/ (45faed44) -- From what I can tell, the code now will only work in the case that you specify a parent diff with a global "# Parent" comment because it now only checks the global origChangesetId, so I think this change just made the issue more complicated.

The good news is that with commits 253ff68 and 5acf49a it is now possible to specify the parent commit ID via the API, so the API parameter might be a nice solution to this problem in versions after 1.7.13. I will try to do some tests on 1.7.13 or a later version. I also have only tested with hg-style diffs, not git-style diffs so those might be different.
  • +
    commit fa54f6eee3f54ce755e8a931b27dab3009310a55
    Author: Patrick Horn <patrick.horn@gmail.com>
    Date:   Fri Oct 11 19:27:42 2013 -0700
        Fix the orig changeset id when parent diffs do not have any files in common with the child diff.
    diff --git a/reviewboard/diffviewer/forms.py b/reviewboard/diffviewer/forms.py
    index 02357ae..6e32dec 100644
    --- a/reviewboard/diffviewer/forms.py
    +++ b/reviewboard/diffviewer/forms.py
    @@ -107,8 +107,7 @@ class UploadDiffForm(forms.Form):
                 # If the user supplied a base diff, we need to parse it and
                 # later apply each of the files that are in the main diff
                 for f in self._process_files(parent_diff_file, basedir,
    -                                         check_existance=True,
    -                                         limit_to=diff_filenames):
    +                                         check_existance=True):
                     parent_files[f.origFile] = f
                     # Store the original changeset ID if we have it; t
#11 david
  • +Component-RBTools
#12 david
This should now be completely fixed in rbtools master as of commit 5a93bb8. This will ship in rbtools 0.6
  • +Fixed