[BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Thomas Gummerer]

Hi,

I just tried to run the test suite with GIT_TEST_SPLIT_INDEX=YesPlease
and noticed that a few tests are broken both in pu and master.

Below the test failures on master:

Test Summary Report
-------------------
t7009-filter-branch-null-sha1.sh                 (Wstat: 256 Tests: 5 Failed: 2)
  Failed tests:  4-5
  Non-zero exit status: 1
t3900-i18n-commit.sh                             (Wstat: 256 Tests: 34 Failed: 1)
  Failed test:  34
  Non-zero exit status: 1
t5407-post-rewrite-hook.sh                       (Wstat: 256 Tests: 14 Failed: 4)
  Failed tests:  11-14
  Non-zero exit status: 1
t3415-rebase-autosquash.sh                       (Wstat: 256 Tests: 19 Failed: 15)
  Failed tests:  4-17, 19
  Non-zero exit status: 1
t3404-rebase-interactive.sh                      (Wstat: 256 Tests: 95 Failed: 54)
  Failed tests:  18, 20, 22-24, 26-43, 45, 47-74, 84-85
  Non-zero exit status: 1

Bisecting between master and v2.10.0 leads me to the merge commit
94c9b5af70 ("Merge branch 'cc/split-index-config'", 2017-03-17).

Unfortunately I don't forsee myself having time to track the bug down
soon.  Sorry for not noticing this earlier in the cycle.

The bisect log is below if someone finds it helpful:

git bisect start
# bad: [6a2c2f8d34fa1e8f3bb85d159d354810ed63692e] Git 2.13-rc0
git bisect bad 6a2c2f8d34fa1e8f3bb85d159d354810ed63692e
# good: [6ebdac1bab966b720d776aa43ca188fe378b1f4b] Git 2.10
git bisect good 6ebdac1bab966b720d776aa43ca188fe378b1f4b
# good: [733671b0fd2fb03edb05273f36ec70bd624e544f] Merge branch 'maint'
git bisect good 733671b0fd2fb03edb05273f36ec70bd624e544f
# good: [04b4f7d579056cedaae2de0a019e5993b4d9c2d0] Merge branch 'sb/submodule-update-initial-runs-custom-script' into maint
git bisect good 04b4f7d579056cedaae2de0a019e5993b4d9c2d0
# bad: [af6865a7f1e1d915d3b63e998226028ca4abb6ee] submodule.c: convert is_submodule_modified to use strbuf_getwholeline
git bisect bad af6865a7f1e1d915d3b63e998226028ca4abb6ee
# good: [3f7ebc6ece46f1c23480d094688b8b5f24eb345c] Merge branch 'jk/tempfile-ferror-fclose-confusion'
git bisect good 3f7ebc6ece46f1c23480d094688b8b5f24eb345c
# bad: [94c9b5af703eb70adba349cfbfaaa3029849744c] Merge branch 'cc/split-index-config'
git bisect bad 94c9b5af703eb70adba349cfbfaaa3029849744c
# good: [36d5286f6815542761dae92fdcdce8db1556727f] Merge branch 'ax/line-log-range-merge-fix'
git bisect good 36d5286f6815542761dae92fdcdce8db1556727f
# good: [c3a008250272295271584c6303463ffb9b055cbc] read-cache: use freshen_shared_index() in read_index_from()
git bisect good c3a008250272295271584c6303463ffb9b055cbc
# good: [228b78752de9d759839665764391262c0ec156cf] Merge branch 'jt/perf-updates'
git bisect good 228b78752de9d759839665764391262c0ec156cf
# good: [073778017158ecf3153b050776029907bc75826f] Merge branch 'kn/ref-filter-branch-list'
git bisect good 073778017158ecf3153b050776029907bc75826f
# good: [b46013950aff31b6626af96ccbf2c48469e36c66] Documentation/git-update-index: explain splitIndex.*
git bisect good b46013950aff31b6626af96ccbf2c48469e36c66
# good: [32c43f595f93cdd4ed5305aef97a3f6aaa74ed72] Sync with 'maint'
git bisect good 32c43f595f93cdd4ed5305aef97a3f6aaa74ed72
# first bad commit: [94c9b5af703eb70adba349cfbfaaa3029849744c] Merge branch 'cc/split-index-config'

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Christian Couder]

