[PATCH 0/2] Work on t3404 in preparation for rebase--helper

[PATCH 0/2] Work on t3404 in preparation for rebase--helper

[Johannes Schindelin]

This is the first patch series in preparation for a faster interactive
rebase.

It actually only prepares the test script that I mainly used to develop
the rebase--helper, and the resilience against running with -x proved to
be invaluable in keeping my sanity.


Johannes Schindelin (2):
  t3404: fix typo
  t3404: be resilient against running with the -x flag

 t/t3404-rebase-interactive.sh | 69 ++++++++++---------------------------------
 1 file changed, 16 insertions(+), 53 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/t3404-fixes-v1
-- 
2.8.2.463.g99156ee

[PATCH 1/2] t3404: fix typo

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3404-rebase-interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d96d0e4..66348f1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -62,7 +62,7 @@ test_expect_success 'setup' '
 
 # "exec" commands are ran with the user shell by default, but this may
 # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
-# to create a file. Unseting SHELL avoids such non-portable behavior
+# to create a file. Unsetting SHELL avoids such non-portable behavior
 # in tests. It must be exported for it to take effect where needed.
 SHELL=
 export SHELL
-- 
2.8.2.463.g99156ee

[PATCH 2/2] t3404: be resilient against running with the -x flag

[Johannes Schindelin]

The -x flag (trace commands) is a priceless tool when hunting down bugs
that trigger test failures. It is a worthless tool if the -x flag
*itself* triggers test failures.

So let's change the offending tests so that they are a bit less
stringent and do not stumble over the "+..." lines generated by the -x
flag.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3404-rebase-interactive.sh | 67 ++++++++++---------------------------------
 1 file changed, 15 insertions(+), 52 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 66348f1..25f1118 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -882,7 +882,8 @@ test_expect_success 'rebase -i --exec without <CMD>' '
 	git reset --hard execute &&
 	set_fake_editor &&
 	test_must_fail git rebase -i --exec 2>tmp &&
-	sed -e "1d" tmp >actual &&
+	sed -e "/option .exec. requires a value/d" -e '/^+/d' \
+		tmp >actual &&
 	test_must_fail git rebase -h >expected &&
 	test_cmp expected actual &&
 	git checkout master
@@ -1149,10 +1150,6 @@ test_expect_success 'drop' '
 	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
 '
 
-cat >expect <<EOF
-Successfully rebased and updated refs/heads/missing-commit.
-EOF
-
 test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
 	test_config rebase.missingCommitsCheck ignore &&
 	rebase_setup_and_clean missing-commit &&
@@ -1160,52 +1157,33 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
 	FAKE_LINES="1 2 3 4" \
 		git rebase -i --root 2>actual &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
-	test_cmp expect actual
+	test_i18ngrep \
+		"Successfully rebased and updated refs/heads/missing-commit." \
+		actual
 '
 
-cat >expect <<EOF
-Warning: some commits may have been dropped accidentally.
-Dropped commits (newer to older):
- - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
-To avoid this message, use "drop" to explicitly remove a commit.
-
-Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
-The possible behaviours are: ignore, warn, error.
-
-Successfully rebased and updated refs/heads/missing-commit.
-EOF
-
 test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
+	line="$(git rev-list --pretty=oneline --abbrev-commit -1 master)" &&
 	test_config rebase.missingCommitsCheck warn &&
 	rebase_setup_and_clean missing-commit &&
 	set_fake_editor &&
 	FAKE_LINES="1 2 3 4" \
 		git rebase -i --root 2>actual &&
-	test_cmp expect actual &&
+	test_i18ngrep "Warning: some commits may have been dropped" actual &&
+	test_i18ngrep "^ - $line" actual &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
-cat >expect <<EOF
-Warning: some commits may have been dropped accidentally.
-Dropped commits (newer to older):
- - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
- - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
-To avoid this message, use "drop" to explicitly remove a commit.
-
-Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
-The possible behaviours are: ignore, warn, error.
-
-You can fix this with 'git rebase --edit-todo'.
-Or you can abort the rebase with 'git rebase --abort'.
-EOF
-
 test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
+	line1="$(git rev-list --pretty=oneline --abbrev-commit -1 master)" &&
+	line2="$(git rev-list --pretty=oneline --abbrev-commit -1 master~2)" &&
 	test_config rebase.missingCommitsCheck error &&
 	rebase_setup_and_clean missing-commit &&
 	set_fake_editor &&
 	test_must_fail env FAKE_LINES="1 2 4" \
 		git rebase -i --root 2>actual &&
-	test_cmp expect actual &&
+	test_i18ngrep "^ - $line1" actual &&
+	test_i18ngrep "^ - $line2" actual &&
 	cp .git/rebase-merge/git-rebase-todo.backup \
 		.git/rebase-merge/git-rebase-todo &&
 	FAKE_LINES="1 2 drop 3 4 drop 5" \
