fix(precommit): smarter PDL schema version bump detection (datahub-project#17459)

Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Author: chakru-r

.github/scripts/bump_schema_versions.py

@@ -66,16 +66,66 @@ def detect_default_branch() -> str:
     return "master"
 
 
-def get_changed_pdl_files(base_branch: str) -> list[str]:
-    """Return repo-relative paths of PDL files that differ from base_branch."""
+def get_merge_base(remote_ref: str) -> str:
+    """Return the merge-base commit SHA between HEAD and remote_ref."""
+    result = subprocess.run(
+        ["git", "merge-base", "HEAD", remote_ref],
+        capture_output=True,
+        text=True,
+    )
+    if result.returncode != 0:
+        print(
+            f"Error finding merge-base with {remote_ref}: {result.stderr}",
+            file=sys.stderr,
+        )
+        sys.exit(1)
+    return result.stdout.strip()
+
+
+def get_base_pdl_changes(merge_base: str, remote_ref: str) -> list[str]:
+    """Return PDL files that changed on remote_ref since merge_base.
+
+    Used to detect whether any of the PDL files touched on this branch have
+    also moved on the base branch — if so, the branch must be rebased before
+    schemaVersion can be bumped correctly.
+    """
     try:
         result = subprocess.run(
             [
                 "git",
                 "diff",
                 "--name-only",
                 "--diff-filter=ACM",
-                base_branch,
+                merge_base,
+                remote_ref,
+                "--",
+                "*.pdl",
+            ],
+            capture_output=True,
+            text=True,
+            check=True,
+        )
+        return [f.strip() for f in result.stdout.strip().splitlines() if f.strip()]
+    except subprocess.CalledProcessError as e:
+        print(f"Error getting base branch PDL changes: {e.stderr}", file=sys.stderr)
+        sys.exit(1)
+
+
+def get_changed_pdl_files(base_ref: str) -> list[str]:
+    """Return repo-relative paths of PDL files that differ between base_ref and the working tree.
+
+    Compares base_ref against the working tree (not HEAD) so that uncommitted
+    changes are included — this function runs inside a pre-commit hook.
+    base_ref may be a branch name, remote tracking ref, or commit SHA.
+    """
+    try:
+        result = subprocess.run(
+            [
+                "git",
+                "diff",
+                "--name-only",
+                "--diff-filter=ACM",
+                base_ref,
                 "--",
                 "*.pdl",
             ],
@@ -544,14 +594,33 @@ def main() -> int:
     )
     args = parser.parse_args()
 
-    directly_changed = get_changed_pdl_files(args.base_branch)
+    remote_ref = f"refs/remotes/origin/{args.base_branch}"
+    merge_base = get_merge_base(remote_ref)
+
+    directly_changed = get_changed_pdl_files(merge_base)
 
     if not directly_changed:
         print("No changed PDL files found.")
         return 0
 
+    # Check whether any PDL files changed on this branch also changed on the
+    # base branch since divergence. If so, the branch must be rebased or
+    # merged before schemaVersion can be bumped correctly.
+    # Unrelated PDL changes on the base branch do not block.
+    base_pdl_changes = get_base_pdl_changes(merge_base, remote_ref)
+    conflicting = sorted(set(directly_changed) & set(base_pdl_changes))
+    if conflicting:
+        files_list = "\n".join(f"  {f}" for f in conflicting)
+        print(
+            f"ERROR: The following PDL file(s) also changed on {args.base_branch} "
+            f"since this branch diverged. Please merge or rebase from "
+            f"{args.base_branch} first:\n{files_list}",
+            file=sys.stderr,
+        )
+        return 1
+
     if args.verbose:
-        print(f"Comparing against branch: {args.base_branch}")
+        print(f"Comparing against branch: {args.base_branch} (merge-base: {merge_base[:12]})")
         print(f"Found {len(directly_changed)} directly changed PDL file(s):")
         for f in directly_changed:
             print(f"  {f}")
@@ -581,7 +650,9 @@ def main() -> int:
         path = Path(filepath)
         current_content = path.read_text(encoding="utf-8")
 
-        base_content = get_file_at_branch(filepath, args.base_branch)
+        # Use remote_ref (refs/remotes/origin/…) rather than the bare branch
+        # name — local checkouts of the base branch may not exist.
+        base_content = get_file_at_branch(filepath, remote_ref)
         base_version = get_schema_version(base_content) if base_content else 0
         current_version = get_schema_version(current_content)
         new_version = base_version + 1

.github/scripts/test/test_bump_schema_versions.py

@@ -629,14 +629,15 @@ def test_unrelated_same_namespace_record_is_not_a_dependency(tmp_path, monkeypat
 
 
 # ---------------------------------------------------------------------------
-# detect_default_branch
+# detect_default_branch / get_merge_base / get_base_pdl_changes
 # ---------------------------------------------------------------------------
 
 
-def _make_proc(returncode: int, stdout: str = "") -> MagicMock:
+def _make_proc(returncode: int, stdout: str = "", stderr: str = "") -> MagicMock:
     m = MagicMock()
     m.returncode = returncode
     m.stdout = stdout
+    m.stderr = stderr
     return m
 
 
@@ -658,6 +659,34 @@ def test_detect_default_branch_fallback_to_master(monkeypatch):
     assert bsv.detect_default_branch() == "master"
 
 
+def test_get_merge_base_returns_sha(monkeypatch):
+    monkeypatch.setattr(subprocess, "run",
+                        lambda *a, **kw: _make_proc(0, "abc123def456\n"))
+    assert bsv.get_merge_base("refs/remotes/origin/acryl-main") == "abc123def456"
+
+
+def test_get_merge_base_failure_exits(monkeypatch):
+    monkeypatch.setattr(subprocess, "run",
+                        lambda *a, **kw: _make_proc(1, stderr="not a git repo"))
+    with pytest.raises(SystemExit):
+        bsv.get_merge_base("refs/remotes/origin/acryl-main")
+
+
+def test_get_base_pdl_changes_returns_files(monkeypatch):
+    monkeypatch.setattr(subprocess, "run",
+                        lambda *a, **kw: _make_proc(0, "a/Foo.pdl\nb/Bar.pdl\n"))
+    result = bsv.get_base_pdl_changes("deadbeef", "refs/remotes/origin/acryl-main")
+    assert result == ["a/Foo.pdl", "b/Bar.pdl"]
+
+
+def test_get_base_pdl_changes_failure_exits(monkeypatch):
+    monkeypatch.setattr(subprocess, "run",
+                        lambda *a, **kw: (_ for _ in ()).throw(
+                            subprocess.CalledProcessError(1, "git", stderr="err")))
+    with pytest.raises(SystemExit):
+        bsv.get_base_pdl_changes("deadbeef", "refs/remotes/origin/acryl-main")
+
+
 # ---------------------------------------------------------------------------
 # Version bump scenarios (via main())
 #
@@ -668,17 +697,23 @@ def test_detect_default_branch_fallback_to_master(monkeypatch):
 # ---------------------------------------------------------------------------
 
 
-def _run_main(tmp_path, monkeypatch, changed_files, base_content_by_path):
+def _run_main(tmp_path, monkeypatch, changed_files, base_content_by_path, *,
+              base_pdl_changes=None):
     """
     Run main() with filesystem and git calls mocked out.
 
-    changed_files        — list of Path objects treated as "changed"
+    changed_files        — list of Path objects treated as "changed" on this branch
     base_content_by_path — dict mapping str(path) → content string on base branch,
                            or None to simulate a new file not present on base
+    base_pdl_changes     — list of str paths changed on the base branch since
+                           divergence (default: empty — no conflict)
     """
     monkeypatch.setenv("PDL_ROOTS", str(tmp_path))
     monkeypatch.setattr(sys, "argv", ["bump_schema_versions.py", "--base-branch", "master"])
-    monkeypatch.setattr(bsv, "get_changed_pdl_files", lambda _branch: [str(f) for f in changed_files])
+    monkeypatch.setattr(bsv, "get_merge_base", lambda _ref: "deadbeef")
+    monkeypatch.setattr(bsv, "get_base_pdl_changes",
+                        lambda _base, _ref: base_pdl_changes or [])
+    monkeypatch.setattr(bsv, "get_changed_pdl_files", lambda _ref: [str(f) for f in changed_files])
     monkeypatch.setattr(bsv, "get_file_at_branch",
                         lambda path, _branch: base_content_by_path.get(path))
     return bsv.main()
@@ -730,14 +765,48 @@ def test_version_bump_new_file_stays_at_1(tmp_path, monkeypatch):
     assert bsv.get_schema_version(aspect.read_text()) == 1
 
 
+def test_conflicting_pdl_on_base_branch_exits_with_error(tmp_path, monkeypatch):
+    # Base ref is current AND the same PDL changed on both branches → block at stage 2
+    base_content = aspect_pdl("myAspect")
+    aspect = write_pdl(tmp_path, "com/linkedin/dataset/MyAspect.pdl", base_content)
+
+    rc = _run_main(
+        tmp_path, monkeypatch,
+        changed_files=[aspect],
+        base_content_by_path={str(aspect): base_content},
+        base_pdl_changes=[str(aspect)],  # same file changed on base
+    )
+
+    assert rc == 1
+    assert bsv.get_schema_version(aspect.read_text()) == 1  # untouched
+
+
+def test_unrelated_base_pdl_changes_do_not_block(tmp_path, monkeypatch):
+    # A different PDL changed on the base branch — should not block this branch's bump
+    base_content = aspect_pdl("myAspect")
+    aspect = write_pdl(tmp_path, "com/linkedin/dataset/MyAspect.pdl", base_content)
+
+    rc = _run_main(
+        tmp_path, monkeypatch,
+        changed_files=[aspect],
+        base_content_by_path={str(aspect): base_content},
+        base_pdl_changes=["com/linkedin/other/Unrelated.pdl"],  # different file on base
+    )
+
+    assert rc == 0
+    assert bsv.get_schema_version(aspect.read_text()) == 2  # bumped normally
+
+
 def test_version_bump_dry_run_does_not_write(tmp_path, monkeypatch):
     base_content = aspect_pdl("myAspect")
     aspect = write_pdl(tmp_path, "com/linkedin/dataset/MyAspect.pdl", base_content)
     original = aspect.read_text()
 
     monkeypatch.setenv("PDL_ROOTS", str(tmp_path))
     monkeypatch.setattr(sys, "argv", ["bump_schema_versions.py", "--base-branch", "master", "--dry-run"])
-    monkeypatch.setattr(bsv, "get_changed_pdl_files", lambda _branch: [str(aspect)])
+    monkeypatch.setattr(bsv, "get_merge_base", lambda _ref: "deadbeef")
+    monkeypatch.setattr(bsv, "get_base_pdl_changes", lambda _base, _ref: [])
+    monkeypatch.setattr(bsv, "get_changed_pdl_files", lambda _ref: [str(aspect)])
     monkeypatch.setattr(bsv, "get_file_at_branch", lambda path, _branch: base_content)
 
     rc = bsv.main()