# git-mv is broken in master 7 messages from 2006-08-15 to 2006-08-16. Participants: Fredrik Kuivinen, David Rientjes, Johannes Schindelin. Thread: https://gitlist.dev/t/5291 ## Fredrik Kuivinen, 2006-08-15 20:51 Subject: git-mv is broken in master Message-ID: <20060815205150.GA467@c165.ib.student.liu.se> URL: https://gitlist.dev/e/20060815205150.GA467%40c165.ib.student.liu.se ``` Hi, With the current master I get the following: $ git-mv README README-renamed fatal: can not move directory into itself, source=README, destination=README-renamed Which is buggy. The culprit seems to be the test if (!bad && !strncmp(destination[i], source[i], strlen(source[i]))) bad = "can not move directory into itself"; at line ~207 in builtin-mv.c. If the source isn't a prefix of the destination things works as expected, $ git mv README renamed-README $ - Fredrik ``` ## David Rientjes, 2006-08-15 21:02 Subject: Re: git-mv is broken in master Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0608151401510.3965%40chino.corp.google.com In-Reply-To: <20060815205150.GA467@c165.ib.student.liu.se> ``` On Tue, 15 Aug 2006, Fredrik Kuivinen wrote: > With the current master I get the following: > > $ git-mv README README-renamed > fatal: can not move directory into itself, source=README, destination=README-renamed > Please try the following patch. David Signed-off-by: David Rientjes --- builtin-mv.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/builtin-mv.c b/builtin-mv.c index a731f8d..1d11bbb 100644 --- a/builtin-mv.c +++ b/builtin-mv.c @@ -203,8 +203,7 @@ int cmd_mv(int argc, const char **argv, } } - if (!bad && - !strncmp(destination[i], source[i], strlen(source[i]))) + if (!bad && !strcmp(destination[i], source[i])) bad = "can not move directory into itself"; if (!bad && cache_name_pos(source[i], strlen(source[i])) < 0) -- 1.4.2.g460c-dirty ``` ## Johannes Schindelin, 2006-08-16 00:20 Subject: [PATCH] git-mv: succeed even if source is a prefix of destination Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0608160209150.28360%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: ``` As noted by Fredrik Kuivinen, without this patch, git-mv fails on git-mv README README-renamed because "README" is a prefix of "README-renamed". Signed-off-by: Johannes Schindelin --- On Tue, 15 Aug 2006, David Rientjes wrote: > On Tue, 15 Aug 2006, Fredrik Kuivinen wrote: > > With the current master I get the following: > > > > $ git-mv README README-renamed > > fatal: can not move directory into itself, source=README, destination=README-renamed > > > > Please try the following patch. > > - if (!bad && > - !strncmp(destination[i], source[i], strlen(source[i]))) > + if (!bad && !strcmp(destination[i], source[i])) This is not sufficient. It will not catch something like git-mv some/path some/path/and/some/more/ builtin-mv.c | 5 ++++- t/t7001-mv.sh | 4 ++++ 2 files changed, 8 insertions(+), 1 deletions(-) diff --git a/builtin-mv.c b/builtin-mv.c index a731f8d..e7b5eb7 100644 --- a/builtin-mv.c +++ b/builtin-mv.c @@ -119,6 +119,7 @@ int cmd_mv(int argc, const char **argv, /* Checking */ for (i = 0; i < count; i++) { + int length; const char *bad = NULL; if (show_only) @@ -204,7 +205,9 @@ int cmd_mv(int argc, const char **argv, } if (!bad && - !strncmp(destination[i], source[i], strlen(source[i]))) + (length = strlen(source[i])) >= 0 && + !strncmp(destination[i], source[i], length) && + (destination[i][length] == 0 || destination[i][length] == '/')) bad = "can not move directory into itself"; if (!bad && cache_name_pos(source[i], strlen(source[i])) < 0) diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 900ca93..e5e0bb9 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -60,6 +60,10 @@ test_expect_success \ grep -E "^R100.+path0/README.+path2/README"' test_expect_success \ + 'succeed when source is a prefix of destination' \ + 'git-mv path2/COPYING path2/COPYING-renamed' + +test_expect_success \ 'moving whole subdirectory into subdirectory' \ 'git-mv path2 path1' -- 1.4.2.g2e3b ``` ## Fredrik Kuivinen, 2006-08-16 05:49 Subject: Re: [PATCH] git-mv: succeed even if source is a prefix of destination Message-ID: <20060816054944.GA5218@c165.ib.student.liu.se> URL: https://gitlist.dev/e/20060816054944.GA5218%40c165.ib.student.liu.se In-Reply-To: ``` On Wed, Aug 16, 2006 at 02:20:32AM +0200, Johannes Schindelin wrote: > > As noted by Fredrik Kuivinen, without this patch, git-mv fails on > > git-mv README README-renamed > > because "README" is a prefix of "README-renamed". > Thank you. 'git-mv README README-renamed' works for me too now. However, there still seems to be some minor problem with git-mv. $ git mv t t fatal: renaming t failed: Invalid argument $ git mv t t/ fatal: renaming t failed: Invalid argument $ git mv t/ t/ fatal: cannot move directory over file, source=t/, destination=t/ $ git mv t/ t fatal: cannot move directory over file, source=t/, destination=t/ I kind of expected to get 'can not move directory into itself' in all of those cases. At least the same error messages should be given in all cases. It looks like we need some kind of path normalization before we do those tests. - Fredrik ``` ## Johannes Schindelin, 2006-08-16 06:34 Subject: Re: [PATCH] git-mv: succeed even if source is a prefix of destination Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0608160833180.28360%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <20060816054944.GA5218@c165.ib.student.liu.se> ``` Hi, On Wed, 16 Aug 2006, Fredrik Kuivinen wrote: > It looks like we need some kind of path normalization before we do those > tests. Yes, you are right. I hoped I did not need to do this... Working on it. Ciao, Dscho ``` ## Johannes Schindelin, 2006-08-16 08:44 Subject: Re: [PATCH] git-mv: succeed even if source is a prefix of destination Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0608161041300.28360%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <20060816054944.GA5218@c165.ib.student.liu.se> ``` Hi, On Wed, 16 Aug 2006, Fredrik Kuivinen wrote: > On Wed, Aug 16, 2006 at 02:20:32AM +0200, Johannes Schindelin wrote: > > > > As noted by Fredrik Kuivinen, without this patch, git-mv fails on > > > > git-mv README README-renamed > > > > because "README" is a prefix of "README-renamed". > > > > Thank you. 'git-mv README README-renamed' works for me too now. > > However, there still seems to be some minor problem with git-mv. > > $ git mv t t > fatal: renaming t failed: Invalid argument > $ git mv t t/ > fatal: renaming t failed: Invalid argument > $ git mv t/ t/ > fatal: cannot move directory over file, source=t/, destination=t/ > $ git mv t/ t > fatal: cannot move directory over file, source=t/, destination=t/ > > I kind of expected to get 'can not move directory into itself' in all > of those cases. At least the same error messages should be given in > all cases. > > It looks like we need some kind of path normalization before we do > those tests. I kind of hoped it was not necessary to do this, since get_pathspec() does a rudimentary version of it (BTW git-mv.perl got it wrong: it substituted "./" by "", which would fail for a directory name like "endsWithADot."). It was a little more involved: -- 8< -- [PATCH] git-mv: add more path normalization We already use the normalization from get_pathspec(), but now we also remove a trailing slash. So, git mv some_path/ into_some_path/ works now. Also, move the "can not move directory into itself" test before the subdirectory expansion. Signed-off-by: Johannes Schindelin --- builtin-mv.c | 25 ++++++++++++++++--------- 1 files changed, 16 insertions(+), 9 deletions(-) diff --git a/builtin-mv.c b/builtin-mv.c index e7b5eb7..c0c8764 100644 --- a/builtin-mv.c +++ b/builtin-mv.c @@ -17,12 +17,19 @@ static const char builtin_mv_usage[] = static const char **copy_pathspec(const char *prefix, const char **pathspec, int count, int base_name) { + int i; const char **result = xmalloc((count + 1) * sizeof(const char *)); memcpy(result, pathspec, count * sizeof(const char *)); result[count] = NULL; - if (base_name) { - int i; - for (i = 0; i < count; i++) { + for (i = 0; i < count; i++) { + int length = strlen(result[i]); + if (length > 0 && result[i][length - 1] == '/') { + char *without_slash = xmalloc(length); + memcpy(without_slash, result[i], length - 1); + without_slash[length] = '\0'; + result[i] = without_slash; + } + if (base_name) { const char *last_slash = strrchr(result[i], '/'); if (last_slash) result[i] = last_slash + 1; @@ -129,6 +136,12 @@ int cmd_mv(int argc, const char **argv, if (lstat(source[i], &st) < 0) bad = "bad source"; + if (!bad && + (length = strlen(source[i])) >= 0 && + !strncmp(destination[i], source[i], length) && + (destination[i][length] == 0 || destination[i][length] == '/')) + bad = "can not move directory into itself"; + if (S_ISDIR(st.st_mode)) { const char *dir = source[i], *dest_dir = destination[i]; int first, last, len = strlen(dir); @@ -204,12 +217,6 @@ int cmd_mv(int argc, const char **argv, } } - if (!bad && - (length = strlen(source[i])) >= 0 && - !strncmp(destination[i], source[i], length) && - (destination[i][length] == 0 || destination[i][length] == '/')) - bad = "can not move directory into itself"; - if (!bad && cache_name_pos(source[i], strlen(source[i])) < 0) bad = "not under version control"; -- 1.4.2.gf71ee ``` ## Fredrik Kuivinen, 2006-08-16 19:22 Subject: Re: [PATCH] git-mv: succeed even if source is a prefix of destination Message-ID: <20060816192254.GA9575@c165.ib.student.liu.se> URL: https://gitlist.dev/e/20060816192254.GA9575%40c165.ib.student.liu.se In-Reply-To: ``` On Wed, Aug 16, 2006 at 10:44:02AM +0200, Johannes Schindelin wrote: > > > > It looks like we need some kind of path normalization before we do > > those tests. > > I kind of hoped it was not necessary to do this, since get_pathspec() does > a rudimentary version of it (BTW git-mv.perl got it wrong: it substituted > "./" by "", which would fail for a directory name like "endsWithADot."). > > It was a little more involved: > > -- 8< -- > [PATCH] git-mv: add more path normalization > > We already use the normalization from get_pathspec(), but now we also > remove a trailing slash. So, > > git mv some_path/ into_some_path/ > > works now. > > Also, move the "can not move directory into itself" test before the > subdirectory expansion. > It works as expected now. Thanks! - Fredrik ```