fix: stg import --reject should create empty commit

When trying to import a patch that does not apply cleanly, and using
--reject option, stg should apply what it can, leave the rest in .rej
files, and create an empty commit. The work to apply the patch is
outsourced to "git apply --reject" which exits with status code 1 if
patch is applied partially and it is treated as error by stg import.

Fix the issue by not treating return code of 1 from "git apply" as
an error when "--reject" option is specified, but rather saving its
output, printing it for the user, and continuing with the rest of
the import logic. Exit with CONFLICT_ERROR rather than COMMAND_ERROR
when the patch does not import/apply cleanly.

"stg fold" reuses much of the same code so it has to be adjusted in
the similar fashion. It will also exit with CONFLICT_ERROR when a
patch does not apply cleanly.

Also add a test case and fix up documentation for "stg import" and
adjust test case for "stg fold".

Closes: #471

Signed-off-by: Dmitry Torokhov <dtor@google.com>

src/cmd/fold.rs

@@ -11,6 +11,7 @@ use clap::{Arg, ArgGroup};
 use crate::{
     ext::{CommitExtended, RepositoryExtended},
     patch::SingleRevisionSpec,
+    print_info_message,
     stack::{InitializationPolicy, Stack, StackAccess, StackStateAccess},
     stupid::Stupid,
 };
@@ -124,32 +125,50 @@ fn run(matches: &clap::ArgMatches) -> Result<()> {
         let orig_head_tree_id = stack.get_branch_head().tree_id()?.detach();
         let base_tree_id = base_commit.tree_id()?.detach();
         stupid.read_tree_checkout(orig_head_tree_id, base_tree_id)?;
-        if let Err(e) = stupid.apply_to_worktree_and_index(
+        let applied_cleanly = match stupid.apply_to_worktree_and_index(
             diff.as_ref(),
             reject_flag,
             false,
             strip_level,
             None,
             context_lines,
         ) {
-            stupid.read_tree_checkout_hard(orig_head_tree_id)?;
-            return Err(e);
-        }
+            Ok(None) => true,
+            Ok(Some(output)) => {
+                // If patch applied with conflicts print output of "git apply".
+                print_info_message(matches, &output);
+                false
+            }
+            Err(e) => {
+                // Reset the work tree on failure.
+                stupid.read_tree_checkout_hard(orig_head_tree_id)?;
+                return Err(e);
+            }
+        };
         let applied_tree_id = stupid.write_tree()?;
         stupid.read_tree_checkout(applied_tree_id, orig_head_tree_id)?;
-        if stupid.merge_recursive(base_tree_id, orig_head_tree_id, applied_tree_id)? {
-            Ok(())
-        } else {
-            Err(super::Error::CausedConflicts("merge conflicts".to_string()).into())
+        if !stupid.merge_recursive(base_tree_id, orig_head_tree_id, applied_tree_id)? {
+            return Err(super::Error::CausedConflicts("merge conflicts".to_string()).into());
+        }
+        if !applied_cleanly {
+            return Err(super::Error::CausedConflicts("patch conflicts".to_string()).into());
         }
+        Ok(())
     } else {
-        stupid.apply_to_worktree_and_index(
+        match stupid.apply_to_worktree_and_index(
             diff.as_ref(),
             reject_flag,
             false,
             strip_level,
             None,
             context_lines,
-        )
+        ) {
+            Ok(None) => Ok(()),
+            Ok(Some(output)) => {
+                print_info_message(matches, &output);
+                Err(super::Error::CausedConflicts("patch conflicts".to_string()).into())
+            }
+            Err(e) => Err(e),
+        }
     }
 }

src/cmd/import.rs

@@ -15,6 +15,7 @@ use crate::{
     color::get_color_stdout,
     ext::{RepositoryExtended, TimeExtended},
     patch::{patchedit, PatchName},
+    print_info_message,
     stack::{InitializationPolicy, Stack, StackAccess, StackStateAccess},
     stupid::Stupid,
 };
@@ -44,8 +45,10 @@ fn make() -> clap::Command {
              allows the patches source to be fetched from a url instead of from a \
              local file.\n\
              \n\
-             If a patch does not apply cleanly, the failed diff is written to a \
-             .stgit-failed.patch file and an empty patch is added to the stack.\n\
+             If a patch does not apply cleanly import is aborted unless '--reject' \
+             is specified, in which case it will apply to the work tree the parts \
+             of the patch that are  applicable, leave the rejected hunks in \
+             corresponding *.rej files, and add an empty patch to the stack.\n\
              \n\
              The patch description must be separated from the diff with a \"---\" line.",
         )
@@ -603,15 +606,15 @@ fn create_patch<'repo>(
         }
     };
 
+    let stupid = stack.repo.stupid();
     let strip_level = strip_level.or_else(|| matches.get_one::<usize>("strip").copied());
 
     let trimmed_diff = diff.trim_end_with(|c| c.is_ascii_whitespace());
 
