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/urlCommandCatalog/harvestUrlCommands.py src/hg/utils/urlCommandCatalog/harvestUrlCommands.py
index 9b1d0f8fbcf..b2ba5c548bd 100755
--- src/hg/utils/urlCommandCatalog/harvestUrlCommands.py
+++ src/hg/utils/urlCommandCatalog/harvestUrlCommands.py
@@ -68,97 +68,129 @@
SKIP_DIRS = {"htdocs", "js", "tests", "expected", "input", "trackDb",
"makeDb/doc", "CVS", ".git", "python", "lowelab"}
# Mined for #define values in addition to everything under SCAN_ROOTS. A CGI
# routinely defines its own command names in its own .c or .h (DO_QUERY lives in
# hgIntegrator.c, the arg* names in hg/hubApi/dataApi.h), so the macro table has
# to cover the scanned trees too or half the names come out as {IDENT}.
MACRO_DIRS = ["inc", "hg/inc"]
# ---------------------------------------------------------------------------
# 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.
Two passes, because the tree defines names in terms of other names:
#define hgHub "hgHubConnect."
#define hgHubDo hgHub "do_"
#define hgHubDoClear hgHubDo "clear"
The concatenating form is handled by resolving right to left over several
rounds until nothing new appears.
+
+ The second return value is the reason this is not just a dict. A pooled
+ table answers for the whole tree, so a name that two CGIs define
+ differently gets whichever value the walk reached first, and the walk order
+ is the filesystem's: SEARCH_TERM is "hggw_term" in hgGateway and
+ "hgcd_term" in hgChooseDb, and a fresh clone and a working tree of the same
+ commit disagreed about which one hgGateway reads. Those names are reported
+ as ambiguous and resolve to {NAME} unless the file being scanned defines
+ them itself, which is the same call CONST_RE makes below and for the same
+ reason.
"""
macro = {}
+ conflict = set()
chains = []
# #define NAME "literal"
lit_re = re.compile(
r'^\s*#\s*define\s+([A-Za-z_]\w*)\s+'
r'("(?:[^"\\]|\\.)*")\s*(?:/[/*].*)?$')
# #define NAME OTHER, or NAME OTHER "suffix", or NAME "prefix" OTHER
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
# ---------------------------------------------------------------------------
# scanning
# ---------------------------------------------------------------------------
# static char *dbCgiName = "db"; Not every name is a #define: web.c holds db,
# org and clade this way, and they are the most-used URL params there are.
# Resolved per file and never pooled, because the same identifier means
# different things in different programs, and a pooled table let whichever file
@@ -225,102 +257,106 @@
def rel(path):
return os.path.relpath(path, ROOT)
def owner(path):
"""Which CGI or library a file belongs to, for grouping."""
r = rel(path)
parts = r.split(os.sep)
if len(parts) >= 2:
return os.sep.join(parts[:-1])
return r
-def find_exclude_vars(text, path, macro, localconst):
+def find_exclude_vars(text, path, macro, localconst, conflict=None):
"""Pull the members out of every excludeVars[] declaration in one file."""
out = []
for m in EXCLUDE_RE.finditer(text):
start = m.end()
depth = 1
i = start
instr = False
while i < len(text) and depth:
c = text[i]
if instr:
if c == "\\":
i += 2
continue
if c == '"':
instr = False
elif c == '"':
instr = True
elif c == "{":
depth += 1
elif c == "}":
depth -= 1
i += 1
body = text[start:i-1]
line = text.count("\n", 0, m.start()) + 1
# strip comments so a commented-out member is not harvested
body = re.sub(r'/\*.*?\*/', '', body, flags=re.S)
body = re.sub(r'//[^\n]*', '', body)
for tok in body.split(","):
- name = resolve(tok, macro, localconst)
+ name = resolve(tok, macro, localconst, conflict)
if name:
out.append((name, "%s:%d" % (rel(path), line)))
return out
-def find_matches(regex, text, path, macro, localconst, skip=()):
+def find_matches(regex, text, path, macro, localconst, skip=(), conflict=None):
out = []
for m in regex.finditer(text):
tok = m.group(1)
if tok in skip:
continue
- name = resolve(tok, macro, localconst)
+ name = resolve(tok, macro, localconst, conflict)
if not name:
continue
line = text.count("\n", 0, m.start()) + 1
out.append((name, "%s:%d" % (rel(path), line)))
return out
def harvest():
- macro = build_macros()
+ macro, conflict = build_macros()
found = {"excludeVars": collections.defaultdict(list),
"cgiReads": collections.defaultdict(list),
"cartRemoves": collections.defaultdict(list)}
for path in source_files():
try:
text = open(path, errors="replace").read()
except OSError:
continue
if "excludeVars" not in text and "cgi" not in text \
and "cartRemove" not in text:
continue
who = owner(path)
+ # 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)}
- for name, src in find_exclude_vars(text, path, macro, localconst):
+ localconst.update(local_defines(text))
+ for name, src in find_exclude_vars(text, path, macro, localconst,
+ conflict):
found["excludeVars"][who].append((name, src))
for name, src in find_matches(CGI_READ_RE, text, path, macro,
- localconst, CGI_READ_SKIP):
+ localconst, CGI_READ_SKIP, conflict):
found["cgiReads"][who].append((name, src))
for name, src in find_matches(CART_REMOVE_RE, text, path, macro,
- localconst):
+ localconst, conflict=conflict):
found["cartRemoves"][who].append((name, src))
return found, macro
# ---------------------------------------------------------------------------
# reporting
# ---------------------------------------------------------------------------
def dedupe(pairs):
"""Collapse repeats of the same name, keeping the first site seen."""
seen = {}
for name, src in pairs:
seen.setdefault(name, src)
return sorted(seen.items())