format-table: give mixed cell types a stable sort order (#43129)

Mixed-type cells were compared by their insertion indices, while cells
of the same type were compared by value. Combining those rules
could make `cell_data_compare()` non-transitive.

Give different non-empty `TableDataType` values a stable order. Define
empty cells as greater than non-empty cells, so they appear last in
ascending sorts and first when the order is reversed. This ensures that
the comparator defines a consistent ordering.

Fixes #43128.

diff --git a/src/shared/format-table.c b/src/shared/format-table.c
index 360fdf596cfc8c7298eecb3accbc48c0406d8195..772d3475c1f4731ca7611eb00e49c805360973fe 100644 (file)
--- a/src/shared/format-table.c
+++ b/src/shared/format-table.c
@@ -1411,154 +1411,156 @@ int table_hide_column_from_display_internal(Table *t, ...) {
         return 0;
 }
 
-static int cell_data_compare(TableData *a, size_t index_a, TableData *b, size_t index_b) {
+static int cell_data_compare(TableData *a, TableData *b) {
         int r;
 
         assert(a);
         assert(b);
 
-        if (a->type == b->type) {
+        r = CMP(a->type == TABLE_EMPTY, b->type == TABLE_EMPTY);
+        if (r != 0)
+                return r;
 
-                /* We only define ordering for cells of the same data type. If cells with different data types are
-                 * compared we follow the order the cells were originally added in */
+        r = CMP(a->type, b->type);
+        if (r != 0)
+                return r;
 
-                switch (a->type) {
+        switch (a->type) {
 
-                case TABLE_STRING:
-                case TABLE_STRING_WITH_ANSI:
-                case TABLE_FIELD:
-                case TABLE_HEADER:
-                        return strcmp(a->string, b->string);
+        case TABLE_STRING:
+        case TABLE_STRING_WITH_ANSI:
+        case TABLE_FIELD:
+        case TABLE_HEADER:
+                return strcmp(a->string, b->string);
 
-                case TABLE_PATH:
-                case TABLE_PATH_BASENAME:
-                        return path_compare(a->string, b->string);
+        case TABLE_PATH:
+        case TABLE_PATH_BASENAME:
+                return path_compare(a->string, b->string);
 
-                case TABLE_VERSION:
-                        return strverscmp_improved(a->string, b->string);
+        case TABLE_VERSION:
+                return strverscmp_improved(a->string, b->string);
 
-                case TABLE_STRV:
-                case TABLE_STRV_WRAPPED:
-                        return strv_compare(a->strv, b->strv);
+        case TABLE_STRV:
+        case TABLE_STRV_WRAPPED:
+                return strv_compare(a->strv, b->strv);
 
-                case TABLE_BOOLEAN:
-                case TABLE_BOOLEAN_CHECKMARK:
-                        if (!a->boolean && b->boolean)
-                                return -1;
-                        if (a->boolean && !b->boolean)
-                                return 1;
-                        return 0;
+        case TABLE_BOOLEAN:
+        case TABLE_BOOLEAN_CHECKMARK:
+                if (!a->boolean && b->boolean)
+                        return -1;
+                if (a->boolean && !b->boolean)
+                        return 1;
+                return 0;
 
-                case TABLE_TRISTATE:
-                        /* NB: we do not use CMP() here, since we want to collapse all negative and all
-                         * positive into one bucket each. */
-                        if ((a->tristate < 0 && b->tristate >= 0) ||
-                            (a->tristate == 0 && b->tristate > 0))
-                                return -1;
-
-                        if ((b->tristate < 0 && a->tristate >= 0) ||
-                            (b->tristate == 0 && a->tristate > 0))
-                                return 1;
-                        return 0;
+        case TABLE_TRISTATE:
+                /* NB: we do not use CMP() here, since we want to collapse all negative and all
+                 * positive into one bucket each. */
+                if ((a->tristate < 0 && b->tristate >= 0) ||
+                    (a->tristate == 0 && b->tristate > 0))
+                        return -1;
+
+                if ((b->tristate < 0 && a->tristate >= 0) ||
+                    (b->tristate == 0 && a->tristate > 0))
+                        return 1;
+                return 0;
 
-                case TABLE_TIMESTAMP:
-                case TABLE_TIMESTAMP_UTC:
-                case TABLE_TIMESTAMP_RELATIVE:
-                case TABLE_TIMESTAMP_RELATIVE_MONOTONIC:
-                case TABLE_TIMESTAMP_LEFT:
-                case TABLE_TIMESTAMP_DATE:
-                        return CMP(a->timestamp, b->timestamp);
+        case TABLE_TIMESTAMP:
+        case TABLE_TIMESTAMP_UTC:
+        case TABLE_TIMESTAMP_RELATIVE:
+        case TABLE_TIMESTAMP_RELATIVE_MONOTONIC:
+        case TABLE_TIMESTAMP_LEFT:
+        case TABLE_TIMESTAMP_DATE:
+                return CMP(a->timestamp, b->timestamp);
 
-                case TABLE_TIMESPAN:
-                case TABLE_TIMESPAN_MSEC:
-                case TABLE_TIMESPAN_DAY:
-                        return CMP(a->timespan, b->timespan);
+        case TABLE_TIMESPAN:
+        case TABLE_TIMESPAN_MSEC:
+        case TABLE_TIMESPAN_DAY:
+                return CMP(a->timespan, b->timespan);
 
-                case TABLE_SIZE:
-                case TABLE_BPS:
-                        return CMP(a->size, b->size);
+        case TABLE_SIZE:
+        case TABLE_BPS:
+                return CMP(a->size, b->size);
 
-                case TABLE_INT:
-                case TABLE_SIGNAL:
-                        return CMP(a->int_val, b->int_val);
+        case TABLE_INT:
+        case TABLE_SIGNAL:
+                return CMP(a->int_val, b->int_val);
 
-                case TABLE_INT8:
-                        return CMP(a->int8, b->int8);
+        case TABLE_INT8:
+                return CMP(a->int8, b->int8);
 
-                case TABLE_INT16:
-                        return CMP(a->int16, b->int16);
+        case TABLE_INT16:
+                return CMP(a->int16, b->int16);
 
-                case TABLE_INT32:
-                        return CMP(a->int32, b->int32);
+        case TABLE_INT32:
+                return CMP(a->int32, b->int32);
 
-                case TABLE_INT64:
-                        return CMP(a->int64, b->int64);
+        case TABLE_INT64:
+                return CMP(a->int64, b->int64);
 
-                case TABLE_UINT:
-                        return CMP(a->uint_val, b->uint_val);
+        case TABLE_UINT:
+                return CMP(a->uint_val, b->uint_val);
 
-                case TABLE_UINT8:
-                        return CMP(a->uint8, b->uint8);
+        case TABLE_UINT8:
+                return CMP(a->uint8, b->uint8);
 
-                case TABLE_UINT16:
-                        return CMP(a->uint16, b->uint16);
+        case TABLE_UINT16:
+                return CMP(a->uint16, b->uint16);
 
-                case TABLE_UINT32:
-                case TABLE_UINT32_HEX:
-                case TABLE_UINT32_HEX_0x:
-                        return CMP(a->uint32, b->uint32);
+        case TABLE_UINT32:
+        case TABLE_UINT32_HEX:
+        case TABLE_UINT32_HEX_0x:
+                return CMP(a->uint32, b->uint32);
 
-                case TABLE_UINT64:
-                case TABLE_UINT64_HEX:
-                case TABLE_UINT64_HEX_0x:
-                        return CMP(a->uint64, b->uint64);
+        case TABLE_UINT64:
+        case TABLE_UINT64_HEX:
+        case TABLE_UINT64_HEX_0x:
+                return CMP(a->uint64, b->uint64);
 
-                case TABLE_PERCENT:
-                        return CMP(a->percent, b->percent);
+        case TABLE_PERCENT:
+                return CMP(a->percent, b->percent);
 
-                case TABLE_IFINDEX:
-                        return CMP(a->ifindex, b->ifindex);
+        case TABLE_IFINDEX:
+                return CMP(a->ifindex, b->ifindex);
 
-                case TABLE_IN_ADDR:
-                        return CMP(a->address.in.s_addr, b->address.in.s_addr);
+        case TABLE_IN_ADDR:
+                return CMP(a->address.in.s_addr, b->address.in.s_addr);
 
-                case TABLE_IN6_ADDR:
-                        return memcmp(&a->address.in6, &b->address.in6, FAMILY_ADDRESS_SIZE(AF_INET6));
+        case TABLE_IN6_ADDR:
+                return memcmp(&a->address.in6, &b->address.in6, FAMILY_ADDRESS_SIZE(AF_INET6));
 
-                case TABLE_UUID:
-                case TABLE_ID128:
-                        return memcmp(&a->id128, &b->id128, sizeof(sd_id128_t));
+        case TABLE_UUID:
+        case TABLE_ID128:
+                return memcmp(&a->id128, &b->id128, sizeof(sd_id128_t));
 
-                case TABLE_UID:
-                        return CMP(a->uid, b->uid);
+        case TABLE_UID:
+                return CMP(a->uid, b->uid);
 
-                case TABLE_GID:
-                        return CMP(a->gid, b->gid);
+        case TABLE_GID:
+                return CMP(a->gid, b->gid);
 
-                case TABLE_PID:
-                        return CMP(a->pid, b->pid);
+        case TABLE_PID:
+                return CMP(a->pid, b->pid);
 
-                case TABLE_MODE:
-                case TABLE_MODE_INODE_TYPE:
-                        return CMP(a->mode, b->mode);
+        case TABLE_MODE:
+        case TABLE_MODE_INODE_TYPE:
+                return CMP(a->mode, b->mode);
 
-                case TABLE_DEVNUM:
-                        r = CMP(major(a->devnum), major(b->devnum));
-                        if (r != 0)
-                                return r;
+        case TABLE_DEVNUM:
+                r = CMP(major(a->devnum), major(b->devnum));
+                if (r != 0)
+                        return r;
 
-                        return CMP(minor(a->devnum), minor(b->devnum));
+                return CMP(minor(a->devnum), minor(b->devnum));
 
-                case TABLE_JSON:
-                        return json_variant_compare(a->json, b->json);
+        case TABLE_JSON:
+                return json_variant_compare(a->json, b->json);
 
-                default:
-                        ;
-                }
-        }
+        case TABLE_EMPTY:
+                return 0;
 
-        /* Generic fallback using the original order in which the cells where added. */
-        return CMP(index_a, index_b);
+        default:
+                assert_not_reached();
+        }
 }
 
 static int table_data_compare(const size_t *a, const size_t *b, Table *t) {
@@ -1586,7 +1588,7 @@ static int table_data_compare(const size_t *a, const size_t *b, Table *t) {
                 d = t->data[*a + t->sort_map[i]];
                 dd = t->data[*b + t->sort_map[i]];
 
-                r = cell_data_compare(d, *a, dd, *b);
+                r = cell_data_compare(d, dd);
                 if (r != 0)
                         return t->reverse_map && t->reverse_map[t->sort_map[i]] ? -r : r;
         }

diff --git a/src/shared/format-table.h b/src/shared/format-table.h
index 43e2b6772cab1f381714fafc31e6ed976f47079c..8c0acac2179a761257abc29651989632a5458603 100644 (file)
--- a/src/shared/format-table.h
+++ b/src/shared/format-table.h
@@ -8,6 +8,7 @@
 #include "pager.h"
 
 typedef enum TableDataType {
+        /* Except for TABLE_EMPTY, declaration order defines the order between cells of different types. */
         TABLE_EMPTY,
         TABLE_STRING,
         TABLE_STRING_WITH_ANSI,    /* like the above, but contains ANSI sequences/TABs. They will be stripped when outputting to JSON */

diff --git a/src/test/test-format-table.c b/src/test/test-format-table.c
index 9aae79e223987304624809dabad5f5edf28d5eb1..4c96d6caf041a28fd35f4d2e7b859eaaec55b3b0 100644 (file)
--- a/src/test/test-format-table.c
+++ b/src/test/test-format-table.c
@@ -581,6 +581,60 @@ TEST(table) {
                              "5min              5min              \n");
 }
 
+TEST(mixed_type_sort) {
+        _cleanup_(table_unrefp) Table *table = NULL;
+        _cleanup_free_ char *formatted = NULL;
+
+        ASSERT_NOT_NULL((table = table_new("key")));
+        table_set_header(table, false);
+        table_set_ersatz_string(table, TABLE_ERSATZ_DASH);
+
+        ASSERT_OK(table_add_many(table, TABLE_UINT, 1U));
+        ASSERT_OK(table_add_cell(table, NULL, TABLE_STRING, "b"));
+        ASSERT_OK(table_add_cell(table, NULL, TABLE_STRING, NULL));
+        ASSERT_OK(table_add_cell(table, NULL, TABLE_STRING, "a"));
+        ASSERT_OK(table_set_sort(table, (size_t) 0));
+        ASSERT_OK(table_format(table, &formatted));
+
+        ASSERT_STREQ(formatted,
+                     "a\n"
+                     "b\n"
+                     "1\n"
+                     "-\n");
+
+        formatted = mfree(formatted);
+        ASSERT_OK(table_set_reverse(table, 0, true));
+        ASSERT_OK(table_format(table, &formatted));
+
+        ASSERT_STREQ(formatted,
+                     "-\n"
+                     "1\n"
+                     "b\n"
+                     "a\n");
+}
+
+TEST(empty_primary_sort) {
+        _cleanup_(table_unrefp) Table *table = NULL;
+        _cleanup_free_ char *formatted = NULL;
+
+        ASSERT_NOT_NULL((table = table_new("primary", "secondary")));
+        table_set_header(table, false);
+        table_set_ersatz_string(table, TABLE_ERSATZ_DASH);
+
+        ASSERT_OK(table_add_many(table,
+                                 TABLE_EMPTY,
+                                 TABLE_STRING, "b"));
+        ASSERT_OK(table_add_many(table,
+                                 TABLE_EMPTY,
+                                 TABLE_STRING, "a"));
+        ASSERT_OK(table_set_sort(table, (size_t) 0, (size_t) 1));
+        ASSERT_OK(table_format(table, &formatted));
+
+        ASSERT_STREQ(formatted,
+                     "- a\n"
+                     "- b\n");
+}
+
 TEST(tristate) {
         _cleanup_(sd_json_variant_unrefp) sd_json_variant *v = NULL, *w = NULL;
         _cleanup_(table_unrefp) Table *t = NULL;