properly update file modes during checkout

Summary:
# The problem
Users were running into issues with files not showing up as the correct type. For example, a user expected file `foo/bar` to be EXECUTABLE, but in reality it was REGULAR. This happened because of the following scenario:

Commit A has a file `foo/bar` which is a REGULAR file
Commit B updates foo/bar to EXECUTABLE

Any update from Commit A to after Commit B would cause foo/bar to exist in the working copy as a REGULAR file instead of an EXECUTABLE file.

Users would not notice this issue, the file would become materialized, and then they would accidentally commit the change that caused `foo/bar` to become a REGULAR file again (Commit C).

This commit would add even more opportunities for people to update from A -> B or B -> C or some combination, and this would add even more chances for the file's mode to become changed.

# The bug and solution
During checkout, we fail to check if a file's mode has changed. We do a check to see if the contents of the file change, but we don't check to see if the file mode has changed. This caused us to fail to update modes across commits that changed file modes.

The solution is to avoid returning early when the modes of oldScmEntry and newScmEntry differ while processing a checkout entry.

Reviewed By: xavierd

Differential Revision: D48667112

fbshipit-source-id: cb424f11cb95df46239ee49d4989baa7c869bc94

eden/fs/inodes/TreeInode.cpp

@@ -3466,8 +3466,15 @@ unique_ptr<CheckoutAction> TreeInode::processCheckoutEntry(
       newScmEntry &&
       getObjectStore().areObjectsKnownIdentical(
           entry.getHash(), newScmEntry->second.getHash())) {
-    // The inode already matches the checkout destination. So do nothing.
-    return nullptr;
+    if (filteredEntryType(
+            oldScmEntry->second.getType(), windowsSymlinksEnabled) ==
+        filteredEntryType(
+            newScmEntry->second.getType(), windowsSymlinksEnabled)) {
+      // The inode already matches the checkout destination. So do nothing.
+      return nullptr;
+    }
+    // The types don't match, so we should fall through and update the
+    // entry. An example is when a file goes from REGULAR -> EXECUTABLE.
   } else {
     switch (getObjectStore().compareObjectsById(
         entry.getHash(), oldScmEntry->second.getHash())) {

eden/integration/hg/update_test.py

@@ -64,6 +64,21 @@ class UpdateTest(EdenHgTestCase):
         repo.write_file("foo/bar.txt", "updated in commit 3\n")
         self.commit3 = repo.commit("Update foo/.gitignore")
 
+    def test_mode_change_with_no_content_change(self) -> None:
+        """Test changing the mode of a file but NOT the contents."""
+        self.assert_status_empty()
+
+        self.chmod("hello.txt", 0o755)
+        self.assert_status({"hello.txt": "M"})
+        commit4 = self.repo.commit("Update hello.txt mode")
+
+        self.repo.update(self.commit1)
+        self.repo.update(commit4)
+        self.touch("hello.txt")
+        self.repo.update(self.commit1)
+        self.repo.update(commit4)
+        self.assert_status_empty()
+
     def test_update_clean_reverts_modified_files(self) -> None:
         """Test using `hg update --clean .` to revert file modifications."""
         self.assert_status_empty()

eden/integration/lib/skip.py

@@ -114,9 +114,7 @@ if sys.platform == "win32":
             "test_grep_directory_from_root",
             "test_grep_directory_from_subdirectory",
         ],
-        "hg.rebase_test.RebaseTest": [
-            "test_rebase_commit_with_independent_folder"
-        ],
+        "hg.rebase_test.RebaseTest": ["test_rebase_commit_with_independent_folder"],
         "hg.rm_test.RmTest": [
             "test_rm_directory_with_modification",
             "test_rm_modified_file_permissions",
@@ -136,6 +134,8 @@ if sys.platform == "win32":
         "hg.update_test.UpdateTest": [
             # TODO: A \r\n is used
             "test_mount_state_during_unmount_with_in_progress_checkout",
+            # Windows doesn't support executable files; mode changes are no-op
+            "test_mode_change_with_no_content_change",
         ],
         "hg.update_test.UpdateTestTreeOnlyInMemory": [
             # kill and restart Eden
@@ -294,16 +294,21 @@ if sys.platform != "win32":
         }
     )
 
+
 def _have_ntapi_extension_module() -> bool:
     if sys.platform != "win32":
         return False
 
     try:
-        from eden.integration.lib.ntapi import get_directory_entry_size  # @manual # @nolint
+        from eden.integration.lib.ntapi import (  # @manual  # @nolint
+            get_directory_entry_size,
+        )
+
         return True
     except ImportError:
         return False
 
+
 # ProjFS enumeration tests depend on a Python extension module, which may not be
 # available with all build systems.
 if not _have_ntapi_extension_module():

eden/integration/lib/testcase.py

@@ -314,6 +314,11 @@ class EdenTestCase(EdenTestCaseBase):
             f.write(contents.encode())
         os.chmod(fullpath, mode)
 
+    def chmod(self, path: str, mode: int = 0o644) -> None:
+        """Create or overwrite a file with the given contents."""
+        fullpath = self.get_path(path)
+        os.chmod(fullpath, mode)
+
     def rename(self, from_path: str, to_path: str) -> None:
         """Rename a file/directory at the specified paths relative to the
         clone.