Respect core.commentChar during restacking (#84)

Git supports setting `core.commentChar` to change
which character it uses for comments in commit messages.
This affects the rebase instruction list that restack sees.

This change adds support to restack for this configuration.
If set, the specified character will be used during restacking;
both while searching for comments and when adding comments.

Resolves #72

.changes/unreleased/Added-20230705-054852.yaml

@@ -0,0 +1,3 @@
+kind: Added
+body: Support core.commentChar during restacking.
+time: 2023-07-05T05:48:52.57829209-07:00

fixtures/empty_commit_comment_char_auto.sh

@@ -0,0 +1,4 @@
+#!/usr/bin/env bash
+
+git config core.commentChar 'auto'
+git commit --allow-empty -m "empty commit"

fixtures/simple_stack_comment_char.sh

@@ -0,0 +1,40 @@
+#!/usr/bin/env bash
+
+# This fixture contains a simple stack of commits.
+#
+# o [main] initial commit
+# |
+# o [foo] foo
+# |
+# o [bar] bar
+# |
+# o [baz, wip] baz
+#
+# It also uses ';' as the comment character.
+
+die() {
+	echo >&2 "$@"
+	exit 1
+}
+
+add_and_commit() {
+	[[ $# -eq 1 ]] || die "add_and_commit expects one argument"
+
+	echo "$1" > "$1"
+	git add "$1"
+	git commit -m "add $1"
+}
+
+git commit --allow-empty -m "empty commit"
+git config core.commentChar ';'
+
+git checkout -b foo
+add_and_commit foo
+
+git checkout -b bar
+add_and_commit bar
+
+git checkout -b baz
+add_and_commit baz
+
+git checkout -b wip

src/git.rs

@@ -39,6 +39,9 @@ pub trait Git {
         let git_dir = self.git_dir(dir).context("Failed to find .git directory")?;
         rebase_head_name(&git_dir)
     }
+
+    /// Reports the character used for comments in the repository at the given path.
+    fn comment_char(&self, dir: &path::Path) -> Result<char>;
 }
 
 const REBASE_STATE_DIRS: &[&str] = &["rebase-apply", "rebase-merge"];

src/git/shell.rs

@@ -106,6 +106,39 @@ impl Git for Shell {
 
         Ok(branches)
     }
+
+    fn comment_char(&self, dir: &path::Path) -> Result<char> {
+        let out = self
+            .cmd()
+            // Looking up git config for a field that is unset
+            // will return a non-zero exit code
+            // if we don't specify a default value.
+            .args(["config", "--get", "--default=#", "core.commentChar"])
+            .current_dir(dir)
+            .output()
+            .context("Failed to run git config")?;
+        out.status.exit_ok().context("git config failed")?;
+
+        let output = std::str::from_utf8(out.stdout.trim_ascii_end())
+            .context("Output of git config is not valid UTF-8")?;
+
+        return match output {
+            // In auto, git will pick an unused character from a pre-defined list.
+            // This might be useful to support in the future.
+            "auto" => anyhow::bail!(
+                "core.commentChar=auto is not supported yet. \
+                 Please set core.commentChar to a single character \
+                 or disable restack by unsetting sequence.editor."
+            ),
+
+            // Unreachable but easy enough to handle.
+            "" => Ok('#'),
+
+            // Git will complain if the value is more than one character
+            // so we don't need to handle that case.
+            _ => Ok(output.chars().next().unwrap()),
+        };
+    }
 }
 
 #[cfg(test)]
@@ -237,4 +270,55 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn comment_char_default() -> Result<()> {
+        let fixture = gitscript::open("simple_stack.sh")?;
+
+        let shell = Shell::new();
+        let comment_char = shell.comment_char(fixture.dir())?;
+        assert!(
+            comment_char == '#',
+            "unexpected comment char: '{}'",
+            comment_char
+        );
+
+        Ok(())
+    }
+
+    #[test]
+    fn comment_char() -> Result<()> {
+        let fixture = gitscript::open("simple_stack_comment_char.sh")?;
+
+        let shell = Shell::new();
+        let comment_char = shell.comment_char(fixture.dir())?;
+        assert!(
+            comment_char == ';',
+            "unexpected comment char: '{}'",
+            comment_char
+        );
+
+        Ok(())
+    }
+
+    #[test]
+    fn comment_char_auto() -> Result<()> {
+        let fixture = gitscript::open("empty_commit_comment_char_auto.sh")?;
+
+        let shell = Shell::new();
+
+        // Should return an error.
+        let err = match shell.comment_char(fixture.dir()) {
+            Ok(v) => panic!("expected an error, got {:?}", v),
+            Err(err) => err,
+        };
+
+        assert!(
+            format!("{}", err).contains("core.commentChar=auto"),
+            "got error: {}",
+            err
+        );
+
+        Ok(())
+    }
 }

