mv: let 'git mv file no-such-dir/' error out

Git used to trim the trailing slash, and make the command equivalent to 'git mv file no-such-dir', which created the file no-such-dir (while the trailing slash explicitly stated that it could only be a directory).

This patch skips the trailing slash removal for the destination path. The path with its trailing slash is passed to rename(2), which errors out with the appropriate message:

  $ git mv file no-such-dir/
  fatal: renaming 'file' failed: Not a directory

Original-patch-by Duy Nguyen <[email protected]>

Tests, tweaks and commit message by: Matthieu Moy <[email protected]>

Signed-off-by Matthieu Moy <[email protected]>

---
 builtin/mv.c  | 23 ++++++++++++++++-------
 t/t7001-mv.sh | 10 ++++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)

builtin/mv.c +16 -7

index 2e0e61b..08fbc03 100644
@@ -16,9 +16,12 @@ static const char * const builtin_mv_usage[] = {
        NULL
 };
 
+#define DUP_BASENAME 1
+#define KEEP_TRAILING_SLASH 2
+
 static const char **internal_copy_pathspec(const char *prefix,
                                           const char **pathspec,
-                                          int count, int base_name)
+                                          int count, unsigned flags)
 {
        int i;
        const char **result = xmalloc((count + 1) * sizeof(const char *));

@@ -27,11 +30,12 @@ static const char **internal_copy_pathspec(const char *prefix,
        for (i = 0; i < count; i++) {
                int length = strlen(result[i]);
                int to_copy = length;
-               while (to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
+               while (!(flags & KEEP_TRAILING_SLASH) &&
+                      to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
                        to_copy--;
-               if (to_copy != length || base_name) {
+               if (to_copy != length || flags & DUP_BASENAME) {
                        char *it = xmemdupz(result[i], to_copy);
-                       if (base_name) {
+                       if (flags & DUP_BASENAME) {
                                result[i] = xstrdup(basename(it));
                                free(it);
                        } else

@@ -87,16 +91,21 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
        source = internal_copy_pathspec(prefix, argv, argc, 0);
        modes = xcalloc(argc, sizeof(enum update_mode));
-       dest_path = internal_copy_pathspec(prefix, argv + argc, 1, 0);
+       /*
+        * Keep trailing slash, needed to let
+        * "git mv file no-such-dir/" error out.
+        */
+       dest_path = internal_copy_pathspec(prefix, argv + argc, 1,
+                                          KEEP_TRAILING_SLASH);
        submodule_gitfile = xcalloc(argc, sizeof(char *));
 
        if (dest_path[0][0] == '\0')
                /* special case: "." was normalized to "" */
-               destination = internal_copy_pathspec(dest_path[0], argv, argc, 1);
+               destination = internal_copy_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
        else if (!lstat(dest_path[0], &st) &&
                        S_ISDIR(st.st_mode)) {
                dest_path[0] = add_slash(dest_path[0]);
-               destination = internal_copy_pathspec(dest_path[0], argv, argc, 1);
+               destination = internal_copy_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
        } else {
                if (argc != 1)
                        die("destination '%s' is not a directory", dest_path[0]);

t/t7001-mv.sh +10 -1

index b90e985..e5c8084 100755
@@ -72,6 +72,16 @@ rm -f idontexist untracked1 untracked2 \
      .git/index.lock
 
 test_expect_success \
+    'moving to target with trailing slash' \
+    'test_must_fail git mv path0/COPYING no-such-dir/ &&
+     test_must_fail git mv path0/COPYING no-such-dir// &&
+     git mv path0/ no-such-dir/'
+
+test_expect_success \
+    'clean up' \
+    'git reset --hard'
+
+test_expect_success \
     'adding another file' \
     'cp "$TEST_DIRECTORY"/../README path0/README &&
      git add path0/README &&
--

1.8.5.rc3.4.g8bd3721

Quoted 10 lines · Matthieu Moy

Git used to trim the trailing slash, and make the command equivalent to 'git mv file no-such-dir', which created the file no-such-dir (while the trailing slash explicitly stated that it could only be a directory).

This patch skips the trailing slash removal for the destination path. The path with its trailing slash is passed to rename(2), which errors out with the appropriate message:

  $ git mv file no-such-dir/
  fatal: renaming 'file' failed: Not a directory

There's something we probably should check. In d78b0f3 ([PATCH]
git-mv: add more path normalization - 2006-08-16), it mentions about

git mv something/ somewhere/

there's no test in that commit so I don't know the actual input and expected outcome.

If "somewhere" is a directory, it errors out with this patch and works without it.

Quoted 8 lines · Duy Nguyen

On Tue, Dec 3, 2013 at 3:32 PM, Matthieu Moy <[email protected]> wrote:
There's something we probably should check. In d78b0f3 ([PATCH]
git-mv: add more path normalization - 2006-08-16), it mentions about

git mv something/ somewhere/

there's no test in that commit so I don't know the actual input and expected outcome.

To me, the expected outcome is "behave like Unix's mv" (which works with or without the trailing slash if somewhere exists).

Quoted 2 lines

If "somewhere" is a directory, it errors out with this patch and works without it.

I can't reproduce. I've added this to my patch (indeed, the area wasn't well tested), and the tests pass.

Quoted 14 lines · Matthieu Moy

Quoted 8 lines · Duy Nguyen

On Tue, Dec 3, 2013 at 3:32 PM, Matthieu Moy <[email protected]> wrote:
There's something we probably should check. In d78b0f3 ([PATCH]
git-mv: add more path normalization - 2006-08-16), it mentions about

git mv something/ somewhere/

there's no test in that commit so I don't know the actual input and expected outcome.

To me, the expected outcome is "behave like Unix's mv" (which works with or without the trailing slash if somewhere exists).

Quoted 5 lines

Quoted 2 lines

If "somewhere" is a directory, it errors out with this patch and works without it.

I can't reproduce. I've added this to my patch (indeed, the area wasn't well tested), and the tests pass.

Now I can't either. Probably some mis-setups or some local bugs in my tree. Good that we have more tests.

It seems that t7001 needs some face-lifting, by the way. Perhaps after this patch solidifies.

The previous commit c57f628 (mv: let 'git mv file no-such-dir/' error out) relies on that rename("src", "dst/") fails if directory dst does not exist (note the trailing slash). This does not work as expected on Windows:
This rename() call is successful. Insert an explicit check for this case.

This changes the error message from

   $ git mv file no-such-dir/
   fatal: renaming 'file' failed: Not a directory

to

   $ git mv file no-such-dir/
   fatal: destination directory does not exist, source=file, destination=no-such-dir/

Signed-off-by Johannes Sixt <[email protected]>

---
 builtin/mv.c | 2 ++
 1 file changed, 2 insertions(+)

builtin/mv.c +2 -0

index 08fbc03..21c46d1 100644
@@ -214,6 +214,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			}
 		} else if (string_list_has_string(&src_for_dst, dst))
 			bad = _("multiple sources for the same target");
+		else if (is_dir_sep(dst[strlen(dst) - 1]))
+			bad = _("destination directory does not exist");
 		else
 			string_list_insert(&src_for_dst, dst);

Quoted 4 lines · Johannes Sixt

The previous commit c57f628 (mv: let 'git mv file no-such-dir/' error out) relies on that rename("src", "dst/") fails if directory dst does not exist (note the trailing slash). This does not work as expected on Windows:
This rename() call is successful. Insert an explicit check for this case.

Could you care to explain "Successful how" a bit here? Do we see no-such-dir mkdir'ed and then no-such-dir/file created? Do we see file moved to a new file whose name is no-such-dir/? I am guessing that it would be the latter, but if that is the case we would need at least an air-quote around "successful".

Quoted 28 lines

This changes the error message from

   $ git mv file no-such-dir/
   fatal: renaming 'file' failed: Not a directory

to

   $ git mv file no-such-dir/
   fatal: destination directory does not exist, source=file, destination=no-such-dir/

Signed-off-by Johannes Sixt <[email protected]>

---
 builtin/mv.c | 2 ++
 1 file changed, 2 insertions(+)

builtin/mv.c +2 -0

index 08fbc03..21c46d1 100644
@@ -214,6 +214,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			}
 		} else if (string_list_has_string(&src_for_dst, dst))
 			bad = _("multiple sources for the same target");
+		else if (is_dir_sep(dst[strlen(dst) - 1]))
+			bad = _("destination directory does not exist");
 		else
 			string_list_insert(&src_for_dst, dst);

The file is renamed to no-such-dir without the slash at the end. The updated commit message would be:

mv: let 'git mv file no-such-dir/' error out on Windows, too

The previous commit c57f628 (mv: let 'git mv file no-such-dir/' error out) relies on that rename("src", "dst/") fails if directory dst does not exist (note the trailing slash). This does not work as expected on Windows:
The rename() call does not fail, but renames src to dst (without the trailing slash). Insert an explicit check for this case to force an error.

This changes the error message from

   $ git mv file no-such-dir/
   fatal: renaming 'file' failed: Not a directory

to

   $ git mv file no-such-dir/
   fatal: destination directory does not exist, source=file, destination=no-such-dir/

Signed-off-by Johannes Sixt <[email protected]>