Hi,

On Thu, Apr 20, 2017 at 10:52 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> Hi,
>
> I just tried to run the test suite with GIT_TEST_SPLIT_INDEX=YesPlease
> and noticed that a few tests are broken both in pu and master.
>
> Below the test failures on master:
>
> Test Summary Report
> -------------------
> t7009-filter-branch-null-sha1.sh                 (Wstat: 256 Tests: 5 Failed: 2)
>   Failed tests:  4-5
>   Non-zero exit status: 1
> t3900-i18n-commit.sh                             (Wstat: 256 Tests: 34 Failed: 1)
>   Failed test:  34
>   Non-zero exit status: 1
> t5407-post-rewrite-hook.sh                       (Wstat: 256 Tests: 14 Failed: 4)
>   Failed tests:  11-14
>   Non-zero exit status: 1
> t3415-rebase-autosquash.sh                       (Wstat: 256 Tests: 19 Failed: 15)
>   Failed tests:  4-17, 19
>   Non-zero exit status: 1
> t3404-rebase-interactive.sh                      (Wstat: 256 Tests: 95 Failed: 54)
>   Failed tests:  18, 20, 22-24, 26-43, 45, 47-74, 84-85
>   Non-zero exit status: 1
>
> Bisecting between master and v2.10.0 leads me to the merge commit
> 94c9b5af70 ("Merge branch 'cc/split-index-config'", 2017-03-17).
>
> Unfortunately I don't forsee myself having time to track the bug down
> soon.  Sorry for not noticing this earlier in the cycle.
>
> The bisect log is below if someone finds it helpful:
>
> git bisect start
> # bad: [6a2c2f8d34fa1e8f3bb85d159d354810ed63692e] Git 2.13-rc0
> git bisect bad 6a2c2f8d34fa1e8f3bb85d159d354810ed63692e
> # good: [6ebdac1bab966b720d776aa43ca188fe378b1f4b] Git 2.10
> git bisect good 6ebdac1bab966b720d776aa43ca188fe378b1f4b
> # good: [733671b0fd2fb03edb05273f36ec70bd624e544f] Merge branch 'maint'
> git bisect good 733671b0fd2fb03edb05273f36ec70bd624e544f
> # good: [04b4f7d579056cedaae2de0a019e5993b4d9c2d0] Merge branch 'sb/submodule-update-initial-runs-custom-script' into maint
> git bisect good 04b4f7d579056cedaae2de0a019e5993b4d9c2d0
> # bad: [af6865a7f1e1d915d3b63e998226028ca4abb6ee] submodule.c: convert is_submodule_modified to use strbuf_getwholeline
> git bisect bad af6865a7f1e1d915d3b63e998226028ca4abb6ee
> # good: [3f7ebc6ece46f1c23480d094688b8b5f24eb345c] Merge branch 'jk/tempfile-ferror-fclose-confusion'
> git bisect good 3f7ebc6ece46f1c23480d094688b8b5f24eb345c
> # bad: [94c9b5af703eb70adba349cfbfaaa3029849744c] Merge branch 'cc/split-index-config'
> git bisect bad 94c9b5af703eb70adba349cfbfaaa3029849744c
> # good: [36d5286f6815542761dae92fdcdce8db1556727f] Merge branch 'ax/line-log-range-merge-fix'
> git bisect good 36d5286f6815542761dae92fdcdce8db1556727f
> # good: [c3a008250272295271584c6303463ffb9b055cbc] read-cache: use freshen_shared_index() in read_index_from()
> git bisect good c3a008250272295271584c6303463ffb9b055cbc
> # good: [228b78752de9d759839665764391262c0ec156cf] Merge branch 'jt/perf-updates'
> git bisect good 228b78752de9d759839665764391262c0ec156cf
> # good: [073778017158ecf3153b050776029907bc75826f] Merge branch 'kn/ref-filter-branch-list'
> git bisect good 073778017158ecf3153b050776029907bc75826f
> # good: [b46013950aff31b6626af96ccbf2c48469e36c66] Documentation/git-update-index: explain splitIndex.*
> git bisect good b46013950aff31b6626af96ccbf2c48469e36c66
> # good: [32c43f595f93cdd4ed5305aef97a3f6aaa74ed72] Sync with 'maint'
> git bisect good 32c43f595f93cdd4ed5305aef97a3f6aaa74ed72
> # first bad commit: [94c9b5af703eb70adba349cfbfaaa3029849744c] Merge branch 'cc/split-index-config'

