Hiredis 0.12.0

[michael-grunder]

Hey everybody,

I have created a develop branch for hiredis, and would like to get this going by going through a variety of outstanding issues and pull requests to see what we might want to get tested and incorporated for the next official release 😺!

Things I'm pretty sure we should merge:

• Free string if it is unused #286 -- This one looks good. I agree with @mattsta that we should probably backport dict.c from Redis, but it seems reasonable to incorporate this one first (as it's a quick one).
• Correct grammar in Readme #285, Couple minor doc comment fixes #282, Fix typos #267 -- Just a tiny grammar fixes, but they look correct to me, so we should merge.
• change some function arg prototypes to use const char *. and add redisAs... #268 -- I need to test this one, but generally speaking I like being idiomatic and using const char * vs. char * so it's clear that the string isn't going to be modified.
• Improve redisAppendCommandArgv performance #260 -- We've been using this in production for ages, and as of yet it has not ruined everything. I think we should merge.
• Fix adapters/libuv.h #251 -- This fixes a libuv compile error, and I favor it over the similar Static declaration of 'redisLibuvAttach' #258 as the non-static declaration isn't necessary at all as far as I can tell.

Things we might want to merge:

• fixed redisReader memory alloc #279 -- This is probably technically correct, but it's not something that really bothers me one way or the other. There are only a few calloc calls in hiredis, and they vary on argument order (when creating just 1 thing).
• Add support for enabling SO_REUSEADDR when connecting to the Redis server using bind #264 -- This one looks fine to me, but I'm only mildly familiar with this TCP option. Just read up on it and I don't see how it would cause any issues.
• Increase maximum SDS size #261 -- I vote yes on this one, but @pietern mentioned that it would break downstream packages, so I'm curious in what way that would be.
• Fix build under kfreebsd. #254 -- I would like some input from fancier individuals such as @mattsta or @badboy. 😃
• Provide sink for formatted commnad allocated by hiredis library #250 -- IMHO I can see why this could be useful (in the case where a custom free was being used), but is not something I have personally run into.
• Bug in net.c #239 -- I'll investigate and test this myself, but on first glance it looks good.

So that takes us back roughly a year for pull requests. I am going to familiarize myself with the ng branch next and see about moving that forward. I think however this is a good start. Most of these are very small and should be pretty safe.

Cheers guys!
Mike

[badboy]

ACK on everything in the Things I'm pretty sure we should merge list.
#279: just cosmetic, there's actually just one place otherwise in sds.c. Best to keep it technically correct in the long run.
#261: I'm on @pietern side here. Let's release a proper bug-fix version first, than work on a major release next including this.
#254: Breaks nothing it seems, fixes kfreebsd. Merge it.
#250: Technical point of view: Yip, providing this makes sense. Code Point of View: Not correct (yet). Decalred as "int" but returns nothing.
#239: why oh why, glibc? For the case where people might compile their own code with _GNU_SOURCE hiredis code might actually break. I don't really like the fix because of glibc, but if that is what it takes to not break I'm ok with it.

[michael-grunder]

Thanks @badboy!

• fixed redisReader memory alloc #279 -- Happy to make the other call conform here.
• Increase maximum SDS size #261 -- Agreed
• Fix build under kfreebsd. #254 -- Sounds good. Thanks for testing this
• Provide sink for formatted commnad allocated by hiredis library #250 -- Nice catch. That's super easy to fix. I'll cherry pick it in and then correct the return type.
• Bug in net.c #239 -- I'll investigate this more before merging. I'm not an expert with these sorts of compiler options.

Cheers!
Mike

[NanXiao]

@badboy @michael-grunder : could you check #257 ? Thanks!

[michael-grunder]

@NanXiao Yes! I've got some time over the xmas to work on this and I'll test for sure. First glance, looks pretty good. :)

[NanXiao]

@michael-grunder 🆗 Thanks! Merry xmas! :)

[badboy]

#257 seems to not break anything, but will cover even more edge cases, so we should include it in a release.

@michael-grunder How do we move on now? Get the mentioned things merged into develop, do further testing and then prepare a release like 1-2 weeks from now?

[mattsta]

How about:

