On Tue, Jan 05, 2016 at 11:33:30AM +0100, SZEDER Gábor wrote:
> 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'
Thanks for thorough explanation. I think your patch makes sense.
> 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...
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).
> 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" &&
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
