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())