Could you try with the following patch:

http://public-inbox.org/git/20170330210354.20018-1-chriscool@tuxfamily.org/

Thanks,
Christian.

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Jeff King]

On Thu, Apr 20, 2017 at 09:52:14PM +0100, Thomas Gummerer wrote:

> I just tried to run the test suite with GIT_TEST_SPLIT_INDEX=YesPlease
> and noticed that a few tests are broken both in pu and master.
> [...]
> Bisecting between master and v2.10.0 leads me to the merge commit
> 94c9b5af70 ("Merge branch 'cc/split-index-config'", 2017-03-17).

Hmm. It looks like the fix from:

  http://public-inbox.org/git/20170329080820.8084-1-chriscool@tuxfamily.org/

hasn't been merged yet. That would possibly explain the problems on
master, but I think it _is_ in pu. So maybe this is something else.

-Peff

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Thomas Gummerer]

On 04/20, Christian Couder wrote:
> Hi,
> 
> On Thu, Apr 20, 2017 at 10:52 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > Hi,
> >
> > I just tried to run the test suite with GIT_TEST_SPLIT_INDEX=YesPlease
> > and noticed that a few tests are broken both in pu and master.
> >
> > Below the test failures on master:
> >
> > Test Summary Report
> > -------------------
> > t7009-filter-branch-null-sha1.sh                 (Wstat: 256 Tests: 5 Failed: 2)
> >   Failed tests:  4-5
> >   Non-zero exit status: 1
> > t3900-i18n-commit.sh                             (Wstat: 256 Tests: 34 Failed: 1)
> >   Failed test:  34
> >   Non-zero exit status: 1
> > t5407-post-rewrite-hook.sh                       (Wstat: 256 Tests: 14 Failed: 4)
> >   Failed tests:  11-14
> >   Non-zero exit status: 1
> > t3415-rebase-autosquash.sh                       (Wstat: 256 Tests: 19 Failed: 15)
> >   Failed tests:  4-17, 19
> >   Non-zero exit status: 1
> > t3404-rebase-interactive.sh                      (Wstat: 256 Tests: 95 Failed: 54)
> >   Failed tests:  18, 20, 22-24, 26-43, 45, 47-74, 84-85
> >   Non-zero exit status: 1
> >
> > Bisecting between master and v2.10.0 leads me to the merge commit
> > 94c9b5af70 ("Merge branch 'cc/split-index-config'", 2017-03-17).
> >
> > Unfortunately I don't forsee myself having time to track the bug down
> > soon.  Sorry for not noticing this earlier in the cycle.
> >
> > The bisect log is below if someone finds it helpful:
> >
> > git bisect start
> > # bad: [6a2c2f8d34fa1e8f3bb85d159d354810ed63692e] Git 2.13-rc0
> > git bisect bad 6a2c2f8d34fa1e8f3bb85d159d354810ed63692e
> > # good: [6ebdac1bab966b720d776aa43ca188fe378b1f4b] Git 2.10
> > git bisect good 6ebdac1bab966b720d776aa43ca188fe378b1f4b
> > # good: [733671b0fd2fb03edb05273f36ec70bd624e544f] Merge branch 'maint'
> > git bisect good 733671b0fd2fb03edb05273f36ec70bd624e544f
> > # good: [04b4f7d579056cedaae2de0a019e5993b4d9c2d0] Merge branch 'sb/submodule-update-initial-runs-custom-script' into maint
> > git bisect good 04b4f7d579056cedaae2de0a019e5993b4d9c2d0
> > # bad: [af6865a7f1e1d915d3b63e998226028ca4abb6ee] submodule.c: convert is_submodule_modified to use strbuf_getwholeline
> > git bisect bad af6865a7f1e1d915d3b63e998226028ca4abb6ee
> > # good: [3f7ebc6ece46f1c23480d094688b8b5f24eb345c] Merge branch 'jk/tempfile-ferror-fclose-confusion'
> > git bisect good 3f7ebc6ece46f1c23480d094688b8b5f24eb345c
> > # bad: [94c9b5af703eb70adba349cfbfaaa3029849744c] Merge branch 'cc/split-index-config'
> > git bisect bad 94c9b5af703eb70adba349cfbfaaa3029849744c
> > # good: [36d5286f6815542761dae92fdcdce8db1556727f] Merge branch 'ax/line-log-range-merge-fix'
> > git bisect good 36d5286f6815542761dae92fdcdce8db1556727f
> > # good: [c3a008250272295271584c6303463ffb9b055cbc] read-cache: use freshen_shared_index() in read_index_from()
> > git bisect good c3a008250272295271584c6303463ffb9b055cbc
> > # good: [228b78752de9d759839665764391262c0ec156cf] Merge branch 'jt/perf-updates'
> > git bisect good 228b78752de9d759839665764391262c0ec156cf
> > # good: [073778017158ecf3153b050776029907bc75826f] Merge branch 'kn/ref-filter-branch-list'
> > git bisect good 073778017158ecf3153b050776029907bc75826f
> > # good: [b46013950aff31b6626af96ccbf2c48469e36c66] Documentation/git-update-index: explain splitIndex.*
> > git bisect good b46013950aff31b6626af96ccbf2c48469e36c66
> > # good: [32c43f595f93cdd4ed5305aef97a3f6aaa74ed72] Sync with 'maint'
> > git bisect good 32c43f595f93cdd4ed5305aef97a3f6aaa74ed72
> > # first bad commit: [94c9b5af703eb70adba349cfbfaaa3029849744c] Merge branch 'cc/split-index-config'
> 
> Could you try with the following patch:
> 
> http://public-inbox.org/git/20170330210354.20018-1-chriscool@tuxfamily.org/

