[PATCH] t3701: two subtests are fixed

[PATCH] t3701: two subtests are fixed

[Michael J Gruber]

0527ccb1b5 ("add -i: default to the built-in implementation", 2021-11-30)
switched to the implementation which fixed to subtest. Mark them as
expect_success now.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
I did check the ML but may have missed a series which contains this. (I
only found one which tries to make the test output clearer in CI.)

 t/t3701-add-interactive.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 94537a6b40..9a06638704 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -538,7 +538,7 @@ test_expect_success 'split hunk "add -p (edit)"' '
 	! grep "^+15" actual
 '
 
-test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
+test_expect_success 'split hunk "add -p (no, yes, edit)"' '
 	test_write_lines 5 10 20 21 30 31 40 50 60 >test &&
 	git reset &&
 	# test sequence is s(plit), n(o), y(es), e(dit)
@@ -562,7 +562,7 @@ test_expect_success 'split hunk with incomplete line at end' '
 	test_must_fail git grep --cached before
 '
 
-test_expect_failure 'edit, adding lines to the first hunk' '
+test_expect_success 'edit, adding lines to the first hunk' '
 	test_write_lines 10 11 20 30 40 50 51 60 >test &&
 	git reset &&
 	tr _ " " >patch <<-EOF &&
-- 
2.37.0.rc0.107.g7a7be657e7

[PATCH v2] add -i tests: mark "TODO" depending on GIT_TEST_ADD_I_USE_BUILTIN

[Ævar Arnfjörð Bjarmason]

Fix an issue that existed before 0527ccb1b55 (add -i: default to the
built-in implementation, 2021-11-30), but which became the default
with that change, we should not be marking tests that are known to
pass as "TODO" tests.

When GIT_TEST_ADD_I_USE_BUILTIN=1 was made the default we started
passing the tests added in 0f0fba2cc87 (t3701: add a test for advanced
split-hunk editing, 2019-12-06) and 1bf01040f0c (add -p: demonstrate
failure when running 'edit' after a split, 2015-04-16).

Thus we've been emitting this sort of output:

	$ prove ./t3701-add-interactive.sh
	./t3701-add-interactive.sh .. ok
	All tests successful.

	Test Summary Report
	-------------------
	./t3701-add-interactive.sh (Wstat: 0 Tests: 70 Failed: 0)
	  TODO passed:   45, 47
	Files=1, Tests=70,  2 wallclock secs ( 0.03 usr  0.00 sys +  0.86 cusr  0.33 csys =  1.22 CPU)
	Result: PASS

Which isn't just cosmetic, but due to issues with
test_expect_failure (see [1]) we could e.g. be hiding something as bad
as a segfault in the new implementation. It makes sense catch that,
especially before we put out a release with the built-in "add -i", so
let's generalize the check we were already doing in 0527ccb1b55 with a
new "ADD_I_USE_BUILTIN" prerequisite.

1. https://lore.kernel.org/git/patch-1.7-4624abc2591-20220318T002951Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Tue, Jun 14 2022, Michael J Gruber wrote:

> 0527ccb1b5 ("add -i: default to the built-in implementation", 2021-11-30)
> switched to the implementation which fixed to subtest. Mark them as
> expect_success now.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
> I did check the ML but may have missed a series which contains this. (I
> only found one which tries to make the test output clearer in CI.)

I was looking at the same earlier and came up with this (before seeing
your patch here), so a proposed v2 I suppose.

Just converting it to "test_expect_success" will break CI and other
setups that are testing with GIT_TEST_ADD_I_USE_BUILTIN=false.

The below fixes it, however.

 t/t2016-checkout-patch.sh  |  2 +-
 t/t3701-add-interactive.sh | 12 ++++++++++--
 t/test-lib.sh              |  4 ++++
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index bc3f69b4b1d..a5822e41af2 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -4,7 +4,7 @@ test_description='git checkout --patch'
 
 . ./lib-patch-mode.sh
 
