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/cartTrackVarCatalog/harvestCartVars.py src/hg/utils/cartTrackVarCatalog/harvestCartVars.py
index fc0b674e40b..dac403f0309 100755
--- src/hg/utils/cartTrackVarCatalog/harvestCartVars.py
+++ src/hg/utils/cartTrackVarCatalog/harvestCartVars.py
@@ -57,59 +57,86 @@
# Everything that draws or configures a track. hgc and hgTables are in the
# default list because they read and write per-track vars too, which is easy
# to forget.
DEFAULT_DIRS = "hg/lib,hg/hgTracks,hg/hgTrackUi,hg/cgilib,hg/hgc,hg/hgTables"
# Extra trees mined for #define values only, not scanned for call sites.
MACRO_DIRS = ["inc", "lib", "hg/inc"]
# ---------------------------------------------------------------------------
# macro table
# ---------------------------------------------------------------------------
def build_macros(dirs):
- """Map every #define that resolves to a string literal, chasing aliases."""
+ """(name -> literal, names defined inconsistently) over the scanned dirs.
+
+ The second return value exists because a pooled table gives a name that two
+ files define differently whichever value the directory listing 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, the same call the per-file char * constants make.
+ """
macro = {}
+ conflict = set()
chains = []
def_re = re.compile(
r'^\s*#\s*define\s+([A-Za-z_][A-Za-z0-9_]*)\s+'
r'("(?:[^"\\]|\\.)*")\s*(?:/[/*].*)?$')
chain_re = re.compile(
r'^\s*#\s*define\s+([A-Za-z_][A-Za-z0-9_]*)\s+'
r'([A-Za-z_][A-Za-z0-9_]*)\s*(?:/[/*].*)?$')
for d in list(dirs) + MACRO_DIRS:
p = os.path.join(ROOT, d)
if not os.path.isdir(p):
continue
for fn in os.listdir(p):
if not fn.endswith((".h", ".c")):
continue
for line in open(os.path.join(p, fn), errors="replace"):
m = def_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 = chain_re.match(line)
if m:
chains.append((m.group(1), m.group(2)))
for _ in range(5):
for a, b in chains:
if a not in macro and b in macro:
macro[a] = macro[b]
- return macro
+ if b in conflict:
+ conflict.add(a) # alias of an ambiguous name
+ 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 local_defines(text):
+ # CONST_RE next door captures inside the quotes, so strip them here to
+ # match: localconst holds bare values.
+ return {m.group(1): m.group(2)[1:-1]
+ for m in LOCAL_DEFINE_RE.finditer(text)}
# ---------------------------------------------------------------------------
# C parsing, such as it is
# ---------------------------------------------------------------------------
CONST_RE = re.compile(
r'(?:static\s+)?(?:const\s+)?char\s*\*\s*([A-Za-z_][A-Za-z0-9_]*)\s*=\s*'
r'"((?:[^"\\]|\\.)*)"')
# Kent style puts function bodies at column 0, so a bare "safef(...)" at
# column 0 looks like a function definition. Requiring a return type plus
# whitespace (or a star) before the name is what keeps that from matching.
FUNCDEF_RE = re.compile(
r'^(?:static\s+)?(?:INLINE\s+)?(?:const\s+)?'
@@ -146,45 +173,52 @@
depth += 1
elif c in ")]}":
depth -= 1
if c == "," and depth == 0:
out.append(cur.strip())
cur = ""
i += 1
continue
cur += c
i += 1
if cur.strip():
out.append(cur.strip())
return out
-def resolve(arg, localconst, macro):
- """Turn an argument into the string it evaluates to, if we can."""
+def resolve(arg, localconst, macro, conflict=None):
+ """Turn an argument into the string it evaluates to, if we can.
+
+ conflict is the set of names the tree defines inconsistently; without the
+ file's own definition to go on, those stay {NAME} rather than taking
+ whichever value was seen first.
+ """
arg = arg.strip()
if re.fullmatch(r'"((?:[^"\\]|\\.)*)"', arg):
return arg[1:-1]
toks = re.findall(r'"(?:[^"\\]|\\.)*"|[A-Za-z_][A-Za-z0-9_]*', arg)
plain = re.sub(r'"(?:[^"\\]|\\.)*"|[A-Za-z_][A-Za-z0-9_]*|\s+', '', arg)
if plain == "" and toks:
# nothing but literals and identifiers, i.e. C string concatenation
vals = []
for t in toks:
if t.startswith('"'):
vals.append(t[1:-1])
elif t in localconst:
vals.append(localconst[t])
+ elif conflict and t in conflict:
+ vals.append("{" + t + "}")
elif t in macro:
vals.append(macro[t])
else:
vals.append("{" + t + "}")
return "".join(vals)
return "EXPR:" + re.sub(r'\s+', ' ', arg)[:60]
def match_close(txt, i):
"""Index of the paren that closes the one at i."""
depth, j, instr = 0, i, False
while j < len(txt):
c = txt[j]
if instr:
if c == "\\":
@@ -206,121 +240,126 @@
def enclosing_functions(lines):
"""Map 1-based line number to the name of the function containing it."""
encl = [None] * (len(lines) + 2)
cur = None
for idx, line in enumerate(lines):
if (line and not line[0].isspace()
and not line.startswith(("#", "/", "*", "}", "{"))):
m = FUNCDEF_RE.match(line)
if m:
cur = m.group(1)
encl[idx + 1] = cur
return encl
-def scan_file(fp, rel, macro):
+def scan_file(fp, rel, macro, conflict=None):
"""Return one record per track-scoped cart name found in one file."""
txt = open(fp, errors="replace").read()
+ # 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) for m in CONST_RE.finditer(txt)}
+ localconst.update(local_defines(txt))
encl = enclosing_functions(txt.split("\n"))
starts = [0]
for i, ch in enumerate(txt):
if ch == "\n":
starts.append(i + 1)
def lineno(pos):
lo, hi = 0, len(starts) - 1
while lo < hi:
mid = (lo + hi + 1) // 2
if starts[mid] <= pos:
lo = mid
else:
hi = mid - 1
return lo + 1
recs = []
for m in CTH_RE.finditer(txt):
i = m.end() - 1
args = split_args(txt[i+1:match_close(txt, i)])
if len(args) >= 4:
ln = lineno(m.start())
- recs.append(dict(var=resolve(args[3], localconst, macro),
+ recs.append(dict(var=resolve(args[3], localconst, macro,
+ conflict),
file=rel, line=ln, func=encl[ln], how="cth"))
for m in FMT_RE.finditer(txt):
i = m.end() - 1
args = split_args(txt[i+1:match_close(txt, i)])
ln = lineno(m.start())
fi = None
for k, a in enumerate(args):
if a.strip().startswith('"'):
fi = k
break
if fi is None:
continue
- fmt = resolve(args[fi], localconst, macro)
+ fmt = resolve(args[fi], localconst, macro, conflict)
rest = args[fi+1:]
mm = re.match(r'^%s([._])(.*)$', fmt or "")
if not mm:
continue
sep, tail = mm.group(1), mm.group(2)
if "%" not in tail and tail:
recs.append(dict(var=sep+tail, file=rel, line=ln,
func=encl[ln], how="fmtlit"))
elif tail == "%s" and len(rest) >= 2:
# "%s.%s", track, SUFFIX -> the suffix is the SECOND vararg
- v = resolve(rest[1], localconst, macro)
+ v = resolve(rest[1], localconst, macro, conflict)
if v and not v.startswith("EXPR:"):
recs.append(dict(var=sep+v, file=rel, line=ln,
func=encl[ln], how="fmt"))
elif tail == "%s.%s" and len(rest) >= 3:
- v1 = resolve(rest[1], localconst, macro)
- v2 = resolve(rest[2], localconst, macro)
+ v1 = resolve(rest[1], localconst, macro, conflict)
+ v2 = resolve(rest[2], localconst, macro, conflict)
if not v1.startswith("EXPR:") and not v2.startswith("EXPR:"):
recs.append(dict(var=sep+v1+"."+v2, file=rel, line=ln,
func=encl[ln], how="fmt3"))
return recs
# ---------------------------------------------------------------------------
# entry point for the catalog next door
# ---------------------------------------------------------------------------
def harvest(dirs=None, quiet=False):
"""Scan the tree and return the raw records.
cartTrackVarCatalog.py --reconcile imports this, so the scan has one
definition rather than one here and a second one written out by hand.
"""
dirs = dirs or [d.strip() for d in DEFAULT_DIRS.split(",") if d.strip()]
- macro = build_macros(dirs)
+ macro, conflict = build_macros(dirs)
files = []
for d in dirs:
p = os.path.join(ROOT, d)
if not os.path.isdir(p):
sys.exit("no such directory: %s" % p)
for fn in sorted(os.listdir(p)):
if fn.endswith(".c"):
files.append(os.path.join(p, fn))
records = []
for fp in files:
- records.extend(scan_file(fp, os.path.relpath(fp, ROOT), macro))
+ records.extend(scan_file(fp, os.path.relpath(fp, ROOT), macro,
+ conflict))
if not quiet:
print("scanned %d files in %d dirs, %d macros, %d records"
% (len(files), len(dirs), len(macro), len(records)),
file=sys.stderr)
return records
def resolved(records):
"""name -> first file:line, for the names the scan resolved to a literal.
An EXPR: or {ident} record marks a name built at run time, which is signal
for a person reading the harvester output but cannot be compared against a
catalog of literal names, so it is dropped here.
"""