New tests are added to the git-rm test case to cover this as well.
Signed-off-by: Carl Worth <cworth@xxxxxxxxxx>
---
On Wed, 22 Feb 2006 00:19:46 -0800, Junio C Hamano wrote:
>
> Note you are not using -z, which means we will c-quote the funny
> characters in the output...
Oh, I didn't expect C-language-style quoting. That's definitely not
going to work.
> > +*)
> > + [[ "$remove_files" = "true" ]] && rm -- $files
>
> Same here. What happens to filenames with IFS letters in them?
> "git-add" does not use -z and xargs -0 without a good reason.
Yeah, this is just me unleashing my shell-programming incompetence on
the world.
The attached patch addresses that problem with a rather blunt
hammer. Let me know if anyone has a more elegant approach than what I
did here.
One thing I've lost is that the previous version had a sort|uniq on
the output of git-ls-files which is useful in the case of failed
merges and other ways in which git-ls-files reports the same file
multiple times. What might be nice is a --unique flag to git-ls-files
that git-rm could use, (but on first glance it doesn't look trivial to
implement as git-ls-files doesn't ever store or sort its entire list).
> Even if rm -- $files were quoted correctly, and tried to remove
> the right files, if some of the files failed to disappear for
> whatever reason, what happens?
My intent with the previous patch was that, when git-rm is given -f,
and the rm fails to remove a file, that the file is then not removed
from the index. Of course, this was hopelessly broken due to two major
typos in the same line:
index_remote_option=--force
instead of:
index_remove_option=--remove
which my tests were insufficient to catch.
This behavior should now work in the current patch as well as the
proper handling of files with funny characters, (tests are included
for both).
The desired behavior when rm fails is debatable, so I'm open to
opinions. One reason I liked this was that in the previous patch, rm
would prompt the user before deleting a read-only file, and if the
user said no, then git-rm would also not remove it from the index.
This did cause another minor problem in that there would then be no
way to get git-rm to use "rm -f" when desired.
In the current patch, with my blunt hammer, there's another sub-shell
before the rm which apparently steals its tty and causes it to not
prompt at all. So that aspect may be moot.
-Carl
PS. What's the syntax/tool support for just replying to an existing
message, and at the end inserting a patch with its own subject and
commit message? Here I've manually whacked the subject and put the
commit message above my reply (in the style of git-format-patch) but
that seem seems inelegant.
git-rm.sh | 37 ++++++++++++++++++++-----------------
t/t3600-rm.sh | 30 ++++++++++++++++++++++--------
2 files changed, 42 insertions(+), 25 deletions(-)
3d52c6d60047390d434f8737368adea77fa26310
diff --git a/git-rm.sh b/git-rm.sh
index 0a3f546..fa361bd 100755
--- a/git-rm.sh
+++ b/git-rm.sh
@@ -4,7 +4,6 @@ USAGE='[-f] [-n] [-v] [--] <file>...'
SUBDIRECTORY_OK='Yes'
. git-sh-setup
-index_remove_option=--force-remove
remove_files=
show_only=
verbose=
@@ -12,7 +11,6 @@ while : ; do
case "$1" in
-f)
remove_files=true
- index_remote_option=--force
;;
-n)
show_only=true
@@ -45,23 +43,28 @@ case "$#" in
;;
esac
-files=$(
- if test -f "$GIT_DIR/info/exclude" ; then
- git-ls-files \
- --exclude-from="$GIT_DIR/info/exclude" \
- --exclude-per-directory=.gitignore -- "$@"
- else
- git-ls-files \
+if test -f "$GIT_DIR/info/exclude"
+then
+ git-ls-files -z \
+ --exclude-from="$GIT_DIR/info/exclude" \
--exclude-per-directory=.gitignore -- "$@"
- fi | sort | uniq
-)
-
-case "$show_only" in
-true)
- echo $files
+else
+ git-ls-files -z \
+ --exclude-per-directory=.gitignore -- "$@"
+fi |
+case "$show_only,remove_files" in
+true,*)
+ xargs -0 echo
+ ;;
+*,true)
+ xargs -0 sh -c "
+ while [ \$# -gt 0 ]; do
+ file=\$1; shift
+ rm -- \"\$file\" && git-update-index --remove $verbose \"\$file\"
+ done
+ " inline
;;
*)
- [[ "$remove_files" = "true" ]] && rm -- $files
- git-update-index $index_remove_option $verbose $files
+ git-update-index --force-remove $verbose -z --stdin
;;
esac
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 8415732..b87beb0 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -7,13 +7,12 @@ test_description='Test of the various op
. ./test-lib.sh
-# Setup some files to be removed
-touch foo bar
-git-add foo bar
-# Need one to test --
-touch -- -q
-git update-index --add -- -q
-git-commit -m "add foo, bar, and -q"
+# Setup some files to be removed, some with funny characters
+touch -- foo bar baz 'space embedded' 'tab embedded' 'newline
+embedded' -q
+git-add -- foo bar baz 'space embedded' 'tab embedded' 'newline
+embedded' -q
+git-commit -m "add files"
test_expect_success \
'Pre-check that foo is in index before git-rm foo' \
@@ -36,7 +35,22 @@ test_expect_failure \
'[ -f bar ]'
test_expect_success \
- 'Test that "git-rm -- -q" works to delete a file named -q' \
+ 'Test that "git-rm -- -q" works to delete a file that looks like an option' \
'git-rm -- -q'
+test_expect_success \
+ "Test that \"git-rm -f\" can remove files with embedded space, tab, or newline characters." \
+ "git-rm 'space embedded' 'tab embedded' 'newline
+embedded"
+
+chmod u-w .
+test_expect_failure \
+ 'Test that "git-rm -f" fails if its rm fails' \
+ 'git-rm -f baz'
+chmod u+w .
+
+test_expect_success \
+ 'When the rm in "git-rm -f" fails, it should not remove the file from the index' \
+ 'git-ls-files --error-unmatch baz'
+
test_done
--
1.2.2.g01a2-dirty
Attachment:
pgpJgn2rC15ta.pgp
Description: PGP signature