Repository navigation
Conversation
…task repo scan .@origin/main read the spec as a path: the revision pattern had no '/'. Allow it (no '..', '//', trailing '/'). repo tasks check and repo run hand a flushing logger to the runners so CI shows lines as they come. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #159 +/- ##
==========================================
+ Coverage 91.64% 91.65% +0.01%
==========================================
Files 222 222
Lines 27409 27418 +9
Branches 4508 4510 +2
==========================================
+ Hits 25118 25129 +11
+ Misses 1620 1619 -1
+ Partials 671 670 -1
🚀 New features to boost your workflow:
|
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 1dec01a. The PATH@REV fix works for origin/main and refs/heads/x, but it breaks the split for unpinned remote URLs with a user. Three points, inline.
What I ran, on a Linux review host at 1dec01a:
make ci-local: 24 passed, 2 failed, 6 skipped. The failures aretest_unbalanced_quote_in_hook_completes_quickly(1.04 s against a 1 s limit, a timing flake under load; the rest of the suite is 6724 passed) and pip-audit on virtualenv 21.14.7 (CVE-2026-113722) from the existing locks, fixed onmainby #152. Changed-line coverage passes.- Reproductions of
RepoSource.parseon the PR head and on the base. - A mutation run over the new tests: 4 of the mutants survive the new tests and the existing
test_repo*.pytests.
Checked, no issues
- Non-rev suffixes: dropping the
..,//or trailing-/check is each caught by the new parametrized test. - Wiring: removing
log=_progressor passinglog=printin either command is caught. - Changelog: the fragment is present.
| # suffixes. No "/" (so "user@host:path" URLs are not split) and no leading "-" | ||
| # (so a revision can never be read as a git option). | ||
| _REV_RE = re.compile(r"^[A-Za-z0-9._][A-Za-z0-9._^~-]*$") | ||
| _REV_RE = re.compile(r"^[A-Za-z0-9._][A-Za-z0-9._/^~-]*$") # "/" for origin/main, refs/heads/x |
There was a problem hiding this comment.
An unpinned ssh:// or https:// URL with a user is now split at its @. Allowing / in _REV_RE means the tail of ssh://git@github.com/o/r.git (github.com/o/r.git) passes _is_rev.
RepoSource.parse output:
- base:
ssh://git@github.com/o/r.git->SourceError: a remote source must be pinned to a revision: ...@<sha> - head:
ssh://git@github.com/o/r.git-> locationssh://git, revgithub.com/o/r.git - head:
https://user@github.com/o/r.git-> locationhttps://user, revgithub.com/o/r.git
The pinning check is skipped, and the user gets a clone failure for ssh://git instead of the pin hint. Pinned specs (...r.git@<sha>) still split at the last @ and are fine. The new test only covers git@github.com:o/r.git, where the : keeps the tail out of the revision.
Also, the comment above (lines 28-30) still says "No /" and "single-segment branch".
Suggestion: for a remote location, split only when the tail is a revision and the head still has a path after the @ (e.g. require that the part of head after the last @ contains a / or :), or reject a tail that looks like host/path (first segment contains a .). Add ssh://git@github.com/o/r.git and https://user@github.com/o/r.git to the tests, and update the comment.
There was a problem hiding this comment.
Fixed in 7ac3f2e: a head that is only a URL scheme plus user (ssh://git, https://user) is no longer split at the @, so an unpinned ssh://git@github.com/o/r.git gets the pin hint again; the comment above _REV_RE is updated. Tests: test_an_unpinned_remote_url_with_a_user_is_not_split_at_its_at_sign (fails on the previous tip), test_a_pinned_remote_url_with_a_user_splits_at_the_last_at_sign.
| env={"PATH": "", "PYTHONPATH": ":".join(sys.path)}) | ||
| try: | ||
| assert proc.stdout is not None | ||
| assert proc.stdout.readline() == "t1: OK\n" # would block 30 s if buffered |
There was a problem hiding this comment.
The flush test passes without the flush. It asserts only the line's content, and the child exits after 30 s, which flushes the buffer, so readline() returns the same text.
Mutation check: with print(line) instead of print(line, flush=True) the full tests/test_repo*.py run still passes (1233 passed), only about 30 s slower. A child that prints and sleeps 3 s under a pipe without a flush: readline() returns 't1: OK\n' after 3.0 s.
Suggestion: bound the wait. Read in a thread with join(5), or select.select([proc.stdout], [], [], 5), and fail if no line arrived before the child exits.
There was a problem hiding this comment.
Fixed in 7ac3f2e: the test waits with select for 5 s and fails if no line arrived. With print(line) instead of print(line, flush=True) test_progress_line_is_flushed_under_a_pipe now fails (5 s).
| def resolve_rev(git_dir: Path, rev: str) -> str: | ||
| """Full commit SHA for ``rev``; raises :class:`SourceError` if unknown.""" | ||
| if not _REV_RE.match(rev): | ||
| if not _is_rev(rev): |
There was a problem hiding this comment.
resolve_rev has no test. Its guard changed from _REV_RE.match to _is_rev, and nothing exercises it.
Mutation check: resolve_rev with the old regex, with _REV_RE.match only, and with the guard removed all pass the full tests/test_repo*.py run (1233 passed). a..b would then reach git rev-parse unchecked.
Suggestion: add a parametrized test that resolve_rev(git_dir, rev) raises SourceError ("invalid revision") for a..b, x//y, y/ and -x, and one positive case, origin/main, on a real repo. Add -x to the non-rev parametrization of parse too: the "no leading -" rule is pinned by no test.
There was a problem hiding this comment.
Added in 7ac3f2e: test_resolve_rev_refuses_what_is_no_revision (a..b, x//y, y/, -x) and test_resolve_rev_takes_a_branch_with_a_slash (origin/main on a real repo); -x is also in the non-rev parse parametrization. Guard swapped back to _REV_RE.match fails the first three cases.
…st resolve_rev Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
This branch has not been deployed
3 participants
Part of #41 (found while porting oneDAL).
Summary
RepoSource.parse("PATH@REV") # rpartition on the last '@' - _REV_RE = [A-Za-z0-9._^~-]+ # origin/main -> no rev -> "not a directory" + _is_rev: [A-Za-z0-9._/^~-]+, no '..', '//', trailing '/' cmd_repo_tasks_check -> run_check(..., log=print) + run_check(..., log=_progress) # print(line, flush=True) cmd_repo_run -> run_batch(..., log=_progress)Every
repocommand that takes a source goes throughRepoSource.parse;resolve_revuses the same check.PYTHONUNBUFFEREDinrepo-eval.ymlis left as is.Evidence
dokimi repo scan .@origin/main->local source is not a directory: .@origin/main; per-task check lines appear only at exit under a pipe.After:
test_source_parse_takes_a_rev_with_a_slash[origin/main|refs/heads/x|HEAD~1|...]pass (local and remote);test_source_parse_leaves_a_non_rev_suffix_in_the_pathkeepsa..betc. in the path;test_progress_line_is_flushed_under_a_pipereads the line from a piped subprocess before it exits.Merge Danger
Door: two-way
Blast Radius: narrow
A local directory literally named
x@origin/mainwould now be read asxatorigin/main; same trade-off that already applied tox@main.Checklist
ruff check .cleandokimi/changelog.d/🤖 Generated with Claude Code