-if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL
+if ! test_have_prereq ADD_I_USE_BUILTIN && ! test_have_prereq PERL
 then
 	skip_all='skipping interactive add tests, PERL not set'
 	test_done
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 94537a6b40a..fc26cb8bae8 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -538,7 +538,15 @@ test_expect_success 'split hunk "add -p (edit)"' '
 	! grep "^+15" actual
 '
 
-test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
+test_expect_success 'setup ADD_I_USE_BUILTIN check' '
+	result=success &&
+	if ! test_have_prereq ADD_I_USE_BUILTIN
+	then
+		result=failure
+	fi
+'
+
+test_expect_$result 'split hunk "add -p (no, yes, edit)"' '
 	test_write_lines 5 10 20 21 30 31 40 50 60 >test &&
 	git reset &&
 	# test sequence is s(plit), n(o), y(es), e(dit)
@@ -562,7 +570,7 @@ test_expect_success 'split hunk with incomplete line at end' '
 	test_must_fail git grep --cached before
 '
 
-test_expect_failure 'edit, adding lines to the first hunk' '
+test_expect_$result 'edit, adding lines to the first hunk' '
 	test_write_lines 10 11 20 30 40 50 51 60 >test &&
 	git reset &&
 	tr _ " " >patch <<-EOF &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 736c6447ecf..f5291ef56ef 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1759,6 +1759,10 @@ test_lazy_prereq SHA1 '
 	esac
 '
 
+test_lazy_prereq ADD_I_USE_BUILTIN '
+	test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true
+'
+
 # Ensure that no test accidentally triggers a Git command
 # that runs the actual maintenance scheduler, affecting a user's
 # system permanently.
-- 
2.36.1.1239.gfba91521d90

Re: [PATCH v2] add -i tests: mark "TODO" depending on GIT_TEST_ADD_I_USE_BUILTIN

[Todd Zullinger]

Ævar Arnfjörð Bjarmason wrote:
> Fix an issue that existed before 0527ccb1b55 (add -i: default to the
> built-in implementation, 2021-11-30), but which became the default
> with that change, we should not be marking tests that are known to
> pass as "TODO" tests.
[...]
> ---
> Just converting it to "test_expect_success" will break CI and other
> setups that are testing with GIT_TEST_ADD_I_USE_BUILTIN=false.
> 
> The below fixes it, however.

Nice catch.  FWIW, I tested w/GIT_TEST_ADD_I_USE_BUILTIN=0
and without.

> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> index bc3f69b4b1d..a5822e41af2 100755
> --- a/t/t2016-checkout-patch.sh
> +++ b/t/t2016-checkout-patch.sh
> @@ -4,7 +4,7 @@ test_description='git checkout --patch'
>  
>  . ./lib-patch-mode.sh
>  
> -if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL
> +if ! test_have_prereq ADD_I_USE_BUILTIN && ! test_have_prereq PERL
>  then
>  	skip_all='skipping interactive add tests, PERL not set'

It's not the fault of this patch, but it makes it obvious
that the `skip_all` message is no longer accurate.  Perhaps
somethine like this?

    skip_all='skipping interactive add tests, missing ADD_I_USE_BUILTIN or PERL'

Maybe a separate `ADD_I` prereq would be better?  Though
without looking closer, I don't know if that would end up
being clearer to anyone running the tests without either
PERL or the add -i builtin enabled.

Thanks for the keen eye and attention to detail, Ævar,

-- 
Todd

Re: [PATCH v2] add -i tests: mark "TODO" depending on GIT_TEST_ADD_I_USE_BUILTIN

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 14 2022, Todd Zullinger wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> Fix an issue that existed before 0527ccb1b55 (add -i: default to the
>> built-in implementation, 2021-11-30), but which became the default
>> with that change, we should not be marking tests that are known to
>> pass as "TODO" tests.
> [...]
>> ---
>> Just converting it to "test_expect_success" will break CI and other
>> setups that are testing with GIT_TEST_ADD_I_USE_BUILTIN=false.
>> 
>> The below fixes it, however.
>
> Nice catch.  FWIW, I tested w/GIT_TEST_ADD_I_USE_BUILTIN=0
> and without.

My patch landed on "master" as 7ccbea564e8 (add -i tests: mark "TODO"
depending on GIT_TEST_ADD_I_USE_BUILTIN, 2022-06-14) so this is water
under the bridge.

