On 8/29/2018 1:12 PM, Junio C Hamano wrote:
Ben Peart <Ben.Peart@xxxxxxxxxxxxx> writes:
This is possible because the current extensions don't access the cache
entries in the index_state structure so are OK that they don't all exist
yet.
The CACHE_EXT_TREE, CACHE_EXT_RESOLVE_UNDO, and CACHE_EXT_UNTRACKED
extensions don't even get a pointer to the index so don't have access to the
cache entries.
CACHE_EXT_LINK only uses the index_state to initialize the split index.
CACHE_EXT_FSMONITOR only uses the index_state to save the fsmonitor last
update and dirty flags.
Good to see such an analysis here. Once we define an extension
section, which requires us to have the cache entries before
populating it, this scheme would falls down, of course, but the
extension mechanism is all about protecting ourselves from the
future changes, so we'd at least need a good feel for how we read an
unknown extension from the future with the current code. Perhaps
just like the main cache entries were pre-scanned to apportion them
to worker threads, we can pre-scan the sections and compare them
with a white-list built into our binary before deciding that it is
safe to read them in parallel (and otherwise, we ask the last thread
for reading extensions to wait until the workers that read the main
index all return)?
Yes, when we add a new extension that requires the cache entries to
exist and be parsed, we will need to add a mechanism to ensure that
happens for that extension. I agree a white list is probably the right
way to deal with it. Until we have that need, it would just add
unnecessary complexity so I think we should wait till it is actually needed.
There isn't any change in behavior with unknown extensions and this
patch. If an unknown extension exists it will just get ignored and
reported as an "unknown extension" or "die" if it is marked as "required."
I'll fix the rest of your suggestions - thanks for the close review.
