-
Notifications
You must be signed in to change notification settings - Fork 486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removing Redis from internal lua function names and comments #288
base: unstable
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -99,7 +99,7 @@ struct ldbState { | |||||
int bp[LDB_BREAKPOINTS_MAX]; /* An array of breakpoints line numbers. */ | ||||||
int bpcount; /* Number of valid entries inside bp. */ | ||||||
int step; /* Stop at next line regardless of breakpoints. */ | ||||||
int luabp; /* Stop at next line because redis.breakpoint() was called. */ | ||||||
int luabp; /* Stop at next line because server.breakpoint() was called. */ | ||||||
sds *src; /* Lua script source code split by line. */ | ||||||
int lines; /* Number of lines in 'src'. */ | ||||||
int currentline; /* Current line number. */ | ||||||
|
@@ -114,7 +114,7 @@ struct ldbState { | |||||
|
||||||
/* Perform the SHA1 of the input string. We use this both for hashing script | ||||||
* bodies in order to obtain the Lua function name, and in the implementation | ||||||
* of redis.sha1(). | ||||||
* of server.sha1(). | ||||||
* | ||||||
* 'digest' should point to a 41 bytes buffer: 40 for SHA1 converted into an | ||||||
* hexadecimal number, plus 1 byte for null term. */ | ||||||
|
@@ -135,12 +135,12 @@ void sha1hex(char *digest, char *script, size_t len) { | |||||
digest[40] = '\0'; | ||||||
} | ||||||
|
||||||
/* redis.breakpoint() | ||||||
/* Adds server.breakpoint() function used by lua debugger. | ||||||
* | ||||||
* Allows to stop execution during a debugging session from within | ||||||
* the Lua code implementation, like if a breakpoint was set in the code | ||||||
* immediately after the function. */ | ||||||
int luaRedisBreakpointCommand(lua_State *lua) { | ||||||
int luaServerBreakpointCommand(lua_State *lua) { | ||||||
if (ldb.active) { | ||||||
ldb.luabp = 1; | ||||||
lua_pushboolean(lua,1); | ||||||
|
@@ -150,12 +150,12 @@ int luaRedisBreakpointCommand(lua_State *lua) { | |||||
return 1; | ||||||
} | ||||||
|
||||||
/* redis.debug() | ||||||
/* Adds server.debug() function used by lua debugger | ||||||
* | ||||||
* Log a string message into the output console. | ||||||
* Can take multiple arguments that will be separated by commas. | ||||||
* Nothing is returned to the caller. */ | ||||||
int luaRedisDebugCommand(lua_State *lua) { | ||||||
int luaServerDebugCommand(lua_State *lua) { | ||||||
if (!ldb.active) return 0; | ||||||
int argc = lua_gettop(lua); | ||||||
sds log = sdscatprintf(sdsempty(),"<debug> line %d: ", ldb.currentline); | ||||||
|
@@ -167,14 +167,14 @@ int luaRedisDebugCommand(lua_State *lua) { | |||||
return 0; | ||||||
} | ||||||
|
||||||
/* redis.replicate_commands() | ||||||
/* Adds server.replicate_commands() | ||||||
* | ||||||
* DEPRECATED: Now do nothing and always return true. | ||||||
* Turn on single commands replication if the script never called | ||||||
* a write command so far, and returns true. Otherwise if the script | ||||||
* already started to write, returns false and stick to whole scripts | ||||||
* replication, which is our default. */ | ||||||
int luaRedisReplicateCommandsCommand(lua_State *lua) { | ||||||
int luaServerReplicateCommandsCommand(lua_State *lua) { | ||||||
lua_pushboolean(lua,1); | ||||||
return 1; | ||||||
} | ||||||
|
@@ -205,27 +205,27 @@ void scriptingInit(int setup) { | |||||
lctx.lua_scripts_lru_list = listCreate(); | ||||||
lctx.lua_scripts_mem = 0; | ||||||
|
||||||
luaRegisterRedisAPI(lua); | ||||||
luaRegisterServerAPI(lua); | ||||||
|
||||||
/* register debug commands */ | ||||||
lua_getglobal(lua,"redis"); | ||||||
/* register debug commands. we only need to add it under 'server' as 'redis' is effectively aliased to 'server' table at this point. */ | ||||||
lua_getglobal(lua,"server"); | ||||||
|
||||||
/* redis.breakpoint */ | ||||||
/* server.breakpoint */ | ||||||
lua_pushstring(lua,"breakpoint"); | ||||||
lua_pushcfunction(lua,luaRedisBreakpointCommand); | ||||||
lua_pushcfunction(lua,luaServerBreakpointCommand); | ||||||
lua_settable(lua,-3); | ||||||
|
||||||
/* redis.debug */ | ||||||
/* server.debug */ | ||||||
lua_pushstring(lua,"debug"); | ||||||
lua_pushcfunction(lua,luaRedisDebugCommand); | ||||||
lua_pushcfunction(lua,luaServerDebugCommand); | ||||||
lua_settable(lua,-3); | ||||||
|
||||||
/* redis.replicate_commands */ | ||||||
/* server.replicate_commands */ | ||||||
lua_pushstring(lua, "replicate_commands"); | ||||||
lua_pushcfunction(lua, luaRedisReplicateCommandsCommand); | ||||||
lua_pushcfunction(lua, luaServerReplicateCommandsCommand); | ||||||
lua_settable(lua, -3); | ||||||
|
||||||
lua_setglobal(lua,"redis"); | ||||||
lua_setglobal(lua,"server"); | ||||||
|
||||||
/* Add a helper function we use for pcall error reporting. | ||||||
* Note that when the error is in the C function we want to report the | ||||||
|
@@ -234,7 +234,7 @@ void scriptingInit(int setup) { | |||||
{ | ||||||
char *errh_func = "local dbg = debug\n" | ||||||
"debug = nil\n" | ||||||
"function __redis__err__handler(err)\n" | ||||||
"function __server__err__handler(err)\n" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest aliasing __server__err__handler to __redis__err__handler to address Madelyn's compat concern. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in next commit. |
||||||
" local i = dbg.getinfo(2,'nSl')\n" | ||||||
" if i and i.what == 'C' then\n" | ||||||
" i = dbg.getinfo(3,'nSl')\n" | ||||||
|
@@ -583,7 +583,7 @@ void evalGenericCommand(client *c, int evalsha) { | |||||
evalCalcFunctionName(evalsha, c->argv[1]->ptr, funcname); | ||||||
|
||||||
/* Push the pcall error handler function on the stack. */ | ||||||
lua_getglobal(lua, "__redis__err__handler"); | ||||||
lua_getglobal(lua, "__server__err__handler"); | ||||||
|
||||||
/* Try to lookup the Lua function */ | ||||||
lua_getfield(lua, LUA_REGISTRYINDEX, funcname); | ||||||
|
@@ -1375,7 +1375,7 @@ char *ldbRedisProtocolToHuman_Double(sds *o, char *reply) { | |||||
/* Log a RESP reply as debugger output, in a human readable format. | ||||||
* If the resulting string is longer than 'len' plus a few more chars | ||||||
* used as prefix, it gets truncated. */ | ||||||
void ldbLogRedisReply(char *reply) { | ||||||
void ldbLogServerReply(char *reply) { | ||||||
sds log = sdsnew("<reply> "); | ||||||
ldbRedisProtocolToHuman(&log,reply); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are still a few "Redis" references in this file. Can we fix them all at once?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we decided to keep references to RedisProtocol? Are we renaming the protocol from RESP to something else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The protocol is not called "server protocol". It doesn't say much. I suggested earlier (another PR) that we keep it as RedisProtocol here. If we insist on changing it, we can change Redis to RESP here, i.e. name the function to ldbRespToHuman or ldbRespProtocolToHuman. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I would vote updating it to RESP, which feels more accurate. |
||||||
ldbLogWithMaxLen(log); | ||||||
|
@@ -1516,30 +1516,30 @@ void ldbEval(lua_State *lua, sds *argv, int argc) { | |||||
} | ||||||
|
||||||
/* Implement the debugger "server" command. We use a trick in order to make | ||||||
* the implementation very simple: we just call the Lua redis.call() command | ||||||
* the implementation very simple: we just call the Lua server.call() command | ||||||
* implementation, with ldb.step enabled, so as a side effect the command | ||||||
* and its reply are logged. */ | ||||||
void ldbRedis(lua_State *lua, sds *argv, int argc) { | ||||||
void ldbServer(lua_State *lua, sds *argv, int argc) { | ||||||
int j; | ||||||
|
||||||
if (!lua_checkstack(lua, argc + 1)) { | ||||||
/* Increase the Lua stack if needed to make sure there is enough room | ||||||
* to push 'argc + 1' elements to the stack. On failure, return error. | ||||||
* Notice that we need, in worst case, 'argc + 1' elements because we push all the arguments | ||||||
* given by the user (without the first argument) and we also push the 'redis' global table and | ||||||
* 'redis.call' function so: | ||||||
* (1 (redis table)) + (1 (redis.call function)) + (argc - 1 (all arguments without the first)) = argc + 1*/ | ||||||
ldbLogRedisReply("max lua stack reached"); | ||||||
* given by the user (without the first argument) and we also push the 'server' global table and | ||||||
* 'server.call' function so: | ||||||
* (1 (server table)) + (1 (server.call function)) + (argc - 1 (all arguments without the first)) = argc + 1*/ | ||||||
ldbLogServerReply("max lua stack reached"); | ||||||
return; | ||||||
} | ||||||
|
||||||
lua_getglobal(lua,"redis"); | ||||||
lua_getglobal(lua,"server"); | ||||||
lua_pushstring(lua,"call"); | ||||||
lua_gettable(lua,-2); /* Stack: redis, redis.call */ | ||||||
lua_gettable(lua,-2); /* Stack: server, server.call */ | ||||||
for (j = 1; j < argc; j++) | ||||||
lua_pushlstring(lua,argv[j],sdslen(argv[j])); | ||||||
ldb.step = 1; /* Force redis.call() to log. */ | ||||||
lua_pcall(lua,argc-1,1,0); /* Stack: redis, result */ | ||||||
ldb.step = 1; /* Force server.call() to log. */ | ||||||
lua_pcall(lua,argc-1,1,0); /* Stack: server, result */ | ||||||
ldb.step = 0; /* Disable logging. */ | ||||||
lua_pop(lua,2); /* Discard the result and clean the stack. */ | ||||||
} | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is closer to how it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have 120 line length, and I think this barely spill over that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I was using Vim. I need to set this up in IntelliJ, it automatically splits larger lines. I will fix it for this PR manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not start a war about this but I believe antirez style is 80 chars width. That's also the standard for terminals. (Open up one a new terminal window you'll see.)
For multi-line comments I think it's nice to keep it around 80. It's easier to read. For long code lines, we can have a soft limit around 120 imo. (Let's not start a war about it though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(In emacs press Alt-Q inside a comment to reflow it.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. The style the old Redis maintainers used was 120 max length, soft limit around 100, 80 encouraged for multi-line paragraphs with a lot of breaks. The only hard limit was 120.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-format is still on my to-do list. After the "rebranding" work (which pretty much makes the code history discussion moot), I think we should just get out of the style discussion altogether and let the "machine" do the work automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a nitpick. This comment "we only need to add it under 'server' as 'redis' is effectively aliased to 'server' table at this point" seems to be more relevant to this PR than something we like to keep in the code for a longer time. The reference to the "server" table is pretty clear to me already. The aliasing of "server" to "redis" is a hack that's better be kept in the smallest possible scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PingXie I did not understand your suggestion. Do you want to modify the comment or remove it altogether? I like to have documentation around any code - temporary or not. We can remove the documentation when we remove the code - in this case 'redis' table from lua.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I meant to say that we don't have to mention that "redis" and "server" are synonymous since that is covered in other part of the code already (like when you set up the "redis" alias for the "server" table).