| Message ID | cf6aee9acadfb666de6b24b9ed63e1a65bfc009e.1655220242.git.git@grubix.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t3701: two subtests are fixed | expand |
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
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 &&
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
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
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
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.
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
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.
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.
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
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.
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 &&
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(-)