Yeah, I tried with and without that patch with the same result.
Unless I'm screwing something up when testing I don't think this fixes
the issue unfortunately.

> Thanks,
> Christian.

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Could you try with the following patch:
>
> http://public-inbox.org/git/20170330210354.20018-1-chriscool@tuxfamily.org/

Ah, this reminds me.  The patch has been in the stalled state for
quite some time due to confusing description.  How about explaining
it like so and merge it to 'next'?

-- >8 --
From: Christian Couder <christian.couder@gmail.com>
Date: Thu, 30 Mar 2017 23:03:54 +0200
Subject: [PATCH] read-cache: avoid using git_path() in freshen_shared_index()

When performing an interactive rebase in split-index mode,
the commit message that one should rework when squashing commits
can contain some garbage instead of the usual concatenation of
both of the commit messages.

The code uses git_path() to compute the shared index filename, and
passes it to check_and_freshen_file() as its argument; there is no
guarantee that the rotating pathname buffer passed as argument will
stay valid during the life of this call.  Make our own copy before
calling the function and pass the copy as its argument to avoid this
risky pattern.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index 8cf0673adc..0b166c211a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1690,9 +1690,10 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
  */
 static void freshen_shared_index(char *base_sha1_hex, int warn)
 {
-	const char *shared_index = git_path("sharedindex.%s", base_sha1_hex);
+	char *shared_index = git_pathdup("sharedindex.%s", base_sha1_hex);
 	if (!check_and_freshen_file(shared_index, 1) && warn)
 		warning("could not freshen shared index '%s'", shared_index);
+	free(shared_index);
 }
 
 int read_index_from(struct index_state *istate, const char *path)
