threads / bug / 45763

[BUG] test suite broken with GIT_TEST_SPLIT_INDEX

Subject: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

## tl;dr

16 messages between Apr 20, 2017 and Apr 24, 2017.

replies: 15people: 5as markdown or json

Thomas Gummerer· Apr 20, 2017, 20:52 UTC · lore
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'

Christian Couder· Apr 20, 2017, 21:03 UTC · re: Thomas Gummerer · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

Hi,
On Thu, Apr 20, 2017 at 10:52 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 61 quoted lines
> 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.

Thomas Gummerer· Apr 20, 2017, 21:24 UTC · re: Christian Couder · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On 04/20, Christian Couder wrote:
Show 68 quoted lines
> 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.
Christian Couder· Apr 21, 2017, 07:10 UTC · re: Thomas Gummerer · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On Thu, Apr 20, 2017 at 11:24 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 9 quoted lines
> 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.

Duy Nguyen· Apr 21, 2017, 09:53 UTC · re: Christian Couder · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On Fri, Apr 21, 2017 at 2:10 PM, Christian Couder <christian.couder@gmail.com> wrote:

Show 17 quoted lines
> 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
Christian Couder· Apr 21, 2017, 11:46 UTC · re: Duy Nguyen · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On Fri, Apr 21, 2017 at 11:53 AM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 27 quoted lines
> 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.

Christian Couder· Apr 21, 2017, 11:57 UTC · re: Christian Couder · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On Fri, Apr 21, 2017 at 1:46 PM, Christian Couder <christian.couder@gmail.com> wrote:

Show 33 quoted lines
> 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?
Duy Nguyen· Apr 21, 2017, 12:25 UTC · re: Christian Couder · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On Fri, Apr 21, 2017 at 6:57 PM, Christian Couder <christian.couder@gmail.com> wrote:

Show 37 quoted lines
> 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
Jeff King· Apr 21, 2017, 17:09 UTC · re: Duy Nguyen · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On Fri, Apr 21, 2017 at 07:25:21PM +0700, Duy Nguyen wrote:
Show 13 quoted lines
> >> 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
Christian Couder· Apr 21, 2017, 14:25 UTC · re: Thomas Gummerer · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On Thu, Apr 20, 2017 at 11:24 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 9 quoted lines
> 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.

Christian Couder· Apr 21, 2017, 15:23 UTC · re: Christian Couder · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On Fri, Apr 21, 2017 at 4:25 PM, Christian Couder <christian.couder@gmail.com> wrote:

Show 13 quoted lines
> 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?

Jeff King· Apr 21, 2017, 17:05 UTC · re: Christian Couder · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On Fri, Apr 21, 2017 at 05:23:51PM +0200, Christian Couder wrote:
Show 24 quoted lines
> > 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 \
Thomas Gummerer· Apr 24, 2017, 22:15 UTC · re: Christian Couder · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On 04/21, Christian Couder wrote:
Show 13 quoted lines
> 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.
Junio C Hamano· Apr 21, 2017, 04:01 UTC · re: Christian Couder · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

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
Christian Couder· Apr 21, 2017, 07:02 UTC · re: Junio C Hamano · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On Fri, Apr 21, 2017 at 6:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> 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!
Jeff King· Apr 20, 2017, 21:06 UTC · re: Thomas Gummerer · lore

Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

On Thu, Apr 20, 2017 at 09:52:14PM +0100, Thomas Gummerer wrote:
Show 5 quoted lines
> 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

← back to recent threads