@@ -1215,20 +1193,13 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
-cat >expect <<EOF
-Warning: the command isn't recognized in the following line:
- - badcmd $(git rev-list --oneline -1 master~1)
-
-You can fix this with 'git rebase --edit-todo'.
-Or you can abort the rebase with 'git rebase --abort'.
-EOF
-
 test_expect_success 'static check of bad command' '
+	line=" - badcmd $(git rev-list --oneline -1 master~1)" &&
 	rebase_setup_and_clean bad-cmd &&
 	set_fake_editor &&
 	test_must_fail env FAKE_LINES="1 2 3 bad 4 5" \
 		git rebase -i --root 2>actual &&
-	test_cmp expect actual &&
+	test_i18ngrep "^$line" actual &&
 	FAKE_LINES="1 2 3 drop 4 5" git rebase --edit-todo &&
 	git rebase --continue &&
 	test E = $(git cat-file commit HEAD | sed -ne \$p) &&
@@ -1250,20 +1221,12 @@ test_expect_success 'tabs and spaces are accepted in the todolist' '
 	test E = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
-cat >expect <<EOF
-Warning: the SHA-1 is missing or isn't a commit in the following line:
- - edit XXXXXXX False commit
-
-You can fix this with 'git rebase --edit-todo'.
-Or you can abort the rebase with 'git rebase --abort'.
-EOF
-
 test_expect_success 'static check of bad SHA-1' '
 	rebase_setup_and_clean bad-sha &&
 	set_fake_editor &&
 	test_must_fail env FAKE_LINES="1 2 edit fakesha 3 4 5 #" \
 		git rebase -i --root 2>actual &&
-	test_cmp expect actual &&
+	test_i18ngrep "^ - edit XXXXXXX False commit" actual &&
 	FAKE_LINES="1 2 4 5 6" git rebase --edit-todo &&
 	git rebase --continue &&
 	test E = $(git cat-file commit HEAD | sed -ne \$p)
-- 
2.8.2.463.g99156ee

Re: [PATCH 2/2] t3404: be resilient against running with the -x flag

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> To: Junio C Hamano <gitster@pobox.com>
> Cc: git@vger.kernel.org

Probably the above is the other way around.

> The -x flag (trace commands) is a priceless tool when hunting down bugs
> that trigger test failures. It is a worthless tool if the -x flag
> *itself* triggers test failures.

True.

I wonder if we can fix "-x" instead so that we do not have to
butcher tests like this patch does.  It was quite clear what it
expected to see before this patch, and it is sad that the workaround
makes less readable (and relies on the real output we are looking
for never begins with '+').

I do agree the change from 1d to /<expected string>/d in this patch
is a very good thing; it makes it clear that we are excluding the
"error: ", and expect that after removing the message what follows
is the help text.  And in the spirit of that change, I think it is
better to match /^error: / instead of /option .exec. requires.../.

