From 19dd1172617d792bbffbddf0f45d9068215ac0ca Mon Sep 17 00:00:00 2001
From: Eddie Kohler <ekohler@gmail.com>
Date: Wed, 30 Aug 2023 09:16:37 -0400
Subject: [PATCH] Avoid storing a tag setting that's inherited from
 $Opt[definedTags].

---
 lib/tagger.php          | 189 ++++++++++++++++++++--------------------
 src/conference.php      |   2 +-
 src/settings/s_tags.php |  93 +++++++++++++++-----
 src/sitype.php          |  10 ++-
 4 files changed, 169 insertions(+), 125 deletions(-)

diff --git a/lib/tagger.php b/lib/tagger.php
index 79d545469..9b933e0b0 100644
--- a/lib/tagger.php
+++ b/lib/tagger.php
@@ -55,22 +55,6 @@ class TagInfo {
     const TFM_VOTES = 0xC0;
     const TFM_DECORATION = 0x1C000;
 
-    /** @param string $s
-     * @return int */
-    static function parse_property($s) {
-        if ($s === "autosearch") {
-            return self::TF_AUTOSEARCH;
-        } else if ($s === "badge") {
-            return self::TF_BADGE;
-        } else if ($s === "hidden") {
-            return self::TF_HIDDEN;
-        } else if ($s === "readonly") {
-            return self::TF_READONLY;
-        } else {
-            return 0;
-        }
-    }
-
     /** @param string $tag
      * @param int $flags */
     function __construct($tag, TagMap $tagmap, $flags = 0) {
@@ -1208,60 +1192,59 @@ function unparse($tag, $value, Contact $viewer, $flags = 0) {
     }
 
 
-    static function make(Conf $conf) {
-        $map = new TagMap($conf);
+    private function merge_settings(Conf $conf) {
         foreach ($conf->track_tags() as $tn) {
-            $map->set($tn, TagInfo::TF_TRACK | TagInfo::TF_CHAIR);
+            $this->set($tn, TagInfo::TF_TRACK | TagInfo::TF_CHAIR);
         }
         if ($conf->has_named_submission_rounds()) {
             foreach ($conf->submission_round_list() as $sr) {
                 if ($sr->tag !== "") {
-                    $map->set($sr->tag, TagInfo::TF_SCLASS | TagInfo::TF_CHAIR);
+                    $this->set($sr->tag, TagInfo::TF_SCLASS | TagInfo::TF_CHAIR);
                 }
             }
         }
         $ct = $conf->setting_data("tag_chair") ?? "";
         foreach (Tagger::split_unpack($ct) as $tv) {
-            $map->set($tv[0], TagInfo::TF_READONLY);
+            $this->set($tv[0], TagInfo::TF_READONLY);
         }
         $ct = $conf->setting_data("tag_hidden") ?? "";
         foreach (Tagger::split_unpack($ct) as $tv) {
-            $map->set($tv[0], TagInfo::TF_HIDDEN);
+            $this->set($tv[0], TagInfo::TF_HIDDEN);
         }
         $ct = $conf->setting_data("tag_sitewide") ?? "";
         foreach (Tagger::split_unpack($ct) as $tv) {
-            $map->set($tv[0], TagInfo::TF_SITEWIDE);
+            $this->set($tv[0], TagInfo::TF_SITEWIDE);
         }
         $ct = $conf->setting_data("tag_conflict_free") ?? "";
         foreach (Tagger::split_unpack($ct) as $tv) {
-            $map->set($tv[0], TagInfo::TF_CONFLICT_FREE);
+            $this->set($tv[0], TagInfo::TF_CONFLICT_FREE);
         }
         $ppu = $conf->setting("tag_vote_private_peruser")
             || $conf->opt("secretPC");
         $ppuf = $ppu ? 0 : TagInfo::TF_PUBLIC_PERUSER;
         $vt = $conf->setting_data("tag_vote") ?? "";
         foreach (Tagger::split_unpack($vt) as $tv) {
-            $ti = new TagInfo($tv[0], $map, TagInfo::TF_ALLOTMENT | TagInfo::TF_AUTOMATIC | $ppuf);
+            $ti = new TagInfo($tv[0], $this, TagInfo::TF_ALLOTMENT | TagInfo::TF_AUTOMATIC | $ppuf);
             $ti->allotment = ($tv[1] ?? 1.0);
-            $map->merge($ti);
+            $this->merge($ti);
         }
         $vt = $conf->setting_data("tag_approval") ?? "";
         foreach (Tagger::split_unpack($vt) as $tv) {
-            $map->set($tv[0], TagInfo::TF_APPROVAL | TagInfo::TF_AUTOMATIC | $ppuf);
+            $this->set($tv[0], TagInfo::TF_APPROVAL | TagInfo::TF_AUTOMATIC | $ppuf);
         }
         $rt = $conf->setting_data("tag_rank") ?? "";
         foreach (Tagger::split_unpack($rt) as $tv) {
-            $map->set($tv[0], TagInfo::TF_RANK | $ppuf);
+            $this->set($tv[0], TagInfo::TF_RANK | $ppuf);
         }
         $ct = $conf->setting_data("tag_color") ?? "";
         if ($ct !== "") {
             foreach (explode(" ", $ct) as $k) {
                 if ($k !== ""
                     && ($p = strpos($k, "=")) > 0
-                    && ($ks = $map->known_style(substr($k, $p + 1))) !== null) {
-                    $ti = new TagInfo(substr($k, 0, $p), $map, TagInfo::TF_STYLE);
+                    && ($ks = $this->known_style(substr($k, $p + 1))) !== null) {
+                    $ti = new TagInfo(substr($k, 0, $p), $this, TagInfo::TF_STYLE);
                     $ti->styles[] = $ks;
-                    $map->merge($ti);
+                    $this->merge($ti);
                 }
             }
         }
@@ -1270,10 +1253,10 @@ static function make(Conf $conf) {
             foreach (explode(" ", $bt) as $k) {
                 if ($k !== ""
                     && ($p = strpos($k, "=")) > 0
-                    && ($ks = $map->known_badge(substr($k, $p + 1))) !== null) {
-                    $ti = new TagInfo(substr($k, 0, $p), $map, TagInfo::TF_BADGE);
+                    && ($ks = $this->known_badge(substr($k, $p + 1))) !== null) {
+                    $ti = new TagInfo(substr($k, 0, $p), $this, TagInfo::TF_BADGE);
                     $ti->badge = $ks;
-                    $map->merge($ti);
+                    $this->merge($ti);
                 }
             }
         }
@@ -1281,82 +1264,96 @@ static function make(Conf $conf) {
         if ($bt !== "") {
             foreach (explode(" ", $bt) as $k) {
                 if ($k !== "" && ($p = strpos($k, "=")) !== false) {
-                    $ti = new TagInfo(substr($k, 0, $p), $map, TagInfo::TF_EMOJI);
+                    $ti = new TagInfo(substr($k, 0, $p), $this, TagInfo::TF_EMOJI);
                     $ti->emoji[] = substr($k, $p + 1);
-                    $map->merge($ti);
+                    $this->merge($ti);
                 }
             }
         }
         $tx = $conf->setting_data("tag_autosearch") ?? "";
         if ($tx !== "") {
             foreach (json_decode($tx) ? : [] as $tag => $search) {
-                $ti = new TagInfo($tag, $map, TagInfo::TF_AUTOMATIC | TagInfo::TF_AUTOSEARCH);
+                $ti = new TagInfo($tag, $this, TagInfo::TF_AUTOMATIC | TagInfo::TF_AUTOSEARCH);
                 $ti->autosearch = $search->q;
                 $ti->autosearch_value = $search->v ?? null;
-                $map->merge($ti);
+                $this->merge($ti);
+            }
+        }
+    }
+
+    private function merge_json($tag, $data) {
+        $flags = 0;
+        if ($data->chair ?? false) {
+            $flags |= TagInfo::TF_CHAIR;
+        }
+        if ($data->readonly ?? false) {
+            $flags |= TagInfo::TF_READONLY;
+        }
+        if ($data->hidden ?? false) {
+            $flags |= TagInfo::TF_HIDDEN;
+        }
+        if ($data->sitewide ?? false) {
+            $flags |= TagInfo::TF_SITEWIDE;
+        }
+        if ($data->conflict_free ?? false) {
+            $flags |= TagInfo::TF_CONFLICT_FREE;
+        }
+        if ($data->autosearch ?? null) {
+            $flags |= TagInfo::TF_AUTOMATIC | TagInfo::TF_AUTOSEARCH;
+        }
+        if ($data->style ?? $data->color /* XXX */ ?? null) {
+            $flags |= TagInfo::TF_STYLE;
+        }
+        if ($data->badge ?? null) {
+            $flags |= TagInfo::TF_BADGE;
+        }
+        if ($data->emoji ?? null) {
+            $flags |= TagInfo::TF_EMOJI;
+        }
+        if ($flags === 0) {
+            return;
+        }
+        $ti = new TagInfo($tag, $this, $flags);
+        if (($flags & TagInfo::TF_AUTOSEARCH) !== 0) {
+            $ti->autosearch = $data->autosearch;
+            $ti->autosearch_value = $data->autosearch_value ?? null;
+        }
+        if (($flags & TagInfo::TF_STYLE) !== 0) {
+            $x = $data->style ?? $data->color;
+            foreach (is_string($x) ? [$x] : $x as $c) {
+                if (($ks = $this->known_style($c)) !== null) {
+                    $ti->styles[] = $ks;
+                }
+            }
+        }
+        if (($flags & TagInfo::TF_BADGE) !== 0) {
+            $x = $data->badge;
+            foreach (is_string($x) ? [$x] : $x as $c) {
+                if (($ks = $this->known_badge($c)) !== null) {
+                    $ti->badge = $ks;
+                }
             }
         }
+        if (($flags & TagInfo::TF_EMOJI) !== 0) {
+            $x = $data->badge;
+            foreach (is_string($x) ? [$x] : $x as $c) {
+                $ti->emoji[] = $c;
+            }
+        }
+        $this->merge($ti);
+    }
+
+    /** @param bool $all
+     * @return TagMap */
+    static function make(Conf $conf, $all) {
+        $map = new TagMap($conf);
+        if ($all) {
+            $map->merge_settings($conf);
+        }
         if (($od = $conf->opt("definedTags"))) {
             foreach (is_string($od) ? [$od] : $od as $ods) {
                 foreach (json_decode($ods) as $tag => $data) {
-                    $flags = 0;
-                    if ($data->chair ?? false) {
-                        $flags |= TagInfo::TF_CHAIR;
-                    }
-                    if ($data->readonly ?? false) {
-                        $flags |= TagInfo::TF_READONLY;
-                    }
-                    if ($data->hidden ?? false) {
-                        $flags |= TagInfo::TF_HIDDEN;
-                    }
-                    if ($data->sitewide ?? false) {
-                        $flags |= TagInfo::TF_SITEWIDE;
-                    }
-                    if ($data->conflict_free ?? false) {
-                        $flags |= TagInfo::TF_CONFLICT_FREE;
-                    }
-                    if ($data->autosearch ?? null) {
-                        $flags |= TagInfo::TF_AUTOMATIC | TagInfo::TF_AUTOSEARCH;
-                    }
-                    if ($data->style ?? $data->color /* XXX */ ?? null) {
-                        $flags |= TagInfo::TF_STYLE;
-                    }
-                    if ($data->badge ?? null) {
-                        $flags |= TagInfo::TF_BADGE;
-                    }
-                    if ($data->emoji ?? null) {
-                        $flags |= TagInfo::TF_EMOJI;
-                    }
-                    if ($flags !== 0) {
-                        $ti = new TagInfo($tag, $map, $flags);
-                        if (($flags & TagInfo::TF_AUTOSEARCH) !== 0) {
-                            $ti->autosearch = $data->autosearch;
-                            $ti->autosearch_value = $data->autosearch_value ?? null;
-                        }
-                        if (($flags & TagInfo::TF_STYLE) !== 0) {
-                            $x = $data->style ?? $data->color;
-                            foreach (is_string($x) ? [$x] : $x as $c) {
-                                if (($ks = $map->known_style($c)) !== null) {
-                                    $ti->styles[] = $ks;
-                                }
-                            }
-                        }
-                        if (($flags & TagInfo::TF_BADGE) !== 0) {
-                            $x = $data->badge;
-                            foreach (is_string($x) ? [$x] : $x as $c) {
-                                if (($ks = $map->known_badge($c)) !== null) {
-                                    $ti->badge = $ks;
-                                }
-                            }
-                        }
-                        if (($flags & TagInfo::TF_EMOJI) !== 0) {
-                            $x = $data->badge;
-                            foreach (is_string($x) ? [$x] : $x as $c) {
-                                $ti->emoji[] = $c;
-                            }
-                        }
-                        $map->merge($ti);
-                    }
+                    $map->merge_json($tag, $data);
                 }
             }
         }
diff --git a/src/conference.php b/src/conference.php
index d0dd360c2..2d7f9ac80 100644
--- a/src/conference.php
+++ b/src/conference.php
@@ -1321,7 +1321,7 @@ function checked_review_field($fid) {
     /** @return TagMap */
     function tags() {
         if (!$this->_tag_map) {
-            $this->_tag_map = TagMap::make($this);
+            $this->_tag_map = TagMap::make($this, true);
         }
         return $this->_tag_map;
     }
diff --git a/src/settings/s_tags.php b/src/settings/s_tags.php
index d211c9afb..0ec623fce 100644
--- a/src/settings/s_tags.php
+++ b/src/settings/s_tags.php
@@ -7,23 +7,44 @@ class Tags_SettingParser extends SettingParser {
     private $sv;
     /** @var Tagger */
     private $tagger;
+    /** @var TagMap */
+    private $base_map;
     private $diffs = [];
     private $cleaned = false;
 
+    /** @return int */
+    static function si_flag(Si $si) {
+        if ($si->name === "tag_hidden") {
+            return TagInfo::TF_HIDDEN;
+        } else if ($si->name === "tag_readonly") {
+            return TagInfo::TF_READONLY;
+        } else if ($si->name === "tag_sitewide") {
+            return TagInfo::TF_SITEWIDE;
+        } else if ($si->name === "tag_vote_approval") {
+            return TagInfo::TF_APPROVAL;
+        } else if ($si->name === "tag_vote_allotment") {
+            return TagInfo::TF_ALLOTMENT;
+        } else if ($si->name === "tag_rank") {
+            return TagInfo::TF_RANK;
+        } else {
+            return 0;
+        }
+    }
+
+    /** @return list<TagInfo> */
+    static function sorted_settings_for(TagMap $map, Si $si) {
+        return $map->sorted_settings_having(self::si_flag($si));
+    }
+
     function __construct(SettingValues $sv) {
         $this->sv = $sv;
         $this->tagger = new Tagger($sv->user);
+        $this->base_map = TagMap::make($sv->conf, false);
     }
 
     function set_oldv(Si $si, SettingValues $sv) {
-        if ($si->name === "tag_hidden") {
-            $sv->set_oldv("tag_hidden", Tags_SettingParser::render_tags($sv->conf->tags()->sorted_settings_having(TagInfo::TF_HIDDEN)));
-        } else if ($si->name === "tag_readonly") {
-            $sv->set_oldv("tag_readonly", Tags_SettingParser::render_tags($sv->conf->tags()->sorted_settings_having(TagInfo::TF_READONLY)));
-        } else if ($si->name === "tag_sitewide") {
-            $sv->set_oldv("tag_sitewide", Tags_SettingParser::render_tags($sv->conf->tags()->sorted_settings_having(TagInfo::TF_SITEWIDE)));
-        } else if ($si->name === "tag_vote_approval") {
-            $sv->set_oldv("tag_vote_approval", Tags_SettingParser::render_tags($sv->conf->tags()->sorted_settings_having(TagInfo::TF_APPROVAL)));
+        if (in_array($si->name, ["tag_hidden", "tag_readonly", "tag_sitewide", "tag_vote_approval"])) {
+            $sv->set_oldv($si->name, self::render_tags(self::sorted_settings_for($sv->conf->tags(), $si)));
         } else if ($si->name === "tag_vote_allotment") {
             $x = [];
             foreach ($sv->conf->tags()->sorted_settings_having(TagInfo::TF_ALLOTMENT) as $t) {
@@ -35,8 +56,18 @@ function set_oldv(Si $si, SettingValues $sv) {
         }
     }
 
+    /** @param list<TagInfo> $tl
+     * @return string */
     static function render_tags($tl) {
-        return join(" ", array_map(function ($t) { return $t->tag; }, $tl));
+        $last = null;
+        $tx = [];
+        foreach ($tl as $ti) {
+            if (!$last || $last->tag !== $ti->tag) {
+                $tx[] = $ti->tag;
+                $last = $ti;
+            }
+        }
+        return join(" ", $tx);
     }
     static function print_tag_chair(SettingValues $sv) {
         $sv->print_entry_group("tag_readonly", null, [
@@ -78,27 +109,41 @@ static function print_tag_seeall(SettingValues $sv) {
         $sv->print_checkbox("tag_visibility_conflict", "PC can see tags for conflicted submissions");
     }
 
+    private function strip_presets($si, $v) {
+        $have = [];
+        foreach (self::sorted_settings_for($this->base_map, $si) as $ti) {
+            $have[$ti->tag] = true;
+        }
+        $newv = [];
+        foreach (Tagger::split_unpack($v) as $tv) {
+            if (!isset($have[$tv[0]])) {
+                $newv[] = $tv[1] === null ? $tv[0] : "{$tv[0]}#{$tv[1]}";
+            }
+        }
+        return join(" ", $newv);
+    }
+
     function apply_req(Si $si, SettingValues $sv) {
         assert($this->sv === $sv);
-        if ($si->name === "tag_vote_allotment") {
-            if (($v = $sv->base_parse_req($si)) !== null
-                && $sv->update("tag_vote_allotment", $v)) {
-                $sv->request_write_lock("PaperTag");
-                $sv->request_store_value($si);
-            }
-        } else if ($si->name === "tag_vote_approval") {
-            if (($v = $sv->base_parse_req($si)) !== null
-                && $sv->update("tag_vote_approval", $v)) {
+        if (($v = $sv->base_parse_req($si)) === null) {
+            return true;
+        }
+        if (self::si_flag($si) !== 0) {
+            $v = $this->strip_presets($si, $v);
+        }
+        if ($si->name === "tag_vote_allotment" || $si->name === "tag_vote_approval") {
+            if ($sv->update($si, $v)) {
                 $sv->request_write_lock("PaperTag");
                 $sv->request_store_value($si);
             }
         } else if ($si->name === "tag_rank") {
-            if (($v = $sv->base_parse_req($si)) !== null
-                && strpos($v, " ") === false) {
+            if (strpos($v, " ") === false) {
                 $sv->save("tag_rank", $v);
             } else if ($v !== null) {
                 $sv->error_at("tag_rank", "<0>Multiple ranking tags are not supported");
             }
+        } else if (self::si_flag($si) !== 0) {
+            $sv->save($si, $v);
         } else {
             return false;
         }
@@ -152,15 +197,15 @@ static function crosscheck(SettingValues $sv) {
             ["tag_rank", "tag_rank", "ranking"]
         ];
         foreach ($vinfo as $di => $dd) {
-            foreach (Tagger::split_unpack($sv->conf->setting_data($dd[0]) ?? "") as $ti) {
-                $ltag = strtolower($ti[0]);
+            foreach (Tagger::split_unpack($sv->conf->setting_data($dd[0]) ?? "") as $tv) {
+                $ltag = strtolower($tv[0]);
                 $vs[$ltag][] = $di;
                 $m = $vs[$ltag];
                 if (count($m) === 2) {
-                    $sv->warning_at($vinfo[$m[0]][1], "<0>Tag ‘{$ti[0]}’ is also used for {$dd[2]}");
+                    $sv->warning_at($vinfo[$m[0]][1], "<0>Tag ‘{$tv[0]}’ is also used for {$dd[2]}");
                 }
                 if (count($m) > 1) {
-                    $sv->warning_at($dd[1], "<0>Tag ‘{$ti[0]}’ is also used for {$vinfo[$m[0]][2]}");
+                    $sv->warning_at($dd[1], "<0>Tag ‘{$tv[0]}’ is also used for {$vinfo[$m[0]][2]}");
                 }
             }
         }
diff --git a/src/sitype.php b/src/sitype.php
index ec815b90c..4fdfa5261 100644
--- a/src/sitype.php
+++ b/src/sitype.php
@@ -585,10 +585,12 @@ function json_examples(Si $si, SettingValues $sv) {
 
 class TagList_Sitype extends Sitype {
     use Data_Sitype;
-    /** @var int */
-    private $flags = Tagger::NOPRIVATE | Tagger::NOCHAIR | Tagger::NOVALUE;
-    /** @var ?float */
-    private $min_idx;
+    /** @var int
+     * @readonly */
+    public $flags = Tagger::NOPRIVATE | Tagger::NOCHAIR | Tagger::NOVALUE;
+    /** @var ?float
+     * @readonly */
+    public $min_idx;
     /** @param string $type
      * @param string $subtype */
     function __construct($type, $subtype) {