git-mv is broken in master

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

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 <rientjes@google.com>
---
 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

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 <redacted>

---

	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

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

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

Hi,

On Wed, 16 Aug 2006, Fredrik Kuivinen wrote:

On Wed, Aug 16, 2006 at 02:20:32AM +0200, Johannes Schindelin wrote:

quoted

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 <redacted>
---
 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

On Wed, Aug 16, 2006 at 10:44:02AM +0200, Johannes Schindelin wrote:

quoted

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