Re: [PATCH] Add a test for git-add --ignore-errors

Alex Riesen [off-list ref] writes:

I like the fact that you added --ignore-errors and made it still error out
when it cannot read some files.  Shouldn't we be testing it here with
"must-fail"?

Signed-off-by: Alex Riesen <redacted>
---
Junio C Hamano, Tue, May 13, 2008 05:48:22 +0200:

Alex Riesen [off-list ref] writes:

quoted

+test_expect_success 'git add --ignore-errors' '
+	git reset --hard &&
+	date >foo1 &&
+	date >foo2 &&
+	chmod 0 foo2 &&
+	git add --verbose --ignore-errors .
+	git ls-files |grep foo1
+'
+
 test_done

I like the fact that you added --ignore-errors and made it still error out
when it cannot read some files.  Shouldn't we be testing it here with
"must-fail"?

Yes. Would you mind replacing that patch with this one?

 t/t3700-add.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 287e058..17ab05a 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh

@@ -179,4 +179,13 @@ test_expect_success 'git add --refresh' '
 	test -z "`git diff-index HEAD -- foo`"
 '
 
+test_expect_success 'git add --ignore-errors' '
+	git reset --hard &&
+	date >foo1 &&
+	date >foo2 &&
+	chmod 0 foo2 &&
+	test_must_fail git add --verbose --ignore-errors . &&
+	git ls-files |grep foo1
+'
+
 test_done

-- 
1.5.5.1.206.g7103c

Junio C Hamano [off-list ref] writes:

Alex Riesen [off-list ref] writes:

quoted

Signed-off-by: Alex Riesen <redacted>
---
 t/t3700-add.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 287e058..ca3e33d 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh

@@ -179,4 +179,13 @@ test_expect_success 'git add --refresh' '
 	test -z "`git diff-index HEAD -- foo`"
 '
 
+test_expect_success 'git add --ignore-errors' '
+	git reset --hard &&
+	date >foo1 &&
+	date >foo2 &&
+	chmod 0 foo2 &&
+	git add --verbose --ignore-errors .
+	git ls-files |grep foo1
+'
+
 test_done

I like the fact that you added --ignore-errors and made it still error out
when it cannot read some files.  Shouldn't we be testing it here with
"must-fail"?

It is human nature to get too enthusiastic demonstrating his own shiny new
toy and forget to check that it does not kick in when not asked.  There is
no test for a case to make sure "git add" fails when foo2 is not readable
and does not add "foo1".

Here is a replacement I've queued.  I have a similar addition to the test
in the patch after this one that demonstrates the configuration variable.
I added tests to check the case when the variable is set to false.

-- >8 --
Add a test for git-add --ignore-errors

Signed-off-by: Alex Riesen <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 t/t3700-add.sh |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 287e058..01e4d62 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh

@@ -179,4 +179,26 @@ test_expect_success 'git add --refresh' '
 	test -z "`git diff-index HEAD -- foo`"
 '
 
+test_expect_success 'git add should fail atomically upon an unreadable file' '
+	git reset --hard &&
+	date >foo1 &&
+	date >foo2 &&
+	chmod 0 foo2 &&
+	test_must_fail git add --verbose . &&
+	! ( git ls-files foo1 | grep foo1 )
+'
+
+rm -f foo2
+
+test_expect_success 'git add --ignore-errors' '
+	git reset --hard &&
+	date >foo1 &&
+	date >foo2 &&
+	chmod 0 foo2 &&
+	test_must_fail git add --verbose --ignore-errors . &&
+	git ls-files foo1 | grep foo1
+'
+
+rm -f foo2
+
 test_done

-- 
1.5.5.1.340.g39dc6

Junio C Hamano, Tue, May 13, 2008 08:05:01 +0200:

Junio C Hamano [off-list ref] writes:

quoted

Alex Riesen [off-list ref] writes:

quoted

Signed-off-by: Alex Riesen <redacted>
---
 t/t3700-add.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 287e058..ca3e33d 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh

@@ -179,4 +179,13 @@ test_expect_success 'git add --refresh' '
 	test -z "`git diff-index HEAD -- foo`"
 '
 
+test_expect_success 'git add --ignore-errors' '
+	git reset --hard &&
+	date >foo1 &&
+	date >foo2 &&
+	chmod 0 foo2 &&
+	git add --verbose --ignore-errors .
+	git ls-files |grep foo1
+'
+
 test_done

I like the fact that you added --ignore-errors and made it still error out
when it cannot read some files.  Shouldn't we be testing it here with
"must-fail"?

It is human nature to get too enthusiastic demonstrating his own shiny new
toy and forget to check that it does not kick in when not asked.  There is
no test for a case to make sure "git add" fails when foo2 is not readable
and does not add "foo1".

Yes, I missed that. Exactly for the reason :)

Here is a replacement I've queued.  I have a similar addition to the test
in the patch after this one that demonstrates the configuration variable.
I added tests to check the case when the variable is set to false.

Thanks.

-- >8 --
Add a test for git-add --ignore-errors

Signed-off-by: Alex Riesen <redacted>

Acked-by: Alex Riesen <redacted>