Issue 11868043: Add ability to review commit messages.
|
Created:
11 years, 5 months ago by jparent1 Modified:
6 months ago CC:
codereview-list_googlegroups.com Visibility:
Public. |
DescriptionAdd ability to review commit messages.
1. When you upload a new issue, creates a magic COMMIT_MSG file that is part of the patchset, with the contents of the issue description. This file is not included in the raw patchset, since it isn't a real file that you'd want to download, but shows up in the UI. This allows you to add comments to the commit message, view diffs between versions, etc.
2. If you edit the description from the Reitveld UI, a new patchset is created, identical to the last one, except with a new COMMIT_MSG, to keep it in sync with the one in the issue description.
https://code.google.com/p/rietveld/issues/detail?id=457
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressing code review feedback #
Total comments: 12
Patch Set 3 : Address stylistic code review feedback. #MessagesTotal messages: 15
Demo server is running, with demo issue, at https://codereview-julie-test.appspot.com/9001/ You can see the new special COMMIT_MSG (always the first "file"), and play with editing the description from the Edit Issue UI to see how it updates with a new patchset.
Sign in to reply to this message.
https://codereview.appspot.com/11868043/diff/1/app.yaml File app.yaml (right): https://codereview.appspot.com/11868043/diff/1/app.yaml#newcode1 app.yaml:1: application: codereview-julie-test This obviously shouldn't be part of this change. Is there an easy way to exclude it when I upload?
Sign in to reply to this message.
https://codereview.appspot.com/11868043/diff/1/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/11868043/diff/1/codereview/views.py#newcode1838 codereview/views.py:1838: filename = "COMMIT_MSG" Can you rename this to "Commit Message"? So it matches how gerrit does. I'll really like to see this feature in production :)
Sign in to reply to this message.
https://codereview.appspot.com/11868043/diff/1/app.yaml File app.yaml (right): https://codereview.appspot.com/11868043/diff/1/app.yaml#newcode1 app.yaml:1: application: codereview-julie-test On 2013/07/26 00:50:56, jparent1 wrote: > This obviously shouldn't be part of this change. Is there an easy way to exclude > it when I upload? (Drive-by-ish: I'll do a more thorough review later :) you can upload like APPID=codereview-julie-test make upload :)
Sign in to reply to this message.
On 2013/07/26 00:59:27, iannucci wrote: > https://codereview.appspot.com/11868043/diff/1/app.yaml > File app.yaml (right): > > https://codereview.appspot.com/11868043/diff/1/app.yaml#newcode1 > app.yaml:1: application: codereview-julie-test > On 2013/07/26 00:50:56, jparent1 wrote: > > This obviously shouldn't be part of this change. Is there an easy way to > exclude > > it when I upload? > > (Drive-by-ish: I'll do a more thorough review later :) > > you can upload like > > APPID=codereview-julie-test make upload > > :) COMMIT_MSG is included in the tar.bz2 download for a patchset. I think we should also remove it when creating the zip.
Sign in to reply to this message.
https://codereview.appspot.com/11868043/diff/1/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/11868043/diff/1/codereview/views.py#newcode1838 codereview/views.py:1838: filename = "COMMIT_MSG" I'd also would like to see a module constant for this string since we refer to it in several locations in this module. https://codereview.appspot.com/11868043/diff/1/upload.py File upload.py (right): https://codereview.appspot.com/11868043/diff/1/upload.py#newcode1181 upload.py:1181: """We found a new commit message, need to create a dummy base file""" Please turn this docstring into a comment.
Sign in to reply to this message.
Addressed all feedback. Special commit file is no longer part of tarball, is now named Commit Message. https://codereview.appspot.com/11868043/diff/1/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/11868043/diff/1/codereview/views.py#newcode1838 codereview/views.py:1838: filename = "COMMIT_MSG" On 2013/07/26 00:55:56, tfarina1 wrote: > Can you rename this to "Commit Message"? So it matches how gerrit does. I'll > really like to see this feature in production :) Done. https://codereview.appspot.com/11868043/diff/1/codereview/views.py#newcode1838 codereview/views.py:1838: filename = "COMMIT_MSG" On 2013/07/29 08:38:34, Andi wrote: > I'd also would like to see a module constant for this string since we refer to > it in several locations in this module. Done. https://codereview.appspot.com/11868043/diff/1/upload.py File upload.py (right): https://codereview.appspot.com/11868043/diff/1/upload.py#newcode1181 upload.py:1181: """We found a new commit message, need to create a dummy base file""" On 2013/07/29 08:38:34, Andi wrote: > Please turn this docstring into a comment. Done.
Sign in to reply to this message.
Any other concerns? Thanks!
Sign in to reply to this message.
looks pretty good to me, but I'd like someone to OK the transactionality of the datastore ops. I'm out this week, so if someone else could land+deploy this, that would be awesome :) https://codereview.appspot.com/11868043/diff/12001/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... codereview/views.py:1841: fromfile="\dev\\null",tofile=COMMIT_MSG) you can use a raw string to avoid the tricky backslashing: r'\dev\null' https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... codereview/views.py:2308: new_patchset.put() I'm a little worried about all these interleaved put()'s. appengine is pretty notorious for spuriously emitting exceptions when interacting with the datastore. The worrysome bit to me is if one of these put()'s throws, I think the datastore will be in a weird state. However I'm not expert enough to be confident in suggesting an answer (using a transaction springs to mind, but I'm not sure if that will work).
Sign in to reply to this message.
https://codereview.appspot.com/11868043/diff/12001/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... codereview/views.py:1837: 2 lines between file level symbols (and line 1854) https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... codereview/views.py:1840: udiff = difflib.unified_diff("",description.splitlines(1), whitespace after comma. https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... codereview/views.py:1879: commitPatch = _create_commit_message_patch(patchset, golang style variable names? :) Usually use commit_patch style. https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... codereview/views.py:2308: new_patchset.put() On 2013/08/19 21:56:47, iannucci (OOO through Aug 27) wrote: > I'm a little worried about all these interleaved put()'s. appengine is pretty > notorious for spuriously emitting exceptions when interacting with the > datastore. The worrysome bit to me is if one of these put()'s throws, I think > the datastore will be in a weird state. However I'm not expert enough to be > confident in suggesting an answer (using a transaction springs to mind, but I'm > not sure if that will work). Agreed, it's annoying when one of the put() throws in the middle of a patchset. It would make it slightly worse, worth a transaction (even if it'll make it even more likely to fail)
Sign in to reply to this message.
https://codereview.appspot.com/11868043/diff/12001/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... codereview/views.py:1841: fromfile="\dev\\null",tofile=COMMIT_MSG) On 2013/08/19 21:56:47, iannucci (OOO through Aug 27) wrote: > you can use a raw string to avoid the tricky backslashing: r'\dev\null' Why do we use backslashes here at all? At least Mercurial uses "/dev/null"
Sign in to reply to this message.
https://codereview.appspot.com/11868043/diff/12001/codereview/views.py File codereview/views.py (right): https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... codereview/views.py:1837: On 2013/08/20 01:24:29, M-A wrote: > 2 lines between file level symbols (and line 1854) Done. https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... codereview/views.py:1840: udiff = difflib.unified_diff("",description.splitlines(1), On 2013/08/20 01:24:29, M-A wrote: > whitespace after comma. Done. https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... codereview/views.py:1841: fromfile="\dev\\null",tofile=COMMIT_MSG) Switched to /dev/null On 2013/08/26 09:39:42, Andi wrote: > On 2013/08/19 21:56:47, iannucci (OOO through Aug 27) wrote: > > you can use a raw string to avoid the tricky backslashing: r'\dev\null' > > Why do we use backslashes here at all? At least Mercurial uses "/dev/null" https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... codereview/views.py:1879: commitPatch = _create_commit_message_patch(patchset, Whoops. I'm a Google-style Javascript gal at heart, so hard to break the habits sometimes! Fixed here and elsewhere. On 2013/08/20 01:24:29, M-A wrote: > golang style variable names? :) Usually use commit_patch style. https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... codereview/views.py:2308: new_patchset.put() So, there is a bit of a problem, you can't allocate ids inside of transactions (see a recent change like https://codereview.appspot.com/10434044 to remove transactions to deal with this). I need to allocate the id for the commit_patch AFTER I create new_patchset, since it uses the parent patchset to create the correct key. I could wrap new_patchset creation + put in a try, and then do the rest of the puts in a txn if putting new_patchset succeeds? On 2013/08/20 01:24:29, M-A wrote: > On 2013/08/19 21:56:47, iannucci (OOO through Aug 27) wrote: > > I'm a little worried about all these interleaved put()'s. appengine is pretty > > notorious for spuriously emitting exceptions when interacting with the > > datastore. The worrysome bit to me is if one of these put()'s throws, I think > > the datastore will be in a weird state. However I'm not expert enough to be > > confident in suggesting an answer (using a transaction springs to mind, but > I'm > > not sure if that will work). > > Agreed, it's annoying when one of the put() throws in the middle of a patchset. > It would make it slightly worse, worth a transaction (even if it'll make it even > more likely to fail)
Sign in to reply to this message.
On 2013/08/27 22:46:38, jparent1 wrote: > https://codereview.appspot.com/11868043/diff/12001/codereview/views.py > File codereview/views.py (right): > > https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... > codereview/views.py:1837: > On 2013/08/20 01:24:29, M-A wrote: > > 2 lines between file level symbols (and line 1854) > > Done. > > https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... > codereview/views.py:1840: udiff = > difflib.unified_diff("",description.splitlines(1), > On 2013/08/20 01:24:29, M-A wrote: > > whitespace after comma. > > Done. > > https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... > codereview/views.py:1841: fromfile="\dev\\null",tofile=COMMIT_MSG) > Switched to /dev/null > > On 2013/08/26 09:39:42, Andi wrote: > > On 2013/08/19 21:56:47, iannucci (OOO through Aug 27) wrote: > > > you can use a raw string to avoid the tricky backslashing: r'\dev\null' > > > > Why do we use backslashes here at all? At least Mercurial uses "/dev/null" > > https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... > codereview/views.py:1879: commitPatch = _create_commit_message_patch(patchset, > Whoops. I'm a Google-style Javascript gal at heart, so hard to break the habits > sometimes! Fixed here and elsewhere. > > On 2013/08/20 01:24:29, M-A wrote: > > golang style variable names? :) Usually use commit_patch style. > > https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... > codereview/views.py:2308: new_patchset.put() > So, there is a bit of a problem, you can't allocate ids inside of transactions > (see a recent change like https://codereview.appspot.com/10434044 to remove > transactions to deal with this). I need to allocate the id for the commit_patch > AFTER I create new_patchset, since it uses the parent patchset to create the > correct key. > > I could wrap new_patchset creation + put in a try, and then do the rest of the > puts in a txn if putting new_patchset succeeds? > > On 2013/08/20 01:24:29, M-A wrote: > > On 2013/08/19 21:56:47, iannucci (OOO through Aug 27) wrote: > > > I'm a little worried about all these interleaved put()'s. appengine is > pretty > > > notorious for spuriously emitting exceptions when interacting with the > > > datastore. The worrysome bit to me is if one of these put()'s throws, I > think > > > the datastore will be in a weird state. However I'm not expert enough to be > > > confident in suggesting an answer (using a transaction springs to mind, but > > I'm > > > not sure if that will work). > > > > Agreed, it's annoying when one of the put() throws in the middle of a > patchset. > > It would make it slightly worse, worth a transaction (even if it'll make it > even > > more likely to fail) Wondering how I should proceed here. Should I go ahead and code up the solution I mentioned above to deal with the transaction issues?
Sign in to reply to this message.
On 2013/09/06 22:55:47, jparent1 wrote: > On 2013/08/27 22:46:38, jparent1 wrote: > > https://codereview.appspot.com/11868043/diff/12001/codereview/views.py > > File codereview/views.py (right): > > > > > https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... > > codereview/views.py:1837: > > On 2013/08/20 01:24:29, M-A wrote: > > > 2 lines between file level symbols (and line 1854) > > > > Done. > > > > > https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... > > codereview/views.py:1840: udiff = > > difflib.unified_diff("",description.splitlines(1), > > On 2013/08/20 01:24:29, M-A wrote: > > > whitespace after comma. > > > > Done. > > > > > https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... > > codereview/views.py:1841: fromfile="\dev\\null",tofile=COMMIT_MSG) > > Switched to /dev/null > > > > On 2013/08/26 09:39:42, Andi wrote: > > > On 2013/08/19 21:56:47, iannucci (OOO through Aug 27) wrote: > > > > you can use a raw string to avoid the tricky backslashing: r'\dev\null' > > > > > > Why do we use backslashes here at all? At least Mercurial uses "/dev/null" > > > > > https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... > > codereview/views.py:1879: commitPatch = _create_commit_message_patch(patchset, > > Whoops. I'm a Google-style Javascript gal at heart, so hard to break the > habits > > sometimes! Fixed here and elsewhere. > > > > On 2013/08/20 01:24:29, M-A wrote: > > > golang style variable names? :) Usually use commit_patch style. > > > > > https://codereview.appspot.com/11868043/diff/12001/codereview/views.py#newcod... > > codereview/views.py:2308: new_patchset.put() > > So, there is a bit of a problem, you can't allocate ids inside of transactions > > (see a recent change like https://codereview.appspot.com/10434044 to remove > > transactions to deal with this). I need to allocate the id for the > commit_patch > > AFTER I create new_patchset, since it uses the parent patchset to create the > > correct key. > > > > I could wrap new_patchset creation + put in a try, and then do the rest of the > > puts in a txn if putting new_patchset succeeds? > > > > On 2013/08/20 01:24:29, M-A wrote: > > > On 2013/08/19 21:56:47, iannucci (OOO through Aug 27) wrote: > > > > I'm a little worried about all these interleaved put()'s. appengine is > > pretty > > > > notorious for spuriously emitting exceptions when interacting with the > > > > datastore. The worrysome bit to me is if one of these put()'s throws, I > > think > > > > the datastore will be in a weird state. However I'm not expert enough to > be > > > > confident in suggesting an answer (using a transaction springs to mind, > but > > > I'm > > > > not sure if that will work). > > > > > > Agreed, it's annoying when one of the put() throws in the middle of a > > patchset. > > > It would make it slightly worse, worth a transaction (even if it'll make it > > even > > > more likely to fail) > > Wondering how I should proceed here. Should I go ahead and code up the solution > I mentioned above to deal with the transaction issues? Yeah, I think that would probably be the best thing to do at this point
Sign in to reply to this message.
|