> So let's change the offending tests so that they are a bit less
> stringent and do not stumble over the "+..." lines generated by the -x
> flag.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t3404-rebase-interactive.sh | 67 ++++++++++---------------------------------
>  1 file changed, 15 insertions(+), 52 deletions(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 66348f1..25f1118 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -882,7 +882,8 @@ test_expect_success 'rebase -i --exec without <CMD>' '
>  	git reset --hard execute &&
>  	set_fake_editor &&
>  	test_must_fail git rebase -i --exec 2>tmp &&
> -	sed -e "1d" tmp >actual &&
> +	sed -e "/option .exec. requires a value/d" -e '/^+/d' \
> +		tmp >actual &&
>  	test_must_fail git rebase -h >expected &&
>  	test_cmp expected actual &&
>  	git checkout master
> @@ -1149,10 +1150,6 @@ test_expect_success 'drop' '
>  	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
>  '
>  
> -cat >expect <<EOF
> -Successfully rebased and updated refs/heads/missing-commit.
> -EOF
> -
>  test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
>  	test_config rebase.missingCommitsCheck ignore &&
>  	rebase_setup_and_clean missing-commit &&
> @@ -1160,52 +1157,33 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
>  	FAKE_LINES="1 2 3 4" \
>  		git rebase -i --root 2>actual &&
>  	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> -	test_cmp expect actual
> +	test_i18ngrep \
> +		"Successfully rebased and updated refs/heads/missing-commit." \
> +		actual

Is this string going to be i18n'ed?  If so this change is good, but
it probably wants to be a separate "prepare for i18n" patch, not
this "work around -x that pollutes 2>actual output" patch.

>  test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
> +	line="$(git rev-list --pretty=oneline --abbrev-commit -1 master)" &&
>  	test_config rebase.missingCommitsCheck warn &&
>  	rebase_setup_and_clean missing-commit &&
>  	set_fake_editor &&
>  	FAKE_LINES="1 2 3 4" \
>  		git rebase -i --root 2>actual &&
> -	test_cmp expect actual &&
> +	test_i18ngrep "Warning: some commits may have been dropped" actual &&
> +	test_i18ngrep "^ - $line" actual &&

The former is good in "prepare for i18n" patch.  The latter is not.

test_i18ngrep is primarily to make sure what is *not* supposed to be
localized are not localized.  GETTEXT_POISON build-time option
builds Git with garbage translations for strings marked for
localization, and test_i18ngrep knows to pretend the test always
passes in POISON build.

We test things that are _not_ to be localized with "grep", so a test
with POISON build will catch if a string (like plumbing output) that
are not supposed to be localized is marked for localization by
mistake.

I stop quoting here, but I think the remainder has the same
over-eager use of i18ngrep.

Re: [PATCH 2/2] t3404: be resilient against running with the -x flag

[Jeff King]

On Tue, May 10, 2016 at 12:49:42PM -0700, Junio C Hamano wrote:

> I wonder if we can fix "-x" instead so that we do not have to
> butcher tests like this patch does.  It was quite clear what it
> expected to see before this patch, and it is sad that the workaround
> makes less readable (and relies on the real output we are looking
> for never begins with '+').

I don't think there is a scalable, portable way to do so. "-x" output is
going to stderr, and is inherited by any functions or subshells. So
either we have to ask "-x" output to go somewhere else, or we have to
turn it off inside the functions and subshells. The latter requires
tweaking each site, which isn't scalable. And there is no way to do the
former in a portable way (AFAIK).

That being said, bash supports BASH_XTRACEFD, so maybe something like
this:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 286c5f3..482ec11 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -321,6 +321,7 @@ then
 else
 	exec 4>/dev/null 3>/dev/null
 fi
+BASH_XTRACEFD=4
 
 test_failure=0
 test_count=0

would help Dscho's case (and people on other shells aren't helped, but
they are not hurt either).

-Peff

Re: [PATCH 2/2] t3404: be resilient against running with the -x flag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, May 10, 2016 at 12:49:42PM -0700, Junio C Hamano wrote:
>
>> I wonder if we can fix "-x" instead so that we do not have to
>> butcher tests like this patch does.  It was quite clear what it
>> expected to see before this patch, and it is sad that the workaround
>> makes less readable (and relies on the real output we are looking
>> for never begins with '+').
>
> I don't think there is a scalable, portable way to do so. "-x" output is
> going to stderr, and is inherited by any functions or subshells. So
> either we have to ask "-x" output to go somewhere else, or we have to
> turn it off inside the functions and subshells. The latter requires
> tweaking each site, which isn't scalable. And there is no way to do the
> former in a portable way (AFAIK).

Yeah, that was the conclusion I was coming to; the same "unscalable"
argument applies to the patch under discussion, too.

> That being said, bash supports BASH_XTRACEFD, so maybe something like
> this:
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 286c5f3..482ec11 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -321,6 +321,7 @@ then
>  else
>  	exec 4>/dev/null 3>/dev/null
>  fi
> +BASH_XTRACEFD=4
>  
>  test_failure=0
>  test_count=0
>
> would help Dscho's case (and people on other shells aren't helped, but
> they are not hurt either).

Yeah, something like that I would greatly appreciate.

[PATCH] test-lib: set BASH_XTRACEFD automatically

[Jeff King]

On Tue, May 10, 2016 at 02:13:26PM -0700, Junio C Hamano wrote:

> > I don't think there is a scalable, portable way to do so. "-x" output is
> > going to stderr, and is inherited by any functions or subshells. So
> > either we have to ask "-x" output to go somewhere else, or we have to
> > turn it off inside the functions and subshells. The latter requires
> > tweaking each site, which isn't scalable. And there is no way to do the
> > former in a portable way (AFAIK).
> 
> Yeah, that was the conclusion I was coming to; the same "unscalable"
> argument applies to the patch under discussion, too.

I think munging the tests themselves is even more horrible than tweaking
test_must_fail, but even the latter is not very scalable (test_must_fail
is undoubtedly the most common function, but it's not the only one; and
one may actually want to see its trace output anyway).

> > +BASH_XTRACEFD=4
> [...]
> 
> Yeah, something like that I would greatly appreciate.

Here it is in patch form. It's sad that we can't automatically help
people using dash (which includes myself). I suppose we could
auto-reinvoke ourselves using bash if it is available, but that feels a
bit too magical. I'm content to let "try running with bash" be a tool in
our toolbox.

