{"thread":{"id":"5291","subject":"git-mv is broken in master","startedAt":"2006-08-15T20:51:50Z","lastAt":"2006-08-16T19:22:54Z","messageCount":7,"participants":["Fredrik Kuivinen","David Rientjes","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"25382","messageId":"20060815205150.GA467@c165.ib.student.liu.se","threadId":"5291","inReplyTo":null,"subject":"git-mv is broken in master","fromName":"Fredrik Kuivinen","fromEmail":"freku045@student.liu.se","sentAt":"2006-08-15T20:51:50Z","receivedAt":"2006-08-15T20:51:50Z","isPatch":false,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"Hi,\n\nWith the current master I get the following:\n\n    $ git-mv README README-renamed\n    fatal: can not move directory into itself, source=README, destination=README-renamed\n\nWhich is buggy. The culprit seems to be the test\n\n    if (!bad &&\n        !strncmp(destination[i], source[i], strlen(source[i])))\n            bad = \"can not move directory into itself\";\n\nat line ~207 in builtin-mv.c.\n\n\nIf the source isn't a prefix of the destination things works as expected,\n\n    $ git mv README renamed-README\n    $\n\n- Fredrik\n"},{"id":"25384","messageId":"Pine.LNX.4.63.0608151401510.3965@chino.corp.google.com","threadId":"5291","inReplyTo":"20060815205150.GA467@c165.ib.student.liu.se","subject":"Re: git-mv is broken in master","fromName":"David Rientjes","fromEmail":"rientjes@google.com","sentAt":"2006-08-15T21:02:38Z","receivedAt":"2006-08-15T21:02:38Z","isPatch":false,"sender":{"key":"rientjes@google.com","avatar":null},"body":"On Tue, 15 Aug 2006, Fredrik Kuivinen wrote:\n> With the current master I get the following:\n> \n>     $ git-mv README README-renamed\n>     fatal: can not move directory into itself, source=README, destination=README-renamed\n> \n\nPlease try the following patch.\n\n\t\tDavid\n\nSigned-off-by: David Rientjes <rientjes@google.com>\n---\n builtin-mv.c |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex a731f8d..1d11bbb 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -203,8 +203,7 @@ int cmd_mv(int argc, const char **argv, \n \t\t\t}\n \t\t}\n \n-\t\tif (!bad &&\n-\t\t    !strncmp(destination[i], source[i], strlen(source[i])))\n+\t\tif (!bad && !strcmp(destination[i], source[i]))\n \t\t\tbad = \"can not move directory into itself\";\n \n \t\tif (!bad && cache_name_pos(source[i], strlen(source[i])) < 0)\n-- \n1.4.2.g460c-dirty\n"},{"id":"25394","messageId":"Pine.LNX.4.63.0608160209150.28360@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"5291","inReplyTo":"Pine.LNX.4.63.0608151401510.3965@chino.corp.google.com","subject":"[PATCH] git-mv: succeed even if source is a prefix of destination","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-08-16T00:20:32Z","receivedAt":"2006-08-16T00:20:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nAs noted by Fredrik Kuivinen, without this patch, git-mv fails on\n\n\tgit-mv README README-renamed\n\nbecause \"README\" is a prefix of \"README-renamed\".\n\nSigned-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n\n---\n\n\tOn Tue, 15 Aug 2006, David Rientjes wrote:\n\n\t> On Tue, 15 Aug 2006, Fredrik Kuivinen wrote:\n\t> > With the current master I get the following:\n\t> > \n\t> >     $ git-mv README README-renamed\n\t> >     fatal: can not move directory into itself, source=README, destination=README-renamed\n\t> > \n\t> \n\t> Please try the following patch.\n\t> \n\t> -\t\tif (!bad &&\n\t> -\t\t    !strncmp(destination[i], source[i], strlen(source[i])))\n\t> +\t\tif (!bad && !strcmp(destination[i], source[i]))\n\n\tThis is not sufficient. It will not catch something like\n\n\t\tgit-mv some/path some/path/and/some/more/\n\n\n builtin-mv.c  |    5 ++++-\n t/t7001-mv.sh |    4 ++++\n 2 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex a731f8d..e7b5eb7 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -119,6 +119,7 @@ int cmd_mv(int argc, const char **argv, \n \n \t/* Checking */\n \tfor (i = 0; i < count; i++) {\n+\t\tint length;\n \t\tconst char *bad = NULL;\n \n \t\tif (show_only)\n@@ -204,7 +205,9 @@ int cmd_mv(int argc, const char **argv, \n \t\t}\n \n \t\tif (!bad &&\n-\t\t    !strncmp(destination[i], source[i], strlen(source[i])))\n+\t\t    (length = strlen(source[i])) >= 0 &&\n+\t\t    !strncmp(destination[i], source[i], length) &&\n+\t\t    (destination[i][length] == 0 || destination[i][length] == '/'))\n \t\t\tbad = \"can not move directory into itself\";\n \n \t\tif (!bad && cache_name_pos(source[i], strlen(source[i])) < 0)\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 900ca93..e5e0bb9 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -60,6 +60,10 @@ test_expect_success \\\n      grep -E \"^R100.+path0/README.+path2/README\"'\n \n test_expect_success \\\n+    'succeed when source is a prefix of destination' \\\n+    'git-mv path2/COPYING path2/COPYING-renamed'\n+\n+test_expect_success \\\n     'moving whole subdirectory into subdirectory' \\\n     'git-mv path2 path1'\n \n-- \n1.4.2.g2e3b\n"},{"id":"25397","messageId":"20060816054944.GA5218@c165.ib.student.liu.se","threadId":"5291","inReplyTo":"Pine.LNX.4.63.0608160209150.28360@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] git-mv: succeed even if source is a prefix of destination","fromName":"Fredrik Kuivinen","fromEmail":"freku045@student.liu.se","sentAt":"2006-08-16T05:49:44Z","receivedAt":"2006-08-16T05:49:44Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Wed, Aug 16, 2006 at 02:20:32AM +0200, Johannes Schindelin wrote:\n> \n> As noted by Fredrik Kuivinen, without this patch, git-mv fails on\n> \n> \tgit-mv README README-renamed\n> \n> because \"README\" is a prefix of \"README-renamed\".\n> \n\nThank you. 'git-mv README README-renamed' works for me too now.\n\nHowever, there still seems to be some minor problem with git-mv.\n\n    $ git mv t t\n    fatal: renaming t failed: Invalid argument\n    $ git mv t t/\n    fatal: renaming t failed: Invalid argument\n    $ git mv t/ t/\n    fatal: cannot move directory over file, source=t/, destination=t/\n    $ git mv t/ t \n    fatal: cannot move directory over file, source=t/, destination=t/\n\nI kind of expected to get 'can not move directory into itself' in all\nof those cases. At least the same error messages should be given in\nall cases.\n\nIt looks like we need some kind of path normalization before we do\nthose tests.\n\n- Fredrik\n"},{"id":"25400","messageId":"Pine.LNX.4.63.0608160833180.28360@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"5291","inReplyTo":"20060816054944.GA5218@c165.ib.student.liu.se","subject":"Re: [PATCH] git-mv: succeed even if source is a prefix of destination","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-08-16T06:34:14Z","receivedAt":"2006-08-16T06:34:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 16 Aug 2006, Fredrik Kuivinen wrote:\n\n> It looks like we need some kind of path normalization before we do those \n> tests.\n\nYes, you are right. I hoped I did not need to do this... Working on it.\n\nCiao,\nDscho\n"},{"id":"25408","messageId":"Pine.LNX.4.63.0608161041300.28360@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"5291","inReplyTo":"20060816054944.GA5218@c165.ib.student.liu.se","subject":"Re: [PATCH] git-mv: succeed even if source is a prefix of destination","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-08-16T08:44:02Z","receivedAt":"2006-08-16T08:44:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 16 Aug 2006, Fredrik Kuivinen wrote:\n\n> On Wed, Aug 16, 2006 at 02:20:32AM +0200, Johannes Schindelin wrote:\n> > \n> > As noted by Fredrik Kuivinen, without this patch, git-mv fails on\n> > \n> > \tgit-mv README README-renamed\n> > \n> > because \"README\" is a prefix of \"README-renamed\".\n> > \n> \n> Thank you. 'git-mv README README-renamed' works for me too now.\n> \n> However, there still seems to be some minor problem with git-mv.\n> \n>     $ git mv t t\n>     fatal: renaming t failed: Invalid argument\n>     $ git mv t t/\n>     fatal: renaming t failed: Invalid argument\n>     $ git mv t/ t/\n>     fatal: cannot move directory over file, source=t/, destination=t/\n>     $ git mv t/ t \n>     fatal: cannot move directory over file, source=t/, destination=t/\n> \n> I kind of expected to get 'can not move directory into itself' in all\n> of those cases. At least the same error messages should be given in\n> all cases.\n> \n> It looks like we need some kind of path normalization before we do\n> those tests.\n\nI kind of hoped it was not necessary to do this, since get_pathspec() does \na rudimentary version of it (BTW git-mv.perl got it wrong: it substituted \n\"./\" by \"\", which would fail for a directory name like \"endsWithADot.\").\n\nIt was a little more involved:\n\n-- 8< --\n[PATCH] git-mv: add more path normalization\n\nWe already use the normalization from get_pathspec(), but now we also\nremove a trailing slash. So,\n\n\tgit mv some_path/ into_some_path/\n\nworks now.\n\nAlso, move the \"can not move directory into itself\" test before the\nsubdirectory expansion.\n\nSigned-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n---\n builtin-mv.c |   25 ++++++++++++++++---------\n 1 files changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex e7b5eb7..c0c8764 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -17,12 +17,19 @@ static const char builtin_mv_usage[] =\n static const char **copy_pathspec(const char *prefix, const char **pathspec,\n \t\t\t\t  int count, int base_name)\n {\n+\tint i;\n \tconst char **result = xmalloc((count + 1) * sizeof(const char *));\n \tmemcpy(result, pathspec, count * sizeof(const char *));\n \tresult[count] = NULL;\n-\tif (base_name) {\n-\t\tint i;\n-\t\tfor (i = 0; i < count; i++) {\n+\tfor (i = 0; i < count; i++) {\n+\t\tint length = strlen(result[i]);\n+\t\tif (length > 0 && result[i][length - 1] == '/') {\n+\t\t\tchar *without_slash = xmalloc(length);\n+\t\t\tmemcpy(without_slash, result[i], length - 1);\n+\t\t\twithout_slash[length] = '\\0';\n+\t\t\tresult[i] = without_slash;\n+\t\t}\n+\t\tif (base_name) {\n \t\t\tconst char *last_slash = strrchr(result[i], '/');\n \t\t\tif (last_slash)\n \t\t\t\tresult[i] = last_slash + 1;\n@@ -129,6 +136,12 @@ int cmd_mv(int argc, const char **argv, \n \t\tif (lstat(source[i], &st) < 0)\n \t\t\tbad = \"bad source\";\n \n+\t\tif (!bad &&\n+\t\t    (length = strlen(source[i])) >= 0 &&\n+\t\t    !strncmp(destination[i], source[i], length) &&\n+\t\t    (destination[i][length] == 0 || destination[i][length] == '/'))\n+\t\t\tbad = \"can not move directory into itself\";\n+\n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\tconst char *dir = source[i], *dest_dir = destination[i];\n \t\t\tint first, last, len = strlen(dir);\n@@ -204,12 +217,6 @@ int cmd_mv(int argc, const char **argv, \n \t\t\t}\n \t\t}\n \n-\t\tif (!bad &&\n-\t\t    (length = strlen(source[i])) >= 0 &&\n-\t\t    !strncmp(destination[i], source[i], length) &&\n-\t\t    (destination[i][length] == 0 || destination[i][length] == '/'))\n-\t\t\tbad = \"can not move directory into itself\";\n-\n \t\tif (!bad && cache_name_pos(source[i], strlen(source[i])) < 0)\n \t\t\tbad = \"not under version control\";\n \n-- \n1.4.2.gf71ee\n"},{"id":"25422","messageId":"20060816192254.GA9575@c165.ib.student.liu.se","threadId":"5291","inReplyTo":"Pine.LNX.4.63.0608161041300.28360@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] git-mv: succeed even if source is a prefix of destination","fromName":"Fredrik Kuivinen","fromEmail":"freku045@student.liu.se","sentAt":"2006-08-16T19:22:54Z","receivedAt":"2006-08-16T19:22:54Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Wed, Aug 16, 2006 at 10:44:02AM +0200, Johannes Schindelin wrote:\n> > \n> > It looks like we need some kind of path normalization before we do\n> > those tests.\n> \n> I kind of hoped it was not necessary to do this, since get_pathspec() does \n> a rudimentary version of it (BTW git-mv.perl got it wrong: it substituted \n> \"./\" by \"\", which would fail for a directory name like \"endsWithADot.\").\n> \n> It was a little more involved:\n> \n> -- 8< --\n> [PATCH] git-mv: add more path normalization\n> \n> We already use the normalization from get_pathspec(), but now we also\n> remove a trailing slash. So,\n> \n> \tgit mv some_path/ into_some_path/\n> \n> works now.\n> \n> Also, move the \"can not move directory into itself\" test before the\n> subdirectory expansion.\n> \n\nIt works as expected now. Thanks!\n\n- Fredrik\n"}]}