3312: Diffs without embedded commit IDs can not be used

gmiros******@rebbi***** (Google Code) (Is this you? Claim this profile.)
July 11, 2014 
What version are you running?
2.0 RC2

What's the URL of the page containing the problem?
/r/1/diff/


What steps will reproduce the problem?
1. Create a new Mercurial repository in RB using the following path:
https://bitbucket.org/laggyluke/rb-bug

2. Create new review request:

        $ curl -i http://admin:admin@rb.dev/api/review-requests/ \
            -d repository=1

        HTTP/1.1 201 CREATED
        Date: Fri, 18 Apr 2014 12:12:42 GMT
        Server: Apache/2.2.22 (Ubuntu)
        Content-Length: 1798
        Content-Language: en
        X-Content-Type-Options: nosniff
        Expires: Fri, 18 Apr 2014 12:12:42 GMT
        Vary: Accept,Cookie,Accept-Language,Accept-Encoding
        Last-Modified: Fri, 18 Apr 2014 12:12:42 GMT
        Cache-Control: max-age=0
        X-Frame-Options: SAMEORIGIN
        Set-Cookie:  rbsessionid=vdjff0woxz2rilan3jzb0syv44lkimxu; expires=Sat, 18-Apr-2015 12:12:42 GMT; httponly; Max-Age=31536000; Path=/
        Content-Type: application/vnd.reviewboard.org.review-request+json

        {"stat": "ok", "review_request": {"status": "pending", "last_updated": "2014-04-18T12:12:42Z", "links": {"diffs": {"href": "http://rb.dev/api/review-requests/1/diffs/", "method": "GET"}, "repository": {"href": "http://rb.dev/api/repositories/1/", "method": "GET", "title": "rb-bug"}, "screenshots": {"href": "http://rb.dev/api/review-requests/1/screenshots/", "method": "GET"}, "self": {"href": "http://rb.dev/api/review-requests/1/", "method": "GET"}, "update": {"href": "http://rb.dev/api/review-requests/1/", "method": "PUT"}, "last_update": {"href": "http://rb.dev/api/review-requests/1/last-update/", "method": "GET"}, "reviews": {"href": "http://rb.dev/api/review-requests/1/reviews/", "method": "GET"}, "file_attachments": {"href": "http://rb.dev/api/review-requests/1/file-attachments/", "method": "GET"}, "draft": {"href": "http://rb.dev/api/review-requests/1/draft/", "method": "GET"}, "diff_context": {"href": "http://rb.dev/api/review-requests/1/diff-context/", "method": "GET"}, "submitter": {"href": "http://rb.dev/api/users/admin/", "method": "GET", "title": "admin"}, "changes": {"href": "http://rb.dev/api/review-requests/1/changes/", "method": "GET"}, "delete": {"href": "http://rb.dev/api/review-requests/1/", "method": "DELETE"}}, "depends_on": [], "issue_resolved_count": 0, "ship_it_count": 0, "id": 1, "target_people": [], "changenum": null, "bugs_closed": [], "testing_done": "", "branch": "", "time_added": "2014-04-18T12:12:42Z", "extra_data": {}, "public": false, "commit_id": null, "blocks": [], "description": "", "text_type": "plain", "issue_open_count": 0, "approved": false, "url": "/r/1/", "absolute_url": "http://rb.dev/r/1/", "target_groups": [], "summary": "", "issue_dropped_count": 0, "approval_failure": "The review request has not been marked \"Ship It!\""}}

3. Observe following diff:

        $ cat test.patch
        diff --git a/README.md b/README.md
        --- a/README.md
        +++ b/README.md
        @@ -1,3 +1,5 @@
         Commits:

         * First commit
        +* Second commit
        +* Third commit