-- 
2.13.0-rc0-176-gf5d713c632

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Christian Couder]

On Fri, Apr 21, 2017 at 6:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> Could you try with the following patch:
>>
>> http://public-inbox.org/git/20170330210354.20018-1-chriscool@tuxfamily.org/
>
> Ah, this reminds me.  The patch has been in the stalled state for
> quite some time due to confusing description.  How about explaining
> it like so and merge it to 'next'?

Yeah, I am ok with this description. Thanks!

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Christian Couder]

On Thu, Apr 20, 2017 at 11:24 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> On 04/20, Christian Couder wrote:
>>
>> Could you try with the following patch:
>>
>> http://public-inbox.org/git/20170330210354.20018-1-chriscool@tuxfamily.org/
>
> Yeah, I tried with and without that patch with the same result.
> Unless I'm screwing something up when testing I don't think this fixes
> the issue unfortunately.

Ok, I will take a look soon.

By the way I think that GIT_TEST_SPLIT_INDEX has become redundant now
that there is core.splitIndex.
So perhaps in the long run it will be best to deprecate
GIT_TEST_SPLIT_INDEX and eventually remove it.

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Duy Nguyen]

On Fri, Apr 21, 2017 at 2:10 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Thu, Apr 20, 2017 at 11:24 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
>> On 04/20, Christian Couder wrote:
>>>
>>> Could you try with the following patch:
>>>
>>> http://public-inbox.org/git/20170330210354.20018-1-chriscool@tuxfamily.org/
>>
>> Yeah, I tried with and without that patch with the same result.
>> Unless I'm screwing something up when testing I don't think this fixes
>> the issue unfortunately.
>
> Ok, I will take a look soon.
>
> By the way I think that GIT_TEST_SPLIT_INDEX has become redundant now
> that there is core.splitIndex.
> So perhaps in the long run it will be best to deprecate
> GIT_TEST_SPLIT_INDEX and eventually remove it.

I think you can't, at least the way I understand this variable. It's a
_test_ variable to force exercise split index code path a whole lot
more, by running the entire test suite with split index always
enabled, instead of just a couple in  t????-split-index.sh. We can't
achieve the same with core.splitIndex because that's more about user
control and you can't just set core.splitIndex for the entire test
suite (can we?).

I would understand if the failed tests are about core.splitIndex (they
both try to control how split index is created), but failed tests look
totally unrelated. We may have discovered some bug (or that git_path
one Jeff mentioned).
-- 
Duy

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Christian Couder]

On Fri, Apr 21, 2017 at 11:53 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Fri, Apr 21, 2017 at 2:10 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Thu, Apr 20, 2017 at 11:24 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
>>> On 04/20, Christian Couder wrote:
>>>>
>>>> Could you try with the following patch:
>>>>
>>>> http://public-inbox.org/git/20170330210354.20018-1-chriscool@tuxfamily.org/
>>>
>>> Yeah, I tried with and without that patch with the same result.
>>> Unless I'm screwing something up when testing I don't think this fixes
>>> the issue unfortunately.
>>
>> Ok, I will take a look soon.
>>
>> By the way I think that GIT_TEST_SPLIT_INDEX has become redundant now
>> that there is core.splitIndex.
>> So perhaps in the long run it will be best to deprecate
>> GIT_TEST_SPLIT_INDEX and eventually remove it.
>
> I think you can't, at least the way I understand this variable. It's a
> _test_ variable to force exercise split index code path a whole lot
> more, by running the entire test suite with split index always
> enabled, instead of just a couple in  t????-split-index.sh. We can't
> achieve the same with core.splitIndex because that's more about user
> control and you can't just set core.splitIndex for the entire test
> suite (can we?).