• merge everything here into develop (with reviews/fixes where necessary—I'll get around to reading all of this soon!)
  • when stable enough, merge develop into master as one final update to the current 0.x version then tag, release, etc.
• all future changes/updates/cleanup will be in a new 1.x.y version
  • first task there is probably re-evaulating code copied from Redis. Update out of sync code, maybe refactor some code with less copy/paste (on both the Redis and hiredis sides), create common reusable components, etc.

[badboy]

Sounds like a good plan.

[michael-grunder]

Agreed. I'll start getting these merged. I would have earlier but the holidays happened 😃

[badboy]

@michael-grunder No one expects people to work on the holidays, so everything is fine (just I'm at a hacker congress, so I thought I could atleat do some hacking)

[mattsta]

All the PRs above look good (with changes where necessary). I've got them in a local develop branch and I'll push it up here later today.

[mattsta]

develop updated with:

f28872c Cleanup tabs and end of line whitespace
dad0516 Update dependency list in Makefile
9abfdf9 Build static library by default
1d2b4d3 Generate pkgconf during build
cc20232 Fix errno error buffers to not clobber errors
ba3e74c Refactor reading code into read.c
0cacb48 Fix sds building with C++
6a00a46 Fix redisAppendCommand error result
0c9ff5b Add GLib 2.0 adapter
e30c96e Add empty pointer check in error cleanup
d1e820d Add error check in redisContextInit
b6a8607 Fix redisvFormatCommand format parsing
9a753b4 Add API to free hiredis (sds) formattings
1b392eb Add API to free hiredis (char *) formattings
85c6bfb Fix build under kfreebsd
9b83ddc Fix clang analyzer warning
7c4d255 Add support for SO_REUSEADDR
a1bc89b Improve calloc() correctness
9c57314 Cleanup libuv adapter
40f7035 Improve redisAppendCommandArgv performance
3315c09 Use stricter function argument types
865a368 Fix README typos
2d814b8 Fix minor comment problems
abbd340 Fix README typo
2a6dbdd Free string if it is unused
a1f6ce0 Add syntax highlighting to README.md

Test/read/check those commits and keep suggesting a few more fixes/changes for the next version!

[badboy]

How about including the following patch (or similar) to avoid the ugly warning:

/usr/include/features.h:148:3: warning: "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE"
      [-W#warnings]

Patch:

diff --git i/fmacros.h w/fmacros.h
index 9e5fec0..1beee89 100644
--- i/fmacros.h
+++ w/fmacros.h
@@ -3,6 +3,7 @@

 #if !defined(_BSD_SOURCE)
 #define _BSD_SOURCE
+#define _DEFAULT_SOURCE
 #endif

 #if defined(__sun__)

I'm not sure wether or not we still need the _BSD_SOURCE to work just fine with older compilers.

[badboy]

Better:

diff --git i/fmacros.h w/fmacros.h
index 9e5fec0..19d7b21 100644
--- i/fmacros.h
+++ w/fmacros.h
@@ -1,8 +1,9 @@
 #ifndef __HIREDIS_FMACRO_H
 #define __HIREDIS_FMACRO_H

-#if !defined(_BSD_SOURCE)
+#if defined(__linux__)
 #define _BSD_SOURCE
+#define _DEFAULT_SOURCE
 #endif

 #if defined(__sun__)

Similar to how Redis does it: redis/redis@af45364

[mattsta]

Any more requests before develop gets promoted to master in a few days?

[mattsta]

Creating new version tomorrow. Looking at the release page, the last proper release was over two years ago. Whoops.

[badboy]

All good things are worth waiting for. (or in German: Was lange währt, wird endlich gut)

[mattsta]

0.12.0 is done as of https://github.com/redis/hiredis/tree/7b51834c165133cf6816b1991920a55bdc64ea78

We can add any bug fixes in 0.12.x versions and the next major release will be 1.0.0.

[mattsta]

oh, and big thanks to @michael-grunder and @badboy for doing all the real work of figuring out exactly what to add/fix and how to add/fix things out of the hiredis issues/PR backlog. :)

[pietern]

Nice! Great work.

[mattsta]

At this point it's mostly open source gardening. You did all the real great work years ago. :)

[badboy]

I'm happy to contribute here. Again thanks to all of you.