Separate storing commit IDs and updating from them.

Review Request #5331 — Created Jan. 26, 2014 and submitted

Information

Review Board
master

Reviewers

Separate storing commit IDs and updating from them.

Review requests previously stored commit IDs and allowed setting them
during creation with the assumption being that they were intended for
setting the review request information and diff.

This change removes that assumption. Commit IDs can now be set on new
review requests or updated later on drafts. The old behavior is invoked
by passing a commit_id in the request along with create_from_commit_id=1
or update_from_commit_id=1 (depending on the operation). For
compatibility reasons, setting changenum will automatically trigger the
old behavior as well.

Since commit IDs are now stored along with drafts, we now record them in
change descriptions as well. Some changes needed to be made to field
recording logic for this. I also split up the existing CommitField
into ChangeField and CommitField in order to better separate out
responsibilities and provide a more appropriate label depending on
whether Perforce-style changesets are used vs. commit IDs.

This does remove the ability to update based on a commit ID using
HTTP PUT on the review request item resource, in favor of the draft API.

JavaScript and Python unit tests passed.

Posted an exiting commit using the New Review Request UI. Saw the commit
information in the draft, plus the summary, description, and diff.
Published and saw the commit information remain.

Updated RBTools to post the commit ID, and posted a new branch. It didn't
try to grab the review request info or diff from the server, but rather used
what I had. Saw the commit ID in the draft. Published it, then made a change
locally and posted again. Commit ID changed. Published, and saw the change
entry for the commit ID.

Posted a change on a Perforce repository using rbt post.
Saw the "Change:" field instead of the "Commit:" field. Published.
Posted an update with the same command, and it successfully reused and
updated my review request.

Description From Last Updated

Why are you changing some of these from commit_id to commit-id? If it's for consistency, can you change all to …

daviddavid

Should this be 'commit-id'?

daviddavid

Hmm. Shouldn't update() still have commit_id in its fields? Or is that handled by self.fields?

daviddavid

Same question with - vs _

daviddavid
SA
  1. Ship It!
    1. This is a production server. Do not post any test messages of any sort here. You can use http://demo.reviewboard.org/ for that.

  2. 
      
SA
  1. Ship it

  2. 
      
david
  1. In your testing done, you list "JavaScript and Python unit tests failed." Shouldn't they... not fail?

    1. Hahaha, that's the typo of the day.

      Succeeded. The unit tests succeeded.

    2. (Or passed. Whatever.)

  2. Why are you changing some of these from commit_id to commit-id? If it's for consistency, can you change all to be commit-id and name your new field to create-from-commit-id ?

    1. GET query parameters always use dashes. POST/PUT fields always use underscores. commit_id was the only query parameter using the wrong format.

  3. Should this be 'commit-id'?

    1. This is a field, so it uses underscores. Same below.

  4. Same question with - vs _

  5. 
      
chipx86
david
  1. 
      
  2. reviewboard/webapi/resources/review_request.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Hmm. Shouldn't update() still have commit_id in its fields? Or is that handled by self.fields?

    1. I've removed the ability to update it in the review request resource itself, and moved that to the draft, since commit_id now works and is treated like any other field.

      I kept support for PUTting changenum on ReviewRequestResource for backwards-compatibility reasons, but since commit_id was introduced in 2.0, no need to keep that compatibility.

    2. Fair enough.

  3. 
      
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Loading...