[PATCH 0/3] Add `-p' option to `git-mv', inspired by `mkdir'

[PATCH 0/3] Add `-p' option to `git-mv', inspired by `mkdir'

[Hugo Sales]

Allow passing a new `-p'/`--parents' option to `git-mv' making it so
missing directories in the given target path get created if they don't
exist. This allows running `git mv -p foo bar/quux/' to first create
the `bar/' and `bar/quux/' directories if they don't exist, and then
move `foo' to `bar/quux/foo'.

This is inspired by `mkdir -p foo/bar/quux/' which will create the
`foo/', `foo/bar/', and `foo/bar/quux/' directories if they don't
exist.

Hugo Sales (3):
  mv: Add -p option to create parent directories
  mv: Add tests for new -p flag
  mv: Add documentation for new `-p' flag

 Documentation/git-mv.txt | 12 ++++--
 builtin/mv.c             | 27 +++++++++-----
 t/t7009-mv-parents.sh    | 79 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+), 13 deletions(-)
 create mode 100755 t/t7009-mv-parents.sh


base-commit: 3a06386e314565108ad56a9bdb8f7b80ac52fb69
-- 
2.42.0

[PATCH 1/3] mv: Add -p option to create parent directories

[Hugo Sales]

Inspired by "mkdir -p", this patch allows specifying a "-p" or
"--parents" flag which will create all non-existent directories in the
destination path before renaming the file.

This allows the user to not have to run two commands to move files to a
new directory.

Signed-off-by: Hugo Sales <hugo@hsal.es>
---
 builtin/mv.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index c596515ad0..5d64d86179 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -168,7 +168,7 @@ static int empty_dir_has_sparse_contents(const char *name)
 int cmd_mv(int argc, const char **argv, const char *prefix)
 {
 	int i, flags, gitmodules_modified = 0;
-	int verbose = 0, show_only = 0, force = 0, ignore_errors = 0, ignore_sparse = 0;
+	int verbose = 0, show_only = 0, force = 0, ignore_errors = 0, ignore_sparse = 0, create_parents = 0;
 	struct option builtin_mv_options[] = {
 		OPT__VERBOSE(&verbose, N_("be verbose")),
 		OPT__DRY_RUN(&show_only, N_("dry run")),
@@ -176,6 +176,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			   PARSE_OPT_NOCOMPLETE),
 		OPT_BOOL('k', NULL, &ignore_errors, N_("skip move/rename errors")),
 		OPT_BOOL(0, "sparse", &ignore_sparse, N_("allow updating entries outside of the sparse-checkout cone")),
+		OPT_BOOL('p', "parents", &create_parents, N_("create missing parent directories")),
 		OPT_END(),
 	};
 	const char **source, **destination, **dest_path, **submodule_gitfile;
@@ -220,8 +221,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	if (dest_path[0][0] == '\0')
 		/* special case: "." was normalized to "" */
 		destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