But just to tie this loose knot I think something went wrong in your
testing.

If I:

    git checkout v2.37.0-rc0
    # Apply your patch from <20220614185218.1091413-1-tmz@pobox.com>

I'll consistently get a failure from:

    GIT_TEST_ADD_I_USE_BUILTIN=false ./t3701-add-interactive.sh

Since we do fail that test with the Perl implementation, and now it's no
longer a TODO test.

Perhaps you used it as a parameter to "make"? I.e.:

    make GIT_TEST_ADD_I_USE_BUILTIN=false
    make test

Which isn't how it works, just speculating...
 
>> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
>> index bc3f69b4b1d..a5822e41af2 100755
>> --- a/t/t2016-checkout-patch.sh
>> +++ b/t/t2016-checkout-patch.sh
>> @@ -4,7 +4,7 @@ test_description='git checkout --patch'
>>  
>>  . ./lib-patch-mode.sh
>>  
>> -if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL
>> +if ! test_have_prereq ADD_I_USE_BUILTIN && ! test_have_prereq PERL
>>  then
>>  	skip_all='skipping interactive add tests, PERL not set'
>
> It's not the fault of this patch, but it makes it obvious
> that the `skip_all` message is no longer accurate.  Perhaps
> somethine like this?
>
>     skip_all='skipping interactive add tests, missing ADD_I_USE_BUILTIN or PERL'
>
> Maybe a separate `ADD_I` prereq would be better?  Though
> without looking closer, I don't know if that would end up
> being clearer to anyone running the tests without either
> PERL or the add -i builtin enabled.

Yeah seems like a good idea for a follow-up, but since it's landed I'll
probably forget :)

> Thanks for the keen eye and attention to detail, Ævar,

Happy to have it fixed!

Re: [PATCH v2] add -i tests: mark "TODO" depending on GIT_TEST_ADD_I_USE_BUILTIN

[Todd Zullinger]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Jun 14 2022, Todd Zullinger wrote:
>> Nice catch.  FWIW, I tested w/GIT_TEST_ADD_I_USE_BUILTIN=0
>> and without.
> 
> My patch landed on "master" as 7ccbea564e8 (add -i tests: mark "TODO"
> depending on GIT_TEST_ADD_I_USE_BUILTIN, 2022-06-14) so this is water
> under the bridge.
> 
> But just to tie this loose knot I think something went wrong in your
> testing.
> 
> If I:
> 
>     git checkout v2.37.0-rc0
>     # Apply your patch from <20220614185218.1091413-1-tmz@pobox.com>
> 
> I'll consistently get a failure from:
> 
>     GIT_TEST_ADD_I_USE_BUILTIN=false ./t3701-add-interactive.sh
> 

Sorry for being unclear.  I meant that I tested your patch.
The patch I sent didn't handle that case. :)

-- 
Todd

Re: [PATCH] t3701: two subtests are fixed

[Derrick Stolee]

On 6/14/2022 11:26 AM, Michael J Gruber wrote:
> 0527ccb1b5 ("add -i: default to the built-in implementation", 2021-11-30)
> switched to the implementation which fixed to subtest. Mark them as
> expect_success now.

s/to subtest/two subtests/

> 
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
> I did check the ML but may have missed a series which contains this. (I
> only found one which tries to make the test output clearer in CI.)

The breakage vanished as of 1fc1879839 (Merge branch 'js/use-builtin-add-i',
2022-05-30). The direct change is likely 0527ccb1b5 (add -i: default to the
built-in implementation, 2021-11-30), but that commit actually fails the
tests, it seems. Something about a parallel topic must have made it work at
the merge point.

Patch looks good. Thanks!

-Stolee

Re: [PATCH] t3701: two subtests are fixed

[Todd Zullinger]

Michael J Gruber wrote:
> 0527ccb1b5 ("add -i: default to the built-in implementation", 2021-11-30)
> switched to the implementation which fixed to subtest. Mark them as
> expect_success now.
> 
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
> I did check the ML but may have missed a series which contains this. (I
> only found one which tries to make the test output clearer in CI.)

