patch, 3 partst6050-replace: make failing editor test more robust

'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@ira.uka.de>
---

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(-)

Erm, subject says "[PATCH 1/3]", but I messed up, this is a standalone patch.

Gábor

On Tue, Jan 05, 2016 at 11:33:30AM +0100, SZEDER Gábor wrote:

Thanks for thorough explanation. I think your patch makes sense.

We usually try to avoid hard-coding error messages, because they end up brittle. I think if we've isolated the failure, it's a reasonable test (in an ideal world, you check that "foo" doesn't fail, and "foo -wrong" does fail; i.e., just changing one variable in your experiment).

We have the same problem when running aliases, or any git command that you want to expand into more complex shell. The usual solution for one-off is something like:

  test_must_fail env GIT_EDITOR="f() { ./fakeeditor; false; } f" git ...

That might be preferable to yours, because a reader can see immediately in the test what is going on, without wondering what it is that failingfakeeditor does. OTOH, it is perhaps somewhat non-obvious. It came to mind to me because it is an idiom we use elsewhere; I remember thinking it was very clever the first time somebody showed it to me. :)

I'd be OK with the patch using either method.

-Peff