modules: Add newlen == 0 handling to RM_StringTruncate (#3717) #3718
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
neomantra
added a commit
to neomantra/redex
that referenced
this pull request
Jul 17, 2017
Since Redis 4 was released without redis/redis#3718 this change was required to make CHOP work properly with Redis 4.0.0. Now all tests pass on stock Redis 4.0.0.
neomantra
force-pushed
the
StringTruncate_newLenZero
branch
from
087936a
to
3e7e215
Compare
itamarhaber
previously approved these changes
Jun 11, 2018
|
@neomantra thanks for this fix - it looks good. @antirez - please consider merging. |
neomantra
force-pushed
the
StringTruncate_newLenZero
branch
from
3e7e215
to
b743ad0
Compare
|
I just rebased this branch to the latest. Any chance to make it into 5.0? |
neomantra
force-pushed
the
StringTruncate_newLenZero
branch
2 times, most recently
from
7602dc0
to
8aed0ee
Compare
|
@antirez @itamarhaber I saw the Tweet about "[Refactoring...] requires a lot of care, and a lot of tests." I remembered that I had added some unmerged module test tooling in this PR and also remembered that the workflow for testing modules could use some improvement. So I put some effort into this last commit 9c7f3d7 ( If you like this and would prefer it on its own branch (without the StringTruncate bugfix), I can do that. |
Previously, passing 0 for newlen would not truncate the string at all. This adds handling of this case, freeing the old string and creating a new empty string.
…XXXReply functions * adds TEST.STRING.TRUNCATE and includes it in TEST.IT This test fails unless issue redis#3717 is resolved * add handling for ERROR replies in TestAssertStringReply and TestAssertIntegerReply Previously these TestAssert functions would output something like, `<test> Unexpected reply type 1` rather than emitting the error message of what was wrong.
* add `test-module` make target * `make distclean` now cleans module src/test directories * `runtest-moduleapi` now builds and exercises `testmodule` * add `clean` target for `commandfilter` * add `.gitignore` for `commandfilter` * typo "SOME TEST NOT PASSED!" -> "SOME TEST DID NOT PASS!"
neomantra
force-pushed
the
StringTruncate_newLenZero
branch
from
9c7f3d7
to
595b272
Compare
|
If I take the time to rebase this, will you actually merge it? I've rebased it 4 times already over 5 years -- it is an obvious bug in a public modules interface. |
zuiderkwast
added
the
state:to-be-merged
|
This one must have been forgotten. @redis/core-team anyone have a look? |
|
Oh sad, it was forgotten. @neomantra Would you mind doing a rebase of the code. I looked through the code and thumbs'd up a couple of other comments that still weren't addressed. Sorry for dropping this! |
oranagra
reviewed
Jun 22, 2021
oranagra
added
the
release-notes
…wLenZero Conflicts: runtest-moduleapi src/Makefile tests/modules/Makefile
oranagra
approved these changes
Jun 22, 2021
oranagra
added a commit
to oranagra/redis
that referenced
this pull request
Jun 22, 2021
- Introduce a new sdssubstr api as a building block for sdsrange. The API of sdsrange is many times hard to work with and also has corner case that cause bugs. sdsrange is easy to work with and also simplifies the implementation of sdsrange. - Revert the fix to RM_StringTruncate and just use sdssubstr instead of sdsrange. - Solve valgrind warnings from the new tests introduced by the previous PR.
|
This is not the first time i see the bad API of |
oranagra
added a commit
that referenced
this pull request
Jun 22, 2021
- Introduce a new sdssubstr api as a building block for sdsrange. The API of sdsrange is many times hard to work with and also has corner case that cause bugs. sdsrange is easy to work with and also simplifies the implementation of sdsrange. - Revert the fix to RM_StringTruncate and just use sdssubstr instead of sdsrange. - Solve valgrind warnings from the new tests introduced by the previous PR.
x77a1
pushed a commit
to x77a1/redis
that referenced
this pull request
Jul 11, 2021
…edis#3718) Previously, passing 0 for newlen would not truncate the string at all. This adds handling of this case, freeing the old string and creating a new empty string. Other changes: - Move `src/modules/testmodule.c` to `tests/modules/basics.c` - Introduce that basic test into the test suite - Add tests to cover StringTruncate - Add `test-modules` build target for the main makefile - Extend `distclean` build target to clean modules too
x77a1
pushed a commit
to x77a1/redis
that referenced
this pull request
Jul 11, 2021
- Introduce a new sdssubstr api as a building block for sdsrange. The API of sdsrange is many times hard to work with and also has corner case that cause bugs. sdsrange is easy to work with and also simplifies the implementation of sdsrange. - Revert the fix to RM_StringTruncate and just use sdssubstr instead of sdsrange. - Solve valgrind warnings from the new tests introduced by the previous PR.
oranagra
pushed a commit
that referenced
this pull request
Jul 19, 2021
src/modules make failed. As in #3718 testmodule.c was removed. But the makefile was not updated
oranagra
pushed a commit
to oranagra/redis
that referenced
this pull request
Jul 20, 2021
…edis#3718) Previously, passing 0 for newlen would not truncate the string at all. This adds handling of this case, freeing the old string and creating a new empty string. Other changes: - Move `src/modules/testmodule.c` to `tests/modules/basics.c` - Introduce that basic test into the test suite - Add tests to cover StringTruncate - Add `test-modules` build target for the main makefile - Extend `distclean` build target to clean modules too (cherry picked from commit 1ccf2ca)
oranagra
added a commit
to oranagra/redis
that referenced
this pull request
Jul 20, 2021
- Introduce a new sdssubstr api as a building block for sdsrange. The API of sdsrange is many times hard to work with and also has corner case that cause bugs. sdsrange is easy to work with and also simplifies the implementation of sdsrange. - Revert the fix to RM_StringTruncate and just use sdssubstr instead of sdsrange. - Solve valgrind warnings from the new tests introduced by the previous PR. (cherry picked from commit ae418ec)
oranagra
pushed a commit
to oranagra/redis
that referenced
this pull request
Jul 20, 2021
src/modules make failed. As in redis#3718 testmodule.c was removed. But the makefile was not updated (cherry picked from commit d54c908)
Merged
oranagra
pushed a commit
that referenced
this pull request
Jul 21, 2021
Previously, passing 0 for newlen would not truncate the string at all. This adds handling of this case, freeing the old string and creating a new empty string. Other changes: - Move `src/modules/testmodule.c` to `tests/modules/basics.c` - Introduce that basic test into the test suite - Add tests to cover StringTruncate - Add `test-modules` build target for the main makefile - Extend `distclean` build target to clean modules too (cherry picked from commit 1ccf2ca)
oranagra
added a commit
that referenced
this pull request
Jul 21, 2021
- Introduce a new sdssubstr api as a building block for sdsrange. The API of sdsrange is many times hard to work with and also has corner case that cause bugs. sdsrange is easy to work with and also simplifies the implementation of sdsrange. - Revert the fix to RM_StringTruncate and just use sdssubstr instead of sdsrange. - Solve valgrind warnings from the new tests introduced by the previous PR. (cherry picked from commit ae418ec)
oranagra
pushed a commit
that referenced
this pull request
Jul 21, 2021
JackieXie168
pushed a commit
to JackieXie168/redis
that referenced
this pull request
Sep 8, 2021
…edis#3718) Previously, passing 0 for newlen would not truncate the string at all. This adds handling of this case, freeing the old string and creating a new empty string. Other changes: - Move `src/modules/testmodule.c` to `tests/modules/basics.c` - Introduce that basic test into the test suite - Add tests to cover StringTruncate - Add `test-modules` build target for the main makefile - Extend `distclean` build target to clean modules too
JackieXie168
pushed a commit
to JackieXie168/redis
that referenced
this pull request
Sep 8, 2021
- Introduce a new sdssubstr api as a building block for sdsrange. The API of sdsrange is many times hard to work with and also has corner case that cause bugs. sdsrange is easy to work with and also simplifies the implementation of sdsrange. - Revert the fix to RM_StringTruncate and just use sdssubstr instead of sdsrange. - Solve valgrind warnings from the new tests introduced by the previous PR.
JackieXie168
pushed a commit
to JackieXie168/redis
that referenced
this pull request
Sep 8, 2021
src/modules make failed. As in redis#3718 testmodule.c was removed. But the makefile was not updated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, passing 0 for newlen would not truncate the string at all.
This adds handling of this case, freeing the old string and creating a new empty string.
Other changes:
src/modules/testmodule.ctotests/modules/basics.ctest-modulesbuild target for the main makefiledistcleanbuild target to clean modules too