Yeah, you are right.
It looks like we have GIT_TEST_OPTS to pass options like --debug,
--valgrind, --verbose, but we don't have an environment variable to
set config options.

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Christian Couder]

On Fri, Apr 21, 2017 at 1:46 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Fri, Apr 21, 2017 at 11:53 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Fri, Apr 21, 2017 at 2:10 PM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> On Thu, Apr 20, 2017 at 11:24 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
>>>> On 04/20, Christian Couder wrote:
>>>>>
>>>>> Could you try with the following patch:
>>>>>
>>>>> http://public-inbox.org/git/20170330210354.20018-1-chriscool@tuxfamily.org/
>>>>
>>>> Yeah, I tried with and without that patch with the same result.
>>>> Unless I'm screwing something up when testing I don't think this fixes
>>>> the issue unfortunately.
>>>
>>> Ok, I will take a look soon.
>>>
>>> By the way I think that GIT_TEST_SPLIT_INDEX has become redundant now
>>> that there is core.splitIndex.
>>> So perhaps in the long run it will be best to deprecate
>>> GIT_TEST_SPLIT_INDEX and eventually remove it.
>>
>> I think you can't, at least the way I understand this variable. It's a
>> _test_ variable to force exercise split index code path a whole lot
>> more, by running the entire test suite with split index always
>> enabled, instead of just a couple in  t????-split-index.sh. We can't
>> achieve the same with core.splitIndex because that's more about user
>> control and you can't just set core.splitIndex for the entire test
>> suite (can we?).
>
> Yeah, you are right.
> It looks like we have GIT_TEST_OPTS to pass options like --debug,
> --valgrind, --verbose, but we don't have an environment variable to
> set config options.

Or maybe GIT_CONFIG_PARAMETERS works for this purpose?

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Duy Nguyen]

On Fri, Apr 21, 2017 at 6:57 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Fri, Apr 21, 2017 at 1:46 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Fri, Apr 21, 2017 at 11:53 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Fri, Apr 21, 2017 at 2:10 PM, Christian Couder
>>> <christian.couder@gmail.com> wrote:
>>>> On Thu, Apr 20, 2017 at 11:24 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
>>>>> On 04/20, Christian Couder wrote:
>>>>>>
>>>>>> Could you try with the following patch:
>>>>>>
>>>>>> http://public-inbox.org/git/20170330210354.20018-1-chriscool@tuxfamily.org/
>>>>>
>>>>> Yeah, I tried with and without that patch with the same result.
>>>>> Unless I'm screwing something up when testing I don't think this fixes
>>>>> the issue unfortunately.
>>>>
>>>> Ok, I will take a look soon.
>>>>
>>>> By the way I think that GIT_TEST_SPLIT_INDEX has become redundant now
>>>> that there is core.splitIndex.
>>>> So perhaps in the long run it will be best to deprecate
>>>> GIT_TEST_SPLIT_INDEX and eventually remove it.
>>>
>>> I think you can't, at least the way I understand this variable. It's a
>>> _test_ variable to force exercise split index code path a whole lot
>>> more, by running the entire test suite with split index always
>>> enabled, instead of just a couple in  t????-split-index.sh. We can't
>>> achieve the same with core.splitIndex because that's more about user
>>> control and you can't just set core.splitIndex for the entire test
>>> suite (can we?).
>>
>> Yeah, you are right.
>> It looks like we have GIT_TEST_OPTS to pass options like --debug,
>> --valgrind, --verbose, but we don't have an environment variable to
>> set config options.
>
> Or maybe GIT_CONFIG_PARAMETERS works for this purpose?

It has to be set inside test-lib.sh, not from outside because
environment variables from outside are filtered if I remember
correctly and only a few specials plus those GIT_TEST_ can survive.
Some tests override GIT_CONFIG_PARAMETERS themselves to pass config
vars to certain command (I know because I just did a couple days ago
;).which loses core.splitIndex.
-- 
Duy

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Christian Couder]

