Standardizes the 'help' subcommand

This adds a new `addReplyHelp` helper that's used by commands
when returning a help text. The following commands have been
touched: DEBUG, OBJECT, COMMAND, PUBSUB, SCRIPT and SLOWLOG.

WIP

Fix entry command table entry for OBJECT for HELP option.

After #4472 the command may have just 2 arguments.

Improve OBJECT HELP descriptions.

See #4472.

WIP 2

WIP 3

src/debug.c

@@ -267,48 +267,29 @@ void debugCommand(client *c) {
         return;
     }
 
-    if (!strcasecmp(c->argv[1]->ptr,"help")) {
+    if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"help")) {
-        void *blenp = addDeferredMultiBulkLength(c);
+        const char *help[] = {
-        int blen = 0;
+            "assert -- Crash by assertion failed.",
-        blen++; addReplyStatus(c,
+            "crash-and-recovery <milliseconds> -- Hard crash and restart after <milliseconds> delay.",
-        "DEBUG <subcommand> arg arg ... arg. Subcommands:");
+            "digest -- Outputs an hex signature representing the current DB content.",
-        blen++; addReplyStatus(c,
+            "htstats <dbid> -- Return hash table statistics of the specified Redis database.",
-        "segfault -- Crash the server with sigsegv.");
+            "loadaof -- Flush the AOF buffers on disk and reload the AOF in memory.",
-        blen++; addReplyStatus(c,
+            "lua-always-replicate-commands (0|1) -- Setting it to 1 makes Lua replication defaulting to replicating single commands, without the script having to enable effects replication.",
-        "panic -- Crash the server simulating a panic.");
+            "object <key> -- Show low level info about key and associated value.",
-        blen++; addReplyStatus(c,
+            "panic -- Crash the server simulating a panic.",
-        "restart  -- Graceful restart: save config, db, restart.");
+            "populate <count> [prefix] [size] -- Create <count> string keys named key:<num>. If a prefix is specified is used instead of the 'key' prefix.",
-        blen++; addReplyStatus(c,
+            "reload -- Save the RDB on disk and reload it back in memory.",
-        "crash-and-recovery <milliseconds> -- Hard crash and restart after <milliseconds> delay.");
+            "restart -- Graceful restart: save config, db, restart.",
-        blen++; addReplyStatus(c,
+            "sdslen <key> -- Show low level SDS string info representing key and value.",
-        "assert   -- Crash by assertion failed.");
+            "segfault -- Crash the server with sigsegv.",
-        blen++; addReplyStatus(c,
+            "set-active-expire (0|1) -- Setting it to 0 disables expiring keys in background when they are not accessed (otherwise the Redis behavior). Setting it to 1 reenables back the default.",
-        "reload   -- Save the RDB on disk and reload it back in memory.");
+            "sleep <seconds> -- Stop the server for <seconds>. Decimals allowed.",
-        blen++; addReplyStatus(c,
+            "structsize -- Return the size of different Redis core C structures.",
-        "loadaof  -- Flush the AOF buffers on disk and reload the AOF in memory.");
+            "ziplist <key> -- Show low level info about the ziplist encoding.",
-        blen++; addReplyStatus(c,
+            "error <string> -- Return a Redis protocol error with <string> as message. Useful for clients unit tests to simulate Redis errors.",
-        "object <key> -- Show low level info about key and associated value.");
+            NULL
-        blen++; addReplyStatus(c,
+        };
-        "sdslen <key> -- Show low level SDS string info representing key and value.");
+        addReplyHelp(c, help);
-        blen++; addReplyStatus(c,
-        "ziplist <key> -- Show low level info about the ziplist encoding.");
-        blen++; addReplyStatus(c,
-        "populate <count> [prefix] [size] -- Create <count> string keys named key:<num>. If a prefix is specified is used instead of the 'key' prefix.");
-        blen++; addReplyStatus(c,
-        "digest   -- Outputs an hex signature representing the current DB content.");
-        blen++; addReplyStatus(c,
-        "sleep <seconds> -- Stop the server for <seconds>. Decimals allowed.");
-        blen++; addReplyStatus(c,
-        "set-active-expire (0|1) -- Setting it to 0 disables expiring keys in background when they are not accessed (otherwise the Redis behavior). Setting it to 1 reenables back the default.");
-        blen++; addReplyStatus(c,
-        "lua-always-replicate-commands (0|1) -- Setting it to 1 makes Lua replication defaulting to replicating single commands, without the script having to enable effects replication.");
-        blen++; addReplyStatus(c,
-        "error <string> -- Return a Redis protocol error with <string> as message. Useful for clients unit tests to simulate Redis errors.");
-        blen++; addReplyStatus(c,
-        "structsize -- Return the size of different Redis core C structures.");
-        blen++; addReplyStatus(c,
-        "htstats <dbid> -- Return hash table statistics of the specified Redis database.");
-        setDeferredMultiBulkLength(c,blenp,blen);
     } else if (!strcasecmp(c->argv[1]->ptr,"segfault")) {
         *((char*)-1) = 'x';
     } else if (!strcasecmp(c->argv[1]->ptr,"panic")) {
@@ -550,8 +531,9 @@ void debugCommand(client *c) {
 
         addReplyBulkSds(c,stats);
     } else {
-        addReplyErrorFormat(c, "Unknown DEBUG subcommand or wrong number of arguments for '%s'",
+        addReplyErrorFormat(c, "Unknown subcommand or wrong number of arguments for '%s'. Try DEBUG help",
             (char*)c->argv[1]->ptr);
+        return;
     }
 }
 

src/networking.c

@@ -584,6 +584,30 @@ void addReplyBulkLongLong(client *c, long long ll) {
     addReplyBulkCBuffer(c,buf,len);
 }
 
+/* Add an array of strings as a bulk reply with a heading.
+ * This function is typically invoked by from commands that support
+ * subcommands in response to the 'help' subcommand. The help array
+ * is terminated by NULL sentinel. */
+void addReplyHelp(client *c, const char **help) {
+    sds cmd = sdsnew((char*) c->argv[0]->ptr);
+    void *blenp = addDeferredMultiBulkLength(c);
+    int blen = 0;
+    int hlen = 0;
+
+    sdstoupper(cmd);
+    addReplyStatusFormat(c,
+        "%s <subcommand> arg arg ... arg. Subcommands are:",cmd);
+    blen++;
+    sdsfree(cmd);
+    
+    while (help[hlen]) {
+        addReplyStatus(c,help[hlen++]);
+        blen++;
+    }
+
+    setDeferredMultiBulkLength(c,blenp,blen);
+}
+
 /* Copy 'src' client output buffers into 'dst' client output buffers.
  * The function takes care of freeing the old output buffers of the
  * destination client. */

src/object.c

@@ -1016,20 +1016,15 @@ robj *objectCommandLookupOrReply(client *c, robj *key, robj *reply) {
 void objectCommand(client *c) {
     robj *o;
 
-    if (!strcasecmp(c->argv[1]->ptr,"help") && c->argc == 2) {
+    if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"help")) {
-        void *blenp = addDeferredMultiBulkLength(c);
+        const char *help[] = {
-        int blen = 0;
+            "encoding <key> -- Return the kind of internal representation used in order to store the value associated with a key.",
-        blen++; addReplyStatus(c,
+            "freq <key> -- Return the access frequency index of the key. The returned integer is proportional to the logarithm of the recent access frequency of the key.",
-        "OBJECT <subcommand> key. Subcommands:");
+            "idletime <key> -- Return the idle time of the key, that is the approximated number of seconds elapsed since the last access to the key.",
-        blen++; addReplyStatus(c,
+            "refcount <key> -- Return the number of references of the value associated with the specified key.",
-        "refcount -- Return the number of references of the value associated with the specified key.");
+            NULL
-        blen++; addReplyStatus(c,
+        };
-        "encoding -- Return the kind of internal representation used in order to store the value associated with a key.");
+        addReplyHelp(c, help);
-        blen++; addReplyStatus(c,
-        "idletime -- Return the number of seconds since the object stored at the specified key is idle.");
-        blen++; addReplyStatus(c,
-        "freq -- Return the inverse logarithmic access frequency counter of the object stored at the specified key.");
-        setDeferredMultiBulkLength(c,blenp,blen);
     } else if (!strcasecmp(c->argv[1]->ptr,"refcount") && c->argc == 3) {
         if ((o = objectCommandLookupOrReply(c,c->argv[2],shared.nullbulk))
                 == NULL) return;
@@ -1057,6 +1052,7 @@ void objectCommand(client *c) {
     } else {
         addReplyErrorFormat(c, "Unknown subcommand or wrong number of arguments for '%s'. Try OBJECT help",
             (char *)c->argv[1]->ptr);
+        return;
     }
 }
 

src/pubsub.c

@@ -325,8 +325,16 @@ void publishCommand(client *c) {
 
 /* PUBSUB command for Pub/Sub introspection. */
 void pubsubCommand(client *c) {
-    if (!strcasecmp(c->argv[1]->ptr,"channels") &&
+    if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"help")) {
-        (c->argc == 2 || c->argc ==3))
+        const char *help[] = {
+            "channels [<pattern>] -- Return the currently active channels matching a pattern (default: all).",
+            "numpat -- Return number of subscriptions to patterns.",
+            "numsub [channel-1 .. channel-N] -- Returns the number of subscribers for the specified channels (excluding patterns, default: none).",
+            NULL
+        };
+        addReplyHelp(c, help);
+    } else if (!strcasecmp(c->argv[1]->ptr,"channels") &&
+        (c->argc == 2 || c->argc == 3))
     {
         /* PUBSUB CHANNELS [<pattern>] */
         sds pat = (c->argc == 2) ? NULL : c->argv[2]->ptr;
@@ -364,8 +372,8 @@ void pubsubCommand(client *c) {
         /* PUBSUB NUMPAT */
         addReplyLongLong(c,listLength(server.pubsub_patterns));
     } else {
-        addReplyErrorFormat(c,
+        addReplyErrorFormat(c, "Unknown subcommand or wrong number of arguments for '%s'. Try PUBSUB help",
-            "Unknown PUBSUB subcommand or wrong number of arguments for '%s'",
             (char*)c->argv[1]->ptr);
+        return;
     }
 }

src/scripting.c

@@ -1430,7 +1430,17 @@ void evalShaCommand(client *c) {
 }
 
 void scriptCommand(client *c) {
-    if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"flush")) {
+    if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"help")) {
+        const char *help[] = {
+            "debug (yes|sync|no) -- Set the debug mode for subsequent scripts executed.",
+            "exists sha1 [sha1 ...] -- Return information about the existence of the scripts in the script cache.",
+            "flush -- Flush the Lua scripts cache.",
+            "kill -- Kill the currently executing Lua script.",
+            "load script -- Load a script into the scripts cache, without executing it.",
+            NULL
+        };
+        addReplyHelp(c, help);
+    } else if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"flush")) {
         scriptingReset();
         addReply(c,shared.ok);
         replicationScriptCacheFlush();
@@ -1489,9 +1499,12 @@ void scriptCommand(client *c) {
             c->flags |= CLIENT_LUA_DEBUG_SYNC;
         } else {
             addReplyError(c,"Use SCRIPT DEBUG yes/sync/no");
+            return;
         }
     } else {
-        addReplyError(c, "Unknown SCRIPT subcommand or wrong # of args.");
+        addReplyErrorFormat(c, "Unknown subcommand or wrong number of arguments for '%s'. Try SCRIPT help",
+            (char*)c->argv[1]->ptr);
+        return;
     }
 }
 

