{"thread":{"id":"14489","subject":"[RFC][PATCH 0/7] Submodule support in git mv, git rm","startedAt":"2008-07-16T19:11:02Z","lastAt":"2008-08-08T23:45:26Z","messageCount":56,"participants":["Petr Baudis","Johannes Schindelin","Junio C Hamano","SZEDER Gábor","Linus Torvalds","david@lang.hm","Dmitry Potapov"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"83556","messageId":"20080716190753.19772.93357.stgit@localhost","threadId":"14489","inReplyTo":null,"subject":"[RFC][PATCH 0/7] Submodule support in git mv, git rm","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-16T19:11:02Z","receivedAt":"2008-07-16T19:11:02Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"The following series implements submodule support in git mv and git rm,\nplus enhancing the submodules testsuite a bit. I'd appreciate comments,\nespecially on the git mv change, since the index_path_src_sha1 is\nreally a horrible hack.\n\nThe pinnacle of this series was supposed to be merge-recursive support\nfor submodule-somethingelese conflicts, however that seems a bit more\ncomplicated than I expected, so I decided to first send the rest for\na review.\n\n---\n\nPetr Baudis (7):\n      t7403: Submodule git mv, git rm testsuite\n      git rm: Support for removing submodules\n      git mv: Support moving submodules\n      submodule.*: Introduce simple C interface for submodule lookup by path\n      git submodule add: Fix naming clash handling\n      t7400: Add short \"git submodule add\" testsuite\n      git-mv: Remove dead code branch\n\n\n Documentation/git-rm.txt   |    6 +\n Makefile                   |    2 \n builtin-mv.c               |   67 ++++++++++--\n builtin-rm.c               |   65 ++++++++++--\n cache.h                    |    2 \n git-submodule.sh           |   15 ++-\n sha1_file.c                |   10 ++\n submodule.c                |   50 +++++++++\n submodule.h                |    8 +\n t/t7400-submodule-basic.sh |   39 +++++++\n t/t7403-submodule-mvrm.sh  |  242 ++++++++++++++++++++++++++++++++++++++++++++\n 11 files changed, 476 insertions(+), 30 deletions(-)\n create mode 100644 submodule.c\n create mode 100644 submodule.h\n create mode 100755 t/t7403-submodule-mvrm.sh\n"},{"id":"83558","messageId":"20080716191107.19772.90413.stgit@localhost","threadId":"14489","inReplyTo":"20080716190753.19772.93357.stgit@localhost","subject":"[PATCH 1/7] git-mv: Remove dead code branch","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-16T19:11:08Z","receivedAt":"2008-07-16T19:11:08Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"The path list builder had a branch for the case the source is not in index, but\nthis can happen only if the source was a directory. However, in that case we\nhave already expanded the list to the directory contents and set mode\nto WORKING_DIRECTORY, which is tested earlier.\n\nThe patch removes the superfluous branch and adds an assert() instead. git-mv\ntestsuite still passes.\n\nSigned-off-by: Petr Baudis <pasky@suse.cz>\n---\n\n builtin-mv.c |   16 +++++++---------\n 1 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex 5530e11..158a83d 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -227,15 +227,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\tif (mode == WORKING_DIRECTORY)\n \t\t\tcontinue;\n \n-\t\tif (cache_name_pos(src, strlen(src)) >= 0) {\n-\t\t\tpath_list_insert(src, &deleted);\n-\n-\t\t\t/* destination can be a directory with 1 file inside */\n-\t\t\tif (path_list_has_path(&overwritten, dst))\n-\t\t\t\tpath_list_insert(dst, &changed);\n-\t\t\telse\n-\t\t\t\tpath_list_insert(dst, &added);\n-\t\t} else\n+\t\tassert(cache_name_pos(src, strlen(src)) >= 0);\n+\n+\t\tpath_list_insert(src, &deleted);\n+\t\t/* destination can be a directory with 1 file inside */\n+\t\tif (path_list_has_path(&overwritten, dst))\n+\t\t\tpath_list_insert(dst, &changed);\n+\t\telse\n \t\t\tpath_list_insert(dst, &added);\n \t}\n \n"},{"id":"83557","messageId":"20080716191113.19772.96571.stgit@localhost","threadId":"14489","inReplyTo":"20080716190753.19772.93357.stgit@localhost","subject":"[PATCH 2/7] t7400: Add short \"git submodule add\" testsuite","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-16T19:11:13Z","receivedAt":"2008-07-16T19:11:13Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"This patch introduces basic tests for\n\n\tgit submodule add\n\ncovering the basic functionality and the -b parameter.\n\nA trivial update --init test fix freeloads on this commit as well.\n\nSigned-off-by: Petr Baudis <pasky@suse.cz>\n---\n\n t/t7400-submodule-basic.sh |   28 +++++++++++++++++++++++++++-\n 1 files changed, 27 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 6c7b902..ab5eb1e 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -200,7 +200,7 @@ test_expect_success 'update --init' '\n \n \tmv init init2 &&\n \tgit config -f .gitmodules submodule.example.url \"$(pwd)/init2\" &&\n-\tgit config --remove-section submodule.example\n+\tgit config --remove-section submodule.example &&\n \tgit submodule update init > update.out &&\n \tgrep \"not initialized\" update.out &&\n \ttest ! -d init/.git &&\n@@ -209,4 +209,30 @@ test_expect_success 'update --init' '\n \n '\n \n+test_expect_success 'submodule add' '\n+\n+\tgit submodule add \"$(pwd)/init2\" init-added &&\n+\ttest -d init-added/.git &&\n+\t[ \"$(git config -f .gitmodules submodule.init-added.url)\" = \"$(pwd)/init2\" ] &&\n+\t[ \"$(git config -f .gitmodules submodule.init-added.path)\" = \"init-added\" ]\n+\n+'\n+\n+test_expect_success 'submodule add -b' '\n+\n+\t(\n+\t\tcd init2 &&\n+\t\tgit checkout -b branch &&\n+\t\techo t >s &&\n+\t\tgit add s &&\n+\t\tgit commit -m \"change branch\" &&\n+\t\tgit checkout master\n+\t) &&\n+\tgit submodule add -b branch -- \"$(pwd)/init2\" init-added-b &&\n+\ttest -d init-added-b/.git &&\n+\t[ \"$(git config -f .gitmodules submodule.init-added-b.url)\" = \"$(pwd)/init2\" ] &&\n+\t[ \"$(cd init2 && git rev-parse branch)\" = \"$(cd init-added-b && git rev-parse HEAD)\" ]\n+\n+'\n+\n test_done\n"},{"id":"83559","messageId":"20080716191118.19772.96984.stgit@localhost","threadId":"14489","inReplyTo":"20080716190753.19772.93357.stgit@localhost","subject":"[PATCH 3/7] git submodule add: Fix naming clash handling","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-16T19:11:18Z","receivedAt":"2008-07-16T19:11:18Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"This patch fixes git submodule add behaviour when we add submodule\nliving at a same path as logical name of existing submodule. This\ncan happen e.g. in case the user git mv's the previous submodule away\nand then git submodule add's another under the same name.\n\nA test-case is obviously included.\n\nThis is not completely satisfactory since .git/config cross-commit\nconflicts can still occur. A question is whether this is worth\nhandling, maybe it would be worth adding some kind of randomization\nof the autogenerated submodule name, e.g. appending $$ or a timestamp.\n\nSigned-off-by: Petr Baudis <pasky@suse.cz>\n---\n\n git-submodule.sh           |   15 ++++++++++++---\n t/t7400-submodule-basic.sh |   11 +++++++++++\n 2 files changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9228f56..a93547b 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -192,10 +192,19 @@ cmd_add()\n \tgit add \"$path\" ||\n \tdie \"Failed to add submodule '$path'\"\n \n-\tgit config -f .gitmodules submodule.\"$path\".path \"$path\" &&\n-\tgit config -f .gitmodules submodule.\"$path\".url \"$repo\" &&\n+\tname=\"$path\"\n+\tif git config -f .gitmodules submodule.\"$name\".path; then\n+\t\tname=\"$path~\"; i=1;\n+\t\twhile git config -f .gitmodules submodule.\"$name\".path; do\n+\t\t\tname=\"$path~$i\"\n+\t\t\ti=$((i+1))\n+\t\tdone\n+\tfi\n+\n+\tgit config -f .gitmodules submodule.\"$name\".path \"$path\" &&\n+\tgit config -f .gitmodules submodule.\"$name\".url \"$repo\" &&\n \tgit add .gitmodules ||\n-\tdie \"Failed to register submodule '$path'\"\n+\tdie \"Failed to register submodule '$path' (name '$name')\"\n }\n \n #\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex ab5eb1e..092dffc 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -235,4 +235,15 @@ test_expect_success 'submodule add -b' '\n \n '\n \n+test_expect_success 'submodule add auto-naming clash' '\n+\n+\tgit submodule add \"$(pwd)/init2/.git\" example &&\n+\ttest -d example/.git &&\n+\t[ \"$(git config -f .gitmodules submodule.example.url)\" = \"$(pwd)/init2\" ] &&\n+\t[ \"$(git config -f .gitmodules submodule.example.path)\" = \"init\" ]\n+\t[ \"$(git config -f .gitmodules submodule.example~.url)\" = \"$(pwd)/init2/.git\" ] &&\n+\t[ \"$(git config -f .gitmodules submodule.example~.path)\" = \"example\" ]\n+\n+'\n+\n test_done\n"},{"id":"83563","messageId":"20080716191123.19772.67813.stgit@localhost","threadId":"14489","inReplyTo":"20080716190753.19772.93357.stgit@localhost","subject":"[PATCH 4/7] submodule.*: Introduce simple C interface for submodule lookup by path","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-16T19:11:24Z","receivedAt":"2008-07-16T19:11:24Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"The interface will be used for git-mv and git-rm submodule support.\nSo far, only the submodule_by_path() function is defined, however more\ncan be probably expected in the future if/when the git-submodule command\nis ported from shell.\n\nSigned-off-by: Petr Baudis <pasky@suse.cz>\n---\n\n Makefile    |    2 ++\n submodule.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++\n submodule.h |    8 ++++++++\n 3 files changed, 60 insertions(+), 0 deletions(-)\n create mode 100644 submodule.c\n create mode 100644 submodule.h\n\ndiff --git a/Makefile b/Makefile\nindex 9b52071..742b5cb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -368,6 +368,7 @@ LIB_H += run-command.h\n LIB_H += sha1-lookup.h\n LIB_H += sideband.h\n LIB_H += strbuf.h\n+LIB_H += submodule.h\n LIB_H += tag.h\n LIB_H += transport.h\n LIB_H += tree.h\n@@ -458,6 +459,7 @@ LIB_OBJS += sha1_name.o\n LIB_OBJS += shallow.o\n LIB_OBJS += sideband.o\n LIB_OBJS += strbuf.o\n+LIB_OBJS += submodule.o\n LIB_OBJS += symlinks.o\n LIB_OBJS += tag.o\n LIB_OBJS += trace.o\ndiff --git a/submodule.c b/submodule.c\nnew file mode 100644\nindex 0000000..2883ae6\n--- /dev/null\n+++ b/submodule.c\n@@ -0,0 +1,50 @@\n+#include \"cache.h\"\n+#include \"submodule.h\"\n+\n+\n+struct gitmodules_info {\n+\tconst char *path;\n+\tchar *key;\n+};\n+\n+static int gitmodules_worker(const char *key, const char *value, void *info_)\n+{\n+\tstruct gitmodules_info *info = info_;\n+\tconst char *subkey;\n+\n+\tif (prefixcmp(key, \"submodule.\"))\n+\t\treturn 0;\n+\n+\tsubkey = strrchr(key, '.');\n+\tif (!subkey)\n+\t\treturn 0;\n+\n+\tif (strcmp(subkey, \".path\"))\n+\t\treturn 0;\n+\n+\tif (strcmp(value, info->path))\n+\t\treturn 0;\n+\n+\t/* Found the key to change. */\n+\tif (info->key) {\n+\t\terror(\"multiple submodules live at path `%s'\", info->path);\n+\t\t/* The last one is supposed to win. */\n+\t\tfree(info->key);\n+\t}\n+\tinfo->key = xstrdup(key);\n+\treturn 0;\n+}\n+\n+char *submodule_by_path(const char *path)\n+{\n+\tstruct gitmodules_info info = { path, NULL };\n+\n+\tconfig_exclusive_filename = \".gitmodules\";\n+\tif (git_config(gitmodules_worker, &info))\n+\t\tdie(\"cannot process .gitmodules\");\n+\tif (!info.key)\n+\t\tdie(\"the submodule of `%s' not found in .gitmodules\", path);\n+\tconfig_exclusive_filename = NULL;\n+\n+\treturn info.key;\n+}\ndiff --git a/submodule.h b/submodule.h\nnew file mode 100644\nindex 0000000..bc74fa0\n--- /dev/null\n+++ b/submodule.h\n@@ -0,0 +1,8 @@\n+#ifndef SUBMODULE_H\n+#define SUBMODULE_H\n+\n+/* Find submodule living at given path in .gitmodules and return the key\n+ * of its path config variable (dynamically allocated). */\n+extern char *submodule_by_path(const char *path);\n+\n+#endif\n"},{"id":"83562","messageId":"20080716191129.19772.41903.stgit@localhost","threadId":"14489","inReplyTo":"20080716190753.19772.93357.stgit@localhost","subject":"[PATCH 5/7] git mv: Support moving submodules","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-16T19:11:29Z","receivedAt":"2008-07-16T19:11:29Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"This patch adds support for moving submodules to 'git mv', including\nrewriting of the .gitmodules file to reflect the movement.\n\nThe usage of struct path_list here is a bit abusive, but keeps the\ncode simple and hopefully still reasonably readable. The horrid\nindex_path_src_sha1 hack is unfortunately much worse, however the author\nis currently unaware of any more reasonable solution of the problem.\n\nSigned-off-by: Petr Baudis <pasky@suse.cz>\n---\n\n builtin-mv.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++------\n cache.h      |    2 ++\n sha1_file.c  |   10 ++++++++++\n 3 files changed, 61 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex 158a83d..30c4e7d 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -9,6 +9,7 @@\n #include \"cache-tree.h\"\n #include \"path-list.h\"\n #include \"parse-options.h\"\n+#include \"submodule.h\"\n \n static const char * const builtin_mv_usage[] = {\n \t\"git-mv [options] <source>... <destination>\",\n@@ -60,6 +61,24 @@ static const char *add_slash(const char *path)\n \treturn path;\n }\n \n+static int ce_is_gitlink(int i)\n+{\n+\treturn i < 0 ? 0 : S_ISGITLINK(active_cache[i]->ce_mode);\n+}\n+\n+static void rename_submodule(struct path_list_item *i)\n+{\n+\tchar *key = submodule_by_path(i->path);\n+\n+\tconfig_exclusive_filename = \".gitmodules\";\n+\tif (git_config_set(key, (const char *) i->util))\n+\t\tdie(\"cannot update .gitmodules\");\n+\tconfig_exclusive_filename = NULL;\n+\n+\tfree(key);\n+}\n+\n+\n static struct lock_file lock_file;\n \n int cmd_mv(int argc, const char **argv, const char *prefix)\n@@ -77,9 +96,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tstruct stat st;\n \tstruct path_list overwritten = {NULL, 0, 0, 0};\n \tstruct path_list src_for_dst = {NULL, 0, 0, 0};\n+\t/* .util contains sha1 for submodules */\n \tstruct path_list added = {NULL, 0, 0, 0};\n \tstruct path_list deleted = {NULL, 0, 0, 0};\n \tstruct path_list changed = {NULL, 0, 0, 0};\n+\t/* .path is source path, .util is destination path */\n+\tstruct path_list submodules = {NULL, 0, 0, 0};\n \n \tgit_config(git_default_config, NULL);\n \n@@ -99,7 +121,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t/* special case: \".\" was normalized to \"\" */\n \t\tdestination = copy_pathspec(dest_path[0], argv, argc, 1);\n \telse if (!lstat(dest_path[0], &st) &&\n-\t\t\tS_ISDIR(st.st_mode)) {\n+\t\t\tS_ISDIR(st.st_mode) &&\n+\t\t\t!ce_is_gitlink(cache_name_pos(dest_path[0], strlen(dest_path[0])))) {\n \t\tdest_path[0] = add_slash(dest_path[0]);\n \t\tdestination = copy_pathspec(dest_path[0], argv, argc, 1);\n \t} else {\n@@ -111,7 +134,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t/* Checking */\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *src = source[i], *dst = destination[i];\n-\t\tint length, src_is_dir;\n+\t\tint length, src_is_dir, src_cache_pos;\n \t\tconst char *bad = NULL;\n \n \t\tif (show_only)\n@@ -126,7 +149,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t} else if ((src_is_dir = S_ISDIR(st.st_mode))\n \t\t\t\t&& lstat(dst, &st) == 0)\n \t\t\tbad = \"cannot move directory over file\";\n-\t\telse if (src_is_dir) {\n+\t\telse if ((src_cache_pos = cache_name_pos(src, length)) < 0 && src_is_dir) {\n \t\t\tconst char *src_w_slash = add_slash(src);\n \t\t\tint len_w_slash = length + 1;\n \t\t\tint first, last;\n@@ -193,7 +216,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t} else\n \t\t\t\t\tbad = \"Cannot overwrite\";\n \t\t\t}\n-\t\t} else if (cache_name_pos(src, length) < 0)\n+\t\t} else if (src_cache_pos < 0)\n \t\t\tbad = \"not under version control\";\n \t\telse if (path_list_has_path(&src_for_dst, dst))\n \t\t\tbad = \"multiple sources for the same target\";\n@@ -218,6 +241,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *src = source[i], *dst = destination[i];\n \t\tenum update_mode mode = modes[i];\n+\t\tstruct path_list_item *last = NULL;\n+\t\tint j;\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@@ -227,14 +252,24 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\tif (mode == WORKING_DIRECTORY)\n \t\t\tcontinue;\n \n-\t\tassert(cache_name_pos(src, strlen(src)) >= 0);\n+\t\tj = cache_name_pos(src, strlen(src));\n+\t\tassert(j >= 0);\n \n \t\tpath_list_insert(src, &deleted);\n \t\t/* destination can be a directory with 1 file inside */\n \t\tif (path_list_has_path(&overwritten, dst))\n \t\t\tpath_list_insert(dst, &changed);\n \t\telse\n-\t\t\tpath_list_insert(dst, &added);\n+\t\t\tlast = path_list_insert(dst, &added);\n+\t\tif (ce_is_gitlink(j)) {\n+\t\t\tpath_list_insert(src, &submodules)->util = (void *) dst;\n+\t\t\t/* We will note the original HEAD sha1; the submodule\n+\t\t\t * may not be checked out, in which case we would have\n+\t\t\t * no way to re-obtain it later when adding the new\n+\t\t\t * entry. */\n+\t\t\tassert(last);\n+\t\t\tlast->util = active_cache[j]->sha1;\n+\t\t}\n \t}\n \n \tif (show_only) {\n@@ -254,13 +289,21 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \n \t\tfor (i = 0; i < added.nr; i++) {\n \t\t\tconst char *path = added.items[i].path;\n+\t\t\tindex_path_src_sha1 = added.items[i].util;\n \t\t\tif (add_file_to_cache(path, verbose ? ADD_CACHE_VERBOSE : 0))\n \t\t\t\tdie(\"updating index entries failed\");\n+\t\t\tindex_path_src_sha1 = NULL;\n \t\t}\n \n \t\tfor (i = 0; i < deleted.nr; i++)\n \t\t\tremove_file_from_cache(deleted.items[i].path);\n \n+\t\tfor (i = 0; i < submodules.nr; i++)\n+\t\t\trename_submodule(&submodules.items[i]);\n+\n+\t\tif (submodules.nr > 0 && add_file_to_cache(\".gitmodules\", 0))\n+\t\t\tdie(\"cannot add new .gitmodules to the index\");\n+\n \t\tif (active_cache_changed) {\n \t\t\tif (write_cache(newfd, active_cache, active_nr) ||\n \t\t\t    commit_locked_index(&lock_file))\ndiff --git a/cache.h b/cache.h\nindex a779d92..998b5fb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -387,6 +387,8 @@ extern int ce_same_name(struct cache_entry *a, struct cache_entry *b);\n extern int ie_match_stat(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);\n extern int ie_modified(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);\n \n+extern unsigned char *index_path_src_sha1;\n+\n extern int ce_path_match(const struct cache_entry *ce, const char **pathspec);\n extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, enum object_type type, const char *path);\n extern int index_pipe(unsigned char *sha1, int fd, const char *type, int write_object);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex e281c14..4da6048 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2416,12 +2416,22 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \treturn ret;\n }\n \n+unsigned char *index_path_src_sha1;\n+\n int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object)\n {\n \tint fd;\n \tchar *target;\n \tsize_t len;\n \n+\tif (index_path_src_sha1) {\n+\t\t/* SHA1 for this path was prepared by the caller specially;\n+\t\t * e.g. comes from original cache entry in some cases of\n+\t\t * a rename. */\n+\t\tmemcpy(sha1, index_path_src_sha1, 20);\n+\t\treturn 0;\n+\t}\n+\n \tswitch (st->st_mode & S_IFMT) {\n \tcase S_IFREG:\n \t\tfd = open(path, O_RDONLY);\n"},{"id":"83561","messageId":"20080716191134.19772.85003.stgit@localhost","threadId":"14489","inReplyTo":"20080716190753.19772.93357.stgit@localhost","subject":"[PATCH 6/7] git rm: Support for removing submodules","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-16T19:11:34Z","receivedAt":"2008-07-16T19:11:34Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"This patch adds support for removing submodules to 'git rm', including\nremoving the appropriate sections from the .gitmodules file to reflect this\n\nSigned-off-by: Petr Baudis <pasky@suse.cz>\n---\n\n Documentation/git-rm.txt |    6 +++-\n builtin-rm.c             |   65 ++++++++++++++++++++++++++++++++++++++--------\n 2 files changed, 58 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt\nindex 4d0c495..bfc3dfa 100644\n--- a/Documentation/git-rm.txt\n+++ b/Documentation/git-rm.txt\n@@ -20,7 +20,8 @@ and no updates to their contents can be staged in the index,\n though that default behavior can be overridden with the `-f` option.\n When '--cached' is given, the staged content has to\n match either the tip of the branch or the file on disk,\n-allowing the file to be removed from just the index.\n+allowing the file to be removed from just the index;\n+this is always the case when removing submodules.\n \n \n OPTIONS\n@@ -56,7 +57,8 @@ OPTIONS\n --cached::\n \tUse this option to unstage and remove paths only from the index.\n \tWorking tree files, whether modified or not, will be\n-\tleft alone.\n+\tleft alone.  Note that this is always assumed when removing\n+\ta checked-out submodule.\n \n --ignore-unmatch::\n \tExit with a zero status even if no files matched.\ndiff --git a/builtin-rm.c b/builtin-rm.c\nindex 22c9bd1..363d1fa 100644\n--- a/builtin-rm.c\n+++ b/builtin-rm.c\n@@ -9,6 +9,7 @@\n #include \"cache-tree.h\"\n #include \"tree-walk.h\"\n #include \"parse-options.h\"\n+#include \"submodule.h\"\n \n static const char * const builtin_rm_usage[] = {\n \t\"git-rm [options] [--] <file>...\",\n@@ -17,16 +18,21 @@ static const char * const builtin_rm_usage[] = {\n \n static struct {\n \tint nr, alloc;\n-\tconst char **name;\n+\tstruct {\n+\t\tconst char *name;\n+\t\tint is_gitlink;\n+\t} *info;\n } list;\n \n-static void add_list(const char *name)\n+static void add_list(const char *name, int is_gitlink)\n {\n \tif (list.nr >= list.alloc) {\n \t\tlist.alloc = alloc_nr(list.alloc);\n-\t\tlist.name = xrealloc(list.name, list.alloc * sizeof(const char *));\n+\t\tlist.info = xrealloc(list.info, list.alloc * sizeof(*list.info));\n \t}\n-\tlist.name[list.nr++] = name;\n+\tlist.info[list.nr].name = name;\n+\tlist.info[list.nr].is_gitlink = is_gitlink;\n+\tlist.nr++;\n }\n \n static int remove_file(const char *name)\n@@ -38,6 +44,13 @@ static int remove_file(const char *name)\n \tif (ret && errno == ENOENT)\n \t\t/* The user has removed it from the filesystem by hand */\n \t\tret = errno = 0;\n+\tif (ret && errno == EISDIR) {\n+\t\t/* This is a gitlink entry; try to remove at least the\n+\t\t * directory if the submodule is not checked out; we always\n+\t\t * leave the checked out ones as they are */\n+\t\tif (!rmdir(name) || errno == ENOTEMPTY)\n+\t\t\tret = errno = 0;\n+\t}\n \n \tif (!ret && (slash = strrchr(name, '/'))) {\n \t\tchar *n = xstrdup(name);\n@@ -65,7 +78,7 @@ static int check_local_mod(unsigned char *head, int index_only)\n \t\tstruct stat st;\n \t\tint pos;\n \t\tstruct cache_entry *ce;\n-\t\tconst char *name = list.name[i];\n+\t\tconst char *name = list.info[i].name;\n \t\tunsigned char sha1[20];\n \t\tunsigned mode;\n \t\tint local_changes = 0;\n@@ -83,7 +96,7 @@ static int check_local_mod(unsigned char *head, int index_only)\n \t\t\t/* It already vanished from the working tree */\n \t\t\tcontinue;\n \t\t}\n-\t\telse if (S_ISDIR(st.st_mode)) {\n+\t\telse if (S_ISDIR(st.st_mode) && !S_ISGITLINK(ce->ce_mode)) {\n \t\t\t/* if a file was removed and it is now a\n \t\t\t * directory, that is the same as ENOENT as\n \t\t\t * far as git is concerned; we do not track\n@@ -122,6 +135,22 @@ static int check_local_mod(unsigned char *head, int index_only)\n \treturn errs;\n }\n \n+static void remove_submodule(const char *name)\n+{\n+\tchar *key = submodule_by_path(name);\n+\tchar *sectend = strrchr(key, '.');\n+\n+\tassert(sectend);\n+\t*sectend = 0;\n+\n+\tconfig_exclusive_filename = \".gitmodules\";\n+\tif (git_config_rename_section(key, NULL) <= 0)\n+\t\tdie(\"cannot remove section `%s' from .gitmodules\", key);\n+\tconfig_exclusive_filename = NULL;\n+\n+\tfree(key);\n+}\n+\n static struct lock_file lock_file;\n \n static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;\n@@ -140,7 +169,7 @@ static struct option builtin_rm_options[] = {\n \n int cmd_rm(int argc, const char **argv, const char *prefix)\n {\n-\tint i, newfd;\n+\tint i, newfd, subs;\n \tconst char **pathspec;\n \tchar *seen;\n \n@@ -168,7 +197,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tif (!match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen))\n \t\t\tcontinue;\n-\t\tadd_list(ce->name);\n+\t\tadd_list(ce->name, S_ISGITLINK(ce->ce_mode));\n \t}\n \n \tif (pathspec) {\n@@ -216,9 +245,11 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t * the index unless all of them succeed.\n \t */\n \tfor (i = 0; i < list.nr; i++) {\n-\t\tconst char *path = list.name[i];\n+\t\tconst char *path = list.info[i].name;\n \t\tif (!quiet)\n-\t\t\tprintf(\"rm '%s'\\n\", path);\n+\t\t\tprintf(\"rm%s '%s'\\n\",\n+\t\t\t\tlist.info[i].is_gitlink ? \"dir\" : \"\",\n+\t\t\t\tpath);\n \n \t\tif (remove_file_from_cache(path))\n \t\t\tdie(\"git-rm: unable to remove %s\", path);\n@@ -238,7 +269,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tif (!index_only) {\n \t\tint removed = 0;\n \t\tfor (i = 0; i < list.nr; i++) {\n-\t\t\tconst char *path = list.name[i];\n+\t\t\tconst char *path = list.info[i].name;\n \t\t\tif (!remove_file(path)) {\n \t\t\t\tremoved = 1;\n \t\t\t\tcontinue;\n@@ -248,6 +279,18 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\t/*\n+\t * Get rid of stale submodule setup.\n+\t */\n+\tsubs = 0;\n+\tfor (i = 0; i < list.nr; i++)\n+\t\tif (list.info[i].is_gitlink) {\n+\t\t\tremove_submodule(list.info[i].name);\n+\t\t\tsubs++;\n+\t\t}\n+\tif (subs && add_file_to_cache(\".gitmodules\", 0))\n+\t\tdie(\"cannot add new .gitmodules to the index\");\n+\n \tif (active_cache_changed) {\n \t\tif (write_cache(newfd, active_cache, active_nr) ||\n \t\t    commit_locked_index(&lock_file))\n"},{"id":"83560","messageId":"20080716191139.19772.16159.stgit@localhost","threadId":"14489","inReplyTo":"20080716190753.19772.93357.stgit@localhost","subject":"[PATCH 7/7] t7403: Submodule git mv, git rm testsuite","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-16T19:11:40Z","receivedAt":"2008-07-16T19:11:40Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"The testsuite for newly added submodule support in git mv, git rm.\n\nSigned-off-by: Petr Baudis <pasky@suse.cz>\n---\n\n t/t7403-submodule-mvrm.sh |  242 +++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 242 insertions(+), 0 deletions(-)\n create mode 100755 t/t7403-submodule-mvrm.sh\n\ndiff --git a/t/t7403-submodule-mvrm.sh b/t/t7403-submodule-mvrm.sh\nnew file mode 100755\nindex 0000000..9b50d6a\n--- /dev/null\n+++ b/t/t7403-submodule-mvrm.sh\n@@ -0,0 +1,242 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2008 Johannes Schindelin\n+#\n+\n+test_description='Test submodules support in git mv and git rm'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\n+\t(mkdir sub-repo &&\n+\t cd sub-repo &&\n+\t git init &&\n+\t echo file > file &&\n+\t git add file &&\n+\t git commit -m \"sub initial\") &&\n+\t(cp -r sub-repo sub2-repo &&\n+\t cd sub2-repo &&\n+\t echo file2 > file &&\n+\t git add file &&\n+\t git commit -m \"sub commit2\") &&\n+\tgit submodule add \"$(pwd)/sub-repo\" sub &&\n+\tgit submodule add \"$(pwd)/sub2-repo\" sub2 &&\n+\tgit commit -m initial &&\n+\ttest \"$(git config -f .gitmodules submodule.sub.path)\" = \"sub\" &&\n+\ttest \"$(git config -f .gitmodules submodule.sub2.path)\" = \"sub2\"\n+\n+'\n+\n+test_expect_success 'git mv of a submodule' '\n+\n+\tgit mv sub sub.moved &&\n+\t! test -d sub &&\n+\ttest -d sub.moved/.git &&\n+\t! git ls-files --error-unmatch sub &&\n+\ttest \"$(git ls-files --stage --error-unmatch sub.moved | cut -d \" \" -f 1)\" = 160000 &&\n+\ttest \"$(git config -f .gitmodules submodule.sub.path)\" = \"sub.moved\" &&\n+\t! git config -f .gitmodules submodule.sub.moved.path\n+\n+'\n+\n+test_expect_success 'git submodule add vs. git mv' '\n+\n+\t! git submodule add \"$(pwd)/sub2-repo\" sub.moved &&\n+\tgit submodule add \"$(pwd)/sub2-repo\" sub &&\n+\ttest -d sub/.git &&\n+\ttest \"$(git config -f .gitmodules submodule.sub.url)\" = \"$(pwd)/sub-repo\" &&\n+\ttest \"$(git config -f .gitmodules submodule.sub.path)\" = \"sub.moved\" &&\n+\ttest \"$(git config -f .gitmodules submodule.sub~.path)\" = \"sub\"\n+\n+'\n+\n+test_expect_success 'git mv onto existing file' '\n+\n+\techo file > file &&\n+\tgit add file &&\n+\t! git mv sub.moved file &&\n+\ttest -d sub.moved &&\n+\t! test -d file/.git &&\n+\ttest \"$(git ls-files --stage --error-unmatch file | cut -d \" \" -f 1)\" = 100644 &&\n+\ttest \"$(git ls-files --stage --error-unmatch sub.moved | cut -d \" \" -f 1)\" = 160000 &&\n+\ttest \"$(git config -f .gitmodules submodule.sub.path)\" = \"sub.moved\"\n+\n+'\n+\n+test_expect_success 'git mv onto existing directory' '\n+\n+\tmkdir -p dir &&\n+\techo file > dir/file &&\n+\tgit add dir/file &&\n+\tgit mv sub.moved dir &&\n+\t! test -d sub.moved &&\n+\ttest -d dir/sub.moved/.git &&\n+\t! git ls-files --error-unmatch sub.moved &&\n+\ttest \"$(git ls-files --stage --error-unmatch dir/sub.moved | cut -d \" \" -f 1)\" = 160000 &&\n+\ttest \"$(git config -f .gitmodules submodule.sub.path)\" = \"dir/sub.moved\" &&\n+\tgit mv dir/sub.moved . &&\n+\ttest \"$(git config -f .gitmodules submodule.sub.path)\" = \"sub.moved\"\n+\n+'\n+\n+test_expect_success 'git mv onto existing submodule' '\n+\n+\t! git mv sub.moved sub2 &&\n+\ttest -d sub.moved/.git &&\n+\t! test -d sub2/sub.moved &&\n+\ttest \"$(git ls-files --stage --error-unmatch sub2 | cut -d \" \" -f 1)\" = 160000 &&\n+\ttest \"$(git ls-files --stage --error-unmatch sub.moved | cut -d \" \" -f 1)\" = 160000 &&\n+\ttest \"$(git config -f .gitmodules submodule.sub.path)\" = \"sub.moved\"\n+\n+'\n+\n+test_expect_success 'git mv of multiple submodules' '\n+\n+\tmkdir -p dir &&\n+\tgit mv sub.moved sub dir &&\n+\t! test -d sub.moved &&\n+\t! test -d sub &&\n+\ttest -d dir/sub.moved/.git &&\n+\ttest -d dir/sub/.git &&\n+\t! git ls-files --error-unmatch sub.moved sub &&\n+\ttest \"$(git ls-files --stage --error-unmatch dir/sub.moved dir/sub | cut -d \" \" -f 1 | uniq)\" = 160000 &&\n+\t! git config -f .gitmodules submodule.dir.path &&\n+\ttest \"$(git config -f .gitmodules submodule.sub.path)\" = \"dir/sub.moved\" &&\n+\ttest \"$(git config -f .gitmodules submodule.sub~.path)\" = \"dir/sub\"\n+\n+'\n+\n+test_expect_success 'git mv of multiple submodules back from a subdir' '\n+\n+\t(cd dir && git mv sub.moved sub .. && cd ..) &&\n+\ttest -d sub.moved &&\n+\ttest -d sub &&\n+\t! test -d dir/sub.moved/.git &&\n+\t! test -d dir/sub/.git &&\n+\t! git ls-files --error-unmatch dir/sub.moved dir/sub &&\n+\ttest \"$(git ls-files --stage --error-unmatch sub.moved sub | cut -d \" \" -f 1 | uniq)\" = 160000 &&\n+\ttest \"$(git config -f .gitmodules submodule.sub.path)\" = \"sub.moved\" &&\n+\ttest \"$(git config -f .gitmodules submodule.sub~.path)\" = \"sub\"\n+\n+'\n+\n+test_expect_success 'git mv of non-checked-out submodules' '\n+\n+\tgit clone . clone &&\n+\t(cd clone &&\n+\ttest -d sub &&\n+\ttest -d sub2 &&\n+\t! test -d sub/.git &&\n+\t! test -d sub2/.git &&\n+\tgit ls-files --stage --error-unmatch sub sub2 > ls-files.out &&\n+\tmkdir -p dir &&\n+\tgit mv sub sub2 dir &&\n+\t! test -d sub &&\n+\t! test -d sub2 &&\n+\ttest -d dir/sub &&\n+\ttest -d dir/sub2 &&\n+\t! git ls-files --error-unmatch sub sub2 &&\n+\ttest \"$(git ls-files --stage --error-unmatch dir/sub dir/sub2 | cut -d \" \" -f 1 | uniq)\" = 160000 &&\n+\tgit ls-files --stage --error-unmatch dir/sub dir/sub2 | sed \"s#dir/##g\" | diff - ls-files.out &&\n+\ttest \"$(git config -f .gitmodules submodule.sub.path)\" = \"dir/sub\" &&\n+\ttest \"$(git config -f .gitmodules submodule.sub2.path)\" = \"dir/sub2\" &&\n+\t(cd dir && git mv sub2 .. && cd ..) &&\n+\ttest -d sub2 &&\n+\t! test -d dir/sub2 &&\n+\t! git ls-files --error-unmatch dir/sub2 &&\n+\ttest \"$(git ls-files --stage --error-unmatch sub2 | cut -d \" \" -f 1)\" = 160000 &&\n+\ttest \"$(git config -f .gitmodules submodule.sub2.path)\" = \"sub2\")\n+\n+'\n+\n+test_expect_success 'checkpointing state with git commit' '\n+\n+\tgit commit -m\"checkpoint\" -a &&\n+\t(cd clone && git commit -m\"clone checkpoint\" -a)\n+\n+'\n+\n+test_expect_success 'git rm of a regular submodule' '\n+\n+\tgit rm sub2 &&\n+\ttest -d sub2/.git &&\n+\t! git ls-files --error-unmatch sub2 &&\n+\t! git config -f .gitmodules submodule.sub2.path &&\n+\t! git config -f .gitmodules submodule.sub2.url\n+\n+'\n+\n+test_expect_success 'git rm of a submodule with name different from path' '\n+\n+\tgit rm sub.moved &&\n+\ttest -d sub.moved/.git &&\n+\t! git ls-files --error-unmatch sub.moved &&\n+\t! git config -f .gitmodules submodule.sub.path &&\n+\t! git config -f .gitmodules submodule.sub.url\n+\n+'\n+\n+test_expect_success 'git rm of a modified submodule' '\n+\n+\tgit mv sub dir/sub && # more fun with richer path\n+\t(cd dir/sub &&\n+\t echo mod > file &&\n+\t git commit -m \"sub mod\" file) &&\n+\tgit add dir/sub &&\n+\t! git rm dir/sub &&\n+\ttest -d dir/sub/.git &&\n+\ttest \"$(git ls-files --stage --error-unmatch dir/sub | cut -d \" \" -f 1)\" = \"160000\" &&\n+\tgit config -f .gitmodules submodule.sub~.path &&\n+\tgit config -f .gitmodules submodule.sub~.url &&\n+\tgit rm -f dir/sub &&\n+\ttest -d dir/sub/.git &&\n+\t! git ls-files --error-unmatch dir/sub &&\n+\t! git config -f .gitmodules submodule.sub~.path &&\n+\t! git config -f .gitmodules submodule.sub~.url\n+\n+'\n+\n+test_expect_success 'git rm of a submodule from within a subdirectory' '\n+\n+\tgit submodule add \"$(pwd)/sub-repo\" sub-torm &&\n+\tmkdir -p dir &&\n+\t# -f since we did not commit the submodule\n+\t(cd dir && git rm -f ../sub-torm && cd ..) &&\n+\ttest -d sub-torm/.git &&\n+\t! git ls-files --error-unmatch sub-torm &&\n+\t! git config -f .gitmodules submodule.sub-torm.path &&\n+\t! git config -f .gitmodules submodule.sub-torm.url\n+\n+'\n+\n+test_expect_success 'git rm of a non-checked-out submodule' '\n+\n+\t(cd clone &&\n+\ttest -d dir/sub &&\n+\t! test -d dir/sub/.git &&\n+\tgit rm dir/sub &&\n+\t! test -d dir/sub &&\n+\t! git ls-files --error-unmatch dir/sub &&\n+\t! git config -f .gitmodules submodule.sub.path &&\n+\t! git config -f .gitmodules submodule.sub.url)\n+\n+'\n+\n+test_expect_success 'git rm of a non-checked-out submodule w/ different working tree' '\n+\n+\t(cd clone &&\n+\trmdir sub2 &&\n+\techo cunning > sub2 &&\n+\t! git rm sub2 &&\n+\ttest -f sub2 &&\n+\ttest \"$(git ls-files --stage --error-unmatch sub2 | cut -d \" \" -f 1)\" = \"160000\" &&\n+\tgit rm -f sub2 &&\n+\t! test -e sub2 &&\n+\t! git ls-files --error-unmatch sub2 &&\n+\t! git config -f .gitmodules submodule.sub2.path &&\n+\t! git config -f .gitmodules submodule.sub2.url)\n+\n+'\n+\n+test_done\n"},{"id":"83611","messageId":"alpine.DEB.1.00.0807170038430.4318@eeepc-johanness","threadId":"14489","inReplyTo":"20080716191134.19772.85003.stgit@localhost","subject":"Re: [PATCH 6/7] git rm: Support for removing submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-16T22:41:57Z","receivedAt":"2008-07-16T22:41:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 16 Jul 2008, Petr Baudis wrote:\n\n> This patch adds support for removing submodules to 'git rm', including \n> removing the appropriate sections from the .gitmodules file to reflect \n> this\n\nI have no time to look at the whole series, or even at the patch, but I \nhave concerns.  Do you really remove the whole directory?  Because if you \ndo, you remove more than what can be possibly reconstructed from the \nobject store.\n\nThat is much more dangerous than what \"git rm\" does.\n\nFor example, you can have local branches, remote settings, untracked \nfiles, etc. in the subdirectory.  And that cannot be recovered once \ndeleted.\n\nI wonder if it really makes sense to integrate that into git-rm, and not \ngit-submodule, if only to introduce another level of consideration for the \nuser before committing what is potentially a big mistake.\n\nCiao,\nDscho\n"},{"id":"83635","messageId":"7vhcapme0q.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"20080716191129.19772.41903.stgit@localhost","subject":"Re: [PATCH 5/7] git mv: Support moving submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-17T02:37:57Z","receivedAt":"2008-07-17T02:37:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis <pasky@suse.cz> writes:\n\n> The usage of struct path_list here is a bit abusive,...\n\nI do not think use of path_list->util is particularly bad.  It is a data\nstructure to store a bag of random pointers that can be indexed with\nstring keys, which is exactly what you are doing here.\n\n> ... The horrid\n> index_path_src_sha1 hack is unfortunately much worse,\n\nThis one certainly is ugly beyond words.\n\nBy the way, why is it even necessary to rehash the contents when you run\n\"mv\"?\n\nIOW,\n\n\t$ >foo\n        $ git add foo\n        $ echo 1 >foo\n        $ git mv foo bar\n\nmakes \"foo\" disappear from the index and adds \"bar\", but it makes the\nlocal change added to the index at the same time.\n\n\tSide note. It actually is a lot worse than that.  When you move\n\tsomething to another existing entry, the code does not even add\n\tthe object name of moved entry recorded in the index, nor rehashes\n\tthe moved file.  This is totally inconsistent!\n\nI personally think these buggy behaviours you are trying to inherit are\nmaking this patch more complex than necessary.  Perhaps this needs to be\nfixed up even further (you did some fix with the first patch) before\nadding new features?  For example, I think it is a bug that the\n\"overwrite\" codepath does not work with symlinks.\n\nBut let's assume that we do not want to \"fix\" any of these existing\n(mis)behaviour, because doing so would change the semantics.\n\nThere are a few cases:\n\n (1) gitlink exists and so is directory but no repository (i.e. the user is\n     not interested);\n\n (2) gitlink exists and there is a repository.  Its HEAD matches what\n     gitlink records;\n\n (3) gitlink exists and there is a repository.  Its HEAD does not match what\n     gitlink records;\n\nThe (mis)behaviour that automatically adds moved files to the index we saw\nearlier suggests that in case (3) you would want to update the gitlink in\nthe index with whatever HEAD the submodule repository has to be\nconsistent.\n\nSo instead of adding a index_path_src_sha1 hack like that, how about\n\n * When you see ce_is_gitlink(j), you keep the original gitlink object\n   name.  That is very good in order to preserve it for not just (1) but\n   also for (3) once we decide to fix this \"auto adding\" misbehaviour in\n   the later round.  But you do not have to do this for case (2) if you\n   are going to rehash anyway, don't you?\n\n   So when you see ce_is_gitlink(j), you check if it has repository and\n   HEAD, and record the path_list->util only when it is case (1).\n\n * Then, only for case (1), you do not call add_file_to_cache() -- because\n   you know you do not have anything you can rehash; instead, factor out\n   the codepath \"git-update-index --cacheinfo\" uses and call that.\n"},{"id":"83677","messageId":"20080717123501.GD10151@machine.or.cz","threadId":"14489","inReplyTo":"alpine.DEB.1.00.0807170038430.4318@eeepc-johanness","subject":"Re: [PATCH 6/7] git rm: Support for removing submodules","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-17T12:35:01Z","receivedAt":"2008-07-17T12:35:01Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"  Hi,\n\nOn Thu, Jul 17, 2008 at 12:41:57AM +0200, Johannes Schindelin wrote:\n> On Wed, 16 Jul 2008, Petr Baudis wrote:\n> \n> > This patch adds support for removing submodules to 'git rm', including \n> > removing the appropriate sections from the .gitmodules file to reflect \n> > this\n> \n> I have no time to look at the whole series, or even at the patch, but I \n> have concerns.  Do you really remove the whole directory?  Because if you \n> do, you remove more than what can be possibly reconstructed from the \n> object store.\n\n  no; I remove only the index entry and the empty directory made for\nnon-checked-out submodules (I just try rmdir() and ignore ENOTEMPTY\nerror).  I make that clear in git rm documentation, but not in the patch\ndescription; I will fix that.\n\n> I wonder if it really makes sense to integrate that into git-rm, and not \n> git-submodule, if only to introduce another level of consideration for the \n> user before committing what is potentially a big mistake.\n\n  That is good question and I forgot to elaborate on this in the cover\nletter. However, I believe that integrating this functionality into the\nbasic commands makes for a smoother user experience, following the\nprinciple of the least surprise. It is difficult for me to argue\nextensively in further favor of this choice, though. :-)\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nGNU, n. An animal of South Africa, which in its domesticated state\nresembles a horse, a buffalo and a stag. In its wild condition it is\nsomething like a thunderbolt, an earthquake and a cyclone. -- A. Pierce\n"},{"id":"83682","messageId":"alpine.DEB.1.00.0807171358550.8986@racer","threadId":"14489","inReplyTo":"20080717123501.GD10151@machine.or.cz","subject":"Re: [PATCH 6/7] git rm: Support for removing submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-17T12:59:31Z","receivedAt":"2008-07-17T12:59:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 17 Jul 2008, Petr Baudis wrote:\n\n> On Thu, Jul 17, 2008 at 12:41:57AM +0200, Johannes Schindelin wrote:\n> > On Wed, 16 Jul 2008, Petr Baudis wrote:\n> > \n> > > This patch adds support for removing submodules to 'git rm', \n> > > including removing the appropriate sections from the .gitmodules \n> > > file to reflect this\n> > \n> > I have no time to look at the whole series, or even at the patch, but \n> > I have concerns.  Do you really remove the whole directory?  Because \n> > if you do, you remove more than what can be possibly reconstructed \n> > from the object store.\n> \n> no; I remove only the index entry and the empty directory made for \n> non-checked-out submodules (I just try rmdir() and ignore ENOTEMPTY \n> error).  I make that clear in git rm documentation, but not in the patch \n> description; I will fix that.\n\nThanks for explanation!\n\n> > I wonder if it really makes sense to integrate that into git-rm, and \n> > not git-submodule, if only to introduce another level of consideration \n> > for the user before committing what is potentially a big mistake.\n> \n> That is good question and I forgot to elaborate on this in the cover \n> letter. However, I believe that integrating this functionality into the \n> basic commands makes for a smoother user experience, following the \n> principle of the least surprise.\n\nMakes sense,\nDscho\n"},{"id":"83684","messageId":"20080717130651.GU32184@machine.or.cz","threadId":"14489","inReplyTo":"7vhcapme0q.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 5/7] git mv: Support moving submodules","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-17T13:06:51Z","receivedAt":"2008-07-17T13:06:51Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"  Hi!\n\nOn Wed, Jul 16, 2008 at 07:37:57PM -0700, Junio C Hamano wrote:\n> Petr Baudis <pasky@suse.cz> writes:\n> \n> > ... The horrid\n> > index_path_src_sha1 hack is unfortunately much worse,\n> \n> This one certainly is ugly beyond words.\n\n  ;-)\n\n> By the way, why is it even necessary to rehash the contents when you run\n> \"mv\"?\n> \n> IOW,\n> \n> \t$ >foo\n>         $ git add foo\n>         $ echo 1 >foo\n>         $ git mv foo bar\n> \n> makes \"foo\" disappear from the index and adds \"bar\", but it makes the\n> local change added to the index at the same time.\n> \n> \tSide note. It actually is a lot worse than that.  When you move\n> \tsomething to another existing entry, the code does not even add\n> \tthe object name of moved entry recorded in the index, nor rehashes\n> \tthe moved file.  This is totally inconsistent!\n> \n> I personally think these buggy behaviours you are trying to inherit are\n> making this patch more complex than necessary.  Perhaps this needs to be\n> fixed up even further (you did some fix with the first patch) before\n> adding new features?  For example, I think it is a bug that the\n> \"overwrite\" codepath does not work with symlinks.\n\n  I agree that it would be much cleaner to fix this; I got puzzled about\nthis behaviour a bit, but I was afraid to break the traditional\nbehaviour. However, if you are feeling this brave, patch to follow up\nshortly. :-)\n\n>  * Then, only for case (1), you do not call add_file_to_cache() -- because\n>    you know you do not have anything you can rehash; instead, factor out\n>    the codepath \"git-update-index --cacheinfo\" uses and call that.\n\n  This is excellent hint, sort of what I hoped for, thanks! I forgot\nabout --cacheinfo completely, which is truly a shame especially when I\nlook at the history of this switch. ;-) (BTW, curiously, the commit\nlists Linus as an author even though the patch is yours. Maybe this was\nmerely some imperfection of the early scripts around Git, though.)\nI really did not touch git internals for way too long.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nGNU, n. An animal of South Africa, which in its domesticated state\nresembles a horse, a buffalo and a stag. In its wild condition it is\nsomething like a thunderbolt, an earthquake and a cyclone. -- A. Pierce\n"},{"id":"83769","messageId":"20080717223036.20680.9672.stgit@localhost","threadId":"14489","inReplyTo":"20080717130651.GU32184@machine.or.cz","subject":"[PATCH] git-mv: Keep moved index entries inact","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-17T22:31:50Z","receivedAt":"2008-07-17T22:31:50Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"The rewrite of git-mv from a shell script to a builtin was perhaps\na little too straightforward: the git add and git rm queues were\nemulated directly, which resulted in a rather complicated code and\ncaused an inconsistent behaviour when moving dirty index entries;\ngit mv would update the entry based on working tree state,\nexcept in case of overwrites, where the new entry would still have\nsha1 of the old file.\n\nThis patch introduces rename_index_entry_at() into the index toolkit,\nwhich will rename an entry while removing any entries the new entry\nmight render duplicate. This is then used in git mv instead\nof all the file queues, resulting in a major simplification\nof the code and an inevitable change in git mv -n output format.\n\nA new test has been added to the testsuite to reflect this change.\nAlso, based on suggestion by Junio about desired symlink behaviour\nof git mv, I have added two tests for that; however, I do not have\nneed or desire to spend time fixing this, so they are expected\nto fail for now until someone gets around to fixing that.\n\nSigned-off-by: Petr Baudis <pasky@suse.cz>\n---\n\n  This patch might depend on git-mv: Remove dead code branch.\n\n builtin-mv.c  |   62 ++++++++-------------------------------------------------\n cache.h       |    2 ++\n read-cache.c  |   15 ++++++++++++++\n t/t7001-mv.sh |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 83 insertions(+), 53 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex 33ad082..28ebc9c 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -36,17 +36,6 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,\n \treturn get_pathspec(prefix, result);\n }\n \n-static void show_list(const char *label, struct path_list *list)\n-{\n-\tif (list->nr > 0) {\n-\t\tint i;\n-\t\tprintf(\"%s\", label);\n-\t\tfor (i = 0; i < list->nr; i++)\n-\t\t\tprintf(\"%s%s\", i > 0 ? \", \" : \"\", list->items[i].path);\n-\t\tputchar('\\n');\n-\t}\n-}\n-\n static const char *add_slash(const char *path)\n {\n \tint len = strlen(path);\n@@ -75,11 +64,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tconst char **source, **destination, **dest_path;\n \tenum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;\n \tstruct stat st;\n-\tstruct path_list overwritten = {NULL, 0, 0, 0};\n \tstruct path_list src_for_dst = {NULL, 0, 0, 0};\n-\tstruct path_list added = {NULL, 0, 0, 0};\n-\tstruct path_list deleted = {NULL, 0, 0, 0};\n-\tstruct path_list changed = {NULL, 0, 0, 0};\n \n \tgit_config(git_default_config, NULL);\n \n@@ -189,7 +174,6 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t\t\" will overwrite!\\n\",\n \t\t\t\t\t\t\tbad);\n \t\t\t\t\tbad = NULL;\n-\t\t\t\t\tpath_list_insert(dst, &overwritten);\n \t\t\t\t} else\n \t\t\t\t\tbad = \"Cannot overwrite\";\n \t\t\t}\n@@ -218,6 +202,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *src = source[i], *dst = destination[i];\n \t\tenum update_mode mode = modes[i];\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@@ -227,45 +212,16 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\tif (mode == WORKING_DIRECTORY)\n \t\t\tcontinue;\n \n-\t\tassert(cache_name_pos(src, strlen(src)) >= 0);\n-\n-\t\tpath_list_insert(src, &deleted);\n-\t\t/* destination can be a directory with 1 file inside */\n-\t\tif (path_list_has_path(&overwritten, dst))\n-\t\t\tpath_list_insert(dst, &changed);\n-\t\telse\n-\t\t\tpath_list_insert(dst, &added);\n+\t\tpos = cache_name_pos(src, strlen(src));\n+\t\tassert(pos >= 0);\n+\t\tif (!show_only)\n+\t\t\trename_cache_entry_at(pos, dst);\n \t}\n \n-\tif (show_only) {\n-\t\tshow_list(\"Changed  : \", &changed);\n-\t\tshow_list(\"Adding   : \", &added);\n-\t\tshow_list(\"Deleting : \", &deleted);\n-\t} else {\n-\t\tfor (i = 0; i < changed.nr; i++) {\n-\t\t\tconst char *path = changed.items[i].path;\n-\t\t\tint j = cache_name_pos(path, strlen(path));\n-\t\t\tstruct cache_entry *ce = active_cache[j];\n-\n-\t\t\tif (j < 0)\n-\t\t\t\tdie (\"Huh? Cache entry for %s unknown?\", path);\n-\t\t\trefresh_cache_entry(ce, 0);\n-\t\t}\n-\n-\t\tfor (i = 0; i < added.nr; i++) {\n-\t\t\tconst char *path = added.items[i].path;\n-\t\t\tif (add_file_to_cache(path, verbose ? ADD_CACHE_VERBOSE : 0))\n-\t\t\t\tdie(\"updating index entries failed\");\n-\t\t}\n-\n-\t\tfor (i = 0; i < deleted.nr; i++)\n-\t\t\tremove_file_from_cache(deleted.items[i].path);\n-\n-\t\tif (active_cache_changed) {\n-\t\t\tif (write_cache(newfd, active_cache, active_nr) ||\n-\t\t\t    commit_locked_index(&lock_file))\n-\t\t\t\tdie(\"Unable to write new index file\");\n-\t\t}\n+\tif (active_cache_changed) {\n+\t\tif (write_cache(newfd, active_cache, active_nr) ||\n+\t\t    commit_locked_index(&lock_file))\n+\t\t\tdie(\"Unable to write new index file\");\n \t}\n \n \treturn 0;\ndiff --git a/cache.h b/cache.h\nindex a779d92..6f1d003 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -260,6 +260,7 @@ static inline void remove_name_hash(struct cache_entry *ce)\n #define unmerged_cache() unmerged_index(&the_index)\n #define cache_name_pos(name, namelen) index_name_pos(&the_index,(name),(namelen))\n #define add_cache_entry(ce, option) add_index_entry(&the_index, (ce), (option))\n+#define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))\n #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))\n #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))\n #define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags))\n@@ -370,6 +371,7 @@ extern int index_name_pos(const struct index_state *, const char *name, int name\n #define ADD_CACHE_JUST_APPEND 8\t\t/* Append only; tree.c::read_tree() */\n extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);\n extern struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really);\n+extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);\n extern int remove_index_entry_at(struct index_state *, int pos);\n extern int remove_file_from_index(struct index_state *, const char *path);\n #define ADD_CACHE_VERBOSE 1\ndiff --git a/read-cache.c b/read-cache.c\nindex 1648428..70e5f57 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -38,6 +38,21 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache\n \tistate->cache_changed = 1;\n }\n \n+void rename_index_entry_at(struct index_state *istate, int nr, const char *new_name)\n+{\n+\tstruct cache_entry *old = istate->cache[nr], *new;\n+\tint namelen = strlen(new_name);\n+\n+\tnew = xmalloc(cache_entry_size(namelen));\n+\tcopy_cache_entry(new, old);\n+\tnew->ce_flags = (new->ce_flags & ~CE_NAMEMASK) | namelen;\n+\tmemcpy(new->name, new_name, namelen);\n+\n+\tcache_tree_invalidate_path(istate->cache_tree, old->name);\n+\tremove_index_entry_at(istate, nr);\n+\tadd_index_entry(istate, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n+}\n+\n /*\n  * This only updates the \"non-critical\" parts of the directory\n  * cache, ie the parts that aren't tracked by GIT, and only used\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 336cfaa..6b615f8 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -156,4 +156,61 @@ test_expect_success 'absolute pathname outside should fail' '(\n \n )'\n \n+# git mv meets angry Git maintainer\n+\n+test_expect_success 'git mv should not change sha1 of moved cache entry' '\n+\n+\trm -fr .git &&\n+\tgit init &&\n+\techo 1 >dirty &&\n+\tgit add dirty &&\n+\tentry=\"$(git ls-files --stage dirty | cut -f 1)\"\n+\tgit mv dirty dirty2 &&\n+\t[ \"$entry\" = \"$(git ls-files --stage dirty2 | cut -f 1)\" ] &&\n+\techo 2 >dirty2 &&\n+\tgit mv dirty2 dirty &&\n+\t[ \"$entry\" = \"$(git ls-files --stage dirty | cut -f 1)\" ]\n+\n+'\n+\n+rm -f dirty dirty2\n+\n+test_expect_failure 'git mv should overwrite symlink to a file' '\n+\n+\trm -fr .git &&\n+\tgit init &&\n+\techo 1 >moved &&\n+\tln -s moved symlink &&\n+\tgit add moved symlink &&\n+\t! git mv moved symlink &&\n+\tgit mv -f moved symlink &&\n+\t[ ! -e moved ] &&\n+\t[ -f symlink ] &&\n+\t[ $(cat symlink) = 1 ] &&\n+\tgit diff-files --quiet\n+\n+'\n+\n+rm -f moved symlink\n+\n+test_expect_failure 'git mv should follow symlink to a directory' '\n+\n+\trm -fr .git &&\n+\tgit init &&\n+\techo 1 >moved &&\n+\tmkdir -p dir &&\n+\ttouch dir/.keep &&\n+\tln -s dir symlink &&\n+\tgit add moved dir/.keep symlink &&\n+\tgit mv moved symlink &&\n+\t[ ! -e moved ] &&\n+\t[ -f symlink/moved ] &&\n+\t[ $(cat symlink/moved) = 1 ] &&\n+\t[ \"$(ls dir)\" = \"$(ls symlink)\" ] &&\n+\tgit diff-files --quiet\n+\n+'\n+\n+rm -rf moved symlink dir\n+\n test_done\n"},{"id":"83770","messageId":"20080717223324.20900.42967.stgit@localhost","threadId":"14489","inReplyTo":"20080717223036.20680.9672.stgit@localhost","subject":"[PATCH] git mv: Support moving submodules","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-17T22:34:28Z","receivedAt":"2008-07-17T22:34:28Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"This patch adds support for moving submodules to 'git mv', including\nrewriting of the .gitmodules file to reflect the movement.\n\nSigned-off-by: Petr Baudis <pasky@suse.cz>\n---\n\n  This is the updated version reflecting the recent rewrite. No other\n  changes in my queue yet (except expanding the git rm explanation).\n\n builtin-mv.c |   37 +++++++++++++++++++++++++++++++++----\n 1 files changed, 33 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex 28ebc9c..62d0c95 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -9,6 +9,7 @@\n #include \"cache-tree.h\"\n #include \"path-list.h\"\n #include \"parse-options.h\"\n+#include \"submodule.h\"\n \n static const char * const builtin_mv_usage[] = {\n \t\"git mv [options] <source>... <destination>\",\n@@ -49,6 +50,24 @@ static const char *add_slash(const char *path)\n \treturn path;\n }\n \n+static int ce_is_gitlink(int i)\n+{\n+\treturn i < 0 ? 0 : S_ISGITLINK(active_cache[i]->ce_mode);\n+}\n+\n+static void rename_submodule(struct path_list_item *i)\n+{\n+\tchar *key = submodule_by_path(i->path);\n+\n+\tconfig_exclusive_filename = \".gitmodules\";\n+\tif (git_config_set(key, (const char *) i->util))\n+\t\tdie(\"cannot update .gitmodules\");\n+\tconfig_exclusive_filename = NULL;\n+\n+\tfree(key);\n+}\n+\n+\n static struct lock_file lock_file;\n \n int cmd_mv(int argc, const char **argv, const char *prefix)\n@@ -65,6 +84,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tenum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;\n \tstruct stat st;\n \tstruct path_list src_for_dst = {NULL, 0, 0, 0};\n+\t/* .path is source path, .util is destination path */\n+\tstruct path_list submodules = {NULL, 0, 0, 0};\n \n \tgit_config(git_default_config, NULL);\n \n@@ -84,7 +105,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t/* special case: \".\" was normalized to \"\" */\n \t\tdestination = copy_pathspec(dest_path[0], argv, argc, 1);\n \telse if (!lstat(dest_path[0], &st) &&\n-\t\t\tS_ISDIR(st.st_mode)) {\n+\t\t\tS_ISDIR(st.st_mode) &&\n+\t\t\t!ce_is_gitlink(cache_name_pos(dest_path[0], strlen(dest_path[0])))) {\n \t\tdest_path[0] = add_slash(dest_path[0]);\n \t\tdestination = copy_pathspec(dest_path[0], argv, argc, 1);\n \t} else {\n@@ -96,7 +118,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t/* Checking */\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *src = source[i], *dst = destination[i];\n-\t\tint length, src_is_dir;\n+\t\tint length, src_is_dir, src_cache_pos;\n \t\tconst char *bad = NULL;\n \n \t\tif (show_only)\n@@ -111,7 +133,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t} else if ((src_is_dir = S_ISDIR(st.st_mode))\n \t\t\t\t&& lstat(dst, &st) == 0)\n \t\t\tbad = \"cannot move directory over file\";\n-\t\telse if (src_is_dir) {\n+\t\telse if ((src_cache_pos = cache_name_pos(src, length)) < 0 && src_is_dir) {\n \t\t\tconst char *src_w_slash = add_slash(src);\n \t\t\tint len_w_slash = length + 1;\n \t\t\tint first, last;\n@@ -177,7 +199,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t} else\n \t\t\t\t\tbad = \"Cannot overwrite\";\n \t\t\t}\n-\t\t} else if (cache_name_pos(src, length) < 0)\n+\t\t} else if (src_cache_pos < 0)\n \t\t\tbad = \"not under version control\";\n \t\telse if (path_list_has_path(&src_for_dst, dst))\n \t\t\tbad = \"multiple sources for the same target\";\n@@ -214,10 +236,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \n \t\tpos = cache_name_pos(src, strlen(src));\n \t\tassert(pos >= 0);\n+\t\tif (ce_is_gitlink(pos))\n+\t\t\tpath_list_insert(src, &submodules)->util = (void *) dst;\n \t\tif (!show_only)\n \t\t\trename_cache_entry_at(pos, dst);\n \t}\n \n+\tfor (i = 0; i < submodules.nr; i++)\n+\t\trename_submodule(&submodules.items[i]);\n+\tif (submodules.nr > 0 && add_file_to_cache(\".gitmodules\", 0))\n+\t\tdie(\"cannot add new .gitmodules to the index\");\n+\n \tif (active_cache_changed) {\n \t\tif (write_cache(newfd, active_cache, active_nr) ||\n \t\t    commit_locked_index(&lock_file))\n"},{"id":"83981","messageId":"7vej5pwhub.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"20080717223036.20680.9672.stgit@localhost","subject":"Re: [PATCH] git-mv: Keep moved index entries inact","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-19T23:54:20Z","receivedAt":"2008-07-19T23:54:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis <pasky@suse.cz> writes:\n\n> A new test has been added to the testsuite to reflect this change.\n> Also, based on suggestion by Junio about desired symlink behaviour\n> of git mv, I have added two tests for that; however, I do not have\n> need or desire to spend time fixing this, so they are expected\n> to fail for now until someone gets around to fixing that.\n\nWell, somebody would eventually come to help, then ;-).\n\n>  builtin-mv.c  |   62 ++++++++-------------------------------------------------\n>  cache.h       |    2 ++\n>  read-cache.c  |   15 ++++++++++++++\n>  t/t7001-mv.sh |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  4 files changed, 83 insertions(+), 53 deletions(-)\n\nVery nice code reduction, isn't it?\n\n> diff --git a/read-cache.c b/read-cache.c\n> index 1648428..70e5f57 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -38,6 +38,21 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache\n>  \tistate->cache_changed = 1;\n>  }\n>  \n> +void rename_index_entry_at(struct index_state *istate, int nr, const char *new_name)\n> +{\n> +\tstruct cache_entry *old = istate->cache[nr], *new;\n> +\tint namelen = strlen(new_name);\n> +\n> +\tnew = xmalloc(cache_entry_size(namelen));\n> +\tcopy_cache_entry(new, old);\n> +\tnew->ce_flags = (new->ce_flags & ~CE_NAMEMASK) | namelen;\n> +\tmemcpy(new->name, new_name, namelen);\n> +\n> +\tcache_tree_invalidate_path(istate->cache_tree, old->name);\n> +\tremove_index_entry_at(istate, nr);\n> +\tadd_index_entry(istate, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n> +}\n\nHmm, would this use of copy_cache_entry() kosher, I have to wonder.  This\nnew copy of cache entry begins its life unhashed, doesn't it?  Shouldn't\nwe be not copying its hashed/unhashed bits from the old one?\n\nAlso setting of that ce_flags looks wrong when namelen does not fit within\nthe width of CE_NAMEMASK.  Shouldn't it be doing the same thing as\ncreate_ce_flags()?\n\n> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\n> index 336cfaa..6b615f8 100755\n> --- a/t/t7001-mv.sh\n> +++ b/t/t7001-mv.sh\n> @@ -156,4 +156,61 @@ test_expect_success 'absolute pathname outside should fail' '(\n>  \n>  )'\n>  \n> +# git mv meets angry Git maintainer\n\nWhat's this comment about?\n\n> +test_expect_success 'git mv should not change sha1 of moved cache entry' '\n> +\n> +\trm -fr .git &&\n> +\tgit init &&\n> +\techo 1 >dirty &&\n> +\tgit add dirty &&\n> +\tentry=\"$(git ls-files --stage dirty | cut -f 1)\"\n\n\"rev-parse :dirty\"?\n"},{"id":"84114","messageId":"20080721002354.GK10151@machine.or.cz","threadId":"14489","inReplyTo":"7vej5pwhub.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-mv: Keep moved index entries inact","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-21T00:23:54Z","receivedAt":"2008-07-21T00:23:54Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Sat, Jul 19, 2008 at 04:54:20PM -0700, Junio C Hamano wrote:\n> Petr Baudis <pasky@suse.cz> writes:\n> \n> > diff --git a/read-cache.c b/read-cache.c\n> > index 1648428..70e5f57 100644\n> > --- a/read-cache.c\n> > +++ b/read-cache.c\n> > @@ -38,6 +38,21 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache\n> >  \tistate->cache_changed = 1;\n> >  }\n> >  \n> > +void rename_index_entry_at(struct index_state *istate, int nr, const char *new_name)\n> > +{\n> > +\tstruct cache_entry *old = istate->cache[nr], *new;\n> > +\tint namelen = strlen(new_name);\n> > +\n> > +\tnew = xmalloc(cache_entry_size(namelen));\n> > +\tcopy_cache_entry(new, old);\n> > +\tnew->ce_flags = (new->ce_flags & ~CE_NAMEMASK) | namelen;\n> > +\tmemcpy(new->name, new_name, namelen);\n> > +\n> > +\tcache_tree_invalidate_path(istate->cache_tree, old->name);\n> > +\tremove_index_entry_at(istate, nr);\n> > +\tadd_index_entry(istate, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n> > +}\n> \n> Hmm, would this use of copy_cache_entry() kosher, I have to wonder.  This\n> new copy of cache entry begins its life unhashed, doesn't it?  Shouldn't\n> we be not copying its hashed/unhashed bits from the old one?\n> \n> Also setting of that ce_flags looks wrong when namelen does not fit within\n> the width of CE_NAMEMASK.  Shouldn't it be doing the same thing as\n> create_ce_flags()?\n\nI have to admit that I don't clearly understand all the index entry\nintricacies. It is good you didn't see my earlier misguided attempts.\n:-) I have patched the two mistakes you pointed out. It is too bad I\ncannot simply use existing functions for this, but I want to keep a\ndifferent set of traits that either copy_cache_entry() or\ncreate_ce_flags() assumes.\n\n> > diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\n> > index 336cfaa..6b615f8 100755\n> > --- a/t/t7001-mv.sh\n> > +++ b/t/t7001-mv.sh\n> > @@ -156,4 +156,61 @@ test_expect_success 'absolute pathname outside should fail' '(\n> >  \n> >  )'\n> >  \n> > +# git mv meets angry Git maintainer\n> \n> What's this comment about?\n\nOh. Well, you sounded agitated in your original mail, but this actually\njust slipped through. :-)\n\n> > +test_expect_success 'git mv should not change sha1 of moved cache entry' '\n> > +\n> > +\trm -fr .git &&\n> > +\tgit init &&\n> > +\techo 1 >dirty &&\n> > +\tgit add dirty &&\n> > +\tentry=\"$(git ls-files --stage dirty | cut -f 1)\"\n> \n> \"rev-parse :dirty\"?\n\nI want to make sure the whole index entry is intact, not just the sha1.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nAs in certain cults it is possible to kill a process if you know\nits true name.  -- Ken Thompson and Dennis M. Ritchie\n"},{"id":"84115","messageId":"20080721002508.26773.92277.stgit@localhost","threadId":"14489","inReplyTo":"20080721002354.GK10151@machine.or.cz","subject":"[PATCHv2] git-mv: Keep moved index entries inact","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-21T00:25:56Z","receivedAt":"2008-07-21T00:25:56Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"The rewrite of git-mv from a shell script to a builtin was perhaps\na little too straightforward: the git add and git rm queues were\nemulated directly, which resulted in a rather complicated code and\ncaused an inconsistent behaviour when moving dirty index entries;\ngit mv would update the entry based on working tree state,\nexcept in case of overwrites, where the new entry would still have\nsha1 of the old file.\n\nThis patch introduces rename_index_entry_at() into the index toolkit,\nwhich will rename an entry while removing any entries the new entry\nmight render duplicate. This is then used in git mv instead\nof all the file queues, resulting in a major simplification\nof the code and an inevitable change in git mv -n output format.\n\nA new test has been added to the testsuite to reflect this change.\nAlso, based on suggestion by Junio about desired symlink behaviour\nof git mv, I have added two tests for that; however, I do not have\nneed or desire to spend time fixing this, so they are expected\nto fail for now until someone gets around to fixing that.\n\nSigned-off-by: Petr Baudis <pasky@suse.cz>\n---\n\n builtin-mv.c  |   62 ++++++++-------------------------------------------------\n cache.h       |    2 ++\n read-cache.c  |   17 ++++++++++++++++\n t/t7001-mv.sh |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 83 insertions(+), 53 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex 33ad082..28ebc9c 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -36,17 +36,6 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,\n \treturn get_pathspec(prefix, result);\n }\n \n-static void show_list(const char *label, struct path_list *list)\n-{\n-\tif (list->nr > 0) {\n-\t\tint i;\n-\t\tprintf(\"%s\", label);\n-\t\tfor (i = 0; i < list->nr; i++)\n-\t\t\tprintf(\"%s%s\", i > 0 ? \", \" : \"\", list->items[i].path);\n-\t\tputchar('\\n');\n-\t}\n-}\n-\n static const char *add_slash(const char *path)\n {\n \tint len = strlen(path);\n@@ -75,11 +64,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tconst char **source, **destination, **dest_path;\n \tenum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;\n \tstruct stat st;\n-\tstruct path_list overwritten = {NULL, 0, 0, 0};\n \tstruct path_list src_for_dst = {NULL, 0, 0, 0};\n-\tstruct path_list added = {NULL, 0, 0, 0};\n-\tstruct path_list deleted = {NULL, 0, 0, 0};\n-\tstruct path_list changed = {NULL, 0, 0, 0};\n \n \tgit_config(git_default_config, NULL);\n \n@@ -189,7 +174,6 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t\t\" will overwrite!\\n\",\n \t\t\t\t\t\t\tbad);\n \t\t\t\t\tbad = NULL;\n-\t\t\t\t\tpath_list_insert(dst, &overwritten);\n \t\t\t\t} else\n \t\t\t\t\tbad = \"Cannot overwrite\";\n \t\t\t}\n@@ -218,6 +202,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *src = source[i], *dst = destination[i];\n \t\tenum update_mode mode = modes[i];\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@@ -227,45 +212,16 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\tif (mode == WORKING_DIRECTORY)\n \t\t\tcontinue;\n \n-\t\tassert(cache_name_pos(src, strlen(src)) >= 0);\n-\n-\t\tpath_list_insert(src, &deleted);\n-\t\t/* destination can be a directory with 1 file inside */\n-\t\tif (path_list_has_path(&overwritten, dst))\n-\t\t\tpath_list_insert(dst, &changed);\n-\t\telse\n-\t\t\tpath_list_insert(dst, &added);\n+\t\tpos = cache_name_pos(src, strlen(src));\n+\t\tassert(pos >= 0);\n+\t\tif (!show_only)\n+\t\t\trename_cache_entry_at(pos, dst);\n \t}\n \n-\tif (show_only) {\n-\t\tshow_list(\"Changed  : \", &changed);\n-\t\tshow_list(\"Adding   : \", &added);\n-\t\tshow_list(\"Deleting : \", &deleted);\n-\t} else {\n-\t\tfor (i = 0; i < changed.nr; i++) {\n-\t\t\tconst char *path = changed.items[i].path;\n-\t\t\tint j = cache_name_pos(path, strlen(path));\n-\t\t\tstruct cache_entry *ce = active_cache[j];\n-\n-\t\t\tif (j < 0)\n-\t\t\t\tdie (\"Huh? Cache entry for %s unknown?\", path);\n-\t\t\trefresh_cache_entry(ce, 0);\n-\t\t}\n-\n-\t\tfor (i = 0; i < added.nr; i++) {\n-\t\t\tconst char *path = added.items[i].path;\n-\t\t\tif (add_file_to_cache(path, verbose ? ADD_CACHE_VERBOSE : 0))\n-\t\t\t\tdie(\"updating index entries failed\");\n-\t\t}\n-\n-\t\tfor (i = 0; i < deleted.nr; i++)\n-\t\t\tremove_file_from_cache(deleted.items[i].path);\n-\n-\t\tif (active_cache_changed) {\n-\t\t\tif (write_cache(newfd, active_cache, active_nr) ||\n-\t\t\t    commit_locked_index(&lock_file))\n-\t\t\t\tdie(\"Unable to write new index file\");\n-\t\t}\n+\tif (active_cache_changed) {\n+\t\tif (write_cache(newfd, active_cache, active_nr) ||\n+\t\t    commit_locked_index(&lock_file))\n+\t\t\tdie(\"Unable to write new index file\");\n \t}\n \n \treturn 0;\ndiff --git a/cache.h b/cache.h\nindex a779d92..6f1d003 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -260,6 +260,7 @@ static inline void remove_name_hash(struct cache_entry *ce)\n #define unmerged_cache() unmerged_index(&the_index)\n #define cache_name_pos(name, namelen) index_name_pos(&the_index,(name),(namelen))\n #define add_cache_entry(ce, option) add_index_entry(&the_index, (ce), (option))\n+#define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))\n #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))\n #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))\n #define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags))\n@@ -370,6 +371,7 @@ extern int index_name_pos(const struct index_state *, const char *name, int name\n #define ADD_CACHE_JUST_APPEND 8\t\t/* Append only; tree.c::read_tree() */\n extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);\n extern struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really);\n+extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);\n extern int remove_index_entry_at(struct index_state *, int pos);\n extern int remove_file_from_index(struct index_state *, const char *path);\n #define ADD_CACHE_VERBOSE 1\ndiff --git a/read-cache.c b/read-cache.c\nindex 1648428..e93ee3c 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -38,6 +38,23 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache\n \tistate->cache_changed = 1;\n }\n \n+void rename_index_entry_at(struct index_state *istate, int nr, const char *new_name)\n+{\n+\tstruct cache_entry *old = istate->cache[nr], *new;\n+\tint namelen = strlen(new_name);\n+\n+\tnew = xmalloc(cache_entry_size(namelen));\n+\tcopy_cache_entry(new, old);\n+\tnew->ce_flags = (new->ce_flags & ~CE_HASHED) | CE_UNHASHED;\n+\tnew->ce_flags = (new->ce_flags & ~CE_NAMEMASK)\n+\t                | (namelen >= CE_NAMEMASK ? CE_NAMEMASK : namelen);\n+\tmemcpy(new->name, new_name, namelen);\n+\n+\tcache_tree_invalidate_path(istate->cache_tree, old->name);\n+\tremove_index_entry_at(istate, nr);\n+\tadd_index_entry(istate, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n+}\n+\n /*\n  * This only updates the \"non-critical\" parts of the directory\n  * cache, ie the parts that aren't tracked by GIT, and only used\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 336cfaa..7e47931 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -156,4 +156,59 @@ test_expect_success 'absolute pathname outside should fail' '(\n \n )'\n \n+test_expect_success 'git mv should not change sha1 of moved cache entry' '\n+\n+\trm -fr .git &&\n+\tgit init &&\n+\techo 1 >dirty &&\n+\tgit add dirty &&\n+\tentry=\"$(git ls-files --stage dirty | cut -f 1)\"\n+\tgit mv dirty dirty2 &&\n+\t[ \"$entry\" = \"$(git ls-files --stage dirty2 | cut -f 1)\" ] &&\n+\techo 2 >dirty2 &&\n+\tgit mv dirty2 dirty &&\n+\t[ \"$entry\" = \"$(git ls-files --stage dirty | cut -f 1)\" ]\n+\n+'\n+\n+rm -f dirty dirty2\n+\n+test_expect_failure 'git mv should overwrite symlink to a file' '\n+\n+\trm -fr .git &&\n+\tgit init &&\n+\techo 1 >moved &&\n+\tln -s moved symlink &&\n+\tgit add moved symlink &&\n+\t! git mv moved symlink &&\n+\tgit mv -f moved symlink &&\n+\t[ ! -e moved ] &&\n+\t[ -f symlink ] &&\n+\t[ $(cat symlink) = 1 ] &&\n+\tgit diff-files --quiet\n+\n+'\n+\n+rm -f moved symlink\n+\n+test_expect_failure 'git mv should follow symlink to a directory' '\n+\n+\trm -fr .git &&\n+\tgit init &&\n+\techo 1 >moved &&\n+\tmkdir -p dir &&\n+\ttouch dir/.keep &&\n+\tln -s dir symlink &&\n+\tgit add moved dir/.keep symlink &&\n+\tgit mv moved symlink &&\n+\t[ ! -e moved ] &&\n+\t[ -f symlink/moved ] &&\n+\t[ $(cat symlink/moved) = 1 ] &&\n+\t[ \"$(ls dir)\" = \"$(ls symlink)\" ] &&\n+\tgit diff-files --quiet\n+\n+'\n+\n+rm -rf moved symlink dir\n+\n test_done\n"},{"id":"84123","messageId":"alpine.DEB.1.00.0807210319410.3305@eeepc-johanness","threadId":"14489","inReplyTo":"20080721002354.GK10151@machine.or.cz","subject":"Re: [PATCH] git-mv: Keep moved index entries inact","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-21T01:20:46Z","receivedAt":"2008-07-21T01:20:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 21 Jul 2008, Petr Baudis wrote:\n\n> On Sat, Jul 19, 2008 at 04:54:20PM -0700, Junio C Hamano wrote:\n> > Petr Baudis <pasky@suse.cz> writes:\n> > \n> > > +test_expect_success 'git mv should not change sha1 of moved cache entry' '\n> > > +\n> > > +\trm -fr .git &&\n> > > +\tgit init &&\n> > > +\techo 1 >dirty &&\n> > > +\tgit add dirty &&\n> > > +\tentry=\"$(git ls-files --stage dirty | cut -f 1)\"\n> > \n> > \"rev-parse :dirty\"?\n> \n> I want to make sure the whole index entry is intact, not just the sha1.\n\n\"rev-parse :dirty\" will have to read the index to get at the object name \nof \"dirty\".  So there you have your index validation for you.\n\nCiao,\nDscho\n"},{"id":"84136","messageId":"7vd4l7yhsm.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"20080721002508.26773.92277.stgit@localhost","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-21T04:36:57Z","receivedAt":"2008-07-21T04:36:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis <pasky@suse.cz> writes:\n\n> This patch introduces rename_index_entry_at() into the index toolkit,\n> which will rename an entry while removing any entries the new entry\n> might render duplicate. This is then used in git mv instead\n> of all the file queues, resulting in a major simplification\n> of the code and an inevitable change in git mv -n output format.\n>\n> A new test has been added to the testsuite to reflect this change.\n> Also, based on suggestion by Junio about desired symlink behaviour\n> of git mv, I have added two tests for that; however, I do not have\n> need or desire to spend time fixing this, so they are expected\n> to fail for now until someone gets around to fixing that.\n\nI haven't looked into the builtin-mv changes to see how involved that fix\nwould be yet (and I do not use \"mv\" and this is somewhat lower priority\nfor me myself), but I did look at changes to read-cache.c.  I've queued a\nfixed up version in 'pu' for now but I am hoping that we can see some\ncomments from more competent people on the patch and move the review\nresult to 'next' shortly.\n\nThis may be a change in semantics, but after thinking about it a bit more,\nI think this can even go into maintenance series.  IOW, it is really a\nfix.  Nobody sane should have been relying on the old behaviour that new\ncontents of dirty paths are staged in the index sometimes, which was\nsimply just crazy.\n"},{"id":"84150","messageId":"20080721071818.GL10151@machine.or.cz","threadId":"14489","inReplyTo":"alpine.DEB.1.00.0807210319410.3305@eeepc-johanness","subject":"Re: [PATCH] git-mv: Keep moved index entries inact","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-21T07:18:18Z","receivedAt":"2008-07-21T07:18:18Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"  Hi,\n\nOn Mon, Jul 21, 2008 at 03:20:46AM +0200, Johannes Schindelin wrote:\n> On Mon, 21 Jul 2008, Petr Baudis wrote:\n> > I want to make sure the whole index entry is intact, not just the sha1.\n> \n> \"rev-parse :dirty\" will have to read the index to get at the object name \n> of \"dirty\".  So there you have your index validation for you.\n\n  it will test if the entry stays _valid_, but not whether it stays the\n_same_.\n\n\t\t\t\tPetr \"Pasky\" Baudis\n"},{"id":"84151","messageId":"7vk5ffvg9j.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"20080721071818.GL10151@machine.or.cz","subject":"Re: [PATCH] git-mv: Keep moved index entries inact","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-21T07:38:16Z","receivedAt":"2008-07-21T07:38:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis <pasky@suse.cz> writes:\n\n> On Mon, Jul 21, 2008 at 03:20:46AM +0200, Johannes Schindelin wrote:\n>> On Mon, 21 Jul 2008, Petr Baudis wrote:\n>> > I want to make sure the whole index entry is intact, not just the sha1.\n>> \n>> \"rev-parse :dirty\" will have to read the index to get at the object name \n>> of \"dirty\".  So there you have your index validation for you.\n>\n>   it will test if the entry stays _valid_, but not whether it stays the\n> _same_.\n\nYou are right.  You would want to catch breakages like a file losing its\nexecutable bits, which you cannot detect by grabbing \"rev-parse :dirty\"\nand comparing it with the expected value.\n"},{"id":"85019","messageId":"7v8wvpm9cl.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"20080721002508.26773.92277.stgit@localhost","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-26T06:46:02Z","receivedAt":"2008-07-26T06:46:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis <pasky@suse.cz> writes:\n\n> The rewrite of git-mv from a shell script to a builtin was perhaps\n> a little too straightforward: the git add and git rm queues were\n> emulated directly, which resulted in a rather complicated code and\n> caused an inconsistent behaviour when moving dirty index entries;\n> git mv would update the entry based on working tree state,\n> except in case of overwrites, where the new entry would still have\n> sha1 of the old file.\n>\n> This patch introduces rename_index_entry_at() into the index toolkit,\n> which will rename an entry while removing any entries the new entry\n> might render duplicate. This is then used in git mv instead\n> of all the file queues, resulting in a major simplification\n> of the code and an inevitable change in git mv -n output format.\n> ...\n\nThanks.  I think I've managed to fix the rename_index_entry_at() in a\nsatisfactory way, and also made builtin-mv to allow \"mv -f symlink file\"\nand \"mv -f file symlink\".\n\nI do not agree with the semantics of this test seems to want, though.\n\n> +test_expect_failure 'git mv should follow symlink to a directory' '\n> +\n> +\trm -fr .git &&\n> +\tgit init &&\n> +\techo 1 >moved &&\n> +\tmkdir -p dir &&\n> +\ttouch dir/.keep &&\n> +\tln -s dir symlink &&\n> +\tgit add moved dir/.keep symlink &&\n> +\tgit mv moved symlink &&\n> +\t[ ! -e moved ] &&\n> +\t[ -f symlink/moved ] &&\n> +\t[ $(cat symlink/moved) = 1 ] &&\n> +\t[ \"$(ls dir)\" = \"$(ls symlink)\" ] &&\n> +\tgit diff-files --quiet\n> +\n> +'\n\nA symlink to us is just a different kind of blob, and by definition a blob\nis the leaf level of a tree structure that represents the working tree in\nthe index. There won't be anything hanging below it, and when adding\nthings to the index we should not dereference the symlink to see where it\nleads to.\n\nTraditionally we have been loose about this check, and the normal \"git\nadd\" and \"git update-index\" codepath is still forever broken, and we\nallow:\n\n\t$ mkdir dir\n        $ >dir/file\n        $ ln -s dir symlink\n        $ git add symlink/file\n\nbut some codepaths that matter more (because they do larger damage\nunattended, as opposed to the above command sequence that can be avoided\nby user education a bit more easily), such as \"git apply\" and \"git\nread-tree\", have been corrected using has_symlink_leading_path() since mid\n2007.  We would need to follow through c40641b (Optimize symlink/directory\ndetection, 2008-05-09) and further fix \"git add\" and \"git update-index\"\ncodepaths to forbid the above command sequence.\n\nSo my take on the above test piece is that after:\n\n\t>moved\n\tmkdir dir\n        >dir/file\n        ln -s dir symlink\n\tgit add moved dir symlink\n\nThis should fail, as it is an overwrite:\n\n\tgit mv moved symlink\n\nand with \"-f\", the command should simply remove symlink and replace it\nwith a regular file whose contents come from the original \"moved\".\n\nIOW, what a symlink points at should not matter.\n"},{"id":"85167","messageId":"20080727134142.GA10151@machine.or.cz","threadId":"14489","inReplyTo":"7v8wvpm9cl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-27T13:41:42Z","receivedAt":"2008-07-27T13:41:42Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Fri, Jul 25, 2008 at 11:46:02PM -0700, Junio C Hamano wrote:\n> Thanks.  I think I've managed to fix the rename_index_entry_at() in a\n> satisfactory way, and also made builtin-mv to allow \"mv -f symlink file\"\n> and \"mv -f file symlink\".\n\nOh, sorry, I didn't realize there were still problems with the original\none, I would try it on my own in that case.\n\n> So my take on the above test piece is that after:\n> \n> \t>moved\n> \tmkdir dir\n>         >dir/file\n>         ln -s dir symlink\n> \tgit add moved dir symlink\n> \n> This should fail, as it is an overwrite:\n> \n> \tgit mv moved symlink\n> \n> and with \"-f\", the command should simply remove symlink and replace it\n> with a regular file whose contents come from the original \"moved\".\n> \n> IOW, what a symlink points at should not matter.\n\nYou convinced me, yes. (Especially since I started actually using\nsymlinks in some of my projects very recently and this would be the\nexact semantic I would eventually expect as well.)\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nAs in certain cults it is possible to kill a process if you know\nits true name.  -- Ken Thompson and Dennis M. Ritchie\n"},{"id":"85168","messageId":"20080727134704.27534.86520.stgit@localhost","threadId":"14489","inReplyTo":"20080727134142.GA10151@machine.or.cz","subject":"[PATCH] t/t7001-mv.sh: Propose ability to use git-mv on conflicting entries","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-27T13:47:48Z","receivedAt":"2008-07-27T13:47:48Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Currently, git-mv will declare \"not under source control\" on an attempt\nto move a conflicted index entry. This patch adds an expect_failure\ntestcase for this case, since this is an artificial restriction. (However,\nthe scenario is not critical enough for the author to fix right now.)\n\nSigned-off-by: Petr Baudis <pasky@suse.cz>\n---\n\nI don't really know if it is ok to make \"feature requests\" like this by\nadding failing testcases...\n\n t/t7001-mv.sh |   27 +++++++++++++++++++++++++++\n 1 files changed, 27 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 7e47931..241e9a2 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -173,6 +173,33 @@ test_expect_success 'git mv should not change sha1 of moved cache entry' '\n \n rm -f dirty dirty2\n \n+cat >multistage <<EOT\n+100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 1\tstaged\n+100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 2\tstaged\n+100755 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 3\tstaged\n+EOT\n+\n+# Rationale: I cannot git mv around a conflicted file. This is unnecessary\n+# restriction in case another part of conflict resolution requires me to\n+# move the file around.\n+test_expect_failure 'git mv should move all stages of cache entry' '\n+\n+\trm -fr .git &&\n+\tgit init &&\n+\t# git mv requires object to exist in working tree (bug?)\n+\ttouch staged &&\n+\tgit update-index --index-info <multistage &&\n+\tgit ls-files --stage >lsf_output &&\n+\ttest_cmp multistage lsf_output &&\n+\tgit mv staged staged-mv &&\n+\tsed \"s/staged/staged-mv/\" <multistage >multistage-mv &&\n+\tgit ls-files --stage >lsf_output &&\n+\ttest_cmp multistage-mv lsf_output\n+\n+'\n+\n+rm -f multistage multistage-mv lsf_output staged\n+\n test_expect_failure 'git mv should overwrite symlink to a file' '\n \n \trm -fr .git &&\n"},{"id":"85216","messageId":"7v4p6ayfmw.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"20080727134704.27534.86520.stgit@localhost","subject":"Re: [PATCH] t/t7001-mv.sh: Propose ability to use git-mv on conflicting entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-28T01:13:59Z","receivedAt":"2008-07-28T01:13:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis <pasky@suse.cz> writes:\n\n> I don't really know if it is ok to make \"feature requests\" like this by\n> adding failing testcases...\n\nOf course it depends on who you are.  It's not Ok for somebody like you,\nwho is known to be competent, and certainly it is not Ok to do it on a\ncommand that you know you care more about than I do.\n\nI've neglected builtin-mv.c since its introduction, mostly because I never\nsay \"git mv\" myself (in other words, I haven't cared very much how broken\nit was.  For one thing, its indentation style is peculiar and is hard to\nread and maintain).\n\n> +# Rationale: I cannot git mv around a conflicted file. This is unnecessary\n> +# restriction in case another part of conflict resolution requires me to\n> +# move the file around.\n\nYes, I would agree this is a reasonable thing to support.  Something like\nthis patch, perhaps.\n\n---\n builtin-mv.c  |   21 ++++++++++++++-------\n t/t7001-mv.sh |   21 +++++++++++++++++++++\n 2 files changed, 35 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex 4f65b5a..cc9e505 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -96,7 +96,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t/* Checking */\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *src = source[i], *dst = destination[i];\n-\t\tint length, src_is_dir;\n+\t\tint length, src_is_dir, pos;\n \t\tconst char *bad = NULL;\n \n \t\tif (show_only)\n@@ -177,7 +177,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t} else\n \t\t\t\t\tbad = \"Cannot overwrite\";\n \t\t\t}\n-\t\t} else if (cache_name_pos(src, length) < 0)\n+\t\t} else if (((pos = cache_name_pos(src, length)) < 0) &&\n+\t\t\t   strcmp(active_cache[-1 - pos]->name, src))\n \t\t\tbad = \"not under version control\";\n \t\telse if (string_list_has_string(&src_for_dst, dst))\n \t\t\tbad = \"multiple sources for the same target\";\n@@ -202,7 +203,6 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *src = source[i], *dst = destination[i];\n \t\tenum update_mode mode = modes[i];\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@@ -212,10 +212,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\tif (mode == WORKING_DIRECTORY)\n \t\t\tcontinue;\n \n-\t\tpos = cache_name_pos(src, strlen(src));\n-\t\tassert(pos >= 0);\n-\t\tif (!show_only)\n-\t\t\trename_cache_entry_at(pos, dst);\n+\t\tif (!show_only) {\n+\t\t\twhile (1) {\n+\t\t\t\tint pos = cache_name_pos(src, strlen(src));\n+\t\t\t\tif (pos < 0)\n+\t\t\t\t\tpos = -1 - pos;\n+\t\t\t\tif ((active_nr <= pos) ||\n+\t\t\t\t    strcmp(active_cache[pos]->name, src))\n+\t\t\t\t\tbreak;\n+\t\t\t\trename_cache_entry_at(pos, dst);\n+\t\t\t}\n+\t\t}\n \t}\n \n \tif (active_cache_changed) {\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex b0fa407..d538f88 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -173,6 +173,27 @@ test_expect_success 'git mv should not change sha1 of moved cache entry' '\n \n rm -f dirty dirty2\n \n+cat >expect <<\\EOT\n+100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 1\tstaged\n+100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 2\tstaged\n+100755 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 3\tstaged\n+EOT\n+\n+test_expect_success 'git mv should move all stages of cache entry' '\n+\trm -fr .git &&\n+\tgit init &&\n+\t>staged &&\n+\tgit update-index --index-info <expect &&\n+\tgit ls-files --stage >actual &&\n+\ttest_cmp expect actual &&\n+\tgit mv staged staged-mv &&\n+\tsed \"s/staged/staged-mv/\" <expect >expect-2 &&\n+\tgit ls-files --stage >actual &&\n+\ttest_cmp expect-2 actual\n+'\n+\n+rm -f expect expect-2 staged actual staged-mv\n+\n test_expect_success 'git mv should overwrite symlink to a file' '\n \n \trm -fr .git &&\n"},{"id":"85225","messageId":"7vzlo2x0q8.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"7v4p6ayfmw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] t/t7001-mv.sh: Propose ability to use git-mv on conflicting entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-28T01:21:19Z","receivedAt":"2008-07-28T01:21:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> +# Rationale: I cannot git mv around a conflicted file. This is unnecessary\n>> +# restriction in case another part of conflict resolution requires me to\n>> +# move the file around.\n>\n> Yes, I would agree this is a reasonable thing to support.  Something like\n> this patch, perhaps.\n> ...\n\nJust in case if somebody is inclined to test the patch and polish it into\na shape good enough for inclusion...\n\n> @@ -177,7 +177,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t} else\n>  \t\t\t\t\tbad = \"Cannot overwrite\";\n>  \t\t\t}\n> -\t\t} else if (cache_name_pos(src, length) < 0)\n> +\t\t} else if (((pos = cache_name_pos(src, length)) < 0) &&\n> +\t\t\t   strcmp(active_cache[-1 - pos]->name, src))\n\nThere is a bug here; \"-1 - pos\" needs to be checked against active_nr\nbefore strcmp().\n"},{"id":"85295","messageId":"20080728142023.GC6701@neumann","threadId":"14489","inReplyTo":"7v8wvpm9cl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2008-07-28T14:20:24Z","receivedAt":"2008-07-28T14:20:24Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nthere is a race somewhere in these 'git-mv: Keep moved index entries\ninact' changes.\n\nThe test cases 'git mv should overwrite symlink to a file' or 'git mv\nshould overwrite file with a symlink' fail occasionaly.  It's quite\nnon-deterministic:  I have run t7001-mv.sh in a loop (see below) and\none or the other usually fails around 50 runs (but sometimes only\nafter 150).  Adding some tracing echos to the tests shows that both\ntests fail when running 'git diff-files' at the end.\n\nRegards,\nGábor\n\n\n---\n#!/bin/bash\n\nret=0\ni=0\nwhile test $ret = 0 ; do\n        GIT_TEST_OPTS='--verbose --debug' make t7001-mv.sh\n        ret=$?\n        i=$((i+1))\ndone\necho \"Failed at ${i}th run\"\n"},{"id":"85298","messageId":"alpine.DEB.1.00.0807281605330.8986@racer","threadId":"14489","inReplyTo":"20080728142023.GC6701@neumann","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-28T15:06:05Z","receivedAt":"2008-07-28T15:06:05Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 28 Jul 2008, SZEDER Gábor wrote:\n\n> there is a race somewhere in these 'git-mv: Keep moved index entries\n> inact' changes.\n> \n> The test cases 'git mv should overwrite symlink to a file' or 'git mv\n> should overwrite file with a symlink' fail occasionaly.  It's quite\n> non-deterministic:  I have run t7001-mv.sh in a loop (see below) and\n> one or the other usually fails around 50 runs (but sometimes only\n> after 150).  Adding some tracing echos to the tests shows that both\n> tests fail when running 'git diff-files' at the end.\n\nTo make it more convenient to test: with this patch it fails all the time:\n\n-- snipsnap --\n\n t/t7001-mv.sh |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex b0fa407..6699abd 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -180,6 +180,7 @@ test_expect_success 'git mv should overwrite symlink to a file' '\n \techo 1 >moved &&\n \tln -s moved symlink &&\n \tgit add moved symlink &&\n+\tsleep 1 &&\n \ttest_must_fail git mv moved symlink &&\n \tgit mv -f moved symlink &&\n \t! test -e moved &&"},{"id":"85299","messageId":"alpine.DEB.1.00.0807281610270.8986@racer","threadId":"14489","inReplyTo":"alpine.DEB.1.00.0807281605330.8986@racer","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-28T15:14:15Z","receivedAt":"2008-07-28T15:14:15Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 28 Jul 2008, Johannes Schindelin wrote:\n\n> On Mon, 28 Jul 2008, SZEDER Gábor wrote:\n> \n> > there is a race somewhere in these 'git-mv: Keep moved index entries\n> > inact' changes.\n> > \n> > The test cases 'git mv should overwrite symlink to a file' or 'git mv\n> > should overwrite file with a symlink' fail occasionaly.  It's quite\n> > non-deterministic:  I have run t7001-mv.sh in a loop (see below) and\n> > one or the other usually fails around 50 runs (but sometimes only\n> > after 150).  Adding some tracing echos to the tests shows that both\n> > tests fail when running 'git diff-files' at the end.\n> \n> To make it more convenient to test: with this patch it fails all the time:\n\nOoops.  Seems like I changed the test 23 to fail, instead of test 24.  \nHowever, I think it is the same bug: the index is newer by one second, so \nit seems that the patch for builtin-mv.c did not really keep the data \n\"intact\".\n\nNote that a test case should use test-chmtime to force this scenario, not \nsleep a second.\n\nUnfortunately, I already spent my Git time budget for today, so the ball \nis out of my half for now.\n\nCiao,\nDscho\n"},{"id":"85315","messageId":"alpine.DEB.1.00.0807281919400.8986@racer","threadId":"14489","inReplyTo":"alpine.DEB.1.00.0807281610270.8986@racer","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-28T18:24:06Z","receivedAt":"2008-07-28T18:24:06Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 28 Jul 2008, Johannes Schindelin wrote:\n\n> On Mon, 28 Jul 2008, Johannes Schindelin wrote:\n> \n> > On Mon, 28 Jul 2008, SZEDER Gábor wrote:\n> > \n> > > there is a race somewhere in these 'git-mv: Keep moved index entries \n> > > inact' changes.\n> > > \n> > > The test cases 'git mv should overwrite symlink to a file' or 'git \n> > > mv should overwrite file with a symlink' fail occasionaly.  It's \n> > > quite non-deterministic:  I have run t7001-mv.sh in a loop (see \n> > > below) and one or the other usually fails around 50 runs (but \n> > > sometimes only after 150).  Adding some tracing echos to the tests \n> > > shows that both tests fail when running 'git diff-files' at the end.\n> > \n> > To make it more convenient to test: with this patch it fails all the \n> > time:\n> \n> Ooops.  Seems like I changed the test 23 to fail, instead of test 24.  \n> However, I think it is the same bug: the index is newer by one second, \n> so it seems that the patch for builtin-mv.c did not really keep the data \n> \"intact\".\n> \n> Note that a test case should use test-chmtime to force this scenario, \n> not sleep a second.\n> \n> Unfortunately, I already spent my Git time budget for today, so the ball \n> is out of my half for now.\n\nHah!  I had a few minutes, and this is my analysis:\n\nJust try to \"mv\" a file, and look at the _ctime_ before and after.  Yes, \nthat is right, at least on my system (ext3) it _changes_.\n\nSo the test 23 and 24 in t7001-mv.sh are totally bogus.  They purport to \ntest that git-mv retains the whole meta-information in the cache and \ntherefore the index does not need to be updated.\n\nHowever, it _does_ need to be updated, exactly because ctime changed.\n\nOnly that the test failed to test what it tried to test, instead \nsucceeding erroneously, just because the index was racy most of the time \nand got silently updated.\n\nSo, this is the analysis.  The fixes will have to be done by somebody \nelse, because /me goes running now.\n\n(Possible fixes I envisage: update ctime via stat() after rename(), or \njust give up and scrap the whole \"leave cache_entry inact\" thing.)\n\nCiao,\nDscho\n"},{"id":"85323","messageId":"7vwsj5rf48.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"alpine.DEB.1.00.0807281605330.8986@racer","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-28T19:19:19Z","receivedAt":"2008-07-28T19:19:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Mon, 28 Jul 2008, SZEDER Gábor wrote:\n>\n>> there is a race somewhere in these 'git-mv: Keep moved index entries\n>> inact' changes.\n>> \n>> The test cases 'git mv should overwrite symlink to a file' or 'git mv\n>> should overwrite file with a symlink' fail occasionaly.  It's quite\n>> non-deterministic:  I have run t7001-mv.sh in a loop (see below) and\n>> one or the other usually fails around 50 runs (but sometimes only\n>> after 150).  Adding some tracing echos to the tests shows that both\n>> tests fail when running 'git diff-files' at the end.\n>\n> To make it more convenient to test: with this patch it fails all the time:\n\nIt's because we rename(2) but do not read back ctime, and reuse the cached\ndata from the old path that was renamed.  After the failed test that moves\na regular file \"move\" to \"symlink\":\n\n$ stat symlink\n  File: `symlink'\n  Size: 2               Blocks: 8          IO Block: 4096   regular file\nDevice: 30ah/778d       Inode: 18104337    Links: 1\nAccess: (0664/-rw-rw-r--)  Uid: ( 1012/   junio)   Gid: (   40/     src)\nAccess: 2008-07-28 11:49:55.000000000 -0700\nModify: 2008-07-28 11:48:41.000000000 -0700\nChange: 2008-07-28 11:48:42.000000000 -0700\n\nBut the cached stat information looks like this:\n\n$ ../../git-ls-files --stat\nctime=1217270921, mtime=1217270921, ino=18104337, mode=100644, uid=1012, gid=40symlink\n\nWe need to refresh the entry to pick up potential ctime changes.\n\n\n read-cache.c       |    7 ++++++-\n builtin-ls-files.c |   21 +++++++++++++++------\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 1cae361..834096f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -40,7 +40,7 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache\n \n void rename_index_entry_at(struct index_state *istate, int nr, const char *new_name)\n {\n-\tstruct cache_entry *old = istate->cache[nr], *new;\n+\tstruct cache_entry *old = istate->cache[nr], *new, *refreshed;\n \tint namelen = strlen(new_name);\n \n \tnew = xmalloc(cache_entry_size(namelen));\n@@ -51,6 +51,11 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n \n \tcache_tree_invalidate_path(istate->cache_tree, old->name);\n \tremove_index_entry_at(istate, nr);\n+\n+\t/* the renaming could have smudged the ctime */\n+\trefreshed = refresh_cache_entry(new, 0);\n+\tif (refreshed && refreshed != new)\n+\t\tnew = refreshed;\n \tadd_index_entry(istate, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n }\n \ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex e8d568e..a6b30c8 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -16,6 +16,7 @@ static int show_deleted;\n static int show_cached;\n static int show_others;\n static int show_stage;\n+static int show_stat;\n static int show_unmerged;\n static int show_modified;\n static int show_killed;\n@@ -205,16 +206,20 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)\n \t\ttag = alttag;\n \t}\n \n-\tif (!show_stage) {\n-\t\tfputs(tag, stdout);\n-\t} else {\n+\tif (show_stage)\n \t\tprintf(\"%s%06o %s %d\\t\",\n \t\t       tag,\n \t\t       ce->ce_mode,\n \t\t       abbrev ? find_unique_abbrev(ce->sha1,abbrev)\n \t\t\t\t: sha1_to_hex(ce->sha1),\n \t\t       ce_stage(ce));\n-\t}\n+\telse if (show_stat)\n+\t\tprintf(\"ctime=%u, mtime=%u, ino=%u, mode=%o, uid=%u, gid=%u\\t\",\n+\t\t       ce->ce_ctime, ce->ce_mtime, ce->ce_ino,\n+\t\t       ce->ce_mode, ce->ce_uid, ce->ce_gid);\n+\n+\telse\n+\t\tfputs(tag, stdout);\n \twrite_name_quoted(ce->name + offset, stdout, line_terminator);\n }\n \n@@ -235,7 +240,7 @@ static void show_files(struct dir_struct *dir, const char *prefix)\n \t\tif (show_killed)\n \t\t\tshow_killed_files(dir);\n \t}\n-\tif (show_cached | show_stage) {\n+\tif (show_cached | show_stage | show_stat) {\n \t\tfor (i = 0; i < active_nr; i++) {\n \t\t\tstruct cache_entry *ce = active_cache[i];\n \t\t\tint dtype = ce_to_dtype(ce);\n@@ -488,6 +493,10 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \t\t\tshow_stage = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"-S\") || !strcmp(arg, \"--stat\")) {\n+\t\t\tshow_stat = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"-k\") || !strcmp(arg, \"--killed\")) {\n \t\t\tshow_killed = 1;\n \t\t\trequire_work_tree = 1;\n@@ -593,7 +602,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \n \t/* With no flags, we default to showing the cached files */\n \tif (!(show_stage | show_deleted | show_others | show_unmerged |\n-\t      show_killed | show_modified))\n+\t      show_killed | show_modified | show_stat))\n \t\tshow_cached = 1;\n \n \tread_cache();\n"},{"id":"85357","messageId":"alpine.DEB.1.00.0807290137370.2725@eeepc-johanness","threadId":"14489","inReplyTo":"7vwsj5rf48.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-28T23:41:00Z","receivedAt":"2008-07-28T23:41:00Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 28 Jul 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > On Mon, 28 Jul 2008, SZEDER Gábor wrote:\n> >\n> >> there is a race somewhere in these 'git-mv: Keep moved index entries\n> >> inact' changes.\n> >> \n> >> The test cases 'git mv should overwrite symlink to a file' or 'git mv\n> >> should overwrite file with a symlink' fail occasionaly.  It's quite\n> >> non-deterministic:  I have run t7001-mv.sh in a loop (see below) and\n> >> one or the other usually fails around 50 runs (but sometimes only\n> >> after 150).  Adding some tracing echos to the tests shows that both\n> >> tests fail when running 'git diff-files' at the end.\n> >\n> > To make it more convenient to test: with this patch it fails all the time:\n> \n> It's because we rename(2) but do not read back ctime, and reuse the cached\n> data from the old path that was renamed.  After the failed test that moves\n> a regular file \"move\" to \"symlink\":\n> \n> $ stat symlink\n>   File: `symlink'\n>   Size: 2               Blocks: 8          IO Block: 4096   regular file\n> Device: 30ah/778d       Inode: 18104337    Links: 1\n> Access: (0664/-rw-rw-r--)  Uid: ( 1012/   junio)   Gid: (   40/     src)\n> Access: 2008-07-28 11:49:55.000000000 -0700\n> Modify: 2008-07-28 11:48:41.000000000 -0700\n> Change: 2008-07-28 11:48:42.000000000 -0700\n> \n> But the cached stat information looks like this:\n> \n> $ ../../git-ls-files --stat\n> ctime=1217270921, mtime=1217270921, ino=18104337, mode=100644, uid=1012, gid=40symlink\n> \n> We need to refresh the entry to pick up potential ctime changes.\n\nYep.\n\nTested-by: me\n\nBTW I have no idea how we could test for this, short of introducing the \n\"sleep 1\" I did earlier.  Maybe guard it with a TEST_EXPENSIVE_CTIME \nvariable or something similar.  Dunno.\n\nAnd my suggestion to use test-chmtime: please just forget about it, and \njust assume that I had some very good wiid(1) in my pipe(2).\n\nCiao,\nDscho"},{"id":"85361","messageId":"alpine.DEB.1.00.0807290155070.2725@eeepc-johanness","threadId":"14489","inReplyTo":"alpine.DEB.1.00.0807290137370.2725@eeepc-johanness","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-28T23:55:48Z","receivedAt":"2008-07-28T23:55:48Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 29 Jul 2008, Johannes Schindelin wrote:\n\n> BTW I have no idea how we could test for this, short of introducing the \n> \"sleep 1\" I did earlier.  Maybe guard it with a TEST_EXPENSIVE_CTIME \n> variable or something similar.  Dunno.\n\nIOW something like this squashed into your patch:\n\n-- snipsnap --\n\n t/t7001-mv.sh |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex b0fa407..c749059 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -179,6 +179,10 @@ test_expect_success 'git mv should overwrite symlink to a file' '\n \tgit init &&\n \techo 1 >moved &&\n \tln -s moved symlink &&\n+\tif test ! -z \"$TEST_EXPENSIVE_CTIME\"\n+\tthen\n+\t\tsleep 1\n+\tfi &&\n \tgit add moved symlink &&\n \ttest_must_fail git mv moved symlink &&\n \tgit mv -f moved symlink &&\n"},{"id":"85368","messageId":"20080729001751.GH10151@machine.or.cz","threadId":"14489","inReplyTo":"7vwsj5rf48.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-29T00:17:51Z","receivedAt":"2008-07-29T00:17:51Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Mon, Jul 28, 2008 at 12:19:19PM -0700, Junio C Hamano wrote:\n> We need to refresh the entry to pick up potential ctime changes.\n> \n>  read-cache.c       |    7 ++++++-\n>  builtin-ls-files.c |   21 +++++++++++++++------\n>  2 files changed, 21 insertions(+), 7 deletions(-)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index 1cae361..834096f 100644\n\nOops, silly me. Sorry for being slower than you at fixing this. ;-)\n\n> diff --git a/builtin-ls-files.c b/builtin-ls-files.c\n> index e8d568e..a6b30c8 100644\n\nIf you are going to apply this, you might be interested in squashing\nthis:\n\ndiff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt\nindex f43af41..2ead7af 100644\n--- a/Documentation/git-ls-files.txt\n+++ b/Documentation/git-ls-files.txt\n@@ -53,7 +53,14 @@ OPTIONS\n \n -s::\n --stage::\n-\tShow stage files in the output\n+\tShow cached files in an extended format also listing the entry\n+\tmode, sha1 and stage number; see below for details.\n+\n+-S::\n+--stat::\n+\tShow cached files in a special format listing stat information\n+\tstored in the index; this is useful probably only for Git\n+\tdebugging purposes.\n \n --directory::\n \tIf a whole directory is classified as \"other\", show just its\n\n(and even if you aren't, maybe the first part is useful, though it's\nminor.)\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nAs in certain cults it is possible to kill a process if you know\nits true name.  -- Ken Thompson and Dennis M. Ritchie\n"},{"id":"85372","messageId":"7vod4ho6uq.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"20080729001751.GH10151@machine.or.cz","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-29T00:46:05Z","receivedAt":"2008-07-29T00:46:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis <pasky@suse.cz> writes:\n\n> On Mon, Jul 28, 2008 at 12:19:19PM -0700, Junio C Hamano wrote:\n>> We need to refresh the entry to pick up potential ctime changes.\n>> \n>>  read-cache.c       |    7 ++++++-\n>>  builtin-ls-files.c |   21 +++++++++++++++------\n>>  2 files changed, 21 insertions(+), 7 deletions(-)\n>> \n>> diff --git a/read-cache.c b/read-cache.c\n>> index 1cae361..834096f 100644\n>\n> Oops, silly me. Sorry for being slower than you at fixing this. ;-)\n\nI did think about ctime issues when I applied the patch.\n\nrename(2) is hardlink to new name followed by unlink of old name, so\ninternally link count changes twice (and the first \"link to new\" can\nexceed max links and is even allowed to make the whole thing fail); but I\ndo not think of any other reason for this change in ctime.\n"},{"id":"85407","messageId":"7vfxptmfg0.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"7vod4ho6uq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv2] git-mv: Keep moved index entries inact","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-29T05:23:27Z","receivedAt":"2008-07-29T05:23:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Petr Baudis <pasky@suse.cz> writes:\n>\n>> On Mon, Jul 28, 2008 at 12:19:19PM -0700, Junio C Hamano wrote:\n>>> We need to refresh the entry to pick up potential ctime changes.\n>>> \n>>>  read-cache.c       |    7 ++++++-\n>>>  builtin-ls-files.c |   21 +++++++++++++++------\n>>>  2 files changed, 21 insertions(+), 7 deletions(-)\n>>> \n>>> diff --git a/read-cache.c b/read-cache.c\n>>> index 1cae361..834096f 100644\n>>\n>> Oops, silly me. Sorry for being slower than you at fixing this. ;-)\n>\n> I did think about ctime issues when I applied the patch.\n\nActually I changed my mind.\n\nI think it is wrong for something as low-level as rename_index_entry_at()\nto unconditionally look at the working tree and try refreshing the entry.\nThe next caller of this function may not even require a working tree.\n\nI think Dscho is correct; expecting the saved cacheinfo to stay fresh\nacross rename does not make much sense.  What we care about with \"git mv\"\nis that we keep what the user staged, i.e. kind of blob and the contents.\nIt cannot be guaranteed if the index entry is stat clean or not, as\nst_ctime may or may not be updated across rename(2) according to POSIX.\n\nSo let's do this instead.\n\n-- >8 --\nt7001: fix \"git mv\" test \n\nThe test assumed that we can keep the cached stat information fresh across\nrename(2); many filesystems however update st_ctime (and POSIX allows them\nto do so), so that assumption does not hold.  We can explicitly refresh\nthe index for the purpose of the test, as the only thing we are interested\nin is the staged contents and the mode bits are preserved across \"mv\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t7001-mv.sh |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex b0fa407..910a28c 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -185,6 +185,7 @@ test_expect_success 'git mv should overwrite symlink to a file' '\n \t! test -e moved &&\n \ttest -f symlink &&\n \ttest \"$(cat symlink)\" = 1 &&\n+\tgit update-index --refresh &&\n \tgit diff-files --quiet\n \n '\n@@ -202,6 +203,7 @@ test_expect_success 'git mv should overwrite file with a symlink' '\n \tgit mv -f symlink moved &&\n \t! test -e symlink &&\n \ttest -h moved &&\n+\tgit update-index --refresh &&\n \tgit diff-files --quiet\n \n '\n"},{"id":"86152","messageId":"7vej5543v5.fsf_-_@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"7v8wvpm9cl.fsf@gitster.siamese.dyndns.org","subject":"Not going beyond symbolic links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-04T07:49:02Z","receivedAt":"2008-08-04T07:49:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> A symlink to us is just a different kind of blob, and by definition a blob\n> is the leaf level of a tree structure that represents the working tree in\n> the index. There won't be anything hanging below it, and when adding\n> things to the index we should not dereference the symlink to see where it\n> leads to.\n>\n> Traditionally we have been loose about this check, and the normal \"git\n> add\" and \"git update-index\" codepath is still forever broken, and we\n> allow:\n>\n>       $ mkdir dir\n>       $ >dir/file\n>       $ ln -s dir symlink\n>       $ git add symlink/file\n>\n> but some codepaths that matter more (because they do larger damage\n> unattended, as opposed to the above command sequence that can be avoided\n> by user education a bit more easily), such as \"git apply\" and \"git\n> read-tree\", have been corrected using has_symlink_leading_path() since mid\n> 2007.  We would need to follow through c40641b (Optimize symlink/directory\n> detection, 2008-05-09) and further fix \"git add\" and \"git update-index\"\n> codepaths to forbid the above command sequence.\n\nI started to revisit this issue and patched \"git update-index --add\"\nand \"git add\" so far.  Patches follow.\n\nI think we should also check the following and fix them if any of them is\nbroken.  Under the same \"dir/file exists but symlink points at dir\"\nscenario:\n\n * \"git rm symlink/file\" --- should fail without removing dir/file;\n\n * \"git ls-tree $commit -- symlink/file\" should *not* fail if $commit does\n   have \"symlink/file\" in it (iow, we cannot add the logic to get_pathspec());\n\n * \"git ls-files --exclude-standard -o -- symlink/file\" should not talk\n   about \"symlink/file\".\n\nIf we reword the paragraph I quoted at the beginning of this message\nslightly:\n\n\tA gitlink to is is just a leaf level of a tree structure that\n\trepresents the working tree in the index.  There won't be anything\n\thanging below it.\n\nWe would need a similar check to stop at module boundary, just like the\nhelper function we use for these patches, has_symlink_leading_path(),\nstops at a symlink.\n\nSo without further ado...\n"},{"id":"86154","messageId":"7v7iax43qp.fsf_-_@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"7vej5543v5.fsf_-_@gitster.siamese.dyndns.org","subject":"[PATCH 1/2] update-index: refuse to add working tree items beyond symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-04T07:51:42Z","receivedAt":"2008-08-04T07:51:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When \"sym\" is a symbolic link that is inside the working tree, and it\npoints at a directory \"dir\" that has \"path\" in it, \"update-index --add\nsym/path\" used to mistakenly add \"sym/path\" as if \"sym\" were a normal\ndirectory.\n\n\"git apply\", \"git diff\" and \"git merge\" have been taught about this issue\nsome time ago, but \"update-index\" and \"add\" have been left ignorant for\ntoo long.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-update-index.c     |    5 ++++-\n t/t0055-beyond-symlinks.sh |   20 ++++++++++++++++++++\n 2 files changed, 24 insertions(+), 1 deletions(-)\n create mode 100755 t/t0055-beyond-symlinks.sh\n\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex 38eb53c..434cb8e 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -194,6 +194,10 @@ static int process_path(const char *path)\n \tint len;\n \tstruct stat st;\n \n+\tlen = strlen(path);\n+\tif (has_symlink_leading_path(len, path))\n+\t\treturn error(\"'%s' is beyond a symbolic link\", path);\n+\n \t/*\n \t * First things first: get the stat information, to decide\n \t * what to do about the pathname!\n@@ -201,7 +205,6 @@ static int process_path(const char *path)\n \tif (lstat(path, &st) < 0)\n \t\treturn process_lstat_error(path, errno);\n \n-\tlen = strlen(path);\n \tif (S_ISDIR(st.st_mode))\n \t\treturn process_directory(path, len, &st);\n \ndiff --git a/t/t0055-beyond-symlinks.sh b/t/t0055-beyond-symlinks.sh\nnew file mode 100755\nindex 0000000..eb11dd7\n--- /dev/null\n+++ b/t/t0055-beyond-symlinks.sh\n@@ -0,0 +1,20 @@\n+#!/bin/sh\n+\n+test_description='update-index refuses to add beyond symlinks'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\t>a &&\n+\tmkdir b &&\n+\tln -s b c &&\n+\t>c/d &&\n+\tgit update-index --add a b/d\n+'\n+\n+test_expect_success 'update-index --add beyond symlinks' '\n+\ttest_must_fail git update-index --add c/d &&\n+\t! ( git ls-files | grep c/d )\n+'\n+\n+test_done\n-- \n1.6.0.rc1.64.g61192\n"},{"id":"86155","messageId":"7v1w1543p6.fsf_-_@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"7vej5543v5.fsf_-_@gitster.siamese.dyndns.org","subject":"[PATCH 2/2] add: refuse to add working tree items beyond symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-04T07:52:37Z","receivedAt":"2008-08-04T07:52:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is the same fix for the issue of adding \"sym/path\" when \"sym\" is a\nsymblic link that points at a directory \"dir\" with \"path\" in it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-add.c              |   12 +++++++++++-\n dir.c                      |    6 +++++-\n t/t0055-beyond-symlinks.sh |    7 ++++++-\n 3 files changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-add.c b/builtin-add.c\nindex fc3f96e..81b64d7 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -153,6 +153,16 @@ static const char **validate_pathspec(int argc, const char **argv, const char *p\n {\n \tconst char **pathspec = get_pathspec(prefix, argv);\n \n+\tif (pathspec) {\n+\t\tconst char **p;\n+\t\tfor (p = pathspec; *p; p++) {\n+\t\t\tif (has_symlink_leading_path(strlen(*p), *p)) {\n+\t\t\t\tint len = prefix ? strlen(prefix) : 0;\n+\t\t\t\tdie(\"'%s' is beyond a symbolic link\", *p + len);\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \treturn pathspec;\n }\n \n@@ -278,7 +288,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\tfprintf(stderr, \"Maybe you wanted to say 'git add .'?\\n\");\n \t\treturn 0;\n \t}\n-\tpathspec = get_pathspec(prefix, argv);\n+\tpathspec = validate_pathspec(argc, argv, prefix);\n \n \t/*\n \t * If we are adding new files, we need to scan the working\ndiff --git a/dir.c b/dir.c\nindex 29d1d5b..ae7046f 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -727,8 +727,12 @@ static void free_simplify(struct path_simplify *simplify)\n \n int read_directory(struct dir_struct *dir, const char *path, const char *base, int baselen, const char **pathspec)\n {\n-\tstruct path_simplify *simplify = create_simplify(pathspec);\n+\tstruct path_simplify *simplify;\n \n+\tif (has_symlink_leading_path(strlen(path), path))\n+\t\treturn dir->nr;\n+\n+\tsimplify = create_simplify(pathspec);\n \tread_directory_recursive(dir, path, base, baselen, 0, simplify);\n \tfree_simplify(simplify);\n \tqsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);\ndiff --git a/t/t0055-beyond-symlinks.sh b/t/t0055-beyond-symlinks.sh\nindex eb11dd7..b29c37a 100755\n--- a/t/t0055-beyond-symlinks.sh\n+++ b/t/t0055-beyond-symlinks.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='update-index refuses to add beyond symlinks'\n+test_description='update-index and add refuse to add beyond symlinks'\n \n . ./test-lib.sh\n \n@@ -17,4 +17,9 @@ test_expect_success 'update-index --add beyond symlinks' '\n \t! ( git ls-files | grep c/d )\n '\n \n+test_expect_success 'add beyond symlinks' '\n+\ttest_must_fail git add c/d &&\n+\t! ( git ls-files | grep c/d )\n+'\n+\n test_done\n-- \n1.6.0.rc1.64.g61192\n"},{"id":"86226","messageId":"alpine.LFD.1.10.0808041719380.3299@nehalem.linux-foundation.org","threadId":"14489","inReplyTo":"7vej5543v5.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: Not going beyond symbolic links","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-08-05T00:21:03Z","receivedAt":"2008-08-05T00:21:03Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 4 Aug 2008, Junio C Hamano wrote:\n> \n> I started to revisit this issue and patched \"git update-index --add\"\n> and \"git add\" so far.  Patches follow.\n\nPatches look good to me, but did you check the performance impact?\n\nThe rewritten 'has_symlink_leading_path()' should do ok, but it migth \nstill be a huge performance downside to check all the paths for things \nlike \"git add -u\".\n\nI didn't test, though.\n\n\t\tLinus\n"},{"id":"86227","messageId":"7v8wvc2seh.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"alpine.LFD.1.10.0808041719380.3299@nehalem.linux-foundation.org","subject":"Re: Not going beyond symbolic links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-05T00:54:14Z","receivedAt":"2008-08-05T00:54:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Mon, 4 Aug 2008, Junio C Hamano wrote:\n>> \n>> I started to revisit this issue and patched \"git update-index --add\"\n>> and \"git add\" so far.  Patches follow.\n>\n> Patches look good to me, but did you check the performance impact?\n>\n> The rewritten 'has_symlink_leading_path()' should do ok, but it migth \n> still be a huge performance downside to check all the paths for things \n> like \"git add -u\".\n\nNot yet.\n\nI think this is a necessary \"correctness\" thing to do regardless of the\nperformance impact, and adding the logic to stop at submodule boundary\n(aka gitlinks) should come before optimization.\n"},{"id":"86234","messageId":"alpine.LFD.1.10.0808041839130.3299@nehalem.linux-foundation.org","threadId":"14489","inReplyTo":"7v8wvc2seh.fsf@gitster.siamese.dyndns.org","subject":"Re: Not going beyond symbolic links","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-08-05T01:43:13Z","receivedAt":"2008-08-05T01:43:13Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 4 Aug 2008, Junio C Hamano wrote:\n> >\n> > The rewritten 'has_symlink_leading_path()' should do ok, but it migth \n> > still be a huge performance downside to check all the paths for things \n> > like \"git add -u\".\n> \n> Not yet.\n> \n> I think this is a necessary \"correctness\" thing to do regardless of the\n> performance impact, and adding the logic to stop at submodule boundary\n> (aka gitlinks) should come before optimization.\n\nWell, \"performance\" is a feature too, and it's not correct to say that \"X \nshould be fixed before optimization\". If \"X\" slows things down, the \nquestion should be whether it really needs fixing..\n\nYes, we find symlinks when we do _new_ files, but is it really so bad to \nassume that existing directories that we have already added to the index \nare stable? It can easily be seen as a feature too that you can force git \nto ignore the symlink and see it as a real directory.\n\nSo that's why it would be interesting to hear about the performance \nimpact. Because this is definitely not a black-and-white \"one behavior is \nwrong and one behavior is right\".\n\n\t\t\tLinus\n"},{"id":"86236","messageId":"alpine.DEB.1.00.0808050356480.9611@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"14489","inReplyTo":"alpine.LFD.1.10.0808041839130.3299@nehalem.linux-foundation.org","subject":"Re: Not going beyond symbolic links","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-08-05T01:59:59Z","receivedAt":"2008-08-05T01:59:59Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 4 Aug 2008, Linus Torvalds wrote:\n\n> On Mon, 4 Aug 2008, Junio C Hamano wrote:\n> > >\n> > > The rewritten 'has_symlink_leading_path()' should do ok, but it \n> > > migth still be a huge performance downside to check all the paths \n> > > for things like \"git add -u\".\n> > \n> > Not yet.\n> > \n> > I think this is a necessary \"correctness\" thing to do regardless of \n> > the performance impact, and adding the logic to stop at submodule \n> > boundary (aka gitlinks) should come before optimization.\n> \n> Well, \"performance\" is a feature too, and it's not correct to say that \n> \"X should be fixed before optimization\". If \"X\" slows things down, the \n> question should be whether it really needs fixing..\n> \n> Yes, we find symlinks when we do _new_ files, but is it really so bad to \n> assume that existing directories that we have already added to the index \n> are stable? It can easily be seen as a feature too that you can force git \n> to ignore the symlink and see it as a real directory.\n\nActually, whatever you want, it needs fixing.\n\nI vividly remember being quite pissed by Git replacing a symbolic link in \nmy working directory with a directory, and instead of updating the files \nwhich were technically outside of the repository, Git populated that newly \ncreated directory.\n\nHowever, please note that Junio's patch affects git-add, AFAIR, not \ngit-update-index.\n\nCiao,\nDscho\n"},{"id":"86240","messageId":"alpine.LFD.1.10.0808041921530.3299@nehalem.linux-foundation.org","threadId":"14489","inReplyTo":"alpine.DEB.1.00.0808050356480.9611@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: Not going beyond symbolic links","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-08-05T02:28:45Z","receivedAt":"2008-08-05T02:28:45Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 5 Aug 2008, Johannes Schindelin wrote:\n> \n> I vividly remember being quite pissed by Git replacing a symbolic link in \n> my working directory with a directory, and instead of updating the files \n> which were technically outside of the repository, Git populated that newly \n> created directory.\n\nWell, that can cut both ways. For example, I vividly remember a time in \nthe distant past when harddisks were tiny, and I didn't have insanely \nhigh-end hardware, and I was building the X server, but had to split \nthings up over two partitions because each individual partition was \ntoo full.\n\nIOW, sometimes you may _want_ to use symlinks that way, even within one \nproject - with a symlink allowing you to move parts of it around \n\"transparently\".\n\nOf course, these days under Linux we can just use bind mounts, so the use \nof symlinks to stitch together two or more different trees is fairly \nold-fashioned, but is still the only option on some systems or if you \ndon't have root.\n\n(These days harddisks are also generally so big that it never happens. But \non my EeePC laptop, I still end up with two filesystems, 4GB  and 8GB \neach. So it's not inconceivable to be in that kind of situation even \ntoday).\n\n\t\t\tLinus\n"},{"id":"86248","messageId":"7v3alk17yh.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"alpine.LFD.1.10.0808041839130.3299@nehalem.linux-foundation.org","subject":"Re: Not going beyond symbolic links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-05T03:01:10Z","receivedAt":"2008-08-05T03:01:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> ... Because this is definitely not a black-and-white \"one behavior is \n> wrong and one behavior is right\".\n\nI wish I could agree with you that this is a feature, but 16a4c61\n(read-tree -m -u: avoid getting confused by intermediate symlinks.,\n2007-05-10) and 64cab59 (apply: do not get confused by symlinks in the\nmiddle, 2007-05-11) came from real world breakage cases and the root cause\nwas that we were too lenient to allow such a \"feature\" that pretends the\nsymlink not to be there.\n\nRight now, we are being careful only while branch switching and patch\napplication, but the codepaths that add directly to the index (add and\nupdate-index) are not fixed (or \"still has the feature\").\n\nI do not see a clean way to keep such a \"feature\" without hurting users\nwho suffered the bugs these two commits from May 2007 fixed.\n"},{"id":"86249","messageId":"alpine.DEB.1.10.0808042003200.29250@asgard.lang.hm","threadId":"14489","inReplyTo":"7v3alk17yh.fsf@gitster.siamese.dyndns.org","subject":"Re: Not going beyond symbolic links","fromName":"","fromEmail":"david@lang.hm","sentAt":"2008-08-05T03:04:31Z","receivedAt":"2008-08-05T03:04:31Z","isPatch":false,"sender":{"key":"david@lang.hm","avatar":null},"body":"On Mon, 4 Aug 2008, Junio C Hamano wrote:\n\n> Linus Torvalds <torvalds@linux-foundation.org> writes:\n>\n>> ... Because this is definitely not a black-and-white \"one behavior is\n>> wrong and one behavior is right\".\n>\n> I wish I could agree with you that this is a feature, but 16a4c61\n> (read-tree -m -u: avoid getting confused by intermediate symlinks.,\n> 2007-05-10) and 64cab59 (apply: do not get confused by symlinks in the\n> middle, 2007-05-11) came from real world breakage cases and the root cause\n> was that we were too lenient to allow such a \"feature\" that pretends the\n> symlink not to be there.\n>\n> Right now, we are being careful only while branch switching and patch\n> application, but the codepaths that add directly to the index (add and\n> update-index) are not fixed (or \"still has the feature\").\n>\n> I do not see a clean way to keep such a \"feature\" without hurting users\n> who suffered the bugs these two commits from May 2007 fixed.\n\nconfig option?\n\nI think a command line is too much work for too little value, but if the \ncheck could be ignored based on a config option without costing too much \nit may be reasonable.\n\nDavid Lang\n"},{"id":"86251","messageId":"7vprooysst.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"alpine.DEB.1.00.0808050356480.9611@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: Not going beyond symbolic links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-05T04:44:34Z","receivedAt":"2008-08-05T04:44:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> However, please note that Junio's patch affects git-add, AFAIR, not \n> git-update-index.\n\nReally?  I thought I added a test case to cover the plumbing as well...\n"},{"id":"86260","messageId":"7vej54xa80.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"alpine.LFD.1.10.0808041921530.3299@nehalem.linux-foundation.org","subject":"Re: Not going beyond symbolic links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-05T06:11:11Z","receivedAt":"2008-08-05T06:11:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> IOW, sometimes you may _want_ to use symlinks that way, even within one \n> project - with a symlink allowing you to move parts of it around \n> \"transparently\".\n\nWhile I admit that I have managed a large directory split across\npartitions grafted via symlinks in pre-git days myself, ever since you\nstarted \"tracking\" symbolic links with 8ae0a8c (git and symlinks as\ntracked content, 2005-05-05), you have pretty much been committed to\n\"track\" symbolic links.\n\nThis goes even before that commit. The readdir() loop done in show-files.c\nwith 8695c8b (Add \"show-files\" command to show the list of managed (or\nnon-managed) files., 2005-04-11) does not dereference symbolic links\npointing at a directory elsewhere, which we still have as read_directory()\nin dir.c without much change in the basic structure.\n\nI would give some leeway to other people who made comments in this thread,\nwho may not be so familiar with the low-level git codebase, but I have to\nsay that if you claim that dereferencing symbolic links in the middle is a\nfeature, you are not being completely honest, you haven't thought through\nthe issues, and/or you simply forgot the details.  I'd suspect most likely\nit is the last one ;-).\n\nThe thing is, the \"feature\" is not very well supported, even without the\nfixes from last night.  If you have a symlink \"sym\" that points at \"dir\"\nthat has \"file\" in it, and if neither \"sym\" nor \"dir/file\" are tracked,\nyou can \"git add sym/file\" to add it (I called it a bug).\n\nHowever:\n\n (1) starting from the same condition, \"git add .\" does _not_ add it (you\n     get the symbolic link \"sym\" added to your index instead, as well as\n     \"dir/file\");\n\n (2) after you add \"sym/file\" through the bug, if you say \"git add .\", it\n     will be removed from the index and you will instead have \"sym\" and\n     \"dir/file\" (with an ancient git before 1.5.0, you will get \"unable to\n     add sym\" error instead).\n\n (3) after you add \"sym/file\", \"git diff\" will immediately notice that you\n     have removed it (this is a fairly recent fix; 1.5.4.X doesn't notice\n     it).\n\nYou cannot have it as a reliably usable feature without a major surgery,\nand this is fundamental. You simply cannot have it both ways without\ntelling git which symlink is \"tracked\" and which are only there for\nstorage sizing.\n\nIf you seriously want to claim that we support such a feature, you would\nat least need to:\n\n (0) have a way for the user to say, \"the project tree may have a\n     directory D, but I do not want to check it out as a directory because\n     my partition is too small.  Whenever you need to create a directory\n     there and hang a tree underneath, instead create a symlink that\n     points at /export/large/D instead\".  Most likely this information\n     would go to .git/config;\n\n (1) whereever we run \"create leading directories\", we notice and honor\n     the above configuration (mostly entry.c::create_directories() called\n     from entry.c::checkout_entry());\n\n (2) whenever we need to check out a file to path D, instead of\n     recursively remove everything under it, we remove the symlink and\n     deposit the file there (mostly entry.c::remove_subtree() and\n     unpack-trees.c::verify_absent());\n\n (3) whenever we traverse working tree using readdir(), notice that the\n     symbolic link we are looking at is the funny \"pointing elsewhere but\n     this is really a directory\" specified in (0) and recurse into the\n     directory pointed by it (dir.c::read_directory_recursive() but there\n     may be others)\n\n (4) and we teach has_symlink_leading_path() to special case such a path\n     you configured in (0).\n\nI personally do not think adding these to support such a \"feature\" is such\na high priority, and I do not think it is honest to claim we support such\na feature without doing any of the above.  The current reality is that our\nsymlink support is still broken in corner cases, and being able to easily\nadd \"sym/path\" via \"git add\" and \"git update-index --add\" is one of them.\n"},{"id":"86266","messageId":"alpine.DEB.1.00.0808051322300.9611@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"14489","inReplyTo":"7vprooysst.fsf@gitster.siamese.dyndns.org","subject":"Re: Not going beyond symbolic links","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-08-05T11:23:04Z","receivedAt":"2008-08-05T11:23:04Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 4 Aug 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > However, please note that Junio's patch affects git-add, AFAIR, not \n> > git-update-index.\n> \n> Really?  I thought I added a test case to cover the plumbing as well...\n\nRight, I missed that one.  I kinda hoped that only porcelain is affected.\n\nCiao,\nDscho\n"},{"id":"86271","messageId":"20080805125422.GX7008@dpotapov.dyndns.org","threadId":"14489","inReplyTo":"7vej54xa80.fsf@gitster.siamese.dyndns.org","subject":"Re: Not going beyond symbolic links","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-08-05T12:54:22Z","receivedAt":"2008-08-05T12:54:22Z","isPatch":false,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Mon, Aug 04, 2008 at 11:11:11PM -0700, Junio C Hamano wrote:\n> \n> The thing is, the \"feature\" is not very well supported, even without the\n> fixes from last night.  If you have a symlink \"sym\" that points at \"dir\"\n> that has \"file\" in it, and if neither \"sym\" nor \"dir/file\" are tracked,\n> you can \"git add sym/file\" to add it (I called it a bug).\n\nWell, I actually used this feature less than a year ago and did not have\nany problem with that. Not that it is very important for me now, but I had\nmakefiles in a separate build directory, and I used a symlink to move\nbuilding to a tmpfs partion, which sped up building slightly.\n\nObviously, if a symlink points to a directory inside of the repository\nand then you use \"git add sym/file\", it is definitely a mistake. OTOH,\nlet's consider the following situation:\n\ngit init\nmkdir newdir\ntouch newdir/foo\ngit add newdir/foo\ngit commit -m 'add foo'\nmv newdir /tmp/\nln -s /tmp/newdir\ntouch newdir/bar\ngit add newdir/bar\ngit commit -m 'add bar'\ngit ls-files\n\nAnd you can see:\nnewdir/bar\nnewdir/foo\n\nGit does exactly what I want here!\n\nAnyway, I more concern with performance impact of your patch. If it\nis noticeable, then it would be nice to have an option to turn this\ncheck off.\n\nDmitry\n"},{"id":"86280","messageId":"alpine.LFD.1.10.0808051011340.3299@nehalem.linux-foundation.org","threadId":"14489","inReplyTo":"7vej54xa80.fsf@gitster.siamese.dyndns.org","subject":"Re: Not going beyond symbolic links","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-08-05T17:15:20Z","receivedAt":"2008-08-05T17:15:20Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 4 Aug 2008, Junio C Hamano wrote:\n> \n> While I admit that I have managed a large directory split across\n> partitions grafted via symlinks in pre-git days myself, ever since you\n> started \"tracking\" symbolic links with 8ae0a8c (git and symlinks as\n> tracked content, 2005-05-05), you have pretty much been committed to\n> \"track\" symbolic links.\n\nYes, but my point is:\n\n - IF the cost is exorbitant (which was my question that triggered this: \n   it sure as h*ll _would_ have been too high back in the days of the \n   original symlink-matching code) ...\n\n - ... then there really are valid cases that say that we could just call \n   it a feature.\n\nThat's really my whole argument. Saying that \"ok, we will assume that \nexisting paths that git already knows about are stable\" is not really an \nodd feature. It's one we have lived with for years, and it's one that is \nactually pretty dang trivial to work with. Yes, it can cause unexpected \nbehaviour, but if you hit it, that unexpected behavior is actually not \n_that_ hard to work around.\n\nThis is all I'm arguing for. People claim that there are 'correctness' \nissues. I say that 'performance issues' are important, and we can and we \n_should_ take performance issues very seriously. So seriously that we'll \nmake performance a _feature_ if required.\n\n\t\t\tLinus\n"},{"id":"86304","messageId":"7v4p5zui9z.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"20080805125422.GX7008@dpotapov.dyndns.org","subject":"Re: Not going beyond symbolic links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-05T23:57:44Z","receivedAt":"2008-08-05T23:57:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Potapov <dpotapov@gmail.com> writes:\n\n> Obviously, if a symlink points to a directory inside of the repository\n> and then you use \"git add sym/file\", it is definitely a mistake. OTOH,\n> let's consider the following situation:\n>\n> git init\n> mkdir newdir\n> touch newdir/foo\n> git add newdir/foo\n> git commit -m 'add foo'\n> mv newdir /tmp/\n> ln -s /tmp/newdir\n> touch newdir/bar\n> git add newdir/bar\n> git commit -m 'add bar'\n> git ls-files\n>\n> And you can see:\n> newdir/bar\n> newdir/foo\n>\n> Git does exactly what I want here!\n\nNow, do this after the above sequence:\n\n  git diff\n  git add . && git ls-files\n\nand tell me what you see.  Does it show exactly what you want here?\n\nI've already told you what kind of changes you would need to make if you\nreally want to claim this is a feature.\n"},{"id":"86392","messageId":"7vk5etjozt.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"alpine.LFD.1.10.0808041719380.3299@nehalem.linux-foundation.org","subject":"Re: Not going beyond symbolic links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-07T06:52:38Z","receivedAt":"2008-08-07T06:52:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Mon, 4 Aug 2008, Junio C Hamano wrote:\n>> \n>> I started to revisit this issue and patched \"git update-index --add\"\n>> and \"git add\" so far.  Patches follow.\n>\n> Patches look good to me, but did you check the performance impact?\n>\n> The rewritten 'has_symlink_leading_path()' should do ok, but it migth \n> still be a huge performance downside to check all the paths for things \n> like \"git add -u\".\n\nI couldn't quite measure much meaningful performance impact.\n\nMy test repository was the kernel tree at v2.6.27-rc2-20-g685d87f, without\nany build products nor editor temporary crufts.\n\nBy the way, if anybody wants to reproduce this, be careful with the tests\nthat run \"rm -f .git/index\" before adding everything.  After doing so, if\nyou check the result with \"git diff --stat HEAD\", you will notice many\nmissing files --- I almost got a heart attack before inspecting this file:\n\n\t$ cat arch/powerpc/.gitignore\n        include\n\nYes, it excludes 261 already tracked files.  Is it intended?  I dunno.\n\nThe file is there since 06f2138 ([POWERPC] Add files build to .gitignore,\n2006-11-26).  Not that having tracked files in an entirely ignored\ndirectory is a bug, but the .gitignore entry seems to me an Oops waiting\nto happen.  There are three commits that affect this directory that is\nentirely ignored after that entry is added:\n\n    b5b9309 (remove unnecessary <linux/hdreg.h> includes, 2008-08-05)\n    9c4cb82 (powerpc: Remove use of CONFIG_PPC_MERGE, 2008-08-02)\n    b8b572e (powerpc: Move include files to arch/powerpc/include/asm, 2008-08-01)\n\nAnyhow, back on topic.  Here are the numbers.\n\nTest #1: With fully up-to-date .git/index, \"git add .\", best of 5 runs\n\n* v1.6.0-rc2\n0.20user 0.20system 0:00.41elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+3622minor)pagefaults 0swaps\n\n* v1.6.0-rc1-73-g725b060 (with patch)\n0.22user 0.19system 0:00.41elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+3635minor)pagefaults 0swaps\n\n\nTest #2: After \"rm -f .git/index\", \"git add .\", best of 5 runs\n\n* v1.6.0-rc2\n1.81user 0.55system 0:02.51elapsed 93%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+92855minor)pagefaults 0swaps\n\n* v1.6.0-rc1-73-g725b060 (with patch)\n1.76user 0.56system 0:02.58elapsed 89%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+92852minor)pagefaults 0swaps\n\n\nTest #3: same as #2, after dropping cache with \"echo 3 > /proc/sys/vm/drop_caches\".\n(Yes, I have slow disks).\n\n* v1.6.0-rc2\n3.43user 2.29system 2:02.45elapsed 4%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (29611major+63244minor)pagefaults 0swaps\n\n* v1.6.0-rc1-73-g725b060 (with patch)\n3.55user 2.33system 1:54.77elapsed 5%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (29616major+63236minor)pagefaults 0swaps\n\nTest #4: same as #1, \"strace -c -e lstat\"\n\n* v1.6.0-rc2\n% time     seconds  usecs/call     calls    errors syscall\n------ ----------- ----------- --------- --------- ----------------\n100.00    0.000247           0     23993           lstat\n\n* v1.6.0-rc1-73-g725b060 (with patch)\n% time     seconds  usecs/call     calls    errors syscall\n------ ----------- ----------- --------- --------- ----------------\n100.00    0.000261           0     23993           lstat\n\nTest #5: same as #2, \"strace -c -e lstat\"\n\n* v1.6.0-rc2\n% time     seconds  usecs/call     calls    errors syscall\n------ ----------- ----------- --------- --------- ----------------\n100.00    0.000639           0     23993           lstat\n\n* v1.6.0-rc1-73-g725b060 (with patch)\n% time     seconds  usecs/call     calls    errors syscall\n------ ----------- ----------- --------- --------- ----------------\n100.00    0.000427           0     23993           lstat\n"},{"id":"86547","messageId":"7vod43b519.fsf@gitster.siamese.dyndns.org","threadId":"14489","inReplyTo":"7vk5etjozt.fsf@gitster.siamese.dyndns.org","subject":"Re: Not going beyond symbolic links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-08T20:55:30Z","receivedAt":"2008-08-08T20:55:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Linus Torvalds <torvalds@linux-foundation.org> writes:\n>\n>> On Mon, 4 Aug 2008, Junio C Hamano wrote:\n>>> \n>>> I started to revisit this issue and patched \"git update-index --add\"\n>>> and \"git add\" so far.  Patches follow.\n>>\n>> Patches look good to me, but did you check the performance impact?\n>>\n>> The rewritten 'has_symlink_leading_path()' should do ok, but it migth \n>> still be a huge performance downside to check all the paths for things \n>> like \"git add -u\".\n>\n> I couldn't quite measure much meaningful performance impact.\n>\n> My test repository was the kernel tree at v2.6.27-rc2-20-g685d87f, without\n> any build products nor editor temporary crufts.\n>\n> By the way, if anybody wants to reproduce this, be careful with the tests\n> that run \"rm -f .git/index\" before adding everything.  After doing so, if\n> you check the result with \"git diff --stat HEAD\", you will notice many\n> missing files --- I almost got a heart attack before inspecting this file:\n>\n> \t$ cat arch/powerpc/.gitignore\n>         include\n>\n> Yes, it excludes 261 already tracked files.  Is it intended?  I dunno.\n> ...\n\nSeeing that you applied my arch/powerpc/.gitignore patch to the kernel\n(Yaay, I now have a short-log entry in the kernel history ;-), you have\nseen the message with some benchmarks I am replying to as well?\n\nI actually was expecting to see measurable slowdown by checking leading\nsymbolic link more often, but I couldn't, which means either there is not\nmuch downside that the bugfix would hurt real-life usage, or I was too\nincompetent to come up with an example to show that the bugfix actually\nhurts the performance.\n\nIf the former, I think it is a very good news (If the latter, well, we\nhave a bigger problem :-<), as the affected codepaths are exactly the ones\nthat need to be fixed when somebody tries to add files in a submodule\ndirectory by mistake, which is an issue (iirc) Dscho had a separate patch\nto fix earlier.  Tracked symbolic links may be rare, but it seems that\nmany people are using submodules, and that case cannot be hand-waved\naround by calling it is a feature.\n"},{"id":"86560","messageId":"alpine.LFD.1.10.0808081639540.3462@nehalem.linux-foundation.org","threadId":"14489","inReplyTo":"7vod43b519.fsf@gitster.siamese.dyndns.org","subject":"Re: Not going beyond symbolic links","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-08-08T23:45:26Z","receivedAt":"2008-08-08T23:45:26Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 8 Aug 2008, Junio C Hamano wrote:\n> \n> Seeing that you applied my arch/powerpc/.gitignore patch to the kernel\n> (Yaay, I now have a short-log entry in the kernel history ;-), you have\n> seen the message with some benchmarks I am replying to as well?\n\nYes, I just felt that closed the discussion.\n\nI only brought up the issue in the first place because I worried about the \nperformance impact. And I was unhappy about how that worry was dismissed \nas being less important than some specious \"correctness\" issue (for the \nlast 3+ years, performance has mattered a _lot_, and the claimed big \n\"correctness\" issue has not mattered one whit).\n\nThe thing is, sometimes \"pi = 3.14\" is (a) infinitely faster than the \n\"correct\" answer and (b) the difference between the \"correct\" and the \n\"wrong\" answer is meaningless. And this is why I get upset when somebody \ndismisses performance issues based on \"correctness\".\n\nThe thing is, some specious value of \"correctness\" is often irrelevant \nbecause it doesn't matter. While performance almost _always_ matters. And \nI absolutely _detest_ the fact that people so often dismiss performance \nconcerns so readily.\n\nBut once the performance numbers are in and they don't show any issues, I \nthink that simply settles the original query, and I'm happy.\n\n\t\tLinus\n"}]}