Replays have commit SHAs that do not reflect anything on the public server

smogon / pokemon-showdown

Pokémon battle simulator.

https://pokemonshowdown.com

MIT License

4.77k stars 2.79k forks source link

Replays have commit SHAs that do not reflect anything on the public server #5412

Closed yuzeh closed 5 years ago

yuzeh commented 5 years ago

https://github.com/Zarel/Pokemon-Showdown/blob/6c1ab309774e3ae1111abea114dc63a1cd5bc49f/server/room-battle.js#L1080-L1087

Is this what is being written into replay logs on the first line?

Zarel commented 5 years ago

Yeah, there was an unpushed commit on the server. I've pushed it now: https://github.com/Zarel/Pokemon-Showdown/commit/12f49b20e16c0fab0cd1a1ca9ac1a5a1cc8d7e82

yuzeh commented 5 years ago

I think it might be more than that. Here's a few examples:

https://replay.pokemonshowdown.com/gen7randombattle-887606429.json references 664768c238d88325eea8f848b9726dcf8abd958e which doesn't exist in the public repo
https://replay.pokemonshowdown.com/gen7randombattle-879384978.json references dab6083d84cf10c82638ffd4b09ad28aa8458e44 which also doesn't exist in the public repo

Is the master branch being overwritten locally on the deploy machine(s)?

Zarel commented 5 years ago

664768c238d8 is actually ac5c0a5760cff588b9c25 in the public repo.

dab6083d84cf10c is actually 9315b8d4d83df3 in the public repo.

They have different hashes because 12f49b20e16c0fab0cd was being constantly rebased on top of them.

yuzeh commented 5 years ago

Oof, okay. I'm going to go back to using guess and check, then.

scheibo commented 5 years ago

Out of curiosity, what are you using it for?

yuzeh commented 5 years ago

Testing two ideas:

Imitation learning
Computing a measure of how "human-like" my current agent is

On Fri, Apr 5, 2019 at 10:25 AM Kirk Scheibelhut notifications@github.com wrote:

Out of curiosity, what are you using it for?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Zarel/Pokemon-Showdown/issues/5412#issuecomment-480356354, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVbL9yc_4Usay7rIhRyLj5PC_QmfDHxks5vd4b9gaJpZM4cfOPr .

Slayer95 commented 5 years ago

Looks like it'd be better to run git merge-base origin/master HEAD ?

yuzeh commented 5 years ago

@Slayer95 agreed. I'll submit a PR.

yuzeh commented 5 years ago

@Slayer95 I've thought about this a bit more. If rebasing on the deploy-local master branch is going to be a common occurrence, I'd rather the system generate a confusing SHA than provide a SHA that is potentially wrong (and won't reconstruct properly).

@Zarel thoughts?

Slayer95 commented 5 years ago

Local edits to the battle engine shouldn't really happen -most should be to the server infrastructure AFAICT.

Even if they do happen, I think it would be better to provide the version in a best-effort basis. In fact, in the specific circumstance above, the hash returned would have been the expected one! Note that you should anyways have logic in place to handle failures in the reconstruction process from the input log (never trust remote server data).

If the proposed change is done, you could iteratively check nearby commits until the battle reconstructs properly (ensuring the winner matches should be enough), and fix the commit hash in your own database.

Slayer95 commented 5 years ago

Also, by checking for an ancestor commit in master as opposed to just returning the last commit in HEAD, the current implementation is already returning a potentially wrong commit hash...

yuzeh commented 5 years ago

Yeah that's a good point. Even with a hash origin/master that doesn't reconstruct properly, the incorrect hash is a good starting point for a search through the commit history.

Okay, this change sounds good to me.

Zarel commented 5 years ago

In general, the main server should never have unpushed commits. This latest one was just something I accidentally forgot to push.