{"thread":{"id":"35617","subject":"[PATCH] git-submodule.sh: Support 'checkout' as a valid update command","startedAt":"2014-01-06T18:58:46Z","lastAt":"2014-01-07T17:42:02Z","messageCount":6,"participants":["Francesco Pretto","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"232750","messageId":"1389034726-8744-1-git-send-email-ceztko@gmail.com","threadId":"35617","inReplyTo":null,"subject":"[PATCH] git-submodule.sh: Support 'checkout' as a valid update command","fromName":"Francesco Pretto","fromEmail":"ceztko@gmail.com","sentAt":"2014-01-06T18:58:46Z","receivedAt":"2014-01-06T18:58:46Z","isPatch":true,"sender":{"key":"ceztko@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3037449?v=4"},"body":"According to \"Documentation/gitmodules.txt\", 'checkout' is a valid\n'submodule.<name>.update' command. Also \"git-submodule.sh\" refers to\nit and processes it correctly. Reflecting commit 'ac1fbb' to support\nthis syntax and also validate property values during 'update' command,\nissuing an error if the value found is unknown.\n\nSigned-off-by: Francesco Pretto <ceztko@gmail.com>\n---\n git-submodule.sh | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 2677f2e..4a30087 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -622,7 +622,7 @@ cmd_init()\n \t\t   test -z \"$(git config submodule.\"$name\".update)\"\n \t\tthen\n \t\t\tcase \"$upd\" in\n-\t\t\trebase | merge | none)\n+\t\t\tcheckout | rebase | merge | none)\n \t\t\t\t;; # known modes of updating\n \t\t\t*)\n \t\t\t\techo >&2 \"warning: unknown update mode '$upd' suggested for submodule '$name'\"\n@@ -805,6 +805,17 @@ cmd_update()\n \t\t\tupdate_module=$update\n \t\telse\n \t\t\tupdate_module=$(git config submodule.\"$name\".update)\n+\t\t\tcase \"$update_module\" in\n+\t\t\t'')\n+\t\t\t\t;; # Unset update mode\n+\t\t\tcheckout | rebase | merge | none)\n+\t\t\t\t;; # Known update modes\n+\t\t\t!*)\n+\t\t\t\t;; # Custom update command\n+\t\t\t*)\n+\t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule '$name'\")\"\n+\t\t\t\t;;\n+\t\t\tesac\n \t\tfi\n \n \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n-- \n1.8.5.2.229.g4448466.dirty\n"},{"id":"232788","messageId":"xmqqtxdgfz8a.fsf@gitster.dls.corp.google.com","threadId":"35617","inReplyTo":"1389034726-8744-1-git-send-email-ceztko@gmail.com","subject":"Re: [PATCH] git-submodule.sh: Support 'checkout' as a valid update command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-06T23:48:53Z","receivedAt":"2014-01-06T23:48:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Francesco Pretto <ceztko@gmail.com> writes:\n\n> According to \"Documentation/gitmodules.txt\", 'checkout' is a valid\n> 'submodule.<name>.update' command.\n\nAs you can see in the surrounding text, we call the value of\nsubmodule.*.update a \"mode\", not a command.\n\n> Also \"git-submodule.sh\" refers to\n> it and processes it correctly.\n\nThis present tense puzzles me.  If it already refers to checkout and\nhandles it correctly is there anything that needs to be done?  Or\ndid you mean \"it should refer to and process it but it doesn't, so\nmake it so?\"\n\n> Reflecting commit 'ac1fbb' to support\n> this syntax and also validate property values during 'update' command,\n> issuing an error if the value found is unknown.\n\nSorry, but -ECANNOTPARSE.\n"},{"id":"232789","messageId":"CALas-ijrD1VnyUcr2yQw_1Je4K3eEdXtxqDNDKdGPZE=1=Nm3A@mail.gmail.com","threadId":"35617","inReplyTo":"xmqqtxdgfz8a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-submodule.sh: Support 'checkout' as a valid update command","fromName":"Francesco Pretto","fromEmail":"ceztko@gmail.com","sentAt":"2014-01-07T00:05:04Z","receivedAt":"2014-01-07T00:05:04Z","isPatch":true,"sender":{"key":"ceztko@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3037449?v=4"},"body":"2014/1/7 Junio C Hamano <gitster@pobox.com>:\n> Francesco Pretto <ceztko@gmail.com> writes:\n>\n>> According to \"Documentation/gitmodules.txt\", 'checkout' is a valid\n>> 'submodule.<name>.update' command.\n>\n> As you can see in the surrounding text, we call the value of\n> submodule.*.update a \"mode\", not a command.\n>\n\nOk.\n\n>> Also \"git-submodule.sh\" refers to\n>> it and processes it correctly.\n>\n> This present tense puzzles me.  If it already refers to checkout and\n> handles it correctly is there anything that needs to be done?  Or\n> did you mean \"it should refer to and process it but it doesn't, so\n> make it so?\"\n>\n\nLike you said, \"it already refers to checkout and handles it\ncorrectly\". I think the use of the simple present tense here is\ncorrect: it's a fact. Feel free to advice another wording if you\nprefer.\n\n>> Reflecting commit 'ac1fbb' to support\n>> this syntax and also validate property values during 'update' command,\n>> issuing an error if the value found is unknown.\n>\n> Sorry, but -ECANNOTPARSE.\n\nNot sure what's wrong here, can you explain why it's failing? I'm\nusing git-format-patch/git-send-email with default settings. Also, if\nyou can edit and keep the sign-off (I'm not familiar with the\nmailing-list maintainer workflow, sorry), feel free to do it.\n\nThanks\n"},{"id":"232793","messageId":"CALas-ijKQgDQsoNd0yrFOntnV8cuQGz8KL2xNWtYqGxXLH9q=w@mail.gmail.com","threadId":"35617","inReplyTo":"xmqqtxdgfz8a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-submodule.sh: Support 'checkout' as a valid update command","fromName":"Francesco Pretto","fromEmail":"ceztko@gmail.com","sentAt":"2014-01-07T01:16:28Z","receivedAt":"2014-01-07T01:16:28Z","isPatch":true,"sender":{"key":"ceztko@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3037449?v=4"},"body":"2014/1/7 Junio C Hamano <gitster@pobox.com>:\n> Sorry, but -ECANNOTPARSE.\n>\n\nA bird told me what -ECANNOTPARSE means. Tell me if this comment sounds better:\n\nAccording to \"Documentation/gitmodules.txt\", 'checkout' is a valid\n'submodule.<name>.update' mode. Also \"git-submodule.sh\" already refers\nto it and handles it correctly. Fix cmd_init() to also accept 'checkout' as\nvalid update mode and add a similar validation in cmd_update(), issuing\nan error if the value read is unknown.\n"},{"id":"232808","messageId":"xmqqlhyrg49c.fsf@gitster.dls.corp.google.com","threadId":"35617","inReplyTo":"CALas-ijrD1VnyUcr2yQw_1Je4K3eEdXtxqDNDKdGPZE=1=Nm3A@mail.gmail.com","subject":"Re: [PATCH] git-submodule.sh: Support 'checkout' as a valid update command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-07T16:12:31Z","receivedAt":"2014-01-07T16:12:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Francesco Pretto <ceztko@gmail.com> writes:\n\n> Like you said, \"it already refers to checkout and handles it\n> correctly\". I think the use of the simple present tense here is\n> correct: it's a fact. Feel free to advice another wording if you\n> prefer.\n\nIt is not about preference but what we want to convey to the\nreaders.  When you start the sentence with \"Oh, it already works\ncorrectly\", the readers need to see this sentence finished: \"It\nalready works, it is handled correctly, but we change the code\nnevertheless because ...?\".\n\nHere is my attempt to fill that \"because ...\" part:\n\n\tSubject: git-submodule.sh: 'checkout' is a valid update mode\n\n\t'checkout' is documented as one of the valid values for\n\t'submodule.<name>.update' variable, and in a repository with\n\tthe variable set to 'checkout', \"git submodule update\"\n\tcommand do update using the 'checkout' mode.\n\n\tHowever, it has been an accident that the implementation\n\tworks this way; any unknown value would trigger the same\n\tcodepath and update using the 'checkout' mode.\n\n        Tighten the codepath and explicitly list 'checkout' as one\n\tof the known update modes, and error out when an unknown\n\tupdate mode is used.\n\n\tAlso, teach the codepath that initializes the configuration\n\tvariable from in-tree .gitmodules that 'checkout' is one of\n\tthe valid values---the code since ac1fbbda (submodule: do\n\tnot copy unknown update mode from .gitmodules, 2013-12-02)\n\tused to treat the value 'checkout' as unknown and mapped it\n\tto 'none', which made little sense.\n"},{"id":"232816","messageId":"CALas-igswi9ro=j1r3YJ9wLd2ikp9fa6M-3ZSYQZXKmaTfGOWw@mail.gmail.com","threadId":"35617","inReplyTo":"xmqqlhyrg49c.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-submodule.sh: Support 'checkout' as a valid update command","fromName":"Francesco Pretto","fromEmail":"ceztko@gmail.com","sentAt":"2014-01-07T17:42:02Z","receivedAt":"2014-01-07T17:42:02Z","isPatch":true,"sender":{"key":"ceztko@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3037449?v=4"},"body":"2014/1/7 Junio C Hamano <gitster@pobox.com>:\n> It is not about preference but what we want to convey to the\n> readers.  When you start the sentence with \"Oh, it already works\n> correctly\", the readers need to see this sentence finished: \"It\n> already works, it is handled correctly, but we change the code\n> nevertheless because ...?\".\n>\n> Here is my attempt to fill that \"because ...\" part:\n>\n>         Subject: git-submodule.sh: 'checkout' is a valid update mode\n>\n>         'checkout' is documented as one of the valid values for\n>         'submodule.<name>.update' variable, and in a repository with\n>         the variable set to 'checkout', \"git submodule update\"\n>         command do update using the 'checkout' mode.\n>\n>         However, it has been an accident that the implementation\n>         works this way; any unknown value would trigger the same\n>         codepath and update using the 'checkout' mode.\n>\n>         Tighten the codepath and explicitly list 'checkout' as one\n>         of the known update modes, and error out when an unknown\n>         update mode is used.\n>\n>         Also, teach the codepath that initializes the configuration\n>         variable from in-tree .gitmodules that 'checkout' is one of\n>         the valid values---the code since ac1fbbda (submodule: do\n>         not copy unknown update mode from .gitmodules, 2013-12-02)\n>         used to treat the value 'checkout' as unknown and mapped it\n>         to 'none', which made little sense.\n>\n\nI wouldn't be able to explain the change better than your description.\nAlso, I was under the improper assumption that the change was obvious.\nThank you very much for the amended patch description.\n\nCheers,\nFrancesco\n"}]}