I sent a patch (<20220614185218.1091413-1-tmz@pobox.com>) as
well.  I mentioned the commits which added these tests, but
didn't call out 0527ccb1b5 (add -i: default to the built-in
implementation, 2021-11-30) explicitly, which is a good
addition.

I'm just happy to see the builtin `add -i` as the default.

>  t/t3701-add-interactive.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 94537a6b40..9a06638704 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -538,7 +538,7 @@ test_expect_success 'split hunk "add -p (edit)"' '
>  	! grep "^+15" actual
>  '
>  
> -test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
> +test_expect_success 'split hunk "add -p (no, yes, edit)"' '
>  	test_write_lines 5 10 20 21 30 31 40 50 60 >test &&
>  	git reset &&
>  	# test sequence is s(plit), n(o), y(es), e(dit)
> @@ -562,7 +562,7 @@ test_expect_success 'split hunk with incomplete line at end' '
>  	test_must_fail git grep --cached before
>  '
>  
> -test_expect_failure 'edit, adding lines to the first hunk' '
> +test_expect_success 'edit, adding lines to the first hunk' '
>  	test_write_lines 10 11 20 30 40 50 51 60 >test &&
>  	git reset &&
>  	tr _ " " >patch <<-EOF &&

-- 
Todd

Re: [PATCH] t3701: two subtests are fixed

[Taylor Blau]

On Tue, Jun 14, 2022 at 05:26:33PM +0200, Michael J Gruber wrote:
> 0527ccb1b5 ("add -i: default to the built-in implementation", 2021-11-30)
> switched to the implementation which fixed to subtest. Mark them as
> expect_success now.

Since v2.37.0-rc0 is the first tag to contain 0527ccb1b5, I bisected
between v2.36 (when these two tests indeed failed) and the tip of
master (8168d5e9c2 (Git 2.37-rc0, 2022-06-13) at the time of writing).

And I also got 0527ccb1b5, so the bisection looks good to me, and this
was likely an oversight when 0527ccb1b5 was written. Thanks for putting
the author on the CC list just in case there is any additional context.

Otherwise, this patch looks good to me.

Thanks,
Taylor

Re: [PATCH] t3701: two subtests are fixed

[Johannes Schindelin]

Hi Michael,

On Tue, 14 Jun 2022, Michael J Gruber wrote:

> 0527ccb1b5 ("add -i: default to the built-in implementation", 2021-11-30)
> switched to the implementation which fixed to subtest. Mark them as
> expect_success now.

Good catch!

However... that commit specifically contains this change:

	diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
	index cc62616d806..660ebe8d108 100755
	--- a/ci/run-build-and-tests.sh
	+++ b/ci/run-build-and-tests.sh
	@@ -29,7 +29,7 @@ linux-gcc)
		export GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1
		export GIT_TEST_MULTI_PACK_INDEX=1
		export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1
	-       export GIT_TEST_ADD_I_USE_BUILTIN=1
	+       export GIT_TEST_ADD_I_USE_BUILTIN=0
		export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
		export GIT_TEST_WRITE_REV_INDEX=1
		export GIT_TEST_CHECKOUT_WORKERS=2

The intention is to have t3701 be run with the non-built-in version of
`git add -i` in the `linux-gcc` job, and I am surprised that those two
tests do not fail for you in that case.

Did you run this through the CI builds?

Thank you,
Dscho

Re: [PATCH] t3701: two subtests are fixed

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 2022-06-15 16:50:40:
> Hi Michael,

Hallo Dscho!

> On Tue, 14 Jun 2022, Michael J Gruber wrote:
> 
> > 0527ccb1b5 ("add -i: default to the built-in implementation", 2021-11-30)
> > switched to the implementation which fixed to subtest. Mark them as
> > expect_success now.
> 
> Good catch!
 
I'm no list regular anymore, but still a "next+ regular". While
experimenting with my own patch I noticed something got fixed
unexpectedly. That goes to show that these unexpected successes
(from expect_failure) go unnoticed too easily. I had missed this on my
regular rebuilds.

