{"thread":{"id":"42366","subject":"[PATCH] pull: warn on --verify-signatures with --rebase","startedAt":"2016-05-18T10:18:27Z","lastAt":"2016-05-19T15:46:25Z","messageCount":4,"participants":["Alexander 'z33ky' Hirsch","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"286860","messageId":"20160518101827.GA14475@netblarch","threadId":"42366","inReplyTo":null,"subject":"[PATCH] pull: warn on --verify-signatures with --rebase","fromName":"Alexander 'z33ky' Hirsch","fromEmail":"1zeeky@gmail.com","sentAt":"2016-05-18T10:18:27Z","receivedAt":"2016-05-18T10:18:27Z","isPatch":true,"sender":{"key":"1zeeky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/289819?v=4"},"body":"Previously git-pull silently ignored the --verify-signatures option for\n--rebase.\n\nSigned-off-by: Alexander 'z33ky' Hirsch <1zeeky@gmail.com>\n---\n\nSorry it took so long for the update.\nI made git-pull warn instead or error and explained why \"the\n--verify-signatures option does not work for --rebase\" in the message.\n\n builtin/pull.c  |  2 ++\n t/t5520-pull.sh | 16 ++++++++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 1d7333c..0eafae7 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -815,6 +815,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\twarning(_(\"git-rebase does not support --verify-signatures\"));\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..d605450 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -341,6 +341,22 @@ 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 warns on --verify-signatures\" '\n+\tgit reset --hard before-rebase &&\n+\tgit pull --rebase --verify-signatures . copy 2>err &&\n+\ttest \"$(git rev-parse HEAD^)\" = \"$(git rev-parse copy)\" &&\n+\ttest new = \"$(git show HEAD:file2)\" &&\n+\ttest_i18ngrep \"git-rebase does not support --verify-signatures\" err\n+'\n+\n+test_expect_success \"pull --rebase does not warn on --no-verify-signatures\" '\n+\tgit reset --hard before-rebase &&\n+\tgit pull --rebase --no-verify-signatures . copy 2>err &&\n+\ttest \"$(git rev-parse HEAD^)\" = \"$(git rev-parse copy)\" &&\n+\ttest new = \"$(git show HEAD:file2)\" &&\n+\ttest_i18ngrep ! \"verify-signatures\" err\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.2\n"},{"id":"286889","messageId":"xmqq37pftks7.fsf@gitster.mtv.corp.google.com","threadId":"42366","inReplyTo":"20160518101827.GA14475@netblarch","subject":"Re: [PATCH] pull: warn on --verify-signatures with --rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-18T16:04:24Z","receivedAt":"2016-05-18T16:04:24Z","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 silently ignored the --verify-signatures option for\n> --rebase.\n\nMissing pieces information that would have made the patch more\ncomplete are answers to these questions:\n\n - Is that a bad thing?  Why?\n\n - Assuming it is a bad thing, what is the solution this patch\n   presents us?  Teach rebase about the option?  Error out the\n   request?  What is the reason why \"warn\" was chosen as the best\n   way forward?\n\n>  builtin/pull.c  |  2 ++\n>  t/t5520-pull.sh | 16 ++++++++++++++++\n>  2 files changed, 18 insertions(+)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 1d7333c..0eafae7 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -815,6 +815,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\nThe logic looks OK.  I would have written that long line as two\nlines, e.g.\n\n\tif (opt_verify_signatures &&\n            !strcmp(opt_verify_signatures, \"--verify-signatures\")\n\nthough.\n\n> +\t\twarning(_(\"git-rebase does not support --verify-signatures\"));\n\nIs this a good warning message?\n\nAs a casual reader, my reaction to this warning would be \"Does not\nsupport?  Then what did it do instead?  Did it refuse to integrate\nmy changes on top of what happened on the remote?\"\n\nSomething like\n\n    warning(_(\"ignored --verify-signatures as it is meaningless in rebase\"));\n\nmay convey what is going on better, in that it makes it clear that\nwe are not failing \"rebase\" and instead we are ignoring \"verify\".\n\nIt is way too long for the final version, though.  A more concise\nway to say the same thing needs to be found.\n\nThanks.\n"},{"id":"286972","messageId":"20160519100221.GB22257@netblarch","threadId":"42366","inReplyTo":"xmqq37pftks7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pull: warn on --verify-signatures with --rebase","fromName":"Alexander 'z33ky' Hirsch","fromEmail":"1zeeky@gmail.com","sentAt":"2016-05-19T10:02:21Z","receivedAt":"2016-05-19T10:02:21Z","isPatch":true,"sender":{"key":"1zeeky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/289819?v=4"},"body":"On Wed, May 18, 2016 at 09:04:24AM -0700, Junio C Hamano wrote:\n> > Previously git-pull silently ignored the --verify-signatures option for\n> > --rebase.\n> \n> Missing pieces information that would have made the patch more\n> complete are answers to these questions:\n> \n>  - Is that a bad thing?  Why?\n> \n>  - Assuming it is a bad thing, what is the solution this patch\n>    presents us?  Teach rebase about the option?  Error out the\n>    request?  What is the reason why \"warn\" was chosen as the best\n>    way forward?\n> \n\nIs the warning a solution \"for now\" and might this become an error\nshould a valid usecase not be found after a while?\n\n> >  builtin/pull.c  |  2 ++\n> >  t/t5520-pull.sh | 16 ++++++++++++++++\n> >  2 files changed, 18 insertions(+)\n> >\n> > diff --git a/builtin/pull.c b/builtin/pull.c\n> > index 1d7333c..0eafae7 100644\n> > --- a/builtin/pull.c\n> > +++ b/builtin/pull.c\n> > @@ -815,6 +815,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> \n> The logic looks OK.  I would have written that long line as two\n> lines, e.g.\n> \n> \tif (opt_verify_signatures &&\n>             !strcmp(opt_verify_signatures, \"--verify-signatures\")\n> \n> though.\n> \n\nI shall format it as per your suggestion in the next submission.\n\n> > +\t\twarning(_(\"git-rebase does not support --verify-signatures\"));\n> \n> Is this a good warning message?\n> \n> As a casual reader, my reaction to this warning would be \"Does not\n> support?  Then what did it do instead?  Did it refuse to integrate\n> my changes on top of what happened on the remote?\"\n> \n\nIndeed.\n\n> Something like\n> \n>     warning(_(\"ignored --verify-signatures as it is meaningless in rebase\"));\n> \n> may convey what is going on better, in that it makes it clear that\n> we are not failing \"rebase\" and instead we are ignoring \"verify\".\n> \n> It is way too long for the final version, though.  A more concise\n> way to say the same thing needs to be found.\n> \n\nWould \"ignoring --verify-signatures for rebase\" be sufficient? It does\nnot describe why it is ignored though.\n\n\nWith Regards,\nAlexander Hirsch\n"},{"id":"286992","messageId":"xmqqeg8ykq3y.fsf@gitster.mtv.corp.google.com","threadId":"42366","inReplyTo":"20160519100221.GB22257@netblarch","subject":"Re: [PATCH] pull: warn on --verify-signatures with --rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-19T15:46:25Z","receivedAt":"2016-05-19T15:46:25Z","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> Would \"ignoring --verify-signatures for rebase\" be sufficient? It does\n> not describe why it is ignored though.\n\nYeah, I agree that that would be sufficient.\n"}]}