[PATCH] t7507-*.sh: Fix test #8 (could not run '"$FAKE_EDITOR"')

[PATCH] t7507-*.sh: Fix test #8 (could not run '"$FAKE_EDITOR"')

[Ramsay Jones]

Signed-off-by: Ramsay Jones <ramsay@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 2>err &&
+	test_set_editor cat &&
+	test_must_fail git commit -a -v 2>err &&
 	test_i18ngrep "Aborting commit due to empty commit message." err
 '
 
-- 
1.8.4

Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '"$FAKE_EDITOR"')

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> Signed-off-by: Ramsay Jones <ramsay@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 2>err
	) &&
        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 2>err &&
> +	test_set_editor cat &&
> +	test_must_fail git commit -a -v 2>err &&
>  	test_i18ngrep "Aborting commit due to empty commit message." err
>  '

Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '"$FAKE_EDITOR"')

[Jeff King]

On Wed, Nov 20, 2013 at 09:22:31AM -0800, Junio C Hamano wrote:

> > 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.

Yeah, I noticed that, too upon reading Ramsay's patch. But I thought the
usual symptom there was that the variable is not properly unset after
the function returns? In other words, it might affect later tests, but
the failure that Ramsay sees is in _this_ test, so it must be a separate
issue.

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?

Puzzled...

-Peff

Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '"$FAKE_EDITOR"')

[Junio C Hamano]

Jeff King <peff@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>").

Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '"$FAKE_EDITOR"')

[Jeff King]

On Wed, Nov 20, 2013 at 10:33:28AM -0800, Junio C Hamano wrote:

> Jeff King <peff@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

Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '"$FAKE_EDITOR"')

[Jeff King]

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

Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '"$FAKE_EDITOR"')

[Ramsay Jones]

On 20/11/13 17:22, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> Signed-off-by: Ramsay Jones <ramsay@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 2>err
> 	) &&
>         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

Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '"$FAKE_EDITOR"')

[Jens Lehmann]

Am 20.11.2013 20:35, schrieb Ramsay Jones:
> On 20/11/13 17:22, Junio C Hamano wrote:
>> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>>
>>> Signed-off-by: Ramsay Jones <ramsay@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 2>err
>> 	) &&
>>         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.