{"thread":{"id":"42166","subject":"[PATCH] pull: make --rebase --verify-signatures illegal","startedAt":"2016-04-28T09:52:54Z","lastAt":"2016-04-28T17:23:26Z","messageCount":4,"participants":["Alexander 'z33ky' Hirsch","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"284780","messageId":"20160428095254.GA5205@netblarch","threadId":"42166","inReplyTo":null,"subject":"[PATCH] pull: make --rebase --verify-signatures illegal","fromName":"Alexander 'z33ky' Hirsch","fromEmail":"1zeeky@gmail.com","sentAt":"2016-04-28T09:52:54Z","receivedAt":"2016-04-28T09:52:54Z","isPatch":true,"sender":{"key":"1zeeky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/289819?v=4"},"body":"Previously git-pull would silently ignore the --verify-signatures\noption.\n\nSigned-off-by: Alexander 'z33ky' Hirsch <1zeeky@gmail.com>\n---\n\nWe had some discussion back in December about a patch that added\n--verify-signatures to git-pull, that was declined.\nI proposed making git-pull --rebase --verify-signatures illegal then,\nalthough it still remained open whether to just warn or make it an\nerror.\n\nIn this patch I opted into making it an error, which breaks the\npreviously accepted git pull --rebase --verify-signatures of course,\nalbeit I'd argue that it probably didn't do what the user expected\nanyways.\nI don't know if there are compatibility concerns with this though.\n\n builtin/pull.c  |  2 ++\n t/t5520-pull.sh | 14 ++++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex d98f481..b6e1507 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -809,6 +809,8 @@ static int run_rebase(const unsigned char *curr_head,\n \t\targv_array_push(&args, \"--no-autostash\");\n \telse if (opt_autostash == 1)\n \t\targv_array_push(&args, \"--autostash\");\n+\tif (opt_verify_signatures && strcmp(opt_verify_signatures, \"--verify-signatures\") == 0)\n+\t\tdie(_(\"The --verify-signatures option does not work for --rebase.\"));\n \n \targv_array_push(&args, \"--onto\");\n \targv_array_push(&args, sha1_to_hex(merge_head));\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 739c089..cb8f741 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -341,6 +341,20 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '\n \ttest new = \"$(git show HEAD:file2)\"\n '\n \n+test_expect_success \"pull --rebase --verify-signatures is illegal\" '\n+\tgit reset --hard before-rebase &&\n+\ttest_must_fail git pull --rebase --verify-signatures . copy 2>err &&\n+\ttest \"$(git rev-parse HEAD)\" = \"$(git rev-parse before-rebase)\" &&\n+\ttest_i18ngrep \"The --verify-signatures option does not work for --rebase.\" err\n+'\n+\n+test_expect_success \"pull --rebase --no-verify-signatures\" '\n+\tgit reset --hard before-rebase &&\n+\tgit pull --rebase --no-verify-signatures . copy &&\n+\ttest \"$(git rev-parse HEAD^)\" = \"$(git rev-parse copy)\" &&\n+\ttest new = \"$(git show HEAD:file2)\"\n+'\n+\n # add a feature branch, keep-merge, that is merged into master, so the\n # test can try preserving the merge commit (or not) with various\n # --rebase flags/pull.rebase settings.\n-- \n2.8.0\n"},{"id":"284796","messageId":"20160428134435.GA5901@netblarch","threadId":"42166","inReplyTo":"20160428095254.GA5205@netblarch","subject":"[PATCH v2] pull: make --rebase --verify-signatures illegal","fromName":"Alexander 'z33ky' Hirsch","fromEmail":"1zeeky@gmail.com","sentAt":"2016-04-28T13:44:35Z","receivedAt":"2016-04-28T13:44:35Z","isPatch":true,"sender":{"key":"1zeeky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/289819?v=4"},"body":"Previously git-pull would silently ignore the --verify-signatures\noption.\n\nSigned-off-by: Alexander 'z33ky' Hirsch <1zeeky@gmail.com>\n---\n\nI made the error-message conform to the CodingGuidelines (removed\ncapitalization and full stop).\n\nAlso, in the previous mail I said that I proposed a patch for git-pull\nlast December, when I actually meant git-rebase.\n\n builtin/pull.c  |  2 ++\n t/t5520-pull.sh | 14 ++++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex d98f481..b6e1507 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -809,6 +809,8 @@ static int run_rebase(const unsigned char *curr_head,\n \t\targv_array_push(&args, \"--no-autostash\");\n \telse if (opt_autostash == 1)\n \t\targv_array_push(&args, \"--autostash\");\n+\tif (opt_verify_signatures && strcmp(opt_verify_signatures, \"--verify-signatures\") == 0)\n+\t\tdie(_(\"the --verify-signatures option does not work for --rebase\"));\n \n \targv_array_push(&args, \"--onto\");\n \targv_array_push(&args, sha1_to_hex(merge_head));\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 739c089..cb8f741 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -341,6 +341,20 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '\n \ttest new = \"$(git show HEAD:file2)\"\n '\n \n+test_expect_success \"pull --rebase --verify-signatures is illegal\" '\n+\tgit reset --hard before-rebase &&\n+\ttest_must_fail git pull --rebase --verify-signatures . copy 2>err &&\n+\ttest \"$(git rev-parse HEAD)\" = \"$(git rev-parse before-rebase)\" &&\n+\ttest_i18ngrep \"The --verify-signatures option does not work for --rebase.\" err\n+'\n+\n+test_expect_success \"pull --rebase --no-verify-signatures\" '\n+\tgit reset --hard before-rebase &&\n+\tgit pull --rebase --no-verify-signatures . copy &&\n+\ttest \"$(git rev-parse HEAD^)\" = \"$(git rev-parse copy)\" &&\n+\ttest new = \"$(git show HEAD:file2)\"\n+'\n+\n # add a feature branch, keep-merge, that is merged into master, so the\n # test can try preserving the merge commit (or not) with various\n # --rebase flags/pull.rebase settings.\n-- \n2.8.0\n"},{"id":"284845","messageId":"CAGZ79kbGOsPQYykWMJoAMcBGzxgQa_5H_RdR=ZXMokiFipWE_w@mail.gmail.com","threadId":"42166","inReplyTo":"20160428095254.GA5205@netblarch","subject":"Re: [PATCH] pull: make --rebase --verify-signatures illegal","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-28T17:12:05Z","receivedAt":"2016-04-28T17:12:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 28, 2016 at 2:52 AM, Alexander 'z33ky' Hirsch\n<1zeeky@gmail.com> wrote:\n> Previously git-pull would silently ignore the --verify-signatures\n> option.\n>\n> Signed-off-by: Alexander 'z33ky' Hirsch <1zeeky@gmail.com>\n> ---\n>\n> We had some discussion back in December about a patch that added\n> --verify-signatures to git-pull, that was declined.\n> I proposed making git-pull --rebase --verify-signatures illegal then,\n> although it still remained open whether to just warn or make it an\n> error.\n>\n> In this patch I opted into making it an error, which breaks the\n> previously accepted git pull --rebase --verify-signatures of course,\n> albeit I'd argue that it probably didn't do what the user expected\n> anyways.\n> I don't know if there are compatibility concerns with this though.\n>\n>  builtin/pull.c  |  2 ++\n>  t/t5520-pull.sh | 14 ++++++++++++++\n>  2 files changed, 16 insertions(+)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index d98f481..b6e1507 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -809,6 +809,8 @@ static int run_rebase(const unsigned char *curr_head,\n>                 argv_array_push(&args, \"--no-autostash\");\n>         else if (opt_autostash == 1)\n>                 argv_array_push(&args, \"--autostash\");\n> +       if (opt_verify_signatures && strcmp(opt_verify_signatures, \"--verify-signatures\") == 0)\n> +               die(_(\"The --verify-signatures option does not work for --rebase.\"));\n\nCan you explain why it doesn't work (In the commit message)?\n\n>\n>         argv_array_push(&args, \"--onto\");\n>         argv_array_push(&args, sha1_to_hex(merge_head));\n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> index 739c089..cb8f741 100755\n> --- a/t/t5520-pull.sh\n> +++ b/t/t5520-pull.sh\n> @@ -341,6 +341,20 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '\n>         test new = \"$(git show HEAD:file2)\"\n>  '\n>\n> +test_expect_success \"pull --rebase --verify-signatures is illegal\" '\n> +       git reset --hard before-rebase &&\n> +       test_must_fail git pull --rebase --verify-signatures . copy 2>err &&\n> +       test \"$(git rev-parse HEAD)\" = \"$(git rev-parse before-rebase)\" &&\n> +       test_i18ngrep \"The --verify-signatures option does not work for --rebase.\" err\n> +'\n> +\n> +test_expect_success \"pull --rebase --no-verify-signatures\" '\n> +       git reset --hard before-rebase &&\n> +       git pull --rebase --no-verify-signatures . copy &&\n> +       test \"$(git rev-parse HEAD^)\" = \"$(git rev-parse copy)\" &&\n> +       test new = \"$(git show HEAD:file2)\"\n> +'\n> +\n>  # add a feature branch, keep-merge, that is merged into master, so the\n>  # test can try preserving the merge commit (or not) with various\n>  # --rebase flags/pull.rebase settings.\n> --\n> 2.8.0\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"284846","messageId":"xmqqmvod641d.fsf@gitster.mtv.corp.google.com","threadId":"42166","inReplyTo":"20160428134435.GA5901@netblarch","subject":"Re: [PATCH v2] pull: make --rebase --verify-signatures illegal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-28T17:23:26Z","receivedAt":"2016-04-28T17:23:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander 'z33ky' Hirsch <1zeeky@gmail.com> writes:\n\n> Previously git-pull would silently ignore the --verify-signatures\n> option.\n\nI do not see a point of making it error out.  Adding a warning about\nthe option being ignored might be a worthwhile thing to do (e.g. it\nmay solicit responses from those who have been depending on it not\nerroring out, saying \"these warnings are pointless and noisy--here\nis my valid use case that ends up passing --rebase and --v-s at the\nsame time\"), though.\n\n\n\n> Signed-off-by: Alexander 'z33ky' Hirsch <1zeeky@gmail.com>\n> ---\n>\n> I made the error-message conform to the CodingGuidelines (removed\n> capitalization and full stop).\n>\n> Also, in the previous mail I said that I proposed a patch for git-pull\n> last December, when I actually meant git-rebase.\n>\n>  builtin/pull.c  |  2 ++\n>  t/t5520-pull.sh | 14 ++++++++++++++\n>  2 files changed, 16 insertions(+)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index d98f481..b6e1507 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -809,6 +809,8 @@ static int run_rebase(const unsigned char *curr_head,\n>  \t\targv_array_push(&args, \"--no-autostash\");\n>  \telse if (opt_autostash == 1)\n>  \t\targv_array_push(&args, \"--autostash\");\n> +\tif (opt_verify_signatures && strcmp(opt_verify_signatures, \"--verify-signatures\") == 0)\n> +\t\tdie(_(\"the --verify-signatures option does not work for --rebase\"));\n>  \n>  \targv_array_push(&args, \"--onto\");\n>  \targv_array_push(&args, sha1_to_hex(merge_head));\n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> index 739c089..cb8f741 100755\n> --- a/t/t5520-pull.sh\n> +++ b/t/t5520-pull.sh\n> @@ -341,6 +341,20 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '\n>  \ttest new = \"$(git show HEAD:file2)\"\n>  '\n>  \n> +test_expect_success \"pull --rebase --verify-signatures is illegal\" '\n> +\tgit reset --hard before-rebase &&\n> +\ttest_must_fail git pull --rebase --verify-signatures . copy 2>err &&\n> +\ttest \"$(git rev-parse HEAD)\" = \"$(git rev-parse before-rebase)\" &&\n> +\ttest_i18ngrep \"The --verify-signatures option does not work for --rebase.\" err\n> +'\n> +\n> +test_expect_success \"pull --rebase --no-verify-signatures\" '\n> +\tgit reset --hard before-rebase &&\n> +\tgit pull --rebase --no-verify-signatures . copy &&\n> +\ttest \"$(git rev-parse HEAD^)\" = \"$(git rev-parse copy)\" &&\n> +\ttest new = \"$(git show HEAD:file2)\"\n> +'\n> +\n>  # add a feature branch, keep-merge, that is merged into master, so the\n>  # test can try preserving the merge commit (or not) with various\n>  # --rebase flags/pull.rebase settings.\n"}]}