Summary:
This didn't matter much in practice because we've never pushrebased a rewritten
commit where one of the parents rewrites to nothing. Nevertheless I think it's
cleaner to return an error in this case
Reviewed By: ikostia
Differential Revision: D24621105
fbshipit-source-id: 8cf62efd28e0c9aed945f727b1872db6922cfeb3
Summary:
This stack has a goal of cleaning up how commits with no parents and
consequently merges are treated in CommitSyncer. At the moment it always uses current version
to rewrite them, and this is incorrect. See example below (here N means "new
commits" i.e. commits that weren't remapped yet but we need to remap them now)
```
large repo
O <- uses current version
|
O
|
O N
|/ |
O | <- uses old version
| N
| |
O N <- this commit should be rewritten with old version!
```
With current logic all N commits will use current version for remapping, but
this is incorrect, and instead "old version" should be used. The goal is to fix
it and make it so that backsyncer and x-repo sync job pick the correct version
to use for remapping.
-----
As it was noted in the previous diff we need a function that overrides a
version for a commit with no parents. We need this function because for a
commit with no parents CommitSyncer can't decide which mapping version to use
because, well, there are no parents which could have hinted about the version.
So let's add this function and cleanup unsafe_sync_commit_impl function to
apply version override only to a commit with no parents. If commit has parents then we'd verify that the version from parents matches the expected version.
In the next diffs I'll make it so that if a version for commit with no parents is not specified then
unsafe_sync_commit_impl fails rather than uses current version.
Reviewed By: ikostia
Differential Revision: D24617953
fbshipit-source-id: a68f837da9d90bb18034f628b7880482a2e548b7
Summary:
This adds some basic Scuba logging to commit sync logic in Mononoke. This can be disabled via a tunable.
Note: of course, the wrapping of logging functions would have been pretty cool to do in a macro, but I don't have time atm to figure it out, so have just code.
Reviewed By: StanislavGlebik
Differential Revision: D24571702
fbshipit-source-id: f8830fc675a2a62d2204671e86ab2c819372c5cc
Summary: This adds a `ScubaSampleBuilder` field to the `CommitSyncer` and ensures this field is instantiated with correct values (real vs discarding sample) depending on circumstances.
Reviewed By: StanislavGlebik
Differential Revision: D24539732
fbshipit-source-id: 37aedcff9aefcfcd6b740a0491ab35f9e5ce7c77
Summary:
Functionality of this function can be replaced with
unsafe_always_rewrite_sync_commit, so let's do that.
However in the next diff we'll need a function similar to
test_unsafe_sync_commit but with a slightly different semantics - instead of
forcing override for every commit we'll just force it for a commit with no
parents. Because of that let's not remove unsafe_sync_commit_impl function.
Reviewed By: ikostia
Differential Revision: D24616824
fbshipit-source-id: 6969145d98cd23604920a6b490bf7ffe47938c08
Summary:
`CommitSyncerArgs` was useful when `CommitSyncer` did not have a way to query
existing configs from the configerator. Now that `SyncDataProvider` is part of
`CommitSyncer`, we can get rid of `CommitSyncerArgs`, which will also make
further improvements more convenient.
Reviewed By: StanislavGlebik
Differential Revision: D24565773
fbshipit-source-id: 4dd507b06d946c6018a4a4e8d5e77c6b27abe195
Summary: A better fix would be to also get rid of `Option` in the struct itself, but I don't want to spend any more time on this atm, and this change is a clear improvement.
Reviewed By: StanislavGlebik
Differential Revision: D24538309
fbshipit-source-id: 6190c6936dc34d996ecd3d700e5f71282d45f651
Summary:
update Phases::add_reachable_as_public to futures03
With this change all the Phases methods are futures03
Reviewed By: krallin
Differential Revision: D24531552
fbshipit-source-id: e9201621b798739d4d7dd197f15188103e9d359a
Summary:
Fewer clones, better code.
Note that in some cases we would previously have a fn that takes `ctx` by
ownership and just passes it through to some other fn outside of the
`cross_repo_sync`. I triead to make all such functions borrow and clone instead
in order to push cloning to the leaf fns of `cross_repo_sync`.
Reviewed By: StanislavGlebik
Differential Revision: D24538028
fbshipit-source-id: 8a3e78d4076b34d8b1767cdee1db3fdbb7acb4f7
Summary: In D24447404, I provided some utility functions that allowed me to avoid constructing and/or passing around a ConfigStore. Remove those functions and fix up the code to run.
Reviewed By: krallin
Differential Revision: D24502692
fbshipit-source-id: 742dbc54fbcf735895d6829745b9317af14dfa0b
Summary: It's designed as a singleton store for normal use - rather than have lots of ways to get different config stores, let's use one global store
Reviewed By: krallin
Differential Revision: D24447404
fbshipit-source-id: 6ecc46351b14794471f654cec98527a11a93d3ef
Summary:
- convert ManifestOps to new style futures
- at this point `//eden/manifest:manifest` crate is completely free from old style futures
Reviewed By: krallin
Differential Revision: D24502214
fbshipit-source-id: f1cdb11bd8234f22af5c905243f71e1e9fca11f1
Summary: Somewhat more convenient to work with, no need to look inside.
Reviewed By: StanislavGlebik
Differential Revision: D24474898
fbshipit-source-id: 7ee0920e7d0d5a2102c68695a5cc0d9e237d958d
Summary:
I removed --debug flag for bookmarks validator on tw because it was too spammy,
but now we get no output at all. Let's make it info so that we can see it in tw
output
Reviewed By: ikostia
Differential Revision: D24445829
fbshipit-source-id: d37c41be46794867f91ecf66f0318e7b2d660d85
Summary: As we saw today it was showing entries in incorrect order
Reviewed By: ikostia
Differential Revision: D24418800
fbshipit-source-id: c926c7904338ae40b26fe08cb1ac973e384bad28
Summary:
Prior to this diff, validation logic did not work when target repo was a large
one (although this limitation wasn't advertised). This was becase we explicitly
never apply a `Mover` to the target repo paths and therefore expect every
single target repo path to have an equivalent in the source repo. This is true
when the target repo is small, but not true when it is large.
It is pretty easy to make this logic direction-agnostic: just proactively check
whether target repo paths rewrite into nothingness or not.
Reviewed By: StanislavGlebik
Differential Revision: D24399029
fbshipit-source-id: 9ca5bb03760e662ff6756c27c0c77204abdb38de
Summary: This command will help us understand if it is safe to enable push-redirection with a certain CommitSyncConfigVersion,
Reviewed By: StanislavGlebik
Differential Revision: D24345898
fbshipit-source-id: c2a4c034a9203025ce3534986b3bb986784ff2b1
Summary:
Previously it was just taking the current version. Instead of guessing let's
allow users to specify whatever version they want to rewrite it with.
Reviewed By: ikostia
Differential Revision: D24360918
fbshipit-source-id: d9f4c55cd66931a9f2ab7da3474d2152d77525d0
Summary:
update_mapping method was silently setting current mapping version. This was
very error-prone and we had a replacement update_mapping_with_version. This
diff removes update_mapping completely.
Reviewed By: krallin
Differential Revision: D24360105
fbshipit-source-id: 57761f4279f75032e9d4ec88a45e5199e250247a
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
