Docserver: Use GitilesFileSystem instead of SubversionFileSystem

Docserver: Use GitilesFileSystem instead of SubversionFileSystem

BUG=404239
NOTRY=True

Committed: https://crrev.com/e380f604301c5cdde7d0fad496a5d46c80233afc
Cr-Commit-Position: refs/heads/master@{#294851}

[ahernandez, 2014-08-20 19:58:49]

Here is my progress so far. It almost works, but I've been stuck on getting
integration_test to finish. When it gets to the render public files stage, it
immediately 404s because of the underlying offline file system. What I can't
figure out is why the caching is messed up, especially since the only major
change is the underlying file system implementation has changed to gitiles
instead of subversion.

Another issue I have is with app_yaml_helper. I added functionality to get the
previous commit ID in gitilesFS, but I don't think that jives well with the
logic in app_yaml_helper, and I'm not sure how I should go about changing it.

[not at google - send to devlin, 2014-08-21 00:46:29]

On 2014/08/20 19:58:49, ahernandez wrote:
> Here is my progress so far. It almost works, but I've been stuck on getting
> integration_test to finish. When it gets to the render public files stage, it
> immediately 404s because of the underlying offline file system. What I can't
> figure out is why the caching is messed up, especially since the only major
> change is the underlying file system implementation has changed to gitiles
> instead of subversion.

Interesting. Perhaps it's a difference in how the test data is being generated?

> 
> Another issue I have is with app_yaml_helper. I added functionality to get the
> previous commit ID in gitilesFS, but I don't think that jives well with the
> logic in app_yaml_helper, and I'm not sure how I should go about changing it.

What doesn't jive well?

[ahernandez, 2014-08-21 17:11:48]

On 2014/08/21 00:46:29, kalman wrote:
> On 2014/08/20 19:58:49, ahernandez wrote:
> > Here is my progress so far. It almost works, but I've been stuck on getting
> > integration_test to finish. When it gets to the render public files stage,
it
> > immediately 404s because of the underlying offline file system. What I can't
> > figure out is why the caching is messed up, especially since the only major
> > change is the underlying file system implementation has changed to gitiles
> > instead of subversion.
> 
> Interesting. Perhaps it's a difference in how the test data is being
generated?

I feel that it's related to the same issues I'm encountering with the OwnersDS
patch. I've been tracing through a lot of caching related code, but no luck so
far in finding the issue.

> > Another issue I have is with app_yaml_helper. I added functionality to get
the
> > previous commit ID in gitilesFS, but I don't think that jives well with the
> > logic in app_yaml_helper, and I'm not sure how I should go about changing
it.
> 
> What doesn't jive well?

I would have to add this new GetPreviousCommitID() method to mock_file_system,
test_file_system, and caching_file_system. And I don't know what
test_file_system should return for previous commit IDs, since it's not fetching
any real data.

[not at google - send to devlin, 2014-08-21 17:31:23]

> I feel that it's related to the same issues I'm encountering with the OwnersDS
> patch. I've been tracing through a lot of caching related code, but no luck so
> far in finding the issue.

I see. Ok, if you upload your latest version of the OwnersDS patch I can play
around with it. There's a chance it's a problem with caching I suppose.

Are there parts of this CL that you can land in the meantime?

> 
> I would have to add this new GetPreviousCommitID() method to mock_file_system,
> test_file_system, and caching_file_system. And I don't know what
> test_file_system should return for previous commit IDs, since it's not
fetching
> any real data.

What is GetPreviousCommitID used for? We could just add it to the FileSystem
interface (similar to what you're trying to do anyway), if there's a suitably
general concept that it maps onto (a concept that makes sense for all
FileSystems)? Or something along those lines?

[ahernandez, 2014-08-21 17:48:02]

> I see. Ok, if you upload your latest version of the OwnersDS patch I can play
> around with it. There's a chance it's a problem with caching I suppose.
> 
> Are there parts of this CL that you can land in the meantime?

I'll upload something for OwnersDS, thanks. It could be that I've been barking
up the wrong tree (I've done it before), so I'd appreciate a fresh pair of eyes
on this.
There are parts I can land right now, I'm currently putting together a CL with
only the working parts.
 

> What is GetPreviousCommitID used for? We could just add it to the FileSystem
> interface (similar to what you're trying to do anyway), if there's a suitably
> general concept that it maps onto (a concept that makes sense for all
> FileSystems)? Or something along those lines?

It's only used in app_yaml_helper (I left a comment there), and the concept of a
previous commit ID is only relevant to GitilesFS as far as I can tell. That's
why I'm hesitant to just add it everywhere.

https://codereview.chromium.org/491653002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/app_yaml_helper.py (right):

https://codereview.chromium.org/491653002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/app_yaml_helper.py:123:
commit=next_file_system.GetPreviousCommitID().Get())
This is the only use case for GetPreviousCommitID().

[ahernandez, 2014-08-21 19:11:55]

I believe I have solved the caching issue! I went through SubversionFS again,
and noticed the comment in GetIdentity() about caching. So I modified
GetIdentity() in GitilesFS to not take commits into account, and it worked.
Integration test finished.

[ahernandez, 2014-08-26 21:53:23]

Patchset #2 (id:40001) has been deleted

[ahernandez, 2014-08-26 21:54:36]

Sorry I'm jumping between CLs. Here is the updated GitilesFS CL, with
terminology changed to reflect our use of Git repositories. 

gitiles_file_system_test.py is not in the diff, but it's tracked on my branch so
I'm not sure what's going on here.

[not at google - send to devlin, 2014-08-26 22:52:24]

Nice. We need to sort out that XXX thing.

One thing I'm a little worried about is the references to 'trunk' in the feature
files, like:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

I didn't see anything that would break those, but I didn't see anything that
might deal with it in the features either?

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/app_yaml_helper.py (right):

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/app_yaml_helper.py:106: return
int(file_system.Stat(APP_YAML).version)
Yeah int-ing this isn't going to work obviously. Could you make
GetPreviousCommitID take the commit to get the previous of? The implementation
of that for SubversionFileSystem would be just int-ify the ID then subtract 1.
The GitilesFileSystem implementation would need to look up the commit history
but that should be straightforward right?

The other place this is assumed to be an int is the "found == 0" call. I don't
even remember what that's supposed to be.

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/app_yaml_helper_test.py (right):

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/app_yaml_helper_test.py:95:
TestFileSystem(test_data, relative_to=SERVER2), updates[:commit])
This won't work either, right? Or does it.. happen to work?

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/warning_master.html
(right):

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/warning_master.html:3: <a
href="http://www.chromium.org/developers/">master</a>.
Maybe let's change this to "from source". master/trunk distinction doesn't mean
a whole lot here.

[ahernandez, 2014-08-26 23:35:56]

My gut tells me the features files should also use 'master' instead of 'trunk',
but I also don't see that stuff being explicitly handled.

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/app_yaml_helper.py (right):

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/app_yaml_helper.py:106: return
int(file_system.Stat(APP_YAML).version)
On 2014/08/26 22:52:23, kalman wrote:
> Yeah int-ing this isn't going to work obviously. Could you make
> GetPreviousCommitID take the commit to get the previous of? The implementation
> of that for SubversionFileSystem would be just int-ify the ID then subtract 1.

Does GetPreviousCommitID need to be implemented in SubversionFS? It's not being
used at all, and I thought it wasn't going to be used in the future either.

> The GitilesFileSystem implementation would need to look up the commit history
> but that should be straightforward right?

Yep, shouldn't be hard at all, I forgot the commit message has the revision
number.

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/app_yaml_helper_test.py (right):

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/app_yaml_helper_test.py:95:
TestFileSystem(test_data, relative_to=SERVER2), updates[:commit])
On 2014/08/26 22:52:23, kalman wrote:
> This won't work either, right? Or does it.. happen to work?

It works because in the test data and test file systems commits are just
integers (stat counts IIRC). All tests are currently passing btw, although that
probably doesn't make you very confident. I plan to run this patch on the dev
server to check if there are any hidden bugs. Still, like you mentioned before
I'm curious to see how this will run on the production server.

[not at google - send to devlin, 2014-08-27 01:07:14]

> My gut tells me the features files should also use 'master' instead of
'trunk',
> but I also don't see that stuff being explicitly handled.

I agree, and we can tackle that separately. We'd need to change some stuff in
the extensions code. It's actually shouldn't really be called 'trunk' in
extensions code, it means 'built from source' so we should be calling it
something like that. Hm.

But I suspect this patch will break that. Try looking at the fileSystemProvider
docs and see what it says.

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/app_yaml_helper.py (right):

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/app_yaml_helper.py:106: return
int(file_system.Stat(APP_YAML).version)
On 2014/08/26 23:35:56, ahernandez wrote:
> On 2014/08/26 22:52:23, kalman wrote:
> > Yeah int-ing this isn't going to work obviously. Could you make
> > GetPreviousCommitID take the commit to get the previous of? The
implementation
> > of that for SubversionFileSystem would be just int-ify the ID then subtract
1.
> 
> Does GetPreviousCommitID need to be implemented in SubversionFS? It's not
being
> used at all, and I thought it wasn't going to be used in the future either.
> 
> > The GitilesFileSystem implementation would need to look up the commit
history
> > but that should be straightforward right?
> 
> Yep, shouldn't be hard at all, I forgot the commit message has the revision
> number.

I was assuming we'd implement all of this functionality which relies on
determining some commit order could use GetPreviousCommitID. I don't even know
if you'd need to look up revision numbers.

I.e. here we assume that the previous commit ID of |found| is |found - 1|.
Really it's GetPreviousCommitID(found), it just happens that Subversion uses
sequential commit IDs.

[ahernandez, 2014-08-27 17:22:35]

> But I suspect this patch will break that. Try looking at the
fileSystemProvider
> docs and see what it says.

fileSystemProvider docs?

> I was assuming we'd implement all of this functionality which relies on
> determining some commit order could use GetPreviousCommitID. I don't even know
> if you'd need to look up revision numbers.
> 
> I.e. here we assume that the previous commit ID of |found| is |found - 1|.
> Really it's GetPreviousCommitID(found), it just happens that Subversion uses
> sequential commit IDs.

Found isn't a commit ID though, so passing it to GetPreviousCommitID doesn't
make sense. It seems easier to leave GetPreviousCommitID the way it is since
it's only used once, and either change the app_yaml_helper logic to not rely on
|found| being an int, or change |found| to not be a Stat version if possible.

[not at google - send to devlin, 2014-08-27 17:35:44]

On 2014/08/27 17:22:35, ahernandez wrote:
> > But I suspect this patch will break that. Try looking at the
> fileSystemProvider
> > docs and see what it says.
> 
> fileSystemProvider docs?

like developer.chrome.com/fileSystemProvider.

> 
> > I was assuming we'd implement all of this functionality which relies on
> > determining some commit order could use GetPreviousCommitID. I don't even
know
> > if you'd need to look up revision numbers.
> > 
> > I.e. here we assume that the previous commit ID of |found| is |found - 1|.
> > Really it's GetPreviousCommitID(found), it just happens that Subversion uses
> > sequential commit IDs.
> 
> Found isn't a commit ID though, so passing it to GetPreviousCommitID doesn't
> make sense. It seems easier to leave GetPreviousCommitID the way it is since
> it's only used once, and either change the app_yaml_helper logic to not rely
on
> |found| being an int, or change |found| to not be a Stat version if possible.

Oh I see. Isn't commit ID the same thing as a stat, more or less? Or rather,
stat uses a commit ID? Can we do that rather than trying to parse the commit
logs and pull out that revision ID?

[ahernandez, 2014-08-27 17:46:28]

> > > I was assuming we'd implement all of this functionality which relies on
> > > determining some commit order could use GetPreviousCommitID. I don't even
> know
> > > if you'd need to look up revision numbers.
> > > 
> > > I.e. here we assume that the previous commit ID of |found| is |found - 1|.
> > > Really it's GetPreviousCommitID(found), it just happens that Subversion
uses
> > > sequential commit IDs.
> > 
> > Found isn't a commit ID though, so passing it to GetPreviousCommitID doesn't
> > make sense. It seems easier to leave GetPreviousCommitID the way it is since
> > it's only used once, and either change the app_yaml_helper logic to not rely
> on
> > |found| being an int, or change |found| to not be a Stat version if
possible.
> 
> Oh I see. Isn't commit ID the same thing as a stat, more or less? Or rather,
> stat uses a commit ID? Can we do that rather than trying to parse the commit
> logs and pull out that revision ID?

Unfortunately not, stat uses tree IDs, which are unrelated to commit IDs.

[not at google - send to devlin, 2014-08-27 19:23:23]

kalman@chromium.org changed reviewers:
+ rockot@chromium.org

[not at google - send to devlin, 2014-08-27 19:23:23]

> Unfortunately not, stat uses tree IDs, which are unrelated to commit IDs.

Ah I don't really understand all of this enough.

Hey Ken can you have a look?

[Ken Rockot(use gerrit already), 2014-08-29 07:24:43]

Looks good in general. Comment about the versioning stuff below:

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/app_yaml_helper.py (right):

https://codereview.chromium.org/491653002/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/app_yaml_helper.py:106: return
int(file_system.Stat(APP_YAML).version)
On 2014/08/27 01:07:13, kalman wrote:
> On 2014/08/26 23:35:56, ahernandez wrote:
> > On 2014/08/26 22:52:23, kalman wrote:
> > > Yeah int-ing this isn't going to work obviously. Could you make
> > > GetPreviousCommitID take the commit to get the previous of? The
> implementation
> > > of that for SubversionFileSystem would be just int-ify the ID then
subtract
> 1.
> > 
> > Does GetPreviousCommitID need to be implemented in SubversionFS? It's not
> being
> > used at all, and I thought it wasn't going to be used in the future either.
> > 
> > > The GitilesFileSystem implementation would need to look up the commit
> history
> > > but that should be straightforward right?
> > 
> > Yep, shouldn't be hard at all, I forgot the commit message has the revision
> > number.
> 
> I was assuming we'd implement all of this functionality which relies on
> determining some commit order could use GetPreviousCommitID. I don't even know
> if you'd need to look up revision numbers.
> 
> I.e. here we assume that the previous commit ID of |found| is |found - 1|.
> Really it's GetPreviousCommitID(found), it just happens that Subversion uses
> sequential commit IDs.

So I'm spinning all of this context back up in my head. It seems like bad news
to really walk backwards through the entire commit history one by one. Couldn't
we just walk backwards through the app.yaml log? i.e.

https://chromium.googlesource.com/chromium/src/+log/master/chrome/common/exte...

Gitiles features that might make this awesomer/possible:

1. Log URLs like above should include a blob field in each log entry,
identifying the blob ID for the referenced file's state at each commit.

So three commits back (at commit ID d1f4a4550f63c4b5c9156f37066a7635c3cd00c6)
app.yaml has blob ID a4b5dc6825c7c2e9468031c5c6d75f2b06622b62 which corresponds
to version 3-39-4.

The third log entry from the JSON log above should include a
'blob'=='a4b5dc6825c7c2e9468031c5c6d75f2b06622b62' entry pair.

2. Support for raw blob fetching by blob ID, which does not yet exist to the
best of my knowledge. i.e. it would be nice to be able to fetch

https://chromium.googlesource.com/chromium/src/+blob/a4b5dc6825c7c2e9468031c5...

and just get a raw copy of app.yaml from commit
d1f4a4550f63c4b5c9156f37066a7635c3cd00c6, aka the one from 3 commits ago where
version is 3-39-4.

[ahernandez, 2014-09-10 18:30:29]

Patchset #3 (id:80001) has been deleted

[Ken Rockot(use gerrit already), 2014-09-13 01:23:59]

Alright, my follow-up patch is way too big to include here.

Can you please land this CL? You'll have to merge some upstream changes and bump
the version up.

I realize this will leave things temporarily in a broken state, but since
they're already broken and I have a follow-up patch, this is acceptable IMO.

Thanks! lgtm

[ahernandez, 2014-09-15 17:39:42]

The CQ bit was checked by ahernandez.miralles@gmail.com

[commit-bot: I haz the power, 2014-09-15 18:02:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/491653002/100001

[commit-bot: I haz the power, 2014-09-15 18:24:51]

Committed patchset #3 (id:100001) as ef50ca64fe859c10c41056c3cceaba217ac74468

[commit-bot: I haz the power, 2014-09-15 18:30:13]

Patchset 3 (id:??) landed as
https://crrev.com/e380f604301c5cdde7d0fad496a5d46c80233afc
Cr-Commit-Position: refs/heads/master@{#294851}