On Thu, Apr 20, 2017 at 11:24 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> On 04/20, Christian Couder wrote:
>>
>> Could you try with the following patch:
>>
>> http://public-inbox.org/git/20170330210354.20018-1-chriscool@tuxfamily.org/
>
> Yeah, I tried with and without that patch with the same result.
> Unless I'm screwing something up when testing I don't think this fixes
> the issue unfortunately.

I just tried on "pu" and only the first test
(t7009-filter-branch-null-sha1.sh) fails there.

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Christian Couder]

On Fri, Apr 21, 2017 at 4:25 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Thu, Apr 20, 2017 at 11:24 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
>> On 04/20, Christian Couder wrote:
>>>
>>> Could you try with the following patch:
>>>
>>> http://public-inbox.org/git/20170330210354.20018-1-chriscool@tuxfamily.org/
>>
>> Yeah, I tried with and without that patch with the same result.
>> Unless I'm screwing something up when testing I don't think this fixes
>> the issue unfortunately.
>
> I just tried on "pu" and only the first test
> (t7009-filter-branch-null-sha1.sh) fails there.

I bisected this test's failure (when using
GIT_TEST_SPLIT_INDEX=YesPlease) to e6a1dd77e1 (read-cache: regenerate
shared index if necessary, 2017-02-27).

The failing test is the following:

test_expect_success 'filter commands are still checked' '
        test_must_fail git filter-branch \
                --force --prune-empty \
                --index-filter "git rm --cached --ignore-unmatch three.t"
'

And if I add the following at the beginning of the test:

        git config splitIndex.maxPercentChange 100 &&

the test then passes.

So It looks like in split index mode the test doesn't expect the
shared index to be regenerated.
Maybe Peff, as he is the author of this test, or Duy have an idea about this?

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Jeff King]

On Fri, Apr 21, 2017 at 05:23:51PM +0200, Christian Couder wrote:

> > I just tried on "pu" and only the first test
> > (t7009-filter-branch-null-sha1.sh) fails there.
> 
> I bisected this test's failure (when using
> GIT_TEST_SPLIT_INDEX=YesPlease) to e6a1dd77e1 (read-cache: regenerate
> shared index if necessary, 2017-02-27).
> 
> The failing test is the following:
> 
> test_expect_success 'filter commands are still checked' '
>         test_must_fail git filter-branch \
>                 --force --prune-empty \
>                 --index-filter "git rm --cached --ignore-unmatch three.t"
> '
> 
> And if I add the following at the beginning of the test:
> 
>         git config splitIndex.maxPercentChange 100 &&
> 
> the test then passes.
> 
> So It looks like in split index mode the test doesn't expect the
> shared index to be regenerated.
> Maybe Peff, as he is the author of this test, or Duy have an idea about this?

Right. The test has a broken tree with a null sha1, and filter-branch
will do:

  GIT_ALLOW_NULL_SHA1=1 git read-tree $broken

to allow it to enter the index. We expect that further commands that
write out the index will not allow it (so you can run commands that
remove the broken entry, but not ones that leave it).

So without split index, this command:

  git rm --cached three.t

will fail. But in split-index mode, the broken entry is in another
index, and is left untouched. I'm not sure there's a way to reconcile
the split-index behavior with what the test is expecting; it's
inherently optimizing out the thing that the test wants to check.

Probably we should catch the broken index entry when we write out the
tree, too. We usually do catch missing objects, but this one is a
gitlink, so it's OK for it to be missing. I think we should catch the
null sha1 specifically, though, as that was the intent of the commit
that added t7009.

So the patch below _almost_ works. It fixes the failing test. But note
the new test I added, with a noop index-filter. That checks that we
don't retain the cache-tree extension from the bogus tree when reading.
But for some reason it fails on split-index mode. Does cache-tree stuff
somehow work differently there?

