From 5e2afa735f5fbe5a8c974877475610787f7077c0 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 29 Nov 2017 21:38:12 -0800 Subject: [PATCH] Change how the UNTRACKED_ADDED conflict and merges are handled. Summary: Previously, we used the Mercurial code `g` when faced with an `UNTRACKED_ADDED` file conflict, but that was allowing merges to silently succeed that should not have. This revision changes our logic to use the code `m` for merge, which unearthed that we were not honoring the user's `update.check` setting properly. Because we use `update.check=noconflict` internally at Facebook, we changed the Eden integration tests to default to verifying Hg running with this setting. To support it properly, we had to port this code from `update.py` in Mercurial to our own `_determine_actions_for_conflicts()` function: ``` if updatecheck == 'noconflict': for f, (m, args, msg) in actionbyfile.iteritems(): if m not in ('g', 'k', 'e', 'r', 'pr'): msg = _("conflicting changes") hint = _("commit or update --clean to discard changes") raise error.Abort(msg, hint=hint) ``` However, this introduced an interesting issue where the `checkOutRevision()` Thrift call from Hg would update the `SNAPSHOT` file on the server, but `.hg/dirstate` would not get updated with the new parents until the update completed on the client. With the new call to `raise error.Abort` on the client, we could get in a state where the `SNAPSHOT` file had the hash of the commit assuming the update succeeded, but `.hg/dirstate` reflected the reality where it failed. To that end, we changed `checkOutRevision()` to take a new parameter, `checkoutMode`, which can take on one of three values: `NORMAL`, `DRY_RUN`, and `FORCE`. Now if the user tries to do an ordinary `hg update` with `update.check=noconflict`, we first do a `DRY_RUN` and examine the potential conflicts. Only if the conflicts should not block the update do we proceed with a call to `checkOutRevision()` in `NORMAL` mode. To make this work, we had to make a number of changes to `CheckoutAction`, `CheckoutContext`, `EdenMount`, and `TreeInode` to keep track of the `checkoutMode` and ensure that no changes are made to the working copy when a `DRY_RUN` is in effect. One minor issue (for which there is a `TODO`) is that a `DRY_RUN` will not report any `DIRECTORY_NOT_EMPTY` conflicts that may exist. As `TreeInode` is implemented today, it is a bit messy to report this type of conflict without modifying the working copy along the way. Finally, any `UNTRACKED_ADDED` conflict should cause an update to abort to match the behavior in stock Mercurial if the user has the following config setting: ``` [commands] update.check = noconflict ``` Though the original name for this setting was: ``` [experimental] updatecheck = noconflict ``` Although I am on Mercurial 4.4.1, the `update.check` setting does not seem to take effect when I run the integration tests, but the `updatecheck` setting does, so for now, I set both in `hg_extension_test_base.py` with a `TODO` to remove `updatecheck` once I can get `update.check` to do its job. Reviewed By: simpkins Differential Revision: D6366007 fbshipit-source-id: bb3ecb1270e77d59d7d9e7baa36ada61971bbc49 --- eden/cli/config.py | 3 +- eden/fs/inodes/CheckoutAction.cpp | 11 ++- eden/fs/inodes/CheckoutContext.cpp | 11 ++- eden/fs/inodes/CheckoutContext.h | 18 ++-- eden/fs/inodes/EdenMount.cpp | 14 ++- eden/fs/inodes/EdenMount.h | 3 +- eden/fs/inodes/TreeInode.cpp | 29 +++++- eden/fs/inodes/test/CheckoutTest.cpp | 49 ++++++---- eden/fs/service/EdenServiceHandler.cpp | 7 +- eden/fs/service/EdenServiceHandler.h | 2 +- eden/fs/service/eden.thrift | 59 +++++++---- .../hg/lib/hg_extension_test_base.py | 9 ++ eden/integration/hg/update_test.py | 98 ++++++++++++++++--- eden/integration/lib/hgrepo.py | 14 ++- 14 files changed, 242 insertions(+), 85 deletions(-) diff --git a/eden/cli/config.py b/eden/cli/config.py index ec94559f85..adc89e51d5 100644 --- a/eden/cli/config.py +++ b/eden/cli/config.py @@ -238,7 +238,8 @@ class Config: def checkout(self, path, snapshot_id): '''Switch the active snapshot id for a given client''' with self.get_thrift_client() as client: - client.checkOutRevision(path, snapshot_id) + client.checkOutRevision(path, snapshot_id, + eden_ttypes.CheckoutMode.NORMAL) def add_repository(self, name, repo_type, source, with_buck=False): # Check if repository already exists diff --git a/eden/fs/inodes/CheckoutAction.cpp b/eden/fs/inodes/CheckoutAction.cpp index 40320cfaef..12fa1237cb 100644 --- a/eden/fs/inodes/CheckoutAction.cpp +++ b/eden/fs/inodes/CheckoutAction.cpp @@ -283,8 +283,15 @@ Future CheckoutAction::doAction() { // All the data is ready and we're ready to go! // Check for conflicts first. - if (hasConflict() && !ctx_->forceUpdate()) { - // hasConflict will have added the conflict information to ctx_ + auto conflictWasAddedToCtx = hasConflict(); + // Note that even if we know we are not going to apply the changes, we must + // still run hasConflict() first because we rely on its side-effects. + if (conflictWasAddedToCtx && !ctx_->forceUpdate()) { + // We only report conflicts for files, not directories. The only possible + // conflict that can occur here if this inode is a TreeInode is that the old + // source control state was for a file. There aren't really any other + // conflicts than this to report, even if we recurse. Anything inside this + // directory is basically just untracked (or possibly ignored) files. return makeFuture(); } diff --git a/eden/fs/inodes/CheckoutContext.cpp b/eden/fs/inodes/CheckoutContext.cpp index 61b282bb48..d8383a2369 100644 --- a/eden/fs/inodes/CheckoutContext.cpp +++ b/eden/fs/inodes/CheckoutContext.cpp @@ -22,8 +22,8 @@ namespace eden { CheckoutContext::CheckoutContext( folly::Synchronized::LockedPtr&& parentsLock, - bool force) - : force_{force}, parentsLock_(std::move(parentsLock)) {} + CheckoutMode checkoutMode) + : checkoutMode_{checkoutMode}, parentsLock_(std::move(parentsLock)) {} CheckoutContext::~CheckoutContext() {} @@ -32,8 +32,11 @@ void CheckoutContext::start(RenameLock&& renameLock) { } vector CheckoutContext::finish(Hash newSnapshot) { - // Update the in-memory snapshot ID - parentsLock_->parents.setParents(newSnapshot); + // Only update the parents if it is not a dry run. + if (!isDryRun()) { + // Update the in-memory snapshot ID + parentsLock_->parents.setParents(newSnapshot); + } // Release our locks. // This would release automatically when the CheckoutContext is destroyed, diff --git a/eden/fs/inodes/CheckoutContext.h b/eden/fs/inodes/CheckoutContext.h index d905a94279..15989f6f88 100644 --- a/eden/fs/inodes/CheckoutContext.h +++ b/eden/fs/inodes/CheckoutContext.h @@ -37,17 +37,17 @@ class CheckoutContext { public: CheckoutContext( folly::Synchronized::LockedPtr&& parentsLock, - bool force); + CheckoutMode checkoutMode); ~CheckoutContext(); /** - * Returns true if the checkout operation should actually update the inodes, - * or false if it should do a dry run, looking for conflicts without actually - * updating the inode contents. + * Returns true if the checkout operation should do a dry run, looking for + * conflicts without actually updating the inode contents. If it returns + * false, it should actually update the inodes as part of the checkout. */ - bool shouldApplyChanges() const { + bool isDryRun() const { // TODO: make this configurable on checkout start - return true; + return checkoutMode_ == CheckoutMode::DRY_RUN; } /** @@ -59,10 +59,10 @@ class CheckoutContext { * new contents, rather than just reporting and skipping files with * conflicts. * - * forceUpdate() can only return true when shouldApplyChanges() is also true. + * forceUpdate() can only return true when isDryRun() is false. */ bool forceUpdate() const { - return force_; + return checkoutMode_ == CheckoutMode::FORCE; } /** @@ -99,7 +99,7 @@ class CheckoutContext { } private: - bool const force_{false}; + CheckoutMode checkoutMode_; folly::Synchronized::LockedPtr parentsLock_; RenameLock renameLock_; diff --git a/eden/fs/inodes/EdenMount.cpp b/eden/fs/inodes/EdenMount.cpp index 7c5a9dbae7..8b6f0c2ead 100644 --- a/eden/fs/inodes/EdenMount.cpp +++ b/eden/fs/inodes/EdenMount.cpp @@ -392,13 +392,14 @@ FileInodePtr EdenMount::getFileInodeBlocking(RelativePathPiece path) const { folly::Future> EdenMount::checkout( Hash snapshotHash, - bool force) { + CheckoutMode checkoutMode) { // Hold the snapshot lock for the duration of the entire checkout operation. // // This prevents multiple checkout operations from running in parallel. auto parentsLock = parentInfo_.wlock(); auto oldParents = parentsLock->parents; - auto ctx = std::make_shared(std::move(parentsLock), force); + auto ctx = + std::make_shared(std::move(parentsLock), checkoutMode); XLOG(DBG1) << "starting checkout for " << this->getPath() << ": " << oldParents << " to " << snapshotHash; @@ -434,8 +435,15 @@ folly::Future> EdenMount::checkout( // Save the new snapshot hash XLOG(DBG1) << "updating snapshot for " << this->getPath() << " from " << oldParents << " to " << snapshotHash; - this->config_->setParentCommits(snapshotHash); auto conflicts = ctx->finish(snapshotHash); + if (ctx->isDryRun()) { + // This is a dry run, so all we need to do is tell the caller about + // the conflicts: we should not modify any files or add any entries to + // the journal. + return folly::makeFuture(conflicts); + } + + this->config_->setParentCommits(snapshotHash); // Write a journal entry return journalDiffCallback diff --git a/eden/fs/inodes/EdenMount.h b/eden/fs/inodes/EdenMount.h index 3a54a4e924..059426441b 100644 --- a/eden/fs/inodes/EdenMount.h +++ b/eden/fs/inodes/EdenMount.h @@ -24,6 +24,7 @@ #include "eden/fs/inodes/InodePtrFwd.h" #include "eden/fs/journal/JournalDelta.h" #include "eden/fs/model/ParentCommits.h" +#include "eden/fs/service/gen-cpp2/eden_types.h" #include "eden/fs/utils/PathFuncs.h" namespace folly { @@ -257,7 +258,7 @@ class EdenMount { */ folly::Future> checkout( Hash snapshotHash, - bool force = false); + CheckoutMode checkoutMode = CheckoutMode::NORMAL); /** * Compute differences between the current commit and the working directory diff --git a/eden/fs/inodes/TreeInode.cpp b/eden/fs/inodes/TreeInode.cpp index a9b697332f..8b43578531 100644 --- a/eden/fs/inodes/TreeInode.cpp +++ b/eden/fs/inodes/TreeInode.cpp @@ -2194,7 +2194,7 @@ unique_ptr TreeInode::processCheckoutEntry( // This is a new entry being added, that did not exist in the old tree // and does not currently exist in the filesystem. Go ahead and add it // now. - if (ctx->shouldApplyChanges()) { + if (!ctx->isDryRun()) { auto newEntry = make_unique(newScmEntry->getMode(), newScmEntry->getHash()); contents.entries.emplace(newScmEntry->getName(), std::move(newEntry)); @@ -2212,7 +2212,7 @@ unique_ptr TreeInode::processCheckoutEntry( ctx->addConflict( ConflictType::REMOVED_MODIFIED, this, oldScmEntry->getName()); if (ctx->forceUpdate()) { - DCHECK(ctx->shouldApplyChanges()); + DCHECK(!ctx->isDryRun()); auto newEntry = make_unique(newScmEntry->getMode(), newScmEntry->getHash()); contents.entries.emplace(newScmEntry->getName(), std::move(newEntry)); @@ -2275,7 +2275,7 @@ unique_ptr TreeInode::processCheckoutEntry( } // Bail out now if we aren't actually supposed to apply changes. - if (!ctx->shouldApplyChanges()) { + if (ctx->isDryRun()) { return nullptr; } @@ -2303,10 +2303,14 @@ Future TreeInode::checkoutUpdateEntry( std::shared_ptr oldTree, std::shared_ptr newTree, const folly::Optional& newScmEntry) { - CHECK(ctx->shouldApplyChanges()); - auto treeInode = inode.asTreePtrOrNull(); if (!treeInode) { + // If the target of the update is not a directory, then we know we do not + // need to recurse into it, looking for more conflicts, so we can exit here. + if (ctx->isDryRun()) { + return makeFuture(); + } + std::unique_ptr deletedInode; auto contents = contents_.wlock(); @@ -2359,6 +2363,15 @@ Future TreeInode::checkoutUpdateEntry( return treeInode->checkout(ctx, std::move(oldTree), std::move(newTree)); } + if (ctx->isDryRun()) { + // TODO(mbolin): As it stands, if this is a dry run, we will not report a + // DIRECTORY_NOT_EMPTY conflict if it exists. We need to do further + // investigation to determine whether this is acceptible behavior. + // Currently, the Hg extension ignores DIRECTORY_NOT_EMPTY conflicts, but + // that may not be the right thing to do. + return makeFuture(); + } + // We need to remove this directory (and possibly replace it with a file). // First we have to recursively unlink everything inside the directory. // Fortunately, calling checkout() with an empty destination tree does @@ -2411,6 +2424,12 @@ Future TreeInode::checkoutUpdateEntry( void TreeInode::saveOverlayPostCheckout( CheckoutContext* ctx, const Tree* tree) { + if (ctx->isDryRun()) { + // If this is a dry run, then we do not want to update the parents or make + // any sort of unnecessary writes to the overlay, so we bail out. + return; + } + bool isMaterialized; bool stateChanged; bool deleteSelf; diff --git a/eden/fs/inodes/test/CheckoutTest.cpp b/eden/fs/inodes/test/CheckoutTest.cpp index 91f5fbac8f..b3e2216693 100644 --- a/eden/fs/inodes/test/CheckoutTest.cpp +++ b/eden/fs/inodes/test/CheckoutTest.cpp @@ -435,7 +435,7 @@ TEST(Checkout, modifyFile) { void testModifyConflict( folly::StringPiece path, LoadBehavior loadType, - bool force, + CheckoutMode checkoutMode, folly::StringPiece contents1, int perms1, folly::StringPiece currentContents, @@ -470,6 +470,7 @@ void testModifyConflict( // Prepare the destination tree auto builder2 = builder1.clone(); builder2.replaceFile(path, contents2, perms2); + builder2.replaceFile("a/b/dddd.c", "new dddd contents\n"); builder2.finalize(testMount.getBackingStore(), true); auto commit2 = testMount.getBackingStore()->putCommit("b", builder2); commit2->setReady(); @@ -477,7 +478,7 @@ void testModifyConflict( loadInodes(testMount, path, loadType, currentContents, currentPerms); auto checkoutResult = - testMount.getEdenMount()->checkout(makeTestHash("b"), force); + testMount.getEdenMount()->checkout(makeTestHash("b"), checkoutMode); ASSERT_TRUE(checkoutResult.isReady()); auto results = checkoutResult.get(); ASSERT_EQ(1, results.size()); @@ -486,12 +487,16 @@ void testModifyConflict( EXPECT_EQ(ConflictType::MODIFIED_MODIFIED, results[0].type); auto postInode = testMount.getFileInode(path); - if (force) { - // Make sure the path is updated as expected - EXPECT_FILE_INODE(postInode, contents2, perms2); - } else { - // Make sure the path has not been changed - EXPECT_FILE_INODE(postInode, currentContents, currentPerms); + switch (checkoutMode) { + case CheckoutMode::FORCE: + // Make sure the path is updated as expected + EXPECT_FILE_INODE(postInode, contents2, perms2); + break; + case CheckoutMode::DRY_RUN: + case CheckoutMode::NORMAL: + // Make sure the path has not been changed + EXPECT_FILE_INODE(postInode, currentContents, currentPerms); + break; } // Unmount and remount the mount point, and verify the changes persisted @@ -499,22 +504,33 @@ void testModifyConflict( postInode.reset(); testMount.remount(); postInode = testMount.getFileInode(path); - if (force) { - EXPECT_FILE_INODE(postInode, contents2, perms2); - } else { - EXPECT_FILE_INODE(postInode, currentContents, currentPerms); + auto ddddInode = testMount.getFileInode("a/b/dddd.c"); + switch (checkoutMode) { + case CheckoutMode::FORCE: + EXPECT_FILE_INODE(postInode, contents2, perms2); + EXPECT_FILE_INODE(ddddInode, "new dddd contents\n", 0644); + break; + case CheckoutMode::DRY_RUN: + EXPECT_FILE_INODE(postInode, currentContents, currentPerms); + EXPECT_FILE_INODE(ddddInode, "this is dddd.c\n", 0644); + break; + case CheckoutMode::NORMAL: + EXPECT_FILE_INODE(postInode, currentContents, currentPerms); + EXPECT_FILE_INODE(ddddInode, "new dddd contents\n", 0644); + break; } } void runModifyConflictTests(folly::StringPiece path) { for (auto loadType : kAllLoadTypes) { - for (bool force : {true, false}) { + for (auto checkoutMode : + {CheckoutMode::NORMAL, CheckoutMode::DRY_RUN, CheckoutMode::FORCE}) { SCOPED_TRACE(folly::to( - "path ", path, " load type ", loadType, " force=", force)); + "path ", path, " load type ", loadType, " force=", checkoutMode)); testModifyConflict( path, loadType, - force, + checkoutMode, "orig file contents.txt", 0644, "current file contents.txt", @@ -550,9 +566,8 @@ TEST(Checkout, modifyThenRevert) { // Now perform a forced checkout to the current commit, // which should discard our edits. - bool force = true; auto checkoutResult = - testMount.getEdenMount()->checkout(originalCommit, force); + testMount.getEdenMount()->checkout(originalCommit, CheckoutMode::FORCE); ASSERT_TRUE(checkoutResult.isReady()); // The checkout should report a/test.txt as a conflict EXPECT_THAT( diff --git a/eden/fs/service/EdenServiceHandler.cpp b/eden/fs/service/EdenServiceHandler.cpp index d2e655931e..80e2b2c237 100644 --- a/eden/fs/service/EdenServiceHandler.cpp +++ b/eden/fs/service/EdenServiceHandler.cpp @@ -146,16 +146,17 @@ void EdenServiceHandler::checkOutRevision( std::vector& results, std::unique_ptr mountPoint, std::unique_ptr hash, - bool force) { + CheckoutMode checkoutMode) { INSTRUMENT_THRIFT_CALL( DBG1, *mountPoint, hashFromThrift(*hash).toString(), - folly::format("force={}", force ? "true" : "false")); + folly::get_default( + _CheckoutMode_VALUES_TO_NAMES, checkoutMode, "(unknown)")); auto hashObj = hashFromThrift(*hash); auto edenMount = server_->getMount(*mountPoint); - auto checkoutFuture = edenMount->checkout(hashObj, force); + auto checkoutFuture = edenMount->checkout(hashObj, checkoutMode); results = checkoutFuture.get(); } diff --git a/eden/fs/service/EdenServiceHandler.h b/eden/fs/service/EdenServiceHandler.h index f097f46262..9c552f9625 100644 --- a/eden/fs/service/EdenServiceHandler.h +++ b/eden/fs/service/EdenServiceHandler.h @@ -46,7 +46,7 @@ class EdenServiceHandler : virtual public StreamingEdenServiceSvIf, std::vector& results, std::unique_ptr mountPoint, std::unique_ptr hash, - bool force) override; + CheckoutMode checkoutMode) override; void resetParentCommits( std::unique_ptr mountPoint, diff --git a/eden/fs/service/eden.thrift b/eden/fs/service/eden.thrift index 6da069e6c8..7c8ca729ee 100644 --- a/eden/fs/service/eden.thrift +++ b/eden/fs/service/eden.thrift @@ -139,6 +139,29 @@ struct ScmStatus { 1: map entries } +/** Option for use with checkOutRevision(). */ +enum CheckoutMode { + /** + * Perform a "normal" checkout, analogous to `hg checkout` in Mercurial. Files + * in the working copy will be changed to reflect the destination snapshot, + * though files with conflicts will not be modified. + */ + NORMAL = 0, + + /** + * Do not checkout: exercise the checkout logic to discover potential + * conflicts. + */ + DRY_RUN = 1, + + /** + * Perform a "forced" checkout, analogous to `hg checkout --clean` in + * Mercurial. Conflicts between the working copy and destination snapshot will + * be forcibly ignored in favor of the state of the new snapshot. + */ + FORCE = 2, +} + enum ConflictType { /** * We failed to update this particular path due to an error @@ -285,31 +308,31 @@ service EdenService extends fb303.FacebookService { void unmount(1: string mountPoint) throws (1: EdenError ex) /** - * Check out the specified snapshot. + * Potentially check out the specified snapshot, reporting conflicts (and + * possibly errors), as appropriate. * - * This updates the contents of the mount point so that they match the - * contents of the given snapshot. - * - * Returns a list of conflicts and errors that occurred when performing the - * checkout operation. - * - * If the force parameter is true, the working directory will be forcibly + * If the checkoutMode is FORCE, the working directory will be forcibly * updated to the contents of the new snapshot, even if there were conflicts. - * Conflicts will still be reported in the return value, but the files will - * be updated to their new state. If the force parameter is false files with - * conflicts will be left unmodified. Files that are untracked in both the - * source and destination snapshots are always left unchanged, even if force - * is true. + * Conflicts will still be reported in the return value, but the files will be + * updated to their new state. * - * On successful return from this function the mount point will point to the - * new commit, even if some paths had conflicts or errors. The caller is - * responsible for taking appropriate action to update these paths as desired - * after checkOutRevision() returns. + * If the checkoutMode is NORMAL, files with conflicts will be left + * unmodified. Files that are untracked in both the source and destination + * snapshots are always left unchanged, even if force is true. + * + * If the checkoutMode is DRY_RUN, then no files are modified in the working + * copy and the current snapshot does not change. However, potential conflicts + * are still reported in the return value. + * + * On successful return from this function (unless it is a DRY_RUN), the mount + * point will point to the new snapshot, even if some paths had conflicts or + * errors. The caller is responsible for taking appropriate action to update + * these paths as desired after checkOutRevision() returns. */ list checkOutRevision( 1: string mountPoint, 2: BinaryHash snapshotHash, - 3: bool force) + 3: CheckoutMode checkoutMode) throws (1: EdenError ex) /** diff --git a/eden/integration/hg/lib/hg_extension_test_base.py b/eden/integration/hg/lib/hg_extension_test_base.py index 76881126f5..57674fec9d 100644 --- a/eden/integration/hg/lib/hg_extension_test_base.py +++ b/eden/integration/hg/lib/hg_extension_test_base.py @@ -56,6 +56,14 @@ def get_default_hgrc() -> configparser.ConfigParser: cases and test case variants. ''' hgrc = configparser.ConfigParser() + # TODO(mbolin): This is supposed to replace experimental.updatecheck, + # but it does not appear to be taking effect today. The + # experimental.updatecheck setting on this hgrc should be removed once + # it has been deprecated and update.check does what it is supposed to + # do. + hgrc['commands'] = { + 'update.check': 'noconflict', + } hgrc['ui'] = { 'origbackuppath': '.hg/origbackups', 'username': 'Kevin Flynn ', @@ -63,6 +71,7 @@ def get_default_hgrc() -> configparser.ConfigParser: hgrc['experimental'] = { 'evolution': 'createmarkers', 'evolutioncommands': 'prev next split fold obsolete metaedit', + 'updatecheck': 'noconflict', } hgrc['extensions'] = { 'absorb': '', diff --git a/eden/integration/hg/update_test.py b/eden/integration/hg/update_test.py index 3781ad6b8d..f834111f2f 100644 --- a/eden/integration/hg/update_test.py +++ b/eden/integration/hg/update_test.py @@ -9,8 +9,7 @@ import os from eden.integration.hg.lib.hg_extension_test_base import ( - EdenHgTestCase, - hg_test + EdenHgTestCase, hg_test ) from eden.integration.lib import hgrepo from textwrap import dedent @@ -201,23 +200,68 @@ class UpdateTest(EdenHgTestCase): ) self.assertEqual(expected_contents, self.read_file('foo/bar.txt')) - def test_update_with_added_file_that_is_tracked_in_destination( + def test_merge_update_added_file_with_same_contents_in_destination( self ) -> None: - self._test_update_with_local_file_that_is_tracked_in_destination(True) + base_commit = self.repo.get_head_hash() - def test_update_with_untracked_file_that_is_tracked_in_destination( + file_contents = 'new file\n' + self.write_file('bar/some_new_file.txt', file_contents) + self.hg('add', 'bar/some_new_file.txt') + self.write_file('foo/bar.txt', 'Modify existing file.\n') + new_commit = self.repo.commit('add some_new_file.txt') + self.assert_status_empty() + + self.repo.update(base_commit) + self.assert_status_empty() + self.write_file('bar/some_new_file.txt', file_contents) + self.hg('add', 'bar/some_new_file.txt') + self.assert_status({'bar/some_new_file.txt': 'A'}) + + # Note the update fails even though some_new_file.txt is the same in + # both the working copy and the destination. + with self.assertRaises(hgrepo.HgError) as context: + self.repo.update(new_commit) + self.assertIn(b'abort: conflicting changes', context.exception.stderr) + self.assertEqual( + base_commit, + self.repo.get_head_hash(), + msg='We should still be on the base commit because ' + 'the merge was aborted.' + ) + self.assert_dirstate( + { + 'bar/some_new_file.txt': ('a', 0, 'MERGE_BOTH'), + } + ) + self.assert_status({'bar/some_new_file.txt': 'A'}) + self.assertEqual(file_contents, self.read_file('bar/some_new_file.txt')) + + # Now do the update with --merge specified. + self.repo.update(new_commit, merge=True) + self.assert_status_empty() + self.assertEqual( + new_commit, + self.repo.get_head_hash(), + msg='Should be expected commit hash because nothing has changed.' + ) + + def test_merge_update_added_file_with_conflict_in_destination(self) -> None: + self._test_merge_update_file_with_conflict_in_destination(True) + + def test_merge_update_untracked_file_with_conflict_in_destination( self ) -> None: - self._test_update_with_local_file_that_is_tracked_in_destination(False) + self._test_merge_update_file_with_conflict_in_destination(False) - def _test_update_with_local_file_that_is_tracked_in_destination( + def _test_merge_update_file_with_conflict_in_destination( self, add_before_updating: bool ) -> None: base_commit = self.repo.get_head_hash() original_contents = 'Original contents.\n' self.write_file('some_new_file.txt', original_contents) self.hg('add', 'some_new_file.txt') + self.write_file('foo/bar.txt', 'Modify existing file.\n') commit = self.repo.commit('Commit a new file.') self.assert_status_empty() @@ -243,10 +287,34 @@ class UpdateTest(EdenHgTestCase): path_to_backup = '.hg/origbackups/some_new_file.txt' expected_backup_file = os.path.join(self.mount, path_to_backup) self.assertFalse(os.path.isfile(expected_backup_file)) - self.repo.update(commit) - self.assertEqual(commit, self.repo.get_head_hash()) - self.assertEqual(original_contents, self.read_file('some_new_file.txt')) - self.assert_status_empty() + with self.assertRaises(hgrepo.HgError) as context: + self.repo.update(commit, merge=True) + self.assertIn( + b'warning: conflicts while merging some_new_file.txt! ' + b'(edit, then use \'hg resolve --mark\')', context.exception.stderr + ) + self.assertEqual( + commit, + self.repo.get_head_hash(), + msg='Even though we have a merge conflict, ' + 'we should still be at the new commit.' + ) + self.assert_dirstate({ + 'some_new_file.txt': ('n', 0, 'MERGE_BOTH'), + }) + self.assert_status({ + 'some_new_file.txt': 'M', + }) + merge_contents = dedent( + '''\ + <<<<<<< working copy + Re-create the file with different contents. + ======= + Original contents. + >>>>>>> destination + ''' + ) + self.assertEqual(merge_contents, self.read_file('some_new_file.txt')) # Verify the previous version of the file was backed up as expected. self.assertTrue(os.path.isfile(expected_backup_file)) @@ -275,16 +343,14 @@ class UpdateTest(EdenHgTestCase): self.hg('add', 'some_new_file.txt') self.repo.commit('Commit a new file.') new_contents = 'Make some changes to that new file.\n' - self.write_file( - 'some_new_file.txt', new_contents - ) + self.write_file('some_new_file.txt', new_contents) self.hg('update', '.^', '--merge', '--tool', ':local') self.assertEqual(new_contents, self.read_file('some_new_file.txt')) self.assert_status({'some_new_file.txt': 'A'}) def test_update_ignores_untracked_directory(self) -> None: - head = self.repo.log()[-1] + base_commit = self.repo.get_head_hash() self.mkdir('foo/bar') self.write_file('foo/bar/a.txt', 'File in directory two levels deep.\n') self.write_file('foo/bar/b.txt', 'Another file.\n') @@ -297,7 +363,7 @@ class UpdateTest(EdenHgTestCase): self.assert_status({ 'foo/bar/b.txt': '?', }) - self.hg('update', head) + self.repo.update(base_commit) self.assert_status({ 'foo/bar/b.txt': '?', }) diff --git a/eden/integration/lib/hgrepo.py b/eden/integration/lib/hgrepo.py index f2d26ee3b5..05bfa63c51 100644 --- a/eden/integration/lib/hgrepo.py +++ b/eden/integration/lib/hgrepo.py @@ -196,12 +196,16 @@ class HgRepository(repobase.Repository): '''Returns the output of `hg status` as a string.''' return self.hg('status') - def update(self, rev: str, clean: bool = False) -> None: + def update( + self, rev: str, clean: bool = False, merge: bool = False + ) -> None: + args = ['update'] if clean: - args = ['update', '--clean', rev] - else: - args = ['update', rev] - self.hg(*args, stdout=None, stderr=None) + args.append('--clean') + if merge: + args.append('--merge') + args.append(rev) + self.hg(*args) def reset(self, rev: str, keep: bool = True) -> None: if keep: