[PATCH] t7500-commit.sh: do not call test_set_editor unnecessarily, it's confusing

[PATCH] t7500-commit.sh: do not call test_set_editor unnecessarily, it's confusing

[Adeodato Simó]

Signed-off-by: Adeodato Simó <dato@net.com.org.es>
---

I was reading this test case, and it took a small bit to figure out the
editor was not being used at all. I hope there was no hidden reason for
it to be there, and it can go away.

Cheers,

 t/t7500-commit.sh |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 6e18a96..5998baf 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -149,10 +149,7 @@ EOF
 
 test_expect_success '--signoff' '
 	echo "yet another content *narf*" >> foo &&
-	echo "zort" | (
-		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
-		git commit -s -F - foo
-	) &&
+	echo "zort" | git commit -s -F - foo &&
 	git cat-file commit HEAD | sed "1,/^$/d" > output &&
 	test_cmp expect output
 '
-- 
1.6.1.307.g07803

Re: [PATCH] t7500-commit.sh: do not call test_set_editor unnecessarily, it's confusing

[Junio C Hamano]

Adeodato Simó <dato@net.com.org.es> writes:

> I was reading this test case, and it took a small bit to figure out the
> editor was not being used at all. I hope there was no hidden reason for
> it to be there, and it can go away.

That 'zort' came from 1320857 (builtin-commit: fix --signoff, 2007-11-11),
and I _think_ it is trying to make sure that presense of "-F -" makes the
editor not to trigger.

Dscho?

Re: [PATCH] t7500-commit.sh: do not call test_set_editor unnecessarily, it's confusing

[Adeodato Simó]

* Junio C Hamano [Mon, 29 Dec 2008 01:46:02 -0800]:

> Adeodato Simó <dato@net.com.org.es> writes:

> > I was reading this test case, and it took a small bit to figure out the
> > editor was not being used at all. I hope there was no hidden reason for
> > it to be there, and it can go away.

> That 'zort' came from 1320857 (builtin-commit: fix --signoff, 2007-11-11),
> and I _think_ it is trying to make sure that presense of "-F -" makes the
> editor not to trigger.

Hm. Well, if that is true, then IMHO it should be in a /separate/ test
case, for clarity. Probably in "message from stdin" test from t7501.

That's of course just my opinion, and I'll accept if you prefer to
maintain it the way it is now. I also volunteer to move it to t7501 if
that's what you prefer, just let me know.

Thanks,

-- 
Adeodato Simó                                     dato at net.com.org.es
Debian Developer                                  adeodato at debian.org
 
                              Listening to: Justin Nozuka - I'm In Peace

Re: [PATCH] t7500-commit.sh: do not call test_set_editor unnecessarily, it's confusing

[Junio C Hamano]

Adeodato Simó <dato@net.com.org.es> writes:

> * Junio C Hamano [Mon, 29 Dec 2008 01:46:02 -0800]:
> ...
>> That 'zort' came from 1320857 (builtin-commit: fix --signoff, 2007-11-11),
>> and I _think_ it is trying to make sure that presense of "-F -" makes the
>> editor not to trigger.
>
> Hm. Well, if that is true, then IMHO it should be in a /separate/ test
> case, for clarity. Probably in "message from stdin" test from t7501.
>
> That's of course just my opinion, and I'll accept if you prefer to
> maintain it the way it is now. I also volunteer to move it to t7501 if
> that's what you prefer, just let me know.

I underscored _think_ and CC'ed Dscho for a reason.

If "-F -" has a bug, and if you do not define GIT_EDITOR, your test may
not work unattended (i.e. neither succeed nor finish but gets stuck).
Perhaps that is the issue?  I dunno.

Re: [PATCH] t7500-commit.sh: do not call test_set_editor unnecessarily, it's confusing

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 839 bytes --]

Hi,

On Mon, 29 Dec 2008, Junio C Hamano wrote:

> Adeodato Simó <dato@net.com.org.es> writes:
> 
> > I was reading this test case, and it took a small bit to figure out 
> > the editor was not being used at all. I hope there was no hidden 
> > reason for it to be there, and it can go away.
> 
> That 'zort' came from 1320857 (builtin-commit: fix --signoff, 
> 2007-11-11), and I _think_ it is trying to make sure that presense of 
> "-F -" makes the editor not to trigger.
> 
> Dscho?

Hmm.  Obviously, I failed to document properly why I tested the editor, 
but I think it makes sense to assume that -F still triggered an 
interactive editor at some stage in the development of builtin commit.

I do not have anything against separating that issue into another test 
case, but I am strongly opposed to simply removing it.

Ciao,
Dscho

[PATCH v2] t7501-commit.sh: explicitly check that -F prevents invoking the editor

[Adeodato Simó]

The "--signoff" test case in t7500-commit.sh was setting VISUAL while
using -F -, which indeed tested that the editor is not spawned with -F.
However, having it there was confusing, since there was no obvious reason
to the casual reader for it to be there.

This commits removes the setting of VISUAL from the --signoff test, and
adds in t7501-commit.sh a dedicated test case, where the rest of tests for
-F are.

Signed-off-by: Adeodato Simó <dato@net.com.org.es>
---
* Johannes Schindelin [Tue, 30 Dec 2008 13:04:46 +0100]:

> Hmm.  Obviously, I failed to document properly why I tested the editor, 
> but I think it makes sense to assume that -F still triggered an 
> interactive editor at some stage in the development of builtin commit.

