{"thread":{"id":"42067","subject":"[PATCH 1/2] mv submodule: respect ignore_errors for errors in submodule code","startedAt":"2016-04-18T23:41:22Z","lastAt":"2016-04-19T18:15:15Z","messageCount":7,"participants":["Stefan Beller","Junio C Hamano","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"283759","messageId":"1461022884-30819-1-git-send-email-sbeller@google.com","threadId":"42067","inReplyTo":null,"subject":"[PATCH 0/2] WAS: [PATCH] mv: allow moving nested submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-18T23:41:22Z","receivedAt":"2016-04-18T23:41:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"A single patch evolves into a series.\nThe second patch is essentially a resend with Junios suggestion squashed[1].\n\nThat patch alone doesn't quite fix it yet, as we need to make sure the\nsubmodule code respects the ignore_errors flag as instructed by the user.\nPatch 1 libifies the used functions from submodule.c and handles the errors\nappropriately.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/291810/focus=291829\n\nThanks,\nStefan\n\nStefan Beller (2):\n mv submodule: respect ignore_errors for errors in submodule code\n mv: allow moving nested submodules\n\n builtin/mv.c | 32 +++++++++++++++++++++++---------\n submodule.c | 33 ++++++++++++++++++++++++---------\n submodule.h | 6 ++++--\n t/t7001-mv.sh | 16 ++++++++++++++++\n 4 files changed, 67 insertions(+), 20 deletions(-)\n\n-- \n2.8.1.211.g24879d1\n"},{"id":"283758","messageId":"1461022884-30819-2-git-send-email-sbeller@google.com","threadId":"42067","inReplyTo":"1461022884-30819-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/2] mv submodule: respect ignore_errors for errors in submodule code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-18T23:41:23Z","receivedAt":"2016-04-18T23:41:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The error handling via passing around a strbuf is well exercised in the\nrefs code, so apply that pattern here, too.\n\nSigned-off-by: Stefan Beller \n---\n builtin/mv.c | 15 ++++++++++++---\n submodule.c | 33 ++++++++++++++++++++++++---------\n submodule.h | 6 ++++--\n 3 files changed, 40 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex aeae855..74516f4 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -247,6 +247,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t}\n \n \tfor (i = 0; i < argc; i++) {\n+\t\tstruct strbuf err = STRBUF_INIT;\n \t\tconst char *src = source[i], *dst = destination[i];\n \t\tenum update_mode mode = modes[i];\n \t\tint pos;\n@@ -256,9 +257,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\tif (rename(src, dst) < 0 && !ignore_errors)\n \t\t\t\tdie_errno(_(\"renaming '%s' failed\"), src);\n \t\t\tif (submodule_gitfile[i]) {\n-\t\t\t\tif (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)\n-\t\t\t\t\tconnect_work_tree_and_git_dir(dst, submodule_gitfile[i]);\n-\t\t\t\tif (!update_path_in_gitmodules(src, dst))\n+\t\t\t\tif ((submodule_gitfile[i] != SUBMODULE_WITH_GITDIR &&\n+\t\t\t\t connect_work_tree_and_git_dir(dst, submodule_gitfile[i], &err)) ||\n+\t\t\t\t update_path_in_gitmodules(src, dst, &err)) {\n+\t\t\t\t\tif (err.len) {\n+\t\t\t\t\t\tif (ignore_errors) {\n+\t\t\t\t\t\t\twarning(\"%s\", err.buf);\n+\t\t\t\t\t\t\tcontinue;\n+\t\t\t\t\t\t} else\n+\t\t\t\t\t\t\tdie(\"%s\", err.buf);\n+\t\t\t\t\t}\n+\t\t\t\t} else\n \t\t\t\t\tgitmodules_modified = 1;\n \t\t\t}\n \t\t}\ndiff --git a/submodule.c b/submodule.c\nindex 90825e1..ed18d34 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -51,7 +51,8 @@ int is_staging_gitmodules_ok(void)\n * .gitmodules file. Return 0 only if a .gitmodules file was found, a section\n * with the correct path= setting was found and we could update it.\n */\n-int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n+int update_path_in_gitmodules(const char *oldpath, const char *newpath,\n+\t\t\t struct strbuf *err)\n {\n \tstruct strbuf entry = STRBUF_INIT;\n \tconst struct submodule *submodule;\n@@ -59,8 +60,10 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n \tif (!file_exists(\".gitmodules\")) /* Do nothing without .gitmodules */\n \t\treturn -1;\n \n-\tif (gitmodules_is_unmerged)\n-\t\tdie(_(\"Cannot change unmerged .gitmodules, resolve merge conflicts first\"));\n+\tif (gitmodules_is_unmerged) {\n+\t\tstrbuf_addf(err, _(\"Cannot change unmerged .gitmodules, resolve merge conflicts first\"));\n+\t\treturn -1;\n+\t}\n \n \tsubmodule = submodule_from_path(null_sha1, oldpath);\n \tif (!submodule || !submodule->name) {\n@@ -1102,27 +1105,39 @@ int merge_submodule(unsigned char result[20], const char *path,\n }\n \n /* Update gitfile and core.worktree setting to connect work tree and git dir */\n-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n+int connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir,\n+\t\t\t\t struct strbuf *err)\n {\n+\tint ret = 0;\n \tstruct strbuf file_name = STRBUF_INIT;\n \tstruct strbuf rel_path = STRBUF_INIT;\n \tconst char *real_work_tree = xstrdup(real_path(work_tree));\n \n \t/* Update gitfile */\n \tstrbuf_addf(&file_name, \"%s/.git\", work_tree);\n-\twrite_file(file_name.buf, \"gitdir: %s\",\n-\t\t relative_path(git_dir, real_work_tree, &rel_path));\n+\tif (write_file_gently(file_name.buf, \"gitdir: %s\",\n+\t relative_path(git_dir, real_work_tree, &rel_path))) {\n+\t\tstrbuf_addf(err, _(\"could not write .git file (%s): %s\"),\n+\t\t\t\tfile_name.buf, strerror(errno));\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n \n \t/* Update core.worktree setting */\n \tstrbuf_reset(&file_name);\n \tstrbuf_addf(&file_name, \"%s/config\", git_dir);\n-\tgit_config_set_in_file(file_name.buf, \"core.worktree\",\n-\t\t\t relative_path(real_work_tree, git_dir,\n-\t\t\t\t\t &rel_path));\n+\tif (git_config_set_in_file_gently(file_name.buf, \"core.worktree\",\n+\t relative_path(real_work_tree, git_dir, &rel_path))) {\n+\t\tstrbuf_addf(err, _(\"could not set core.worktree in %s\"), file_name.buf);\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n \n+out:\n \tstrbuf_release(&file_name);\n \tstrbuf_release(&rel_path);\n \tfree((void *)real_work_tree);\n+\treturn ret;\n }\n \n int parallel_submodules(void)\ndiff --git a/submodule.h b/submodule.h\nindex 7ef3775..bf816e5 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -30,7 +30,8 @@ struct submodule_update_strategy {\n #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}\n \n int is_staging_gitmodules_ok(void);\n-int update_path_in_gitmodules(const char *oldpath, const char *newpath);\n+int update_path_in_gitmodules(const char *oldpath, const char *newpath,\n+\t\t\t struct strbuf *err);\n int remove_path_from_gitmodules(const char *path);\n void stage_updated_gitmodules(void);\n void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n@@ -58,7 +59,8 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c\n int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,\n \t\tstruct string_list *needs_pushing);\n int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n+int connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir,\n+\t\t\t\t struct strbuf *err);\n int parallel_submodules(void);\n \n #endif\n-- \n2.8.1.211.g24879d1\n"},{"id":"283760","messageId":"1461022884-30819-3-git-send-email-sbeller@google.com","threadId":"42067","inReplyTo":"1461022884-30819-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/2] mv: allow moving nested submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-18T23:41:24Z","receivedAt":"2016-04-18T23:41:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When directories are moved using `git mv` all files in the directory\nhave been just moved, but no further action was taken on them. This\nwas done by assigning the mode = WORKING_DIRECTORY to the files\ninside a moved directory.\n\nsubmodules however need to update their link to the git directory as\nwell as updates to the .gitmodules file. By removing the condition of\n`mode != INDEX` (the remaining modes are BOTH and WORKING_DIRECTORY) for\nthe required submodule actions, we perform these for submodules in a\nmoved directory.\n\nSigned-off-by: Stefan Beller \n---\n builtin/mv.c | 39 ++++++++++++++++++++++-----------------\n t/t7001-mv.sh | 16 ++++++++++++++++\n 2 files changed, 38 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 74516f4..2deb95b 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -253,23 +253,28 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\tint pos;\n \t\tif (show_only || verbose)\n \t\t\tprintf(_(\"Renaming %s to %s\\n\"), src, dst);\n-\t\tif (!show_only && mode != INDEX) {\n-\t\t\tif (rename(src, dst) < 0 && !ignore_errors)\n-\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), src);\n-\t\t\tif (submodule_gitfile[i]) {\n-\t\t\t\tif ((submodule_gitfile[i] != SUBMODULE_WITH_GITDIR &&\n-\t\t\t\t connect_work_tree_and_git_dir(dst, submodule_gitfile[i], &err)) ||\n-\t\t\t\t update_path_in_gitmodules(src, dst, &err)) {\n-\t\t\t\t\tif (err.len) {\n-\t\t\t\t\t\tif (ignore_errors) {\n-\t\t\t\t\t\t\twarning(\"%s\", err.buf);\n-\t\t\t\t\t\t\tcontinue;\n-\t\t\t\t\t\t} else\n-\t\t\t\t\t\t\tdie(\"%s\", err.buf);\n-\t\t\t\t\t}\n-\t\t\t\t} else\n-\t\t\t\t\tgitmodules_modified = 1;\n-\t\t\t}\n+\t\tif (show_only)\n+\t\t\tcontinue;\n+\t\tif (mode != INDEX &&\n+\t\t rename(src, dst) < 0) {\n+\t\t\tif (ignore_errors)\n+\t\t\t\tcontinue;\n+\t\t\tdie_errno(_(\"renaming '%s' failed\"), src);\n+\t\t}\n+\n+\t\tif (submodule_gitfile[i]) {\n+\t\t\tif ((submodule_gitfile[i] != SUBMODULE_WITH_GITDIR &&\n+\t\t\t connect_work_tree_and_git_dir(dst, submodule_gitfile[i], &err)) ||\n+\t\t\t update_path_in_gitmodules(src, dst, &err)) {\n+\t\t\t\tif (err.len) {\n+\t\t\t\t\tif (ignore_errors) {\n+\t\t\t\t\t\twarning(\"%s\", err.buf);\n+\t\t\t\t\t\tcontinue;\n+\t\t\t\t\t} else\n+\t\t\t\t\t\tdie(\"%s\", err.buf);\n+\t\t\t\t}\n+\t\t\t} else\n+\t\t\t\tgitmodules_modified = 1;\n \t\t}\n \n \t\tif (mode == WORKING_DIRECTORY)\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 4008fae..4a2570e 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -292,6 +292,9 @@ test_expect_success 'setup submodule' '\n \techo content >file &&\n \tgit add file &&\n \tgit commit -m \"added sub and file\" &&\n+\tmkdir -p deep/directory/hierachy &&\n+\tgit submodule add ./. deep/directory/hierachy/sub &&\n+\tgit commit -m \"added another submodule\" &&\n \tgit branch submodule\n '\n \n@@ -475,4 +478,17 @@ test_expect_success 'mv -k does not accidentally destroy submodules' '\n \tgit checkout .\n '\n \n+test_expect_success 'moving a submodule in nested directories' '\n+\t(\n+\t\tcd deep &&\n+\t\tgit mv directory ../ &&\n+\t\t# git status would fail if the update of linking git dir to\n+\t\t# work dir of the submodule failed.\n+\t\tgit status &&\n+\t\tgit config -f ../.gitmodules submodule.deep/directory/hierachy/sub.path >../actual &&\n+\t\techo \"directory/hierachy/sub\" >../expect\n+\t) &&\n+\ttest_cmp actual expect\n+'\n+\n test_done\n-- \n2.8.1.211.g24879d1\n"},{"id":"283762","messageId":"xmqqwpnu31qu.fsf@gitster.mtv.corp.google.com","threadId":"42067","inReplyTo":"1461022884-30819-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 0/2] WAS: [PATCH] mv: allow moving nested submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-19T00:01:45Z","receivedAt":"2016-04-19T00:01:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> A single patch evolves into a series.\n\nPower of code inspection to see bugs that are not reported, perhaps\n;-)?\n\nI wonder if we can come up with test cases to cover these potential\nissues that are addressed in [1/2]?\n\nThanks.\n"},{"id":"283771","messageId":"CA+P7+xrzH3gMWsCBmW9y40AFOEfUxMQR+s8O05ofMN09nF_Pkw@mail.gmail.com","threadId":"42067","inReplyTo":"1461022884-30819-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/2] mv: allow moving nested submodules","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-04-19T07:13:32Z","receivedAt":"2016-04-19T07:13:32Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Apr 18, 2016 at 4:41 PM, Stefan Beller wrote:\n> When directories are moved using `git mv` all files in the directory\n> have been just moved, but no further action was taken on them. This\n> was done by assigning the mode = WORKING_DIRECTORY to the files\n> inside a moved directory.\n>\n> submodules however need to update their link to the git directory as\n> well as updates to the .gitmodules file. By removing the condition of\n> `mode != INDEX` (the remaining modes are BOTH and WORKING_DIRECTORY) for\n> the required submodule actions, we perform these for submodules in a\n> moved directory.\n>\n> Signed-off-by: Stefan Beller \n\n\nThe patch looks good to me. I've marked some comments that I thought\nthrough while reviewing but it looks correct.\n\n> ---\n> builtin/mv.c | 39 ++++++++++++++++++++++-----------------\n> t/t7001-mv.sh | 16 ++++++++++++++++\n> 2 files changed, 38 insertions(+), 17 deletions(-)\n>\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index 74516f4..2deb95b 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -253,23 +253,28 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> int pos;\n> if (show_only || verbose)\n> printf(_(\"Renaming %s to %s\\n\"), src, dst);\n> - if (!show_only && mode != INDEX) {\n> - if (rename(src, dst) < 0 && !ignore_errors)\n> - die_errno(_(\"renaming '%s' failed\"), src);\n> - if (submodule_gitfile[i]) {\n> - if ((submodule_gitfile[i] != SUBMODULE_WITH_GITDIR &&\n> - connect_work_tree_and_git_dir(dst, submodule_gitfile[i], &err)) ||\n> - update_path_in_gitmodules(src, dst, &err)) {\n> - if (err.len) {\n> - if (ignore_errors) {\n> - warning(\"%s\", err.buf);\n> - continue;\n> - } else\n> - die(\"%s\", err.buf);\n> - }\n> - } else\n> - gitmodules_modified = 1;\n> - }\n> + if (show_only)\n> + continue;\n\nSo here, we skip the item after displaying when we're in show only\nmode. That seems correct.\n\n> + if (mode != INDEX &&\n> + rename(src, dst) < 0) {\n> + if (ignore_errors)\n> + continue;\n> + die_errno(_(\"renaming '%s' failed\"), src);\n> + }\n\nThen when the mode isn't INDEX, we attempt the rename.\n\n> +\n> + if (submodule_gitfile[i]) {\n> + if ((submodule_gitfile[i] != SUBMODULE_WITH_GITDIR &&\n> + connect_work_tree_and_git_dir(dst, submodule_gitfile[i], &err)) ||\n> + update_path_in_gitmodules(src, dst, &err)) {\n> + if (err.len) {\n> + if (ignore_errors) {\n> + warning(\"%s\", err.buf);\n> + continue;\n> + } else\n> + die(\"%s\", err.buf);\n> + }\n> + } else\n> + gitmodules_modified = 1;\n> }\n\nFinally for all modes, we perform the steps for submodules. That makes\nsense to me.\n>\n\nThis version results in a much larger diff, but I think the resulting\ncode is much easier to follow. The use of the continue allows us to\ndrop a layer of indentation making the remaining code a bit easier on\nthe eyes. The patch description doesn't at first glance match the code\nchange, since now it's a much larger chunk of moved code. However on\ninspection, I don't think anything needs to change, as it's just the\nconversion to if (show_only) { continue; }\n\nReviewed-by: Jacob Keller \n\nThanks,\nJake\n"},{"id":"283804","messageId":"CAGZ79kYEjRSpet-5EQibJJ-57EkefQZm+zZr-d1K8Y9-=-fAmQ@mail.gmail.com","threadId":"42067","inReplyTo":"xmqqwpnu31qu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] WAS: [PATCH] mv: allow moving nested submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-19T16:56:07Z","receivedAt":"2016-04-19T16:56:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Apr 18, 2016 at 5:01 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> A single patch evolves into a series.\n>\n> Power of code inspection to see bugs that are not reported, perhaps\n> ;-)?\n>\n> I wonder if we can come up with test cases to cover these potential\n> issues that are addressed in [1/2]?\n\nI think we can do a chmod a-w on the .git file and then updating that file\nfails. (renaming is fine, but putting the new, possibly different link\nto the git dir\nin there fails.)\n\nI do not quite know what we'd expect from a \"git mv -k\" on that.\nIt is documented as \"skip move/rename errors\", so maybe we need\nto undo the rename to actually skip it once an error occurs?\n\nThat said I think I know how to write a test for that, but I am unsure\nif patch 1 is a good idea.\n\nThanks,\nStefan\n\n\n>\n> Thanks.\n>\n"},{"id":"283821","messageId":"xmqqlh491n4c.fsf@gitster.mtv.corp.google.com","threadId":"42067","inReplyTo":"CAGZ79kYEjRSpet-5EQibJJ-57EkefQZm+zZr-d1K8Y9-=-fAmQ@mail.gmail.com","subject":"Re: [PATCH 0/2] WAS: [PATCH] mv: allow moving nested submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-19T18:15:15Z","receivedAt":"2016-04-19T18:15:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> ..., but I am unsure\n> if patch 1 is a good idea.\n\nThen let's postpone it for now. I too would like to hear opinion\nfrom other submodule folks, especially Jens, for what 1/2 does\nbefore committing us to the course.\n\nCan you do only the 2/2 on top of maint (or maint-2.6) for now then?\n"}]}