diff --git a/cache-tree.c b/cache-tree.c
index 345ea3596..34baa6d85 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -354,7 +354,9 @@ static int update_one(struct cache_tree *it,
 			entlen = pathlen - baselen;
 			i++;
 		}
-		if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {
+
+		if (is_null_sha1(sha1) ||
+		    (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))) {
 			strbuf_release(&buffer);
 			if (expected_missing)
 				return -1;
diff --git a/read-cache.c b/read-cache.c
index b3d0f3c30..15a4779f2 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2197,6 +2197,7 @@ static int do_write_index(struct index_state *istate, int newfd,
 	int entries = istate->cache_nr;
 	struct stat st;
 	struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
+	int drop_cache_tree = 0;
 
 	for (i = removed = extended = 0; i < entries; i++) {
 		if (cache[i]->ce_flags & CE_REMOVE)
@@ -2247,6 +2248,8 @@ static int do_write_index(struct index_state *istate, int newfd,
 				warning(msg, ce->name);
 			else
 				return error(msg, ce->name);
+
+			drop_cache_tree = 1;
 		}
 		if (ce_write_entry(&c, newfd, ce, previous_name) < 0)
 			return -1;
@@ -2265,7 +2268,7 @@ static int do_write_index(struct index_state *istate, int newfd,
 		if (err)
 			return -1;
 	}
-	if (!strip_extensions && istate->cache_tree) {
+	if (!strip_extensions && !drop_cache_tree && istate->cache_tree) {
 		struct strbuf sb = STRBUF_INIT;
 
 		cache_tree_write(&sb, istate->cache_tree);
diff --git a/t/t7009-filter-branch-null-sha1.sh b/t/t7009-filter-branch-null-sha1.sh
index c27f90f28..a8d9ec498 100755
--- a/t/t7009-filter-branch-null-sha1.sh
+++ b/t/t7009-filter-branch-null-sha1.sh
@@ -31,6 +31,12 @@ test_expect_success 'setup: bring HEAD and index in sync' '
 	git commit -a -m "back to normal"
 '
 
+test_expect_success 'noop filter-branch complains' '
+	test_must_fail git filter-branch \
+		--force --prune-empty \
+		--index-filter "true"
+'
+
 test_expect_success 'filter commands are still checked' '
 	test_must_fail git filter-branch \
 		--force --prune-empty \

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Jeff King]

On Fri, Apr 21, 2017 at 07:25:21PM +0700, Duy Nguyen wrote:

> >> Yeah, you are right.
> >> It looks like we have GIT_TEST_OPTS to pass options like --debug,
> >> --valgrind, --verbose, but we don't have an environment variable to
> >> set config options.
> >
> > Or maybe GIT_CONFIG_PARAMETERS works for this purpose?
> 
> It has to be set inside test-lib.sh, not from outside because
> environment variables from outside are filtered if I remember
> correctly and only a few specials plus those GIT_TEST_ can survive.
> Some tests override GIT_CONFIG_PARAMETERS themselves to pass config
> vars to certain command (I know because I just did a couple days ago
> ;).which loses core.splitIndex.

It does seem reasonable to have a GIT_TEST_REPO_CONFIG that sets some
variables by default in the trash repository created by
test_create_repo.

That doesn't quite catch everything, because a few tests make their own
sub-repos. But those cases are usually testing something very specific
anyway.

-Peff

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

[Thomas Gummerer]

On 04/21, Christian Couder wrote:
> On Thu, Apr 20, 2017 at 11:24 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > On 04/20, Christian Couder wrote:
> >>
> >> Could you try with the following patch:
> >>
> >> http://public-inbox.org/git/20170330210354.20018-1-chriscool@tuxfamily.org/
> >
> > Yeah, I tried with and without that patch with the same result.
> > Unless I'm screwing something up when testing I don't think this fixes
> > the issue unfortunately.
> 
> I just tried on "pu" and only the first test
> (t7009-filter-branch-null-sha1.sh) fails there.

I can't seem to reproduce this now either with the latest pu or next.
I must have messed something up while testing.

Sorry for the noise.