{"thread":{"id":"54412","subject":"[PATCH v3 3/3] sequencer: pass explicit --no-gpg-sign to merge","startedAt":"2020-10-14T09:21:58Z","lastAt":"2020-10-17T23:16:26Z","messageCount":9,"participants":["Samuel Čavoj","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"407494","messageId":"20201013213021.3671432-3-samuel@cavoj.net","threadId":"54412","inReplyTo":"20201013213021.3671432-1-samuel@cavoj.net","subject":"[PATCH v3 3/3] sequencer: pass explicit --no-gpg-sign to merge","fromName":"Samuel Čavoj","fromEmail":"samuel@cavoj.net","sentAt":"2020-10-13T21:30:24Z","receivedAt":"2020-10-14T09:21:58Z","isPatch":true,"sender":{"key":"samuel@cavoj.net","avatar":"https://avatars.githubusercontent.com/u/703905?v=4"},"body":"The merge subcommand launched for merges with non-default strategy would\nuse its own default behaviour to decide how to sign commits, regardless\nof what opts->gpg_sign was set to. For example the --no-gpg-sign flag\ngiven to rebase explicitly would get ignored, if commit.gpgsign was set\nto true.\n\nFix the issue and add a test case excercising this behaviour.\n\nSigned-off-by: Samuel Čavoj <samuel@cavoj.net>\n---\nv2 -> v3:\n    - added test case\n---\n sequencer.c                | 2 ++\n t/t3435-rebase-gpg-sign.sh | 7 +++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 88ccff4838..043d606829 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3678,6 +3678,8 @@ static int do_merge(struct repository *r,\n \t\tstrvec_push(&cmd.args, git_path_merge_msg(r));\n \t\tif (opts->gpg_sign)\n \t\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n+\t\telse\n+\t\t\tstrvec_push(&cmd.args, \"--no-gpg-sign\");\n \n \t\t/* Add the tips to be merged */\n \t\tfor (j = to_merge; j; j = j->next)\ndiff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\nindex 9d2faffa03..773c2a1d72 100755\n--- a/t/t3435-rebase-gpg-sign.sh\n+++ b/t/t3435-rebase-gpg-sign.sh\n@@ -81,4 +81,11 @@ test_expect_success 'rebase -r, GPG config and merge strategies' '\n \tgit verify-commit HEAD\n '\n \n+test_expect_success 'rebase -r, --no-gpg-sign and merge strategies' '\n+\tgit reset --hard merged &&\n+\ttest_config commit.gpgsign true &&\n+\tgit rebase -fr --no-gpg-sign -s resolve --root &&\n+\ttest_must_fail git verify-commit HEAD\n+'\n+\n test_done\n-- \n2.28.0\n\n"},{"id":"407495","messageId":"20201013213021.3671432-2-samuel@cavoj.net","threadId":"54412","inReplyTo":"20201013213021.3671432-1-samuel@cavoj.net","subject":"[PATCH v3 2/3] sequencer: fix gpg option passed to merge subcommand","fromName":"Samuel Čavoj","fromEmail":"samuel@cavoj.net","sentAt":"2020-10-13T21:30:23Z","receivedAt":"2020-10-14T09:22:10Z","isPatch":true,"sender":{"key":"samuel@cavoj.net","avatar":"https://avatars.githubusercontent.com/u/703905?v=4"},"body":"When performing a rebase with --rebase-merges using either a custom\nstrategy specified with -s or an octopus merge, and at the same time\nhaving gpgsign enabled (either rebase -S or config commit.gpgsign), the\noperation would fail on making the merge commit. Instead of \"-S%s\" with\nthe key id substituted, only the bare key id would get passed to the\nunderlying merge command, which tried to interpret it as a ref.\n\nFix the issue and add test cases as suggested by Johannes Schindelin.\n\nSigned-off-by: Samuel Čavoj <samuel@cavoj.net>\n---\nchanged v2 -> v3:\n    - first test is now okay, thanks to patch 1/3\n    - added another test for config commit.gpgsign\n---\n sequencer.c                |  2 +-\n t/t3435-rebase-gpg-sign.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 00acb12496..88ccff4838 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,\n \t\tstrvec_push(&cmd.args, \"-F\");\n \t\tstrvec_push(&cmd.args, git_path_merge_msg(r));\n \t\tif (opts->gpg_sign)\n-\t\t\tstrvec_push(&cmd.args, opts->gpg_sign);\n+\t\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n \n \t\t/* Add the tips to be merged */\n \t\tfor (j = to_merge; j; j = j->next)\ndiff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\nindex 696cb6b6a4..9d2faffa03 100755\n--- a/t/t3435-rebase-gpg-sign.sh\n+++ b/t/t3435-rebase-gpg-sign.sh\n@@ -68,4 +68,17 @@ test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' '\n \ttest_must_fail git verify-commit HEAD\n '\n \n+test_expect_success 'rebase -r, GPG --gpg-sign and merge strategies' '\n+\tgit reset --hard merged &&\n+\tgit rebase -fr --gpg-sign -s resolve --root &&\n+\tgit verify-commit HEAD\n+'\n+\n+test_expect_success 'rebase -r, GPG config and merge strategies' '\n+\tgit reset --hard merged &&\n+\ttest_config commit.gpgsign true &&\n+\tgit rebase -fr -s resolve --root &&\n+\tgit verify-commit HEAD\n+'\n+\n test_done\n-- \n2.28.0\n\n"},{"id":"407497","messageId":"20201013213021.3671432-1-samuel@cavoj.net","threadId":"54412","inReplyTo":null,"subject":"[PATCH v3 1/3] t3435: use `test_config` instead of `git config`","fromName":"Samuel Čavoj","fromEmail":"samuel@cavoj.net","sentAt":"2020-10-13T21:30:22Z","receivedAt":"2020-10-14T09:24:08Z","isPatch":true,"sender":{"key":"samuel@cavoj.net","avatar":"https://avatars.githubusercontent.com/u/703905?v=4"},"body":"Replace usages of `git config` with `test_config` in t3435 to prevent\nside-effects between tests.\n\nSigned-off-by: Samuel Čavoj <samuel@cavoj.net>\n---\nchanged v2 -> v3:\n    - added this patch\n---\n t/t3435-rebase-gpg-sign.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\nindex b47c59c190..696cb6b6a4 100755\n--- a/t/t3435-rebase-gpg-sign.sh\n+++ b/t/t3435-rebase-gpg-sign.sh\n@@ -27,7 +27,7 @@ test_rebase_gpg_sign () {\n \tshift\n \ttest_expect_success \"rebase $* with commit.gpgsign=$conf $will sign commit\" \"\n \t\tgit reset two &&\n-\t\tgit config commit.gpgsign $conf &&\n+\t\ttest_config commit.gpgsign $conf &&\n \t\tset_fake_editor &&\n \t\tFAKE_LINES='r 1 p 2' git rebase --force-rebase --root $* &&\n \t\t$must_fail git verify-commit HEAD^ &&\n@@ -63,7 +63,7 @@ test_rebase_gpg_sign   false -i --no-gpg-sign --gpg-sign\n \n test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' '\n \tgit reset --hard merged &&\n-\tgit config commit.gpgsign true &&\n+\ttest_config commit.gpgsign true &&\n \tgit rebase -p --no-gpg-sign --onto=one fork-point master &&\n \ttest_must_fail git verify-commit HEAD\n '\n-- \n2.28.0\n\n"},{"id":"407627","messageId":"xmqqlfg7h23g.fsf@gitster.c.googlers.com","threadId":"54412","inReplyTo":"20201013213021.3671432-1-samuel@cavoj.net","subject":"Re: [PATCH v3 1/3] t3435: use `test_config` instead of `git config`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-15T16:43:15Z","receivedAt":"2020-10-15T16:43:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Čavoj <samuel@cavoj.net> writes:\n\n> Replace usages of `git config` with `test_config` in t3435 to prevent\n> side-effects between tests.\n>\n> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> ---\n> changed v2 -> v3:\n>     - added this patch\n> ---\n>  t/t3435-rebase-gpg-sign.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n\nAs I already said, I think this is not needed.\n\nThe way the existing tests protect themselves from what previously\nhappened is to explicitly set up _their_ expectation in them before\nrunning the part that is affected by the configuration setting,\nwhich is an approach that is easy to read and understand because it\nis explicit in what these tests care about.\n\n> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\n> index b47c59c190..696cb6b6a4 100755\n> --- a/t/t3435-rebase-gpg-sign.sh\n> +++ b/t/t3435-rebase-gpg-sign.sh\n> @@ -27,7 +27,7 @@ test_rebase_gpg_sign () {\n>  \tshift\n>  \ttest_expect_success \"rebase $* with commit.gpgsign=$conf $will sign commit\" \"\n>  \t\tgit reset two &&\n> -\t\tgit config commit.gpgsign $conf &&\n> +\t\ttest_config commit.gpgsign $conf &&\n>  \t\tset_fake_editor &&\n>  \t\tFAKE_LINES='r 1 p 2' git rebase --force-rebase --root $* &&\n>  \t\t$must_fail git verify-commit HEAD^ &&\n\nThese are the first three uses of this test helper.\n\n    test_rebase_gpg_sign ! false\n    test_rebase_gpg_sign   true\n    test_rebase_gpg_sign ! true  --no-gpg-sign\n\nExamine what this patch does to the second test that is run by the\nsecond invocation of test_rebase_gpg_sign, for example.  Before this\npatch, \"git config\" that was done by the first test is left and\ncommit.gpgsign was set to 'false' when the second test started.\nWith this patch, it is removed by the clean-up action set by\ntest_config, so the second test starts without the configuration.\n\nBut this patch does *not* affect correctness of the second invocation.\nWhy?\n\nBecause the way these tests protect themselves is NOT based on the\n\"I muck with configuration, so I'll clean them up for the next\nperson\" attitude which is made easy to do with test_config ANYWAY.\nThe correctness of the second test still relies on the fact that it\nitself tweaks the variable it cares about to the state it wants it\nto be.\n\nSo in this particular test script, where all test pieces are about\nchecking their behaviour on different setting of commit.gpgsign, use\nof test_config does not buy us anything other than extra clean-up\nafter each test that is unnecessary.\n\nWhere test_config shines is when only a few (or a single) test cares\nabout different setting of a variable, and all later tests want to\ntest the behaviour under the default setting.  Among 47 tests, the\nsecond and 42nd tests may want to test with commit.gpgsign set to\ntrue, while the remainder may want to test without the variable at\nall.  In such a case, it is easier to declare that the normal state\nin that test script is not to have the configuration, and use\ntest_config in these selected tests to tentatively set it and remove\nit after the tests are done.\n\nBut that rationale does not apply to this script.  It seems that\neach test piece wants to be in control of and be tested under\nspecific setting of the variable.\n\n> @@ -63,7 +63,7 @@ test_rebase_gpg_sign   false -i --no-gpg-sign --gpg-sign\n>  \n>  test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' '\n>  \tgit reset --hard merged &&\n> -\tgit config commit.gpgsign true &&\n> +\ttest_config commit.gpgsign true &&\n>  \tgit rebase -p --no-gpg-sign --onto=one fork-point master &&\n>  \ttest_must_fail git verify-commit HEAD\n>  '\n"},{"id":"407628","messageId":"xmqqh7qvh1wq.fsf@gitster.c.googlers.com","threadId":"54412","inReplyTo":"20201013213021.3671432-2-samuel@cavoj.net","subject":"Re: [PATCH v3 2/3] sequencer: fix gpg option passed to merge subcommand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-15T16:47:17Z","receivedAt":"2020-10-15T16:47:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Čavoj <samuel@cavoj.net> writes:\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 00acb12496..88ccff4838 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,\n>  \t\tstrvec_push(&cmd.args, \"-F\");\n>  \t\tstrvec_push(&cmd.args, git_path_merge_msg(r));\n>  \t\tif (opts->gpg_sign)\n> -\t\t\tstrvec_push(&cmd.args, opts->gpg_sign);\n> +\t\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n\nGood.  And later you'd add \"else\" to explicitly turn it off, I presume?\n\n> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\n> index 696cb6b6a4..9d2faffa03 100755\n> --- a/t/t3435-rebase-gpg-sign.sh\n> +++ b/t/t3435-rebase-gpg-sign.sh\n> @@ -68,4 +68,17 @@ test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' '\n>  \ttest_must_fail git verify-commit HEAD\n>  '\n>  \n> +test_expect_success 'rebase -r, GPG --gpg-sign and merge strategies' '\n> +\tgit reset --hard merged &&\n\nInsert\n\n\ttest_unconfig commit.gpgsign &&\n\nhere.  That would make it explicit that we are checking the base\ncase where only the command line option is given.\n\n> +\tgit rebase -fr --gpg-sign -s resolve --root &&\n> +\tgit verify-commit HEAD\n> +'\n> +\n> +test_expect_success 'rebase -r, GPG config and merge strategies' '\n> +\tgit reset --hard merged &&\n> +\ttest_config commit.gpgsign true &&\n\nUse \"git config\" for consistency.\n\n> +\tgit rebase -fr -s resolve --root &&\n> +\tgit verify-commit HEAD\n> +'\n> +\n\nWe'd want another one, which is to give --gpg-sign from the command\nline when this is in effect:\n\n\tgit config commit.gpgsign false\n\ni.e. not \"test_unconfig\" that removes the configuration, but\nexplicitly configured not to sign\n\nThanks.\n"},{"id":"407629","messageId":"xmqqd01jh17n.fsf@gitster.c.googlers.com","threadId":"54412","inReplyTo":"20201013213021.3671432-3-samuel@cavoj.net","subject":"Re: [PATCH v3 3/3] sequencer: pass explicit --no-gpg-sign to merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-15T17:02:20Z","receivedAt":"2020-10-15T17:02:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Čavoj <samuel@cavoj.net> writes:\n\n> The merge subcommand launched for merges with non-default strategy would\n> use its own default behaviour to decide how to sign commits, regardless\n> of what opts->gpg_sign was set to. For example the --no-gpg-sign flag\n> given to rebase explicitly would get ignored, if commit.gpgsign was set\n> to true.\n>\n> Fix the issue and add a test case excercising this behaviour.\n>\n> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> ---\n> v2 -> v3:\n>     - added test case\n> ---\n>  sequencer.c                | 2 ++\n>  t/t3435-rebase-gpg-sign.sh | 7 +++++++\n>  2 files changed, 9 insertions(+)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 88ccff4838..043d606829 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3678,6 +3678,8 @@ static int do_merge(struct repository *r,\n>  \t\tstrvec_push(&cmd.args, git_path_merge_msg(r));\n>  \t\tif (opts->gpg_sign)\n>  \t\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n> +\t\telse\n> +\t\t\tstrvec_push(&cmd.args, \"--no-gpg-sign\");\n\nMakes sense, I guess.  As long as opts->gpg_sign reflects not just\nthe command line but also the configuration.  Otherwise, an\ninvocation of \"git rebase\" with no gpg-sign related command line\noptions would say \"ah, opts->gpg_sign is false, we must have been\ntold from the command line not to sign, so pass --no-gpg-sign here\"\nand that is not correct.\n\n> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\n> index 9d2faffa03..773c2a1d72 100755\n> --- a/t/t3435-rebase-gpg-sign.sh\n> +++ b/t/t3435-rebase-gpg-sign.sh\n> @@ -81,4 +81,11 @@ test_expect_success 'rebase -r, GPG config and merge strategies' '\n>  \tgit verify-commit HEAD\n>  '\n>  \n> +test_expect_success 'rebase -r, --no-gpg-sign and merge strategies' '\n> +\tgit reset --hard merged &&\n> +\ttest_config commit.gpgsign true &&\n> +\tgit rebase -fr --no-gpg-sign -s resolve --root &&\n> +\ttest_must_fail git verify-commit HEAD\n> +'\n\nI think that before this patch, we've tested the \"no command line\noption, but configuration tells us to sign\" combination already to\nmake sure the result is signed, so this new test is sufficient.\n\nI briefly wondered if \"test_must_fail git verify-commit\" sufficient\nto make sure that the rebased commits are not signed (i.e. verify\nmay fail for reasons other than the commit lacks signature, like the\ncommit is signed but with a wrong key, etc.), but I think it is OK\nat least for now.  Others might have clever ideas to cleanly and\ncheaply reject other kinds of failures, in which case we may want to\nadopt such a solution.\n\nNow that we know that the root cause of the bug you fixed was\nbecause rebase rebase with the default merge strategy for two-head\nmerges use separate codepaths from and all other rebases, I wonder\nif it is prudent to also test the same cases this series adds\nwithout giving \"-s resolve\".  That would exercise the other codepath\nthat handles the default merge strategy for two-head merges.  Yes,\nwe know that other codepath has been working even before this fix,\nbut tests are not about showing off what we fixed, but are about\nmaking sure similar breakage won't be introduced by mistake in the\nfuture.\n\nThanks.\n"},{"id":"407814","messageId":"20201017220224.o6fs5ukwqq7sqgrb@fastboi.localdomain","threadId":"54412","inReplyTo":"xmqqd01jh17n.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 3/3] sequencer: pass explicit --no-gpg-sign to merge","fromName":"Samuel Čavoj","fromEmail":"samuel@cavoj.net","sentAt":"2020-10-17T22:02:24Z","receivedAt":"2020-10-17T22:04:22Z","isPatch":true,"sender":{"key":"samuel@cavoj.net","avatar":"https://avatars.githubusercontent.com/u/703905?v=4"},"body":"Hi Junio,\n\nOn 15.10.2020 10:02, Junio C Hamano wrote:\n> Samuel Čavoj <samuel@cavoj.net> writes:\n> \n> > The merge subcommand launched for merges with non-default strategy would\n> > use its own default behaviour to decide how to sign commits, regardless\n> > of what opts->gpg_sign was set to. For example the --no-gpg-sign flag\n> > given to rebase explicitly would get ignored, if commit.gpgsign was set\n> > to true.\n> >\n> > Fix the issue and add a test case excercising this behaviour.\n> >\n> > Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> > ---\n> > v2 -> v3:\n> >     - added test case\n> > ---\n> >  sequencer.c                | 2 ++\n> >  t/t3435-rebase-gpg-sign.sh | 7 +++++++\n> >  2 files changed, 9 insertions(+)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index 88ccff4838..043d606829 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -3678,6 +3678,8 @@ static int do_merge(struct repository *r,\n> >  \t\tstrvec_push(&cmd.args, git_path_merge_msg(r));\n> >  \t\tif (opts->gpg_sign)\n> >  \t\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n> > +\t\telse\n> > +\t\t\tstrvec_push(&cmd.args, \"--no-gpg-sign\");\n> \n> Makes sense, I guess.  As long as opts->gpg_sign reflects not just\n> the command line but also the configuration.  Otherwise, an\n> invocation of \"git rebase\" with no gpg-sign related command line\n> options would say \"ah, opts->gpg_sign is false, we must have been\n> told from the command line not to sign, so pass --no-gpg-sign here\"\n> and that is not correct.\n> \n> > diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\n> > index 9d2faffa03..773c2a1d72 100755\n> > --- a/t/t3435-rebase-gpg-sign.sh\n> > +++ b/t/t3435-rebase-gpg-sign.sh\n> > @@ -81,4 +81,11 @@ test_expect_success 'rebase -r, GPG config and merge strategies' '\n> >  \tgit verify-commit HEAD\n> >  '\n> >  \n> > +test_expect_success 'rebase -r, --no-gpg-sign and merge strategies' '\n> > +\tgit reset --hard merged &&\n> > +\ttest_config commit.gpgsign true &&\n> > +\tgit rebase -fr --no-gpg-sign -s resolve --root &&\n> > +\ttest_must_fail git verify-commit HEAD\n> > +'\n> \n> I think that before this patch, we've tested the \"no command line\n> option, but configuration tells us to sign\" combination already to\n> make sure the result is signed, so this new test is sufficient.\n> \n> I briefly wondered if \"test_must_fail git verify-commit\" sufficient\n> to make sure that the rebased commits are not signed (i.e. verify\n> may fail for reasons other than the commit lacks signature, like the\n> commit is signed but with a wrong key, etc.), but I think it is OK\n> at least for now.  Others might have clever ideas to cleanly and\n> cheaply reject other kinds of failures, in which case we may want to\n> adopt such a solution.\n> \n> Now that we know that the root cause of the bug you fixed was\n> because rebase rebase with the default merge strategy for two-head\n> merges use separate codepaths from and all other rebases, I wonder\n> if it is prudent to also test the same cases this series adds\n> without giving \"-s resolve\".  That would exercise the other codepath\n\nI will leave that for someone else to tackle eventually.\n\n> that handles the default merge strategy for two-head merges.  Yes,\n> we know that other codepath has been working even before this fix,\n> but tests are not about showing off what we fixed, but are about\n> making sure similar breakage won't be introduced by mistake in the\n> future.\n> \n> Thanks.\n\nAs the number of very similar test is slowly growing, do you think it is\nworth copying (or making more generic) the test_rebase_gpg_sign for this\nsituation as well? We currently have 4 almost identical tests (counting\nthe new one you suggested for v4). Just a thought, as it is simpler to\njust add it at this point. Thanks for the feedback.\n\nRegards,\nSamuel\n"},{"id":"407817","messageId":"xmqq5z788osw.fsf@gitster.c.googlers.com","threadId":"54412","inReplyTo":"20201017220224.o6fs5ukwqq7sqgrb@fastboi.localdomain","subject":"Re: [PATCH v3 3/3] sequencer: pass explicit --no-gpg-sign to merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-17T22:34:23Z","receivedAt":"2020-10-17T22:34:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Čavoj <samuel@cavoj.net> writes:\n\n>> Now that we know that the root cause of the bug you fixed was\n>> because rebase rebase with the default merge strategy for two-head\n>> merges use separate codepaths from and all other rebases, I wonder\n>> if it is prudent to also test the same cases this series adds\n>> without giving \"-s resolve\".  That would exercise the other codepath\n>\n> I will leave that for someone else to tackle eventually.\n\nWe know that other codepath has been working even before this fix,\nbut tests are not about showing off what we fixed, but are about\nmaking sure similar breakage won't be introduced by mistake in the\nfuture.  Leaving it \"for someone\", when we know what the problem is\nand how to solve it, is asking for the \"evantually\" not materialize\nforever.\n\n> As the number of very similar test is slowly growing, do you think it is\n> worth copying (or making more generic) the test_rebase_gpg_sign for this\n> situation as well? We currently have 4 almost identical tests (counting\n> the new one you suggested for v4). Just a thought, as it is simpler to\n> just add it at this point. Thanks for the feedback.\n\nThat is a tough question.  Often, a generic test helper makes it too\neasy to do a full matrix of tests and encourages us to overdo it,\nwhich we probably would want to avoid.  I think what I've suggested\nso far is a bare minimum combination for code coverage.\n\n"},{"id":"407822","messageId":"20201017231622.afi73e62dp4yjvk4@fastboi.localdomain","threadId":"54412","inReplyTo":"xmqq5z788osw.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 3/3] sequencer: pass explicit --no-gpg-sign to merge","fromName":"Samuel Čavoj","fromEmail":"samuel@cavoj.net","sentAt":"2020-10-17T23:16:22Z","receivedAt":"2020-10-17T23:16:26Z","isPatch":true,"sender":{"key":"samuel@cavoj.net","avatar":"https://avatars.githubusercontent.com/u/703905?v=4"},"body":"Hi,\n\nOn 17.10.2020 15:34, Junio C Hamano wrote:\n> Samuel Čavoj <samuel@cavoj.net> writes:\n> \n> >> Now that we know that the root cause of the bug you fixed was\n> >> because rebase rebase with the default merge strategy for two-head\n> >> merges use separate codepaths from and all other rebases, I wonder\n> >> if it is prudent to also test the same cases this series adds\n> >> without giving \"-s resolve\".  That would exercise the other codepath\n> >\n> > I will leave that for someone else to tackle eventually.\n> \n> We know that other codepath has been working even before this fix,\n> but tests are not about showing off what we fixed, but are about\n> making sure similar breakage won't be introduced by mistake in the\n> future.  Leaving it \"for someone\", when we know what the problem is\n> and how to solve it, is asking for the \"evantually\" not materialize\n> forever.\n\nI agree with that, don't take me wrong, but in general, people have\nother things to do, than implement test cases only marginally related to\nthe inital patch they submitted.\n\nAnyway, as it didn't take long in this case, I added them as patch 3/3\nin v4.\n\n> \n> > As the number of very similar test is slowly growing, do you think it is\n> > worth copying (or making more generic) the test_rebase_gpg_sign for this\n> > situation as well? We currently have 4 almost identical tests (counting\n> > the new one you suggested for v4). Just a thought, as it is simpler to\n> > just add it at this point. Thanks for the feedback.\n> \n> That is a tough question.  Often, a generic test helper makes it too\n> easy to do a full matrix of tests and encourages us to overdo it,\n> which we probably would want to avoid.  I think what I've suggested\n> so far is a bare minimum combination for code coverage.\n> \nAlright, I will leave them as is.\n\nRegards,\nSamuel\n"}]}