threads / patch / 35367

patcht7507-*.sh: Fix test #8 (could not run '"$FAKE_EDITOR"')

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

## tl;dr

8 messages between Nov 20, 2013 and Nov 20, 2013. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Ramsay Jones· Nov 20, 2013, 16:45 UTC · lore
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(-)
Show changes to t/t7507-commit-verbose.sh +2 −1
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
Junio C Hamano· Nov 20, 2013, 17:22 UTC · re: Ramsay Jones · lore

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

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
Show 15 quoted lines
> 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.
Show 19 quoted lines
> 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
>  '
Jeff King· Nov 20, 2013, 17:42 UTC · re: Junio C Hamano · lore

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

On Wed, Nov 20, 2013 at 09:22:31AM -0800, Junio C Hamano wrote:
Show 14 quoted lines
> > 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
Junio C Hamano· Nov 20, 2013, 18:33 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> 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>").

Jeff King· Nov 20, 2013, 18:54 UTC · re: Junio C Hamano · lore

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:
Show 5 quoted lines
> 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:

Show 5 quoted lines
> 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
Jeff King· Nov 20, 2013, 19:01 UTC · re: Jeff King · lore

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
Ramsay Jones· Nov 20, 2013, 19:35 UTC · re: Junio C Hamano · lore

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

On 20/11/13 17:22, Junio C Hamano wrote:
Show 35 quoted lines
> 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

Jens Lehmann· Nov 20, 2013, 21:32 UTC · re: Ramsay Jones · lore

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

Am 20.11.2013 20:35, schrieb Ramsay Jones:
Show 39 quoted lines
> 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.

← back to recent threads