|
|
|
Created:
16 years, 5 months ago by C. Scott Ananian Modified:
15 years, 10 months ago Base URL:
http://rietveld.googlecode.com/svn/trunk/ Visibility:
Public. |
This commit improves upload.py by:
a) automatically creating a subject line and description from the git commit message when --rev <git commit hash> is used on the command line.
b) allowing specification of commit ranges (ie --rev origin..HEAD) and automatically titling the patches in the standard format ([PATCH 1/4], etc).
c) using the .git/description file to add an extra prefix (if set), to ease the use of a single reviews instance for code from multiple git repositories.
By filling in implementations of the new abstract methods added to VersionControlSystem, these features can be added for other version control systems as well.
Documentation:
* To upload a specific git commit for review, use the --rev option, for example:
$ upload.py --rev 960831af2317f0b0b52ee4d13f980d2a56e91cd5
This will make the subject line default to the git log subject (first line of the commit message) and the description default to the git log body (all
but first line of the commit message) with a terse git commitid. If you set .git/description in your checkout, it will add this to the subject line, which helps distinguish patches to different components. For example, I've
set mini/.git/description to "mini", so my patch subject lines for commits to mini start "[PATCH] mini: ".
* You can use ranges in your --rev specification; this will create multiple issues for review, one per commit in the range. For example:
$ upload.py --rev master...my-branch
or
$ upload.py --rev origin..HEAD
The subject lines for each commit begin "[PATCH 1/4]", "[PATCH 2/4]", etc.
Patch Set 1 #
Total comments: 3
Patch Set 2 : upload.py: improve git support; add revision range support [updated] #
Total comments: 4
Total messages: 8
|
C. Scott Ananian
This patch is for http://code.google.com/p/rietveld/issues/detail?id=140
|
16 years, 5 months ago (2009年08月05日 18:33:50 UTC) #1 |
This patch is for http://code.google.com/p/rietveld/issues/detail?id=140
I'm afraid Andi will have to review this; I don't even have git installed so I cannot test this.
This is not a full review. I'm adding Evan as this patch would change upload.py's default behavior, esp. when used with git. Evan, what do you think about the changed command line options and how they work? I assume you know more use cases of upload.py+git than I :) http://codereview.appspot.com/101056/diff/1/2 File static/upload.py (right): http://codereview.appspot.com/101056/diff/1/2#newcode1397 Line 1397: for (i,r) in zip(xrange(1, sys.maxint), rev_range): This would change the current mean of options.revision. You can use --rev start:end to specify that you want to upload *one* diff containing changes between start and end. http://codereview.appspot.com/101056/diff/1/2#newcode1429 Line 1429: message = vcs.GetSubject() Again, this changes the current behaviour. I'm not sure if it's the right thing to assume that a user wants to use the commit message when "-m" is not given on the command line (but "--rev" is present). http://codereview.appspot.com/101056/diff/1/2#newcode1460 Line 1460: description = vcs.GetDescription() The same here.
Updated patch to handle move-of-an-unmodified file better (but see issue 115). On 2009年08月17日 21:40:17, Andi Albrecht wrote: > Evan, what do you think about the changed command line options and how they > work? I assume you know more use cases of upload.py+git than I :) > > http://codereview.appspot.com/101056/diff/1/2 > File static/upload.py (right): > > http://codereview.appspot.com/101056/diff/1/2#newcode1397 > Line 1397: for (i,r) in zip(xrange(1, sys.maxint), rev_range): > This would change the current mean of options.revision. You can use --rev > start:end to specify that you want to upload *one* diff containing changes > between start and end. It wouldn't change the current meaning of --rev for any backend but git. --rev start:end certainly wouldn't do anything for the git backend (although it's unfortunate that you'd get opposite behavior for the different backends: to generate one patch for a range you'd use start..end *without* the --rev flag, both with current upload.py and with my patches). > http://codereview.appspot.com/101056/diff/1/2#newcode1429 > Line 1429: message = vcs.GetSubject() > Again, this changes the current behaviour. I'm not sure if it's the right thing > to assume that a user wants to use the commit message when "-m" is not given on > the command line (but "--rev" is present). > > http://codereview.appspot.com/101056/diff/1/2#newcode1460 > Line 1460: description = vcs.GetDescription() > The same here. Both of these methods return None in the default implementation, so this would be changing the behavior for the git backend only. (Although you could implement them appropriately in the other backends if you wanted consistent behavior.)
I'm not opposed to this in principle, as git-cl doesn't use the --rev flag. I have a number of style and design comments that should happen after you rebase.
On 2009年08月21日 16:10:21, Evan Martin wrote: > I'm not opposed to this in principle, as git-cl doesn't use the --rev flag. I > have a number of style and design comments that should happen after you rebase. Arrrgh, switched usernames and lost comments, let me recreate.
http://codereview.appspot.com/101056/diff/4001/4002 File static/upload.py (right): http://codereview.appspot.com/101056/diff/4001/4002#newcode1049 Line 1049: (["git", "log", "--pretty=format:%H", range_spec]) why not rev-list? http://codereview.appspot.com/101056/diff/4001/4002#newcode1064 Line 1064: if not d.startswith("Unnamed") and len(d) < 20: this "20" feels pretty arbitrary http://codereview.appspot.com/101056/diff/4001/4002#newcode1092 Line 1092: # the second filename. This will conflict with changes I've landed upstream, so please rebase and reupload. http://codereview.appspot.com/101056/diff/4001/4002#newcode1413 Line 1413: for (i,r) in zip(xrange(1, sys.maxint), rev_range): enumerate() ?
When uploading multiple patches you get the The following files are not added to version control: [...] Are you sure to continue?(y/N) confirmation for every patch, which is pretty pointless. Once per upload should be sufficient. I think there should also be a confirmation such as: About to send 3 patches for review: add foo fix bar remove bug Are you sure to continue?(y/N) to be confident you got the patch range / branch / whatever right.