src/restack.rs

@@ -46,6 +46,15 @@ where
         src: I,
         dst: O,
     ) -> Result<()> {
+        let comment_char = match self.git.comment_char(self.cwd) {
+            Ok(c) => c,
+            Err(e) => {
+                eprintln!("WARN: {}", e);
+                eprintln!("WARN: Falling back to `#` as comment character");
+                '#'
+            },
+        };
+
         let rebase_branch_name = self
             .git
             .rebase_head_name(self.cwd)
@@ -63,6 +72,7 @@ where
         let src = io::BufReader::new(src);
         let mut restack = Restack {
             remote_name,
+            comment_char,
             rebase_branch_name: &rebase_branch_name,
             dst: io::BufWriter::new(dst),
             known_branches: &known_branches,
@@ -91,6 +101,9 @@ struct Restack<'a, O: io::Write> {
     /// Name of the branch we're rebasing.
     rebase_branch_name: &'a str,
 
+    /// Character that starts a comment.
+    comment_char: char,
+
     /// Destination writer.
     dst: io::BufWriter<O>,
 
@@ -119,7 +132,7 @@ impl<'a, O: io::Write> Restack<'a, O> {
 
         // Comments usually mark the end of instructions.
         // Flush optional "git push" statements.
-        if line.get(0..1) == Some("#") {
+        if line.starts_with(self.comment_char) {
             self.update_previous_branches()?;
             self.write_push_section(false, true)
                 .context("Could not write 'git push' section")?;
@@ -141,7 +154,9 @@ impl<'a, O: io::Write> Restack<'a, O> {
         // Most lines go as-is.
         self.write_line(line)?;
 
-        let Some(cmd) = cmd else { return Ok(()); };
+        let Some(cmd) = cmd else {
+            return Ok(());
+        };
         let hash = match cmd {
             "p" | "pick" | "r" | "reword" | "e" | "edit" => match parts.next() {
                 Some(s) => s,
@@ -167,14 +182,24 @@ impl<'a, O: io::Write> Restack<'a, O> {
             return Ok(());
         }
 
-        let Some(remote_name) = self.remote_name else { return Ok(()); };
+        let Some(remote_name) = self.remote_name else {
+            return Ok(());
+        };
 
         if pad_before {
             writeln!(self.dst)?;
         }
-        writeln!(self.dst, "# Uncomment this section to push the changes.")?;
+        writeln!(
+            self.dst,
+            "{} Uncomment this section to push the changes.",
+            self.comment_char
+        )?;
         for br in &self.updated_branches {
-            writeln!(self.dst, "# exec git push -f {} {}", remote_name, br.name)?;
+            writeln!(
+                self.dst,
+                "{} exec git push -f {} {}",
+                self.comment_char, remote_name, br.name
+            )?;
         }
         if pad_after {
             writeln!(self.dst)?;

src/restack/tests.rs

@@ -12,6 +12,7 @@ use crate::git;
 struct StubGit {
     branches: Vec<git::Branch>,
     rebase_head_name: String,
+    comment_char: Option<char>,
 }
 
 impl git::Git for StubGit {
@@ -34,6 +35,10 @@ impl git::Git for StubGit {
     fn rebase_head_name(&self, _: &path::Path) -> Result<String> {
         Ok(self.rebase_head_name.clone())
     }
+
+    fn comment_char(&self, _: &path::Path) -> Result<char> {
+        Ok(self.comment_char.unwrap_or('#'))
+    }
 }
 
 struct TestBranch<'a> {
@@ -45,6 +50,7 @@ struct TestCase<'a> {
     remote_name: Option<&'a str>,
     rebase_head_name: &'a str,
     branches: &'a [TestBranch<'a>],
+    comment_char: Option<char>,
     give: &'a str,
     want: &'a str,
 }
@@ -62,6 +68,7 @@ fn restack_test(test: &TestCase) -> Result<()> {
     let stub_git = StubGit {
         branches,
         rebase_head_name: test.rebase_head_name.to_owned(),
+        comment_char: test.comment_char,
     };
 
     let cfg = Config::new(tempdir.path(), stub_git);
@@ -96,6 +103,7 @@ testcase!(
     TestCase {
         remote_name: Some("origin"),
         rebase_head_name: "feature",
+        comment_char: None,
         branches: &[branch("feature", "hash1")],
         give: indoc! {"
             pick hash0 Do something
@@ -115,6 +123,7 @@ testcase!(
     TestCase {
         remote_name: None,
         rebase_head_name: "foo",
+        comment_char: None,
         branches: &[],
         give: indoc! {"
             pick
@@ -130,6 +139,7 @@ testcase!(
     TestCase {
         remote_name: Some("origin"),
         rebase_head_name: "feature/wip",
+        comment_char: None,
         branches: &[branch("feature/1", "hash1"), branch("feature/wip", "hash1"),],
         give: indoc! {"
             pick hash0 Do something
@@ -155,11 +165,43 @@ testcase!(
     }
 );
 
+testcase!(
+    comment_char_set,
+    TestCase {
+        remote_name: Some("origin"),
+        rebase_head_name: "feature/wip",
+        comment_char: Some(';'),
+        branches: &[branch("feature/1", "hash1"), branch("feature/wip", "hash1"),],
+        give: indoc! {"
+            pick hash0 Do something
+            exec make test
+            pick hash1 Implement feature
+            exec make test
+
+            ; Rebase instructions
+        "},
+        want: indoc! {"
+            pick hash0 Do something
+            exec make test
+            pick hash1 Implement feature
+            exec git branch -f feature/1
+
+            exec make test
+
+            ; Uncomment this section to push the changes.
+            ; exec git push -f origin feature/1
+
+            ; Rebase instructions
+        "},
+    }
+);
+
 testcase!(
     rebase_instructions_comment_missing,
     TestCase {
         remote_name: Some("origin"),
         rebase_head_name: "feature/wip",
+        comment_char: None,
         branches: &[
             branch("feature/1", "hash1"),
             branch("feature/2", "hash3"),
@@ -209,6 +251,7 @@ testcase!(
     TestCase {
         remote_name: None,
         rebase_head_name: "b",
+        comment_char: None,
         branches: &[branch("a", "hash1"), branch("b", "hash3")],
         give: indoc! {"
             pick hash0 do thing
@@ -232,6 +275,7 @@ testcase!(
     TestCase {
         remote_name: None,
         rebase_head_name: "b",
+        comment_char: None,
         branches: &[branch("a", "hash1"), branch("b", "hash3")],
         give: indoc! {"
             pick hash0 do thing
@@ -255,6 +299,7 @@ testcase!(
     TestCase {
         remote_name: Some("origin"),
         rebase_head_name: "stack",
+        comment_char: None,
         branches: &[
             branch("5601-connection-refused", "29b83a30c"),
             branch("5460-publish-bundle", "ae23c4203"),
@@ -320,6 +365,7 @@ testcase!(
     TestCase {
         remote_name: Some("origin"),
         rebase_head_name: "feature/wip",
+        comment_char: None,
         branches: &[branch("feature/1", "hash1"), branch("feature/2", "hash2")],
         give: indoc! {"
             pick hash1 Implement feature 1

tests/edit/mod.rs

@@ -26,10 +26,11 @@ where
 }
 
 #[rstest]
-#[case::editor_flag(true)]
-#[case::editor_env(false)]
-fn simple_stack(#[case] editor_flag: bool) -> Result<()> {
-    let repo_fixture = open_fixture("simple_stack.sh")?;
+#[case::editor_flag(true, "simple_stack.sh")]
+#[case::editor_env(false, "simple_stack.sh")]
+#[case::comment_char(false, "simple_stack_comment_char.sh")]
+fn simple_stack(#[case] editor_flag: bool, #[case] fixture: &str) -> Result<()> {
+    let repo_fixture = open_fixture(fixture)?;
 
     let editor = FIXTURES_DIR.join("bin/add_break.sh");