Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18328003:
Add a git cache for gclient sync operations. (Closed)
|
Created:
7 years, 5 months ago by iannucci Modified:
7 years, 5 months ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org Visibility:
Public. |
DescriptionAdd a git cache for gclient sync operations.
Instead of cloning straight into place, clones are made to a global cache dir,
and then local (using --shared) clones are made from the cache to the final
resting place. This means the 'final' clones are full repos with no shenanigans,
meaning that branches, commits, etc. all work, which should allow the rest of
the gclient ecosystem to work without change as well.
The primary benefit is, of course, reduced network IO, and a much lower cost for
'clobber' operations (assuming we don't clobber the cache). It also means that
a given bot can have a greater number of checkouts, since the entire git history
will only be stored once per machine, instead of once per checkout.
R=dpranke@chromium.org, szager@chromium.org
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=210024
Patch Set 1 #Patch Set 2 : Use long form of --shared #Patch Set 3 : extra newline #
Total comments: 18
Patch Set 4 : Fix comments #
Total comments: 9
Patch Set 5 : Fix comments #Patch Set 6 : Fix comments, and make cache_dir a top level setting in .gclient #Patch Set 7 : Capture not Run #
Total comments: 7
Patch Set 8 : Make cache_dir .gclient only, and move it to GitWrapper #
Messages
Total messages: 26 (0 generated)
On 2013/07/01 11:06:04, iannucci wrote: One thing I'm not sure about is if this should actually be an option for sync (as it is now), or if it should be a statement in the .gclient file. I'm actually starting to lean towards the latter.
On 2013/07/01 19:54:39, iannucci wrote: > On 2013/07/01 11:06:04, iannucci wrote: > > One thing I'm not sure about is if this should actually be an option for sync > (as it is now), or if it should be a statement in the .gclient file. I'm > actually starting to lean towards the latter. Oh... I should also note that this fixes a bug in the GitFilter: Previously, it would print out lines which simply had a percentage whose value was % 10. This has two flaws: * Some very long operations emit multiple lines which are % 10, leading to weird output. * Some very long operations will trigger the nag timer in between % 10 percentage outputs. The new behavior passes through all lines, and lines which display a percentage are throttled to 1/2Hz. Additionally, it lets you stack a predicate function on top of the git filter behavior to simply drop unwanted lines. This is used to omit printing refs which are already up to date (which happens quite a lot, and for some gerrit repos can actually slow down the overall operation).
+mmoss PTAL :)
I like the idea, as long as it's billed to users as experimental. https://codereview.chromium.org/18328003/diff/5001/gclient.py File gclient.py (right): https://codereview.chromium.org/18328003/diff/5001/gclient.py#newcode1559 gclient.py:1559: options.cache_lock = threading.Lock() options.cache_lock shouldn't be necessary; vivifying a new entry in options.cache_locks is thread-safe and atomic, because threading.Lock is a built-in type. https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode179 gclient_scm.py:179: if (now - self.last_time) >= 2: Maybe parameterize the suppression interval? Two seconds may be a good interval for interactive use; for bots, we might want to make it longer. https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode371 gclient_scm.py:371: current_url = self._Capture(['config', 'remote.origin.url']) Maybe do something here to help people who are switching from non-cached to cached, or vice-versa? https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode731 gclient_scm.py:731: OUT: /cache/chromium.googlesource.com-chromium-src.git Any reason not to mimic the full directory structure? Admittedly it's unlikely, but this would be a naming conflict: https://chromium.googlesource.com/foo/bar.git https://chromium.googlesource.com/foo-bar.git https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode783 gclient_scm.py:783: clone_cmd.append('--shared') Hmm... not sure I agree with the use of --shared. For our use case, it seems to add risk without providing benefit. Here's the best explanation I can find online about the difference between --local (which is implied when cloning from a file path) and --shared: http://lists-archives.com/git/494327-clarify-git-clone-local-shared-reference...
The only users I plan on billing it to are the bots :) The main use here is to get efficient multi-checkout + clobber operations under git. Hence why we can't use --local (because git on windows will do a full object copy). https://codereview.chromium.org/18328003/diff/5001/gclient.py File gclient.py (right): https://codereview.chromium.org/18328003/diff/5001/gclient.py#newcode1559 gclient.py:1559: options.cache_lock = threading.Lock() On 2013/07/01 23:38:29, szager1 wrote: > options.cache_lock shouldn't be necessary; vivifying a new entry in > options.cache_locks is thread-safe and atomic, because threading.Lock is a > built-in type. defaultdict guarantees that item addition is atomic? In that case, I'm happy to ditch the uberlock. https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode179 gclient_scm.py:179: if (now - self.last_time) >= 2: On 2013/07/01 23:38:29, szager1 wrote: > Maybe parameterize the suppression interval? Two seconds may be a good interval > for interactive use; for bots, we might want to make it longer. I was making it shorter explicitly FOR the bots. Maybe nag_timer / 2? https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode371 gclient_scm.py:371: current_url = self._Capture(['config', 'remote.origin.url']) On 2013/07/01 23:38:29, szager1 wrote: > Maybe do something here to help people who are switching from non-cached to > cached, or vice-versa? I think that should all work as intended, since the remote url is just different, so all the normal replacement logic should work as before, right? https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode731 gclient_scm.py:731: OUT: /cache/chromium.googlesource.com-chromium-src.git On 2013/07/01 23:38:29, szager1 wrote: > Any reason not to mimic the full directory structure? Admittedly it's unlikely, > but this would be a naming conflict: > > https://chromium.googlesource.com/foo/bar.git > https://chromium.googlesource.com/foo-bar.git Eh... I'm much more inclined to flatten it out, since it makes observing the caches easier (esp. on windows). I could replace '-' with '--' to deal with this conflict though. https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode783 gclient_scm.py:783: clone_cmd.append('--shared') On 2013/07/01 23:38:29, szager1 wrote: > Hmm... not sure I agree with the use of --shared. For our use case, it seems to > add risk without providing benefit. > > Here's the best explanation I can find online about the difference between > --local (which is implied when cloning from a file path) and --shared: > > http://lists-archives.com/git/494327-clarify-git-clone-local-shared-reference... --local won't work correctly on windows, or cross-device (relies on hardlinks). I explicitly wanted shared because I want there to only be one copy of the object per-machine. The normal pitfalls of --shared won't apply here, since we know that the git repos in cache are full mirrors of the remote. Any issues there (i.e. force push of checked out refs) would be exactly the same amount of pain as cloning directly from the remote, and the remedy is the same (clobber the non-cache checkout).
https://codereview.chromium.org/18328003/diff/5001/gclient.py File gclient.py (right): https://codereview.chromium.org/18328003/diff/5001/gclient.py#newcode1559 gclient.py:1559: options.cache_lock = threading.Lock() On 2013/07/01 23:55:52, iannucci wrote: > On 2013/07/01 23:38:29, szager1 wrote: > > options.cache_lock shouldn't be necessary; vivifying a new entry in > > options.cache_locks is thread-safe and atomic, because threading.Lock is a > > built-in type. > > defaultdict guarantees that item addition is atomic? > > In that case, I'm happy to ditch the uberlock. As long as the __init__ method for the subject class is atomic (which is true for all built-in types, including threading.Lock), defaultdict is thread-safe. https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode179 gclient_scm.py:179: if (now - self.last_time) >= 2: On 2013/07/01 23:55:52, iannucci wrote: > On 2013/07/01 23:38:29, szager1 wrote: > > Maybe parameterize the suppression interval? Two seconds may be a good > interval > > for interactive use; for bots, we might want to make it longer. > > I was making it shorter explicitly FOR the bots. Maybe nag_timer / 2? That would be fine, or even something like 10; but two seconds will cause an awful lot of log file gas for clobber checkouts. https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode371 gclient_scm.py:371: current_url = self._Capture(['config', 'remote.origin.url']) On 2013/07/01 23:55:52, iannucci wrote: > On 2013/07/01 23:38:29, szager1 wrote: > > Maybe do something here to help people who are switching from non-cached to > > cached, or vice-versa? > > I think that should all work as intended, since the remote url is just > different, so all the normal replacement logic should work as before, right? Actually, what I meant was that when switching from non-cached to cached, it would be nice if the existing checkout could be used to create the cache, rather than re-cloning from scratch. https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode731 gclient_scm.py:731: OUT: /cache/chromium.googlesource.com-chromium-src.git On 2013/07/01 23:55:52, iannucci wrote: > On 2013/07/01 23:38:29, szager1 wrote: > > Any reason not to mimic the full directory structure? Admittedly it's > unlikely, > > but this would be a naming conflict: > > > > https://chromium.googlesource.com/foo/bar.git > > https://chromium.googlesource.com/foo-bar.git > > Eh... I'm much more inclined to flatten it out, since it makes observing the > caches easier (esp. on windows). I could replace '-' with '--' to deal with this > conflict though. I'd be satisfied with collision detection logic. https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode783 gclient_scm.py:783: clone_cmd.append('--shared') On 2013/07/01 23:55:52, iannucci wrote: > On 2013/07/01 23:38:29, szager1 wrote: > > Hmm... not sure I agree with the use of --shared. For our use case, it seems > to > > add risk without providing benefit. > > > > Here's the best explanation I can find online about the difference between > > --local (which is implied when cloning from a file path) and --shared: > > > > > http://lists-archives.com/git/494327-clarify-git-clone-local-shared-reference... > > --local won't work correctly on windows, or cross-device (relies on hardlinks). > I explicitly wanted shared because I want there to only be one copy of the > object per-machine. > > The normal pitfalls of --shared won't apply here, since we know that the git > repos in cache are full mirrors of the remote. Any issues there (i.e. force push > of checked out refs) would be exactly the same amount of pain as cloning > directly from the remote, and the remedy is the same (clobber the non-cache > checkout). Fair enough!
PTAL :) https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode179 gclient_scm.py:179: if (now - self.last_time) >= 2: On 2013/07/02 00:08:05, szager1 wrote: > On 2013/07/01 23:55:52, iannucci wrote: > > On 2013/07/01 23:38:29, szager1 wrote: > > > Maybe parameterize the suppression interval? Two seconds may be a good > > interval > > > for interactive use; for bots, we might want to make it longer. > > > > I was making it shorter explicitly FOR the bots. Maybe nag_timer / 2? > > That would be fine, or even something like 10; but two seconds will cause an > awful lot of log file gas for clobber checkouts. Ok, I set it to nag_timer / 2 (which should be 15 seconds in the default case). https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode371 gclient_scm.py:371: current_url = self._Capture(['config', 'remote.origin.url']) On 2013/07/02 00:08:05, szager1 wrote: > On 2013/07/01 23:55:52, iannucci wrote: > > On 2013/07/01 23:38:29, szager1 wrote: > > > Maybe do something here to help people who are switching from non-cached to > > > cached, or vice-versa? > > > > I think that should all work as intended, since the remote url is just > > different, so all the normal replacement logic should work as before, right? > > Actually, what I meant was that when switching from non-cached to cached, it > would be nice if the existing checkout could be used to create the cache, rather > than re-cloning from scratch. Ah! Yeah we could do that with --reference, I think. Then we could repack -a once the mirror was done. Good idea. I'll do this in a followup cl :) https://codereview.chromium.org/18328003/diff/5001/gclient_scm.py#newcode731 gclient_scm.py:731: OUT: /cache/chromium.googlesource.com-chromium-src.git On 2013/07/02 00:08:05, szager1 wrote: > On 2013/07/01 23:55:52, iannucci wrote: > > On 2013/07/01 23:38:29, szager1 wrote: > > > Any reason not to mimic the full directory structure? Admittedly it's > > unlikely, > > > but this would be a naming conflict: > > > > > > https://chromium.googlesource.com/foo/bar.git > > > https://chromium.googlesource.com/foo-bar.git > > > > Eh... I'm much more inclined to flatten it out, since it makes observing the > > caches easier (esp. on windows). I could replace '-' with '--' to deal with > this > > conflict though. > > I'd be satisfied with collision detection logic. Done.
does this LGTY? I'd like to get this in so I can start playing with it on the bots.
On 2013/07/02 00:59:15, iannucci wrote: > does this LGTY? I'd like to get this in so I can start playing with it on the > bots. 'lo? Anyone? :/
lgtm https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py File gclient_scm.py (right): https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py#new... gclient_scm.py:159: """Create a new GitFilter. I tend to overdo code comments, but even I'm offended by this one. Then again, we do have """Wrapper for Git""" for the GitWrapper class right below :/ https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py#new... gclient_scm.py:161: Args: When documenting args, we generally document them all (doesn't pylint complain if you don't?). https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py#new... gclient_scm.py:163: The line will be skipped if the predicate is False. At first I read this as "if predicate == False". Probably should say "if the predicate returns False."
https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py File gclient_scm.py (right): https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py#new... gclient_scm.py:764: # --progress, so use fetch instead. existing_url = self._Run(['config', 'remote.origin.url']) if existing_url != url: raise ...
Fixed stuff. See comment re: exception for not-matching urls :) https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py File gclient_scm.py (right): https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py#new... gclient_scm.py:159: """Create a new GitFilter. On 2013/07/02 20:47:52, Michael Moss wrote: > I tend to overdo code comments, but even I'm offended by this one. Then again, > we do have """Wrapper for Git""" for the GitWrapper class right below :/ Yeah, the state of docstrings in this file is fairly lamentable :/ I'll put something more reasonable here... https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py#new... gclient_scm.py:161: Args: On 2013/07/02 20:47:52, Michael Moss wrote: > When documenting args, we generally document them all (doesn't pylint complain > if you don't?). Oops! Done. https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py#new... gclient_scm.py:163: The line will be skipped if the predicate is False. On 2013/07/02 20:47:52, Michael Moss wrote: > At first I read this as "if predicate == False". Probably should say "if the > predicate returns False." Done. https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py#new... gclient_scm.py:764: # --progress, so use fetch instead. On 2013/07/02 20:58:21, szager1 wrote: > existing_url = self._Run(['config', 'remote.origin.url']) > if existing_url != url: > raise ... Why? The premise is that it doesn't matter what access method is used for a given domain+path. This'll let us make the cache repos use whatever the best access method/remotes we want on the bots later (i.e. for forming a mesh of git mirrors in the golo, switching to a different auth scheme if necessary, etc.).
https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py File gclient_scm.py (right): https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py#new... gclient_scm.py:764: # --progress, so use fetch instead. On 2013/07/02 22:10:07, iannucci wrote: > On 2013/07/02 20:58:21, szager1 wrote: > > existing_url = self._Run(['config', 'remote.origin.url']) > > if existing_url != url: > > raise ... > > Why? The premise is that it doesn't matter what access method is used for a > given domain+path. This'll let us make the cache repos use whatever the best > access method/remotes we want on the bots later (i.e. for forming a mesh of git > mirrors in the golo, switching to a different auth scheme if necessary, etc.). This is the collision-detection logic. Maybe strip out the hostname and just match the path; it's unlikely that we'll be switching paths.
On 2013/07/02 22:14:04, szager1 wrote: > https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py > File gclient_scm.py (right): > > https://chromiumcodereview.appspot.com/18328003/diff/11001/gclient_scm.py#new... > gclient_scm.py:764: # --progress, so use fetch instead. > On 2013/07/02 22:10:07, iannucci wrote: > > On 2013/07/02 20:58:21, szager1 wrote: > > > existing_url = self._Run(['config', 'remote.origin.url']) > > > if existing_url != url: > > > raise ... > > > > Why? The premise is that it doesn't matter what access method is used for a > > given domain+path. This'll let us make the cache repos use whatever the best > > access method/remotes we want on the bots later (i.e. for forming a mesh of > git > > mirrors in the golo, switching to a different auth scheme if necessary, etc.). > > This is the collision-detection logic. Maybe strip out the hostname and just > match the path; it's unlikely that we'll be switching paths. I'm confused... How /could/ there be a collision? We just generated the folder name from url? If the folder exists, then by definition, it's referring to the same repo (modulo access method)...
lgtm > I'm confused... How /could/ there be a collision? We just generated the folder > name from url? If the folder exists, then by definition, it's referring to the > same repo (modulo access method)... Fair enough, I guess this is too pathological to actually happen: url1 = 'https://chromium.googlesource.com/foo-/bar' url2 = 'https://chromium.googlesource.com/foo/-bar' Incidentally, I think this is a feature that *should* be advertised to developers. One of the common complaints I hear from people using git on windows is that they have to maintain multiple full checkouts, because they want to be able to develop one branch while compiling another. I know some Windows power-users who would probably jump on this.
On 2013/07/02 22:44:50, szager1 wrote: > lgtm > > > I'm confused... How /could/ there be a collision? We just generated the folder > > name from url? If the folder exists, then by definition, it's referring to the > > same repo (modulo access method)... > > Fair enough, I guess this is too pathological to actually happen: > > url1 = 'https://chromium.googlesource.com/foo-/bar' > url2 = 'https://chromium.googlesource.com/foo/-bar' Ew... Ok... that would be pretty ugly :D. I'll add a check. Good thinking. > > Incidentally, I think this is a feature that *should* be advertised to > developers. One of the common complaints I hear from people using git on > windows is that they have to maintain multiple full checkouts, because they want > to be able to develop one branch while compiling another. I know some Windows > power-users who would probably jump on this. Ok, that makes sense to me. I think seeding the cache repos from the existing repos would be a requirement for that sort of thing then. How about we land this one, I add the seeding (it should actually be pretty easy, I think), and then when we think it's working 'for reals' (maybe you, me, and others can dogfood it, and I'll be watching it on the git-based bots), then we can send the PSA?
On 2013/07/02 22:48:45, iannucci wrote: > On 2013/07/02 22:44:50, szager1 wrote: > > lgtm > > > > > I'm confused... How /could/ there be a collision? We just generated the > folder > > > name from url? If the folder exists, then by definition, it's referring to > the > > > same repo (modulo access method)... > > > > Fair enough, I guess this is too pathological to actually happen: > > > > url1 = 'https://chromium.googlesource.com/foo-/bar' > > url2 = 'https://chromium.googlesource.com/foo/-bar' > > Ew... Ok... that would be pretty ugly :D. > > I'll add a check. Good thinking. > > > > > Incidentally, I think this is a feature that *should* be advertised to > > developers. One of the common complaints I hear from people using git on > > windows is that they have to maintain multiple full checkouts, because they > want > > to be able to develop one branch while compiling another. I know some Windows > > power-users who would probably jump on this. > > Ok, that makes sense to me. I think seeding the cache repos from the existing > repos would be a requirement for that sort of thing then. How about we land this > one, I add the seeding (it should actually be pretty easy, I think), and then > when we think it's working 'for reals' (maybe you, me, and others can dogfood > it, and I'll be watching it on the git-based bots), then we can send the PSA? Oh, also: What do you think about adding the cache_dir to the .gclient? I'm thinking it'll get really tedious to add it on the command line every time you do a sync.
On 2013/07/02 22:49:31, iannucci wrote: > > Ok, that makes sense to me. I think seeding the cache repos from the existing > > repos would be a requirement for that sort of thing then. How about we land > this > > one, I add the seeding (it should actually be pretty easy, I think), and then > > when we think it's working 'for reals' (maybe you, me, and others can dogfood > > it, and I'll be watching it on the git-based bots), then we can send the PSA? sgtm > Oh, also: What do you think about adding the cache_dir to the .gclient? I'm > thinking it'll get really tedious to add it on the command line every time you > do a sync. That is an absolute must-have for devs.
On 2013/07/02 23:04:40, szager1 wrote: > On 2013/07/02 22:49:31, iannucci wrote: > > > > Ok, that makes sense to me. I think seeding the cache repos from the > existing > > > repos would be a requirement for that sort of thing then. How about we land > > this > > > one, I add the seeding (it should actually be pretty easy, I think), and > then > > > when we think it's working 'for reals' (maybe you, me, and others can > dogfood > > > it, and I'll be watching it on the git-based bots), then we can send the > PSA? > > sgtm > > > Oh, also: What do you think about adding the cache_dir to the .gclient? I'm > > thinking it'll get really tedious to add it on the command line every time you > > do a sync. > > That is an absolute must-have for devs. OK, I implemented that in what I think is the least-bad way that gclient allows. PTAL (last one for this CL, I promise!).
lgtm https://chromiumcodereview.appspot.com/18328003/diff/27002/gclient_scm.py File gclient_scm.py (right): https://chromiumcodereview.appspot.com/18328003/diff/27002/gclient_scm.py#new... gclient_scm.py:128: cache_dir = None Since this is git-only, any reason not to put it in GitWrapper (and update references in gclient.py)?
https://chromiumcodereview.appspot.com/18328003/diff/27002/gclient.py File gclient.py (right): https://chromiumcodereview.appspot.com/18328003/diff/27002/gclient.py#newcode901 gclient.py:901: cache_dir = %(cache_dir)r str(None) == repr(None), so for the sake of consistency, maybe make this %(cache_dir)s https://chromiumcodereview.appspot.com/18328003/diff/27002/gclient.py#newcode... gclient.py:1731: parser.add_option('--cache-dir', I think I'd prefer that --cache-dir can only be specified when running 'gclient init'. For all other commands, the setting must come from the .gclient file. I can't think of a good reason to allow people to override --cache-dir on individual gclient invocations, and it creates an opportunity for lots of mistakes. https://chromiumcodereview.appspot.com/18328003/diff/27002/gclient_scm.py File gclient_scm.py (right): https://chromiumcodereview.appspot.com/18328003/diff/27002/gclient_scm.py#new... gclient_scm.py:128: cache_dir = None On 2013/07/03 14:34:23, Michael Moss wrote: > Since this is git-only, any reason not to put it in GitWrapper (and update > references in gclient.py)? Yeah, I agree with mmoss.
fixed :) https://codereview.chromium.org/18328003/diff/27002/gclient.py File gclient.py (right): https://codereview.chromium.org/18328003/diff/27002/gclient.py#newcode901 gclient.py:901: cache_dir = %(cache_dir)r On 2013/07/03 16:49:52, szager1 wrote: > str(None) == repr(None), so for the sake of consistency, maybe make this > %(cache_dir)s Ah, but str('foobar') != repr('foobar') :) we need the quotes. https://codereview.chromium.org/18328003/diff/27002/gclient.py#newcode1731 gclient.py:1731: parser.add_option('--cache-dir', On 2013/07/03 16:49:52, szager1 wrote: > I think I'd prefer that --cache-dir can only be specified when running 'gclient > init'. For all other commands, the setting must come from the .gclient file. I > can't think of a good reason to allow people to override --cache-dir on > individual gclient invocations, and it creates an opportunity for lots of > mistakes. Fair enough. https://codereview.chromium.org/18328003/diff/27002/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/18328003/diff/27002/gclient_scm.py#newcode128 gclient_scm.py:128: cache_dir = None On 2013/07/03 16:49:52, szager1 wrote: > On 2013/07/03 14:34:23, Michael Moss wrote: > > Since this is git-only, any reason not to put it in GitWrapper (and update > > references in gclient.py)? > > Yeah, I agree with mmoss. ... What a good idea! And THAT's why you Always get code reviews...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iannucci@chromium.org/18328003/45001
Message was sent while issue was closed.
Change committed as 210024 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
