'git replace --edit' should error out when the invoked editor fails,
but the test checking this behavior would not notice if this weren't
the case.
The test in question, ever since it was added in 85f98fc037ae
(replace: add tests for --edit, 2014-05-17), has simulated a failing
editor in an unconventional way:
test_must_fail env GIT_EDITOR='./fakeeditor;false' git replace --edit
I presume the reason for this unconventional editor was the fact that
'git replace --edit' requires the edited object to be different from
the original, but a mere 'false' as editor would leave the object
unchanged and 'git replace --edit' would error out anyway complaining
about the new and the original object files being the same. Running
'fakeeditor' before 'false' was supposed to ensure that the object
file is modified and thus 'git replace --edit' errors out because of
the failed editor.
However, this editor doesn't actually modify the edited object,
because start_command() turns this editor into:
/bin/sh -c './fakeeditor;false "$@"' './fakeeditor;false' \
'.../.git/REPLACE_EDITOBJ'
This means that the test's fakeeditor script doesn't even get the path
of the object to be edited as argument, triggering error messages from
the commands executed inside the script ('sed' and 'mv'), and
ultimately leaving the object file unchanged.
If a patch were to remove the die() from the error path after
launch_editor(), the test would not catch it, because 'git replace'
would continue execution past launch_editor() and would error out a
bit later due to the unchanged edited object. Though 'git replace'
would error out for the wrong reason, this would satisfy
'test_must_fail' just as well, and the test would succeed leaving the
undesired change unnoticed.
Create a proper failing fake editor script for this test to ensure
that the edited object is in fact modified and 'git replace --edit'
won't error out because the new and original object files are the
same.
Signed-off-by: SZEDER Gábor <szeder@xxxxxxxxxx>
---
Should we be more thorough, perhaps, and check the error message to be
extra sure that 'git replace --edit' errors out for the expected
reason? There are oh so many 'test_must_fail's in our test scripts
and we don't check the error message in most of the cases...
I looked through the output of 'git grep GIT_EDITOR t/' and didn't
notice any other similar case where GIT_EDITOR consists of multiple
commands.
t/t6050-replace.sh | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 4d5a25eedfef..c630aba657e9 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -351,11 +351,15 @@ test_expect_success 'test --format long' '
test_cmp expected actual
'
-test_expect_success 'setup a fake editor' '
- write_script fakeeditor <<-\EOF
+test_expect_success 'setup fake editors' '
+ write_script fakeeditor <<-\EOF &&
sed -e "s/A U Thor/A fake Thor/" "$1" >"$1.new"
mv "$1.new" "$1"
EOF
+ write_script failingfakeeditor <<-\EOF
+ ./fakeeditor "$@"
+ false
+ EOF
'
test_expect_success '--edit with and without already replaced object' '
@@ -372,7 +376,7 @@ test_expect_success '--edit with and without already replaced object' '
test_expect_success '--edit and change nothing or command failed' '
git replace -d "$PARA3" &&
test_must_fail env GIT_EDITOR=true git replace --edit "$PARA3" &&
- test_must_fail env GIT_EDITOR="./fakeeditor;false" git replace --edit "$PARA3" &&
+ test_must_fail env GIT_EDITOR="./failingfakeeditor" git replace --edit "$PARA3" &&
GIT_EDITOR=./fakeeditor git replace --edit "$PARA3" &&
git replace -l | grep "$PARA3" &&
git cat-file commit "$PARA3" | grep "A fake Thor"
--
2.7.0.rc2.34.g28a1f98
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html