> However... that commit specifically contains this change:
> 
>         diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
>         index cc62616d806..660ebe8d108 100755
>         --- a/ci/run-build-and-tests.sh
>         +++ b/ci/run-build-and-tests.sh
>         @@ -29,7 +29,7 @@ linux-gcc)
>                 export GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1
>                 export GIT_TEST_MULTI_PACK_INDEX=1
>                 export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1
>         -       export GIT_TEST_ADD_I_USE_BUILTIN=1
>         +       export GIT_TEST_ADD_I_USE_BUILTIN=0
>                 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
>                 export GIT_TEST_WRITE_REV_INDEX=1
>                 export GIT_TEST_CHECKOUT_WORKERS=2
> 
> The intention is to have t3701 be run with the non-built-in version of
> `git add -i` in the `linux-gcc` job, and I am surprised that those two
> tests do not fail for you in that case.
> 
> Did you run this through the CI builds?

That's why I mentioned "no list regular" - I didn't know about that knob
nor the intention to have the test suite run with either implementation
(rather than switching to the new one for good).

I do local builds, usually with

```
DEVELOPER=1 (which I had to disable during the bisect run; gcc12...)
DEFAULT_TEST_TARGET=prove
GIT_PROVE_OPTS=--jobs 4
GIT_TEST_OPTS=--root=/dev/shm/t --chain-lint
SHELL_PATH=/bin/dash
SKIP_DASHED_BUILT_INS=y
```

in config.mak. Nothing else strikes me as potentially relevant.

Ævar noticed this and has a better version of my patch, I think.

Michael

Re: [PATCH] t3701: two subtests are fixed

[Junio C Hamano]

Michael J Gruber <git@grubix.eu> writes:

> Johannes Schindelin venit, vidit, dixit 2022-06-15 16:50:40:
>> Hi Michael,
>
> Hallo Dscho!
>
>> On Tue, 14 Jun 2022, Michael J Gruber wrote:
>> 
>> > 0527ccb1b5 ("add -i: default to the built-in implementation", 2021-11-30)
>> > switched to the implementation which fixed to subtest. Mark them as
>> > expect_success now.
>> 
>> Good catch!
>  
> I'm no list regular anymore, but still a "next+ regular". While
> experimenting with my own patch I noticed something got fixed
> unexpectedly. That goes to show that these unexpected successes
> (from expect_failure) go unnoticed too easily. I had missed this on my
> regular rebuilds.

Thanks for being a "next+ regular".  They are giving us a valuable
service to catch bugs and questionable design decisions before they
hit the "master" branch.

> Ævar noticed this and has a better version of my patch, I think.

Yup.  Eventually we will make it even impossible to opt out of the
built-in variant, but until then, we'd need the conditional stuff.

Thanks.

Re: [PATCH] t3701: two subtests are fixed

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4012 bytes --]

Hi Michael,

On Thu, 16 Jun 2022, Michael J Gruber wrote:

> Johannes Schindelin venit, vidit, dixit 2022-06-15 16:50:40:
>
> > On Tue, 14 Jun 2022, Michael J Gruber wrote:
> >
> > > 0527ccb1b5 ("add -i: default to the built-in implementation", 2021-11-30)
> > > switched to the implementation which fixed to subtest. Mark them as
> > > expect_success now.
> >
> > Good catch!
>
> I'm no list regular anymore, but still a "next+ regular". While
> experimenting with my own patch I noticed something got fixed
> unexpectedly. That goes to show that these unexpected successes
> (from expect_failure) go unnoticed too easily. I had missed this on my
> regular rebuilds.

Makes sense.

