[gpgsign-at-last] Add support for GPG-signed commits (mystor#46)

GPG-sign commits if option -S/--gpg-sign is given, or if configurations
revise.gpgSign or commit.gpgSign are set to true.

Unlike Git, we do not support passing the key ID as argument.  Git's
way of accepting this optional argument is a bit weird.  It would be
awkward to try to support the same with argparse.  So it's probably
best to use a separate argument for that in future.

For now, the key ID is taken from Git configuration (user.signingKey)
or the committer signature as fallback.

A call of `git-revise COMMIT --gpg-sign` w/o any changes in the index
causes creation of GPG commit signatures for all commits since COMMIT.

Conversely, if GPG signing is disabled, git-revise will remove
the signature, even if the commits were not modified otherwise.
So adding/removing a signature is considered a modification, which
seems consistent.  This logic is applied to all commits in the
todo-list for more consistency.

The test is a bit large, but the pieces are mostly independenty.  I'm
not sure if it's worth to have a separate GPG secret key initialization
for each test, and I don't know how to share that.

Closes # 46
Closes # 73

Co-authored-by: Johannes Altmanninger <aclopte@gmail.com>
Signed-off-by: Nicolas Schier <nicolas@fjasle.eu>

CHANGELOG.md

@@ -6,6 +6,7 @@
 * Fix autosquash order of multiple fixup commits with the same target (#72)
 * Use `GIT_SEQUENCE_EDITOR` instead of `SEQUENCE_EDITOR` (#71)
 * Fix handling of multiline commit subjects (#86)
+* Add support for `commit.gpgSign` (#46)
 
 ## v0.6.0
 

docs/man.rst

@@ -58,6 +58,11 @@ General options
 
    Working branch to update; defaults to ``HEAD``.
 
+.. option:: -S, --gpg-sign, --no-gpg-sign
+
+   GPG-sign commits.  Overrides both the ``commit.gpgSign`` and
+   ``revise.gpgSign`` git configurations.
+
 Main modes of operation
 -----------------------
 
@@ -126,6 +131,13 @@ Configuration is managed by :manpage:`git-config(1)`.
    is specified. Overridden by :option:`--no-autosquash`. Defaults to false. If
    not set, the value of ``rebase.autoSquash`` is used instead.
 
+.. gitconfig:: revise.gpgSign
+
+   If set to true, GPG-sign new commits; defaults to false.  This setting
+   overrides the original git configuration ``commit.gpgSign`` and may be
+   overridden by the command line options ``--gpg-sign`` and
+   ``--no-gpg-sign``.
+
 
 CONFLICT RESOLUTION
 ===================

git-revise.1

@@ -79,6 +79,12 @@ Reset target commit\(aqs author to the current user.
 .B \-\-ref <gitref>
 Working branch to update; defaults to \fBHEAD\fP\&.
 .UNINDENT
+.INDENT 0.0
+.TP
+.B \-S, \-\-gpg\-sign, \-\-no\-gpg\-sign
+GPG\-sign commits.  Overrides both the \fBcommit.gpgSign\fP and
+\fBrevise.gpgSign\fP git configurations.
+.UNINDENT
 .SS Main modes of operation
 .INDENT 0.0
 .TP
@@ -147,6 +153,14 @@ If set to true, imply \fI\%\-\-autosquash\fP whenever \fI\%\-\-interactive\fP
 is specified. Overridden by \fI\%\-\-no\-autosquash\fP\&. Defaults to false. If
 not set, the value of \fBrebase.autoSquash\fP is used instead.
 .UNINDENT
+.INDENT 0.0
+.TP
+.B revise.gpgSign
+If set to true, GPG\-sign new commits; defaults to false.  This setting
+overrides the original git configuration \fBcommit.gpgSign\fP and may be
+overridden by the command line options \fB\-\-gpg\-sign\fP and
+\fB\-\-no\-gpg\-sign\fP\&.
+.UNINDENT
 .SH CONFLICT RESOLUTION
 .sp
 When a conflict is encountered, \fBgit revise\fP will attempt to resolve

gitrevise/odb.py

@@ -17,6 +17,7 @@
     Tuple,
     cast,
 )
+import sys
 from types import TracebackType
 from pathlib import Path
 from enum import Enum
@@ -32,6 +33,13 @@ def __init__(self, ref: str) -> None:
         Exception.__init__(self, f"Object {ref} does not exist")
 
 
+class GPGSignError(Exception):
+    """Exception raised when we fail to sign a commit"""
+
+    def __init__(self, stderr: str) -> None:
+        Exception.__init__(self, f"unable to sign object: {stderr}")
+
+
 T = TypeVar("T")  # pylint: disable=invalid-name
 
 
@@ -80,7 +88,10 @@ class Signature(bytes):
 
     sig_re = re.compile(
         rb"""
-        (?P<name>[^<>]+)<(?P<email>[^<>]+)>[ ]
+        (?P<signing_key>
+            (?P<name>[^<>]+)<(?P<email>[^<>]+)>
+        )
+        [ ]
         (?P<timestamp>[0-9]+)
         (?:[ ](?P<offset>[\+\-][0-9]+))?
         """,
@@ -101,6 +112,13 @@ def email(self) -> bytes:
         assert match, "invalid signature"
         return match.group("email").strip()
 
+    @property
+    def signing_key(self) -> bytes:
+        """user name <email>"""
+        match = self.sig_re.fullmatch(self)
+        assert match, "invalid signature"
+        return match.group("signing_key").strip()
+
     @property
     def timestamp(self) -> bytes:
         """unix timestamp"""
@@ -134,6 +152,12 @@ class Repository:
     index: "Index"
     """current index state"""
 
+    sign_commits: bool
+    """sign commits with gpg"""
+
+    gpg: bytes
+    """path to GnuPG binary"""
+
     _objects: Dict[int, Dict[Oid, "GitObj"]]
     _catfile: Popen
     _tempdir: Optional[TemporaryDirectory]
@@ -144,6 +168,8 @@ class Repository:
         "default_author",
         "default_committer",
         "index",
+        "sign_commits",
+        "gpg",
         "_objects",
         "_catfile",
         "_tempdir",
@@ -162,6 +188,12 @@ def __init__(self, cwd: Optional[Path] = None) -> None:
 
         self.index = Index(self)
 
+        self.sign_commits = self.bool_config(
+            "revise.gpgSign", default=self.bool_config("commit.gpgSign", default=False)
+        )
+
+        self.gpg = self.config("gpg.program", default=b"gpg")
+
         # Pylint 2.8 emits a false positive; fixed in 2.9.
         self._catfile = Popen(  # pylint: disable=consider-using-with
             ["git", "cat-file", "--batch"],
@@ -274,10 +306,43 @@ def new_commit(
             body += b"parent " + parent.oid.hex().encode() + b"\n"
         body += b"author " + author + b"\n"
         body += b"committer " + committer + b"\n"
-        body += b"\n"
-        body += message
+
+        body_tail = b"\n" + message
+        body += self.sign_buffer(body + body_tail)
+        body += body_tail
+
         return Commit(self, body)
 
+    def sign_buffer(self, buffer: bytes) -> bytes:
+        """Return the text of the signed commit object."""
+        if not self.sign_commits:
+            return b""
+
+        key_id = self.config(
+            "user.signingKey", default=self.default_committer.signing_key
+        )
+        gpg = None
+        try:
+            gpg = run(
+                (self.gpg, "--status-fd=2", "-bsau", key_id),
+                stdout=PIPE,
+                stderr=PIPE,
+                input=buffer,
+                check=True,
+            )
+        except CalledProcessError as gpg:
+            print(gpg.stderr.decode(), file=sys.stderr, end="")
+            print("gpg failed to sign commit", file=sys.stderr)
+            raise
+
+        if b"\n[GNUPG:] SIG_CREATED " not in gpg.stderr:
+            raise GPGSignError(gpg.stderr.decode())
+
+        signature = b"gpgsig"
+        for line in gpg.stdout.splitlines():
+            signature += b" " + line + b"\n"
+        return signature
+
     def new_tree(self, entries: Mapping[bytes, "Entry"]) -> "Tree":
         """Directly create an in-memory tree object, without persisting it.
         If a tree object with these entries already exists, it will be
@@ -476,10 +541,13 @@ class Commit(GitObj):
     committer: Signature
     """:class:`Signature` of this commit's committer"""
 
+    gpgsig: Optional[bytes]
+    """GPG signature of this commit"""
+
     message: bytes
     """Body of this commit's message"""
 
-    __slots__ = ("tree_oid", "parent_oids", "author", "committer", "message")
+    __slots__ = ("tree_oid", "parent_oids", "author", "committer", "gpgsig", "message")
 
     def _parse_body(self) -> None:
         # Split the header from the core commit message.
@@ -493,6 +561,7 @@ def _parse_body(self) -> None:
             key, value = hdr.split(maxsplit=1)
             value = value.replace(b"\n ", b"\n")
 
+            self.gpgsig = None
             if key == b"tree":
                 self.tree_oid = Oid.fromhex(value.decode())
             elif key == b"parent":
@@ -501,6 +570,8 @@ def _parse_body(self) -> None:
                 self.author = Signature(value)
             elif key == b"committer":
                 self.committer = Signature(value)
+            elif key == b"gpgsig":
+                self.gpgsig = value
 
     def tree(self) -> "Tree":
         """``tree`` object corresponding to this commit"""
@@ -539,6 +610,7 @@ def update(
         parents: Optional[Sequence["Commit"]] = None,
         message: Optional[bytes] = None,
         author: Optional[Signature] = None,
+        recommit: bool = False,
     ) -> "Commit":
         """Create a new commit with specific properties updated or replaced"""
         # Compute parameters used to create the new object.
@@ -551,16 +623,18 @@ def update(
         if author is None:
             author = self.author
 
-        # Check if the commit was unchanged to avoid creating a new commit if
-        # only the committer has changed.
-        unchanged = (
-            tree == self.tree()
-            and parents == self.parents()
-            and message == self.message
-            and author == self.author
-        )
-        if unchanged:
-            return self
+        if not recommit:
+            # Check if the commit was unchanged to avoid creating a new commit if
+            # only the committer has changed.
+            unchanged = (
+                tree == self.tree()
+                and parents == self.parents()
+                and message == self.message
+                and author == self.author
+            )
+            if unchanged:
+                return self
+
         return self.repo.new_commit(tree, parents, message, author)
 
     def _persist_deps(self) -> None:

gitrevise/tui.py

@@ -88,6 +88,19 @@ def build_parser() -> ArgumentParser:
         action="store_true",
         help="interactively cut a commit into two smaller commits",
     )
+
+    gpg_group = parser.add_mutually_exclusive_group()
+    gpg_group.add_argument(
+        "--gpg-sign",
+        "-S",
+        action="store_true",
+        help="GPG sign commits",
+    )
+    gpg_group.add_argument(
+        "--no-gpg-sign",
+        action="store_true",
+        help="do not GPG sign commits",
+    )
     return parser
 
 
@@ -172,11 +185,20 @@ def noninteractive(
     if args.cut:
         current = cut_commit(current)
 
-    if current != replaced:
+    # Add or remove GPG signatures.
+    if repo.sign_commits != bool(current.gpgsig):
+        current = current.update(recommit=True)
+    change_signature = any(
+        repo.sign_commits != bool(commit.gpgsig) for commit in to_rebase
+    )
+
+    if current != replaced or change_signature:
         print(f"{current.oid.short()} {current.summary()}")
 
         # Rebase commits atop the commit range.
         for commit in to_rebase:
+            if repo.sign_commits != bool(commit.gpgsig):
+                commit = commit.update(recommit=True)
             current = commit.rebase(current)
             print(f"{current.oid.short()} {current.summary()}")
 
@@ -192,6 +214,11 @@ def inner_main(args: Namespace, repo: Repository) -> None:
     if args.patch:
         repo.git("add", "-p")
 
+    if args.gpg_sign:
+        repo.sign_commits = True
+    if args.no_gpg_sign:
+        repo.sign_commits = False
+
     # Create a commit with changes from the index
     staged = None
     if not args.no_index:

tests/test_gpgsign.py

@@ -0,0 +1,77 @@
+# pylint: skip-file
+
+from conftest import *
+from subprocess import CalledProcessError
+
+
+def test_gpgsign(repo):
+    bash("git commit --allow-empty -m 'commit 1'")
+    assert repo.get_commit("HEAD").gpgsig is None
+
+    bash(
+        """
+        mkdir ~/.gnupg/
+        cat > ~/.gnupg/gpg.conf << EOF
+        pinentry-mode loopback
+        EOF
+
+        name=$(git config user.name)
+        email=$(git config user.email)
+        gpg --batch --passphrase "" --quick-gen-key "$name <$email>" >/dev/null 2>&1
+    """
+    )
+
+    bash("git config commit.gpgSign true")
+    main(["HEAD"])
+    assert (
+        repo.get_commit("HEAD").gpgsig is not None
+    ), "git config commit.gpgSign activates GPG signing"
+
+    bash("git config revise.gpgSign false")
+    main(["HEAD"])
+    assert (
+        repo.get_commit("HEAD").gpgsig is None
+    ), "git config revise.gpgSign overrides commit.gpgSign"
+
+    main(["HEAD", "--gpg-sign"])
+    assert (
+        repo.get_commit("HEAD").gpgsig is not None
+    ), "commandline option overrides configuration"
+
+    main(["HEAD", "--no-gpg-sign"])
+    assert repo.get_commit("HEAD").gpgsig is None, "long option"
+
+    main(["HEAD", "-S"])
+    assert repo.get_commit("HEAD").gpgsig is not None, "short option"
+
+    bash("git config gpg.program false")
+    try:
+        main(["HEAD", "--gpg-sign"])
+        assert False, "Overridden gpg.program should fail"
+    except CalledProcessError:
+        pass
+    bash("git config --unset gpg.program")
+
+    # Check that we can sign multiple commits.
+    bash(
+        """
+        git -c commit.gpgSign=false commit --allow-empty -m 'commit 2'
+        git -c commit.gpgSign=false commit --allow-empty -m 'commit 3'
+        git -c commit.gpgSign=false commit --allow-empty -m 'commit 4'
+    """
+    )
+    main(["HEAD~~", "--gpg-sign"])
+    assert repo.get_commit("HEAD~~").gpgsig is not None
+    assert repo.get_commit("HEAD~").gpgsig is not None
+    assert repo.get_commit("HEAD").gpgsig is not None
+
+    # Check that we can remove signatures from multiple commits.
+    main(["HEAD~", "--no-gpg-sign"])
+    assert repo.get_commit("HEAD~").gpgsig is None
+    assert repo.get_commit("HEAD").gpgsig is None
+
+    # Check that we add signatures, even if the target commit already has one.
+    assert repo.get_commit("HEAD~~").gpgsig is not None
+    main(["HEAD~~", "--gpg-sign"])
+    assert repo.get_commit("HEAD~").gpgsig is not None
+    assert repo.get_commit("HEAD").gpgsig is not None