4. As the above diff doesn't have any commit IDs, explicitly specify
    `base_commit_id` during upload:

        $ curl -i http://admin:admin@rb.dev/api/review-requests/1/draft/diffs/ \
            -F base_commit_id=4c43907 \
            -F path=@test.patch

        HTTP/1.1 100 Continue

        HTTP/1.1 201 CREATED
        Date: Fri, 18 Apr 2014 12:17:53 GMT
        Server: Apache/2.2.22 (Ubuntu)
        Content-Length: 559
        Content-Language: en
        X-Content-Type-Options: nosniff
        Expires: Fri, 18 Apr 2014 12:17:53 GMT
        Vary: Accept,Cookie,Accept-Language,Accept-Encoding
        Last-Modified: Fri, 18 Apr 2014 12:17:53 GMT
        Cache-Control: max-age=0
        X-Frame-Options: SAMEORIGIN
        Set-Cookie:  rbsessionid=ebozaewk9kmyzv4go6emonz6139ppmrl; expires=Sat, 18-Apr-2015 12:17:53 GMT; httponly; Max-Age=31536000; Path=/
        Content-Type: text/plain

        {"diff": {"name": "test.patch", "links": {"self": {"href": "http://rb.dev/api/review-requests/1/draft/diffs/1/", "method": "GET"}, "draft_files": {"href": "http://rb.dev/api/review-requests/1/draft/diffs/1/files/", "method": "GET"}, "update": {"href": "http://rb.dev/api/review-requests/1/draft/diffs/1/", "method": "PUT"}, "repository": {"href": "http://rb.dev/api/repositories/1/", "method": "GET", "title": "rb-bug"}}, "timestamp": "2014-04-18T12:17:36Z", "basedir": "", "extra_data": {}, "revision": 1, "id": 1, "base_commit_id": "4c43907"}, "stat": "ok"}

5. Open the diff in a web interface.

What is the expected output? What do you see instead?
I expect to see the diff.
Instead, I see this:
http://i.imgur.com/cUQGJ4o.png

What operating system are you using? What browser?
N/A

Please provide any additional information below.
This may be specific to Mercurial repositories, but I'm not sure.
The following patch seems to fix this, but it may have some other undesired side effects that I haven't considered:
https://gist.github.com/laggyluke/a7f9b082ad7db95ab564
david
#1 david 
The base commit ID is generally different from the file IDs embedded in the diffs (one is the revision of the commit, the others are the specific hashes of the file data).

Can you do a little investigation and see how your method differs from what RBTools does for bitbucket mercurial repositories?
  • +NeedInfo
#2 george.mi*********@gmai***** (Google Code) (Is this you? Claim this profile.) 
Unfortunately I'm not using RBTools, I talk directly to Web API.
I've looked into how this is handled by RBTools and I couldn't find anything - just some bits for handling empty files, but that doesn't seem relevant.
I've also checked this bug against the latest master (9469487cff4e6e1353080581f808cd0f43a39a0d) and it's still there.

Maybe I misunderstood how base_commit_id should work, so I've posted the question on mailing list (pending moderation).
#3 george.mi*********@gmai***** (Google Code) (Is this you? Claim this profile.) 
Just tried to use "rbt post" which failed too, but maybe this is a separate issue:
https://gist.github.com/laggyluke/cf1c8885948621a4afa2
#4 bruce*****@gmai***** (Google Code) (Is this you? Claim this profile.) 
I think this problem is (was?) caused by using git diffs by default, with the following in mercurial.ini/.hgrc:

[diff]
git = True

However, I've just tried posting with this configuration on an instance with the base commit id fix that's in 2.0.2 and the diff does display correctly, with the revision shown as "Revision UNKNOWN".
#5 gmiros******@rebbi***** (Google Code) (Is this you? Claim this profile.) 
That's right, dropping "--git" does generate a diff acceptable for Review Board.
While diffs without commit IDs are still broken and base_commit_id param is a bit misleading, that workaround works for me.
#6 bruce*****@gmai***** (Google Code) (Is this you? Claim this profile.) 
Sorry - I just installed 2.0.1 and found that uploading the same diff *does* work - it shows 'Revision UNKNOWN' but still works. So it's not something that's changed between 2.0.1 and 2.0.2

The hg postreview code handles git diffs by adding a header:

        if opts.get('git') or ui.configbool('diff', 'git'):
            # Git diffs don't include the revision numbers with each file, so
            # we have to put them in the header instead.
            output += "# Node ID " + node.hex(r.node()) + "\n"
            output += "# Parent  " + node.hex(parent.node()) + "\n"
david
#7 david 
Yeah, there's a lot that rbtools does in order to create diffs that contain enough information for various different SCMs and hosting services. You'll either have to switch to using it to post (you could also use 'rbt diff' if you really want to use the API yourself) or re-implement some of that logic.
  • -NeedInfo
    +ThirdParty