{"thread":{"id":"33884","subject":"[PATCH v2] transport-helper: check if the dry-run is supported","startedAt":"2013-05-21T01:32:04Z","lastAt":"2013-05-22T03:07:06Z","messageCount":5,"participants":["Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"218031","messageId":"1369099924-13763-1-git-send-email-felipe.contreras@gmail.com","threadId":"33884","inReplyTo":null,"subject":"[PATCH v2] transport-helper: check if the dry-run is supported","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-21T01:32:04Z","receivedAt":"2013-05-21T01:32:04Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Certain remote-helpers (the ones with 'export') would try to push\nregardless.\n\nObviously this is not what the user wants.\n\nAlso, add a check for the 'dry-run' option, so remote-helpers can\nimplement it.\n\nSigned-off-by: Felipe Contreras \n---\n transport-helper.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 522d791..c8c39fc 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -789,6 +789,11 @@ static int push_refs_with_export(struct transport *transport,\n \tstruct string_list revlist_args = STRING_LIST_INIT_NODUP;\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tif (flags & TRANSPORT_PUSH_DRY_RUN) {\n+\t\tif (set_helper_option(transport, \"dry-run\", \"true\") != 0)\n+\t\t\tdie(\"helper %s does not support dry-run\", data->name);\n+\t}\n+\n \thelper = get_helper(transport);\n \n \twrite_constant(helper->in, \"export\\n\");\n-- \n1.8.3.rc3.1.gf11a2b7.dirty\n"},{"id":"218060","messageId":"7vli78p920.fsf@alter.siamese.dyndns.org","threadId":"33884","inReplyTo":"1369099924-13763-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2] transport-helper: check if the dry-run is supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-21T16:55:03Z","receivedAt":"2013-05-21T16:55:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Certain remote-helpers (the ones with 'export') would try to push\n> regardless.\n>\n> Obviously this is not what the user wants.\n>\n> Also, add a check for the 'dry-run' option, so remote-helpers can\n> implement it.\n\nThis sounds like a good thing to do. Perhaps the refspec mapping\ncan be handled the same way as a backend feature so that you do not\nhave to unconditionally disable it in the other patch.\n\nWill queue both but not for 1.8.3.\n\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> transport-helper.c | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 522d791..c8c39fc 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -789,6 +789,11 @@ static int push_refs_with_export(struct transport *transport,\n> \tstruct string_list revlist_args = STRING_LIST_INIT_NODUP;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \n> +\tif (flags & TRANSPORT_PUSH_DRY_RUN) {\n> +\t\tif (set_helper_option(transport, \"dry-run\", \"true\") != 0)\n> +\t\t\tdie(\"helper %s does not support dry-run\", data->name);\n> +\t}\n> +\n> \thelper = get_helper(transport);\n> \n> \twrite_constant(helper->in, \"export\\n\");\n"},{"id":"218096","messageId":"CAMP44s14ZBUYhxDkSwjoi+6JPx4whfQDhiq_4QfMteALsbuQ_Q@mail.gmail.com","threadId":"33884","inReplyTo":"7vli78p920.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] transport-helper: check if the dry-run is supported","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-21T21:14:12Z","receivedAt":"2013-05-21T21:14:12Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, May 21, 2013 at 11:55 AM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> Certain remote-helpers (the ones with 'export') would try to push\n>> regardless.\n>>\n>> Obviously this is not what the user wants.\n>>\n>> Also, add a check for the 'dry-run' option, so remote-helpers can\n>> implement it.\n>\n> This sounds like a good thing to do. Perhaps the refspec mapping\n> can be handled the same way as a backend feature so that you do not\n> have to unconditionally disable it in the other patch.\n\nWith my patch the remote helper doesn't need to know about the refspec\nhandling at all, it just works magically.\n\n-- \nFelipe Contreras\n"},{"id":"218119","messageId":"7vip2blu1k.fsf@alter.siamese.dyndns.org","threadId":"33884","inReplyTo":"CAMP44s14ZBUYhxDkSwjoi+6JPx4whfQDhiq_4QfMteALsbuQ_Q@mail.gmail.com","subject":"Re: [PATCH v2] transport-helper: check if the dry-run is supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-22T00:47:35Z","receivedAt":"2013-05-22T00:47:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Tue, May 21, 2013 at 11:55 AM, Junio C Hamano wrote:\n>\n>> This sounds like a good thing to do. Perhaps the refspec mapping\n>> can be handled the same way as a backend feature so that you do not\n>> have to unconditionally disable it in the other patch.\n>\n> With my patch the remote helper doesn't need to know about the refspec\n> handling at all, it just works magically.\n\nThe consumers of \"git fast-export\" do not need to know how to flip\nrefspecs when consuming output from \"git fast-export\", because you\ntaught \"git fast-export\" to do the mapping.\n\nBut doesn't that coin have a flip side? When somebody else (not\ngit) generates a fast-import stream, because these consumers are not\nprepared to flip refspecs, they cannot rename while importing. All\nthe producers have to be taught to do the ref mapping.\n\nI do not know if this matters in real life, and even if it did, in\nthe eventual ideal world, both importers and exporters would learn\nto do so. So I do not think what you did in your patch is a bad\ndesign in that sense. It is a half step in the right direction.\n\nI however found it somewhat ugly that the interface to specify set\nof refs to traverse history to find the set of objects to export\nstays the same as before, and the ref-mapping arguments are bolted\non to the machinery, without having any relationship between them.\nThe user is free to tell it to export only 'next', while telling it\nto map 'master' to 'trunk', for example.\n\nThis is an external interface that is exposed to any users of \"git\nfast-export\", so if we go that route, we would have to keep that\ninterface working forever, even when later somebody else wants to\nadd an interface that only requires ref-mapping arguments (and infer\nwhat is exported from the left hand side of the refspecs).\n\nThat part is what I found is less than ideal in the patch.\n"},{"id":"218140","messageId":"CAMP44s1Ne-+RZ5r-vNnvs+LhqL+QeMbfq9=Nmffkd9=A6RpNTw@mail.gmail.com","threadId":"33884","inReplyTo":"7vip2blu1k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] transport-helper: check if the dry-run is supported","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-22T03:07:06Z","receivedAt":"2013-05-22T03:07:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, May 21, 2013 at 7:47 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> On Tue, May 21, 2013 at 11:55 AM, Junio C Hamano wrote:\n>>\n>>> This sounds like a good thing to do. Perhaps the refspec mapping\n>>> can be handled the same way as a backend feature so that you do not\n>>> have to unconditionally disable it in the other patch.\n>>\n>> With my patch the remote helper doesn't need to know about the refspec\n>> handling at all, it just works magically.\n>\n> The consumers of \"git fast-export\" do not need to know how to flip\n> refspecs when consuming output from \"git fast-export\", because you\n> taught \"git fast-export\" to do the mapping.\n>\n> But doesn't that coin have a flip side? When somebody else (not\n> git) generates a fast-import stream, because these consumers are not\n> prepared to flip refspecs, they cannot rename while importing. All\n> the producers have to be taught to do the ref mapping.\n\nNot true. There can be an intermediary in between.\n\n> I do not know if this matters in real life, and even if it did, in\n> the eventual ideal world, both importers and exporters would learn\n> to do so.\n\nNo. Only one side *needs* to learn that.\n\n> So I do not think what you did in your patch is a bad\n> design in that sense. It is a half step in the right direction.\n\nWhat is the other step, and how would that benefit anyone?\n\n> I however found it somewhat ugly that the interface to specify set\n> of refs to traverse history to find the set of objects to export\n> stays the same as before, and the ref-mapping arguments are bolted\n> on to the machinery, without having any relationship between them.\n> The user is free to tell it to export only 'next', while telling it\n> to map 'master' to 'trunk', for example.\n>\n> This is an external interface that is exposed to any users of \"git\n> fast-export\", so if we go that route, we would have to keep that\n> interface working forever, even when later somebody else wants to\n> add an interface that only requires ref-mapping arguments (and infer\n> what is exported from the left hand side of the refspecs).\n\nNot true. We don't *have* to keep anything forever, we are free to\ndecide anything we want, and live with the consequences.\n\nIf a better approach is found, we can remove this interface in v2.0,\nor v3.0, or even v10.0.\n\nWhy are we shooting ourselves in the foot in the meantime? We already\nhave something that works perfectly fine.\n\nNow, I specifically asked if such an interface would make sense,\nbecause there are too many warts, and I did not receive a satisfactory\nanswer in my opinion. I will explore this interface once more, but I\nnever received any positive feedback from yours that we indeed want to\nteach 'fast-export' to parse refspecs, it's just that the interface to\ndo so was not ideal; you explicitly said you thought it made more\nsense on the other side; the receiver.\n\n-- \nFelipe Contreras\n"}]}