-	else if (!lstat(dest_path[0], &st) &&
-			S_ISDIR(st.st_mode)) {
+	else if (create_parents ||
+		 (!lstat(dest_path[0], &st) && S_ISDIR(st.st_mode))) {
 		destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
 	} else {
 		if (!path_in_sparse_checkout(dst_w_slash, &the_index) &&
@@ -381,7 +382,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			bad = _("multiple sources for the same target");
 			goto act_on_entry;
 		}
-		if (is_dir_sep(dst[strlen(dst) - 1])) {
+
+		if (!create_parents && is_dir_sep(dst[strlen(dst) - 1])) {
 			bad = _("destination directory does not exist");
 			goto act_on_entry;
 		}
@@ -459,11 +461,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		if (show_only)
 			continue;
 		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
-		    !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
-		    rename(src, dst) < 0) {
-			if (ignore_errors)
-				continue;
-			die_errno(_("renaming '%s' failed"), src);
+		    !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE))) {
+			if (create_parents && safe_create_leading_directories_const(dst) < 0) {
+				if (ignore_errors)
+					continue;
+				die_errno(_("creating parent directories for '%s' failed"), dst);
+			}
+
+			if (rename(src, dst) < 0) {
+				if (ignore_errors)
+					continue;
+				die_errno(_("renaming '%s' failed"), src);
+			}
 		}
 		if (submodule_gitfile[i]) {
 			if (!update_path_in_gitmodules(src, dst))
-- 
2.42.0

[PATCH 2/3] mv: Add tests for new -p flag

[Hugo Sales]

Signed-off-by: Hugo Sales <hugo@hsal.es>
---
 t/t7009-mv-parents.sh | 79 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100755 t/t7009-mv-parents.sh

diff --git a/t/t7009-mv-parents.sh b/t/t7009-mv-parents.sh
new file mode 100755
index 0000000000..cc1d9dae08
--- /dev/null
+++ b/t/t7009-mv-parents.sh
@@ -0,0 +1,79 @@
+#!/bin/sh
+
+test_description='git mv -p'
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+test_expect_success 'mv fails to move a file if the target directory does not exist' '
+	echo test >test1 &&
+	git add test1 &&
+	test_must_fail git mv test1 foo/
+'
+
+test_expect_success 'mv fails to move multiple files if the target directory does not exist' '
+	echo test >test2-1 &&
+	echo test >test2-2 &&
+	git add test2-1 test2-2 &&
+	test_must_fail git mv test2-1 test2-2 foo/
+'
+
+test_expect_success 'mv fails to move a file if the target refers to a file in a directory that does not exist' '
+	echo test >test3 &&
+	git add test3 &&
+	test_must_fail git mv test3 foo/test3.txt
+'
+
+test_expect_success 'mv succeeds to move a file even if the target directory does not exist' '
+	echo test >test4 &&
+	git add test4 &&
+	git commit -m test4-commit1 &&
+	git mv -p test4 dir4/ &&
+	git commit -m test4-commit2 &&
+	git diff-tree -r -M --name-status HEAD^ HEAD >test4-actual &&
+	grep "^R100..*test4..*dir4/test4" test4-actual
+'
+
+test_expect_success 'mv succeeds to move multiple files even if the target directory does not exist' '
+	echo test >test5-1 &&
+	echo test >test5-2 &&
+	git add test5-1 test5-2 &&
+	git commit -m test5-commit1 &&
+	git mv -p test5-1 test5-2 dir5/ &&
+	git commit -m test5-commit2 &&
+	git diff-tree -r -M --name-status HEAD^ HEAD >test5-actual &&
+	grep -e "^R100..*test5-1..*dir5/test5-1" -e "^R100..*test5-2..*dir5/test5-2" test5-actual
+'
+
+test_expect_success 'mv succeeds to move a file even if the target refers to a file in a directory that does not exist' '
+	echo test >test6 &&
+	git add test6 &&
+	git commit -m test6-commmit-1 &&
+	git mv -p test6 dir6/test6.txt &&
+	git commit -m test6-commit2 &&
+	git diff-tree -r -M --name-status HEAD^ HEAD >test6-actual &&
+	grep "^R100..*test6..*dir6/test6.txt" test6-actual
+'
+
+test_expect_success 'mv succeeds to move a file even if the target refers to a file in a directory inside a directory that does not exist' '
+	echo test >test7 &&
+	git add test7 &&
+	git commit -m test7-commit1 &&
+	git mv -p test7 dir7/dir7/test7.txt &&
+	git commit -m test7-commit2 &&
+	git diff-tree -r -M --name-status HEAD^ HEAD >test7-actual &&
+	grep "^R100..*test7..*dir7/dir7/test7.txt" test7-actual
+'
+
+test_expect_success 'mv succeeds to move a file even if the target refers to a file in a directory inside a directory inside a directory that does not exist' '
+	echo test >test8 &&
+	git add test8 &&
+	git commit -m test8-commit1 &&
+	git mv -p test8 dir8/dir8/dir8/test8.txt &&
+	git commit -m test8-commit2 &&
+	git diff-tree -r -M --name-status HEAD^ HEAD >test8-actual &&
+	grep "^R100..*test8..*dir8/dir8/dir8/test8.txt" test8-actual
+'
+
+test_done
-- 
2.42.0

[PATCH 3/3] mv: Add documentation for new `-p' flag

[Hugo Sales]

Signed-off-by: Hugo Sales <hugo@hsal.es>
---
 Documentation/git-mv.txt | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
index fb0220fd18..70c1e9572c 100644
--- a/Documentation/git-mv.txt
+++ b/Documentation/git-mv.txt
@@ -15,13 +15,14 @@ DESCRIPTION
 -----------
 Move or rename a file, directory or symlink.
 
- git mv [-v] [-f] [-n] [-k] <source> <destination>
- git mv [-v] [-f] [-n] [-k] <source> ... <destination directory>
+ git mv [-v] [-f] [-n] [-k] [-p] <source> <destination>
+ git mv [-v] [-f] [-n] [-k] [-p] <source> ... <destination directory>
 
 In the first form, it renames <source>, which must exist and be either
 a file, symlink or directory, to <destination>.
-In the second form, the last argument has to be an existing
-directory; the given sources will be moved into this directory.
+In the second form, the last argument refers to a directory, which has
+to exist unless -p is specified, in which case it gets created; the
+given sources will be moved into this directory.
 
 The index is updated after successful completion, but the change must still be
 committed.
@@ -39,6 +40,9 @@ OPTIONS
 -n::
 --dry-run::
 	Do nothing; only show what would happen
+-p::
+--parents::
+	Create missing parent directories in the <destination> or <destination directory> path. Similar to `mkdir -p', all missing leading directories are created.
 
 -v::
 --verbose::
-- 
2.42.0

Re: [PATCH 0/3] Add `-p' option to `git-mv', inspired by `mkdir'

[Junio C Hamano]

Hugo Sales <hugo@hsal.es> writes:

> Allow passing a new `-p'/`--parents' option to `git-mv' making it so
> missing directories in the given target path get created if they don't
> exist. This allows running `git mv -p foo bar/quux/' to first create
> the `bar/' and `bar/quux/' directories if they don't exist, and then
> move `foo' to `bar/quux/foo'.
>
> This is inspired by `mkdir -p foo/bar/quux/' which will create the
> `foo/', `foo/bar/', and `foo/bar/quux/' directories if they don't
> exist.

I'll step back and ask a related design question without reading the
patches (yet).

Is there a reason why somebody benefits by us retaining the current
behaviour, where

    $ git mv file no/such/dir/yet/file

fails with "No such file or directory", and they are forced to say
either

    $ mkdir -p no/such/dir/yet
    $ git mv file no/such/dir/yet/file

or

    $ git mv -p file no/such/dir/yet/file

Yes, what I am getting at is if it makes sense to just "fix" the
behaviour of "git mv" about missing leading directories and do what
the end-user wanted to do without requiring "-p".

Regardless of the "do we require, or is it sifficient to imply, the
'-p' option when we lack leading directories?" question, once we
start "auto-creating" the leading directory hierarchy, one worrysome
thing such a new feature introduces is an ambiguity and unexpected
behaviour.  Imagine there is no "no" directory (so nothing under it
exists), and you did this (you do have a regular file "file").

    $ git mv [-p] file no/such/dir/yet

What should happen?  Would we do the equivalent of

    $ mkdir -p no/such/dir && git mv file no/such/dir/yet

or are we doing

    $ mkdir -p no/such/dir/yet && git mv file no/such/dir/yet/file

Both are plausible, and "mkdir -p" does not have such a nasty
ambiguity.  That is what makes me unsure about the new feature
(again, either with required "-p" or with implied "-p").

Thanks.

Re: [PATCH 0/3] Add `-p' option to `git-mv', inspired by `mkdir'

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Regardless of the "do we require, or is it sifficient to imply, the
> '-p' option when we lack leading directories?" question, once we
> start "auto-creating" the leading directory hierarchy, one worrysome
> thing such a new feature introduces is an ambiguity and unexpected
> behaviour. ...
> Both are plausible, and "mkdir -p" does not have such a nasty
> ambiguity.  That is what makes me unsure about the new feature
> (again, either with required "-p" or with implied "-p").

I checked what POSIX, who does give "-p" to mkdir(1), has with
mv(1), and they do not have "-p".  Neither GNU, which tends to add
such "usability" enhancements on top of what everybody else has.

And I think they are being very sensible.  By not adding such an
option to "mv", they can sidestep the unnecessary ambiguity, and it
isn't too bad to do a "mkdir -p" separately before doing such a "mv"
into a (potential) new directory.

So, let's not do this patch.

Thanks.

Re: [PATCH 0/3] Add `-p' option to `git-mv', inspired by `mkdir'

[Hugo Sales]

> On 10/10/2023 12:39 AM GMT Junio C Hamano <gitster@pobox.com> wrote:
> 
> Is there a reason why somebody benefits by us retaining the current
> behaviour, where
> 
>     $ git mv file no/such/dir/yet/file
> 
> fails with "No such file or directory", and they are forced to say
> either
> 
>     $ mkdir -p no/such/dir/yet
>     $ git mv file no/such/dir/yet/file
> 
> or
> 
>     $ git mv -p file no/such/dir/yet/file

Somewhat, as it could be a typo, so if you actually want to create a new folder, you have to be explicit. I wouldn't be opposed to removing the need for the flag, but if we did, I think we should warn the user that a new directory was created.

> Imagine there is no "no" directory (so nothing under it
> exists), and you did this (you do have a regular file "file").
> 
>     $ git mv [-p] file no/such/dir/yet
> 
> What should happen?  Would we do the equivalent of
> 
>     $ mkdir -p no/such/dir && git mv file no/such/dir/yet
> 
> or are we doing
> 
>     $ mkdir -p no/such/dir/yet && git mv file no/such/dir/yet/file
> 
> Both are plausible, and "mkdir -p" does not have such a nasty
> ambiguity.  That is what makes me unsure about the new feature
> (again, either with required "-p" or with implied "-p").

I think the ambiguity is resolved by the inclusion of lack thereof of a trailing `/`. This is what the implementation already does, too, before this patch. So

     $ git mv [-p] file no/such/dir/yet

would be the same as

     $ mkdir -p no/such/dir && git mv file no/such/dir/yet

and

     $ git mv [-p] file no/such/dir/yet/

would be the same as

     $ mkdir -p no/such/dir/yet && git mv file no/such/dir/yet/

Moving `file` to `no/such/dir/yet/file`

Thanks for the feedback,
Hugo

P.S. Apologies for the duplicated email, Junio, I did a Reply instead of a Reply All

Re: [PATCH 0/3] Add `-p' option to `git-mv', inspired by `mkdir'

[Junio C Hamano]

Hugo Sales <hugo@hsal.es> writes:

>> Both are plausible, and "mkdir -p" does not have such a nasty
>> ambiguity.  That is what makes me unsure about the new feature
>> (again, either with required "-p" or with implied "-p").
>
> I think the ambiguity is resolved by the inclusion of lack thereof
> of a trailing `/`.

The question is not if we can come up with a rule that the user can
use to disambiguate.  It is if the user will find that such a rule
is a naturally acceptable way to disambiguate.

When both of

    git mv file there/exists/such/a/directory
    git mv file there/exists/such/a/directory/

create "there/exists/such/a/directory/file" and removes "file", with
or without a trailing slash, "you should add a slash if you want a
directory, and otherwise you should not add a slash" is a rather
arbitrary rule.  Let's not go there.  I still view the downside more
grave than having to occasionally do "mkdir".

Re: [PATCH 0/3] Add `-p' option to `git-mv', inspired by `mkdir'

[Dragan Simic]

On 2023-10-31 05:30, Junio C Hamano wrote:
> Hugo Sales <hugo@hsal.es> writes:
> 
>>> Both are plausible, and "mkdir -p" does not have such a nasty
>>> ambiguity.  That is what makes me unsure about the new feature
>>> (again, either with required "-p" or with implied "-p").
>> 
>> I think the ambiguity is resolved by the inclusion of lack thereof
>> of a trailing `/`.
> 
> The question is not if we can come up with a rule that the user can
> use to disambiguate.  It is if the user will find that such a rule
> is a naturally acceptable way to disambiguate.
> 
> When both of
> 
>     git mv file there/exists/such/a/directory
>     git mv file there/exists/such/a/directory/
> 
> create "there/exists/such/a/directory/file" and removes "file", with
> or without a trailing slash, "you should add a slash if you want a
> directory, and otherwise you should not add a slash" is a rather
> arbitrary rule.  Let's not go there.  I still view the downside more
> grave than having to occasionally do "mkdir".

Please note that the above-described git-mv operation succeeds with no 
trailing slash if "there/exists/such/a/directory" doesn't already exist 
as a directory, and creates "there/exists/such/a/directory" as a file.  
With the trailing slash, the git-mv operation succeeds only if 
"there/exists/such/a/directory" already exists as a directory, and fails 
otherwise.

A quite similar ambiguity exists in cp(1) in mv(1), which is also 
resolved by the use of the trailing slash character.  However, I've 
encountered only one person aware of that disambiguation, and in cp(1) 
only, but in the "I always include the trailing slash" way, without 
actually understanding it fully.  Maybe I need to encounter more people, 
I don't know.

Re: [PATCH 0/3] Add `-p' option to `git-mv', inspired by `mkdir'

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> A quite similar ambiguity exists in cp(1) in mv(1), which is also
> resolved by the use of the trailing slash character.  However, I've
> encountered only one person aware of that disambiguation, and in cp(1)
> only, but in the "I always include the trailing slash" way, without
> actually understanding it fully.  Maybe I need to encounter more
> people, I don't know.

If the majority of (perhaps new) users you already know find such
disambiguation method unfamiliar, that already is a good anecdata
without any need for you to meet more people to tell us that it is
not a very easy-to-understand thing for them, no?

Re: [PATCH 0/3] Add `-p' option to `git-mv', inspired by `mkdir'

[Dragan Simic]

On 2023-10-31 06:58, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> A quite similar ambiguity exists in cp(1) in mv(1), which is also
>> resolved by the use of the trailing slash character.  However, I've
>> encountered only one person aware of that disambiguation, and in cp(1)
>> only, but in the "I always include the trailing slash" way, without
>> actually understanding it fully.  Maybe I need to encounter more
>> people, I don't know.
> 
> If the majority of (perhaps new) users you already know find such
> disambiguation method unfamiliar, that already is a good anecdata
> without any need for you to meet more people to tell us that it is
> not a very easy-to-understand thing for them, no?

Quite frankly, I'm divided there.  On the one hand, you're right that 
this disambiguation method is a bit confusing and it's absolutely not 
very well known.  On the other hand, it's already out there in the wild, 
including git, and it's actually quite useful when used properly.

If I had to vote, I'd give my vote to embracing this disambiguation 
method, but only with good documentation and some kind of education 
through an article or two.  I believe that proper education simply isn't 
present, or at least not in a user-friendly manner, which should be 
corrected, regardless of the new "git mv -p" feature being accepted or 
not.