-- >8 --
Subject: [PATCH] test-lib: set BASH_XTRACEFD automatically

Passing "-x" to a test script enables the shell's "set -x"
tracing, which can help with tracking down the command that
is causing a failure. Unfortunately, it can also _cause_
failures in some tests that redirect the stderr of a shell
function.  Inside the function the shell continues to
respect "set -x", and the trace output is collected along
with whatever stderr is generated normally by the function.

You can see an example of this by running:

  ./t0040-parse-options.sh -x -i

which will fail immediately in the first test, as it
expects:

  test_must_fail some-cmd 2>output.err

to leave output.err empty (but with "-x" it has our trace
output).

Unfortunately there isn't a portable or scalable solution to
this. We could teach test_must_fail to disable "set -x", but
that doesn't help any of the other functions or subshells.

However, we can work around it by pointing the "set -x"
output to our descriptor 4, which always points to the
original stderr of the test script. Unfortunately this only
works for bash, but it's better than nothing (and other
shells will just ignore the BASH_XTRACEFD variable).

The patch itself is a trivial-looking one-liner, but there
are a few subtleties worth mentioning:

  - the variable is _not_ exported; the "set -x" is local to
    our process, and so the tracefd should match

  - this line has to come after we do the redirection for fd
    4, as bash will otherwise complain during the variable
    assignment

  - likewise, we cannot ever unset this variable, as it
    would close descriptor 4

  - setting it once here is sufficient to cover both the
    regular "-x" case (which implies "--verbose"), as well
    as "--verbose-only=1". This works because the latter
    flips "set -x" off and on for particular tests (if it
    didn't, we would get tracing for all tests, as going to
    descriptor 4 effectively circumvents the verbose flag).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/README      | 6 +++---
 t/test-lib.sh | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/README b/t/README
index 1dc908e..76a0daa 100644
--- a/t/README
+++ b/t/README
@@ -84,9 +84,9 @@ appropriately before running "make".
 
 -x::
 	Turn on shell tracing (i.e., `set -x`) during the tests
-	themselves. Implies `--verbose`. Note that this can cause
-	failures in some tests which redirect and test the
-	output of shell functions. Use with caution.
+	themselves. Implies `--verbose`. Note that in non-bash shells,
+	this can cause failures in some tests which redirect and test
+	the output of shell functions. Use with caution.
 
 -d::
 --debug::
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 286c5f3..482ec11 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -321,6 +321,7 @@ then
 else
 	exec 4>/dev/null 3>/dev/null
 fi
+BASH_XTRACEFD=4
 
 test_failure=0
 test_count=0
-- 
2.8.2.660.ge43c418

Re: [PATCH] test-lib: set BASH_XTRACEFD automatically

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The patch itself is a trivial-looking one-liner, but there
> are a few subtleties worth mentioning:
>
>   - the variable is _not_ exported; the "set -x" is local to
>     our process, and so the tracefd should match
>
>   - this line has to come after we do the redirection for fd
>     4, as bash will otherwise complain during the variable
>     assignment
>
>   - likewise, we cannot ever unset this variable, as it
>     would close descriptor 4
>
>   - setting it once here is sufficient to cover both the
>     regular "-x" case (which implies "--verbose"), as well
>     as "--verbose-only=1". This works because the latter
>     flips "set -x" off and on for particular tests (if it
>     didn't, we would get tracing for all tests, as going to
>     descriptor 4 effectively circumvents the verbose flag).

Thanks.  Some of them may deserve to be next to the one-liner to
prevent people from making changes that tickle them?

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/README      | 6 +++---
>  t/test-lib.sh | 1 +
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/t/README b/t/README
> index 1dc908e..76a0daa 100644
> --- a/t/README
> +++ b/t/README
> @@ -84,9 +84,9 @@ appropriately before running "make".
>  
>  -x::
>  	Turn on shell tracing (i.e., `set -x`) during the tests
> -	themselves. Implies `--verbose`. Note that this can cause
> -	failures in some tests which redirect and test the
> -	output of shell functions. Use with caution.
> +	themselves. Implies `--verbose`. Note that in non-bash shells,
> +	this can cause failures in some tests which redirect and test
> +	the output of shell functions. Use with caution.
>  
>  -d::
>  --debug::
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 286c5f3..482ec11 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -321,6 +321,7 @@ then
>  else
>  	exec 4>/dev/null 3>/dev/null
>  fi
> +BASH_XTRACEFD=4
>  
>  test_failure=0
>  test_count=0

[PATCH v2] test-lib: set BASH_XTRACEFD automatically

[Jeff King]

