{"thread":{"id":"16906","subject":"[PATCH] t7500-commit.sh: do not call test_set_editor unnecessarily, it's confusing","startedAt":"2008-12-29T09:24:18Z","lastAt":"2009-01-10T11:48:28Z","messageCount":9,"participants":["Adeodato Simó","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"98877","messageId":"1230542658-9758-1-git-send-email-dato@net.com.org.es","threadId":"16906","inReplyTo":null,"subject":"[PATCH] t7500-commit.sh: do not call test_set_editor unnecessarily, it's confusing","fromName":"Adeodato Simó","fromEmail":"dato@net.com.org.es","sentAt":"2008-12-29T09:24:18Z","receivedAt":"2008-12-29T09:24:18Z","isPatch":true,"sender":{"key":"dato@net.com.org.es","avatar":"https://gravatar.com/avatar/952ec7d5d5663eb8baf631b5c37f9c58480a881920dd5f8a2d3a71f969b72b53?d=mp&s=160"},"body":"Signed-off-by: Adeodato Simó <dato@net.com.org.es>\n---\n\nI was reading this test case, and it took a small bit to figure out the\neditor was not being used at all. I hope there was no hidden reason for\nit to be there, and it can go away.\n\nCheers,\n\n t/t7500-commit.sh |    5 +----\n 1 files changed, 1 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7500-commit.sh b/t/t7500-commit.sh\nindex 6e18a96..5998baf 100755\n--- a/t/t7500-commit.sh\n+++ b/t/t7500-commit.sh\n@@ -149,10 +149,7 @@ EOF\n \n test_expect_success '--signoff' '\n \techo \"yet another content *narf*\" >> foo &&\n-\techo \"zort\" | (\n-\t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n-\t\tgit commit -s -F - foo\n-\t) &&\n+\techo \"zort\" | git commit -s -F - foo &&\n \tgit cat-file commit HEAD | sed \"1,/^$/d\" > output &&\n \ttest_cmp expect output\n '\n-- \n1.6.1.307.g07803\n"},{"id":"98878","messageId":"7vmyefco11.fsf@gitster.siamese.dyndns.org","threadId":"16906","inReplyTo":"1230542658-9758-1-git-send-email-dato@net.com.org.es","subject":"Re: [PATCH] t7500-commit.sh: do not call test_set_editor unnecessarily, it's confusing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-29T09:46:02Z","receivedAt":"2008-12-29T09:46:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adeodato Simó <dato@net.com.org.es> writes:\n\n> I was reading this test case, and it took a small bit to figure out the\n> editor was not being used at all. I hope there was no hidden reason for\n> it to be there, and it can go away.\n\nThat 'zort' came from 1320857 (builtin-commit: fix --signoff, 2007-11-11),\nand I _think_ it is trying to make sure that presense of \"-F -\" makes the\neditor not to trigger.\n\nDscho?\n"},{"id":"98880","messageId":"20081229095220.GA26942@chistera.yi.org","threadId":"16906","inReplyTo":"7vmyefco11.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] t7500-commit.sh: do not call test_set_editor unnecessarily, it's confusing","fromName":"Adeodato Simó","fromEmail":"dato@net.com.org.es","sentAt":"2008-12-29T09:52:20Z","receivedAt":"2008-12-29T09:52:20Z","isPatch":true,"sender":{"key":"dato@net.com.org.es","avatar":"https://gravatar.com/avatar/952ec7d5d5663eb8baf631b5c37f9c58480a881920dd5f8a2d3a71f969b72b53?d=mp&s=160"},"body":"* Junio C Hamano [Mon, 29 Dec 2008 01:46:02 -0800]:\n\n> Adeodato Simó <dato@net.com.org.es> writes:\n\n> > I was reading this test case, and it took a small bit to figure out the\n> > editor was not being used at all. I hope there was no hidden reason for\n> > it to be there, and it can go away.\n\n> That 'zort' came from 1320857 (builtin-commit: fix --signoff, 2007-11-11),\n> and I _think_ it is trying to make sure that presense of \"-F -\" makes the\n> editor not to trigger.\n\nHm. Well, if that is true, then IMHO it should be in a /separate/ test\ncase, for clarity. Probably in \"message from stdin\" test from t7501.\n\nThat's of course just my opinion, and I'll accept if you prefer to\nmaintain it the way it is now. I also volunteer to move it to t7501 if\nthat's what you prefer, just let me know.\n\nThanks,\n\n-- \nAdeodato Simó                                     dato at net.com.org.es\nDebian Developer                                  adeodato at debian.org\n \n                              Listening to: Justin Nozuka - I'm In Peace\n"},{"id":"98882","messageId":"7vbpuvcnhh.fsf@gitster.siamese.dyndns.org","threadId":"16906","inReplyTo":"20081229095220.GA26942@chistera.yi.org","subject":"Re: [PATCH] t7500-commit.sh: do not call test_set_editor unnecessarily, it's confusing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-29T09:57:46Z","receivedAt":"2008-12-29T09:57:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adeodato Simó <dato@net.com.org.es> writes:\n\n> * Junio C Hamano [Mon, 29 Dec 2008 01:46:02 -0800]:\n> ...\n>> That 'zort' came from 1320857 (builtin-commit: fix --signoff, 2007-11-11),\n>> and I _think_ it is trying to make sure that presense of \"-F -\" makes the\n>> editor not to trigger.\n>\n> Hm. Well, if that is true, then IMHO it should be in a /separate/ test\n> case, for clarity. Probably in \"message from stdin\" test from t7501.\n>\n> That's of course just my opinion, and I'll accept if you prefer to\n> maintain it the way it is now. I also volunteer to move it to t7501 if\n> that's what you prefer, just let me know.\n\nI underscored _think_ and CC'ed Dscho for a reason.\n\nIf \"-F -\" has a bug, and if you do not define GIT_EDITOR, your test may\nnot work unattended (i.e. neither succeed nor finish but gets stuck).\nPerhaps that is the issue?  I dunno.\n"},{"id":"98962","messageId":"alpine.DEB.1.00.0812301250210.30769@pacific.mpi-cbg.de","threadId":"16906","inReplyTo":"7vmyefco11.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] t7500-commit.sh: do not call test_set_editor unnecessarily, it's confusing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-12-30T12:04:46Z","receivedAt":"2008-12-30T12:04:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 29 Dec 2008, Junio C Hamano wrote:\n\n> Adeodato Simó <dato@net.com.org.es> writes:\n> \n> > I was reading this test case, and it took a small bit to figure out \n> > the editor was not being used at all. I hope there was no hidden \n> > reason for it to be there, and it can go away.\n> \n> That 'zort' came from 1320857 (builtin-commit: fix --signoff, \n> 2007-11-11), and I _think_ it is trying to make sure that presense of \n> \"-F -\" makes the editor not to trigger.\n> \n> Dscho?\n\nHmm.  Obviously, I failed to document properly why I tested the editor, \nbut I think it makes sense to assume that -F still triggered an \ninteractive editor at some stage in the development of builtin commit.\n\nI do not have anything against separating that issue into another test \ncase, but I am strongly opposed to simply removing it.\n\nCiao,\nDscho\n"},{"id":"99799","messageId":"1231522205-10510-1-git-send-email-dato@net.com.org.es","threadId":"16906","inReplyTo":"alpine.DEB.1.00.0812301250210.30769@pacific.mpi-cbg.de","subject":"[PATCH v2] t7501-commit.sh: explicitly check that -F prevents invoking the editor","fromName":"Adeodato Simó","fromEmail":"dato@net.com.org.es","sentAt":"2009-01-09T17:30:05Z","receivedAt":"2009-01-09T17:30:05Z","isPatch":true,"sender":{"key":"dato@net.com.org.es","avatar":"https://gravatar.com/avatar/952ec7d5d5663eb8baf631b5c37f9c58480a881920dd5f8a2d3a71f969b72b53?d=mp&s=160"},"body":"The \"--signoff\" test case in t7500-commit.sh was setting VISUAL while\nusing -F -, which indeed tested that the editor is not spawned with -F.\nHowever, having it there was confusing, since there was no obvious reason\nto the casual reader for it to be there.\n\nThis commits removes the setting of VISUAL from the --signoff test, and\nadds in t7501-commit.sh a dedicated test case, where the rest of tests for\n-F are.\n\nSigned-off-by: Adeodato Simó <dato@net.com.org.es>\n---\n* Johannes Schindelin [Tue, 30 Dec 2008 13:04:46 +0100]:\n\n> Hmm.  Obviously, I failed to document properly why I tested the editor, \n> but I think it makes sense to assume that -F still triggered an \n> interactive editor at some stage in the development of builtin commit.\n\n> I do not have anything against separating that issue into another test \n> case, but I am strongly opposed to simply removing it.\n\nOk, I've moved it to a separate test case, please review to see if you\napprove of it.\n\nThanks,\n\n t/t7500-commit.sh |    5 +----\n t/t7501-commit.sh |   20 ++++++++++++++++++++\n 2 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7500-commit.sh b/t/t7500-commit.sh\nindex 6e18a96..5998baf 100755\n--- a/t/t7500-commit.sh\n+++ b/t/t7500-commit.sh\n@@ -149,10 +149,7 @@ EOF\n \n test_expect_success '--signoff' '\n \techo \"yet another content *narf*\" >> foo &&\n-\techo \"zort\" | (\n-\t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n-\t\tgit commit -s -F - foo\n-\t) &&\n+\techo \"zort\" | git commit -s -F - foo &&\n \tgit cat-file commit HEAD | sed \"1,/^$/d\" > output &&\n \ttest_cmp expect output\n '\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 63bfc6d..b4e2b4d 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -127,6 +127,26 @@ test_expect_success \\\n \t\"showing committed revisions\" \\\n \t\"git rev-list HEAD >current\"\n \n+cat >editor <<\\EOF\n+#!/bin/sh\n+sed -e \"s/good/bad/g\" < \"$1\" > \"$1-\"\n+mv \"$1-\" \"$1\"\n+EOF\n+chmod 755 editor\n+\n+cat >msg <<EOF\n+A good commit message.\n+EOF\n+\n+test_expect_success \\\n+\t'editor not invoked if -F is given' '\n+\t echo \"moo\" >file &&\n+\t VISUAL=./editor git commit -a -F msg &&\n+\t git show -s --pretty=format:\"%s\" | grep -q good &&\n+\t echo \"quack\" >file &&\n+\t echo \"Another good message.\" | VISUAL=./editor git commit -a -F - &&\n+\t git show -s --pretty=format:\"%s\" | grep -q good\n+\t '\n # We could just check the head sha1, but checking each commit makes it\n # easier to isolate bugs.\n \n-- \n1.6.1.134.g55c35\n"},{"id":"99862","messageId":"alpine.DEB.1.00.0901101117100.30769@pacific.mpi-cbg.de","threadId":"16906","inReplyTo":"1231522205-10510-1-git-send-email-dato@net.com.org.es","subject":"Re: [PATCH v2] t7501-commit.sh: explicitly check that -F prevents invoking the editor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-10T10:19:43Z","receivedAt":"2009-01-10T10:19:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 9 Jan 2009, Adeodato Simó wrote:\n\n> diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh\n> index 6e18a96..5998baf 100755\n> --- a/t/t7500-commit.sh\n> +++ b/t/t7500-commit.sh\n> @@ -149,10 +149,7 @@ EOF\n>  \n>  test_expect_success '--signoff' '\n>  \techo \"yet another content *narf*\" >> foo &&\n> -\techo \"zort\" | (\n> -\t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n> -\t\tgit commit -s -F - foo\n> -\t) &&\n> +\techo \"zort\" | git commit -s -F - foo &&\n>  \tgit cat-file commit HEAD | sed \"1,/^$/d\" > output &&\n>  \ttest_cmp expect output\n>  '\n\nAFAICT this still tests if -F - launches an editor, except that it _does_ \nlaunch the editor, waiting for the user to quit the editor.  Which is bad.\n\nIn the end I think it is not worth all that effort (as the issue was fixed \nlong time ago, probably even before builtin-commit entered 'next'), so I'd \njust leave the test as-is, documenting why the editor is set to \nadd-content.\n\nCiao,\nDscho"},{"id":"99865","messageId":"20090110103252.GA32151@chistera.yi.org","threadId":"16906","inReplyTo":"alpine.DEB.1.00.0901101117100.30769@pacific.mpi-cbg.de","subject":"Re: [PATCH v2] t7501-commit.sh: explicitly check that -F prevents invoking the editor","fromName":"Adeodato Simó","fromEmail":"dato@net.com.org.es","sentAt":"2009-01-10T10:32:52Z","receivedAt":"2009-01-10T10:32:52Z","isPatch":true,"sender":{"key":"dato@net.com.org.es","avatar":"https://gravatar.com/avatar/952ec7d5d5663eb8baf631b5c37f9c58480a881920dd5f8a2d3a71f969b72b53?d=mp&s=160"},"body":"* Johannes Schindelin [Sat, 10 Jan 2009 11:19:43 +0100]:\n\n> Hi,\n\nHello,\n\n> >  test_expect_success '--signoff' '\n> >  \techo \"yet another content *narf*\" >> foo &&\n> > -\techo \"zort\" | (\n> > -\t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n> > -\t\tgit commit -s -F - foo\n> > -\t) &&\n> > +\techo \"zort\" | git commit -s -F - foo &&\n> >  \tgit cat-file commit HEAD | sed \"1,/^$/d\" > output &&\n> >  \ttest_cmp expect output\n> >  '\n\n> AFAICT this still tests if -F - launches an editor, except that it _does_ \n> launch the editor, waiting for the user to quit the editor.  Which is bad.\n\nThe default value of VISUAL for the test suite is \":\" AFAICS. Hence,\neven if it's called, it will return immediately.\n\nIf it would be called, without my patch the \"--signoff\" test would fail,\nbut there would be no obvious reason as to why. Seeing \"editor not\ninvoked if -F is given FAILED\" is much more clear IMHO.\n\nAlso note that there plenty of places in the test suite where -F is\nused, but VISUAL is not set explicitly.\n\nCheers,\n\n-- \nAdeodato Simó                                     dato at net.com.org.es\nDebian Developer                                  adeodato at debian.org\n \nExcuse me for thinking a banana-eating contest was about eating a banana!\n                -- Paris Geller\n"},{"id":"99877","messageId":"alpine.DEB.1.00.0901101248080.30769@pacific.mpi-cbg.de","threadId":"16906","inReplyTo":"20090110103252.GA32151@chistera.yi.org","subject":"Re: [PATCH v2] t7501-commit.sh: explicitly check that -F prevents invoking the editor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-10T11:48:28Z","receivedAt":"2009-01-10T11:48:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 10 Jan 2009, Adeodato Simó wrote:\n\n> * Johannes Schindelin [Sat, 10 Jan 2009 11:19:43 +0100]:\n> \n> > >  test_expect_success '--signoff' '\n> > >  \techo \"yet another content *narf*\" >> foo &&\n> > > -\techo \"zort\" | (\n> > > -\t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n> > > -\t\tgit commit -s -F - foo\n> > > -\t) &&\n> > > +\techo \"zort\" | git commit -s -F - foo &&\n> > >  \tgit cat-file commit HEAD | sed \"1,/^$/d\" > output &&\n> > >  \ttest_cmp expect output\n> > >  '\n> \n> > AFAICT this still tests if -F - launches an editor, except that it _does_ \n> > launch the editor, waiting for the user to quit the editor.  Which is bad.\n> \n> The default value of VISUAL for the test suite is \":\" AFAICS. Hence,\n> even if it's called, it will return immediately.\n\nAh.  Okay then.\n\nSorry for the noise,\nDscho"}]}