threads / patch / 31967

patchTeach rm to remove submodules when given with a trailing '/'

Subject: [PATCH] Teach rm to remove submodules when given with a trailing '/'

## tl;dr

5 messages between Oct 28, 2012 and Nov 22, 2012. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Jens Lehmann· Oct 28, 2012, 23:28 UTC · lore
Doing a "git rm submod/" on a submodule results in an error:
	fatal: pathspec 'submod/' did not match any files
This is really inconvenient as e.g. using TAB completion in a shell on a
submodule automatically adds the trailing '/' when it completes the path
of the submodule directory. The user has then to remove the '/' herself to
make a "git rm" succeed. Doing a "git rm -r somedir/" is working fine, so
there is no reason why that shouldn't work for submodules too.

Teach git rm to not error out when a '/' is appended to the path of a submodule. Achieve this by chopping off trailing slashes from the path names given if they represent directories. Add tests to make sure that logic only applies to directories and not to files.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

This patch applies on top of the jl/submodule-rm branch merged into current next.

 builtin/rm.c  |  7 +++++++
 t/t3600-rm.sh | 17 +++++++++++++++++
 2 files changed, 24 insertions(+)
Show changes to 2 files +24 −0

builtin/rm.c, t/t3600-rm.sh

diff --git a/builtin/rm.c b/builtin/rm.c
index 2aea3b5..5381d3f 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -234,6 +234,13 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (read_cache() < 0)
 		die(_("index file corrupt"));

+	/* Remove trailing '/' from directories to find submodules in the index */
+	for (i = 0; i < argc; i++) {
+		size_t pathlen = strlen(argv[i]);
+		if (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1] == '/'))
+			argv[i] = xmemdupz(argv[i], pathlen - 1);
+	}
+
 	pathspec = get_pathspec(prefix, argv);
 	refresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 97254e8..06f6384 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -302,6 +302,23 @@ test_expect_success 'rm removes work tree of unmodified submodules' '
 	test_cmp expect actual
 '

+test_expect_success 'rm removes a submodule with a trailing /' '
+	git reset --hard &&
+	git submodule update &&
+	git rm submod/ &&
+	test ! -d submod &&
+	git status -s -uno --ignore-submodules=none > actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rm fails when given a file with a trailing /' '
+	test_must_fail git rm empty/
+'
+
+test_expect_success 'rm succeeds when given a directory with a trailing /' '
+	git rm -r frotz/
+'
+
 test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' '
 	git reset --hard &&
 	git submodule update &&

-- 
1.8.0.42.g3346551
Johannes Sixt· Oct 29, 2012, 07:11 UTC · re: Jens Lehmann · lore

Re: [PATCH] Teach rm to remove submodules when given with a trailing '/'

Am 10/29/2012 0:28, schrieb Jens Lehmann:
Show 9 quoted lines
> +	/* Remove trailing '/' from directories to find submodules in the index */
> +	for (i = 0; i < argc; i++) {
> +		size_t pathlen = strlen(argv[i]);
> +		if (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1] == '/'))
> +			argv[i] = xmemdupz(argv[i], pathlen - 1);
> +	}
> +
>  	pathspec = get_pathspec(prefix, argv);
>  	refresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);

That's wrong: Either move the check below get_pathspec() (which normalizes backslashes to forward-slashes on Windows) or use is_dir_sep().

But isn't it somewhat dangerous to check pathspec for existance in the worktree without interpreting them? Think of magic pathspec syntax (that we do not have yet, but which may materialize sometime in the future).

-- Hannes
Jens Lehmann· Oct 30, 2012, 21:28 UTC · re: Johannes Sixt · lore

Re: [PATCH] Teach rm to remove submodules when given with a trailing '/'

Am 29.10.2012 08:11, schrieb Johannes Sixt:
Show 13 quoted lines
> Am 10/29/2012 0:28, schrieb Jens Lehmann:
>> +	/* Remove trailing '/' from directories to find submodules in the index */
>> +	for (i = 0; i < argc; i++) {
>> +		size_t pathlen = strlen(argv[i]);
>> +		if (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1] == '/'))
>> +			argv[i] = xmemdupz(argv[i], pathlen - 1);
>> +	}
>> +
>>  	pathspec = get_pathspec(prefix, argv);
>>  	refresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);
> 
> That's wrong: Either move the check below get_pathspec() (which normalizes
> backslashes to forward-slashes on Windows) or use is_dir_sep().
Thanks for bringing this up.
> But isn't it somewhat dangerous to check pathspec for existance in the
> worktree without interpreting them? Think of magic pathspec syntax (that
> we do not have yet, but which may materialize sometime in the future).

I have to admit I'm not aware of magic pathspec syntax. Do you happen to have any pointers where I could look at code doing similar things right?

Johannes Sixt· Oct 31, 2012, 06:29 UTC · re: Jens Lehmann · lore

Re: [PATCH] Teach rm to remove submodules when given with a trailing '/'