On Tue, May 10, 2016 at 03:06:59PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The patch itself is a trivial-looking one-liner, but there
> > are a few subtleties worth mentioning:
> >
> >   - the variable is _not_ exported; the "set -x" is local to
> >     our process, and so the tracefd should match
> >
> >   - this line has to come after we do the redirection for fd
> >     4, as bash will otherwise complain during the variable
> >     assignment
> >
> >   - likewise, we cannot ever unset this variable, as it
> >     would close descriptor 4
> >
> >   - setting it once here is sufficient to cover both the
> >     regular "-x" case (which implies "--verbose"), as well
> >     as "--verbose-only=1". This works because the latter
> >     flips "set -x" off and on for particular tests (if it
> >     didn't, we would get tracing for all tests, as going to
> >     descriptor 4 effectively circumvents the verbose flag).
> 
> Thanks.  Some of them may deserve to be next to the one-liner to
> prevent people from making changes that tickle them?

Good idea. Here's a v2 that moves most of that text into a comment.

-- >8 --
Subject: test-lib: set BASH_XTRACEFD automatically

Passing "-x" to a test script enables the shell's "set -x"
tracing, which can help with tracking down the command that
is causing a failure. Unfortunately, it can also _cause_
failures in some tests that redirect the stderr of a shell
function.  Inside the function the shell continues to
respect "set -x", and the trace output is collected along
with whatever stderr is generated normally by the function.

You can see an example of this by running:

  ./t0040-parse-options.sh -x -i

which will fail immediately in the first test, as it
expects:

  test_must_fail some-cmd 2>output.err

to leave output.err empty (but with "-x" it has our trace
output).

Unfortunately there isn't a portable or scalable solution to
this. We could teach test_must_fail to disable "set -x", but
that doesn't help any of the other functions or subshells.

However, we can work around it by pointing the "set -x"
output to our descriptor 4, which always points to the
original stderr of the test script. Unfortunately this only
works for bash, but it's better than nothing (and other
shells will just ignore the BASH_XTRACEFD variable).

The patch itself is a simple one-liner, but note the caveats
in the accompanying comments.

Automatic tests for our "-x" option may be a bit too meta
(and a pain, because they are bash-specific), but I did
confirm that it works correctly both with regular "-x" and
with "--verbose-only=1". This works because the latter flips
"set -x" off and on for particular tests (if it didn't, we
would get tracing for all tests, as going to descriptor 4
effectively circumvents the verbose flag).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/README      |  6 +++---
 t/test-lib.sh | 13 +++++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/t/README b/t/README
index 1dc908e..76a0daa 100644
--- a/t/README
+++ b/t/README
@@ -84,9 +84,9 @@ appropriately before running "make".
 
 -x::
 	Turn on shell tracing (i.e., `set -x`) during the tests
-	themselves. Implies `--verbose`. Note that this can cause
-	failures in some tests which redirect and test the
-	output of shell functions. Use with caution.
+	themselves. Implies `--verbose`. Note that in non-bash shells,
+	this can cause failures in some tests which redirect and test
+	the output of shell functions. Use with caution.
 
 -d::
 --debug::
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 286c5f3..0055ebb 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -322,6 +322,19 @@ else
 	exec 4>/dev/null 3>/dev/null
 fi
 
+# Send any "-x" output directly to stderr to avoid polluting tests
+# which capture stderr. We can do this unconditionally since it
+# has no effect if tracing isn't turned on.
+#
+# Note that this sets up the trace fd as soon as we assign the variable, so it
+# must come after the creation of descriptor 4 above. Likewise, we must never
+# unset this, as it has the side effect of closing descriptor 4, which we
+# use to show verbose tests to the user.
+#
+# Note also that we don't need or want to export it. The tracing is local to
+# this shell, and we would not want to influence any shells we exec.
+BASH_XTRACEFD=4
+
 test_failure=0
 test_count=0
 test_fixed=0
-- 
2.8.2.812.gd91b08f

Re: [PATCH v2] test-lib: set BASH_XTRACEFD automatically

[Johannes Schindelin]

Hi Peff,

On Wed, 11 May 2016, Jeff King wrote:

> Subject: test-lib: set BASH_XTRACEFD automatically

I confirm that this simple weird trick obsoletes my painstakingly
developed patch ;-)

To make it easy for everybody involved, I'll just go ahead and send out
the next iteration with your patch.

Thanks,
Dscho

[PATCH v2 0/2] Work on t3404 in preparation for rebase--helper

[Johannes Schindelin]

This is the first patch series in preparation for a faster interactive
rebase.

It actually only prepares the test script that I mainly used to develop
the rebase--helper, and the resilience against running with -x proved to
be invaluable in keeping my sanity.


Jeff King (1):
  test-lib: set BASH_XTRACEFD automatically

Johannes Schindelin (1):
  t3404: fix typo

 t/README                      |  6 +++---
 t/t3404-rebase-interactive.sh |  2 +-
 t/test-lib.sh                 | 13 +++++++++++++
 3 files changed, 17 insertions(+), 4 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/t3404-fixes-v2
