{"thread":{"id":"42274","subject":"[PATCH 0/2] Work on t3404 in preparation for rebase--helper","startedAt":"2016-05-10T13:59:59Z","lastAt":"2016-05-16T06:33:55Z","messageCount":17,"participants":["Johannes Schindelin","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"286054","messageId":"cover.1462888768.git.johannes.schindelin@gmx.de","threadId":"42274","inReplyTo":null,"subject":"[PATCH 0/2] Work on t3404 in preparation for rebase--helper","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-10T13:59:59Z","receivedAt":"2016-05-10T13:59:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is the first patch series in preparation for a faster interactive\nrebase.\n\nIt actually only prepares the test script that I mainly used to develop\nthe rebase--helper, and the resilience against running with -x proved to\nbe invaluable in keeping my sanity.\n\n\nJohannes Schindelin (2):\n t3404: fix typo\n t3404: be resilient against running with the -x flag\n\n t/t3404-rebase-interactive.sh | 69 ++++++++++---------------------------------\n 1 file changed, 16 insertions(+), 53 deletions(-)\n\nPublished-As: https://github.com/dscho/git/releases/tag/t3404-fixes-v1\n-- \n2.8.2.463.g99156ee\n"},{"id":"286055","messageId":"alpine.DEB.2.20.1605101605480.4092@virtualbox","threadId":"42274","inReplyTo":"cover.1462888768.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/2] t3404: fix typo","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-10T14:05:58Z","receivedAt":"2016-05-10T14:05:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Signed-off-by: Johannes Schindelin \n---\n t/t3404-rebase-interactive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d96d0e4..66348f1 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -62,7 +62,7 @@ test_expect_success 'setup' '\n \n # \"exec\" commands are ran with the user shell by default, but this may\n # be non-POSIX. For example, if SHELL=zsh then \">file\" doesn't work\n-# to create a file. Unseting SHELL avoids such non-portable behavior\n+# to create a file. Unsetting SHELL avoids such non-portable behavior\n # in tests. It must be exported for it to take effect where needed.\n SHELL=\n export SHELL\n-- \n2.8.2.463.g99156ee\n"},{"id":"286056","messageId":"alpine.DEB.2.20.1605101607180.4092@virtualbox","threadId":"42274","inReplyTo":"cover.1462888768.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/2] t3404: be resilient against running with the -x flag","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-10T14:07:22Z","receivedAt":"2016-05-10T14:07:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The -x flag (trace commands) is a priceless tool when hunting down bugs\nthat trigger test failures. It is a worthless tool if the -x flag\n*itself* triggers test failures.\n\nSo let's change the offending tests so that they are a bit less\nstringent and do not stumble over the \"+...\" lines generated by the -x\nflag.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3404-rebase-interactive.sh | 67 ++++++++++---------------------------------\n 1 file changed, 15 insertions(+), 52 deletions(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 66348f1..25f1118 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -882,7 +882,8 @@ test_expect_success 'rebase -i --exec without ' '\n \tgit reset --hard execute &&\n \tset_fake_editor &&\n \ttest_must_fail git rebase -i --exec 2>tmp &&\n-\tsed -e \"1d\" tmp >actual &&\n+\tsed -e \"/option .exec. requires a value/d\" -e '/^+/d' \\\n+\t\ttmp >actual &&\n \ttest_must_fail git rebase -h >expected &&\n \ttest_cmp expected actual &&\n \tgit checkout master\n@@ -1149,10 +1150,6 @@ test_expect_success 'drop' '\n \ttest A = $(git cat-file commit HEAD^^ | sed -ne \\$p)\n '\n \n-cat >expect <actual &&\n \ttest D = $(git cat-file commit HEAD | sed -ne \\$p) &&\n-\ttest_cmp expect actual\n+\ttest_i18ngrep \\\n+\t\t\"Successfully rebased and updated refs/heads/missing-commit.\" \\\n+\t\tactual\n '\n \n-cat >expect <actual &&\n-\ttest_cmp expect actual &&\n+\ttest_i18ngrep \"Warning: some commits may have been dropped\" actual &&\n+\ttest_i18ngrep \"^ - $line\" actual &&\n \ttest D = $(git cat-file commit HEAD | sed -ne \\$p)\n '\n \n-cat >expect <actual &&\n-\ttest_cmp expect actual &&\n+\ttest_i18ngrep \"^ - $line1\" actual &&\n+\ttest_i18ngrep \"^ - $line2\" actual &&\n \tcp .git/rebase-merge/git-rebase-todo.backup \\\n \t\t.git/rebase-merge/git-rebase-todo &&\n \tFAKE_LINES=\"1 2 drop 3 4 drop 5\" \\\n@@ -1215,20 +1193,13 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '\n \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n-cat >expect <actual &&\n-\ttest_cmp expect actual &&\n+\ttest_i18ngrep \"^$line\" actual &&\n \tFAKE_LINES=\"1 2 3 drop 4 5\" git rebase --edit-todo &&\n \tgit rebase --continue &&\n \ttest E = $(git cat-file commit HEAD | sed -ne \\$p) &&\n@@ -1250,20 +1221,12 @@ test_expect_success 'tabs and spaces are accepted in the todolist' '\n \ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n '\n \n-cat >expect <actual &&\n-\ttest_cmp expect actual &&\n+\ttest_i18ngrep \"^ - edit XXXXXXX False commit\" actual &&\n \tFAKE_LINES=\"1 2 4 5 6\" git rebase --edit-todo &&\n \tgit rebase --continue &&\n \ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n-- \n2.8.2.463.g99156ee\n"},{"id":"286101","messageId":"xmqqshxpofqh.fsf@gitster.mtv.corp.google.com","threadId":"42274","inReplyTo":"alpine.DEB.2.20.1605101607180.4092@virtualbox","subject":"Re: [PATCH 2/2] t3404: be resilient against running with the -x flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T19:49:42Z","receivedAt":"2016-05-10T19:49:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> To: Junio C Hamano \n> Cc: git@vger.kernel.org\n\nProbably the above is the other way around.\n\n> The -x flag (trace commands) is a priceless tool when hunting down bugs\n> that trigger test failures. It is a worthless tool if the -x flag\n> *itself* triggers test failures.\n\nTrue.\n\nI wonder if we can fix \"-x\" instead so that we do not have to\nbutcher tests like this patch does. It was quite clear what it\nexpected to see before this patch, and it is sad that the workaround\nmakes less readable (and relies on the real output we are looking\nfor never begins with '+').\n\nI do agree the change from 1d to //d in this patch\nis a very good thing; it makes it clear that we are excluding the\n\"error: \", and expect that after removing the message what follows\nis the help text. And in the spirit of that change, I think it is\nbetter to match /^error: / instead of /option .exec. requires.../.\n\n> So let's change the offending tests so that they are a bit less\n> stringent and do not stumble over the \"+...\" lines generated by the -x\n> flag.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> t/t3404-rebase-interactive.sh | 67 ++++++++++---------------------------------\n> 1 file changed, 15 insertions(+), 52 deletions(-)\n>\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 66348f1..25f1118 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -882,7 +882,8 @@ test_expect_success 'rebase -i --exec without ' '\n> \tgit reset --hard execute &&\n> \tset_fake_editor &&\n> \ttest_must_fail git rebase -i --exec 2>tmp &&\n> -\tsed -e \"1d\" tmp >actual &&\n> +\tsed -e \"/option .exec. requires a value/d\" -e '/^+/d' \\\n> +\t\ttmp >actual &&\n> \ttest_must_fail git rebase -h >expected &&\n> \ttest_cmp expected actual &&\n> \tgit checkout master\n> @@ -1149,10 +1150,6 @@ test_expect_success 'drop' '\n> \ttest A = $(git cat-file commit HEAD^^ | sed -ne \\$p)\n> '\n> \n> -cat >expect < -Successfully rebased and updated refs/heads/missing-commit.\n> -EOF\n> -\n> test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '\n> \ttest_config rebase.missingCommitsCheck ignore &&\n> \trebase_setup_and_clean missing-commit &&\n> @@ -1160,52 +1157,33 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '\n> \tFAKE_LINES=\"1 2 3 4\" \\\n> \t\tgit rebase -i --root 2>actual &&\n> \ttest D = $(git cat-file commit HEAD | sed -ne \\$p) &&\n> -\ttest_cmp expect actual\n> +\ttest_i18ngrep \\\n> +\t\t\"Successfully rebased and updated refs/heads/missing-commit.\" \\\n> +\t\tactual\n\nIs this string going to be i18n'ed? If so this change is good, but\nit probably wants to be a separate \"prepare for i18n\" patch, not\nthis \"work around -x that pollutes 2>actual output\" patch.\n\n> test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '\n> +\tline=\"$(git rev-list --pretty=oneline --abbrev-commit -1 master)\" &&\n> \ttest_config rebase.missingCommitsCheck warn &&\n> \trebase_setup_and_clean missing-commit &&\n> \tset_fake_editor &&\n> \tFAKE_LINES=\"1 2 3 4\" \\\n> \t\tgit rebase -i --root 2>actual &&\n> -\ttest_cmp expect actual &&\n> +\ttest_i18ngrep \"Warning: some commits may have been dropped\" actual &&\n> +\ttest_i18ngrep \"^ - $line\" actual &&\n\nThe former is good in \"prepare for i18n\" patch. The latter is not.\n\ntest_i18ngrep is primarily to make sure what is *not* supposed to be\nlocalized are not localized. GETTEXT_POISON build-time option\nbuilds Git with garbage translations for strings marked for\nlocalization, and test_i18ngrep knows to pretend the test always\npasses in POISON build.\n\nWe test things that are _not_ to be localized with \"grep\", so a test\nwith POISON build will catch if a string (like plumbing output) that\nare not supposed to be localized is marked for localization by\nmistake.\n\nI stop quoting here, but I think the remainder has the same\nover-eager use of i18ngrep.\n"},{"id":"286113","messageId":"20160510205800.GC19958@sigill.intra.peff.net","threadId":"42274","inReplyTo":"xmqqshxpofqh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] t3404: be resilient against running with the -x flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-10T20:58:01Z","receivedAt":"2016-05-10T20:58:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 10, 2016 at 12:49:42PM -0700, Junio C Hamano wrote:\n\n> I wonder if we can fix \"-x\" instead so that we do not have to\n> butcher tests like this patch does. It was quite clear what it\n> expected to see before this patch, and it is sad that the workaround\n> makes less readable (and relies on the real output we are looking\n> for never begins with '+').\n\nI don't think there is a scalable, portable way to do so. \"-x\" output is\ngoing to stderr, and is inherited by any functions or subshells. So\neither we have to ask \"-x\" output to go somewhere else, or we have to\nturn it off inside the functions and subshells. The latter requires\ntweaking each site, which isn't scalable. And there is no way to do the\nformer in a portable way (AFAIK).\n\nThat being said, bash supports BASH_XTRACEFD, so maybe something like\nthis:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 286c5f3..482ec11 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -321,6 +321,7 @@ then\n else\n \texec 4>/dev/null 3>/dev/null\n fi\n+BASH_XTRACEFD=4\n \n test_failure=0\n test_count=0\n\nwould help Dscho's case (and people on other shells aren't helped, but\nthey are not hurt either).\n\n-Peff\n"},{"id":"286117","messageId":"xmqqpostmxah.fsf@gitster.mtv.corp.google.com","threadId":"42274","inReplyTo":"20160510205800.GC19958@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] t3404: be resilient against running with the -x flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T21:13:26Z","receivedAt":"2016-05-10T21:13:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, May 10, 2016 at 12:49:42PM -0700, Junio C Hamano wrote:\n>\n>> I wonder if we can fix \"-x\" instead so that we do not have to\n>> butcher tests like this patch does. It was quite clear what it\n>> expected to see before this patch, and it is sad that the workaround\n>> makes less readable (and relies on the real output we are looking\n>> for never begins with '+').\n>\n> I don't think there is a scalable, portable way to do so. \"-x\" output is\n> going to stderr, and is inherited by any functions or subshells. So\n> either we have to ask \"-x\" output to go somewhere else, or we have to\n> turn it off inside the functions and subshells. The latter requires\n> tweaking each site, which isn't scalable. And there is no way to do the\n> former in a portable way (AFAIK).\n\nYeah, that was the conclusion I was coming to; the same \"unscalable\"\nargument applies to the patch under discussion, too.\n\n> That being said, bash supports BASH_XTRACEFD, so maybe something like\n> this:\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 286c5f3..482ec11 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -321,6 +321,7 @@ then\n> else\n> \texec 4>/dev/null 3>/dev/null\n> fi\n> +BASH_XTRACEFD=4\n> \n> test_failure=0\n> test_count=0\n>\n> would help Dscho's case (and people on other shells aren't helped, but\n> they are not hurt either).\n\nYeah, something like that I would greatly appreciate.\n"},{"id":"286119","messageId":"20160510213218.GA27028@sigill.intra.peff.net","threadId":"42274","inReplyTo":"xmqqpostmxah.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] test-lib: set BASH_XTRACEFD automatically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-10T21:32:18Z","receivedAt":"2016-05-10T21:32:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 10, 2016 at 02:13:26PM -0700, Junio C Hamano wrote:\n\n> > I don't think there is a scalable, portable way to do so. \"-x\" output is\n> > going to stderr, and is inherited by any functions or subshells. So\n> > either we have to ask \"-x\" output to go somewhere else, or we have to\n> > turn it off inside the functions and subshells. The latter requires\n> > tweaking each site, which isn't scalable. And there is no way to do the\n> > former in a portable way (AFAIK).\n> \n> Yeah, that was the conclusion I was coming to; the same \"unscalable\"\n> argument applies to the patch under discussion, too.\n\nI think munging the tests themselves is even more horrible than tweaking\ntest_must_fail, but even the latter is not very scalable (test_must_fail\nis undoubtedly the most common function, but it's not the only one; and\none may actually want to see its trace output anyway).\n\n> > +BASH_XTRACEFD=4\n> [...]\n> \n> Yeah, something like that I would greatly appreciate.\n\nHere it is in patch form. It's sad that we can't automatically help\npeople using dash (which includes myself). I suppose we could\nauto-reinvoke ourselves using bash if it is available, but that feels a\nbit too magical. I'm content to let \"try running with bash\" be a tool in\nour toolbox.\n\n-- >8 --\nSubject: [PATCH] test-lib: set BASH_XTRACEFD automatically\n\nPassing \"-x\" to a test script enables the shell's \"set -x\"\ntracing, which can help with tracking down the command that\nis causing a failure. Unfortunately, it can also _cause_\nfailures in some tests that redirect the stderr of a shell\nfunction. Inside the function the shell continues to\nrespect \"set -x\", and the trace output is collected along\nwith whatever stderr is generated normally by the function.\n\nYou can see an example of this by running:\n\n ./t0040-parse-options.sh -x -i\n\nwhich will fail immediately in the first test, as it\nexpects:\n\n test_must_fail some-cmd 2>output.err\n\nto leave output.err empty (but with \"-x\" it has our trace\noutput).\n\nUnfortunately there isn't a portable or scalable solution to\nthis. We could teach test_must_fail to disable \"set -x\", but\nthat doesn't help any of the other functions or subshells.\n\nHowever, we can work around it by pointing the \"set -x\"\noutput to our descriptor 4, which always points to the\noriginal stderr of the test script. Unfortunately this only\nworks for bash, but it's better than nothing (and other\nshells will just ignore the BASH_XTRACEFD variable).\n\nThe patch itself is a trivial-looking one-liner, but there\nare a few subtleties worth mentioning:\n\n - the variable is _not_ exported; the \"set -x\" is local to\n our process, and so the tracefd should match\n\n - this line has to come after we do the redirection for fd\n 4, as bash will otherwise complain during the variable\n assignment\n\n - likewise, we cannot ever unset this variable, as it\n would close descriptor 4\n\n - setting it once here is sufficient to cover both the\n regular \"-x\" case (which implies \"--verbose\"), as well\n as \"--verbose-only=1\". This works because the latter\n flips \"set -x\" off and on for particular tests (if it\n didn't, we would get tracing for all tests, as going to\n descriptor 4 effectively circumvents the verbose flag).\n\nSigned-off-by: Jeff King \n---\n t/README | 6 +++---\n t/test-lib.sh | 1 +\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 1dc908e..76a0daa 100644\n--- a/t/README\n+++ b/t/README\n@@ -84,9 +84,9 @@ appropriately before running \"make\".\n \n -x::\n \tTurn on shell tracing (i.e., `set -x`) during the tests\n-\tthemselves. Implies `--verbose`. Note that this can cause\n-\tfailures in some tests which redirect and test the\n-\toutput of shell functions. Use with caution.\n+\tthemselves. Implies `--verbose`. Note that in non-bash shells,\n+\tthis can cause failures in some tests which redirect and test\n+\tthe output of shell functions. Use with caution.\n \n -d::\n --debug::\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 286c5f3..482ec11 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -321,6 +321,7 @@ then\n else\n \texec 4>/dev/null 3>/dev/null\n fi\n+BASH_XTRACEFD=4\n \n test_failure=0\n test_count=0\n-- \n2.8.2.660.ge43c418\n"},{"id":"286122","messageId":"xmqqd1otmut8.fsf@gitster.mtv.corp.google.com","threadId":"42274","inReplyTo":"20160510213218.GA27028@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: set BASH_XTRACEFD automatically","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T22:06:59Z","receivedAt":"2016-05-10T22:06:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The patch itself is a trivial-looking one-liner, but there\n> are a few subtleties worth mentioning:\n>\n> - the variable is _not_ exported; the \"set -x\" is local to\n> our process, and so the tracefd should match\n>\n> - this line has to come after we do the redirection for fd\n> 4, as bash will otherwise complain during the variable\n> assignment\n>\n> - likewise, we cannot ever unset this variable, as it\n> would close descriptor 4\n>\n> - setting it once here is sufficient to cover both the\n> regular \"-x\" case (which implies \"--verbose\"), as well\n> as \"--verbose-only=1\". This works because the latter\n> flips \"set -x\" off and on for particular tests (if it\n> didn't, we would get tracing for all tests, as going to\n> descriptor 4 effectively circumvents the verbose flag).\n\nThanks. Some of them may deserve to be next to the one-liner to\nprevent people from making changes that tickle them?\n\n> Signed-off-by: Jeff King \n> ---\n> t/README | 6 +++---\n> t/test-lib.sh | 1 +\n> 2 files changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/README b/t/README\n> index 1dc908e..76a0daa 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -84,9 +84,9 @@ appropriately before running \"make\".\n> \n> -x::\n> \tTurn on shell tracing (i.e., `set -x`) during the tests\n> -\tthemselves. Implies `--verbose`. Note that this can cause\n> -\tfailures in some tests which redirect and test the\n> -\toutput of shell functions. Use with caution.\n> +\tthemselves. Implies `--verbose`. Note that in non-bash shells,\n> +\tthis can cause failures in some tests which redirect and test\n> +\tthe output of shell functions. Use with caution.\n> \n> -d::\n> --debug::\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 286c5f3..482ec11 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -321,6 +321,7 @@ then\n> else\n> \texec 4>/dev/null 3>/dev/null\n> fi\n> +BASH_XTRACEFD=4\n> \n> test_failure=0\n> test_count=0\n"},{"id":"286266","messageId":"20160511134404.GB19356@sigill.intra.peff.net","threadId":"42274","inReplyTo":"xmqqd1otmut8.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] test-lib: set BASH_XTRACEFD automatically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-11T13:44:04Z","receivedAt":"2016-05-11T13:44:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 10, 2016 at 03:06:59PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The patch itself is a trivial-looking one-liner, but there\n> > are a few subtleties worth mentioning:\n> >\n> > - the variable is _not_ exported; the \"set -x\" is local to\n> > our process, and so the tracefd should match\n> >\n> > - this line has to come after we do the redirection for fd\n> > 4, as bash will otherwise complain during the variable\n> > assignment\n> >\n> > - likewise, we cannot ever unset this variable, as it\n> > would close descriptor 4\n> >\n> > - setting it once here is sufficient to cover both the\n> > regular \"-x\" case (which implies \"--verbose\"), as well\n> > as \"--verbose-only=1\". This works because the latter\n> > flips \"set -x\" off and on for particular tests (if it\n> > didn't, we would get tracing for all tests, as going to\n> > descriptor 4 effectively circumvents the verbose flag).\n> \n> Thanks. Some of them may deserve to be next to the one-liner to\n> prevent people from making changes that tickle them?\n\nGood idea. Here's a v2 that moves most of that text into a comment.\n\n-- >8 --\nSubject: test-lib: set BASH_XTRACEFD automatically\n\nPassing \"-x\" to a test script enables the shell's \"set -x\"\ntracing, which can help with tracking down the command that\nis causing a failure. Unfortunately, it can also _cause_\nfailures in some tests that redirect the stderr of a shell\nfunction. Inside the function the shell continues to\nrespect \"set -x\", and the trace output is collected along\nwith whatever stderr is generated normally by the function.\n\nYou can see an example of this by running:\n\n ./t0040-parse-options.sh -x -i\n\nwhich will fail immediately in the first test, as it\nexpects:\n\n test_must_fail some-cmd 2>output.err\n\nto leave output.err empty (but with \"-x\" it has our trace\noutput).\n\nUnfortunately there isn't a portable or scalable solution to\nthis. We could teach test_must_fail to disable \"set -x\", but\nthat doesn't help any of the other functions or subshells.\n\nHowever, we can work around it by pointing the \"set -x\"\noutput to our descriptor 4, which always points to the\noriginal stderr of the test script. Unfortunately this only\nworks for bash, but it's better than nothing (and other\nshells will just ignore the BASH_XTRACEFD variable).\n\nThe patch itself is a simple one-liner, but note the caveats\nin the accompanying comments.\n\nAutomatic tests for our \"-x\" option may be a bit too meta\n(and a pain, because they are bash-specific), but I did\nconfirm that it works correctly both with regular \"-x\" and\nwith \"--verbose-only=1\". This works because the latter flips\n\"set -x\" off and on for particular tests (if it didn't, we\nwould get tracing for all tests, as going to descriptor 4\neffectively circumvents the verbose flag).\n\nSigned-off-by: Jeff King \n---\n t/README | 6 +++---\n t/test-lib.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 1dc908e..76a0daa 100644\n--- a/t/README\n+++ b/t/README\n@@ -84,9 +84,9 @@ appropriately before running \"make\".\n \n -x::\n \tTurn on shell tracing (i.e., `set -x`) during the tests\n-\tthemselves. Implies `--verbose`. Note that this can cause\n-\tfailures in some tests which redirect and test the\n-\toutput of shell functions. Use with caution.\n+\tthemselves. Implies `--verbose`. Note that in non-bash shells,\n+\tthis can cause failures in some tests which redirect and test\n+\tthe output of shell functions. Use with caution.\n \n -d::\n --debug::\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 286c5f3..0055ebb 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -322,6 +322,19 @@ else\n \texec 4>/dev/null 3>/dev/null\n fi\n \n+# Send any \"-x\" output directly to stderr to avoid polluting tests\n+# which capture stderr. We can do this unconditionally since it\n+# has no effect if tracing isn't turned on.\n+#\n+# Note that this sets up the trace fd as soon as we assign the variable, so it\n+# must come after the creation of descriptor 4 above. Likewise, we must never\n+# unset this, as it has the side effect of closing descriptor 4, which we\n+# use to show verbose tests to the user.\n+#\n+# Note also that we don't need or want to export it. The tracing is local to\n+# this shell, and we would not want to influence any shells we exec.\n+BASH_XTRACEFD=4\n+\n test_failure=0\n test_count=0\n test_fixed=0\n-- \n2.8.2.812.gd91b08f\n"},{"id":"286331","messageId":"alpine.DEB.2.20.1605121738260.4092@virtualbox","threadId":"42274","inReplyTo":"20160511134404.GB19356@sigill.intra.peff.net","subject":"Re: [PATCH v2] test-lib: set BASH_XTRACEFD automatically","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-12T15:39:53Z","receivedAt":"2016-05-12T15:39:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 11 May 2016, Jeff King wrote:\n\n> Subject: test-lib: set BASH_XTRACEFD automatically\n\nI confirm that this simple weird trick obsoletes my painstakingly\ndeveloped patch ;-)\n\nTo make it easy for everybody involved, I'll just go ahead and send out\nthe next iteration with your patch.\n\nThanks,\nDscho\n"},{"id":"286334","messageId":"cover.1463067811.git.johannes.schindelin@gmx.de","threadId":"42274","inReplyTo":"cover.1462888768.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 0/2] Work on t3404 in preparation for rebase--helper","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-12T15:43:54Z","receivedAt":"2016-05-12T15:43:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is the first patch series in preparation for a faster interactive\nrebase.\n\nIt actually only prepares the test script that I mainly used to develop\nthe rebase--helper, and the resilience against running with -x proved to\nbe invaluable in keeping my sanity.\n\n\nJeff King (1):\n test-lib: set BASH_XTRACEFD automatically\n\nJohannes Schindelin (1):\n t3404: fix typo\n\n t/README | 6 +++---\n t/t3404-rebase-interactive.sh | 2 +-\n t/test-lib.sh | 13 +++++++++++++\n 3 files changed, 17 insertions(+), 4 deletions(-)\n\nPublished-As: https://github.com/dscho/git/releases/tag/t3404-fixes-v2\nInterdiff vs v1:\n\n diff --git a/t/README b/t/README\n index 1dc908e..76a0daa 100644\n --- a/t/README\n +++ b/t/README\n @@ -84,9 +84,9 @@ appropriately before running \"make\".\n \n -x::\n \tTurn on shell tracing (i.e., `set -x`) during the tests\n -\tthemselves. Implies `--verbose`. Note that this can cause\n -\tfailures in some tests which redirect and test the\n -\toutput of shell functions. Use with caution.\n +\tthemselves. Implies `--verbose`. Note that in non-bash shells,\n +\tthis can cause failures in some tests which redirect and test\n +\tthe output of shell functions. Use with caution.\n \n -d::\n --debug::\n diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n index 25f1118..66348f1 100755\n --- a/t/t3404-rebase-interactive.sh\n +++ b/t/t3404-rebase-interactive.sh\n @@ -882,8 +882,7 @@ test_expect_success 'rebase -i --exec without ' '\n \tgit reset --hard execute &&\n \tset_fake_editor &&\n \ttest_must_fail git rebase -i --exec 2>tmp &&\n -\tsed -e \"/option .exec. requires a value/d\" -e '/^+/d' \\\n -\t\ttmp >actual &&\n +\tsed -e \"1d\" tmp >actual &&\n \ttest_must_fail git rebase -h >expected &&\n \ttest_cmp expected actual &&\n \tgit checkout master\n @@ -1150,6 +1149,10 @@ test_expect_success 'drop' '\n \ttest A = $(git cat-file commit HEAD^^ | sed -ne \\$p)\n '\n \n +cat >expect <actual &&\n \ttest D = $(git cat-file commit HEAD | sed -ne \\$p) &&\n -\ttest_i18ngrep \\\n -\t\t\"Successfully rebased and updated refs/heads/missing-commit.\" \\\n -\t\tactual\n +\ttest_cmp expect actual\n '\n \n +cat >expect <actual &&\n -\ttest_i18ngrep \"Warning: some commits may have been dropped\" actual &&\n -\ttest_i18ngrep \"^ - $line\" actual &&\n +\ttest_cmp expect actual &&\n \ttest D = $(git cat-file commit HEAD | sed -ne \\$p)\n '\n \n +cat >expect <actual &&\n -\ttest_i18ngrep \"^ - $line1\" actual &&\n -\ttest_i18ngrep \"^ - $line2\" actual &&\n +\ttest_cmp expect actual &&\n \tcp .git/rebase-merge/git-rebase-todo.backup \\\n \t\t.git/rebase-merge/git-rebase-todo &&\n \tFAKE_LINES=\"1 2 drop 3 4 drop 5\" \\\n @@ -1193,13 +1215,20 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '\n \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n +cat >expect <actual &&\n -\ttest_i18ngrep \"^$line\" actual &&\n +\ttest_cmp expect actual &&\n \tFAKE_LINES=\"1 2 3 drop 4 5\" git rebase --edit-todo &&\n \tgit rebase --continue &&\n \ttest E = $(git cat-file commit HEAD | sed -ne \\$p) &&\n @@ -1221,12 +1250,20 @@ test_expect_success 'tabs and spaces are accepted in the todolist' '\n \ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n '\n \n +cat >expect <actual &&\n -\ttest_i18ngrep \"^ - edit XXXXXXX False commit\" actual &&\n +\ttest_cmp expect actual &&\n \tFAKE_LINES=\"1 2 4 5 6\" git rebase --edit-todo &&\n \tgit rebase --continue &&\n \ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n diff --git a/t/test-lib.sh b/t/test-lib.sh\n index 286c5f3..0055ebb 100644\n --- a/t/test-lib.sh\n +++ b/t/test-lib.sh\n @@ -322,6 +322,19 @@ else\n \texec 4>/dev/null 3>/dev/null\n fi\n \n +# Send any \"-x\" output directly to stderr to avoid polluting tests\n +# which capture stderr. We can do this unconditionally since it\n +# has no effect if tracing isn't turned on.\n +#\n +# Note that this sets up the trace fd as soon as we assign the variable, so it\n +# must come after the creation of descriptor 4 above. Likewise, we must never\n +# unset this, as it has the side effect of closing descriptor 4, which we\n +# use to show verbose tests to the user.\n +#\n +# Note also that we don't need or want to export it. The tracing is local to\n +# this shell, and we would not want to influence any shells we exec.\n +BASH_XTRACEFD=4\n +\n test_failure=0\n test_count=0\n test_fixed=0\n\n-- \n2.8.2.465.gb077790\n"},{"id":"286333","messageId":"79ea39ba9583465cdd6e908dcd7ef056591d941e.1463067811.git.johannes.schindelin@gmx.de","threadId":"42274","inReplyTo":"cover.1463067811.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/2] t3404: fix typo","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-12T15:44:01Z","receivedAt":"2016-05-12T15:44:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Signed-off-by: Johannes Schindelin \n---\n t/t3404-rebase-interactive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d96d0e4..66348f1 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -62,7 +62,7 @@ test_expect_success 'setup' '\n \n # \"exec\" commands are ran with the user shell by default, but this may\n # be non-POSIX. For example, if SHELL=zsh then \">file\" doesn't work\n-# to create a file. Unseting SHELL avoids such non-portable behavior\n+# to create a file. Unsetting SHELL avoids such non-portable behavior\n # in tests. It must be exported for it to take effect where needed.\n SHELL=\n export SHELL\n-- \n2.8.2.465.gb077790\n"},{"id":"286336","messageId":"ffc132a9d6cdb97c49861bd9b6b2c6aca42cec93.1463067811.git.johannes.schindelin@gmx.de","threadId":"42274","inReplyTo":"cover.1463067811.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/2] test-lib: set BASH_XTRACEFD automatically","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-12T15:44:12Z","receivedAt":"2016-05-12T15:44:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Jeff King \n\nPassing \"-x\" to a test script enables the shell's \"set -x\"\ntracing, which can help with tracking down the command that\nis causing a failure. Unfortunately, it can also _cause_\nfailures in some tests that redirect the stderr of a shell\nfunction. Inside the function the shell continues to\nrespect \"set -x\", and the trace output is collected along\nwith whatever stderr is generated normally by the function.\n\nYou can see an example of this by running:\n\n ./t0040-parse-options.sh -x -i\n\nwhich will fail immediately in the first test, as it\nexpects:\n\n test_must_fail some-cmd 2>output.err\n\nto leave output.err empty (but with \"-x\" it has our trace\noutput).\n\nUnfortunately there isn't a portable or scalable solution to\nthis. We could teach test_must_fail to disable \"set -x\", but\nthat doesn't help any of the other functions or subshells.\n\nHowever, we can work around it by pointing the \"set -x\"\noutput to our descriptor 4, which always points to the\noriginal stderr of the test script. Unfortunately this only\nworks for bash, but it's better than nothing (and other\nshells will just ignore the BASH_XTRACEFD variable).\n\nThe patch itself is a simple one-liner, but note the caveats\nin the accompanying comments.\n\nAutomatic tests for our \"-x\" option may be a bit too meta\n(and a pain, because they are bash-specific), but I did\nconfirm that it works correctly both with regular \"-x\" and\nwith \"--verbose-only=1\". This works because the latter flips\n\"set -x\" off and on for particular tests (if it didn't, we\nwould get tracing for all tests, as going to descriptor 4\neffectively circumvents the verbose flag).\n\nSigned-off-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n t/README | 6 +++---\n t/test-lib.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 1dc908e..76a0daa 100644\n--- a/t/README\n+++ b/t/README\n@@ -84,9 +84,9 @@ appropriately before running \"make\".\n \n -x::\n \tTurn on shell tracing (i.e., `set -x`) during the tests\n-\tthemselves. Implies `--verbose`. Note that this can cause\n-\tfailures in some tests which redirect and test the\n-\toutput of shell functions. Use with caution.\n+\tthemselves. Implies `--verbose`. Note that in non-bash shells,\n+\tthis can cause failures in some tests which redirect and test\n+\tthe output of shell functions. Use with caution.\n \n -d::\n --debug::\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 286c5f3..0055ebb 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -322,6 +322,19 @@ else\n \texec 4>/dev/null 3>/dev/null\n fi\n \n+# Send any \"-x\" output directly to stderr to avoid polluting tests\n+# which capture stderr. We can do this unconditionally since it\n+# has no effect if tracing isn't turned on.\n+#\n+# Note that this sets up the trace fd as soon as we assign the variable, so it\n+# must come after the creation of descriptor 4 above. Likewise, we must never\n+# unset this, as it has the side effect of closing descriptor 4, which we\n+# use to show verbose tests to the user.\n+#\n+# Note also that we don't need or want to export it. The tracing is local to\n+# this shell, and we would not want to influence any shells we exec.\n+BASH_XTRACEFD=4\n+\n test_failure=0\n test_count=0\n test_fixed=0\n-- \n2.8.2.465.gb077790\n"},{"id":"286338","messageId":"xmqq8tzfgsbd.fsf@gitster.mtv.corp.google.com","threadId":"42274","inReplyTo":"cover.1463067811.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 0/2] Work on t3404 in preparation for rebase--helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-12T16:21:58Z","receivedAt":"2016-05-12T16:21:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I took these separately already, and plan to fast-track them as they\nare both \"trivially correct\"; I double checked that what I have\nmatch these two, too.\n\nThanks.\n"},{"id":"286432","messageId":"alpine.DEB.2.20.1605130835070.4092@virtualbox","threadId":"42274","inReplyTo":"xmqq8tzfgsbd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/2] Work on t3404 in preparation for rebase--helper","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-13T06:37:36Z","receivedAt":"2016-05-13T06:37:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 12 May 2016, Junio C Hamano wrote:\n\n> I took these separately already, and plan to fast-track them as they are\n> both \"trivially correct\"; I double checked that what I have match these\n> two, too.\n\nOh, okay. I just wanted to make things easier for you, and now that I have\na script to prepare patch series, it's really almost as trivial for me to\nsend out a new iteration as it would be to update a Pull Request on\nGitHub.\n\nDo you want me to hold off with new iterations in the future until you\nclarified your preferred course of action?\n\nCiao,\nDscho\n"},{"id":"286472","messageId":"xmqqa8jtaliw.fsf@gitster.mtv.corp.google.com","threadId":"42274","inReplyTo":"alpine.DEB.2.20.1605130835070.4092@virtualbox","subject":"Re: [PATCH v2 0/2] Work on t3404 in preparation for rebase--helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-13T17:57:27Z","receivedAt":"2016-05-13T17:57:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Thu, 12 May 2016, Junio C Hamano wrote:\n>\n>> I took these separately already, and plan to fast-track them as they are\n>> both \"trivially correct\"; I double checked that what I have match these\n>> two, too.\n>\n> Oh, okay. I just wanted to make things easier for you, and now that I have\n> a script to prepare patch series, it's really almost as trivial for me to\n> send out a new iteration as it would be to update a Pull Request on\n> GitHub.\n>\n> Do you want me to hold off with new iterations in the future until you\n> clarified your preferred course of action?\n\nNo. You've been doing great. I just wanted to clarify what I did\nto your patch before I merge them separately to 'next' ahead of the\nremainder that you'd be sending out, expecting a possible course\ncorrection, e.g. \"that would make it harder to queue the other patches\nyet to come, all of which would depend on both of them--it would be\nbetter to queue them on a single topic to be extended with these\nother patches, after all these two are not that urgent\".\n"},{"id":"286646","messageId":"alpine.DEB.2.20.1605160830340.3303@virtualbox","threadId":"42274","inReplyTo":"xmqqa8jtaliw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/2] Work on t3404 in preparation for rebase--helper","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-16T06:33:55Z","receivedAt":"2016-05-16T06:33:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 13 May 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Thu, 12 May 2016, Junio C Hamano wrote:\n> >\n> >> I took these separately already, and plan to fast-track them as they\n> >> are both \"trivially correct\"; I double checked that what I have match\n> >> these two, too.\n> >\n> > Oh, okay. I just wanted to make things easier for you, and now that I\n> > have a script to prepare patch series, it's really almost as trivial\n> > for me to send out a new iteration as it would be to update a Pull\n> > Request on GitHub.\n> >\n> > Do you want me to hold off with new iterations in the future until you\n> > clarified your preferred course of action?\n> \n> No. You've been doing great. I just wanted to clarify what I did\n> to your patch before I merge them separately to 'next' ahead of the\n> remainder that you'd be sending out, expecting a possible course\n> correction, e.g. \"that would make it harder to queue the other patches\n> yet to come, all of which would depend on both of them--it would be\n> better to queue them on a single topic to be extended with these\n> other patches, after all these two are not that urgent\".\n\nThanks.\n\nI planned to work on the remainder as a \"topic thicket\" using my Git\ngarden shears [*1*] anyway, picking up the changes you picked up,\nreplacing my original patches.\n\nCiao,\nDscho\n\nFootnote *1*: The garden shears are kind of a `git rebase -i -p` as I wish\nI had designed it originally. Thet live here:\nhttps://github.com/git-for-windows/build-extra/blob/master/shears.sh\n"}]}