Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
184343003:
Now trychange can store patches in a Git repo (Closed)
|
Created:
6 years, 9 months ago by nodir Modified:
6 years, 8 months ago CC:
chromium-reviews, cmp-cc_chromium.org, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org Visibility:
Public. |
DescriptionNow trychange can store patches in a Git repo
A git patch repo is cloned to .git/git-try/patches-git, if was not
cloned before. Otherwise, changes are pulled. Then a patch is committed and
pushed.
--revision=auto (Git only) is resolved to the revision the diff is generated
against.
R=stip@chromium.org, agable@chromium.org, iannucci@chromium.org, maruel@chromium.org
BUG=325882
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260471
Patch Set 1 #Patch Set 2 : nit #
Total comments: 16
Patch Set 3 : Addressed agable's suggestions #Patch Set 4 : Made it obvious that options is current_vcs.options #Patch Set 5 : Import email from the current repo instead of using global config #
Total comments: 9
Patch Set 6 : use contextmanager #
Total comments: 8
Patch Set 7 : #
Total comments: 15
Patch Set 8 : using branch file and separate branches for patches #
Total comments: 12
Patch Set 9 : nit #Patch Set 10 : minor changes #Patch Set 11 : branch -> ref #
Total comments: 2
Messages
Total messages: 35 (0 generated)
PTAL, now trychange.py can upload patches to a Git repo
Hey, Mike, can you take a look at this?
https://codereview.chromium.org/184343003/diff/20001/scm.py File scm.py (right): https://codereview.chromium.org/184343003/diff/20001/scm.py#newcode398 scm.py:398: return GIT.Capture(['rev-parse', '--git-dir'], cwd=cwd) Note that this returns a relative path if you're adjacent to the .git directory, and an absolute path otherwise. https://codereview.chromium.org/184343003/diff/20001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode410 trychange.py:410: _SendChangeGit.""" nit: multiline docstring formatting. https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode485 trychange.py:485: Args: Should be a complete list of args, not just the confusing ones. https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode490 trychange.py:490: # Create a temporary file and put description into it nit: sentence-like comments should have periods. https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode619 trychange.py:619: target_filename = os.path.abspath(target_filename) Combine this into one statement, with indentation at +4 on the second line. https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode622 trychange.py:622: scm.GIT.Capture(['add', '-f', target_filename], cwd=patch_dir) the generated patch file that is being added into the repo should never be in .gitignore. Why '-f'? https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode935 trychange.py:935: if options.git_repo and not vcs_is_git: current_vcs.options.git_repo, to ensure settings from AutomagicalSettings() are picked up.
Mostly done except "current_vcs.options.git_repo" thing: IIUC, we either should not use options variable in that method at all but use current_vcs.options, or it is the same thing (which seems to be the case) https://codereview.chromium.org/184343003/diff/20001/scm.py File scm.py (right): https://codereview.chromium.org/184343003/diff/20001/scm.py#newcode398 scm.py:398: return GIT.Capture(['rev-parse', '--git-dir'], cwd=cwd) On 2014/03/04 21:45:59, agable wrote: > Note that this returns a relative path if you're adjacent to the .git directory, > and an absolute path otherwise. Done. https://codereview.chromium.org/184343003/diff/20001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode410 trychange.py:410: _SendChangeGit.""" On 2014/03/04 21:45:59, agable wrote: > nit: multiline docstring formatting. Done. https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode485 trychange.py:485: Args: On 2014/03/04 21:45:59, agable wrote: > Should be a complete list of args, not just the confusing ones. Done. https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode490 trychange.py:490: # Create a temporary file and put description into it On 2014/03/04 21:45:59, agable wrote: > nit: sentence-like comments should have periods. Done. https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode619 trychange.py:619: target_filename = os.path.abspath(target_filename) On 2014/03/04 21:45:59, agable wrote: > Combine this into one statement, with indentation at +4 on the second line. Done. https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode622 trychange.py:622: scm.GIT.Capture(['add', '-f', target_filename], cwd=patch_dir) On 2014/03/04 21:45:59, agable wrote: > the generated patch file that is being added into the repo should never be in > .gitignore. Why '-f'? Done. https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode935 trychange.py:935: if options.git_repo and not vcs_is_git: On 2014/03/04 21:45:59, agable wrote: > current_vcs.options.git_repo, to ensure settings from AutomagicalSettings() are > picked up. AFAIU, SCM/GIT classes do not clone options, so `options is current_vcs.options`
Interesting. Would this be a way forward for the bisect bot stuff? We could give the bots their own 'perf-try' repo that they have write access to?
https://codereview.chromium.org/184343003/diff/20001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode935 trychange.py:935: if options.git_repo and not vcs_is_git: On 2014/03/04 22:51:19, nodir wrote: > On 2014/03/04 21:45:59, agable wrote: > > current_vcs.options.git_repo, to ensure settings from AutomagicalSettings() > are > > picked up. > > AFAIU, SCM/GIT classes do not clone options, so `options is current_vcs.options` Yes, that's correct. current_vcs.options === options, and the current code is correct. I mostly want to make the code more *obviously* correct, by not relying on magical side-effects of AutomagicalSettings() affecting things outside the current_vcs object.
https://codereview.chromium.org/184343003/diff/20001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode935 trychange.py:935: if options.git_repo and not vcs_is_git: On 2014/03/05 19:49:50, agable wrote: > On 2014/03/04 22:51:19, nodir wrote: > > On 2014/03/04 21:45:59, agable wrote: > > > current_vcs.options.git_repo, to ensure settings from AutomagicalSettings() > > are > > > picked up. > > > > AFAIU, SCM/GIT classes do not clone options, so `options is > current_vcs.options` > > Yes, that's correct. current_vcs.options === options, and the current code is > correct. I mostly want to make the code more *obviously* correct, by not relying > on magical side-effects of AutomagicalSettings() affecting things outside the > current_vcs object. Done.
On 2014/03/05 20:07:00, nodir wrote: > https://codereview.chromium.org/184343003/diff/20001/trychange.py > File trychange.py (right): > > https://codereview.chromium.org/184343003/diff/20001/trychange.py#newcode935 > trychange.py:935: if options.git_repo and not vcs_is_git: > On 2014/03/05 19:49:50, agable wrote: > > On 2014/03/04 22:51:19, nodir wrote: > > > On 2014/03/04 21:45:59, agable wrote: > > > > current_vcs.options.git_repo, to ensure settings from > AutomagicalSettings() > > > are > > > > picked up. > > > > > > AFAIU, SCM/GIT classes do not clone options, so `options is > > current_vcs.options` > > > > Yes, that's correct. current_vcs.options === options, and the current code is > > correct. I mostly want to make the code more *obviously* correct, by not > relying > > on magical side-effects of AutomagicalSettings() affecting things outside the > > current_vcs object. > > Done. ping
+navabi
Sorry for the late drive-by comments, but why do you/we want to store patches in a git repo? Back in the day, we used to use an svn repo for trychange, and supporting both 'patches outside of the review tool' (aka git-try/git-wktry) and 'patches inside the review tool') (gcl try) went quite some ways to making the upload / try job code convoluted and hard to maintain. This seems like rewinding the clock in some ways. Can we not use the 'git-cl try' mechanisms somehow w/ reitveld and/or gerrit?
This is a temporary solution for those projects that use Gerrit. At the moment git-cl-try/tryServer don't support Gerrit. We will definitely implement this in future, but QuickOffice team needs a tryserver ASAP. AFAIU, storing patches in Git is less evil than storing them in SVN because * Git runs on machines we don't have to maintain * it is more secure -Nodir On Mon, Mar 10, 2014 at 7:36 PM, <dpranke@chromium.org> wrote: > Sorry for the late drive-by comments, but why do you/we want to store > patches in > a git repo? > > Back in the day, we used to use an svn repo for trychange, and supporting > both > 'patches outside of the review tool' (aka git-try/git-wktry) and 'patches > inside > the review tool') (gcl try) went quite some ways to making the upload / > try job > code convoluted and hard to maintain. > > This seems like rewinding the clock in some ways. Can we not use the > 'git-cl > try' mechanisms somehow w/ reitveld and/or gerrit? > > https://codereview.chromium.org/184343003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Have you used this successfully in an end-to-end test, posting a patch to the git repo and pulling that patch using a locally-running tryserver?
On 2014/03/17 23:15:42, agable wrote: > Have you used this successfully in an end-to-end test, posting a patch to the > git repo and pulling that patch using a locally-running tryserver? Yes Here is an uploaded patch https://quickoffice-internal.googlesource.com/chromeoffice-try/+/76c64a010e81...
basically lg, with a few comments https://chromiumcodereview.appspot.com/184343003/diff/70001/trychange.py File trychange.py (right): https://chromiumcodereview.appspot.com/184343003/diff/70001/trychange.py#newc... trychange.py:482: def _PrepareDescriptionAndPatchFiles(description, options, callback): this would probably make more sense as a context manager: http://docs.python.org/2/library/contextlib.html https://chromiumcodereview.appspot.com/184343003/diff/70001/trychange.py#newc... trychange.py:510: callback(full_patch_filename, description_file.name) why are you using a callback here? prefer changing this to be a context manager and then use yield https://chromiumcodereview.appspot.com/184343003/diff/70001/trychange.py#newc... trychange.py:544: command = [exe, 'import', '-q', patch_dir, options.svn_repo, '--file', with _PrepateDescriotionAndPatchFiles(...): command = ... https://chromiumcodereview.appspot.com/184343003/diff/70001/trychange.py#newc... trychange.py:601: if not options.git_repo: you may want to persist this in the gitconfig or codereview.settings somehow https://chromiumcodereview.appspot.com/184343003/diff/70001/trychange.py#newc... trychange.py:936: current_vcs = GuessVCS(options, path, file_list) this outputs checkouts[0]?
https://codereview.chromium.org/184343003/diff/70001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/70001/trychange.py#newcode510 trychange.py:510: callback(full_patch_filename, description_file.name) On 2014/03/19 01:28:53, stip wrote: > why are you using a callback here? prefer changing this to be a context manager > and then use yield +1 This wasn't an option last year but it's fine now that we require 2.7.
https://codereview.chromium.org/184343003/diff/70001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/70001/trychange.py#newcode482 trychange.py:482: def _PrepareDescriptionAndPatchFiles(description, options, callback): On 2014/03/19 01:28:53, stip wrote: > this would probably make more sense as a context manager: > http://docs.python.org/2/library/contextlib.html Done. https://codereview.chromium.org/184343003/diff/70001/trychange.py#newcode601 trychange.py:601: if not options.git_repo: On 2014/03/19 01:28:53, stip wrote: > you may want to persist this in the gitconfig or codereview.settings somehow I read it from codereview.settings, see line 168 https://codereview.chromium.org/184343003/diff/70001/trychange.py#newcode936 trychange.py:936: current_vcs = GuessVCS(options, path, file_list) On 2014/03/19 01:28:53, stip wrote: > this outputs checkouts[0]? changed checkouts variable initialization
https://codereview.chromium.org/184343003/diff/90001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/90001/trychange.py#newcode524 trychange.py:524: yield full_patch_filename, description_file.name If description_file.name is to be opened by another process, it'll likely won't work on Windows due to the way NamedTemporaryFile() works (it doesn't allow file share). In this case you'll have to revert to mkstemp(). Note that the previous code seemed to work so it may be fine. Use a sensible prefix= value for it and also for mkdtemp call. https://codereview.chromium.org/184343003/diff/90001/trychange.py#newcode569 trychange.py:569: """Get a path to a Git repo with patches with latest changes, Imperattive everywhere. https://codereview.chromium.org/184343003/diff/90001/trychange.py#newcode605 trychange.py:605: scm.GIT.Capture(['pull', 'origin', 'master'], cwd=patch_dir) Technically, you don't want to pull other's patches ? https://codereview.chromium.org/184343003/diff/90001/trychange.py#newcode642 trychange.py:642: scm.GIT.Capture(['push', 'origin', 'HEAD'], cwd=patch_dir) Pushing on origin/HEAD? Usually you push to a branch otherwise the change may be lost.
This issue becomes urgent because I've finally got the servers, which was the bottleneck https://codereview.chromium.org/184343003/diff/90001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/90001/trychange.py#newcode524 trychange.py:524: yield full_patch_filename, description_file.name On 2014/03/25 01:42:56, M-A Ruel wrote: > If description_file.name is to be opened by another process, it'll likely won't > work on Windows due to the way NamedTemporaryFile() works (it doesn't allow file > share). In this case you'll have to revert to mkstemp(). Note that the previous > code seemed to work so it may be fine. > > Use a sensible prefix= value for it and also for mkdtemp call. > Done. https://codereview.chromium.org/184343003/diff/90001/trychange.py#newcode569 trychange.py:569: """Get a path to a Git repo with patches with latest changes, On 2014/03/25 01:42:56, M-A Ruel wrote: > Imperattive everywhere. Not sure I got it. Isn't "Get a patch..." imperative? https://codereview.chromium.org/184343003/diff/90001/trychange.py#newcode605 trychange.py:605: scm.GIT.Capture(['pull', 'origin', 'master'], cwd=patch_dir) On 2014/03/25 01:42:56, M-A Ruel wrote: > Technically, you don't want to pull other's patches ? Probably this is safer because the push won't be fast-forward https://codereview.chromium.org/184343003/diff/90001/trychange.py#newcode642 trychange.py:642: scm.GIT.Capture(['push', 'origin', 'HEAD'], cwd=patch_dir) On 2014/03/25 01:42:56, M-A Ruel wrote: > Pushing on origin/HEAD? Usually you push to a branch otherwise the change may be > lost. Done.
https://codereview.chromium.org/184343003/diff/110001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode494 trychange.py:494: """Create a temporary directory, append the specified name and yield. Imperative verb tense means "Creates" instead of "Create". https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode515 trychange.py:515: remove one line https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode550 trychange.py:550: if sys.platform == "cygwin": This file uses single quotes, please keep it consistent. https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode607 trychange.py:607: scm.GIT.Capture(['pull', 'origin', 'master'], cwd=patch_dir) One issue I see here is that do you want the user to pull everyone else patches. So you should have a branch with the empty commit to fetch. https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode644 trychange.py:644: scm.GIT.Capture(['push', 'origin', 'master'], cwd=patch_dir) This is problematic, since it creates a race condition when two devs trigger try jobs simultaneously. Each one should create a unique (or user name based) branch so it doesn't interfere with others.
cmp, Gerrit does not allow to create new branches when I do push. Is it a matter of changing settings? -Nodir On Wed, Mar 26, 2014 at 5:57 AM, <maruel@chromium.org> wrote: > > https://codereview.chromium.org/184343003/diff/110001/trychange.py > File trychange.py (right): > > https://codereview.chromium.org/184343003/diff/110001/ > trychange.py#newcode494 > trychange.py:494: """Create a temporary directory, append the specified > name and yield. > Imperative verb tense means "Creates" instead of "Create". > > https://codereview.chromium.org/184343003/diff/110001/ > trychange.py#newcode515 > trychange.py:515: > remove one line > > https://codereview.chromium.org/184343003/diff/110001/ > trychange.py#newcode550 > trychange.py:550: if sys.platform == "cygwin": > This file uses single quotes, please keep it consistent. > > https://codereview.chromium.org/184343003/diff/110001/ > trychange.py#newcode607 > trychange.py:607: scm.GIT.Capture(['pull', 'origin', 'master'], > cwd=patch_dir) > One issue I see here is that do you want the user to pull everyone else > patches. So you should have a branch with the empty commit to fetch. > > https://codereview.chromium.org/184343003/diff/110001/ > trychange.py#newcode644 > trychange.py:644: scm.GIT.Capture(['push', 'origin', 'master'], > cwd=patch_dir) > This is problematic, since it creates a race condition when two devs > trigger try jobs simultaneously. Each one should create a unique (or > user name based) branch so it doesn't interfere with others. > > https://codereview.chromium.org/184343003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/184343003/diff/110001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode599 trychange.py:599: scm.GIT.Capture(['clone', git_url, GIT_PATCH_DIR_BASENAME], cwd=git_dir) The patch-specific refs are going to be non-branch refs (i.e. refs/patches/<unique id>), so this should be okay. If you decide to put each patch on its own branch (i.e. refs/heads/<unique id>) then the clone is a bad idea. https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode607 trychange.py:607: scm.GIT.Capture(['pull', 'origin', 'master'], cwd=patch_dir) On 2014/03/26 12:57:10, M-A Ruel wrote: > One issue I see here is that do you want the user to pull everyone else patches. > So you should have a branch with the empty commit to fetch. I think the plan is for origin/master to contain a single file that gets updated once per patch, and for each patch to live on its own ref that isn't in the default fetch refspec. https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode639 trychange.py:639: try: This whole block needs to be changed to commit the patch on a new unique ref.
https://codereview.chromium.org/184343003/diff/110001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode494 trychange.py:494: """Create a temporary directory, append the specified name and yield. On 2014/03/26 12:57:10, M-A Ruel wrote: > Imperative verb tense means "Creates" instead of "Create". A lot of methods in this file uses non-imperative tense. I will update them all in another CL https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode515 trychange.py:515: On 2014/03/26 12:57:10, M-A Ruel wrote: > remove one line Done. https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode550 trychange.py:550: if sys.platform == "cygwin": On 2014/03/26 12:57:10, M-A Ruel wrote: > This file uses single quotes, please keep it consistent. There is a lot of double quotes in this file. I will update them all in a separate CL. https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode607 trychange.py:607: scm.GIT.Capture(['pull', 'origin', 'master'], cwd=patch_dir) On 2014/03/26 12:57:10, M-A Ruel wrote: > One issue I see here is that do you want the user to pull everyone else patches. > So you should have a branch with the empty commit to fetch. I have a CL locally that does something similar: trychange.py creates a new branch with a unique name, puts one file there "patch.diff". In order for GitPoller to notice the change, it also updates special file "branch" in the master, so the buildbot master sees the new patches. The "branch" file contains the name of the branch. As a result, the developer machine never pulls other's patches. The buidlbot master listens to changes in the master branch. For each new change, it reads the branch file content at a certain commit, creates a new build request and specifies the patch branch name in properties. As a result, the buildbot master never pulls patches. A slave reads the "branch" property and pulls only that branch, reads the patch.diff and applies it. The slaves machine don't pull other's patches. The problem with this approach that I faced yesterday, is that Gerrit didn't allot me to push new branches. https://codereview.chromium.org/184343003/diff/110001/trychange.py#newcode644 trychange.py:644: scm.GIT.Capture(['push', 'origin', 'master'], cwd=patch_dir) On 2014/03/26 12:57:10, M-A Ruel wrote: > This is problematic, since it creates a race condition when two devs trigger try > jobs simultaneously. Each one should create a unique (or user name based) branch > so it doesn't interfere with others. See above.
agable, here is what I see:
Command git push origin master:master
nodir.diff#e8c95.2014-03-26-10.20.18.828554.diff:nodir.diff#e8c95.2014-03-26-10.20.18.828554.diff
returned non-zero exit status 1 in
/usr/local/google/home/nodir/cr/quickoffice/src/.git/git-try/patches-git
remote: Resolving deltas: 100% (1/1)
remote: Processing changes: refs: 1, done
To https://quickoffice-internal.googlesource.com/chromeoffice-try
fb222cd..7f305c1 master -> master
! [remote rejected] nodir.diff#e8c95.2014-03-26-10.20.18.828554.diff ->
nodir.diff#e8c95.2014-03-26-10.20.18.828554.diff (prohibited by Gerrit)
error: failed to push some refs to
'https://quickoffice-internal.googlesource.com/chromeoffice-try'
Permissions where granted
Ok about the plan, I'll let Aaron complete the review.
Uploaded the patch to this CL. It is difficult to see the changes in a separate CL.
some comments https://chromiumcodereview.appspot.com/184343003/diff/110001/trychange.py File trychange.py (right): https://chromiumcodereview.appspot.com/184343003/diff/110001/trychange.py#new... trychange.py:172: # primarily for revision=auto nit: Capitalize comments and end with a period. https://chromiumcodereview.appspot.com/184343003/diff/110001/trychange.py#new... trychange.py:512: """Create temporary files with description and patch, yield execution, nit: """One line with short desciption < 80 chars. More lines. """
On 2014/03/26 19:16:36, stip wrote: > some comments > > https://chromiumcodereview.appspot.com/184343003/diff/110001/trychange.py > File trychange.py (right): > > https://chromiumcodereview.appspot.com/184343003/diff/110001/trychange.py#new... > trychange.py:172: # primarily for revision=auto > nit: Capitalize comments and end with a period. > > https://chromiumcodereview.appspot.com/184343003/diff/110001/trychange.py#new... > trychange.py:512: """Create temporary files with description and patch, yield > execution, > nit: """One line with short desciption < 80 chars. > > More lines. > """ Note that other parts of this file, not touched by this CL, violate some style rules. I will fix them in a separate CL.
https://codereview.chromium.org/184343003/diff/160001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/160001/trychange.py#newcode571 trychange.py:571: """Get a path to a Git repo with patches with latest changes, Just a single line for the first line of a docstring. https://codereview.chromium.org/184343003/diff/160001/trychange.py#newcode584 trychange.py:584: def do_git(*args, **kwargs): the name of this function doesn't clarify that it is "doing git" *inside the patch dir*. I'd argue that this isn't necessary or helpful enough to be worth it. Especially since you then explicitly override the cwd the first time you call it. https://codereview.chromium.org/184343003/diff/160001/trychange.py#newcode633 trychange.py:633: def do_git(*args): If you're defining the same inner function multiple times, it should be defined once on the outside. Again, I think this isn't worth it. GIT.capture is already the right level of abstraction. https://codereview.chromium.org/184343003/diff/160001/trychange.py#newcode645 trychange.py:645: target_branch = ('refs/tryserver/' + I'd say refs/patches/, since this system could potentially be in use beyond the tryserver. https://codereview.chromium.org/184343003/diff/160001/trychange.py#newcode649 trychange.py:649: try: This try is around a lot of different calls, only some of which actually touch the network. The try/except block should have much finer granularity. For example, a CalledProcessError raised by 'git reset' should not result in a NoTryServerAccess error. https://codereview.chromium.org/184343003/diff/160001/trychange.py#newcode681 trychange.py:681: def _retry(action, fixup=None, attempts=3): There are already other retry mechanisms floating around, there's probably not a need to actually implement a new one. And, this method is only called once, so I would just inline it. In fact, the whole mess of inner methods in _SendChangeGit only exist so that they can be passed to this function. Remove it and you can streamline _SendChangeGit and make it much more straight-line code.
https://codereview.chromium.org/184343003/diff/160001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/160001/trychange.py#newcode571 trychange.py:571: """Get a path to a Git repo with patches with latest changes, On 2014/03/26 20:27:04, agable wrote: > Just a single line for the first line of a docstring. Done. https://codereview.chromium.org/184343003/diff/160001/trychange.py#newcode584 trychange.py:584: def do_git(*args, **kwargs): On 2014/03/26 20:27:04, agable wrote: > the name of this function doesn't clarify that it is "doing git" *inside the > patch dir*. I'd argue that this isn't necessary or helpful enough to be worth > it. Especially since you then explicitly override the cwd the first time you > call it. Removed the function https://codereview.chromium.org/184343003/diff/160001/trychange.py#newcode633 trychange.py:633: def do_git(*args): On 2014/03/26 20:27:04, agable wrote: > If you're defining the same inner function multiple times, it should be defined > once on the outside. Again, I think this isn't worth it. GIT.capture is already > the right level of abstraction. Renamed this one to patch_git. Still, trying to avoid repetition. The reasons why do_git was in two different places were 1) it closes over patch_dir 2) it is small https://codereview.chromium.org/184343003/diff/160001/trychange.py#newcode645 trychange.py:645: target_branch = ('refs/tryserver/' + On 2014/03/26 20:27:04, agable wrote: > I'd say refs/patches/, since this system could potentially be in use beyond the > tryserver. Done.
ping
lgtm % mine and aaron's comments https://chromiumcodereview.appspot.com/184343003/diff/210001/trychange.py File trychange.py (right): https://chromiumcodereview.appspot.com/184343003/diff/210001/trychange.py#new... trychange.py:895: group.add_option("--use_git", it doesn't look like this option is used, please delete
Message was sent while issue was closed.
Committed patchset #11 manually as r260471.
Message was sent while issue was closed.
stip@ Just noticed that I didn't send my comment https://codereview.chromium.org/184343003/diff/160001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/160001/trychange.py#newcode649 trychange.py:649: try: On 2014/03/26 20:27:04, agable wrote: > This try is around a lot of different calls, only some of which actually touch > the network. The try/except block should have much finer granularity. For > example, a CalledProcessError raised by 'git reset' should not result in a > NoTryServerAccess error. Done. https://codereview.chromium.org/184343003/diff/160001/trychange.py#newcode681 trychange.py:681: def _retry(action, fixup=None, attempts=3): On 2014/03/26 20:27:04, agable wrote: > There are already other retry mechanisms floating around, there's probably not a > need to actually implement a new one. And, this method is only called once, so I > would just inline it. In fact, the whole mess of inner methods in _SendChangeGit > only exist so that they can be passed to this function. Remove it and you can > streamline _SendChangeGit and make it much more straight-line code. Done. https://codereview.chromium.org/184343003/diff/210001/trychange.py File trychange.py (right): https://codereview.chromium.org/184343003/diff/210001/trychange.py#newcode895 trychange.py:895: group.add_option("--use_git", On 2014/03/28 23:24:08, stip wrote: > it doesn't look like this option is used, please delete It is used the same way as use_svn. If set, the options.send_patch contains _SendChangeGit. Better to be consistent. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