Interdiff vs v1:

 diff --git a/t/README b/t/README
 index 1dc908e..76a0daa 100644
 --- a/t/README
 +++ b/t/README
 @@ -84,9 +84,9 @@ appropriately before running "make".
  
  -x::
  	Turn on shell tracing (i.e., `set -x`) during the tests
 -	themselves. Implies `--verbose`. Note that this can cause
 -	failures in some tests which redirect and test the
 -	output of shell functions. Use with caution.
 +	themselves. Implies `--verbose`. Note that in non-bash shells,
 +	this can cause failures in some tests which redirect and test
 +	the output of shell functions. Use with caution.
  
  -d::
  --debug::
 diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
 index 25f1118..66348f1 100755
 --- a/t/t3404-rebase-interactive.sh
 +++ b/t/t3404-rebase-interactive.sh
 @@ -882,8 +882,7 @@ test_expect_success 'rebase -i --exec without <CMD>' '
  	git reset --hard execute &&
  	set_fake_editor &&
  	test_must_fail git rebase -i --exec 2>tmp &&
 -	sed -e "/option .exec. requires a value/d" -e '/^+/d' \
 -		tmp >actual &&
 +	sed -e "1d" tmp >actual &&
  	test_must_fail git rebase -h >expected &&
  	test_cmp expected actual &&
  	git checkout master
 @@ -1150,6 +1149,10 @@ test_expect_success 'drop' '
  	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
  '
  
 +cat >expect <<EOF
 +Successfully rebased and updated refs/heads/missing-commit.
 +EOF
 +
  test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
  	test_config rebase.missingCommitsCheck ignore &&
  	rebase_setup_and_clean missing-commit &&
 @@ -1157,33 +1160,52 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
  	FAKE_LINES="1 2 3 4" \
  		git rebase -i --root 2>actual &&
  	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
 -	test_i18ngrep \
 -		"Successfully rebased and updated refs/heads/missing-commit." \
 -		actual
 +	test_cmp expect actual
  '
  
 +cat >expect <<EOF
 +Warning: some commits may have been dropped accidentally.
 +Dropped commits (newer to older):
 + - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
 +To avoid this message, use "drop" to explicitly remove a commit.
 +
 +Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
 +The possible behaviours are: ignore, warn, error.
 +
 +Successfully rebased and updated refs/heads/missing-commit.
 +EOF
 +
  test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
 -	line="$(git rev-list --pretty=oneline --abbrev-commit -1 master)" &&
  	test_config rebase.missingCommitsCheck warn &&
  	rebase_setup_and_clean missing-commit &&
  	set_fake_editor &&
  	FAKE_LINES="1 2 3 4" \
  		git rebase -i --root 2>actual &&
 -	test_i18ngrep "Warning: some commits may have been dropped" actual &&
 -	test_i18ngrep "^ - $line" actual &&
 +	test_cmp expect actual &&
  	test D = $(git cat-file commit HEAD | sed -ne \$p)
  '
  
 +cat >expect <<EOF
 +Warning: some commits may have been dropped accidentally.
 +Dropped commits (newer to older):
 + - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
 + - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
 +To avoid this message, use "drop" to explicitly remove a commit.
 +
 +Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
 +The possible behaviours are: ignore, warn, error.
 +
 +You can fix this with 'git rebase --edit-todo'.
 +Or you can abort the rebase with 'git rebase --abort'.
 +EOF
 +
  test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
 -	line1="$(git rev-list --pretty=oneline --abbrev-commit -1 master)" &&
 -	line2="$(git rev-list --pretty=oneline --abbrev-commit -1 master~2)" &&
  	test_config rebase.missingCommitsCheck error &&
  	rebase_setup_and_clean missing-commit &&
  	set_fake_editor &&
  	test_must_fail env FAKE_LINES="1 2 4" \
  		git rebase -i --root 2>actual &&
 -	test_i18ngrep "^ - $line1" actual &&
 -	test_i18ngrep "^ - $line2" actual &&
 +	test_cmp expect actual &&
  	cp .git/rebase-merge/git-rebase-todo.backup \
  		.git/rebase-merge/git-rebase-todo &&
  	FAKE_LINES="1 2 drop 3 4 drop 5" \
 @@ -1193,13 +1215,20 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
  	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
  '
  
 +cat >expect <<EOF
 +Warning: the command isn't recognized in the following line:
 + - badcmd $(git rev-list --oneline -1 master~1)
 +
 +You can fix this with 'git rebase --edit-todo'.
 +Or you can abort the rebase with 'git rebase --abort'.
 +EOF
 +
  test_expect_success 'static check of bad command' '
 -	line=" - badcmd $(git rev-list --oneline -1 master~1)" &&
  	rebase_setup_and_clean bad-cmd &&
  	set_fake_editor &&
  	test_must_fail env FAKE_LINES="1 2 3 bad 4 5" \
  		git rebase -i --root 2>actual &&
 -	test_i18ngrep "^$line" actual &&
 +	test_cmp expect actual &&
  	FAKE_LINES="1 2 3 drop 4 5" git rebase --edit-todo &&
  	git rebase --continue &&
  	test E = $(git cat-file commit HEAD | sed -ne \$p) &&
 @@ -1221,12 +1250,20 @@ test_expect_success 'tabs and spaces are accepted in the todolist' '
  	test E = $(git cat-file commit HEAD | sed -ne \$p)
  '
  
 +cat >expect <<EOF
 +Warning: the SHA-1 is missing or isn't a commit in the following line:
 + - edit XXXXXXX False commit
 +
 +You can fix this with 'git rebase --edit-todo'.
 +Or you can abort the rebase with 'git rebase --abort'.
 +EOF
 +
  test_expect_success 'static check of bad SHA-1' '
  	rebase_setup_and_clean bad-sha &&
  	set_fake_editor &&
  	test_must_fail env FAKE_LINES="1 2 edit fakesha 3 4 5 #" \
  		git rebase -i --root 2>actual &&
 -	test_i18ngrep "^ - edit XXXXXXX False commit" actual &&
 +	test_cmp expect actual &&
  	FAKE_LINES="1 2 4 5 6" git rebase --edit-todo &&
  	git rebase --continue &&
  	test E = $(git cat-file commit HEAD | sed -ne \$p)
 diff --git a/t/test-lib.sh b/t/test-lib.sh
 index 286c5f3..0055ebb 100644
 --- a/t/test-lib.sh
 +++ b/t/test-lib.sh
 @@ -322,6 +322,19 @@ else
  	exec 4>/dev/null 3>/dev/null
  fi
  
 +# Send any "-x" output directly to stderr to avoid polluting tests
 +# which capture stderr. We can do this unconditionally since it
 +# has no effect if tracing isn't turned on.
 +#
 +# Note that this sets up the trace fd as soon as we assign the variable, so it
 +# must come after the creation of descriptor 4 above. Likewise, we must never
 +# unset this, as it has the side effect of closing descriptor 4, which we
 +# use to show verbose tests to the user.
 +#
 +# Note also that we don't need or want to export it. The tracing is local to
 +# this shell, and we would not want to influence any shells we exec.
 +BASH_XTRACEFD=4
 +
  test_failure=0
  test_count=0
  test_fixed=0

