Cleanup work

Key changes:

  - Added a prepared-layout API in layout-custom.c:
      - Parse and validate once.
      - Retain the validated temporary tree.
      - Apply through a no-failure, consuming operation.

  - Multi-window layouts prepare every record before changing any window.
  - Removed duplicate parsing, recalculation, and notification work.
  - Restored very old legacy layouts where pane IDs are absent.
  - Added structural validation before and after surplus-cell pruning.
  - Split pane-ID uniqueness and matching logic.
  - Renamed LAYOUT_FORMAT_NEW to LAYOUT_FORMAT.
  - Kept LAYOUT_CELL_HIDDEN as the requested serialization/rendering marker
    only.

  - Updated documentation and regression coverage, including:
      - Historical bb62,... layouts.
      - Malformed discarded cells.
      - Maximum parser nesting depth.

cmd-select-layout.c

@@ -34,8 +34,9 @@ static enum cmd_retval	cmd_select_layout_exec_multiple(struct cmdq_item *,
 			    const char *);
 
 struct cmd_select_layout_record {
-	struct window	*w;
-	char		*layout;
+	struct window			*w;
+	char				*layout;
+	struct layout_prepared		*prepared;
 };
 
 const struct cmd_entry cmd_select_layout_entry = {
@@ -83,8 +84,10 @@ cmd_select_layout_free_records(struct cmd_select_layout_record *records,
 {
 	u_int	i;
 
-	for (i = 0; i < nrecords; i++)
+	for (i = 0; i < nrecords; i++) {
 		free(records[i].layout);
+		layout_free_prepared(records[i].prepared);
+	}
 	free(records);
 }
 
@@ -148,6 +151,7 @@ cmd_select_layout_exec_multiple(struct cmdq_item *item, const char *input)
 		records[nrecords].w = w;
 		records[nrecords].layout = xstrndup(layoutstart,
 		    layoutend - layoutstart);
+		records[nrecords].prepared = NULL;
 		nrecords++;
 	}
 	if (nrecords == 0)
@@ -155,8 +159,9 @@ cmd_select_layout_exec_multiple(struct cmdq_item *item, const char *input)
 
 	/* Validate every record before changing any window. */
 	for (i = 0; i < nrecords; i++) {
-		if (layout_validate(records[i].w, records[i].layout,
-		    &cause) != 0) {
+		records[i].prepared = layout_prepare(records[i].w,
+		    records[i].layout, &cause);
+		if (records[i].prepared == NULL) {
 			cmdq_error(item, "@%u: %s", records[i].w->id, cause);
 			free(cause);
 			goto fail;
@@ -168,17 +173,14 @@ cmd_select_layout_exec_multiple(struct cmdq_item *item, const char *input)
 		server_unzoom_window(w);
 		oldlayout = w->old_layout;
 		w->old_layout = layout_dump(w, w->layout_root);
-		if (layout_parse(w, records[i].layout, &cause) != 0) {
-			cmdq_error(item, "@%u: %s", w->id, cause);
-			free(cause);
-			free(w->old_layout);
-			w->old_layout = oldlayout;
-			goto fail;
-		}
+		layout_apply_prepared(w, records[i].prepared);
+		records[i].prepared = NULL;
 		free(oldlayout);
-		recalculate_sizes();
-		server_redraw_window(w);
-		notify_window("window-layout-changed", w);
+	}
+	recalculate_sizes();
+	for (i = 0; i < nrecords; i++) {
+		server_redraw_window(records[i].w);
+		notify_window("window-layout-changed", records[i].w);
 	}
 
 	cmd_select_layout_free_records(records, nrecords);
@@ -199,6 +201,7 @@ cmd_select_layout_exec(struct cmd *self, struct cmdq_item *item)
 	struct winlink		*wl = target->wl;
 	struct window		*w = wl->window;
 	struct window_pane	*wp = target->wp;
+	struct layout_prepared	*prepared = NULL;
 	const char		*layoutname;
 	char			*oldlayout, *cause;
 	int			 next, previous, layout;
@@ -213,11 +216,13 @@ cmd_select_layout_exec(struct cmd *self, struct cmdq_item *item)
 			ptr++;
 		if (*ptr == '@')
 			return (cmd_select_layout_exec_multiple(item, ptr));
-		if (layout_set_lookup(ptr) == -1 &&
-		    layout_validate(w, ptr, &cause) != 0) {
-			cmdq_error(item, "%s: %s", cause, ptr);
-			free(cause);
-			return (CMD_RETURN_ERROR);
+		if (layout_set_lookup(ptr) == -1) {
+			prepared = layout_prepare(w, ptr, &cause);
+			if (prepared == NULL) {
+				cmdq_error(item, "%s: %s", cause, ptr);
+				free(cause);
+				return (CMD_RETURN_ERROR);
+			}
 		}
 	}
 
@@ -265,11 +270,16 @@ cmd_select_layout_exec(struct cmd *self, struct cmdq_item *item)
 	}
 
 	if (layoutname != NULL) {
-		if (layout_parse(w, layoutname, &cause) == -1) {
-			cmdq_error(item, "%s: %s", cause, layoutname);
-			free(cause);
-			goto error;
+		if (prepared == NULL) {
+			prepared = layout_prepare(w, layoutname, &cause);
+			if (prepared == NULL) {
+				cmdq_error(item, "%s: %s", cause, layoutname);
+				free(cause);
+				goto error;
+			}
 		}
+		layout_apply_prepared(w, prepared);
+		prepared = NULL;
 		goto changed;
 	}
 
@@ -284,6 +294,7 @@ changed:
 	return (CMD_RETURN_NORMAL);
 
 error:
+	layout_free_prepared(prepared);
 	free(w->old_layout);
 	w->old_layout = oldlayout;
 	return (CMD_RETURN_ERROR);

layout-custom.c

@@ -20,6 +20,7 @@
 
 #include <ctype.h>
 #include <limits.h>
+#include <stdlib.h>
 #include <string.h>
 
 #include "tmux.h"
@@ -30,7 +31,7 @@
 enum layout_format {
 	LAYOUT_FORMAT_UNKNOWN,
 	LAYOUT_FORMAT_LEGACY,
-	LAYOUT_FORMAT_NEW
+	LAYOUT_FORMAT
 };
 
 struct layout_parse_ctx {
@@ -41,6 +42,14 @@ struct layout_parse_ctx {
 	u_int			 cells;
 };
 
+struct layout_prepared {
+	struct layout_cell	*root;
+	struct layout_cell	*zoomed;
+	enum layout_format	 format;
+	u_int			 ncells;
+	int			 pane_ids;
+};
+
 static struct layout_cell	*layout_find_bottomright(struct layout_cell *);
 static u_short			 layout_checksum(const char *, const char *);
 static int			 layout_append(struct window *,
@@ -435,7 +444,7 @@ layout_parse_position(struct layout_parse_ctx *ctx, int *value,
 
 /* Parse legacy comma geometry or current X-style geometry. */
 static int
-layout_parse_geometry(struct layout_parse_ctx *ctx, int current, u_int *sxp,
+layout_parse_geometry(struct layout_parse_ctx *ctx, int is_pane, u_int *sxp,
     u_int *syp, int *xoffp, int *yoffp, int *xrelative, int *yrelative)
 {
 	int	has_x, has_y;
@@ -446,7 +455,7 @@ layout_parse_geometry(struct layout_parse_ctx *ctx, int current, u_int *sxp,
 	    layout_parse_uint(ctx, syp) != 0)
 		return (-1);
 	layout_skip_space(ctx);
-	if (!current && ctx->ptr != ctx->end && *ctx->ptr == ',') {
+	if (!is_pane && ctx->ptr != ctx->end && *ctx->ptr == ',') {
 		if (layout_set_format(ctx, LAYOUT_FORMAT_LEGACY) != 0 ||
 		    layout_parse_char(ctx, ',') != 0 ||
 		    layout_parse_int(ctx, xoffp) != 0 ||
@@ -457,7 +466,7 @@ layout_parse_geometry(struct layout_parse_ctx *ctx, int current, u_int *sxp,
 			return (-1);
 		return (0);
 	}
-	if (layout_set_format(ctx, LAYOUT_FORMAT_NEW) != 0)
+	if (layout_set_format(ctx, LAYOUT_FORMAT) != 0)
 		return (-1);
 	*xoffp = *yoffp = 0;
 	has_x = layout_parse_position(ctx, xoffp, xrelative);
@@ -519,7 +528,7 @@ layout_construct(struct layout_parse_ctx *ctx, struct layout_cell *parent,
     struct layout_cell **lcp)
 {
 	struct layout_cell	*lc = NULL, *lcchild;
-	const char		*body;
+	const char		*body, *saved;
 	u_int			 sx, sy, id, z;
 	u_short			 csum = 0;
 	int			 is_pane, xoff, yoff, xrelative, yrelative;
@@ -621,7 +630,7 @@ layout_construct(struct layout_parse_ctx *ctx, struct layout_cell *parent,
 			}
 			separator = *ctx->ptr++;
 			if (separator == ';') {
-				if (layout_set_format(ctx, LAYOUT_FORMAT_NEW) != 0)
+				if (layout_set_format(ctx, LAYOUT_FORMAT) != 0)
 					goto fail;
 			} else if (separator == ',') {
 				if (layout_set_format(ctx, LAYOUT_FORMAT_LEGACY) != 0)
@@ -630,11 +639,21 @@ layout_construct(struct layout_parse_ctx *ctx, struct layout_cell *parent,
 				goto fail;
 		}
 	} else {
-		if (layout_parse_char(ctx, ',') != 0 ||
-		    layout_parse_uint(ctx, &id) != 0 ||
-		    layout_set_format(ctx, LAYOUT_FORMAT_LEGACY) != 0)
+		if (layout_set_format(ctx, LAYOUT_FORMAT_LEGACY) != 0)
 			goto fail;
-		lc->pane_id = id;
+		layout_skip_space(ctx);
+		if (ctx->ptr != ctx->end && *ctx->ptr == ',') {
+			saved = ctx->ptr++;
+			if (layout_parse_uint(ctx, &id) != 0) {
+				ctx->ptr = saved;
+			} else {
+				layout_skip_space(ctx);
+				if (ctx->ptr != ctx->end && *ctx->ptr == 'x')
+					ctx->ptr = saved;
+				else
+					lc->pane_id = id;
+			}
+		}
 	}
 
 	if (has_checksum && csum != layout_checksum(body, ctx->ptr))
@@ -761,27 +780,36 @@ layout_count_pane_id(struct layout_cell *lc, u_int pane_id)
 	return (count);
 }
 
-/* Check IDs are unique and count those belonging to the target window. */
+/* Return whether every parsed pane ID is unique. */
 static int
-layout_pane_ids_status1(struct window *w, struct layout_cell *root,
-    struct layout_cell *lc, u_int *matches)
+layout_pane_ids_unique(struct layout_cell *root, struct layout_cell *lc)
+{
+	struct layout_cell	*lcchild;
+
+	if (lc->type == LAYOUT_WINDOWPANE)
+		return (layout_count_pane_id(root, lc->pane_id) == 1);
+	TAILQ_FOREACH(lcchild, &lc->cells, entry) {
+		if (!layout_pane_ids_unique(root, lcchild))
+			return (0);
+	}
+	return (1);
+}
+
+/* Count parsed pane IDs belonging to the target window. */
+static u_int
+layout_pane_ids_matching(struct window *w, struct layout_cell *lc)
 {
 	struct layout_cell	*lcchild;
 	struct window_pane	*wp;
+	u_int			 matches = 0;
 
 	if (lc->type == LAYOUT_WINDOWPANE) {
-		if (layout_count_pane_id(root, lc->pane_id) != 1)
-			return (-1);
 		wp = window_pane_find_by_id(lc->pane_id);
-		if (wp != NULL && wp->window == w)
-			(*matches)++;
-		return (0);
+		return (wp != NULL && wp->window == w);
 	}
-	TAILQ_FOREACH(lcchild, &lc->cells, entry) {
-		if (layout_pane_ids_status1(w, root, lcchild, matches) != 0)
-			return (-1);
-	}
-	return (0);
+	TAILQ_FOREACH(lcchild, &lc->cells, entry)
+		matches += layout_pane_ids_matching(w, lcchild);
+	return (matches);
 }
 
 /* Return 1 to assign by ID, 0 by tree order, or -1 for ambiguous IDs. */
@@ -789,10 +817,9 @@ static int
 layout_pane_ids_status(struct window *w, struct layout_cell *root,
     u_int ncells)
 {
-	u_int	matches = 0;
+	u_int	matches;
 
-	if (layout_pane_ids_status1(w, root, root, &matches) != 0)
-		return (-1);
+	matches = layout_pane_ids_matching(w, root);
 	if (matches == 0)
 		return (0);
 	if (matches == ncells)
@@ -800,21 +827,21 @@ layout_pane_ids_status(struct window *w, struct layout_cell *root,
 	return (-1);
 }
 
-/* Parse a layout string and optionally arrange the window. */
-static int
-layout_parse1(struct window *w, const char *layout, char **cause, int apply)
+/* Parse and validate a layout without changing the window. */
+struct layout_prepared *
+layout_prepare(struct window *w, const char *layout, char **cause)
 {
 	struct layout_parse_ctx	 ctx;
 	struct layout_cell	*lcchild, *root = NULL, *zoomed = NULL;
-	struct window_pane	*wp;
+	struct layout_prepared	*prepared;
 	const char		*body;
-	u_int			 npanes, ncells, old_ncells, z, matches;
+	u_int			 npanes, ncells, old_ncells;
 	u_short			 csum;
 	int			 has_checksum, pane_ids = 0;
 
 	if (strlen(layout) >= LAYOUT_STRING_MAX) {
 		*cause = xstrdup("invalid layout");
-		return (-1);
+		return (NULL);
 	}
 	ctx.ptr = layout;
 	ctx.end = layout + strlen(layout);
@@ -827,33 +854,36 @@ layout_parse1(struct window *w, const char *layout, char **cause, int apply)
 	    layout_construct(&ctx, NULL, &root) != 0) {
 		*cause = xstrdup("invalid layout");
 		layout_free_cell(root);
-		return (-1);
+		return (NULL);
 	}
 	layout_skip_space(&ctx);
 	if (ctx.ptr != ctx.end || root == NULL) {
 		*cause = xstrdup("invalid layout");
 		layout_free_cell(root);
-		return (-1);
+		return (NULL);
 	}
 	if (layout_resolve_relative(root,
 	    root->type == LAYOUT_WINDOWPANE ? w->sx : root->sx,
 	    root->type == LAYOUT_WINDOWPANE ? w->sy : root->sy) != 0) {
 		*cause = xstrdup("invalid layout");
 		layout_free_cell(root);
-		return (-1);
+		return (NULL);
 	}
 
 	npanes = window_count_panes(w, 1);
 	ncells = layout_count_cells(root);
-	if (ctx.format == LAYOUT_FORMAT_NEW) {
+	if (ctx.format == LAYOUT_FORMAT) {
 		old_ncells = ncells;
-		matches = 0;
 		if (npanes > ncells ||
 		    layout_validate_new(root, ncells, &zoomed) != 0 ||
-		    layout_pane_ids_status1(w, root, root, &matches) != 0) {
+		    !layout_pane_ids_unique(root, root)) {
 			*cause = xstrdup("invalid layout");
 			goto fail;
 		}
+		if (!layout_check(root)) {
+			*cause = xstrdup("size mismatch before applying layout");
+			goto fail;
+		}
 		if (npanes < ncells) {
 			while (npanes < ncells) {
 				lcchild = layout_find_bottomright(root);
@@ -898,23 +928,52 @@ layout_parse1(struct window *w, const char *layout, char **cause, int apply)
 		*cause = xstrdup("size mismatch after applying layout");
 		goto fail;
 	}
-	if (!apply) {
-		layout_free_cell(root);
-		return (0);
-	}
 
-	/* The layout is fully validated before the existing layout is changed. */
+	prepared = xcalloc(1, sizeof *prepared);
+	prepared->root = root;
+	prepared->zoomed = zoomed;
+	prepared->format = ctx.format;
+	prepared->ncells = ncells;
+	prepared->pane_ids = pane_ids;
+	return (prepared);
+
+fail:
+	layout_free_cell(root);
+	return (NULL);
+}
+
+/* Free a prepared layout without applying it. */
+void
+layout_free_prepared(struct layout_prepared *prepared)
+{
+	if (prepared == NULL)
+		return;
+	layout_free_cell(prepared->root);
+	free(prepared);
+}
+
+/* Apply and free a prepared layout. This cannot fail. */
+void
+layout_apply_prepared(struct window *w, struct layout_prepared *prepared)
+{
+	struct layout_cell	*lcchild, *root = prepared->root;
+	struct window_pane	*wp;
+	u_int			 z;
+
+	prepared->root = NULL;
+
+	/* The layout was fully validated before the existing layout is changed. */
 	layout_free_cell(w->layout_root);
 	w->layout_root = root;
 	wp = TAILQ_FIRST(&w->panes);
-	layout_assign(w, &wp, root, pane_ids);
+	layout_assign(w, &wp, root, prepared->pane_ids);
 
 	while (!TAILQ_EMPTY(&w->z_index)) {
 		wp = TAILQ_FIRST(&w->z_index);
 		TAILQ_REMOVE(&w->z_index, wp, zentry);
 	}
-	if (ctx.format == LAYOUT_FORMAT_NEW) {
-		for (z = 0; z < ncells; z++) {
+	if (prepared->format == LAYOUT_FORMAT) {
+		for (z = 0; z < prepared->ncells; z++) {
 			lcchild = layout_find_zindex(root, z);
 			TAILQ_INSERT_TAIL(&w->z_index, lcchild->wp, zentry);
 		}
@@ -925,33 +984,13 @@ layout_parse1(struct window *w, const char *layout, char **cause, int apply)
 		window_resize(w, root->sx, root->sy, -1, -1);
 	layout_fix_offsets(w);
 	layout_fix_panes(w, NULL);
-	recalculate_sizes();
 	layout_print_cell(root, __func__, 0);
 
-	if (zoomed != NULL) {
-		zoomed->flags &= ~LAYOUT_CELL_ZOOMED;
-		window_zoom(zoomed->wp);
+	if (prepared->zoomed != NULL) {
+		prepared->zoomed->flags &= ~LAYOUT_CELL_ZOOMED;
+		window_zoom(prepared->zoomed->wp);
 	}
-	notify_window("window-layout-changed", w);
-	return (0);
-
-fail:
-	layout_free_cell(root);
-	return (-1);
-}
-
-/* Validate a layout string without changing the window. */
-int
-layout_validate(struct window *w, const char *layout, char **cause)
-{
-	return (layout_parse1(w, layout, cause, 0));
-}
-
-/* Parse a layout string and arrange window as layout. */
-int
-layout_parse(struct window *w, const char *layout, char **cause)
-{
-	return (layout_parse1(w, layout, cause, 1));
+	free(prepared);
 }
 
 /* Assign panes to cells in tree order. */

regress/layout-custom.sh

@@ -65,6 +65,12 @@ must_equal "$($TMUX list-panes -F '#{pane_width}x#{pane_height},#{pane_left},#{p
     '56x28,0,0
 56x28,57,0'
 
+# Layouts from before pane IDs were added remain accepted.
+historical='bb62,159x48,0,0{79x48,0,0,79x48,80,0}'
+$TMUX select-layout "$historical" || fail "legacy layout without IDs failed"
+must_equal "$($TMUX display-message -p '#{window_width}x#{window_height}')" \
+    '159x48'
+
 # Return to the smaller layout for the remaining parser tests.
 $TMUX select-layout "$legacy" || fail "legacy layout could not be reapplied"
 layout=$($TMUX display-message -p '#{window_layout}')
@@ -112,6 +118,10 @@ case "$($TMUX display-message -p '#{window_layout}')" in
 *'%0,0:26x24+0+0;%1,1:53x24+27+0}'*) ;;
 *) fail "z-indexes were not compacted after removing a cell" ;;
 esac
+# The complete tree must be structurally valid before surplus cells are
+# removed; the last child has the wrong height here.
+must_fail $TMUX select-layout \
+    '80x24+0+0{%100,0:39x24+0+0;%101,1:20x24+40+0;%102,2:19x10+61+0}'
 must_fail $TMUX select-layout '%100,0:80x24+0+0'
 $TMUX select-layout "$legacy" || fail "legacy layout could not be restored"
 
@@ -142,6 +152,18 @@ must_fail $TMUX select-layout \
 must_equal "$($TMUX display-message -p -t %0 '#{pane_zoomed_flag}')" 1
 $TMUX resize-pane -t %0 -Z || fail "unzoom failed"
 
+# The parser accepts its maximum nesting depth and safely rejects one more.
+$TMUX kill-server 2>/dev/null
+$TMUX new-session -d -x 80 -y 24 || exit 1
+deep='%0,0:1x1+0+0'
+i=1
+while [ $i -lt 64 ]; do
+	deep="1x1+0+0{$deep}"
+	i=$((i + 1))
+done
+$TMUX select-layout "$deep" || fail "maximum layout depth was rejected"
+must_fail $TMUX select-layout "1x1+0+0{$deep}"
+
 # X-style geometry supports right/bottom-relative offsets, absolute negative
 # offsets, doubled plus signs, and omitted positions.
 $TMUX kill-server 2>/dev/null

tmux.1

@@ -4051,9 +4051,14 @@ and does not include z-indexes or flags:
 legacy-geometry = widthxheight,x,y
 cell            = legacy-geometry{cell,cell,...}
                 | legacy-geometry[cell,cell,...]
-                | legacy-geometry,pane-id
+                | legacy-geometry[,pane-id]
 .Ed
 .Pp
+Pane IDs are optional in legacy layouts and are ignored when assigning panes.
+This permits layouts from versions of
+.Nm
+before pane IDs were added.
+.Pp
 The checksum is optional.
 If present, it is four hexadecimal digits and covers all non-whitespace text
 after its comma; a layout with an incorrect checksum is rejected.

tmux.h

@@ -1506,7 +1506,7 @@ struct layout_cell {
 	enum layout_type type;
 
 #define LAYOUT_CELL_FLOATING 0x1
-#define LAYOUT_CELL_HIDDEN 0x2
+#define LAYOUT_CELL_HIDDEN 0x2	/* hidden in a serialized layout */
 #define LAYOUT_CELL_ZOOMED 0x4	/* only while parsing a layout */
 #define LAYOUT_CELL_X_RELATIVE 0x8 /* only while parsing a layout */
 #define LAYOUT_CELL_Y_RELATIVE 0x10 /* only while parsing a layout */
@@ -3737,9 +3737,11 @@ int		 layout_remove_tile(struct window *, struct layout_cell *);
 int		 layout_insert_tile(struct window *, struct layout_cell *);
 
 /* layout-custom.c */
+struct layout_prepared;
 char		*layout_dump(struct window *, struct layout_cell *);
-int		 layout_validate(struct window *, const char *, char **);
-int		 layout_parse(struct window *, const char *, char **);
+struct layout_prepared *layout_prepare(struct window *, const char *, char **);
+void		 layout_free_prepared(struct layout_prepared *);
+void		 layout_apply_prepared(struct window *, struct layout_prepared *);
 
 /* layout-set.c */
 int		 layout_set_lookup(const char *);