| Message ID | 20201013213021.3671432-3-samuel@cavoj.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] t3435: use `test_config` instead of `git config` | expand |
Samuel Čavoj <samuel@cavoj.net> 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@cavoj.net> > --- > 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 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.
Hi Junio, On 15.10.2020 10:02, Junio C Hamano wrote: > Samuel Čavoj <samuel@cavoj.net> 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@cavoj.net> > > --- > > 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
Samuel Čavoj <samuel@cavoj.net> writes: >> 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. 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. Leaving it "for someone", when we know what the problem is and how to solve it, is asking for the "evantually" not materialize forever. > 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. That is a tough question. Often, a generic test helper makes it too easy to do a full matrix of tests and encourages us to overdo it, which we probably would want to avoid. I think what I've suggested so far is a bare minimum combination for code coverage.
Hi, On 17.10.2020 15:34, Junio C Hamano wrote: > Samuel Čavoj <samuel@cavoj.net> writes: > > >> 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. > > 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. Leaving it "for someone", when we know what the problem is > and how to solve it, is asking for the "evantually" not materialize > forever. I agree with that, don't take me wrong, but in general, people have other things to do, than implement test cases only marginally related to the inital patch they submitted. Anyway, as it didn't take long in this case, I added them as patch 3/3 in v4. > > > 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. > > That is a tough question. Often, a generic test helper makes it too > easy to do a full matrix of tests and encourages us to overdo it, > which we probably would want to avoid. I think what I've suggested > so far is a bare minimum combination for code coverage. > Alright, I will leave them as is. Regards, Samuel
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"); /* Add the tips to be merged */ for (j = to_merge; j; j = j->next) 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 +' + test_done
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@cavoj.net> --- v2 -> v3: - added test case --- sequencer.c | 2 ++ t/t3435-rebase-gpg-sign.sh | 7 +++++++ 2 files changed, 9 insertions(+)