-- 
2.8.2.465.gb077790

[PATCH v2 1/2] t3404: fix typo

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3404-rebase-interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d96d0e4..66348f1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -62,7 +62,7 @@ test_expect_success 'setup' '
 
 # "exec" commands are ran with the user shell by default, but this may
 # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
-# to create a file. Unseting SHELL avoids such non-portable behavior
+# to create a file. Unsetting SHELL avoids such non-portable behavior
 # in tests. It must be exported for it to take effect where needed.
 SHELL=
 export SHELL
-- 
2.8.2.465.gb077790

[PATCH v2 2/2] test-lib: set BASH_XTRACEFD automatically

[Johannes Schindelin]

From: Jeff King <peff@peff.net>

Passing "-x" to a test script enables the shell's "set -x"
tracing, which can help with tracking down the command that
is causing a failure. Unfortunately, it can also _cause_
failures in some tests that redirect the stderr of a shell
function.  Inside the function the shell continues to
respect "set -x", and the trace output is collected along
with whatever stderr is generated normally by the function.

You can see an example of this by running:

  ./t0040-parse-options.sh -x -i

which will fail immediately in the first test, as it
expects:

  test_must_fail some-cmd 2>output.err

to leave output.err empty (but with "-x" it has our trace
output).

Unfortunately there isn't a portable or scalable solution to
this. We could teach test_must_fail to disable "set -x", but
that doesn't help any of the other functions or subshells.

However, we can work around it by pointing the "set -x"
output to our descriptor 4, which always points to the
original stderr of the test script. Unfortunately this only
works for bash, but it's better than nothing (and other
shells will just ignore the BASH_XTRACEFD variable).

The patch itself is a simple one-liner, but note the caveats
in the accompanying comments.

Automatic tests for our "-x" option may be a bit too meta
(and a pain, because they are bash-specific), but I did
confirm that it works correctly both with regular "-x" and
with "--verbose-only=1". This works because the latter flips
"set -x" off and on for particular tests (if it didn't, we
would get tracing for all tests, as going to descriptor 4
effectively circumvents the verbose flag).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/README      |  6 +++---
 t/test-lib.sh | 13 +++++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/t/README b/t/README
