|
|
|
Created:
14 years, 9 months ago by arkadi Modified:
14 years, 9 months ago CC:
codereview-discuss_googlegroups.com Base URL:
http://rietveld.googlecode.com/svn/trunk/ Visibility:
Public. |
http://code.google.com/p/rietveld/issues/detail?id=189
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rietveld patch to fix upload.py -r N:M failure on new directories - try2 #Total messages: 4
|
arkadi
|
14 years, 9 months ago (2011年04月06日 12:58:32 UTC) #1 |
http://codereview.appspot.com/4368045/diff/1/upload.py File upload.py (right): http://codereview.appspot.com/4368045/diff/1/upload.py#newcode686 upload.py:686: Tuple (output, return code) Apart from missing value, you can swap the order of arguments here to be (return code, stdout, errout). The description above could reflect this too. http://codereview.appspot.com/4368045/diff/1/upload.py#newcode939 upload.py:939: data = RunShell(cmd, True) Debugging code? http://codereview.appspot.com/4368045/diff/1/upload.py#newcode946 upload.py:946: logging.warn("No valid patches found in output from svn diff") What's the purpose to continue then? http://codereview.appspot.com/4368045/diff/1/upload.py#newcode1025 upload.py:1025: if re.match('^svn: Unable to find repository location for .+ in revision \d+', err): See the comment at http://code.google.com/p/rietveld/issues/detail?id=189#c8 I feel we should add this check. I don't know what the exact error would be, but the following should work: if re.match('^svn: WE\d+', errout): ErrorExit("Unknown error code from SVN, please report the message to Rietveld tracker:\n%s" % errout) Or it is possible to ask Subversion developers about the code.
http://codereview.appspot.com/4368045/diff/1/upload.py File upload.py (right): http://codereview.appspot.com/4368045/diff/1/upload.py#newcode686 upload.py:686: Tuple (output, return code) ok, will do http://codereview.appspot.com/4368045/diff/1/upload.py#newcode939 upload.py:939: data = RunShell(cmd, True) used by integration script, change removed http://codereview.appspot.com/4368045/diff/1/upload.py#newcode946 upload.py:946: logging.warn("No valid patches found in output from svn diff") i'll remove this sometimes the diff output is empty for the reason I do not remember currently. as we use Rietveld in automated script I prefer it to skip the revision instead of stalling the process http://codereview.appspot.com/4368045/diff/1/upload.py#newcode1025 upload.py:1025: if re.match('^svn: Unable to find repository location for .+ in revision \d+', err): i'll update the code when new Subversion with error numbers is released
LGTM, except changes for trailing whitespaces. The patch also doesn't apply to the trunk clearly. As this is your first patch, I've fixed it for you. Committed in r700 - you've got a lucky number. =) This issue can be closed now. However, see comments in bug tracker.