> I do not have anything against separating that issue into another test 
> case, but I am strongly opposed to simply removing it.

Ok, I've moved it to a separate test case, please review to see if you
approve of it.

Thanks,

 t/t7500-commit.sh |    5 +----
 t/t7501-commit.sh |   20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 6e18a96..5998baf 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -149,10 +149,7 @@ EOF
 
 test_expect_success '--signoff' '
 	echo "yet another content *narf*" >> foo &&
-	echo "zort" | (
-		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
-		git commit -s -F - foo
-	) &&
+	echo "zort" | git commit -s -F - foo &&
 	git cat-file commit HEAD | sed "1,/^$/d" > output &&
 	test_cmp expect output
 '
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 63bfc6d..b4e2b4d 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -127,6 +127,26 @@ test_expect_success \
 	"showing committed revisions" \
 	"git rev-list HEAD >current"
 
+cat >editor <<\EOF
+#!/bin/sh
+sed -e "s/good/bad/g" < "$1" > "$1-"
+mv "$1-" "$1"
+EOF
+chmod 755 editor
+
+cat >msg <<EOF
+A good commit message.
+EOF
+
+test_expect_success \
+	'editor not invoked if -F is given' '
+	 echo "moo" >file &&
+	 VISUAL=./editor git commit -a -F msg &&
+	 git show -s --pretty=format:"%s" | grep -q good &&
+	 echo "quack" >file &&
+	 echo "Another good message." | VISUAL=./editor git commit -a -F - &&
+	 git show -s --pretty=format:"%s" | grep -q good
+	 '
 # We could just check the head sha1, but checking each commit makes it
 # easier to isolate bugs.
 
-- 
1.6.1.134.g55c35

Re: [PATCH v2] t7501-commit.sh: explicitly check that -F prevents invoking the editor

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 945 bytes --]

Hi,

On Fri, 9 Jan 2009, Adeodato Simó wrote:

> diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
> index 6e18a96..5998baf 100755
> --- a/t/t7500-commit.sh
> +++ b/t/t7500-commit.sh
> @@ -149,10 +149,7 @@ EOF
>  
>  test_expect_success '--signoff' '
>  	echo "yet another content *narf*" >> foo &&
> -	echo "zort" | (
> -		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
> -		git commit -s -F - foo
> -	) &&
> +	echo "zort" | git commit -s -F - foo &&
>  	git cat-file commit HEAD | sed "1,/^$/d" > output &&
>  	test_cmp expect output
>  '

AFAICT this still tests if -F - launches an editor, except that it _does_ 
launch the editor, waiting for the user to quit the editor.  Which is bad.

In the end I think it is not worth all that effort (as the issue was fixed 
long time ago, probably even before builtin-commit entered 'next'), so I'd 
just leave the test as-is, documenting why the editor is set to 
add-content.

Ciao,
Dscho

Re: [PATCH v2] t7501-commit.sh: explicitly check that -F prevents invoking the editor

[Adeodato Simó]

* Johannes Schindelin [Sat, 10 Jan 2009 11:19:43 +0100]:

> Hi,

Hello,

> >  test_expect_success '--signoff' '
> >  	echo "yet another content *narf*" >> foo &&
> > -	echo "zort" | (
> > -		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
> > -		git commit -s -F - foo
> > -	) &&
> > +	echo "zort" | git commit -s -F - foo &&
> >  	git cat-file commit HEAD | sed "1,/^$/d" > output &&
> >  	test_cmp expect output
> >  '

> AFAICT this still tests if -F - launches an editor, except that it _does_ 
> launch the editor, waiting for the user to quit the editor.  Which is bad.

The default value of VISUAL for the test suite is ":" AFAICS. Hence,
even if it's called, it will return immediately.

If it would be called, without my patch the "--signoff" test would fail,
but there would be no obvious reason as to why. Seeing "editor not
invoked if -F is given FAILED" is much more clear IMHO.

Also note that there plenty of places in the test suite where -F is
used, but VISUAL is not set explicitly.

Cheers,

-- 
Adeodato Simó                                     dato at net.com.org.es
Debian Developer                                  adeodato at debian.org
 
Excuse me for thinking a banana-eating contest was about eating a banana!
                -- Paris Geller

Re: [PATCH v2] t7501-commit.sh: explicitly check that -F prevents invoking the editor

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 816 bytes --]

Hi,

On Sat, 10 Jan 2009, Adeodato Simó wrote:

> * Johannes Schindelin [Sat, 10 Jan 2009 11:19:43 +0100]:
> 
> > >  test_expect_success '--signoff' '
> > >  	echo "yet another content *narf*" >> foo &&
> > > -	echo "zort" | (
> > > -		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
> > > -		git commit -s -F - foo
> > > -	) &&
> > > +	echo "zort" | git commit -s -F - foo &&
> > >  	git cat-file commit HEAD | sed "1,/^$/d" > output &&
> > >  	test_cmp expect output
> > >  '
> 
> > AFAICT this still tests if -F - launches an editor, except that it _does_ 
> > launch the editor, waiting for the user to quit the editor.  Which is bad.
> 
> The default value of VISUAL for the test suite is ":" AFAICS. Hence,
> even if it's called, it will return immediately.

Ah.  Okay then.

Sorry for the noise,
Dscho