index 1dc908e..76a0daa 100644
--- a/t/README
+++ b/t/README
@@ -84,9 +84,9 @@ appropriately before running "make".
 
 -x::
 	Turn on shell tracing (i.e., `set -x`) during the tests
-	themselves. Implies `--verbose`. Note that this can cause
-	failures in some tests which redirect and test the
-	output of shell functions. Use with caution.
+	themselves. Implies `--verbose`. Note that in non-bash shells,
+	this can cause failures in some tests which redirect and test
+	the output of shell functions. Use with caution.
 
 -d::
 --debug::
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 286c5f3..0055ebb 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -322,6 +322,19 @@ else
 	exec 4>/dev/null 3>/dev/null
 fi
 
+# Send any "-x" output directly to stderr to avoid polluting tests
+# which capture stderr. We can do this unconditionally since it
+# has no effect if tracing isn't turned on.
+#
+# Note that this sets up the trace fd as soon as we assign the variable, so it
+# must come after the creation of descriptor 4 above. Likewise, we must never
+# unset this, as it has the side effect of closing descriptor 4, which we
+# use to show verbose tests to the user.
+#
+# Note also that we don't need or want to export it. The tracing is local to
+# this shell, and we would not want to influence any shells we exec.
+BASH_XTRACEFD=4
+
 test_failure=0
 test_count=0
 test_fixed=0
-- 
2.8.2.465.gb077790

Re: [PATCH v2 0/2] Work on t3404 in preparation for rebase--helper

[Junio C Hamano]

I took these separately already, and plan to fast-track them as they
are both "trivially correct"; I double checked that what I have
match these two, too.

Thanks.

Re: [PATCH v2 0/2] Work on t3404 in preparation for rebase--helper

[Johannes Schindelin]

Hi Junio,

On Thu, 12 May 2016, Junio C Hamano wrote:

> I took these separately already, and plan to fast-track them as they are
> both "trivially correct"; I double checked that what I have match these
> two, too.

Oh, okay. I just wanted to make things easier for you, and now that I have
a script to prepare patch series, it's really almost as trivial for me to
send out a new iteration as it would be to update a Pull Request on
GitHub.

Do you want me to hold off with new iterations in the future until you
clarified your preferred course of action?

Ciao,
Dscho

Re: [PATCH v2 0/2] Work on t3404 in preparation for rebase--helper

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Thu, 12 May 2016, Junio C Hamano wrote:
>
>> I took these separately already, and plan to fast-track them as they are
>> both "trivially correct"; I double checked that what I have match these
>> two, too.
>
> Oh, okay. I just wanted to make things easier for you, and now that I have
> a script to prepare patch series, it's really almost as trivial for me to
> send out a new iteration as it would be to update a Pull Request on
> GitHub.
>
> Do you want me to hold off with new iterations in the future until you
> clarified your preferred course of action?

No.  You've been doing great.  I just wanted to clarify what I did
to your patch before I merge them separately to 'next' ahead of the
remainder that you'd be sending out, expecting a possible course
correction, e.g. "that would make it harder to queue the other patches
yet to come, all of which would depend on both of them--it would be
better to queue them on a single topic to be extended with these
other patches, after all these two are not that urgent".

Re: [PATCH v2 0/2] Work on t3404 in preparation for rebase--helper

[Johannes Schindelin]

Hi Junio,

On Fri, 13 May 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Thu, 12 May 2016, Junio C Hamano wrote:
> >
> >> I took these separately already, and plan to fast-track them as they
> >> are both "trivially correct"; I double checked that what I have match
> >> these two, too.
> >
> > Oh, okay. I just wanted to make things easier for you, and now that I
> > have a script to prepare patch series, it's really almost as trivial
> > for me to send out a new iteration as it would be to update a Pull
> > Request on GitHub.
> >
> > Do you want me to hold off with new iterations in the future until you
> > clarified your preferred course of action?
> 
> No.  You've been doing great.  I just wanted to clarify what I did
> to your patch before I merge them separately to 'next' ahead of the
> remainder that you'd be sending out, expecting a possible course
> correction, e.g. "that would make it harder to queue the other patches
> yet to come, all of which would depend on both of them--it would be
> better to queue them on a single topic to be extended with these
> other patches, after all these two are not that urgent".

Thanks.

I planned to work on the remainder as a "topic thicket" using my Git
garden shears [*1*] anyway, picking up the changes you picked up,
replacing my original patches.

Ciao,
Dscho

Footnote *1*: The garden shears are kind of a `git rebase -i -p` as I wish
I had designed it originally. Thet live here:
https://github.com/git-for-windows/build-extra/blob/master/shears.sh