{"thread":{"id":"46941","subject":"[PATCH] submodule: spell out API of submodule_move_head","startedAt":"2017-10-09T22:06:27Z","lastAt":"2017-10-10T01:06:41Z","messageCount":7,"participants":["Stefan Beller","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"330056","messageId":"20171009220615.27541-1-sbeller@google.com","threadId":"46941","inReplyTo":null,"subject":"[PATCH] submodule: spell out API of submodule_move_head","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-09T22:06:15Z","receivedAt":"2017-10-09T22:06:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This way users of this function do not need to read the implementation to\nknow what it will do.\n\nSigned-off-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n submodule.c | 5 -----\n submodule.h | 18 ++++++++++++++++++\n 2 files changed, 18 insertions(+), 5 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex c1edc91a13..d12e4ea96c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1507,11 +1507,6 @@ static void submodule_reset_index(const char *path)\n \t\tdie(\"could not reset submodule index\");\n }\n \n-/**\n- * Moves a submodule at a given path from a given head to another new head.\n- * For edge cases (a submodule coming into existence or removing a submodule)\n- * pass NULL for old or new respectively.\n- */\n int submodule_move_head(const char *path,\n \t\t\t const char *old,\n \t\t\t const char *new,\ndiff --git a/submodule.h b/submodule.h\nindex f0da0277a4..3c15977be9 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -111,6 +111,24 @@ extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git\n */\n int submodule_to_gitdir(struct strbuf *buf, const char *submodule);\n \n+/**\n+ * Move the HEAD and content of the active submodule at 'path' from object id\n+ * 'old' to 'new'.\n+ *\n+ * Updates the submodule at 'path' and files in its work tree to commit\n+ * 'new'. The commit previously pointed to by the submodule is named by\n+ * 'old'. This updates the submodule's HEAD, index, and work tree but\n+ * does not touch its gitlink entry in the superproject.\n+ *\n+ * If the submodule did not previously exist, then 'old' should be NULL.\n+ * Similarly, if the submodule is to be removed, 'new' should be NULL.\n+ *\n+ * If updating the submodule would cause local changes to be overwritten,\n+ * then instead of updating the submodule, this function prints an error\n+ * message and returns -1.\n+ *\n+ * If the submodule is not active, this does nothing and returns 0.\n+ */\n #define SUBMODULE_MOVE_HEAD_DRY_RUN (1<<0)\n #define SUBMODULE_MOVE_HEAD_FORCE (1<<1)\n extern int submodule_move_head(const char *path,\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"330057","messageId":"20171009222224.GD19555@aiede.mtv.corp.google.com","threadId":"46941","inReplyTo":"20171009220615.27541-1-sbeller@google.com","subject":"Re: [PATCH] submodule: spell out API of submodule_move_head","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-09T22:22:24Z","receivedAt":"2017-10-09T22:22:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> This way users of this function do not need to read the implementation to\n> know what it will do.\n>\n> Signed-off-by: Jonathan Nieder \n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 5 -----\n> submodule.h | 18 ++++++++++++++++++\n> 2 files changed, 18 insertions(+), 5 deletions(-)\n\nLooks good (well, you'd expect me to think that :)). Thanks.\n"},{"id":"330071","messageId":"xmqqtvz7rh68.fsf@gitster.mtv.corp.google.com","threadId":"46941","inReplyTo":"20171009220615.27541-1-sbeller@google.com","subject":"Re: [PATCH] submodule: spell out API of submodule_move_head","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T00:16:15Z","receivedAt":"2017-10-10T00:16:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +/**\n> + * Move the HEAD and content of the active submodule at 'path' from object id\n> + * 'old' to 'new'.\n> + *\n> + * Updates the submodule at 'path' and files in its work tree to commit\n> + * 'new'. The commit previously pointed to by the submodule is named by\n> + * 'old'. This updates the submodule's HEAD, index, and work tree but\n> + * does not touch its gitlink entry in the superproject.\n> + *\n> + * If the submodule did not previously exist, then 'old' should be NULL.\n> + * Similarly, if the submodule is to be removed, 'new' should be NULL.\n> + *\n> + * If updating the submodule would cause local changes to be overwritten,\n> + * then instead of updating the submodule, this function prints an error\n> + * message and returns -1.\n\nThis is not a new issue (the removed comment did not mention this at\nall), but is it correct to say that updates to \"index and work tree\"\nwas as if we did \"git -C $path checkout new\" (and of course, HEAD in\nthe $path submodule must be at 'old')?\n\nWhat should happen if 'old' does not match reality (i.e. old is NULL\nbut HEAD does point at some commit, old and HEAD are different,\netc.)? Should the call be aborted?\n\n> + * If the submodule is not active, this does nothing and returns 0.\n> + */\n> #define SUBMODULE_MOVE_HEAD_DRY_RUN (1<<0)\n> #define SUBMODULE_MOVE_HEAD_FORCE (1<<1)\n> extern int submodule_move_head(const char *path,\n"},{"id":"330073","messageId":"20171010002731.GM19555@aiede.mtv.corp.google.com","threadId":"46941","inReplyTo":"xmqqtvz7rh68.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule: spell out API of submodule_move_head","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-10T00:27:31Z","receivedAt":"2017-10-10T00:27:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Stefan Beller writes:\n\n>> +/**\n>> + * Move the HEAD and content of the active submodule at 'path' from object id\n>> + * 'old' to 'new'.\n>> + *\n>> + * Updates the submodule at 'path' and files in its work tree to commit\n>> + * 'new'. The commit previously pointed to by the submodule is named by\n>> + * 'old'. This updates the submodule's HEAD, index, and work tree but\n>> + * does not touch its gitlink entry in the superproject.\n>> + *\n>> + * If the submodule did not previously exist, then 'old' should be NULL.\n>> + * Similarly, if the submodule is to be removed, 'new' should be NULL.\n>> + *\n>> + * If updating the submodule would cause local changes to be overwritten,\n>> + * then instead of updating the submodule, this function prints an error\n>> + * message and returns -1.\n>\n> This is not a new issue (the removed comment did not mention this at\n> all), but is it correct to say that updates to \"index and work tree\"\n> was as if we did \"git -C $path checkout new\" (and of course, HEAD in\n> the $path submodule must be at 'old')?\n\nI don't understand the question. This comment doesn't say it's like\n\"git checkout\" --- are you saying it should?\n\nThe function is more like \"git read-tree -m -u\" (or --reset when\nSUBMODULE_MOVE_HEAD_FORCE is passed) than like \"git checkout\".\nPerhaps what you are hinting at is that read-tree --recurse-submodules\nis not necessarily the right primitive to implement \"git checkout\"\nwith. But that's a separate issue from documenting the current\nbehavior of the function.\n\n> What should happen if 'old' does not match reality (i.e. old is NULL\n> but HEAD does point at some commit, old and HEAD are different,\n> etc.)? Should the call be aborted?\n\nNo.\n\nThanks,\nJonathan\n\n>> + * If the submodule is not active, this does nothing and returns 0.\n>> + */\n>> #define SUBMODULE_MOVE_HEAD_DRY_RUN (1<<0)\n>> #define SUBMODULE_MOVE_HEAD_FORCE (1<<1)\n>> extern int submodule_move_head(const char *path,\n"},{"id":"330074","messageId":"xmqqmv4zrfye.fsf@gitster.mtv.corp.google.com","threadId":"46941","inReplyTo":"20171010002731.GM19555@aiede.mtv.corp.google.com","subject":"Re: [PATCH] submodule: spell out API of submodule_move_head","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T00:42:33Z","receivedAt":"2017-10-10T00:42:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> This is not a new issue (the removed comment did not mention this at\n>> all), but is it correct to say that updates to \"index and work tree\"\n>> was as if we did \"git -C $path checkout new\" (and of course, HEAD in\n>> the $path submodule must be at 'old')?\n>\n> I don't understand the question. This comment doesn't say it's like\n> \"git checkout\" --- are you saying it should?\n\nNo, I am pointing out that this comment does not say what it's like\nclearly enough. s/is it correct/am I correct/ would have been less\nprone to get misunderstood, I guess.\n\n> The function is more like \"git read-tree -m -u\" (or --reset when\n> SUBMODULE_MOVE_HEAD_FORCE is passed) than like \"git checkout\".\n\nIf it behaves like two-tree \"read-tree -m -u\", I'd say that the best\nexplanation an average developer would understand is that the update\ndone to \"index and work tree\" is like using 'git checkout' to switch\nto the branch whose tip is 'new'.\n\n> Perhaps what you are hinting at is that read-tree --recurse-submodules\n> is not necessarily the right primitive to implement \"git checkout\"\n> with. But that's a separate issue from documenting the current\n> behavior of the function.\n\nNo, you are way too ahead in this discussion. I was merely saying\nthat what the comment says was unclear and does not allow me to get\nthat far due to lack of clear information ;-).\n\n>> What should happen if 'old' does not match reality (i.e. old is NULL\n>> but HEAD does point at some commit, old and HEAD are different,\n>> etc.)? Should the call be aborted?\n>\n> No.\n\n... and that is because?\n\nWhen does it make sense to do a two-tree \"read-tree -m -u\" giving\nthe 'old' that is very different from the real 'old' tree-ish that\ncorresponds to where your index started at?\n\n"},{"id":"330076","messageId":"20171010005520.GN19555@aiede.mtv.corp.google.com","threadId":"46941","inReplyTo":"xmqqmv4zrfye.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule: spell out API of submodule_move_head","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-10T00:55:20Z","receivedAt":"2017-10-10T00:55:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>> Junio C Hamano wrote:\n\n>>> This is not a new issue (the removed comment did not mention this at\n>>> all), but is it correct to say that updates to \"index and work tree\"\n>>> was as if we did \"git -C $path checkout new\" (and of course, HEAD in\n>>> the $path submodule must be at 'old')?\n>>\n>> I don't understand the question. This comment doesn't say it's like\n>> \"git checkout\" --- are you saying it should?\n>\n> No, I am pointing out that this comment does not say what it's like\n> clearly enough. s/is it correct/am I correct/ would have been less\n> prone to get misunderstood, I guess.\n\nNo problem. I think a word or two about how it's like read-tree\nin the docstring could be an improvement.\n\n> If it behaves like two-tree \"read-tree -m -u\", I'd say that the best\n> explanation an average developer would understand is that the update\n> done to \"index and work tree\" is like using 'git checkout' to switch\n> to the branch whose tip is 'new'.\n\nIf it says it's like \"git checkout\", then I fear that can just lead to\nmore confusion, since \"git checkout\" does a number of things (e.g.\nupdating the HEAD symref) that this function does not do.\n\nIt could say that it's like \"git reset --keep\", I suppose.\n\n[...]\n>>> What should happen if 'old' does not match reality (i.e. old is NULL\n>>> but HEAD does point at some commit, old and HEAD are different,\n>>> etc.)? Should the call be aborted?\n>>\n>> No.\n>\n> ... and that is because?\n>\n> When does it make sense to do a two-tree \"read-tree -m -u\" giving\n> the 'old' that is very different from the real 'old' tree-ish that\n> corresponds to where your index started at?\n\nBecause that is not the purpose of the function.\n\nThe caller is responsible for setting 'old' appropriately. A word or\ntwo in that direction would not be a terrible thing.\n\nAll that said, I want this function to go away completely. :)\nDocumenting how it currently behaves is just a good way to understand\nwhat is happening when doing so.\n\nThanks,\nJonathan\n"},{"id":"330078","messageId":"xmqqinfnreud.fsf@gitster.mtv.corp.google.com","threadId":"46941","inReplyTo":"20171010005520.GN19555@aiede.mtv.corp.google.com","subject":"Re: [PATCH] submodule: spell out API of submodule_move_head","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T01:06:34Z","receivedAt":"2017-10-10T01:06:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> All that said, I want this function to go away completely. :)\n\nOh, if you are planning to replace it with something else, then I\nwouldn't bother wasting my time trying to understand the updated\ncomment, or making it more understandable by others.\n\n"}]}