Hi Junio, On 15.10.2020 10:02, Junio C Hamano wrote: > Samuel Čavoj <samuel@xxxxxxxxx> writes: > > > The merge subcommand launched for merges with non-default strategy would > > use its own default behaviour to decide how to sign commits, regardless > > of what opts->gpg_sign was set to. For example the --no-gpg-sign flag > > given to rebase explicitly would get ignored, if commit.gpgsign was set > > to true. > > > > Fix the issue and add a test case excercising this behaviour. > > > > Signed-off-by: Samuel Čavoj <samuel@xxxxxxxxx> > > --- > > v2 -> v3: > > - added test case > > --- > > sequencer.c | 2 ++ > > t/t3435-rebase-gpg-sign.sh | 7 +++++++ > > 2 files changed, 9 insertions(+) > > > > diff --git a/sequencer.c b/sequencer.c > > index 88ccff4838..043d606829 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -3678,6 +3678,8 @@ static int do_merge(struct repository *r, > > strvec_push(&cmd.args, git_path_merge_msg(r)); > > if (opts->gpg_sign) > > strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); > > + else > > + strvec_push(&cmd.args, "--no-gpg-sign"); > > Makes sense, I guess. As long as opts->gpg_sign reflects not just > the command line but also the configuration. Otherwise, an > invocation of "git rebase" with no gpg-sign related command line > options would say "ah, opts->gpg_sign is false, we must have been > told from the command line not to sign, so pass --no-gpg-sign here" > and that is not correct. > > > diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh > > index 9d2faffa03..773c2a1d72 100755 > > --- a/t/t3435-rebase-gpg-sign.sh > > +++ b/t/t3435-rebase-gpg-sign.sh > > @@ -81,4 +81,11 @@ test_expect_success 'rebase -r, GPG config and merge strategies' ' > > git verify-commit HEAD > > ' > > > > +test_expect_success 'rebase -r, --no-gpg-sign and merge strategies' ' > > + git reset --hard merged && > > + test_config commit.gpgsign true && > > + git rebase -fr --no-gpg-sign -s resolve --root && > > + test_must_fail git verify-commit HEAD > > +' > > I think that before this patch, we've tested the "no command line > option, but configuration tells us to sign" combination already to > make sure the result is signed, so this new test is sufficient. > > I briefly wondered if "test_must_fail git verify-commit" sufficient > to make sure that the rebased commits are not signed (i.e. verify > may fail for reasons other than the commit lacks signature, like the > commit is signed but with a wrong key, etc.), but I think it is OK > at least for now. Others might have clever ideas to cleanly and > cheaply reject other kinds of failures, in which case we may want to > adopt such a solution. > > Now that we know that the root cause of the bug you fixed was > because rebase rebase with the default merge strategy for two-head > merges use separate codepaths from and all other rebases, I wonder > if it is prudent to also test the same cases this series adds > without giving "-s resolve". That would exercise the other codepath I will leave that for someone else to tackle eventually. > that handles the default merge strategy for two-head merges. Yes, > we know that other codepath has been working even before this fix, > but tests are not about showing off what we fixed, but are about > making sure similar breakage won't be introduced by mistake in the > future. > > Thanks. As the number of very similar test is slowly growing, do you think it is worth copying (or making more generic) the test_rebase_gpg_sign for this situation as well? We currently have 4 almost identical tests (counting the new one you suggested for v4). Just a thought, as it is simpler to just add it at this point. Thanks for the feedback. Regards, Samuel
