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"], {