[PATCH] t7507-*.sh: Fix test #8 (could not run '$FAKE_EDITOR')
Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk --- Hi Jens, commit 61b6a633 (commit -v: strip diffs and submodule shortlogs from the commit message, 19-11-2013) in 'pu' fails the new test it added to t7507. I didn't spend too long looking at the problem, so take this patch as nothing more than a quick suggestion for a possible solution! :-P [The err file contained something like: There was a problem with the editor '$FAKE_EDITOR']. Having said that, this fixes it for me ... ATB, Ramsay Jones t/t7507-commit-verbose.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index 09c1150..49cfb3c 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -79,7 +79,8 @@ test_expect_success 'submodule log is stripped out too with -v' ' echo more file git commit -a -m submodule commit ) - GIT_EDITOR=cat test_must_fail git commit -a -v 2err + test_set_editor cat + test_must_fail git commit -a -v 2err test_i18ngrep Aborting commit due to empty commit message. err ' -- 1.8.4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '$FAKE_EDITOR')
Ramsay Jones ram...@ramsay1.demon.co.uk writes: Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk --- Hi Jens, commit 61b6a633 (commit -v: strip diffs and submodule shortlogs from the commit message, 19-11-2013) in 'pu' fails the new test it added to t7507. I didn't spend too long looking at the problem, so take this patch as nothing more than a quick suggestion for a possible solution! :-P [The err file contained something like: There was a problem with the editor '$FAKE_EDITOR']. Having said that, this fixes it for me ... Well spotted. test_must_fail being a shell function, not a command, we shouldn't have used the VAR=val cmd one-shot environment assignment for portability. Even though this happens to be the last test in the script, using test_set_editor to permanently affect the choice of editor for tests that follow is not generally a good idea. It would be safer to do this, I would have to say: git commit -a -m submodule commit ) ( GIT_EDITOR=cat export GIT_EDITOR test_must_fail git commit -a -v 2err ) test_i18ngrep Aborting ... Thanks. ATB, Ramsay Jones t/t7507-commit-verbose.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index 09c1150..49cfb3c 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -79,7 +79,8 @@ test_expect_success 'submodule log is stripped out too with -v' ' echo more file git commit -a -m submodule commit ) - GIT_EDITOR=cat test_must_fail git commit -a -v 2err + test_set_editor cat + test_must_fail git commit -a -v 2err test_i18ngrep Aborting commit due to empty commit message. err ' -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '$FAKE_EDITOR')
Jeff King p...@peff.net writes:
The test_set_editor helper does some magic to help with quoting, but
that should not be an issue in this case (since we are using cat). We
are using test_set_editor elsewhere in the script, which would have set
EDITOR previously. But I would think that GIT_EDITOR, which we are using
here, would supersede that. However, the error message he shows
indicates that git is using EDITOR (as FAKE_EDITOR is part of that quote
magic).
Am I misremembering the issues with one-shot variables and functions?
I think there are two problems involved.
The first is that we are not really using GIT_EDITOR under some
shells; to wit:
8
$ cat /var/tmp/dashtest.sh \EOF
#!/bin/sh
test_must_fail () {
(
env | sed -n -e '/EDITOR/s/^/ /p'
)
}
EDITOR=dog
export EDITOR
GIT_EDITOR=cat test_must_fail foo
EOF
$ dash /var/tmp/dashtest.sh
EDITOR=dog
$ bash /var/tmp/dashtest.sh
GIT_EDITOR=cat
EDITOR=dog
8
So it appears that GIT_EDITOR that was never exported in the script
fails to get exported with the VAR=VAL cmd syntax under dash, when
cmd is not a command but is a shell function. The git commit in
question ends up using $EDITOR.
Another is that EDITOR=$FAKE_EDITOR that is set up earlier in the
is having trouble launching (I have a feeling that it never was
actually used because everybody uses commit -F file).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '$FAKE_EDITOR')
On Wed, Nov 20, 2013 at 10:33:28AM -0800, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
Am I misremembering the issues with one-shot variables and functions?
I think there are two problems involved.
OK, I was misremembering. I recalled the does not unset afterwards
part, but not the does not export part. I think because:
test_must_fail () {
(
env | sed -n -e '/EDITOR/s/^/ /p'
)
}
...here we _do_ have GIT_EDITOR set properly in the function itself, but
not in the subprocess.
Previous discussion and links to POSIX are here:
http://article.gmane.org/gmane.comp.version-control.git/137095
Not that they matter compared to the code you demonstrated, but I was
digging them up when you responded. :)
Another is that EDITOR=$FAKE_EDITOR that is set up earlier in the
is having trouble launching (I have a feeling that it never was
actually used because everybody uses commit -F file).
I think it is used, as there are several git commit --amend -v
invocations. Which makes sense, as you should not be able to test -v
with -F, I would think.
I'm not sure why the old $FAKE_EDITOR doesn't work there, though (not
that it would make the test pass anyway, as it does something different
than what the test wants, but I would not expect the shell to complain
of failure).
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '$FAKE_EDITOR')
On Wed, Nov 20, 2013 at 01:54:20PM -0500, Jeff King wrote: I'm not sure why the old $FAKE_EDITOR doesn't work there, though (not that it would make the test pass anyway, as it does something different than what the test wants, but I would not expect the shell to complain of failure). Oh, I see. The original FAKE_EDITOR is: exec grep '^diff --git' \$1 But the whole point of the test is that we do not have such a diff header, being only a submodule update, and so grep exits non-zero, and git thinks the editor has failed. So mystery solved, and the sole problem is the proper setting of the variable. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '$FAKE_EDITOR')
On 20/11/13 17:22, Junio C Hamano wrote: Ramsay Jones ram...@ramsay1.demon.co.uk writes: Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk --- Hi Jens, commit 61b6a633 (commit -v: strip diffs and submodule shortlogs from the commit message, 19-11-2013) in 'pu' fails the new test it added to t7507. I didn't spend too long looking at the problem, so take this patch as nothing more than a quick suggestion for a possible solution! :-P [The err file contained something like: There was a problem with the editor '$FAKE_EDITOR']. Having said that, this fixes it for me ... Well spotted. test_must_fail being a shell function, not a command, we shouldn't have used the VAR=val cmd one-shot environment assignment for portability. Even though this happens to be the last test in the script, using test_set_editor to permanently affect the choice of editor for tests that follow is not generally a good idea. It would be safer to do this, I would have to say: git commit -a -m submodule commit ) ( GIT_EDITOR=cat export GIT_EDITOR test_must_fail git commit -a -v 2err ) test_i18ngrep Aborting ... Yes, this works great (and I very nearly wrote exactly this, but since the test was using test_set_editor anyway ...) :-D Thanks. ATB, Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '$FAKE_EDITOR')
Am 20.11.2013 20:35, schrieb Ramsay Jones: On 20/11/13 17:22, Junio C Hamano wrote: Ramsay Jones ram...@ramsay1.demon.co.uk writes: Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk --- Hi Jens, commit 61b6a633 (commit -v: strip diffs and submodule shortlogs from the commit message, 19-11-2013) in 'pu' fails the new test it added to t7507. I didn't spend too long looking at the problem, so take this patch as nothing more than a quick suggestion for a possible solution! :-P [The err file contained something like: There was a problem with the editor '$FAKE_EDITOR']. Having said that, this fixes it for me ... Well spotted. test_must_fail being a shell function, not a command, we shouldn't have used the VAR=val cmd one-shot environment assignment for portability. Even though this happens to be the last test in the script, using test_set_editor to permanently affect the choice of editor for tests that follow is not generally a good idea. It would be safer to do this, I would have to say: git commit -a -m submodule commit ) ( GIT_EDITOR=cat export GIT_EDITOR test_must_fail git commit -a -v 2err ) test_i18ngrep Aborting ... Yes, this works great (and I very nearly wrote exactly this, but since the test was using test_set_editor anyway ...) :-D Thanks all, will use that in the next iteration. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