> > However... that commit specifically contains this change:
> >
> >         diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> >         index cc62616d806..660ebe8d108 100755
> >         --- a/ci/run-build-and-tests.sh
> >         +++ b/ci/run-build-and-tests.sh
> >         @@ -29,7 +29,7 @@ linux-gcc)
> >                 export GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1
> >                 export GIT_TEST_MULTI_PACK_INDEX=1
> >                 export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1
> >         -       export GIT_TEST_ADD_I_USE_BUILTIN=1
> >         +       export GIT_TEST_ADD_I_USE_BUILTIN=0
> >                 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
> >                 export GIT_TEST_WRITE_REV_INDEX=1
> >                 export GIT_TEST_CHECKOUT_WORKERS=2
> >
> > The intention is to have t3701 be run with the non-built-in version of
> > `git add -i` in the `linux-gcc` job, and I am surprised that those two
> > tests do not fail for you in that case.
> >
> > Did you run this through the CI builds?
>
> That's why I mentioned "no list regular" - I didn't know about that knob
> nor the intention to have the test suite run with either implementation
> (rather than switching to the new one for good).
>
> I do local builds, usually with
>
> ```
> DEVELOPER=1 (which I had to disable during the bisect run; gcc12...)
> DEFAULT_TEST_TARGET=prove
> GIT_PROVE_OPTS=--jobs 4
> GIT_TEST_OPTS=--root=/dev/shm/t --chain-lint
> SHELL_PATH=/bin/dash
> SKIP_DASHED_BUILT_INS=y
> ```
>
> in config.mak. Nothing else strikes me as potentially relevant.
>
> Ævar noticed this and has a better version of my patch, I think.

So you did not find it utterly rude and presumptuous that somebody sent a
new iteration of your patch without even so much as consulting with you
whether you're okay with this? I salute your forbearance, then.

Besides, it is not really a better version of your patch. That would have
been:

-- snip --
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 94537a6b40a..6d1032fe8ae 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -538,7 +538,9 @@ test_expect_success 'split hunk "add -p (edit)"' '
 	! grep "^+15" actual
 '

-test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
+test_lazy_prereq BUILTIN_ADD_I 'test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true'
+
+test_expect_success BUILTIN_ADD_I 'split hunk "add -p (no, yes, edit)"' '
 	test_write_lines 5 10 20 21 30 31 40 50 60 >test &&
 	git reset &&
 	# test sequence is s(plit), n(o), y(es), e(dit)
@@ -562,7 +564,7 @@ test_expect_success 'split hunk with incomplete line at end' '
 	test_must_fail git grep --cached before
 '

-test_expect_failure 'edit, adding lines to the first hunk' '
+test_expect_failure BUILTIN_ADD_I 'edit, adding lines to the first hunk' '
 	test_write_lines 10 11 20 30 40 50 51 60 >test &&
 	git reset &&
 	tr _ " " >patch <<-EOF &&
-- snap --

As you can see, this is _actually_ building on your work rather than
replacing it.

But since that replacement made it into -rc1, I will stop spending brain
cycles on it.

Thank you for your contribution, I am glad that you keep sending patches
to the Git mailing list!
Dscho

Re: [PATCH] t3701: two subtests are fixed

[Junio C Hamano]

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

>> in config.mak. Nothing else strikes me as potentially relevant.
>>
>> Ævar noticed this and has a better version of my patch, I think.
>
> So you did not find it utterly rude and presumptuous that somebody sent a
> new iteration of your patch without even so much as consulting with you
> whether you're okay with this? I salute your forbearance, then.

I had an impression that these (wasn't there another one) were
independent discoveries and patching that happened at the same time.

> Besides, it is not really a better version of your patch. That would have
> been:
>
> -- snip --
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 94537a6b40a..6d1032fe8ae 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -538,7 +538,9 @@ test_expect_success 'split hunk "add -p (edit)"' '
>  	! grep "^+15" actual
>  '
>
> -test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
> +test_lazy_prereq BUILTIN_ADD_I 'test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true'
> +
> +test_expect_success BUILTIN_ADD_I 'split hunk "add -p (no, yes, edit)"' '
>  	test_write_lines 5 10 20 21 30 31 40 50 60 >test &&
>  	git reset &&
>  	# test sequence is s(plit), n(o), y(es), e(dit)

Prerequisite lets you skip.  

This stops saying that "with scripted version 'add -p' does not
behave in the way we want to see, and we want to leave us a mental
note about it".  I do not know if that is what we want.

Once scripted version gets fully retired, it of course stops
mattering ;-)

