gps: source: requirements tweak

[jmank88]

What does this do / why do we need it?

Drops convertToRevision's sourceHasLatestLocally requirement, and optimizes the sourceHasLatestVersionList requirement check. This should increase cache hits.

What should your reviewer look out for in this PR?

Correctness.

Which issue(s) does this PR fix?

Related to #431

[sdboyer]

definitely faster - but not safe 😢. this was added specifically to address #484.

[sdboyer]

this is one of the kinds of things for which we may need to refactor the FSM in order to eke out more performance.

[jmank88]

I'm a little confused. Doesn't sourceHasLatestVersionList trigger a git ls-remote call on it's own (which calls storeVersionMap)? Also, doesn't that happen before the sourceHasLatestLocally flag is processed anyways (calling updateLocal which doesn't update the version map)?

[sdboyer]

it may actually be the preceding check that's the problem here:

	if sg.srcState&sourceHasLatestVersionList != 0 {

this may need to check both flags, sourceHasLatestVersionList and sourceHasLatestLocally. ...maaaaybe, that's just OTTOMH.

yes, sourceHasLatestVersionList will guarantee that a git ls-remote has been performed. it will always be set by the time maybeGitSource.try() returns without error. however, there's no guarantee that a git fetch will have been run - that's what sourceHasLatestLocally maps to. so, for runs where the repo already exists, it's possible that there may exist revs that we know exist (i.e. are in the cache) but don't actually exist under .git/objects.

this isn't generally possible with hg or bzr, as sourceHasLatestVersionList and sourceHasLatestLocally end up meaning the same thing. this is one reason i tend to refer to this FSM as slapdash - the sourceGateway pretends like it knows all the rules, but things would really be better if it was the source implementations that decided the shape of the machine. maybeSource has this power already, as it can return the bitset indicating the states that have been reached.

[jmank88]

So are you suggesting that git ls-remote doesn't necessarily hit the network, and may just read from .git/objects?

Even given that, aren't the flags still processed in the wrong order?

Edit: Or are you suggesting that it is incorrect by definition for the cache to return a version for which the sources are not locally cached in git yet?

[sdboyer]

So are you suggesting that git ls-remote doesn't necessarily hit the network, and may just read from .git/objects?

err...not trying to say anything even close to that :/

Or are you suggesting that it is incorrect by definition for the cache to return a version for which the sources are not locally cached in git yet?

no, the idea behind the logic is to only update from the remote if we can't satisfy the call with the information we have on hand.

Even given that, aren't the flags still processed in the wrong order?

no, i'm...whew, we're having some kind of colossal miscommunication here. i'm going to try to switch to inline comments, maybe that can at least help target the discussion.

[sdboyer]

oh, well, shit, part of the issue is that i was conflating convertToRevision() with another method, which i've repeatedly done. but still, switching to inline.

[sdboyer]

so this conditional is kinda undermining the purpose of the FSM - it's effectively saying that EITHER the sourceHasLatestVersionList bit can be set, OR the cache can have a non-empty version set. it was this kind of multiple-sources-of-truth stuff in the old implementation that made me move to this model in the first place.

basically, if there's a case where this conditional isn't hit, it means there's a bug somewhere else, as that place should've also known to go and update sg.srcState accordingly.

[jmank88]

This other case doing a check threw me off:

case sourceExistsLocally:
 				if !sg.src.existsLocally(ctx) {

So I added that check thinking it could be the source of my problems, but never confirmed it. Happy to revert.

[sdboyer]

OK, i understand again now.

Or are you suggesting that it is incorrect by definition for the cache to return a version for which the sources are not locally cached in git yet?

yes, that's what i'm saying that method has ended up being used for. though, now that i'm looking at the context around it, that's making a lot less sense - it seems like 4871d2f is what's actually playing that defense correctly. i'm starting to agree that i can't actually see what purpose is served by fetching there.

[jmank88]

look at all of its callers - if the rev isn't present, they will immediately fail.

Don't they all have logic to make their own appropriate require call if necessary? What sort of failure?

if we want to avoid that safety fetch, then it would require a call to source.revisionPresentIn()

Can you elaborate?

[jmank88]

@sdboyer Is there anything left to reconsider here?

[sdboyer]

nope, we're good