* [PATCH] commit-tree: do not pay attention to commit.gpgsign
@ 2016-05-02 21:58 Junio C Hamano
2016-05-03 4:12 ` Jeff King
0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2016-05-02 21:58 UTC (permalink / raw)
To: git
ba3c69a9 (commit: teach --gpg-sign option, 2011-10-05) introduced a
"signed commit" by teaching --[no-gpg-sign option and 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 (e.g. "git filter-branch --no-gpgsign").
Ignoring commit.gpgsign option _obviously_ breaks the backward
compatibility, but I seriously doubt anybody sane is depending on
this misfeature that commit-tree blindly follows commit.gpgsign in
any third-party script that calls it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This is an simpler alternative that forces commit-tree callers
that want to honor commit.gpgsign to do so themselves.
builtin/commit-tree.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 3feeffe..e4ba0d8 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -8,7 +8,6 @@
#include "tree.h"
#include "builtin.h"
#include "utf8.h"
-#include "gpg-interface.h"
static const char commit_tree_usage[] = "git commit-tree [(-p <sha1>)...] [-S[<keyid>]] [-m <message>] [-F <file>] <sha1>";
@@ -28,18 +27,6 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p)
commit_list_insert(parent, parents_p);
}
-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);
-}
-
int cmd_commit_tree(int argc, const char **argv, const char *prefix)
{
int i, got_tree = 0;
@@ -48,7 +35,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
unsigned char commit_sha1[20];
struct strbuf buffer = STRBUF_INIT;
- git_config(commit_tree_config, NULL);
+ git_config(git_default_config, NULL);
if (argc < 2 || !strcmp(argv[1], "-h"))
usage(commit_tree_usage);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] commit-tree: do not pay attention to commit.gpgsign
2016-05-02 21:58 [PATCH] commit-tree: do not pay attention to commit.gpgsign Junio C Hamano
@ 2016-05-03 4:12 ` Jeff King
2016-05-03 18:01 ` Re* " Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2016-05-03 4:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Mon, May 02, 2016 at 02:58:45PM -0700, Junio C Hamano wrote:
> ba3c69a9 (commit: teach --gpg-sign option, 2011-10-05) introduced a
> "signed commit" by teaching --[no-gpg-sign option and 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 (e.g. "git filter-branch --no-gpgsign").
>
> Ignoring commit.gpgsign option _obviously_ breaks the backward
> compatibility, but I seriously doubt anybody sane is depending on
> this misfeature that commit-tree blindly follows commit.gpgsign in
> any third-party script that calls it.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * This is an simpler alternative that forces commit-tree callers
> that want to honor commit.gpgsign to do so themselves.
I don't have any such scripts myself (aside from git-stash, whose
signing behavior is moderately annoying), but I think this simpler form
is fine. There is already an escape hatch for scripts, and it is:
if test "$(git config --bool commit.gpgsign)" = "true"; then
sign=-S
else
sign=
fi
git commit-tree $sign ...
That is a few more lines than "--use-commit-gpgsign-config", but it's
simple enough to be acceptable, and matches the same technique that
other config options need when used with plumbing.
So I think the motivation and premise are good, but...
> -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.
So I think you just want to drop the commit.gpgsign block here, and keep
the rest.
-Peff
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re* [PATCH] commit-tree: do not pay attention to commit.gpgsign
2016-05-03 4:12 ` Jeff King
@ 2016-05-03 18:01 ` Junio C Hamano
2016-05-03 18:58 ` Jeff King
0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2016-05-03 18:01 UTC (permalink / raw)
To: Jeff King, Eric Sunshine; +Cc: git
Jeff King <peff@peff.net> writes:
> So I think the motivation and premise are good, but...
>
>> -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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Re* [PATCH] commit-tree: do not pay attention to commit.gpgsign
2016-05-03 18:01 ` Re* " Junio C Hamano
@ 2016-05-03 18:58 ` Jeff King
0 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2016-05-03 18:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, git
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 <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-05-03 18:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-02 21:58 [PATCH] commit-tree: do not pay attention to commit.gpgsign Junio C Hamano
2016-05-03 4:12 ` Jeff King
2016-05-03 18:01 ` Re* " Junio C Hamano
2016-05-03 18:58 ` Jeff King
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).