src/server.c

@@ -276,7 +276,7 @@ struct redisCommand redisCommandTable[] = {
     {"readonly",readonlyCommand,1,"F",0,NULL,0,0,0,0,0},
     {"readwrite",readwriteCommand,1,"F",0,NULL,0,0,0,0,0},
     {"dump",dumpCommand,2,"r",0,NULL,1,1,1,0,0},
-    {"object",objectCommand,3,"r",0,NULL,2,2,2,0,0},
+    {"object",objectCommand,-2,"r",0,NULL,2,2,2,0,0},
     {"memory",memoryCommand,-2,"r",0,NULL,0,0,0,0,0},
     {"client",clientCommand,-2,"as",0,NULL,0,0,0,0,0},
     {"eval",evalCommand,-3,"s",0,evalGetKeys,0,0,0,0,0},
@@ -2733,7 +2733,16 @@ void commandCommand(client *c) {
     dictIterator *di;
     dictEntry *de;
 
-    if (c->argc == 1) {
+    if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"help")) {
+        const char *help[] = {
+            "(no subcommand) -- Return details about all Redis commands.",
+            "count -- Return the total number of commands in this Redis server.",
+            "getkeys <full-command> -- Return the keys from a full Redis command.",
+            "info [command-name ...] -- Return details about multiple Redis commands.",
+            NULL
+        };
+        addReplyHelp(c, help);
+    } else if (c->argc == 1) {
         addReplyMultiBulkLen(c, dictSize(server.commands));
         di = dictGetIterator(server.commands);
         while ((de = dictNext(di)) != NULL) {
@@ -2767,7 +2776,8 @@ void commandCommand(client *c) {
         for (j = 0; j < numkeys; j++) addReplyBulk(c,c->argv[keys[j]+2]);
         getKeysFreeResult(keys);
     } else {
-        addReplyError(c, "Unknown subcommand or wrong number of arguments.");
+        addReplyErrorFormat(c, "Unknown subcommand or wrong number of arguments for '%s'. Try COMMAND help",
+            (char*)c->argv[1]->ptr);
         return;
     }
 }

src/server.h

@@ -1357,6 +1357,7 @@ void addReplyDouble(client *c, double d);
 void addReplyHumanLongDouble(client *c, long double d);
 void addReplyLongLong(client *c, long long ll);
 void addReplyMultiBulkLen(client *c, long length);
+void addReplyHelp(client *c, const char **help);
 void copyClientOutputBuffer(client *dst, client *src);
 size_t sdsZmallocSize(sds s);
 size_t getStringObjectSdsUsedMemory(robj *o);

src/slowlog.c

@@ -140,7 +140,15 @@ void slowlogReset(void) {
 /* The SLOWLOG command. Implements all the subcommands needed to handle the
  * Redis slow log. */
 void slowlogCommand(client *c) {
-    if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"reset")) {
+    if (!strcasecmp(c->argv[1]->ptr,"help") && c->argc == 2) {
+        const char *help[] = {
+            "get [count] -- Return the top entries from the slowlog (default: 10).",
+            "len -- Return the length of the slowlog.",
+            "reset -- Reset the slowlog.",
+            NULL
+        };
+        addReplyHelp(c, help);
+    } else if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"reset")) {
         slowlogReset();
         addReply(c,shared.ok);
     } else if (c->argc == 2 && !strcasecmp(c->argv[1]->ptr,"len")) {
@@ -177,7 +185,8 @@ void slowlogCommand(client *c) {
         }
         setDeferredMultiBulkLength(c,totentries,sent);
     } else {
-        addReplyError(c,
+         addReplyErrorFormat(c, "Unknown subcommand or wrong number of arguments for '%s'. Try SLOWLOG help",
-            "Unknown SLOWLOG subcommand or wrong # of args. Try GET, RESET, LEN.");
+            (char*)c->argv[1]->ptr);
+        return;
     }
 }