Am 10/30/2012 22:28, schrieb Jens Lehmann:
Show 23 quoted lines
> Am 29.10.2012 08:11, schrieb Johannes Sixt:
>> Am 10/29/2012 0:28, schrieb Jens Lehmann:
>>> +	/* Remove trailing '/' from directories to find submodules in the index */
>>> +	for (i = 0; i < argc; i++) {
>>> +		size_t pathlen = strlen(argv[i]);
>>> +		if (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1] == '/'))
>>> +			argv[i] = xmemdupz(argv[i], pathlen - 1);
>>> +	}
>>> +
>>>  	pathspec = get_pathspec(prefix, argv);
>>>  	refresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);
>>
>> That's wrong: Either move the check below get_pathspec() (which normalizes
>> backslashes to forward-slashes on Windows) or use is_dir_sep().
> 
> Thanks for bringing this up.
> 
>> But isn't it somewhat dangerous to check pathspec for existance in the
>> worktree without interpreting them? Think of magic pathspec syntax (that
>> we do not have yet, but which may materialize sometime in the future).
> 
> I have to admit I'm not aware of magic pathspec syntax. Do you happen to
> have any pointers where I could look at code doing similar things right?

cmd_mv() in builtin/mv.c looks like a good candidate. It has to check whether the destination (the last argument) is a directory.

-- Hannes
Jens Lehmann· Nov 22, 2012, 22:32 UTC · re: Johannes Sixt · lore

[PATCH v2] Teach rm to remove submodules when given with a trailing '/'

Doing a "git rm submod/" on a submodule results in an error:
	fatal: pathspec 'submod/' did not match any files
This is really inconvenient as e.g. using TAB completion in a shell on a
submodule automatically adds the trailing '/' when it completes the path
of the submodule directory. The user has then to remove the '/' herself to
make a "git rm" succeed. Doing a "git rm -r somedir/" is working fine, so
there is no reason why that shouldn't work for submodules too.

Teach git rm to not error out when a '/' is appended to the path of a submodule. Achieve this by chopping off trailing slashes from the path names given if they represent directories. Add tests to make sure that logic only applies to directories and not to files.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
Am 31.10.2012 07:29, schrieb Johannes Sixt:
Show 17 quoted lines
> Am 10/30/2012 22:28, schrieb Jens Lehmann:
>> Am 29.10.2012 08:11, schrieb Johannes Sixt:
>>> Am 10/29/2012 0:28, schrieb Jens Lehmann:
>>>> +	/* Remove trailing '/' from directories to find submodules in the index */
>>>> +	for (i = 0; i < argc; i++) {
>>>> +		size_t pathlen = strlen(argv[i]);
>>>> +		if (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1] == '/'))
>>>> +			argv[i] = xmemdupz(argv[i], pathlen - 1);
>>>> +	}
>>>> +
>>>>  	pathspec = get_pathspec(prefix, argv);
>>>>  	refresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);
>>>
>>> That's wrong: Either move the check below get_pathspec() (which normalizes
>>> backslashes to forward-slashes on Windows) or use is_dir_sep().
>>
>> Thanks for bringing this up.

And sorry for taking so long to follow up on this, but I'm currently pretty occupied with real life issues and am rather short on git time.

Show 9 quoted lines
>>> But isn't it somewhat dangerous to check pathspec for existance in the
>>> worktree without interpreting them? Think of magic pathspec syntax (that
>>> we do not have yet, but which may materialize sometime in the future).
>>
>> I have to admit I'm not aware of magic pathspec syntax. Do you happen to
>> have any pointers where I could look at code doing similar things right?
> 
> cmd_mv() in builtin/mv.c looks like a good candidate. It has to check
> whether the destination (the last argument) is a directory.

Thanks, that was a good pointer (and judging from the comment just before prefix_pathspec() in setup.c I currently can't do /that/ much about the magic pathspec syntax as it has not materialized yet ;-).

I was thinking about reusing the copy_pathspec() function from mv.c but came to the conclusion not to do so because it a) doesn't care if a '/' is following a file (where I believe it should not be dropped but mv should error out, but it currently doesn't) and b) it also has the base_name argument I don't need (and which seems to be rather special to the needs of mv.c). So I coded that loop myself using is_dir_sep() (taking care that is_directory() will only be called on paths which have a trailing directory separator).

 builtin/rm.c  | 15 +++++++++++++++
 t/t3600-rm.sh | 17 +++++++++++++++++
 2 files changed, 32 insertions(+)
Show changes to 2 files +32 −0

builtin/rm.c, t/t3600-rm.sh

diff --git a/builtin/rm.c b/builtin/rm.c
index 2aea3b5..dabfcf6 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -234,6 +234,21 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (read_cache() < 0)
 		die(_("index file corrupt"));

+	/*
+	 * Drop trailing directory separators from directories so we'll find
+	 * submodules in the index.
+	 */
+	for (i = 0; i < argc; i++) {
+		size_t pathlen = strlen(argv[i]);
+		if (pathlen && is_dir_sep(argv[i][pathlen - 1]) &&
+		    is_directory(argv[i])) {
+			do {
+				pathlen--;
+			} while (pathlen && is_dir_sep(argv[i][pathlen - 1]));
+			argv[i] = xmemdupz(argv[i], pathlen);
+		}
+	}
+
 	pathspec = get_pathspec(prefix, argv);
 	refresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 97254e8..06f6384 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -302,6 +302,23 @@ test_expect_success 'rm removes work tree of unmodified submodules' '
 	test_cmp expect actual
 '

+test_expect_success 'rm removes a submodule with a trailing /' '
+	git reset --hard &&
+	git submodule update &&
+	git rm submod/ &&
+	test ! -d submod &&
+	git status -s -uno --ignore-submodules=none > actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rm fails when given a file with a trailing /' '
+	test_must_fail git rm empty/
+'
+
+test_expect_success 'rm succeeds when given a directory with a trailing /' '
+	git rm -r frotz/
+'
+
 test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' '
 	git reset --hard &&
 	git submodule update &&

-- 
1.8.0.91.gf07e555

← back to recent threads