From 53911b076b56a0176d5e568de824ee567a236abe Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Thu, 14 Apr 2022 23:03:08 -0700
Subject: [PATCH] working_copy: fix crash when updating and only executable bit
 changed

---
 CHANGELOG.md                   |  4 ++++
 lib/src/working_copy.rs        | 28 ++++++++++++++--------------
 lib/tests/test_working_copy.rs | 12 +++++++++++-
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index acd8e61e0..8f544b8fa 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -44,6 +44,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * Errors are now printed to stderr (they used to be printed to stdout).
 
+* Updating the working copy to a commit where a file's executable bit changed
+  but the contents was the same used to lead to a crash. That has now been
+  fixed. 
+
 * `jj untrack` now requires at least one path (allowing no arguments was a UX
   bug).
 
diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs
index bb856cf63..030d2377f 100644
--- a/lib/src/working_copy.rs
+++ b/lib/src/working_copy.rs
@@ -592,23 +592,23 @@ impl TreeState {
                     self.file_states.insert(path.clone(), file_state);
                     stats.added_files += 1;
                 }
+                Diff::Modified(
+                    TreeValue::Normal {
+                        id: old_id,
+                        executable: old_executable,
+                    },
+                    TreeValue::Normal { id, executable },
+                ) if id == old_id => {
+                    // Optimization for when only the executable bit changed
+                    assert_ne!(executable, old_executable);
+                    self.set_executable(&disk_path, executable);
+                    let file_state = self.file_states.get_mut(&path).unwrap();
+                    file_state.mark_executable(executable);
+                    stats.updated_files += 1;
+                }
                 Diff::Modified(before, after) => {
                     fs::remove_file(&disk_path).ok();
                     let file_state = match (before, after) {
-                        (
-                            TreeValue::Normal {
-                                id: old_id,
-                                executable: old_executable,
-                            },
-                            TreeValue::Normal { id, executable },
-                        ) if id == old_id => {
-                            // Optimization for when only the executable bit changed
-                            assert_ne!(executable, old_executable);
-                            self.set_executable(&disk_path, executable);
-                            let mut file_state = self.file_states.get(&path).unwrap().clone();
-                            file_state.mark_executable(executable);
-                            file_state
-                        }
                         (_, TreeValue::Normal { id, executable }) => {
                             self.write_file(&disk_path, &path, &id, executable)
                         }
diff --git a/lib/tests/test_working_copy.rs b/lib/tests/test_working_copy.rs
index 62e7b3fbc..f3f5513fc 100644
--- a/lib/tests/test_working_copy.rs
+++ b/lib/tests/test_working_copy.rs
@@ -66,6 +66,8 @@ fn test_checkout_file_transitions(use_git: bool) {
         Missing,
         Normal,
         Executable,
+        // Executable, but same content as Normal, to test transition where only the bit changed
+        ExecutableNormalContent,
         Conflict,
         #[cfg_attr(windows, allow(dead_code))]
         Symlink,
@@ -99,6 +101,13 @@ fn test_checkout_file_transitions(use_git: bool) {
                     executable: true,
                 }
             }
+            Kind::ExecutableNormalContent => {
+                let id = testutils::write_file(store, path, "normal file contents");
+                TreeValue::Normal {
+                    id,
+                    executable: true,
+                }
+            }
             Kind::Conflict => {
                 let base_file_id = testutils::write_file(store, path, "base file contents");
                 let left_file_id = testutils::write_file(store, path, "left file contents");
@@ -162,6 +171,7 @@ fn test_checkout_file_transitions(use_git: bool) {
         Kind::Missing,
         Kind::Normal,
         Kind::Executable,
+        Kind::ExecutableNormalContent,
         Kind::Conflict,
         Kind::Tree,
     ];
@@ -217,7 +227,7 @@ fn test_checkout_file_transitions(use_git: bool) {
                     path
                 );
             }
-            Kind::Executable => {
+            Kind::Executable | Kind::ExecutableNormalContent => {
                 assert!(maybe_metadata.is_ok(), "{:?} should exist", path);
                 let metadata = maybe_metadata.unwrap();
                 assert!(metadata.is_file(), "{:?} should be a file", path);