> @@ -562,7 +564,7 @@ test_expect_success 'split hunk with incomplete line at end' '
>  	test_must_fail git grep --cached before
>  '
>
> -test_expect_failure 'edit, adding lines to the first hunk' '
> +test_expect_failure BUILTIN_ADD_I 'edit, adding lines to the first hunk' '

I am not sure if this is a good change, quite honestly.  With
s/failure/success/, perhaps, but not in the posted form.

Re: [PATCH] t3701: two subtests are fixed

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 2022-06-21 17:45:04:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> in config.mak. Nothing else strikes me as potentially relevant.
> >>
> >> Ævar noticed this and has a better version of my patch, I think.
> >
> > So you did not find it utterly rude and presumptuous that somebody sent a
> > new iteration of your patch without even so much as consulting with you
> > whether you're okay with this? I salute your forbearance, then.
> 
> I had an impression that these (wasn't there another one) were
> independent discoveries and patching that happened at the same time.

Yes, while it looked funny at first, Ævar explained it well. So,
everything is fine for me. Besides, we're no strangers to each other ;)

As for the question which version covers the expectations best: I'm
lacking the necessary overview of the expectations (which implementation
to check by default, in CI etc.) which is why I won't chime in on that.

Re: [PATCH] t3701: two subtests are fixed

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2903 bytes --]

Hi Junio,

I did not want to spend more brain cycles about this, but since you left a
few questions hanging...

On Tue, 21 Jun 2022, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> in config.mak. Nothing else strikes me as potentially relevant.
> >>
> >> Ævar noticed this and has a better version of my patch, I think.
> >
> > So you did not find it utterly rude and presumptuous that somebody sent a
> > new iteration of your patch without even so much as consulting with you
> > whether you're okay with this? I salute your forbearance, then.
>
> I had an impression that these (wasn't there another one) were
> independent discoveries and patching that happened at the same time.

If this was the first time an unsolicited iteration was sent on another
contributor's behalf, I would be able to give the benefit of the doubt.
Even if it was the second or third time. It's been many more times,
though. And it is not leaving the impression of an inviting, welcoming
culture I would like to see on the Git mailing list. But it's your
project to lead, not mine, therefore I have no say in this.

> > -- snip --
> > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> > index 94537a6b40a..6d1032fe8ae 100755
> > --- a/t/t3701-add-interactive.sh
> > +++ b/t/t3701-add-interactive.sh
> > @@ -538,7 +538,9 @@ test_expect_success 'split hunk "add -p (edit)"' '
> >  	! grep "^+15" actual
> >  '
> >
> > -test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
> > +test_lazy_prereq BUILTIN_ADD_I 'test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true'
> > +
> > +test_expect_success BUILTIN_ADD_I 'split hunk "add -p (no, yes, edit)"' '
> >  	test_write_lines 5 10 20 21 30 31 40 50 60 >test &&
> >  	git reset &&
> >  	# test sequence is s(plit), n(o), y(es), e(dit)
>
> Prerequisite lets you skip.

Yes. It lets you skip a test for a known breakage in code we're never
going to fix because we're going to delete it instead, for example. Saving
some electricity, too, by avoiding to run said test case.

> > @@ -562,7 +564,7 @@ test_expect_success 'split hunk with incomplete line at end' '
> >  	test_must_fail git grep --cached before
> >  '
> >
> > -test_expect_failure 'edit, adding lines to the first hunk' '
> > +test_expect_failure BUILTIN_ADD_I 'edit, adding lines to the first hunk' '
>
> I am not sure if this is a good change, quite honestly.  With
> s/failure/success/, perhaps, but not in the posted form.

Indeed, this was an oversight on my part, as you might have guessed from
the `failure` being replaced with `success` in the previous hunk. I simply
forgot it here.

But a more complicated solution for the same problem was applied directly
to the main branch, so I'd like to shift my attention to problems where my
input has a chance of mattering.

Ciao,
Dscho

Re: [PATCH] t3701: two subtests are fixed

[Junio C Hamano]

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

> But a more complicated solution for the same problem was applied directly
> to the main branch, so I'd like to shift my attention to problems where my
> input has a chance of mattering.

Any reasonable input makes difference.  You can even improve incrementally
with follow-up patches.

Thanks.