degesch: thorough review, no functional changes

degesch.c

@@ -1,7 +1,7 @@
 /*
  * degesch.c: the experimental IRC client
  *
- * Copyright (c) 2015 - 2017, Přemysl Janouch <p.janouch@gmail.com>
+ * Copyright (c) 2015 - 2018, Přemysl Janouch <p.janouch@gmail.com>
  *
  * Permission to use, copy, modify, and/or distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -2042,7 +2042,6 @@ struct app_context
 	struct buffer *current_buffer;      ///< The current buffer
 	struct buffer *last_buffer;         ///< Last used buffer
 
-	// TODO: make buffer names fully unique like weechat does
 	struct str_map buffers_by_name;     ///< Buffers by name
 
 	unsigned backlog_limit;             ///< Limit for buffer lines
@@ -2096,6 +2095,7 @@ filter_color_cube_for_acceptable_nick_colors (size_t *len)
 	size_t len_counter = 0;
 	for (int x = 0; x < 6 * 6 * 6; x++)
 	{
+		// FIXME this isn't exactly right, the values aren't linear
 		int r =  x / 36;
 		int g = (x / 6) % 6;
 		int b = (x % 6);
@@ -3122,6 +3122,8 @@ static const int g_mirc_to_terminal[] =
 	[MIRC_L_GRAY]   = COLOR_256         (WHITE,   252),
 };
 
+// TODO: support more colors, see https://modern.ircdocs.horse/formatting.html
+//   + http://anti.teamidiot.de/static/nei/*/extended_mirc_color_proposal.html
 static const char *
 formatter_parse_mirc_color (struct formatter *self, const char *s)
 {
@@ -3778,7 +3780,7 @@ static void
 buffer_line_write_time (struct formatter *f, struct buffer_line *line,
 	FILE *stream, int flush_opts)
 {
-	// Normal timestamps don't include the date, this way the user won't be
+	// Normal timestamps don't include the date, make sure the user won't be
 	// confused as to when an event has happened
 	buffer_update_time (f->ctx, line->when, stream, flush_opts);
 
@@ -4003,7 +4005,6 @@ buffer_get_log_path (struct buffer *buffer)
 
 	(void) mkdir_with_parents (path.str, NULL);
 
-	// TODO: make sure global and server buffers don't collide with filenames
 	str_append_c (&path, '/');
 	make_log_filename (buffer->name, &path);
 	str_append (&path, ".log");
@@ -4016,6 +4017,7 @@ buffer_open_log_file (struct app_context *ctx, struct buffer *buffer)
 	if (!ctx->logging || buffer->log_file)
 		return;
 
+	// TODO: should we try to reopen files wrt. case mapping?
 	char *path = buffer_get_log_path (buffer);
 	if (!(buffer->log_file = fopen (path, "ab")))
 		log_global_error (ctx, "Couldn't open log file `#s': #l",
@@ -4064,7 +4066,8 @@ buffer_add (struct app_context *ctx, struct buffer *buffer)
 
 	buffer_open_log_file (ctx, buffer);
 
-	// In theory this can't cause changes in the prompt
+	// Normally this doesn't cause changes in the prompt but a prompt hook
+	// could decide to show some information for all buffers nonetheless
 	refresh_prompt (ctx);
 }
 
@@ -4140,7 +4143,7 @@ buffer_print_backlog (struct app_context *ctx, struct buffer *buffer)
 		free (buffer_name_localized);
 	}
 
-	// That is, minus the readline prompt
+	// That is, minus the readline prompt (taking at least one line)
 	int display_limit = MAX (10, g_terminal.lines - 1);
 	int to_display = 0;
 
@@ -4176,6 +4179,9 @@ buffer_activate (struct app_context *ctx, struct buffer *buffer)
 {
 	if (ctx->current_buffer == buffer)
 		return;
+
+	// This is the only place where the unread messages marker
+	// and highlight indicator are reset
 	if (ctx->current_buffer)
 	{
 		ctx->current_buffer->new_messages_count = 0;
@@ -5025,6 +5031,7 @@ irc_process_hooks (struct server *s, char *input)
 			return NULL;
 		}
 
+		// The old input may get freed, so we compare against a hash of it
 		uint64_t new_hash = siphash_wrapper (input, strlen (input));
 		if (new_hash != hash)
 			log_server_debug (s, "#a>= \"#S\"#r", ATTR_JOIN, input);
@@ -5067,7 +5074,9 @@ irc_try_read (struct server *s)
 	enum socket_io_result result = s->transport->try_read (s);
 	if (s->read_buffer.len >= (1 << 20))
 	{
-		// XXX: this is stupid; if anything, count it in dependence of time
+		// XXX: this is stupid; if anything, count it in dependence of time;
+		//   we could make transport_tls_try_read() limit the immediate amount
+		//   of data read like socket_io_try_read() does and remove this check
 		log_server_error (s, s->buffer,
 			"The IRC server seems to spew out data frantically");
 		return SOCKET_IO_ERROR;
@@ -5302,7 +5311,7 @@ transport_tls_init_ctx (struct server *s, SSL_CTX *ssl_ctx, struct error **e)
 			return false;
 		}
 
-		// Only inform the user if we're not actually verifying
+		// Just inform the user if we're not actually verifying
 		log_server_error (s, s->buffer, "#s", error->message);
 		error_free (error);
 	}
@@ -5363,7 +5372,7 @@ transport_tls_init (struct server *s, const char *hostname, struct error **e)
 
 	// Enable SNI, FWIW; literal IP addresses aren't allowed
 	struct in6_addr dummy;
-	if (!inet_pton (AF_INET, hostname, &dummy)
+	if (!inet_pton (AF_INET,  hostname, &dummy)
 	 && !inet_pton (AF_INET6, hostname, &dummy))
 		SSL_set_tlsext_host_name (ssl, hostname);
 
@@ -6035,7 +6044,7 @@ irc_is_highlight (struct server *s, const char *message)
 
 	// Special characters allowed in nicknames by RFC 2812: []\`_^{|} and -
 	// Also excluded from the ASCII: common user channel prefixes: +%@&~
-	// XXX: why did I exclude those? It won't match when newbies use them.
+	// XXX: why did I exclude those?  It won't match when IRC newbies use them.
 	const char *delimiters = ",.;:!?()<>/=#$* \t\r\n\v\f\"'";
 
 	bool result = false;
@@ -6329,6 +6338,7 @@ irc_handle_sent_privmsg (struct server *s, const struct irc_message *msg)
 		return;
 
 	// This ignores empty messages which we should not normally send
+	// and the server is likely going to reject with an error reply anyway
 	struct ctcp_chunk *chunks = ctcp_parse (msg->params.vector[1]);
 	LIST_FOR_EACH (struct ctcp_chunk, iter, chunks)
 	{
@@ -6367,6 +6377,7 @@ static void
 irc_process_sent_message (const struct irc_message *msg, struct server *s)
 {
 	// The server is free to reject even a matching prefix
+	// XXX: even though no prefix should normally be present, this is racy
 	if (msg->prefix && !irc_is_this_us (s, msg->prefix))
 		return;
 
@@ -7135,6 +7146,10 @@ irc_on_registered (struct server *s, const char *nickname)
 	refresh_prompt (s->ctx);
 
 	// XXX: we can also use WHOIS if it's not supported (optional by RFC 2812)
+	// TODO: maybe rather always use RPL_ISUPPORT NICKLEN & USERLEN & HOSTLEN
+	//   since we don't seem to follow any subsequent changes in userhost;
+	//   unrealircd sends RPL_HOSTHIDDEN (396), which has an optional user part,
+	//   and there is also CAP CHGHOST which /may/ send it to ourselves
 	irc_send (s, "USERHOST %s", s->irc_user->nickname);
 
 	// A little hack that reinstates auto-away status when we get disconnected
@@ -7225,7 +7240,7 @@ irc_handle_rpl_namreply (struct server *s, const struct irc_message *msg)
 
 struct channel_user_sort_entry
 {
-	struct server *s;                   ///< Server
+	struct server *s;                   ///< Server (because of the qsort API)
 	struct channel_user *channel_user;  ///< Channel user
 };
 
@@ -8182,7 +8197,10 @@ server_rename (struct app_context *ctx, struct server *s, const char *new_name)
 	struct buffer *buffer;
 	while ((buffer = str_map_iter_next (&iter)))
 	{
-		// FIXME: creation of buffer names should be centralized
+		// TODO: creation of buffer names should be centralized -> replace
+		//   calls to buffer_rename() and manual setting of buffer names
+		//   with something like buffer_autorename() -- just mind the mess
+		//   in irc_handle_nick(), which can hopefully be simplified
 		char *x = NULL;
 		switch (buffer->type)
 		{
@@ -8387,6 +8405,7 @@ lua_plugin_log_error
 	error_free (error);
 }
 
+/// Pop "n" values from the stack into a table, using their indexes as keys
 static void
 lua_plugin_pack (lua_State *L, int n)
 {
@@ -8541,7 +8560,7 @@ LUA_WEAK_DECLARE (channel, XLUA_CHANNEL_METATABLE)
 LUA_WEAK_DECLARE (buffer,  XLUA_BUFFER_METATABLE)
 LUA_WEAK_DECLARE (server,  XLUA_SERVER_METATABLE)
 
-// The global context is kind of fake and don't have any ref-counting,
+// The global context is kind of fake and doesn't have any ref-counting,
 // however it's still very much an object
 static struct lua_weak_info lua_ctx_info =
 {
@@ -9332,8 +9351,13 @@ lua_connection_close (lua_State *L)
 	if (self->socket_fd != -1)
 	{
 		self->closing = true;
+		// NOTE: this seems to do nothing on Linux
 		(void) shutdown (self->socket_fd, SHUT_RD);
 
+		// Right now we want to wait until all data is flushed to the socket
+		// and can't call close() here immediately -- a rewrite to use async
+		// would enable the user to await on either :send() or :flush();
+		// a successful send() doesn't necessarily mean anything though
 		if (!self->write_buffer.len)
 			lua_connection_discard (self);
 	}
@@ -9413,6 +9437,8 @@ static bool
 lua_connection_invoke_on_error (struct lua_connection *self,
 	const char *error, struct error **e)
 {
+	// XXX: not sure if ignoring errors after :close() is always desired;
+	//   code might want to make sure that data are transferred successfully
 	if (!self->closing
 	 && lua_connection_cb_lookup (self, "on_error", e)
 	 && !lua_connection_eat_nil (self))
@@ -9791,8 +9817,13 @@ lua_wait_dial_check (struct lua_wait_channel *wchannel)
 	lua_State *L = self->super.task->thread;
 	if (self->connection)
 	{
-		// FIXME: this way the connection can leak, it shouldn't stay in cache
+		// FIXME: this way the connection may leak -- we pass the value to the
-		//   automatically all the time but clean itself up on GC
+		//   task manager on the stack and forget about it but still leave the
+		//   connection in the cache.  That is because right now, when Lua code
+		//   sets up callbacks in the connection object and returns, it might
+		//   get otherwise GC'd since nothing else keeps referencing it.
+		//   By rewriting lua_connection using async, tasks and wait channels
+		//   would hold a reference, allowing us to remove it from the cache.
 		lua_cache_get (L, self->connection);
 		lua_pushstring (L, self->hostname);
 		self->connection = NULL;
@@ -10040,7 +10071,7 @@ lua_plugin_push_struct (lua_State *L, struct lua_plugin *self,
 {
 	if (type == ISPECT_STR)
 	{
-		struct str *s = value;
+		const struct str *s = value;
 		lua_pushlstring (L, s->str, s->len);
 		return;
 	}
@@ -10194,7 +10225,8 @@ lua_plugin_reg_meta (lua_State *L, const char *name, luaL_Reg *fns)
 static void
 lua_plugin_reg_weak (lua_State *L, struct lua_weak_info *info, luaL_Reg *fns)
 {
-	// Create a mapping from the object type back to our metadata
+	// Create a mapping from the object type (info->ispect) back to metadata
+	// so that we can figure out what to create from ISPECT_REF fields
 	lua_pushlightuserdata (L, info);
 	lua_rawsetp (L, LUA_REGISTRYINDEX, info->ispect);
 
@@ -10359,7 +10391,8 @@ plugin_load (struct app_context *ctx, const char *name)
 	struct plugin *plugin = plugin_load_by_name (ctx, name, &e);
 	if (plugin)
 	{
-		// FIXME: this way the real name isn't available to the plugin on load
+		// FIXME: this way the real name isn't available to the plugin on load,
+		//   which has effect on e.g. plugin_config_name()
 		free (plugin->name);
 		plugin->name = xstrdup (name);
 
@@ -10464,6 +10497,7 @@ static char *
 maybe_cut_word_from_end (char **s, word_validator_fn validator, void *user_data)
 {
 	// Find the start and end of the last word
+	// Contrary to maybe_cut_word(), we ignore all whitespace at the end
 	char *start = *s, *end = start + strlen (start);
 	while (end  > start &&  strchr (WORD_BREAKING_CHARS, end [-1]))
 		end--;
@@ -11149,7 +11183,9 @@ static bool
 handle_command_topic (struct handler_args *a)
 {
 	if (*a->arguments)
-		// FIXME: there's no way to unset the topic
+		// FIXME: there's no way to start the topic with whitespace
+		// FIXME: there's no way to unset the topic;
+		//   we could adopt the Tcl style of "-switches" with "--" sentinels
 		irc_send (a->s, "TOPIC %s :%s", a->channel_name, a->arguments);
 	else
 		irc_send (a->s, "TOPIC %s", a->channel_name);
@@ -11228,6 +11264,7 @@ mass_channel_mode_mask_list
 	for (size_t i = 0; i < v.len; i++)
 	{
 		char *target = v.vector[i];
+		// TODO: support EXTBAN and leave those alone, too
 		if (strpbrk (target, "!@*?"))
 			continue;
 
@@ -11768,6 +11805,7 @@ handle_command_help (struct handler_args *a)
 	if (!*a->arguments)
 		return show_command_list (ctx);
 
+	// TODO: we should probably also accept commands names with a leading slash
 	char *command = cut_word (&a->arguments);
 	for (size_t i = 0; i < N_ELEMENTS (g_command_handlers); i++)
 	{
@@ -12963,7 +13001,9 @@ on_readline_input (char *line)
 			add_history (line);
 
 		// readline always erases the input line after returning from here,
-		// but we don't want that in order to allow correct buffer switching
+		// but we don't want that to happen if the command to be executed
+		// would switch the buffer (we'd keep the already executed command in
+		// the old buffer and delete any input restored from the new buffer)
 		strv_append_owned (&ctx->pending_input, line);
 		poller_idle_set (&ctx->input_event);
 	}
@@ -13450,6 +13490,7 @@ process_mirc_escape (const struct pollfd *fd, struct app_context *ctx)
 		goto error;
 	buf->str[++buf->len] = '\0';
 
+	// XXX: I think this should be global and shared with Readline/libedit
 	mbstate_t state;
 	memset (&state, 0, sizeof state);