Exposing bug in string index

[kspangsege]

NOT FIXED!

@astigsen

[astigsen]

Fix in 85bb805

[kspangsege]

I guess you forgot to commit to the PR branch. Anyway, cool. 👍

What about the unit tests in the branch? Have you taken care of getting them merged? Do they pass?

[kspangsege]

Alexander, please read my comments above, and finalize this pull request.

[astigsen]

It was on purpose that I did not push it to your branch. In general I think it is a bad idea to push into other peoples repos. You never know if they notice, and I have personally had unintended merges happen when pulling from my own repo, without having noticed that other had pushed to it.

I think the best approach is to let the owner of the PR pull in the fix.

The fix does make it pass the unit tests.

[kspangsege]

Ok with respect to not pushing to each others branches. However, when I create a pull-request with the purpose of getting a bug fixed, and the task of fixing the bug gets assigned to you, you are supposed to work within that pull request, and push commits to the associated branch. In this case, you could have considered that branch, your own.

Anyway, you either didn't read or didn't answer this question:

What about the unit tests in this branch? Have you taken care of getting them merged?

Note that this PR does contain commits that are related to the bug, and that are supposed to be merged into master.

[kspangsege]

If you want me to, in the future, I can create branches like this one in the "no mans land" repo, Tightdb:tightdb.

[bmunkholm]

I think that's a good idea. No everyone has rights to push to other peoples
branches.

// Brian

On Tue, Sep 24, 2013 at 8:31 PM, Kristian Spangsege <
[email protected]> wrote:

If you want me to, in the future, I can create branches like this one in
the "no mans land" repo, Tightdb:tightdb.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/145#issuecomment-25030473
.

[astigsen]

I have pushed the fix to this PR. The unit tests also seem to merge clean.

[kspangsege]

Ok, cool, but you still haven't made it clear to me, whether you have also checked that the new unit test added by this PR passes.

Also, I would expect you to push the "Merge pull request" button, but if you refuse to, let me know.

[astigsen]

That was answered in one of the first comments:

The fix does make it pass the unit tests.

[kspangsege]

It wasn't clear to me, but ok. I assume you did see the second part of my comment too.

[astigsen]

Just waiting for a +1 :-)

On Tue, Sep 24, 2013 at 5:32 PM, Kristian Spangsege <
[email protected]> wrote:

It wasn't clear to me, but ok. I assume you did see the second part of my
comment too.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/145#issuecomment-25054186
.

[kspangsege]

👍