99c145df11c04f80820690fd67e9499f296b4d7d braney Sat Aug 1 12:58:39 2026 -0700 harvesters: resolve #defines per file, not pooled refs #37923 refs #37925 refs #37838 The first nightly run reported a new URL parameter, hggw_term, that had been in the tree since the hgGateway redesign. It was not new; the harvester had been answering the question differently in two checkouts of the same commit. SEARCH_TERM is "hggw_term" in hgGateway and "hgcd_term" in hgChooseDb. The #define table was pooled across the whole tree with setdefault, so the winner was whichever file the filesystem walk reached first, and hgGateway's reads came out as hgcd_term in my working tree and hggw_term in a fresh clone. For a nightly cron that means mail whenever a directory listing changes order, which is worse than no cron. So a name a file defines itself now wins, and a name the tree defines inconsistently resolves to {NAME} instead of to a guess. This is the call bdc473369e1 already made for char * constants, for the same reason and with the same tradeoff written up in the CONST_RE comment: an honest {ident} beats a confident wrong answer. Verified by harvesting both trees and diffing. Twelve names leave the URL baseline as a result, the gisaidTable and hgg_ prefix families, which were pooled values from sibling CGIs rather than reads in the file they were attributed to. Recovering those properly means following the #include chain to the header that defines them, which is not done here. The URL catalog carried hgt_tSearch twice, once correctly as the track search variable and once as hgGateway's search term, which was this bug showing up in the curated half. --check did not catch the duplicate because it only looks within a section. hgGateway now has hggw_term and hgChooseDb hgcd_term, both confirmed at their call sites and in their CGI's excludeVars. diff --git src/hg/utils/hgConfCatalog/harvestHgConf.py src/hg/utils/hgConfCatalog/harvestHgConf.py index 697820aeb9c..5a7a3703b7c 100755 --- src/hg/utils/hgConfCatalog/harvestHgConf.py +++ src/hg/utils/hgConfCatalog/harvestHgConf.py @@ -136,92 +136,120 @@ # same identifier means different things in different programs: hgTracks passes # a runtime `database` to cfgNamesWithPrefix, while docIdView.c has a # file-scope `char *database = "encpipeline_prod"`. Pooling them let one # program's constant answer for every other file, first one walked winning, and # put a hardcoded MySQL database name in the registry as an hg.conf setting. CONST_RE = re.compile( r'^[ \t]*(?:static[ \t]+)?(?:const[ \t]+)?char[ \t]*\*[ \t]*([A-Za-z_]\w*)' r'[ \t]*=[ \t]*("(?:[^"\\]|\\.)*")[ \t]*;', re.M) # --------------------------------------------------------------------------- # macro table # --------------------------------------------------------------------------- def build_macros(): - """Map every #define that resolves to a string literal, chasing aliases. + """(name -> literal, names defined inconsistently) over the whole tree. Same two-pass approach as harvestUrlCommands.py: the tree defines names in terms of other names, so concatenating forms are resolved right to left over several rounds until nothing new appears. + + And the same reason for the second return value: a pooled table gives a name + that two files define differently whichever value the filesystem walk + reached first, so the answer changes between two checkouts of the same + commit. Those names resolve to {NAME} unless the file being scanned defines + them itself. """ macro = {} + conflict = set() chains = [] lit_re = re.compile( r'^\s*#\s*define\s+([A-Za-z_]\w*)\s+' r'("(?:[^"\\]|\\.)*")\s*(?:/[/*].*)?$') cat_re = re.compile( r'^\s*#\s*define\s+([A-Za-z_]\w*)\s+' r'((?:(?:"(?:[^"\\]|\\.)*")|(?:[A-Za-z_]\w*))' r'(?:\s+(?:(?:"(?:[^"\\]|\\.)*")|(?:[A-Za-z_]\w*)))*)' r'\s*(?:/[/*].*)?$') piece_re = re.compile(r'"(?:[^"\\]|\\.)*"|[A-Za-z_]\w*') for fn in macro_files(): for line in open(fn, errors="replace"): m = lit_re.match(line) if m: - macro.setdefault(m.group(1), m.group(2)[1:-1]) + name, val = m.group(1), m.group(2)[1:-1] + if name in macro and macro[name] != val: + conflict.add(name) + macro.setdefault(name, val) continue m = cat_re.match(line) if m: chains.append((m.group(1), piece_re.findall(m.group(2)))) for _ in range(5): for name, pieces in chains: if name in macro: continue out = "" for piece in pieces: if piece.startswith('"'): out += piece[1:-1] elif piece in macro: out += macro[piece] + if piece in conflict: + conflict.add(name) # built on an ambiguous piece else: out = None break if out is not None: macro[name] = out - return macro + return macro, conflict + + +# #define NAME "literal" in the file being scanned. Its own definition is the +# one that file means, whatever the rest of the tree says. +LOCAL_DEFINE_RE = re.compile( + r'^[ \t]*#[ \t]*define[ \t]+([A-Za-z_]\w*)[ \t]+' + r'("(?:[^"\\]|\\.)*")[ \t]*(?:/[/*].*)?$', re.M) -def resolve(tok, macro, localconst=None): +def local_defines(text): + return {m.group(1): m.group(2)[1:-1] + for m in LOCAL_DEFINE_RE.finditer(text)} + + +def resolve(tok, macro, localconst=None, conflict=None): """Turn one C token into the name it stands for, or {ident} if unknown. - localconst is the file's own char * constants, which take precedence over - the shared #define table and must never be shared between files: see - CONST_RE for why. + localconst is the file's own char * constants and #defines, which take + precedence over the shared #define table and must never be shared between + files: see CONST_RE for why. conflict is the set of names the tree defines + inconsistently; without the file's own definition to go on, those are + {ident} rather than a guess. """ tok = tok.strip() if not tok: return None if tok.startswith('"') and tok.endswith('"') and len(tok) >= 2: return tok[1:-1] if tok == "NULL": return None if localconst and tok in localconst: return localconst[tok] + if conflict and tok in conflict: + return "{%s}" % tok if tok in macro: return macro[tok] if re.match(r'^[A-Za-z_]\w*$', tok): return "{%s}" % tok return None # --------------------------------------------------------------------------- # file walking # --------------------------------------------------------------------------- def macro_files(): """Every .h and .c that could hold a #define we need to resolve.""" for d in MACRO_DIRS: p = os.path.join(ROOT, d) @@ -340,122 +368,125 @@ Newlines are kept so text.count("\\n", 0, pos) is still the line number, which matters because a commented-out cfgOption call would otherwise be harvested as a live read. """ def blank(m): return re.sub(r'[^\n]', ' ', m.group(0)) text = re.sub(r'/\*.*?\*/', blank, text, flags=re.S) text = re.sub(r'//[^\n]*', blank, text) return text # --------------------------------------------------------------------------- # scanning # --------------------------------------------------------------------------- -def scan_file(path, macro, found): +def scan_file(path, macro, found, conflict=None): try: raw = open(path, errors="replace").read() except OSError: return if "cfgOption" not in raw and "cfgVal" not in raw: return # hgConfig.c defines the accessors; its own calls are the implementation, # not settings reads. Its cfgVal/cfgOption uses inside other functions are # still real, so only the definitions are skipped, by name, below. text = strip_comments(raw) + # The file's own #defines join its char * constants: both are what this + # file means, whatever the rest of the tree calls the same name. localconst = {m.group(1): m.group(2)[1:-1] for m in CONST_RE.finditer(text)} + localconst.update(local_defines(text)) who = owner(path) for m in CFG_CALL_RE.finditer(text): op = m.end() - 1 fn = func_name(text, op) spec = ACCESSORS.get(fn) if spec is None: continue args = split_args(text, op) if not args: continue line = text.count("\n", 0, m.start()) + 1 src = "%s:%d" % (rel(path), line) default = None if spec.get("defArg") is not None and len(args) > spec["defArg"]: default = args[spec["defArg"]].strip() if spec.get("twoPart"): if len(args) < 2: continue # cfgOption2/cfgOptionDefault2 are themselves defined in terms of # each other in hgConfig.c, passing their own prefix/suffix # parameters through. Those are the implementation, not a read. if args[0] in NAME_SKIP or args[1] in NAME_SKIP: continue - pre = resolve(args[0], macro, localconst) - suf = resolve(args[1], macro, localconst) + pre = resolve(args[0], macro, localconst, conflict) + suf = resolve(args[1], macro, localconst, conflict) if pre is None or suf is None: continue if pre.startswith("{"): # runtime profile name: record the suffix as a family member found["profiles"][suf].append((pre, src, fn)) else: found["reads"][who].append( {"name": "%s.%s" % (pre, suf), "src": src, "func": fn, "default": default}) continue idx = spec["nameArg"] if len(args) <= idx: continue tok = args[idx] if tok in NAME_SKIP: continue - name = resolve(tok, macro, localconst) + name = resolve(tok, macro, localconst, conflict) if not name: continue rec = {"name": name, "src": src, "func": fn, "default": default} if spec.get("envArg") is not None and len(args) > spec["envArg"]: - env = resolve(args[spec["envArg"]], macro, localconst) + env = resolve(args[spec["envArg"]], macro, localconst, conflict) if env: rec["env"] = env if spec.get("boolean"): rec["boolean"] = True if spec.get("required"): rec["required"] = True found["reads"][who].append(rec) for m in PREFIX_CALL_RE.finditer(text): if m.group(1) in NAME_SKIP: # cfgValsWithPrefix passing its own parameter to cfgNamesWithPrefix continue - name = resolve(m.group(1), macro, localconst) + name = resolve(m.group(1), macro, localconst, conflict) if name: line = text.count("\n", 0, m.start()) + 1 found["prefixScans"][name].append("%s:%d" % (rel(path), line)) def harvest(): - macro = build_macros() + macro, conflict = build_macros() found = {"reads": collections.defaultdict(list), "profiles": collections.defaultdict(list), "prefixScans": collections.defaultdict(list)} for path in source_files(): # The accessors themselves live here; their bodies read the config # hash directly and would otherwise show up as reads of {name}. if rel(path) in ("hg/lib/hgConfig.c", "hg/inc/hgConfig.h"): - scan_file(path, macro, found) + scan_file(path, macro, found, conflict) continue - scan_file(path, macro, found) + scan_file(path, macro, found, conflict) return found, macro def all_reads(found): """Every read record, flattened.""" for recs in found["reads"].values(): for rec in recs: yield rec def by_name(found): """Collapse reads to one record per name, keeping every call site.""" out = {} for rec in all_reads(found): d = out.setdefault(rec["name"], {