Summary:
The x-repo usually have "noop" mapping i.e. mapping that doesn't change the
paths (it might have arbitrary name though)
It's useful for commits that are identical between small and large repo to be
able to backfill this mapping. This diff adds a command to do that.
Reviewed By: krallin
Differential Revision: D24337281
fbshipit-source-id: 89a058f95677e4a5c8686122a317eadf8b1bb995
Summary: It will be used in the next diff, so let's move it to a separate function.
Reviewed By: krallin
Differential Revision: D24334717
fbshipit-source-id: e50d13d45c633397504cf08954f2ced9ace8f570
Summary:
Except for the tail mode of x_repo_sync_job which we use normally there's also
"once" mode which means "sync a single commit". Previously it did just that -
synced a single commit and failed if parents of this commit weren't synced.
However this is very unpleasant to use - instead let's change the semantics to
sync the commit and all of its ancestors.
also I made target_bookmark an optional parameter - sometimes we just want to sync a commit without any bookmarks at all.
Reviewed By: mitrandir77
Differential Revision: D24135771
fbshipit-source-id: 341c1808a44c58f89536b8c07394b77d8ced3f37
Summary: This allows the user to see how the mover of a particular version operates on any given path.
Reviewed By: StanislavGlebik
Differential Revision: D24335975
fbshipit-source-id: f67847112eb0d3c8c49584604e2f9d93579cdde4
Summary:
Current large to small movers do not cover some of the edge cases, and this
diff attempts to fix them. More details are in the comment
Reviewed By: aslpavel, ikostia
Differential Revision: D24328574
fbshipit-source-id: 35cceff61f21603baf446d83e8daa4bda2f17d2a
Summary:
We had a small issue with our logging when pushredirection was enabled. See for
example https://fburl.com/scuba/mononoke_test_perf/orsld6yh. If you count the
number of "Start processing" and "Command processed" entries you'll notice that
there's one more instance of the former. That's wrong, and turned out this
additional "Start processing" line is coming from backsyncing.
Turned out that the problem was two fold:
1) start_command() logs "Start processing" row and also it sets "Start
processing" as default message for every next scuba calls. That doesn't seem
intentional, so this diff clones scuba sample before logging "Start
processing" to avoid "poisoning" the "tag" field
2) Backsyncing weren't setting the log_tag at all. So this diff fixes it
Reviewed By: ikostia
Differential Revision: D24305167
fbshipit-source-id: 3448b88693e0c5c8f5a9b90f602bc2dfe99db15a
Summary:
In D19023924 (e9df42c192) ikostia allowed non-prefix free movers, however he left the
safeguard in just in case. Well, now we finally need to use non-prefix-free
mapping, so let's remove the safeguard.
Reviewed By: ikostia
Differential Revision: D24277040
fbshipit-source-id: 4d658ad813171ab0dcb23656e95e3e443ec9961a
Summary:
This is a tool that's handy to use right before the binding. It accepts a list
of large repo changesets, and for each commit does not yet have a mapping it
inserts a NotSyncCandidate. This is exactly what we want to do right before the
bind - we'd like large repo to have most of its commits marked as
NotSyncCandidate.
Reviewed By: ikostia
Differential Revision: D24276642
fbshipit-source-id: b040e5dee981c06da8a4c58dacbb8c2660f0917d
Summary:
We don't have any Preserved entries anymore - now all preserved entries will be
rewritten with "noop" mapping.
This diff removes it completely
Reviewed By: mitrandir77, ikostia
Differential Revision: D24173538
fbshipit-source-id: f2d6238633cea8dc3c06f2e607b2abd76edfca6b
Summary: This state is going to be removed soon, so no need for tests anymore
Reviewed By: mitrandir77
Differential Revision: D24221363
fbshipit-source-id: 19dce04549ccbfe59255463a73e56c70f1c8bc4d
Summary: Just as with the previous diff, let's remove it from cross_repo_sync_test_utisl
Reviewed By: mitrandir77
Differential Revision: D24220618
fbshipit-source-id: 95c5ddc955f720101f6576b34e9787435b6deb4c
Summary:
This diff is a first step in a preparation for removing Preserved state from
CommitSyncOutcome. It does two things:
1) Remove unsafe_preserve_commit method and instead rewrite commit with noop
config version - this is exactly how we are going to do it in production
2) While there also select correct mover for validation - previously we were
always picking the current mover
I've also added a bunch of print statements, since it was tricky to debug what
was going on in the test.
Reviewed By: ikostia
Differential Revision: D24216168
fbshipit-source-id: 86e81aea61e638d93bdb33e7c9fd713f7b5e6c3b
Summary:
All the new entries in our mapping tables should have their version set. Let's
enforce it in the code
Reviewed By: ikostia
Differential Revision: D24217688
fbshipit-source-id: 95f01d8929a9c3a19b84434c91db6d08a6e5f863
Summary:
To give a bit of context - we are getting rid of Preserved state in
CommitSyncOutcome completely since it can be fully replaced with with
EquivalentWorkingCopyAncestor/RewrittenAs states.
ikostia@ did a similar change in D24142837 (2035a34a0e) to consider Preserved only if
mapping is None. However this diff diff it only for mapping.get() method. Let's do
the same for mapping.get_equivalent_working_copy().
Reviewed By: ikostia
Differential Revision: D24216455
fbshipit-source-id: f1f8d46263de54cb2e11d33b6c17f371b79e80f9
Summary: This was probably copied from backsyncer some time ago.
Reviewed By: StanislavGlebik
Differential Revision: D24198742
fbshipit-source-id: 3d8fad0ddc94185acd28ede7163b43424935830d
Summary:
This diff adds tests for `sync_merge` version-determination logic:
- when both parents were rewritten with the same version and its identical to the current one
- when both parents were rewritten with the same version and its different to the current one
- when both parents are Preserved
- when one parent is Preserved
Reviewed By: StanislavGlebik
Differential Revision: D24104680
fbshipit-source-id: 075eb40e6f76d4f3271fdf243a5728322698ff46
Summary: Allow bookmark to be optional - again, will be used in the next diffs
Reviewed By: ahornby
Differential Revision: D24163608
fbshipit-source-id: e037731117181d0b1bbe4eb273301245142b507d
Summary: This functionality will be used in the next diffs.
Reviewed By: ahornby
Differential Revision: D24163517
fbshipit-source-id: 36e5c9646e21913f0e0d79d77dd11862f5aa5331
Summary:
This diff fixes how syncing of merge commits decides on the `CommitSyncConfigVersion` to use. Old and incorrect behavior just always uses current version from `LiveCommitSyncConfig`. The desired behavior is to reuse the version with which parent commits are synced, and manually sync commits when version changes are needed.
For merges it is more interesting, as merges have multiple parents. The overarching idea is to force all of the parents to have the same version and bail a merge if this is not the case. However, that is an ideal, and we are not there yet, because:
- there are `NotSyncCandidate` parents, which can (and should at the moment) be safely excluded from the list of parents of the synced commit.
- there are `Preserved` parents (which will turn into the ones synced with a `noop` version)
- there are `RewrittenAs` and `EquivalentWorkingCopy` parents, which don't have an associated version.
So until the problems above are solved:
- absent `RewrittenAs`/`EquivalentWorkingCopy` versions are replaced with the current version
- `Preserved` merge parents cause merge sync to fail.
Reviewed By: StanislavGlebik
Differential Revision: D24033905
fbshipit-source-id: c1c98b3e7097513af980b5a9f00cc62d248fc03b
Summary:
Our higher-level goal is to get rid of `CommitSyncOutcome::Preserved` altogether. This diff is a step in that direction. Specifically, this diff removes the creation of "accidental" Preserved commits: the ones where the hashes are identical, although a `Mover` of some version have been applied. There are a few sides to this fix:
- `get_commit_sync_outcome` now returns `Preserved` only when the source and target hashes are identical, plus stored version is `None` (previously it would only look at hashes).
- `sync_commit_no_parents` now records the `Mover` version it used to rewrite the commit (previously it did not, which would sometimes create `Preserved` roots)
- there are now just two ways to sync commits as `Preserved`:
- `unsafe_preserve_commit` (when the caller explicitly asks for it). The idea is to only remove it once we remove the callers of this methods, of course.
- `sync_commit_single_parent` when the parent is also `Preserved`. Note that automatically upgrading from `Preserved` parent to a rewritten changeset is incorrect for now: `Preserved` does not have an associated version by definition, so we would have to use a current version, which may corrupt the repo. Once we get rid of `Preserved`, this case will naturally go away.
- as we now have `update_mapping_with_version` and `update_mapping` (which consumes current version), we need to add explicit `update_mapping_no_version` for preserved commits we are still creating (again, recording a current version is a mistake here, same reason as above)
NB: I've added/changed a bunch of `println`s in tests, leaving them here, as they are genuinely useful IMO and not harmful.
Reviewed By: StanislavGlebik
Differential Revision: D24142837
fbshipit-source-id: 2153d3c5cc406b3410eadbdfca370f79d01471f9
Summary:
This diff adds two things:
- the ability to compute the reverse of a `CommitSyncDataProvider::Test`, useful when creating both small-to-large and large-to-small `CommitSyncer` structs in tests
- the ability to set a current `CommitSyncConfigVersion` in the provider, which can also be useful, when simulating current version changes.
NB: I ended up not needing the set version functionality in my tests (further in the stack) in the end, so I can remove it, but I do think it will prove useful eventually.
Reviewed By: StanislavGlebik
Differential Revision: D24103206
fbshipit-source-id: 389169b2984684d83b0f6fdeb3be597d84cc0f12
Summary:
This is one more fix to use correct commit sync config version. In particular,
this diff fixes a case where a single parent commit was rewritten out. E.g.
if a large repo commit touches only files that do not remap in a small repo. In
that case we still want to record correct mapping so that all descendants used
the correct mapping as well.
Reviewed By: ikostia
Differential Revision: D24109221
fbshipit-source-id: bcdbb01b964d70227dff8363e77964716a345261
Summary:
Let's move initialization into a separate function. I'm planning to use it in
the next diff for another test
Reviewed By: ikostia
Differential Revision: D24109222
fbshipit-source-id: 73142dd46ef3de15ff381670ed6d5e31653c5dd4
Summary:
Normally, sync logic infers `CommitSyncConfigVersion` to use from parent commits (or from current version for root commits). However, for test purposes it is convenient to force a version override This logic does not change any of the production behaviors, and will be used in a later diff.
TODO: can it ever be needed beyond tests? I've thought about using this for "version boundary" commits, but those would probably just be constructed while completely bypassing the sync logic.
TBH, I am not certain this diff is a good change. But I've spend a very large amount of time crafting the repos used in the `sync_merge` tests later in this stack, so I am proposing to land this, then spend some time refactoring sync tests (and hopefully making it easier to craft test repos), then removing this logic. Obviously, this logic should only be landed if we land the tests in the first place.
Reviewed By: StanislavGlebik
Differential Revision: D24104101
fbshipit-source-id: 0825f04ed74532e89fd5f1fbebeee5f2001fedcd
Summary: No need to allocate a new vector if we just need to remove items from the current one.
Reviewed By: StanislavGlebik
Differential Revision: D24088319
fbshipit-source-id: 10804d925f20fe8dd1e2bb8500aa06d30bd367c1
Summary:
This just adds a single fn. I did not come up with a better place/name to put
it, suggestions are welcome. Seems generic enough to belong at the top-level
common location.
I've already needed this twice, so decided to extract. Second callsite will be further in the stack.
Reviewed By: StanislavGlebik
Differential Revision: D24080193
fbshipit-source-id: c3e0646f263562f3eed93f1fdbab9a076729f33c
Summary: `clippy` often complains about the use of `.len() != 0`, `.len() > 0` or `.len() == 0`and proposes to use `.is_empty()` instead. This diff does that across Mononoke.
Reviewed By: aslpavel
Differential Revision: D24099427
fbshipit-source-id: 1bba2f958485b7efb3f41bf3eae820879c92b0e5
Summary: Now that there are no more use-cases of `get_one`, let's remove it completely.
Reviewed By: farnz
Differential Revision: D24027990
fbshipit-source-id: 47baa6b1e28eedd94d95808efca0a98007a1d388
Summary:
This is a bit of a cargo-cult diff: it replaces the uses of `get_one` with `get` in tests, just to make the same wrong decisions later - use the first item from the produced list of items. So the only thing it does it removes a call site for `get_one`.
The reason it is ok to do `.into_iter().next()` here is because these are tests and we control the situation precisely - we know that there will be one mapping. Same reason we use `.unwrap()` in tests.
Reviewed By: farnz
Differential Revision: D24027785
fbshipit-source-id: 1c11acadfc9f7c6c4af658b414589c32008a6cce
Summary:
`get_one` is a deprecated method, because it uses incorrect logic to resolve ambiguities of multi-mapped commits: if just selects the very first of the potentially many mappings.
Correct resolution is to either handle the ambiguity at the caller site, or rely on provided resolution logic in `commit_sync_outcome.rs`.
Therefore, I am removing the uses of this method in this and a few surrounding commits.
In this case, we can just rely on a provided `CommitSyncer::commit_sync_outcome_exists` method.
Reviewed By: farnz
Differential Revision: D24026470
fbshipit-source-id: 9f150eb3d6c39a58bb4b0d16d4cf18c324359013
Summary:
This diff adds an ability to optionally pass a `CandidateSelectionHint` to `scs` implementation of the `xrepo-lookup` call, which would help in cases when ancestor commits have multiple mappings in the large repo.
Adding this functionality to `scsc xrepo-lookup` is essentially a way to manually fix multi-mapping problems, which could otherwise block Mononoke progress.
For more information on multi-mapping problems, see https://fburl.com/gmywf2d6.
TLDR is that `synced_commit_mapping` is `1:n` with `n` on the large repo side. When syncing commits, we need a way to disambiguate multi-mapped ancestors. `CandidateSelectionHint` is our way of doing this: it expressed desired properties of the commit we would like Mononoke to choose among the list of multi-mapped candidates.
Reviewed By: markbt
Differential Revision: D23991178
fbshipit-source-id: 29c90b7910ad1b84ff71964d6609521fded2f987
Summary:
Let's start actually fixing what commit sync config version is used to remap a commit i.e. we
should use a commit sync config version that was used to remap a parent instead
of using a current version. See more details in
https://fb.quip.com/VYqAArwP0nr1
This diff fixes one particular case and also leaves a few TODOs that we need to
do later
Reviewed By: krallin
Differential Revision: D23953213
fbshipit-source-id: 021da04b0f431767fec5d1c4408287870cb83de1
Summary:
TestLiveCommitSyncConfig is supposed to be a test replacement of
CfgrLiveCommitSyncConfig, however it was quite a different semantic. In
particular, it wasn't even possible to have two versions of the mapping for the
single repo.
This diff changes that. Now we'll have a method to add commit sync config
version, and mark/remove a version as current
Reviewed By: krallin
Differential Revision: D23951202
fbshipit-source-id: 242b4f088f67dac504544987e484cc290ee4e400
Summary: Instead of always fetching the current version name to verify working copy let's instead fetch whatever the version was actually used to create this commit.
Reviewed By: krallin
Differential Revision: D23936503
fbshipit-source-id: 811e427eb62741401b866970b4a0de0c1753edb3
Summary:
Turned out validation didn't report an error if source repo contained an entry
that was supposed to be present in target repo but was actually missing.
This diff fixes it.
Reviewed By: krallin
Differential Revision: D23949909
fbshipit-source-id: 17813b4ad924470c2e8dcd9d3dc0852c79473c61
Summary: Since now we store it in the db, let's also expose it in CommitSyncOutcome enum
Reviewed By: krallin
Differential Revision: D23936502
fbshipit-source-id: a0758143ceaa8f5706f1d9cfe3040ac91c7bac49
Summary:
See motivation for the change in D23845720 (5de500bb99).
We'll need to store version name even for commits that weren't rewritten, but that have an equivalent working copy in another repo.
Reviewed By: ikostia
Differential Revision: D23864571
fbshipit-source-id: 408b68c3b0aa9885a9cd248b0b4abc2b87cd4cca
Summary:
get_source_target_mover likely awaits the same fate as
get_current_mover_DEPRECATED functions i.e. get_source_target_mover will likely
be removed.
This diff just removes a few intances of this function.
Reviewed By: ikostia
Differential Revision: D23929748
fbshipit-source-id: 2ac09da164de3916a552757acf0c39387f6126e4
Summary:
get_mover() and get_reverse_mover() functions return the mover for the
"current" version of the commit sync config, which means these are movers for the version
of the config that's used to create the latest commits on master branch.
So this function returns correct mover only for the latest master commit, but
for all other commits it returns an incorrect mover! This is wrong and it
happened to work just by change, and that's why these functions are marked as deprecated
now, and later we'll add functions 'get_mover_by_version()' which could be used to
replace deprecated functions.
Note that the story for get_bookmark_renamer()/get_reverse_bookmark_renamer()
functions seems to be different. If we can always figure out what's the correct
mover for a commit by e.g. look at its parent we can't really do the same for
bookmarks. Because of that I suggest to keep using the current version for
get_bookmark_renamer() function.
Reviewed By: ikostia
Differential Revision: D23929582
fbshipit-source-id: 3e5e9b46224aca0b75cf2d981ea21c4f9a378ba9
Summary: Finally remove version_name from CommitSyncRepos. Note that this diff adds a few TODOs that we'd need to fix later.
Reviewed By: ikostia
Differential Revision: D23929010
fbshipit-source-id: c72130af548ac7b26bc20ddaac9a59562cc75e0b
Summary: Just as in the previous diff, but this time remove bookmark_renamers from CommitSyncRepos
Reviewed By: ikostia
Differential Revision: D23910295
fbshipit-source-id: 0c2d147057c8d3e0749d5b31ef98ab5022255d95
Summary: Just as the previous diff, but this time it removes reverse_mover
Reviewed By: ikostia
Differential Revision: D23879509
fbshipit-source-id: ed111ca2d106120229c4facc0bb2435913c27966
Summary:
This diff starts to use CommitSyncDataProvider introduced in the previous diff
and removes Mover from CommitSyncRepos struct.
Reviewed By: ikostia
Differential Revision: D23878683
fbshipit-source-id: 0d54f889781aebe4726b3388343a87df783c17d4
Summary:
As described in D23845720 (5de500bb99) we are doing a pretty significant change in the
CommitSyncer. Previously it stored static Movers and BookmarkRenamers, but it
needs to change and they would need to fetch config from LiveCommitSyncConfig.
Unfortunately we already have a bunch of tests that create weird movers that
would be very hard to model via LiveCommitSyncConfig. So we have 2 options:
1) delete or rewrite all these tests
2) Create a wrapper that would return a mover/bookmark renamers.
Option #2 is preferable, and that's what this diff does. In production it would
use LiveCommitSyncConfig, but it also let's tests specify whatever weird movers
they need.
Reviewed By: ikostia
Differential Revision: D23909432
fbshipit-source-id: 83fb627812f625e07f7e40044e2f69274cd2d768
Summary:
A higher-level goal is to provide an interface for the manual remediation of
various xrepo-sync blockages. The nature of a candidate selection is such that
it may fail if the hint is not sufficient to decide which remapping changeset
is the best candidate. This is especially true about the `Only` hint variant:
it is designed to fail when there's more than one candidate. But even with
bookmark ancestorship hints, there are corner cases when the algorithm cannot
make a decision (as well as there may just be bugs in the algorithm). These
cases should be **extremely** rare. Nevertheless, we want to be able to unblock
ourselves. To do so, it is proposed to acccept parent selection hints in the `xrepo-lookup` scs
method. By default, it will use `Only` as a hint and be semantically equivalent
to the current behavior. But we'll provide CLI options to select other hints.
In order to make this work, we need the `sync_commit` method of the
`CommitSyncer` to accept hints too.
Reviewed By: StanislavGlebik
Differential Revision: D23913216
fbshipit-source-id: 05e1ff99cd2c6522829a6e8569040b226600af60
Summary:
This diff adds the use of candidate selection hints to `cross_repo_sync` code sights, which need to query `CommitSyncOutcome` in the small-to-large direction. Specifically: `unsafe_sync_commit` and `unsafe_sync_commit_pushrebase` are the two main functions.
One will now get `CandidateSelectionHint` from the callsight (most notably: `push_redirector`), the other one will build a bookmark-based hint itself.
Reviewed By: StanislavGlebik
Differential Revision: D23715259
fbshipit-source-id: 3f4924f1337b09f3762cc050c4017c5d2bd6cab6
