| Message ID | 20200331064456.GA15850@danh.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase.c: teach --no-gpg-sign to git-rebase | expand |
Danh Doan <congdanhqx@gmail.com> writes: > From: Đoàn Trần Công Danh <congdanhqx@gmail.com> > Subject: [PATCH] rebase.c: teach --no-gpg-sign to git-rebase > > Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> > --- > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index f7a6033607..54023cf3bb 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -358,6 +358,11 @@ See also INCOMPATIBLE OPTIONS below. > defaults to the committer identity; if specified, it must be > stuck to the option without a space. > > +--no-gpg-sign:: > + Countermand `commit.gpgSign` configuration variable that is > + set to force each and every commit to be signed. > + > + Two points. - There must be already an entry for '--gpg-sign'. It would make more sense to make this addtion a part of its description. - The --no-<option> form is not just to override a configured default, but also to coumtermand an option given earlier on the command line. In other words "rebase -S --no-gpg-sign" without any commit.gpgSign should work just fine. > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 27a07d4e78..a8cc5cfe0c 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1593,6 +1593,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > > options.allow_empty_message = 1; > git_config(rebase_config, &options); > + // options.gpg_sign_opt will be either "-S" or NULL > + // It'll be freed later, hence, no skip-prefix Don't use //- comments. > + gpg_sign = options.gpg_sign_opt ? "" : NULL; We've read configured commit.gpgSign in options.gpg_sign_opt; it is either a freeable "-S" or NULL depending on its value. We initialize the local gpg_sign variable to either an unfreeable "" or NULL here. Let's see how that local variable is later used here. We know it is given as the target variable to OPTION_STRING, which will overwrite with the value given from the command line, so "" that is unfreeable avoids an unnecessary leak. - If we did not have --gpg-sign, or --no-gpg-sign, then the local variable gpg_sign will stay to be either "" or NULL after parse_options() returns. - If we had --gpg-sign or --no-gpg-sign, we will have the value given from the last one of them on the command line in gpg_sign after parse_options() returns. > @@ -1823,10 +1826,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (options.empty != EMPTY_UNSPECIFIED) > imply_merge(&options, "--empty"); > > - if (gpg_sign) { > - free(options.gpg_sign_opt); > + free(options.gpg_sign_opt); > + if (gpg_sign) > options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign); > - } > + else > + options.gpg_sign_opt = NULL; Now we _always_ override options.gpg_sign_opt based on the value in the local gpg_sign variable, so the *ONLY* time options.gpg_sign_opt is used is immediately after git_config() returns to decide what value to assign to gpg_sign we saw above. I *think* it would be much clearer to FREE_AND_NULL options.gpg_sign_out immediately after we initialize gpg_sign above, instead of freeing it here. Then you do not need the elese clause here, either. This is a total tangent, but do we ever call cmd_rebase__interactive() these days? It does not seem to do the config thing, and assigns the string taken from the command line to opts.gpg_sign_opt, which means that it is an error to free the field in any codepath that can be reached from there. I suspect that after removing "rebase --preserve-merges", there is nobody that calls "git rebase--interactive", and at that point the function will be dead-code and can safely be removed. Thanks.
On 2020-04-01 10:47:15-0700, Junio C Hamano <gitster@pobox.com> wrote: > Two points. > > - There must be already an entry for '--gpg-sign'. It would make > more sense to make this addtion a part of its description. > > - The --no-<option> form is not just to override a configured > default, but also to coumtermand an option given earlier on the > command line. In other words "rebase -S --no-gpg-sign" without > any commit.gpgSign should work just fine. That paragraph was copy-pasted from git-commit documentation. I think it would need a clean up there, too. And, mention of --no-gpg-sign in am, cherry-pick, revert, merge-option. While writing this, I've checked (again) all commands mentioned --gpg-sign. To my surprise, "revert" (despite shares most of code with "cherry-pick") doesn't honour --no-gpg-sign, either. I'll teach "--no-gpg-sign" too revert and update all documentation for this. > > diff --git a/builtin/rebase.c b/builtin/rebase.c > > index 27a07d4e78..a8cc5cfe0c 100644 > > --- a/builtin/rebase.c > > +++ b/builtin/rebase.c > > @@ -1593,6 +1593,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > > > > options.allow_empty_message = 1; > > git_config(rebase_config, &options); > > + // options.gpg_sign_opt will be either "-S" or NULL > > + // It'll be freed later, hence, no skip-prefix > > Don't use //- comments. > > > + gpg_sign = options.gpg_sign_opt ? "" : NULL; > > We've read configured commit.gpgSign in options.gpg_sign_opt; it is > either a freeable "-S" or NULL depending on its value. We initialize > the local gpg_sign variable to either an unfreeable "" or NULL here. > > Let's see how that local variable is later used here. We know it is > given as the target variable to OPTION_STRING, which will overwrite > with the value given from the command line, so "" that is unfreeable > avoids an unnecessary leak. > > - If we did not have --gpg-sign, or --no-gpg-sign, then the local > variable gpg_sign will stay to be either "" or NULL after > parse_options() returns. > > - If we had --gpg-sign or --no-gpg-sign, we will have the value > given from the last one of them on the command line in gpg_sign > after parse_options() returns. > > > > > @@ -1823,10 +1826,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > > if (options.empty != EMPTY_UNSPECIFIED) > > imply_merge(&options, "--empty"); > > > > - if (gpg_sign) { > > - free(options.gpg_sign_opt); > > + free(options.gpg_sign_opt); > > + if (gpg_sign) > > options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign); > > - } > > + else > > + options.gpg_sign_opt = NULL; > > Now we _always_ override options.gpg_sign_opt based on the value in > the local gpg_sign variable, so the *ONLY* time options.gpg_sign_opt > is used is immediately after git_config() returns to decide what > value to assign to gpg_sign we saw above. I *think* it would be > much clearer to FREE_AND_NULL options.gpg_sign_out immediately after > we initialize gpg_sign above, instead of freeing it here. Make sense, > Then you do not need the elese clause here, either. > > This is a total tangent, but do we ever call cmd_rebase__interactive() > these days? It does not seem to do the config thing, and assigns the > string taken from the command line to opts.gpg_sign_opt, which means > that it is an error to free the field in any codepath that can be > reached from there. cmd_rebase__interactive go through different code path, and it doesn't run into above line > I suspect that after removing "rebase --preserve-merges", there is > nobody that calls "git rebase--interactive", and at that point the > function will be dead-code and can safely be removed. I've grep-ed the code and it's look like only "rebase -p" call cmd_rebase__interactive, I've drafted a test, and "rebase -p" indeeds doesn't honour "--no-gpg-sign", Consider the deprecation of "--preserve-merges" is more than a year, I think I'll mark that test as broken instead of trying to fix it.
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index f7a6033607..54023cf3bb 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -358,6 +358,11 @@ See also INCOMPATIBLE OPTIONS below. defaults to the committer identity; if specified, it must be stuck to the option without a space. +--no-gpg-sign:: + Countermand `commit.gpgSign` configuration variable that is + set to force each and every commit to be signed. + + -q:: --quiet:: Be quiet. Implies --no-stat. diff --git a/builtin/rebase.c b/builtin/rebase.c index 27a07d4e78..a8cc5cfe0c 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1593,6 +1593,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.allow_empty_message = 1; git_config(rebase_config, &options); + // options.gpg_sign_opt will be either "-S" or NULL + // It'll be freed later, hence, no skip-prefix + gpg_sign = options.gpg_sign_opt ? "" : NULL; if (options.use_legacy_rebase || !git_env_bool("GIT_TEST_REBASE_USE_BUILTIN", -1)) @@ -1823,10 +1826,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (options.empty != EMPTY_UNSPECIFIED) imply_merge(&options, "--empty"); - if (gpg_sign) { - free(options.gpg_sign_opt); + free(options.gpg_sign_opt); + if (gpg_sign) options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign); - } + else + options.gpg_sign_opt = NULL; if (exec.nr) { int i; diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh new file mode 100755 index 0000000000..d12b30b033 --- /dev/null +++ b/t/t3435-rebase-gpg-sign.sh @@ -0,0 +1,72 @@ +#!/bin/sh +# +# Copyright (c) 2020 Doan Tran Cong Danh +# + +test_description='test rebase --[no-]gpg-sign' + +. ./test-lib.sh +. "$TEST_DIRECTORY/lib-gpg.sh" + +if ! test_have_prereq GPG +then + skip_all='skip all test rebase --[no-]gpg-sign, gpg not available' + test_done +fi + +test_expect_success 'setup: not-signed commit' ' + test_commit one && + test_commit two && + test_must_fail git verify-commit HEAD && + test_must_fail git verify-commit HEAD^ && + git tag unsigned +' + +test_expect_success 'setup: rebase --gpg-sign to sign all commit' ' + git rebase --gpg-sign --force-rebase --root && + git verify-commit HEAD && + git verify-commit HEAD^ && + git tag signed +' + +test_expect_success 'rebase without commit.gpgsign config' ' + git reset --hard signed && + test_might_fail git config --unset commit.gpgsign && + git rebase --force-rebase --root && + test_must_fail git verify-commit HEAD && + test_must_fail git verify-commit HEAD^ +' + +test_expect_success 'rebase respects commit.gpgsign=true config' ' + git reset --hard unsigned && + git config commit.gpgsign true && + git rebase --force-rebase --root && + git verify-commit HEAD && + git verify-commit HEAD^ +' + +test_expect_success 'rebase --no-gpg-sign overrides commit.gpgsign' ' + git reset --hard unsigned && + git config commit.gpgsign true && + git rebase --no-gpg-sign --force-rebase --root && + test_must_fail git verify-commit HEAD && + test_must_fail git verify-commit HEAD^ +' + +test_expect_success 'rebase --no-gpg-sign clear signed commit' ' + git reset --hard signed && + git config commit.gpgsign true && + git rebase --no-gpg-sign --force-rebase --root && + test_must_fail git verify-commit HEAD && + test_must_fail git verify-commit HEAD^ +' + +test_expect_success 'rebase -i --no-gpg-sign override commit.gpgsign' ' + git reset --hard signed && + git config commit.gpgsign true && + GIT_EDITOR=true git rebase -i --no-gpg-sign --force-rebase --root && + test_must_fail git verify-commit HEAD && + test_must_fail git verify-commit HEAD^ +' + +test_done