From bf648739ae0da4a1e45521e8d6580a230d3b94cc Mon Sep 17 00:00:00 2001
From: Ruben Arts <ruben.arts@hotmail.com>
Date: Fri, 20 Dec 2024 17:12:57 +0100
Subject: [PATCH] fix: undo changes to manifest if it fails to solve and
 install on add or upgrade

---
 src/cli/add.rs                            | 17 +++++++++++++++--
 src/cli/upgrade.rs                        | 16 ++++++++++++++--
 src/project/mod.rs                        |  4 ++--
 tests/integration_python/test_main_cli.py | 18 ++++++++++++++++++
 4 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/src/cli/add.rs b/src/cli/add.rs
index 259ec5139..d0cfe58b6 100644
--- a/src/cli/add.rs
+++ b/src/cli/add.rs
@@ -1,5 +1,6 @@
 use clap::Parser;
 use indexmap::IndexMap;
+use miette::IntoDiagnostic;
 use pixi_manifest::FeatureName;
 
 use super::has_specs::HasSpecs;
@@ -120,7 +121,11 @@ pub async fn execute(args: Args) -> miette::Result<()> {
     // TODO: add dry_run logic to add
     let dry_run = false;
 
-    let update_deps = project
+    // Save original manifest
+    let original_manifest_content =
+        fs_err::read_to_string(project.manifest_path()).into_diagnostic()?;
+
+    let update_deps = match project
         .update_dependencies(
             match_specs,
             pypi_deps,
@@ -130,7 +135,15 @@ pub async fn execute(args: Args) -> miette::Result<()> {
             args.editable,
             dry_run,
         )
-        .await?;
+        .await
+    {
+        Ok(update_deps) => update_deps,
+        Err(e) => {
+            // Restore original manifest
+            fs_err::write(project.manifest_path(), original_manifest_content).into_diagnostic()?;
+            return Err(e);
+        }
+    };
 
     if let Some(update_deps) = update_deps {
         // Notify the user we succeeded
diff --git a/src/cli/upgrade.rs b/src/cli/upgrade.rs
index ce914f179..20c594b58 100644
--- a/src/cli/upgrade.rs
+++ b/src/cli/upgrade.rs
@@ -68,7 +68,11 @@ pub async fn execute(args: Args) -> miette::Result<()> {
 
     let (match_specs, pypi_deps) = parse_specs(feature, &args, &project)?;
 
-    let update_deps = project
+    // Save original manifest
+    let original_manifest_content =
+        fs_err::read_to_string(project.manifest_path()).into_diagnostic()?;
+
+    let update_deps = match project
         .update_dependencies(
             match_specs,
             pypi_deps,
@@ -78,7 +82,15 @@ pub async fn execute(args: Args) -> miette::Result<()> {
             false,
             args.dry_run,
         )
-        .await?;
+        .await
+    {
+        Ok(update_deps) => update_deps,
+        Err(e) => {
+            // Restore original manifest
+            fs_err::write(project.manifest_path(), original_manifest_content).into_diagnostic()?;
+            return Err(e);
+        }
+    };
 
     // Is there something to report?
     if let Some(update_deps) = update_deps {
diff --git a/src/project/mod.rs b/src/project/mod.rs
index a27ef18c0..d0ab15fa3 100644
--- a/src/project/mod.rs
+++ b/src/project/mod.rs
@@ -714,7 +714,7 @@ impl Project {
             }
         }
 
-        // Only save to disk if not a dry run
+        // TODO: Figure out if we really need this save here
         if !dry_run {
             self.save()?;
         }
@@ -803,7 +803,7 @@ impl Project {
             implicit_constraints.extend(pypi_constraints);
         }
 
-        // Only write to disk if not a dry run
+        // TODO: figure out if we really need this save here.
         if !dry_run {
             self.save()?;
         }
diff --git a/tests/integration_python/test_main_cli.py b/tests/integration_python/test_main_cli.py
index 5c222b4d5..399231c09 100644
--- a/tests/integration_python/test_main_cli.py
+++ b/tests/integration_python/test_main_cli.py
@@ -714,3 +714,21 @@ def test_concurrency_flags(
             "package3",
         ]
     )
+
+
+def test_dont_add_broken_dep(pixi: Path, tmp_pixi_workspace: Path, dummy_channel_1: str) -> None:
+    manifest_path = tmp_pixi_workspace / "pixi.toml"
+
+    # Create a new project
+    verify_cli_command([pixi, "init", "--channel", dummy_channel_1, tmp_pixi_workspace])
+
+    manifest_content = tmp_pixi_workspace.joinpath("pixi.toml").read_text()
+
+    # Add a non existing package should error
+    verify_cli_command(
+        [pixi, "add", "--manifest-path", manifest_path, "dummy-a=1000000"],
+        ExitCode.FAILURE,
+    )
+
+    # It should not have modified the manifest on failure
+    assert manifest_content == tmp_pixi_workspace.joinpath("pixi.toml").read_text()