SplitUrlRevision when obtaining the cache_dir in gclient_scm.GetActualRemoteURL

SplitUrlRevision when obtaining the cache_dir in gclient_scm.GetActualRemoteURL

BUG=361155
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=263038

[borenet, 2014-04-09 12:39:41]

Per v.putkinen's suggestion on https://codereview.chromium.org/228793002/

[iannucci, 2014-04-09 17:11:05]

good suggestion. lgtm

[borenet, 2014-04-09 17:23:08]

On 2014/04/09 17:11:05, iannucci wrote:
> good suggestion. lgtm

Thanks, Robbie.  What about the bit about auto-recovering from deleted
cache_dir?

[iannucci, 2014-04-09 17:34:44]

On 2014/04/09 17:23:08, borenet wrote:
> On 2014/04/09 17:11:05, iannucci wrote:
> > good suggestion. lgtm
> 
> Thanks, Robbie.  What about the bit about auto-recovering from deleted
> cache_dir?

So there're a lot of cases. Basically:

cache_dir [exists / deleted] X cache_dir option [enabled / disabled] X .gclient
url [same / different]

times the transitions between those states. I can't really thing about it too
much, but a state diagram is probably in order. If the unit tests weren't
totally gross, I would suggest encoding the state diagram in the tests...

[iannucci, 2014-04-09 17:34:55]

On 2014/04/09 17:34:44, iannucci wrote:
> On 2014/04/09 17:23:08, borenet wrote:
> > On 2014/04/09 17:11:05, iannucci wrote:
> > > good suggestion. lgtm
> > 
> > Thanks, Robbie.  What about the bit about auto-recovering from deleted
> > cache_dir?
> 
> So there're a lot of cases. Basically:
> 
> cache_dir [exists / deleted] X cache_dir option [enabled / disabled] X
.gclient
> url [same / different]
> 
> times the transitions between those states. I can't really thing about it too
> much, but a state diagram is probably in order. If the unit tests weren't
> totally gross, I would suggest encoding the state diagram in the tests...

s/thing/think

[borenet, 2014-04-10 14:11:14]

Uploaded patch set 2. My inclination is to let szager@ or someone else worry
about what happens if the cache_dir disappears.

[v.putkinen, 2014-04-10 14:31:54]

On 2014/04/10 14:11:14, borenet wrote:
> Uploaded patch set 2. My inclination is to let szager@ or someone else worry
> about what happens if the cache_dir disappears.

That is fine by me. If the cache disappears, GetActualRemoteURL no longer
crashes and the URL mismatch error will give a hint about what is wrong.
Besides, I don't expect caches to go missing too often - at least in my use
cases.

[iannucci, 2014-04-10 17:29:08]

On 2014/04/10 14:31:54, v.putkinen wrote:
> On 2014/04/10 14:11:14, borenet wrote:
> > Uploaded patch set 2. My inclination is to let szager@ or someone else worry
> > about what happens if the cache_dir disappears.
> 
> That is fine by me. If the cache disappears, GetActualRemoteURL no longer
> crashes and the URL mismatch error will give a hint about what is wrong.
> Besides, I don't expect caches to go missing too often - at least in my use
> cases.

Yeah, and the bots should never hit this code with a missing cache (there's a
step before gclient which populates all the caches).

[borenet, 2014-04-10 18:48:32]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2014-04-10 18:48:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/borenet@google.com/230773002/20001

[commit-bot: I haz the power, 2014-04-10 18:50:23]

Change committed as 263038