Re* [PATCH] commit-tree: do not pay attention to commit.gpgsign

Jeff King [off-list ref] writes:

So I think the motivation and premise are good, but...

quoted

-static int commit_tree_config(const char *var, const char *value, void *cb)
-{
-	int status = git_gpg_config(var, value, NULL);
-	if (status)
-		return status;
-	if (!strcmp(var, "commit.gpgsign")) {
-		sign_commit = git_config_bool(var, value) ? "" : NULL;
-		return 0;
-	}
-	return git_default_config(var, value, cb);
-}
-

I think this may be going too far. If I do "git commit-tree -S", I'd
expect it to use gpg.program, but here you are dropping the call to
git_gpg_config. Likewise for user.signingkey.

Thanks (and thanks Eric for typospotting).

-- >8 --
ba3c69a9 (commit: teach --gpg-sign option, 2011-10-05) introduced a
"signed commit" by teaching the --[no]-gpg-sign option and the
commit.gpgsign configuration variable to various commands that
create commits.

Teaching these to "git commit" and "git merge", both of which are
end-user facing Porcelain commands, was perfectly fine.  Allowing
the plumbing "git commit-tree" to suddenly change the behaviour to
surprise the scripts by paying attention to commit.gpgsign was not.

Among the in-tree scripts, filter-branch, quiltimport, rebase and
stash are the commands that run "commit-tree".  If any of these
wants to allow users to always sign every single commit, they should
offer their own configuration (e.g. "filterBranch.gpgsign") with an
option to disable signing (e.g. "git filter-branch --no-gpgsign").

Ignoring commit.gpgsign option _obviously_ breaks the backward
compatibility, but it is easy to follow the standard pattern in
scripts to honor whatever configuration variable they choose to
follow.  E.g.

	case $(git config --bool commit.gpgsign) in
	true) sign=-S ;;
	*) sign= ;;
	esac &&
	git commit-tree $sign ...whatever other args...

Do so to make sure that "git rebase" keeps paying attention to the
configuration variable, which unfortunately is a documented mistake.

Helped-by: Jeff King [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
---
 Documentation/git-commit-tree.txt |  4 ++--
 builtin/commit-tree.c             |  4 ----
 git-rebase.sh                     |  5 ++++-
 t/t7510-signed-commit.sh          | 13 ++++++++++---
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
index f5f2a8d..eb273c3 100644
--- a/Documentation/git-commit-tree.txt
+++ b/Documentation/git-commit-tree.txt

@@ -59,8 +59,8 @@ OPTIONS
 	GPG-sign commit.
 
 --no-gpg-sign::
-	Countermand `commit.gpgSign` configuration variable that is
-	set to force each and every commit to be signed.
+	Do not GPG-sign commit, to countermand a `--gpg-sign` option
+	given earlier on the command line.
 
 
 Commit Information

diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 25aa2cd..15de7e8 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c

@@ -33,10 +33,6 @@ static int commit_tree_config(const char *var, const char *value, void *cb)
 	int status = git_gpg_config(var, value, NULL);
 	if (status)
 		return status;
-	if (!strcmp(var, "commit.gpgsign")) {
-		sign_commit = git_config_bool(var, value) ? "" : NULL;
-		return 0;
-	}
 	return git_default_config(var, value, cb);
 }
 

diff --git a/git-rebase.sh b/git-rebase.sh
index 90854e3..4d46662 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -87,7 +87,10 @@ preserve_merges=
 autosquash=
 keep_empty=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
-gpg_sign_opt=
+case "$(git config --bool commit.gpgsign)" in
+true)	gpg_sign_opt=-S ;;
+*)	gpg_sign_opt= ;;
+esac
 
 read_basic_state () {
 	test -f "$state_dir/head-name" &&

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 13331e5..7b365ee 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh

@@ -45,12 +45,18 @@ test_expect_success GPG 'create signed commits' '
 	git tag seventh-signed &&
 
 	echo 8 >file && test_tick && git commit -a -m eighth -SB7227189 &&
-	git tag eighth-signed-alt
+	git tag eighth-signed-alt &&
+
+	# commit.gpgsign is still on but this must not be signed
+	git tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) &&
+	# explicit -S of course must sign.
+	git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree})
 '
 
 test_expect_success GPG 'verify and show signatures' '
 	(
-		for commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed
+		for commit in initial second merge fourth-signed \
+			fifth-signed sixth-signed seventh-signed tenth-signed
 		do
 			git verify-commit $commit &&
 			git show --pretty=short --show-signature $commit >actual &&

@@ -60,7 +66,8 @@ test_expect_success GPG 'verify and show signatures' '
 		done
 	) &&
 	(
-		for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
+		for commit in merge^2 fourth-unsigned sixth-unsigned \
+			seventh-unsigned ninth-unsigned
 		do
 			test_must_fail git verify-commit $commit &&
 			git show --pretty=short --show-signature $commit >actual &&

-- 
2.8.2-486-gecbb083

On Tue, May 03, 2016 at 11:01:11AM -0700, Junio C Hamano wrote:

Ignoring commit.gpgsign option _obviously_ breaks the backward
compatibility, but it is easy to follow the standard pattern in
scripts to honor whatever configuration variable they choose to
follow.  E.g.

	case $(git config --bool commit.gpgsign) in
	true) sign=-S ;;
	*) sign= ;;
	esac &&
	git commit-tree $sign ...whatever other args...

Do so to make sure that "git rebase" keeps paying attention to the
configuration variable, which unfortunately is a documented mistake.

Helped-by: Jeff King [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
---
 Documentation/git-commit-tree.txt |  4 ++--
 builtin/commit-tree.c             |  4 ----
 git-rebase.sh                     |  5 ++++-
 t/t7510-signed-commit.sh          | 13 ++++++++++---
 4 files changed, 16 insertions(+), 10 deletions(-)

Thanks, this looks good to me[1]. Especially thinking about the rebase
case you handle here makes me more convinced than ever that an option
like "--respect-commit-gpgsign-config" is the wrong path. Because the
ultimate fate for rebase may be something like:

  case $(git config --bool rebase.gpgsign) in
  true) sign=-S ;;
  false) sign= ;;
  *)
	case $(git config --bool commit.gpgsign) in
	true) sign=-S ;;
	*) sign= ;;
	esac
	;;
  esac

You _can_ implement that by falling back to --respect... in the "*"
case, but at that point it is not saving much code, and merely making
things unnecessarily confusing.

-Peff

[1] I will say that I am happy with rebase respecting commit.gpgsign
    myself. The config I want is really "sign all commits I create", so
    I'd end up setting rebase.gpgsign, too, if it existed. But maybe
    other people have different workflows.