{"thread":{"id":"54404","subject":"[PATCH v2 2/2] sequencer: pass explicit --no-gpg-sign to merge","startedAt":"2020-10-12T23:49:10Z","lastAt":"2020-10-16T20:15:35Z","messageCount":18,"participants":["Samuel Čavoj","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"407406","messageId":"20201012234901.1356948-2-samuel@cavoj.net","threadId":"54404","inReplyTo":"20201012234901.1356948-1-samuel@cavoj.net","subject":"[PATCH v2 2/2] sequencer: pass explicit --no-gpg-sign to merge","fromName":"Samuel Čavoj","fromEmail":"samuel@cavoj.net","sentAt":"2020-10-12T23:49:01Z","receivedAt":"2020-10-12T23:49:10Z","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\nSigned-off-by: Samuel Čavoj <samuel@cavoj.net>\n---\n sequencer.c | 2 ++\n 1 file changed, 2 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)\n-- \n2.28.0\n\n"},{"id":"407407","messageId":"20201012234901.1356948-1-samuel@cavoj.net","threadId":"54404","inReplyTo":null,"subject":"[PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Samuel Čavoj","fromEmail":"samuel@cavoj.net","sentAt":"2020-10-12T23:49:00Z","receivedAt":"2020-10-12T23:49:10Z","isPatch":true,"sender":{"key":"samuel@cavoj.net","avatar":"https://avatars.githubusercontent.com/u/703905?v=4"},"body":"From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n\nWhen 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 a test case as suggested by Johannes Schindelin.\n\nSigned-off-by: Samuel Čavoj <samuel@cavoj.net>\n---\nchanged v1 -> v2:\n    added test case\n---\n sequencer.c                | 2 +-\n t/t3435-rebase-gpg-sign.sh | 6 ++++++\n 2 files changed, 7 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 b47c59c190..f70b280f5c 100755\n--- a/t/t3435-rebase-gpg-sign.sh\n+++ b/t/t3435-rebase-gpg-sign.sh\n@@ -68,4 +68,10 @@ 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 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_done\n-- \n2.28.0\n\n"},{"id":"407408","messageId":"20201012235234.mj7ejpmnbkfshte6@fastboi.localdomain","threadId":"54404","inReplyTo":"20201012234901.1356948-1-samuel@cavoj.net","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Samuel Čavoj","fromEmail":"samuel@cavoj.net","sentAt":"2020-10-12T23:52:34Z","receivedAt":"2020-10-12T23:52:37Z","isPatch":true,"sender":{"key":"samuel@cavoj.net","avatar":"https://avatars.githubusercontent.com/u/703905?v=4"},"body":"On 13.10.2020 01:49, Samuel Čavoj wrote:\n> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n\nOh boy, I left in a line by accident. Please remove this before\napplying, unless a v3 comes around. Sorry about that.\n\n> \n> When performing a rebase with --rebase-merges using either a custom\n> strategy specified with -s or an octopus merge, and at the same time\n> having gpgsign enabled (either rebase -S or config commit.gpgsign), the\n> operation would fail on making the merge commit. Instead of \"-S%s\" with\n> the key id substituted, only the bare key id would get passed to the\n> underlying merge command, which tried to interpret it as a ref.\n> \n> Fix the issue and add a test case as suggested by Johannes Schindelin.\n> \n> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> ---\n> changed v1 -> v2:\n>     added test case\n> ---\n>  sequencer.c                | 2 +-\n>  t/t3435-rebase-gpg-sign.sh | 6 ++++++\n>  2 files changed, 7 insertions(+), 1 deletion(-)\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>  \n>  \t\t/* Add the tips to be merged */\n>  \t\tfor (j = to_merge; j; j = j->next)\n> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\n> index b47c59c190..f70b280f5c 100755\n> --- a/t/t3435-rebase-gpg-sign.sh\n> +++ b/t/t3435-rebase-gpg-sign.sh\n> @@ -68,4 +68,10 @@ 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 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_done\n> -- \n> 2.28.0\n> \n"},{"id":"407421","messageId":"xmqqwnzulolg.fsf@gitster.c.googlers.com","threadId":"54404","inReplyTo":"20201012234901.1356948-2-samuel@cavoj.net","subject":"Re: [PATCH v2 2/2] sequencer: pass explicit --no-gpg-sign to merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-13T04:46:51Z","receivedAt":"2020-10-13T04:46:59Z","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> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> ---\n>  sequencer.c | 2 ++\n>  1 file changed, 2 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>  \t\t/* Add the tips to be merged */\n>  \t\tfor (j = to_merge; j; j = j->next)\n\nA test that passes --no-gpg-sign from the command line (because\ncommit.gpgsign is set in the repository used in these tests) would\nbe a good thing to have, next to the test you added in [PATCH 1/2].\nYou'd want to make sure the command line overrides the configured\ndefault by checking that the resulting history are not signed.\n\nThanks.\n\n"},{"id":"407434","messageId":"92b32c0a-68ef-f0f2-48d4-ddb3e1cc7245@gmail.com","threadId":"54404","inReplyTo":"20201012234901.1356948-1-samuel@cavoj.net","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-13T09:55:36Z","receivedAt":"2020-10-13T09:55:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Samuel\n\nThanks for re-rolling this\n\nOn 13/10/2020 00:49, Samuel Čavoj wrote:\n> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> \n> When performing a rebase with --rebase-merges using either a custom\n> strategy specified with -s or an octopus merge, and at the same time\n> having gpgsign enabled (either rebase -S or config commit.gpgsign), the\n> operation would fail on making the merge commit.\n\nSmall nit-pick: I think it worked fine with if commit.gpgsign was set \nand the user did not pass -S or --no-gpg-sign because merge would sign \nthe commits as commit.gpgsign was set, it was only if the user passed a \ngpg signing option to rebase that we had problems. I'm not sure it's \nworth a re-roll just for that though\n\nBest Wishes\n\nPhillip\n\n> Instead of \"-S%s\" with\n> the key id substituted, only the bare key id would get passed to the\n> underlying merge command, which tried to interpret it as a ref.\n> \n> Fix the issue and add a test case as suggested by Johannes Schindelin.\n> \n> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> ---\n> changed v1 -> v2:\n>      added test case\n> ---\n>   sequencer.c                | 2 +-\n>   t/t3435-rebase-gpg-sign.sh | 6 ++++++\n>   2 files changed, 7 insertions(+), 1 deletion(-)\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>   \n>   \t\t/* Add the tips to be merged */\n>   \t\tfor (j = to_merge; j; j = j->next)\n> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\n> index b47c59c190..f70b280f5c 100755\n> --- a/t/t3435-rebase-gpg-sign.sh\n> +++ b/t/t3435-rebase-gpg-sign.sh\n> @@ -68,4 +68,10 @@ 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 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_done\n> \n\n"},{"id":"407435","messageId":"d2491759-e761-647b-7e76-3515b8eeaf3f@gmail.com","threadId":"54404","inReplyTo":"20201012234901.1356948-1-samuel@cavoj.net","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-13T10:02:38Z","receivedAt":"2020-10-13T10:02:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Samuel\n\nOn 13/10/2020 00:49, Samuel Čavoj wrote:\n> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> \n> When performing a rebase with --rebase-merges using either a custom\n> strategy specified with -s or an octopus merge, and at the same time\n> having gpgsign enabled (either rebase -S or config commit.gpgsign), the\n> operation would fail on making the merge commit. Instead of \"-S%s\" with\n> the key id substituted, only the bare key id would get passed to the\n> underlying merge command, which tried to interpret it as a ref.\n> \n> Fix the issue and add a test case as suggested by Johannes Schindelin.\n> \n> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> ---\n> changed v1 -> v2:\n>      added test case\n> ---\n>   sequencer.c                | 2 +-\n>   t/t3435-rebase-gpg-sign.sh | 6 ++++++\n>   2 files changed, 7 insertions(+), 1 deletion(-)\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>   \n>   \t\t/* Add the tips to be merged */\n>   \t\tfor (j = to_merge; j; j = j->next)\n> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\n> index b47c59c190..f70b280f5c 100755\n> --- a/t/t3435-rebase-gpg-sign.sh\n> +++ b/t/t3435-rebase-gpg-sign.sh\n> @@ -68,4 +68,10 @@ 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 and merge strategies' '\n> +\tgit reset --hard merged &&\n> +\tgit rebase -fr --gpg-sign -s resolve --root &&\n> +\tgit verify-commit HEAD\n> +'\n\nUnfortunately I've just noticed that the test above runs\n\n\tgit config commit.gpgsign true\n\nSo this test would pass anyway if merge picks up that config setting. \nThe previous test needs to be changed to\n\n\ttest_config commit.gpgsign true\n\nso that the config setting is cleared when that test finishes. That \nprevious test would then be a good template for testing `rebase -r \n--no-gpg-sign` if you wanted to add a test for that to the next patch as \nJunio suggested.\n\nBest Wishes\n\nPhillip\n\n>   test_done\n> \n\n"},{"id":"407436","messageId":"c408b835-a09d-d261-0b98-cd1d0db3610d@gmail.com","threadId":"54404","inReplyTo":"20201012234901.1356948-2-samuel@cavoj.net","subject":"Re: [PATCH v2 2/2] sequencer: pass explicit --no-gpg-sign to merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-13T10:03:48Z","receivedAt":"2020-10-13T10:03:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Samuel\n\nOn 13/10/2020 00:49, Samuel Čavoj wrote:\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\nThat's a good explanation, thanks very much for adding this patch\n\nPhillip\n\n> \n> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> ---\n>   sequencer.c | 2 ++\n>   1 file changed, 2 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>   \t\t/* Add the tips to be merged */\n>   \t\tfor (j = to_merge; j; j = j->next)\n> \n\n"},{"id":"407437","messageId":"20201013104346.oyjvlcnqhuuzkpzh@fastboi.localdomain","threadId":"54404","inReplyTo":"92b32c0a-68ef-f0f2-48d4-ddb3e1cc7245@gmail.com","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Samuel Čavoj","fromEmail":"samuel@cavoj.net","sentAt":"2020-10-13T10:43:46Z","receivedAt":"2020-10-13T10:43:50Z","isPatch":true,"sender":{"key":"samuel@cavoj.net","avatar":"https://avatars.githubusercontent.com/u/703905?v=4"},"body":"Hi Phillip,\n\nOn 13.10.2020 10:55, Phillip Wood wrote:\n> Hi Samuel\n> \n> Thanks for re-rolling this\n> \n> On 13/10/2020 00:49, Samuel Čavoj wrote:\n> > From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> > \n> > When performing a rebase with --rebase-merges using either a custom\n> > strategy specified with -s or an octopus merge, and at the same time\n> > having gpgsign enabled (either rebase -S or config commit.gpgsign), the\n> > operation would fail on making the merge commit.\n> \n> Small nit-pick: I think it worked fine with if commit.gpgsign was set and\n> the user did not pass -S or --no-gpg-sign because merge would sign the\n> commits as commit.gpgsign was set, it was only if the user passed a gpg\n> signing option to rebase that we had problems. I'm not sure it's worth a\n> re-roll just for that though\n\nThis is not the case. That's how I encountered the problem initially, I\nhave commit.gpgsign set to true globally. I ran a rebase -ir, over an\noctopus merge and then it would fail with an error in the lines of 'not\nsomething we can merge'. I later found out it didn't happen on my\nlaptop, where gpgsign is not set, so that's what gave it away. In either\ncase, I did not pass neither -S nor --no-gpg-sign to rebase.\n\nYes, _if_ the merge command went through, it would have choosen the\ncorrect signing behaviour (i.e. the default), but the merge died,\nbecause an empty string was being passed to it as one of the commits to\nmerge.\n\noff-topic p.s.:\nMy mail server does not currently have a proper rDNS record (yeah yeah,\nI know) and for this reason, gmx.net drops my emails unconditionally. As\nsuch, I am unable to send emails directly to Johannes.Schindelin@gmx.de,\nwithout major hackery, anyway. I am dropping him from Cc, as to prevent\nsad mailservers. I hope these messages reach him via the mailing list.\n\nRegards,\nSamuel\n\n> \n> Best Wishes\n> \n> Phillip\n> \n> > Instead of \"-S%s\" with\n> > the key id substituted, only the bare key id would get passed to the\n> > underlying merge command, which tried to interpret it as a ref.\n> > \n> > Fix the issue and add a test case as suggested by Johannes Schindelin.\n> > \n> > Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> > ---\n> > changed v1 -> v2:\n> >      added test case\n> > ---\n> >   sequencer.c                | 2 +-\n> >   t/t3435-rebase-gpg-sign.sh | 6 ++++++\n> >   2 files changed, 7 insertions(+), 1 deletion(-)\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> >   \t\t/* Add the tips to be merged */\n> >   \t\tfor (j = to_merge; j; j = j->next)\n> > diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\n> > index b47c59c190..f70b280f5c 100755\n> > --- a/t/t3435-rebase-gpg-sign.sh\n> > +++ b/t/t3435-rebase-gpg-sign.sh\n> > @@ -68,4 +68,10 @@ test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' '\n> >   \ttest_must_fail git verify-commit HEAD\n> >   '\n> > +test_expect_success 'rebase -r, GPG 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_done\n> > \n> \n"},{"id":"407438","messageId":"20201013105159.rfpczn563agvih5y@fastboi.localdomain","threadId":"54404","inReplyTo":"d2491759-e761-647b-7e76-3515b8eeaf3f@gmail.com","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Samuel Čavoj","fromEmail":"samuel@cavoj.net","sentAt":"2020-10-13T10:51:59Z","receivedAt":"2020-10-13T10:52:05Z","isPatch":true,"sender":{"key":"samuel@cavoj.net","avatar":"https://avatars.githubusercontent.com/u/703905?v=4"},"body":"Hi Phillip,\n\nthank you very much for the feedback.\n\nOn 13.10.2020 11:02, Phillip Wood wrote:\n> Hi Samuel\n> \n> On 13/10/2020 00:49, Samuel Čavoj wrote:\n> > From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> > \n> > When performing a rebase with --rebase-merges using either a custom\n> > strategy specified with -s or an octopus merge, and at the same time\n> > having gpgsign enabled (either rebase -S or config commit.gpgsign), the\n> > operation would fail on making the merge commit. Instead of \"-S%s\" with\n> > the key id substituted, only the bare key id would get passed to the\n> > underlying merge command, which tried to interpret it as a ref.\n> > \n> > Fix the issue and add a test case as suggested by Johannes Schindelin.\n> > \n> > Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> > ---\n> > changed v1 -> v2:\n> >      added test case\n> > ---\n> >   sequencer.c                | 2 +-\n> >   t/t3435-rebase-gpg-sign.sh | 6 ++++++\n> >   2 files changed, 7 insertions(+), 1 deletion(-)\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> >   \t\t/* Add the tips to be merged */\n> >   \t\tfor (j = to_merge; j; j = j->next)\n> > diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\n> > index b47c59c190..f70b280f5c 100755\n> > --- a/t/t3435-rebase-gpg-sign.sh\n> > +++ b/t/t3435-rebase-gpg-sign.sh\n> > @@ -68,4 +68,10 @@ test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' '\n> >   \ttest_must_fail git verify-commit HEAD\n> >   '\n> > +test_expect_success 'rebase -r, GPG 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> Unfortunately I've just noticed that the test above runs\n> \n> \tgit config commit.gpgsign true\n> \n> So this test would pass anyway if merge picks up that config setting. The\n> previous test needs to be changed to\n> \n> \ttest_config commit.gpgsign true\n\nShould I do that now, maybe as a separate part of the patch series? Or\njust override the config with a `test_config commit.gpgsign false` in\nthe test I added?\n\nThere is another usage of `git config` in the file, in the\ntest_rebase_gpg_sign function. \n\n> \n> so that the config setting is cleared when that test finishes. That previous\n> test would then be a good template for testing `rebase -r --no-gpg-sign` if\n> you wanted to add a test for that to the next patch as Junio suggested.\n\nYes, I will definitely do that in v3. Also, the previous test is expect_failure,\nthat means its a known bug?\n\nRegards,\nSamuel\n\n> \n> Best Wishes\n> \n> Phillip\n> \n> >   test_done\n> > \n> \n"},{"id":"407440","messageId":"4571911c-bb2c-935a-e0bf-5f9cbccd341e@gmail.com","threadId":"54404","inReplyTo":"20201013104346.oyjvlcnqhuuzkpzh@fastboi.localdomain","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-13T13:15:43Z","receivedAt":"2020-10-13T13:15:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Samuel\n\nOn 13/10/2020 11:43, Samuel Čavoj wrote:\n> Hi Phillip,\n> \n> On 13.10.2020 10:55, Phillip Wood wrote:\n>> Hi Samuel\n>>\n>> Thanks for re-rolling this\n>>\n>> On 13/10/2020 00:49, Samuel Čavoj wrote:\n>>> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n>>>\n>>> When performing a rebase with --rebase-merges using either a custom\n>>> strategy specified with -s or an octopus merge, and at the same time\n>>> having gpgsign enabled (either rebase -S or config commit.gpgsign), the\n>>> operation would fail on making the merge commit.\n>>\n>> Small nit-pick: I think it worked fine with if commit.gpgsign was set and\n>> the user did not pass -S or --no-gpg-sign because merge would sign the\n>> commits as commit.gpgsign was set, it was only if the user passed a gpg\n>> signing option to rebase that we had problems. I'm not sure it's worth a\n>> re-roll just for that though\n> \n> This is not the case. That's how I encountered the problem initially, I\n> have commit.gpgsign set to true globally. I ran a rebase -ir, over an\n> octopus merge and then it would fail with an error in the lines of 'not\n> something we can merge'. I later found out it didn't happen on my\n> laptop, where gpgsign is not set, so that's what gave it away. In either\n> case, I did not pass neither -S nor --no-gpg-sign to rebase.\n> \n> Yes, _if_ the merge command went through, it would have choosen the\n> correct signing behaviour (i.e. the default), but the merge died,\n> because an empty string was being passed to it as one of the commits to\n> merge.\n\nOh sorry for some reason I thought we just ignored opts->gpg_sign before \nyour patch I'd forgotten we actually passed it without the '-S' - thanks \nfor correcting me.\n\n> off-topic p.s.:\n> My mail server does not currently have a proper rDNS record (yeah yeah,\n> I know) and for this reason, gmx.net drops my emails unconditionally.\n\nIt was dropping my emails sent from a gmail account a few weeks back\n\n> As\n> such, I am unable to send emails directly to Johannes.Schindelin@gmx.de,\n> without major hackery, anyway. I am dropping him from Cc, as to prevent\n> sad mailservers. I hope these messages reach him via the mailing list.\n\nHe's subscribed to the list so hopefully will see them that way\n\nBest Wishes\n\nPhillip\n\n> Regards,\n> Samuel\n> \n>>\n>> Best Wishes\n>>\n>> Phillip\n>>\n>>> Instead of \"-S%s\" with\n>>> the key id substituted, only the bare key id would get passed to the\n>>> underlying merge command, which tried to interpret it as a ref.\n>>>\n>>> Fix the issue and add a test case as suggested by Johannes Schindelin.\n>>>\n>>> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n>>> ---\n>>> changed v1 -> v2:\n>>>       added test case\n>>> ---\n>>>    sequencer.c                | 2 +-\n>>>    t/t3435-rebase-gpg-sign.sh | 6 ++++++\n>>>    2 files changed, 7 insertions(+), 1 deletion(-)\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>>>    \t\t/* Add the tips to be merged */\n>>>    \t\tfor (j = to_merge; j; j = j->next)\n>>> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\n>>> index b47c59c190..f70b280f5c 100755\n>>> --- a/t/t3435-rebase-gpg-sign.sh\n>>> +++ b/t/t3435-rebase-gpg-sign.sh\n>>> @@ -68,4 +68,10 @@ test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' '\n>>>    \ttest_must_fail git verify-commit HEAD\n>>>    '\n>>> +test_expect_success 'rebase -r, GPG 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_done\n>>>\n>>\n"},{"id":"407441","messageId":"1bc5b455-68f7-eb7a-763d-d541086ddcdb@gmail.com","threadId":"54404","inReplyTo":"20201013105159.rfpczn563agvih5y@fastboi.localdomain","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-13T13:28:20Z","receivedAt":"2020-10-13T13:28:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Samuel\n\nOn 13/10/2020 11:51, Samuel Čavoj wrote:\n> Hi Phillip,\n> \n> thank you very much for the feedback.\n> \n> On 13.10.2020 11:02, Phillip Wood wrote:\n>> Hi Samuel\n>>\n>> On 13/10/2020 00:49, Samuel Čavoj wrote:\n>>> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n>>>\n>>> When performing a rebase with --rebase-merges using either a custom\n>>> strategy specified with -s or an octopus merge, and at the same time\n>>> having gpgsign enabled (either rebase -S or config commit.gpgsign), the\n>>> operation would fail on making the merge commit. Instead of \"-S%s\" with\n>>> the key id substituted, only the bare key id would get passed to the\n>>> underlying merge command, which tried to interpret it as a ref.\n>>>\n>>> Fix the issue and add a test case as suggested by Johannes Schindelin.\n>>>\n>>> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n>>> ---\n>>> changed v1 -> v2:\n>>>       added test case\n>>> ---\n>>>    sequencer.c                | 2 +-\n>>>    t/t3435-rebase-gpg-sign.sh | 6 ++++++\n>>>    2 files changed, 7 insertions(+), 1 deletion(-)\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>>>    \t\t/* Add the tips to be merged */\n>>>    \t\tfor (j = to_merge; j; j = j->next)\n>>> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\n>>> index b47c59c190..f70b280f5c 100755\n>>> --- a/t/t3435-rebase-gpg-sign.sh\n>>> +++ b/t/t3435-rebase-gpg-sign.sh\n>>> @@ -68,4 +68,10 @@ test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' '\n>>>    \ttest_must_fail git verify-commit HEAD\n>>>    '\n>>> +test_expect_success 'rebase -r, GPG 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>> Unfortunately I've just noticed that the test above runs\n>>\n>> \tgit config commit.gpgsign true\n>>\n>> So this test would pass anyway if merge picks up that config setting.\n\nThat's predicated on my misunderstanding of the behavior without your \npatch but it would be good to fix the test anyway\n\n>> The\n>> previous test needs to be changed to\n>>\n>> \ttest_config commit.gpgsign true\n> \n> Should I do that now, maybe as a separate part of the patch series? Or\n> just override the config with a `test_config commit.gpgsign false` in\n> the test I added?\n\nAs you've found another instance that needs changing it would probably \nbe better as a separate patch\n\n> There is another usage of `git config` in the file, in the\n> test_rebase_gpg_sign function.\n\nWell spotted, that should be changed as well\n\n>> so that the config setting is cleared when that test finishes. That previous\n>> test would then be a good template for testing `rebase -r --no-gpg-sign` if\n>> you wanted to add a test for that to the next patch as Junio suggested.\n> \n> Yes, I will definitely do that in v3.\n\nThanks\n\n> Also, the previous test is expect_failure,\n> that means its a known bug?\n\nYes, `rebase -p` is deprecated and it looks like it was skipped by the \nrecent fix for `rebase --no-gpg-sign` in c241371c04 (\"rebase.c: honour \n--no-gpg-sign\", 2020-04-03)\n\n\nBest Wishes\n\nPhillip\n\n> Regards,\n> Samuel\n> \n>>\n>> Best Wishes\n>>\n>> Phillip\n>>\n>>>    test_done\n>>>\n>>\n"},{"id":"407490","messageId":"xmqqy2k9ixvz.fsf@gitster.c.googlers.com","threadId":"54404","inReplyTo":"d2491759-e761-647b-7e76-3515b8eeaf3f@gmail.com","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-13T22:06:40Z","receivedAt":"2020-10-13T22:06:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n>>   +test_expect_success 'rebase -r, GPG 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> Unfortunately I've just noticed that the test above runs\n>\n> \tgit config commit.gpgsign true\n>\n> So this test would pass anyway if merge picks up that config\n> setting. The previous test needs to be changed to\n>\n> \ttest_config commit.gpgsign true\n>\n> so that the config setting is cleared when that test finishes.\n\nThanks for a review, but I do not think that is a right way to go.\n\ntest_config has an inherent assumption that not having the config at\nall is somehow the \"natural\" state, and if that holds true, that\nwould be OK.  But what is \"natural\" is subjective X-<.\n\nThe way each test is run by calling test_rebase_gpg_sign repeatedly\nuses a different and more robust approach to ensure that previous\ntest does not affect the current one.  Each invocation of test\nexplicitly sets the configuration to the state the test wants to,\ncancelling what the previous test did.\n\nTo blend in better with existing tests and match their robustness\nexpectations, the right fix is for this new test to explicitly use\n\"git config --set\" or \"git config --unset\" to make the variable into\nthe desired state, regardless of what the previous tests did.  \n\nIf the test quoted at the beginning of this message wants to make\nsure that --gpg-sign from the command line takes effect without\ncommit.gpgsign set, it should unset the variable explicitly.\nAnother combination worth testing is to ensure that --gpg-sign takes\neffect when commit.gpgsign is explicitly set to false (not \"left\nunset\").\n\nThanks.\n\n\n\n"},{"id":"407499","messageId":"20201013234519.73grqc2kkybccryf@fastboi.localdomain","threadId":"54404","inReplyTo":"xmqqy2k9ixvz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Samuel Čavoj","fromEmail":"samuel@cavoj.net","sentAt":"2020-10-13T23:45:19Z","receivedAt":"2020-10-14T09:29:27Z","isPatch":true,"sender":{"key":"samuel@cavoj.net","avatar":"https://avatars.githubusercontent.com/u/703905?v=4"},"body":"Hi,\n\nOn 13.10.2020 15:06, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n> >>   +test_expect_success 'rebase -r, GPG 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> > Unfortunately I've just noticed that the test above runs\n> >\n> > \tgit config commit.gpgsign true\n> >\n> > So this test would pass anyway if merge picks up that config\n> > setting. The previous test needs to be changed to\n> >\n> > \ttest_config commit.gpgsign true\n> >\n> > so that the config setting is cleared when that test finishes.\n> \n> Thanks for a review, but I do not think that is a right way to go.\n> \n> test_config has an inherent assumption that not having the config at\n> all is somehow the \"natural\" state, and if that holds true, that\n> would be OK.  But what is \"natural\" is subjective X-<.\n> \n> The way each test is run by calling test_rebase_gpg_sign repeatedly\n> uses a different and more robust approach to ensure that previous\n> test does not affect the current one.  Each invocation of test\n> explicitly sets the configuration to the state the test wants to,\n> cancelling what the previous test did.\n> \n> To blend in better with existing tests and match their robustness\n> expectations, the right fix is for this new test to explicitly use\n> \"git config --set\" or \"git config --unset\" to make the variable into\n> the desired state, regardless of what the previous tests did.  \n\nThis is more or less what I did in v3, although not completely\ncomprehensively. Of course, the test_rebase_gpg_sign function could be\neither duplicated or modified to be more generic in order to accommodate\nall combinations for the new tests, though I considered this not worth\ndoing. I am open to it, however.\n\nI'm unfamiliar with the git codebase and the testing practices, so a\nlittle hand-holding might be needed, I hope that's not a bother for you.\n\nIn addition, I have replaced `git config` usages with `test_config`, not\nsure if that should be reverted -- I see no harm in using it, and I\nguess it is slightly more explicit.\n\nSpeaking of v3, I pulled the trigger a little too fast on that one,\nshould have waited a little longer for feedback. Sorry about that.\n\nThough, I am not sure what has happened to it. I sent it out in the\nusual manner, but it hasn't appeared in any of the mailing list\narchives. Can you please confirm (or deny) that you have received it?\nIn case not, should I just send it again, with the same Message-Id's?\nAll the recipient mail servers responded with a 250 Ok from my postfix\nlogs.\n\nThank you for all the help.\n\nRegards,\nSamuel\n\n> \n> If the test quoted at the beginning of this message wants to make\n> sure that --gpg-sign from the command line takes effect without\n> commit.gpgsign set, it should unset the variable explicitly.\n> Another combination worth testing is to ensure that --gpg-sign takes\n> effect when commit.gpgsign is explicitly set to false (not \"left\n> unset\").\n> \n> Thanks.\n> \n"},{"id":"407519","messageId":"xmqqimbcj02p.fsf@gitster.c.googlers.com","threadId":"54404","inReplyTo":"20201013234519.73grqc2kkybccryf@fastboi.localdomain","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-14T15:31:42Z","receivedAt":"2020-10-14T15:31:47Z","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> Speaking of v3, I pulled the trigger a little too fast on that one,\n> should have waited a little longer for feedback. Sorry about that.\n\nNothing to be sorry about---working asynchronously, mails crossing\nand timings not coinciding are just expected parts of life.  It is\nthe kind of thing participants need to learn to adjust, as \"normal\nlatency\" is different from community to community.\n\n> Though, I am not sure what has happened to it. I sent it out in the\n> usual manner, but it hasn't appeared in any of the mailing list\n> archives. Can you please confirm (or deny) that you have received it?\n> In case not, should I just send it again, with the same Message-Id's?\n> All the recipient mail servers responded with a 250 Ok from my postfix\n> logs.\n\nI see the message I am responding to has a timestamp that is about\n16 hours ago (relative to the time I am typing this response), but I\nam fairly sure that I didn't see it ten hours ago (i.e. my last\nnight's final e-mail scanning).\n\nPerhaps there was congestion somewhere that held some messages.\n"},{"id":"407717","messageId":"31ce457b-e71c-0ca0-e5be-a9aebb9cf785@gmail.com","threadId":"54404","inReplyTo":"xmqqy2k9ixvz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-16T13:40:00Z","receivedAt":"2020-10-16T13:40:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nIt seems to be that the config handling here is another example an \ninterdependence between tests that makes life harder for contributors \nand reviewers.\n\nOn 13/10/2020 23:06, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>>>    +test_expect_success 'rebase -r, GPG 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>> Unfortunately I've just noticed that the test above runs\n>>\n>> \tgit config commit.gpgsign true\n>>\n>> So this test would pass anyway if merge picks up that config\n>> setting. The previous test needs to be changed to\n>>\n>> \ttest_config commit.gpgsign true\n>>\n>> so that the config setting is cleared when that test finishes.\n> \n> Thanks for a review, but I do not think that is a right way to go.\n> \n> test_config has an inherent assumption that not having the config at\n> all is somehow the \"natural\" state, and if that holds true, that\n> would be OK.  But what is \"natural\" is subjective X-<.\n\nWhat is \"natural\" is subjective but what is the default config is not. \nIf test scripts set random config variables and assumes that they will \nbe cleared in later tests if they don't want them it makes it very hard \nfor contributors and reviewers to check that new tests are sound as they \nhave to analyze each existing test in the script. In this example I \nbelieve the new test was contributed by dscho who is an experienced \ncontributor with an interest in the test suite. However the test did not \nclear the relevant config variable - if an experienced contributor did \nnot realize that the variable needed to be cleared how are new \ncontributors supposed to figure it out? If each test starts with the \ndefault config it is much easier to reason about it.\n\n> The way each test is run by calling test_rebase_gpg_sign repeatedly\n> uses a different and more robust approach to ensure that previous\n> test does not affect the current one.  Each invocation of test\n> explicitly sets the configuration to the state the test wants to,\n> cancelling what the previous test did.\n\nIt is only robust if contributors and reviewers realize that is what is \nexpected. Reviewers that only read the patch without loading up the test \nfile in their editor have no indication that the test should be clearing \nthe config variable.\n\nBest Wishes\n\nPhillip\n\n> To blend in better with existing tests and match their robustness\n> expectations, the right fix is for this new test to explicitly use\n> \"git config --set\" or \"git config --unset\" to make the variable into\n> the desired state, regardless of what the previous tests did.\n> \n> If the test quoted at the beginning of this message wants to make\n> sure that --gpg-sign from the command line takes effect without\n> commit.gpgsign set, it should unset the variable explicitly.\n> Another combination worth testing is to ensure that --gpg-sign takes\n> effect when commit.gpgsign is explicitly set to false (not \"left\n> unset\").\n> \n> Thanks.\n> \n> \n> \n\n"},{"id":"407725","messageId":"xmqqy2k6cejl.fsf@gitster.c.googlers.com","threadId":"54404","inReplyTo":"31ce457b-e71c-0ca0-e5be-a9aebb9cf785@gmail.com","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-16T16:37:50Z","receivedAt":"2020-10-16T16:37:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> ... Reviewers that only read the patch without loading up the\n> test file in their editor have no indication that the test should be\n> clearing the config variable.\n\nIt is not a review if the code being updated is not checked for\nsanity in its own context, is it?\n\n"},{"id":"407728","messageId":"4bbf0055-ed50-6560-7208-dce9378389c8@gmail.com","threadId":"54404","inReplyTo":"xmqqy2k6cejl.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-16T17:25:05Z","receivedAt":"2020-10-16T17:25:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/10/2020 17:37, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> ... Reviewers that only read the patch without loading up the\n>> test file in their editor have no indication that the test should be\n>> clearing the config variable.\n> \n> It is not a review if the code being updated is not checked for\n> sanity in its own context, is it?\n\nIt is disappointing that you have chosen to cut the original to those \nfew lines and reply just to them. The original message made the broader \npoint that having to check the previous tests in a file to figure out \nwhat the state is at the beginning of any new test that gets added makes \nlife harder than it needs to be both for contributors and reviewers.\n\nPhillip\n"},{"id":"407752","messageId":"xmqq8sc6apwj.fsf@gitster.c.googlers.com","threadId":"54404","inReplyTo":"31ce457b-e71c-0ca0-e5be-a9aebb9cf785@gmail.com","subject":"Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-16T20:15:24Z","receivedAt":"2020-10-16T20:15:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> What is \"natural\" is subjective but what is the default config is\n> not. If test scripts set random config variables and assumes that they\n> will be ...\n> ... cleared how are new contributors supposed to figure it out? If each\n> test starts with the default config it is much easier to reason about\n> it.\n\nIf this were a test script with many tests, all of which depend on\nstarting from clean slate wrt the configuration variable space, and\nif you are adding just one or two tests that wants to run with\nconfiguration variable in effect, the story would be quite\ndifferent.  For example, I would think it would make a lot of sense\nin <20201014232517.3068298-1-emilyshaffer@google.com> to use\ntest_config instead of \"git config\" as it is clear that leading 100+\nlines of tests run with the default configuration, and it is sane to\nexpact that later tests that may be added in the future would be the\nsame.\n\nLook at the test script in question again and notice that it is\nabout seeing behaviour with the single configuration variable set to\ntrue or false.  Using test_config would still signal the test pieces\nthat use it do depend on the shown setting of the variable, but it\ndoes not help at all the test pieces that wants to see what happens\nwhen there is no configuration.  Explicitly using \"git config\" and\n\"test_unconfig\" actually would _help_ reviewers who only looks\nbetween the pre- and post- context lines that are affected by the\npatch, as they do not have to know or assume that \"normal state is\nnothing configured\" (which needs to be verified by seeing all the\nexisting tests that are not shown in the patch use test_config to\nclear the setting when they are done).\n\nAll of the above would be obvious once you think about it, I'd\nthink.\n\nThanks.\n\n"}]}