-    let tree_id = if trimmed_diff.is_empty() || trimmed_diff == b"---" {
-        stack.get_branch_head().tree_id()?.detach()
+    let applied_cleanly = if trimmed_diff.is_empty() || trimmed_diff == b"---" {
+        true
     } else {
-        let stupid = stack.repo.stupid();
-        stupid.apply_to_worktree_and_index(
+        match stupid.apply_to_worktree_and_index(
             diff,
             matches.get_flag("reject"),
             matches.get_flag("3way"),
@@ -620,11 +623,19 @@ fn create_patch<'repo>(
                 .get_one::<PathBuf>("directory")
                 .map(|path_buf| path_buf.as_path()),
             matches.get_one::<usize>("context-lines").copied(),
-        )?;
-
-        stupid.write_tree()?
+        ) {
+            Ok(None) => true,
+            Ok(Some(output)) => {
+                print_info_message(matches, &output);
+                false
+            }
+            Err(e) => return Err(e),
+        }
     };
 
+    // If the patch was empty this will not create a new tree object.
+    let tree_id = stupid.write_tree()?;
+
     let (new_patchname, commit_id) = match crate::patch::edit::EditBuilder::default()
         .original_patchname(Some(&patchname))
         .override_tree_id(tree_id)
@@ -647,7 +658,7 @@ fn create_patch<'repo>(
         ),
     };
 
-    stack
+    let stack = stack
         .setup_transaction()
         .with_output_stream(get_color_stdout(matches))
         .use_index_and_worktree(false)
@@ -658,7 +669,13 @@ fn create_patch<'repo>(
             }
             trans.new_applied(&new_patchname, commit_id)
         })
-        .execute(&format!("import: {new_patchname}"))
+        .execute(&format!("import: {new_patchname}"))?;
+
+    if !applied_cleanly {
+        return Err(super::Error::CausedConflicts("patch conflicts".to_string()).into());
+    }
+
+    Ok(stack)
 }
 
 fn stripname(name: &str) -> &str {

src/stupid/context.rs

@@ -121,6 +121,11 @@ impl<'repo, 'index> StupidContext<'repo, 'index> {
         Ok(())
     }
 
+    /// Apply a patch (diff) to both the index and the working tree.
+    ///
+    /// Returns `None` if the patch applies cleanly or output of the `git apply`
+    /// command if the patch does not apply and caller indicated that rejects
+    /// are allowed.
     pub(crate) fn apply_to_worktree_and_index(
         &self,
         diff: &BStr,
@@ -129,7 +134,7 @@ impl<'repo, 'index> StupidContext<'repo, 'index> {
         strip_level: Option<usize>,
         directory: Option<&Path>,
         context_lines: Option<usize>,
-    ) -> Result<()> {
+    ) -> Result<Option<String>> {
         let mut command = self.git_in_work_root()?;
         command.args(["apply", "--index"]);
         if reject {
@@ -148,11 +153,17 @@ impl<'repo, 'index> StupidContext<'repo, 'index> {
         if let Some(context_lines) = context_lines {
             command.arg(format!("-C{context_lines}"));
         }
-        command
-            .stdout(Stdio::null())
-            .in_and_out(diff)?
-            .require_success("apply --index")?;
-        Ok(())
+        let apply_output = command.stdout(Stdio::null()).in_and_out(diff)?;
+        if apply_output.status.success() {
+            Ok(None)
+        } else {
+            let err = git_command_error("apply --index", &apply_output.stderr);
+            if reject && apply_output.status.code() == Some(1) {
+                Ok(Some(format!("{err:#}")))
+            } else {
+                Err(err)
+            }
+        }
     }
 
     /// Apply diff between two trees to specified index.

t/t1800-import.sh

@@ -366,6 +366,16 @@ test_expect_success 'Import series from stdin' '
     stg delete --top
 '
 
+test_expect_success 'Import patch that does not apply cleanly with --reject' '
+    conflict stg import --reject "$TEST_DIRECTORY"/t1800/diff-with-rejects 2>err &&
+    git log -1 --pretty=format:%b >body &&
+    test_when_finished "rm foo.txt.rej err body" &&
+    test "$(echo $(stg top))" = "diff-with-rejects" &&
+    test_line_count = 0 body &&
+    stg reset --hard &&
+    stg delete --top
+'
+
 test_expect_success STG_IMPORT_URL 'Attempt url' '
     general_error stg import --url 2>err &&
     grep -e "required arguments were not provided" err

t/t1800/diff-with-rejects

@@ -0,0 +1,29 @@
+test patch with rejects
+
+---
+
+ t/t1800/foo.txt | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/t/t1800/foo.txt b/t/t1800/foo.txt
+index ad01662f..d609de80 100644
+--- a/foo.txt
++++ b/foo.txt
+@@ -6,7 +6,7 @@ dobidim
+ dobidum
+ dobodam
+ dobodim
+-dobodum
++dObodum
+ dibedam
+ dibedim
+ dibedum
+@@ -18,7 +18,7 @@ dibodim
+ dibodum
+ dabedam
+ dabedim
+-Dabedum
++dAbedum
+ dabidam
+ dabidim
+ dabidum

t/t3600-fold.sh

@@ -91,7 +91,7 @@ test_expect_success 'Attempt to fold conflicting patch with rejects' '
     echo "hello" >foo.txt &&
     echo "from p2" >>foo.txt &&
     stg refresh &&
-    command_error stg fold --reject fold1.diff 2>err &&
+    conflict stg fold --reject fold1.diff 2>err &&
     grep "patch failed" err &&
     test -z "$(echo $(stg status --porcelain foo.txt))" &&
     test -e foo.txt.rej &&