From 8385378cdb38932521a25068782c61ec1435e6c7 Mon Sep 17 00:00:00 2001 From: John Reeves Date: Thu, 20 Aug 2026 20:24:51 +1000 Subject: [PATCH] fix(import,update): stop writing .dvc metadata that disagrees with dvc (0.24.0) Four defects from #182, all in the metadata dt records for repo imports. outs.size: 0 on multi-GB imports. get_file_size_from_cache built only the v3 object path, so a half-migrated remote -- v2 objects at / alongside v3 ones -- reported no size for every v2 resident, and the caller folded each miss into the total as zero. Checkout of the same objects worked throughout, because that path already falls back to v2, which is why the data was right and only the size was wrong. Sizing now goes through a shared cache_ops.object_size (both layouts, source remote then local cache); an object that cannot be found means no size field at all rather than a silent undercount. Verified against visium-raw: the 7 affected imports size to 4.18-10.60 GB, nothing unsized. outs.size left describing the previous hash. `dvc list --size` against a repo URL reports no sizes -- a .dir manifest holds only md5 and relpath, so the only place a size exists is the object -- and a None size was read as "leave it alone", retaining a figure 3x the real directory. Sizes are now totalled by stat'ing the objects, and a size belonging to the old hash is deleted rather than kept. deps.repo.rev left contradicting rev_lock, so the next plain `dvc update` resolved rev and rolled the import back. --rev now records the spec in rev and the resolved 40-char sha in rev_lock (it was writing branch names into rev_lock); without --rev, an import tracking a branch advances to that branch's tip; a pin that contradicts the new lock is dropped, loudly. Rebuilt .dir omitting git-tracked files. DVC's repo filesystem excludes only *.dvc, dvc.yaml, dvc.lock and .dvcignore, so importing a directory that is not itself an out hashes the git-tracked files there -- including the .gitignore DVC generated -- into the payload. dt cannot, so its manifest diverged, and was then published into a shared upstream remote. dt import and dt update now refuse such a path and name the files, pushing nothing. dt init seeds .dvcignore with .gitignore so a repo's own bookkeeping stops leaking into importers' data, and dt doctor flags repos that lack it. With that pattern in place dt import now matches dvc import byte-for-byte on such a directory. Two smaller bugs found in passing: the .dvc files inside a directory of outs were treated as payload candidates (DVC excludes them too), and a directory containing exactly one file was recorded as a single-file import, losing its .dir and nfiles. Co-Authored-By: Claude --- docs/doctor.md | 4 +- docs/import.md | 33 +++ docs/init.md | 16 +- docs/update.md | 46 +++- dt/__init__.py | 2 +- dt/cache_ops.py | 34 +++ dt/doctor.py | 38 ++++ dt/import_data.py | 188 ++++++++++----- dt/init.py | 43 ++++ dt/update.py | 405 ++++++++++++++++++++++++++------- dt/utils.py | 148 +++++++++++- pyproject.toml | 2 +- tests/unit/test_doctor.py | 33 +++ tests/unit/test_import_data.py | 177 ++++++++++++++ tests/unit/test_init.py | 35 +++ tests/unit/test_update.py | 227 ++++++++++++++++++ tests/unit/test_utils.py | 121 ++++++++++ 17 files changed, 1416 insertions(+), 136 deletions(-) diff --git a/docs/doctor.md b/docs/doctor.md index 31a8b63..d2ce529 100644 --- a/docs/doctor.md +++ b/docs/doctor.md @@ -27,6 +27,7 @@ dt doctor -v # Verbose output, includes dvc doctor | Archived remotes | No configured DVC remote has been archived (carries an `ARCHIVED.yaml` signpost) | | Git repo | The current directory is inside a git repository | | DVC repo | The current directory is inside a DVC repository | +| dvcignore | `.dvcignore` excludes `.gitignore`, so this repo's generated ignore files cannot be hashed into the payload of anyone importing a directory from it ([why](init.md#why-dvcignore-starts-with-gitignore)) | ### Verbose-only checks @@ -55,8 +56,9 @@ DVC Tools version: 0.1.0 ✓ No archived remotes detected ✓ In git repository (/scratch/a56/me/my-project) ✓ In DVC repository (/scratch/a56/me/my-project) +✓ .dvcignore excludes .gitignore files -All 10 checks passed. +All 11 checks passed. ``` With issues: diff --git a/docs/import.md b/docs/import.md index d74e6f2..514fae1 100644 --- a/docs/import.md +++ b/docs/import.md @@ -20,6 +20,39 @@ Imports files or directories from another DVC repository by: `dt import` first tries a local-cache workflow that does not require network access to remote object storage. If the source repository has no locally-accessible remote/cache, it falls back to `dvc import`. +## The path must be a single DVC out + +`dt import` builds its manifest from object hashes, so every file under the +imported path needs one. That holds when the path is itself a DVC out (or lives +inside one). It does not hold for a plain directory that merely *contains* outs: +DVC's repo filesystem presents git-tracked files there as part of the same tree, +and `dvc import` hashes them from the git worktree into the payload — including +the `.gitignore` files DVC generated in that directory. + +Given such a path, `dt import` refuses and names the offending files rather than +writing a manifest that disagrees with `dvc import` in hash, `nfiles` and +`size`. The fixes, in order of preference: + +1. Import the out itself, e.g. `data/annotations/in_house` rather than + `data/annotations`. +2. If the only stray files are DVC's own generated ignore files, add + `.gitignore` to the **source** repo's `.dvcignore` and commit + (`dt init` seeds this; `dt doctor` flags repos that lack it). The payload + then contains only real data and `dt import` matches `dvc import` exactly. +3. Make the path a single out upstream (`dvc add `). +4. Use plain `dvc import`, which hashes the git-tracked files too — at the cost + of a payload that includes them, and that your own remote cannot serve + (DVC excludes repo-import stages from `dvc push`). + +## Recorded size + +`size` comes from stat'ing the cache objects, in both the `files/md5//` v3 +layout and the legacy `/` v2 one — a remote part-way through migration holds +some objects in each. If any object cannot be found, `dt import` records **no** +`size` and says so on stderr, rather than counting the miss as zero: a `size: 0` +on a multi-GB import reads as an empty payload and silently undercounts every +storage estimate downstream. + ## Options - `--out, -o `: Destination path for imported files (default: basename of source path) diff --git a/docs/init.md b/docs/init.md index c8cc70f..390948b 100644 --- a/docs/init.md +++ b/docs/init.md @@ -15,10 +15,24 @@ my-project/ │ ├── config # DVC settings (tracked) │ ├── config.local # Local DVC settings (not tracked) │ └── ... -├── .dvcignore # DVC ignore patterns +├── .dvcignore # DVC ignore patterns, seeded with `.gitignore` └── .gitignore # Updated with DVC patterns ``` +### Why `.dvcignore` starts with `.gitignore` + +DVC writes a `.gitignore` next to everything it tracks. Its repo filesystem +excludes only `*.dvc`, `dvc.yaml`, `dvc.lock` and `.dvcignore` when another repo +reads through it — so `dvc import `, where `some/dir` is +not itself an out, hashes those generated ignore files and folds them into the +importer's payload, `nfiles` and `size`. One pattern stops this repo's +bookkeeping from becoming someone else's data. + +DVC still creates and maintains those `.gitignore` files; it just stops treating +them as content. `dt doctor` flags repos that predate this, and adding the +pattern later is safe — but note it changes the hash, `nfiles` and `size` of any +directory import of a path that contains one. + ## Usage ```bash diff --git a/docs/update.md b/docs/update.md index 3bde874..934cf1f 100644 --- a/docs/update.md +++ b/docs/update.md @@ -165,6 +165,21 @@ dt update imported/dir.dvc `dt update` does not check files out into the workspace — run `dvc checkout` afterwards if you need the workspace copies linked. +## Imports of a path that is not a single out + +Step 5 needs a hash for every file under the path. A plain directory that merely +*contains* outs also contains git-tracked files, which the source repo records +no hash for — including the `.gitignore` files DVC generated there. `dvc import` +hashes those from the git worktree and counts them; `dt update` cannot, so its +rebuilt `.dir` would differ from DVC's in hash, `nfiles` and `size`, and step 7 +would publish that difference into a *shared* upstream remote as an object no +DVC operation would ever produce. + +`dt update` therefore refuses such a target, names the files, and pushes +nothing. See [dt import](import.md#the-path-must-be-a-single-dvc-out) for the +fixes — most often adding `.gitignore` to the source repo's `.dvcignore`, after +which dt and dvc agree exactly. + ## Metadata population `dt update` produces first-class `.dvc` files with complete metadata: @@ -188,7 +203,36 @@ outs: size: 52428800 # File size (50 MiB) ``` -This enables `dt du` to report accurate sizes and file counts. Note that size information is only available if the source repository's `.dvc` files also contain size metadata. +This enables `dt du` to report accurate sizes and file counts. + +### Where `size` comes from + +`dvc list --size` against a repository *URL* usually reports no sizes at all: a +`.dir` manifest records only an md5 and a relpath per file, so the only place a +size actually exists is the object itself. When the listing is silent, +`dt update` sizes each object by stat'ing it in the source repository's remote +(if that remote is on this filesystem) or in the local cache — both the +`files/md5//` v3 layout and the legacy `/` v2 one. + +If even one object cannot be sized, `dt update` writes **no** `size` field +rather than a partial total, and removes any `size` left over from the previous +hash. An absent size means "not known"; a stale or partial one is a false +statement that `dt du` and every downstream storage estimate will believe. +Re-run `dt update` once the objects are locally readable, or `dvc update` to let +DVC record it. + +### `rev` and `rev_lock` + +`deps.repo.rev` is the revision *spec* you track (a branch, tag, or pinned +commit) and `rev_lock` is its resolution. `dt update` keeps the two consistent: + +- `--rev ` records the spec in `rev` and the resolved 40-char + commit in `rev_lock`. +- Without `--rev`, an import that tracks a branch advances `rev_lock` to the tip + of *that branch*, not to whatever the clone has checked out. +- A `rev` pinned to a commit that the new `rev_lock` contradicts is removed, + with a message. Left in place, the next plain `dvc update` would resolve `rev` + and roll the import back to the commit you just updated away from. ## Import detection diff --git a/dt/__init__.py b/dt/__init__.py index 48fa9c3..67e64dd 100644 --- a/dt/__init__.py +++ b/dt/__init__.py @@ -1,3 +1,3 @@ """DVC Tools - Convenient tools for working with DVC in HPC environments.""" -__version__ = "0.23.0" +__version__ = "0.24.0" diff --git a/dt/cache_ops.py b/dt/cache_ops.py index 92ba698..3b3c258 100644 --- a/dt/cache_ops.py +++ b/dt/cache_ops.py @@ -300,6 +300,40 @@ def find_source_file( return None +def object_size(md5: str, *cache_roots: Optional[str]) -> Optional[int]: + """Size of a cache object, searched across roots and both layouts. + + The object *is* the file content, so its own size is authoritative -- no + need to ask the source repo. Uses :func:`find_source_file` so a remote in + the legacy v2 layout (or a half-migrated one holding some objects at + ``/`` and others under ``files/md5/``) answers as readily as a v3 + one. That fallback is the whole point: sizing only the v3 path against a + mixed remote misses silently, and a miss that is then treated as zero + understates a multi-GB import (issue #182). + + Args: + md5: The MD5 hash (with optional .dir suffix). + *cache_roots: Cache/remote roots to search, in priority order. + None/empty entries are skipped so callers can pass optional paths. + + Returns: + Size in bytes, or None if the object was not found in any root. + """ + for root in cache_roots: + if not root: + continue + path = find_source_file(md5, Path(root)) + if path is None: + continue + try: + return path.stat().st_size + except OSError: + # Vanished or unreadable between the lookup and the stat -- try + # the next root rather than reporting a size we did not observe. + continue + return None + + def populate_cache_file( md5: str, source_cache: str, diff --git a/dt/doctor.py b/dt/doctor.py index 6f58ec7..6aece5f 100644 --- a/dt/doctor.py +++ b/dt/doctor.py @@ -521,6 +521,43 @@ def check_dvc_repo() -> DiagnosticResult: ) +def check_dvcignore() -> DiagnosticResult: + """Check that .dvcignore keeps this repo's .gitignore files out of imports. + + DVC writes a ``.gitignore`` beside everything it tracks, and its repo + filesystem does not exclude those files -- so anyone importing a *directory* + from this repo that is not itself an out gets our bookkeeping hashed into + their payload, ``nfiles`` and ``size`` (issue #182). ``dt init`` seeds the + pattern; repos created before that need it adding. + """ + from . import utils + + in_repo, root = check_in_dvc_repo() + if not in_repo or root is None: + return DiagnosticResult( + "dvcignore", True, "Not in a DVC repository (skipped)" + ) + + dvcignore = Path(root) / '.dvcignore' + patterns = set() + if dvcignore.exists(): + patterns = {l.strip() for l in dvcignore.read_text().splitlines()} + + if utils.DVCIGNORE_GITIGNORE_PATTERN in patterns: + return DiagnosticResult( + "dvcignore", True, ".dvcignore excludes .gitignore files" + ) + + return DiagnosticResult( + "dvcignore", False, + ".dvcignore does not exclude .gitignore files", + "DVC's own generated .gitignore files can be hashed into the payload " + "of anyone importing a directory from this repo. Add a '.gitignore' " + "line to .dvcignore and commit it. Note this changes the hash, nfiles " + "and size of any such directory import." + ) + + def check_network() -> DiagnosticResult: """Check network connectivity (for dt doctor output).""" has_network = check_network_connectivity(timeout=2.0) @@ -625,6 +662,7 @@ def run_diagnostics(verbose: bool = False) -> list[DiagnosticResult]: # Repository context checks results.append(check_git_repo()) results.append(check_dvc_repo()) + results.append(check_dvcignore()) # Environment checks (may be slow) if verbose: diff --git a/dt/import_data.py b/dt/import_data.py index 96782a8..48c2b45 100644 --- a/dt/import_data.py +++ b/dt/import_data.py @@ -523,26 +523,71 @@ def list_files( def get_file_size_from_cache(cache_path: str, md5: str) -> Optional[int]: """Get file size by looking at the cached file. - + + Delegates to :func:`cache_ops.object_size`, which checks the v3 layout + (``files/md5//``) *and* the legacy v2 one (``/``). + This used to look only under ``files/md5``, which meant a mixed-layout + remote -- v2 objects that a migration left behind, alongside v3 ones -- + reported no size at all for the v2 residents, while the checkout of the + same objects succeeded because that path already fell back to v2 (#182). + Args: - cache_path: Path to the cache directory. - md5: MD5 hash of the file. - + cache_path: Path to the cache/remote root. + md5: MD5 hash of the file (with optional .dir suffix). + Returns: File size in bytes, or None if not found. """ - # Handle .dir suffix - hash_value = md5.replace('.dir', '') - cache_file = Path(cache_path) / 'files' / 'md5' / hash_value[:2] / hash_value[2:] - - # Check for .dir file - if md5.endswith('.dir'): - cache_file = Path(str(cache_file) + '.dir') - - if cache_file.exists(): - return cache_file.stat().st_size - - return None + return cache_ops.object_size(md5, cache_path) + + +def _primary_cache_root() -> Optional[str]: + """Root of this repo's DVC cache, i.e. the dir that holds ``files/md5``. + + ``utils.get_cache_dir()`` hands back the ``files/md5`` level, which double- + nests if passed to anything that appends the layout itself. + """ + primary_cache = utils.get_cache_dir() + if not primary_cache: + return None + primary_cache_str = str(primary_cache) + if primary_cache_str.endswith(('/files/md5', '\\files\\md5')): + return str(Path(primary_cache_str).parent.parent) + return primary_cache_str + + +def _sum_object_sizes( + md5s: List[str], + *cache_roots: Optional[str], +) -> Tuple[int, List[str]]: + """Add up the sizes of cache objects, reporting the ones we could not find. + + Returns the running total and the hashes that could not be sized, so the + caller can tell a genuinely empty payload from an unanswered question. The + distinction matters: silently folding a miss into the total as zero is what + wrote ``size: 0`` onto multi-GB imports (#182). + """ + total = 0 + missing: List[str] = [] + for md5 in md5s: + size = cache_ops.object_size(md5, *cache_roots) + if size is None: + missing.append(md5) + else: + total += size + return total, missing + + +def _warn_unsized(missing: List[str], total_count: int, cache_path: str) -> None: + """Explain that ``outs.size`` is being left out, and why.""" + print( + f" WARNING: could not determine the size of {len(missing)} of " + f"{total_count} object(s) under {cache_path} " + f"(e.g. {missing[0][:12]}...). Recording no size rather than an " + f"undercount; run 'dvc status'/'dt du' against the checked-out data " + f"for the real figure.", + file=sys.stderr, + ) def compute_dir_hash(entries: List[Dict[str, str]]) -> str: @@ -640,19 +685,21 @@ def create_dvc_file( dest_path: Path, name: str, md5: str, - size: int, + size: Optional[int], nfiles: Optional[int] = None, repo_url: Optional[str] = None, repo_path: Optional[str] = None, rev_lock: Optional[str] = None, ) -> Path: """Create a .dvc file. - + Args: dest_path: Destination directory for the .dvc file. name: Name of the output (file or directory name). md5: MD5 hash (with .dir suffix for directories). - size: Size in bytes. + size: Size in bytes, or None to omit the field. Omitting says "not + known"; a wrong number says something false, which is worse -- + downstream storage sums believe it. nfiles: Number of files (for directories). repo_url: Source repository URL (for imports). repo_path: Path within source repository (for imports). @@ -687,8 +734,9 @@ def create_dvc_file( # md5, size, nfiles (dirs only), hash, path. out: Dict[str, Any] = { 'md5': md5, - 'size': size, } + if size is not None: + out['size'] = size if nfiles is not None: out['nfiles'] = nfiles out['hash'] = 'md5' @@ -910,28 +958,64 @@ def import_data( if not files: raise ImportError(f"No files found at {path}") - + + # Refuse a path whose tree mixes git-tracked files with DVC outs. Those + # files have no hash to put in a manifest, and dvc import would include + # them -- so anything we build here disagrees with dvc. Until this check + # existed the None md5 simply travelled on until something dereferenced it + # and raised AttributeError (#182). + unhashed = utils.unhashed_listing_paths(files) + if unhashed: + raise ImportError(utils.describe_mixed_tree( + unhashed, path, repository, action='import' + )) + + # Drop DVC's own bookkeeping files. `dvc list` reports the .dvc files + # inside a directory of outs, but `dvc import` excludes them from the + # payload -- and they carry no hash, so they cannot go in a manifest. + files = [ + f for f in files + if not utils.is_dvc_metadata_file(f.get('path') or '') + ] + if not files: + raise ImportError(f"No DVC-tracked files found at {path}") + if verbose: print(f"Found {len(files)} file(s)") - - # Track whether this is a directory or single file import - is_directory = not (len(files) == 1 and not files[0].get('isdir', False)) + + # Track whether this is a directory or single file import. A lone entry is + # a single file only when it *is* the requested path -- a directory holding + # one file also lists one entry, but as 'name/file', and must still get a + # .dir manifest. Same rule as update._compute_source_hash. + is_single_file = ( + len(files) == 1 + and not files[0].get('isdir', False) + and (files[0].get('path') or '') == Path(path).name + ) + is_directory = not is_single_file root_hash = None # Will be set to dir_hash or single file md5 + # Sizes come from stat'ing the objects themselves. Look in the source + # remote first, then this repo's own cache -- a partly-migrated remote may + # be missing an object the local cache already holds. + size_roots = (cache_path, _primary_cache_root()) + # Step 5: Create .dvc file (and .dir file if needed) if not is_directory: # Single file import file_info = files[0] root_hash = file_info['md5'] - - # Get size from cache - size = get_file_size_from_cache(cache_path, root_hash) + + # Get size from cache. None means "not found", which stays None: a + # zero here would read as an empty import (#182). + size = cache_ops.object_size(root_hash, *size_roots) if size is None: - size = 0 # Fallback - + _warn_unsized([root_hash], 1, cache_path) + if verbose: - print(f"Importing single file: {root_hash} ({size} bytes)") - + size_str = f"{size} bytes" if size is not None else "size unknown" + print(f"Importing single file: {root_hash} ({size_str})") + dvc_file = create_dvc_file( dest_path=out_path.parent, name=out_path.name, @@ -948,28 +1032,32 @@ def import_data( # Build entries for .dir file entries = [] - total_size = 0 - + for f in files: if f.get('isdir', False): continue # Skip directory entries - + md5 = f['md5'] relpath = f['path'] - + entries.append({ 'md5': md5, 'relpath': relpath, }) - - # Get size from cache - file_size = get_file_size_from_cache(cache_path, md5) - if file_size: - total_size += file_size - + if not entries: raise ImportError(f"No files found in {path}") - + + # Total the object sizes. If any object cannot be found we record no + # size at all: a partial sum is indistinguishable from the truth and + # anything totalling recorded sizes would quietly undercount (#182). + total_size, unsized = _sum_object_sizes( + [e['md5'] for e in entries], *size_roots + ) + if unsized: + _warn_unsized(unsized, len(entries), cache_path) + total_size = None + # Create .dir file in the cache dir_hash, dir_size = create_dir_file(entries, cache_path) root_hash = dir_hash # Already includes .dir suffix @@ -1006,17 +1094,11 @@ def import_data( if verbose: print(f"Populating cache for {dvc_file}...") - # Populate primary cache with links from source cache. - # get_cache_dir() returns the .../files/md5 path; strip that suffix - # so populate_cache_file (which appends files/md5 for v3 layout) - # doesn't double-nest the path. - primary_cache = utils.get_cache_dir() - if primary_cache: - primary_cache_str = str(primary_cache) - if primary_cache_str.endswith(('/files/md5', '\\files\\md5')): - primary_cache_root = str(Path(primary_cache_str).parent.parent) - else: - primary_cache_root = primary_cache_str + # Populate primary cache with links from source cache. The root is the + # dir holding files/md5, since populate_cache_file appends the layout + # itself and would otherwise double-nest the path. + primary_cache_root = _primary_cache_root() + if primary_cache_root: # First, add the .dir file or single file hash to primary cache if root_hash: populate_cache_file( diff --git a/dt/init.py b/dt/init.py index 765bc91..c2ec0ad 100644 --- a/dt/init.py +++ b/dt/init.py @@ -136,6 +136,40 @@ def init_dvc(repo_path: Path, verbose: bool = True) -> bool: return True +def init_dvcignore(repo_path: Path, verbose: bool = True) -> bool: + """Seed .dvcignore so this repo's .gitignore files stay out of its data. + + DVC writes a ``.gitignore`` beside everything it tracks. When someone + imports a *directory* from this repo that is not itself an out, DVC's repo + filesystem hands them the git-tracked files too -- including those + generated ignore files -- and hashes them into the importer's payload, + inflating its ``nfiles`` and ``size`` with our bookkeeping (issue #182). + One pattern prevents it, for every future importer. + + Safe on an existing repo: appends only if the pattern is absent, and DVC + keeps maintaining the ignore files either way. + + Returns: + True if .dvcignore was created or appended to. + """ + dvcignore, changed = utils.ensure_dvcignore(repo_path) + + if changed and verbose: + print(f"Ignoring .gitignore files in {dvcignore.name} " + f"(keeps them out of imports of this repo)") + + if changed and get_dvc_autostage(repo_path): + subprocess.run( + ['git', 'add', str(dvcignore)], + cwd=repo_path, + capture_output=True, + ) + if verbose: + print(" Auto-staged .dvcignore") + + return changed + + def install_dvc_hooks(repo_path: Path, verbose: bool = True) -> None: """Install dt git hooks (replaces dvc install). @@ -300,6 +334,15 @@ def init_project( if verbose: print(f"Warning: {e}") + # Step 2c: .dvcignore, so our .gitignore files never become someone + # else's data (see init_dvcignore). + if not no_dvc: + try: + init_dvcignore(repo_path, verbose=verbose) + except OSError as e: + if verbose: + print(f"Warning: could not write .dvcignore: {e}") + # Step 3: DVC Tools directory (.dt) init_dt_directory(repo_path, verbose=verbose) diff --git a/dt/update.py b/dt/update.py index 07b0160..846b2bb 100644 --- a/dt/update.py +++ b/dt/update.py @@ -20,6 +20,7 @@ import yaml from dvc.utils.serialize import dump_yaml +from . import cache_ops from . import dvc_deps from . import dvc_lock from . import find as find_mod @@ -41,6 +42,7 @@ class ImportInfo: locked_rev: str current_hash: Optional[str] # Current outs.md5 (may be None) is_directory: bool # True if hash ends with .dir + rev_spec: Optional[str] = None # deps.repo.rev as written (branch/tag/sha) @dataclass @@ -54,6 +56,9 @@ class SourceChanges: diff_summary: str # Human-readable summary new_path: Optional[str] = None # If deleted but moved, the new path diff_error: Optional[str] = None # Error message if diff failed + # Git-tracked paths inside the import that the source repo does not hash. + # Their presence means no hash we compute can match what dvc records. + mixed_tree: Optional[List[str]] = None # ============================================================================= @@ -86,6 +91,7 @@ def _parse_import_info(dvc_path: Path) -> Optional[ImportInfo]: locked_rev=ref.locked_rev, current_hash=ref.md5, is_directory=ref.is_directory, + rev_spec=ref.rev, ) @@ -102,12 +108,65 @@ def _get_head_rev(clone_path: Path) -> str: return result.stdout.strip() +def _is_sha_like(rev: str) -> bool: + """True if ``rev`` looks like a commit sha rather than a branch or tag. + + A sha is immutable, so it pins; a branch name is a moving target that a + ``rev_lock`` is expected to resolve. The two need opposite treatment when + ``rev_lock`` advances, and this is the only signal available from the + ``.dvc`` file alone. + """ + if not rev or len(rev) < 7 or len(rev) > 40: + return False + return all(c in '0123456789abcdefABCDEF' for c in rev) + + +def _resolve_rev(clone_path: Path, rev: str) -> Optional[str]: + """Resolve a git revision spec to a full commit sha in the clone. + + Tries the spec as given, then under ``origin/`` -- the tmp clones track + remote branches, so a bare branch name usually only exists as + ``origin/``. + + Returns: + The full 40-char sha, or None if the spec does not resolve. + """ + for candidate in (rev, f'origin/{rev}'): + result = subprocess.run( + ['git', 'rev-parse', '--verify', '--quiet', f'{candidate}^{{commit}}'], + cwd=clone_path, + capture_output=True, + text=True, + ) + sha = result.stdout.strip() + if result.returncode == 0 and sha: + return sha + return None + + +def _tracked_tip(clone_path: Path, rev_spec: Optional[str], verbose: bool = False) -> str: + """Current tip of the revision this import tracks. + + ``dvc update`` advances ``rev_lock`` to the tip of ``deps.repo.rev``, not + to whatever the clone happens to have checked out. Honouring the recorded + spec keeps ``rev`` and ``rev_lock`` consistent; ignoring it is how the two + fields end up asserting different revisions (issue #182). + """ + if rev_spec and not _is_sha_like(rev_spec): + resolved = _resolve_rev(clone_path, rev_spec) + if resolved: + return resolved + if verbose: + print(f" Recorded rev '{rev_spec}' does not resolve in the clone; using HEAD") + return _get_head_rev(clone_path) + + def _compute_source_hash( repo_url: str, path: str, rev: str, verbose: bool = False, -) -> Tuple[Optional[str], int]: +) -> Tuple[Optional[str], int, List[str]]: """Compute the DVC hash of source data at a revision. Lists the path at ``rev`` and reconstructs the hash exactly the way @@ -125,29 +184,34 @@ def _compute_source_hash( verbose: Print progress messages. Returns: - Tuple of (hash, nfiles). Returns (None, 0) if the path lists - nothing or cannot be listed at this revision (e.g. it was moved - or deleted). + Tuple of (hash, nfiles, unhashed_paths). Returns (None, 0, []) if the + path lists nothing or cannot be listed at this revision (e.g. it was + moved or deleted). ``unhashed_paths`` is non-empty when the path mixes + git-tracked files with DVC outs, in which case no hash computed from + hashes alone can match what ``dvc import`` recorded. """ try: - entries = _get_file_listing(repo_url, path, rev, verbose=verbose) + entries, unhashed = _get_file_listing(repo_url, path, rev, verbose=verbose) except UpdateError as e: if verbose: print(f" Could not list {path} at {rev[:12]}...: {e}") - return None, 0 + return None, 0, [] + + if unhashed: + return None, 0, unhashed if not entries: - return None, 0 + return None, 0, [] # Single file: exactly one entry whose relpath is just the filename. source_filename = Path(path).name if len(entries) == 1 and entries[0]['relpath'] == source_filename: - return entries[0]['md5'], 1 + return entries[0]['md5'], 1, [] # Directory: hash matches outs.md5 (the .dir manifest md5). manifest_content = utils.build_dir_manifest(entries) dir_hash = f"{hashlib.md5(manifest_content).hexdigest()}.dir" - return dir_hash, len(entries) + return dir_hash, len(entries), [] def _check_source_changes( @@ -204,7 +268,23 @@ def _check_source_changes( diff_error="No recorded hash in .dvc file", ) - head_hash, _ = _compute_source_hash(repo_url, path, head_rev, verbose=verbose) + head_hash, _, unhashed = _compute_source_hash( + repo_url, path, head_rev, verbose=verbose + ) + + if unhashed: + # Nothing to compare: dt's hash of this path can never equal the one + # dvc recorded, because dvc counted the git-tracked files too. Report + # it rather than the "data changed" it would otherwise look like. + return SourceChanges( + has_changes=True, + head_rev=head_rev, + added=0, + modified=0, + deleted=0, + diff_summary="Path mixes git-tracked files with DVC outs", + mixed_tree=unhashed, + ) if head_hash is None: # Path lists nothing at HEAD - data may have moved or been deleted. @@ -269,19 +349,23 @@ def _get_file_listing( path: str, revision: str, verbose: bool = False, -) -> List[Dict[str, any]]: +) -> Tuple[List[Dict[str, any]], List[str]]: """Get file listing with hashes and sizes from source repo using dvc list. - + Args: repo_url: URL of source repository. path: Path within repo to list. revision: Git revision to list at. verbose: Print progress messages. - + Returns: - List of dicts with 'md5', 'relpath', and 'size' keys. - Size may be None if not available from source. - + Tuple of (entries, unhashed_paths). Entries are dicts with 'md5', + 'relpath' and optionally 'size'. ``unhashed_paths`` holds the files the + source repo records no hash for -- git-tracked files in a directory + that is not itself an out. They are reported rather than dropped: + dropping them silently produced a manifest that disagreed with dvc's + (issue #182). + Raises: UpdateError: If listing fails. """ @@ -309,7 +393,9 @@ def _get_file_listing( except json.JSONDecodeError as e: raise UpdateError(f"Failed to parse dvc list output: {e}") - # Filter to files with hashes + # Split into hashed entries and the git-tracked files the source repo + # records no hash for. + unhashed = utils.unhashed_listing_paths(files) entries = [] for f in files: if f.get('isdir'): @@ -324,19 +410,89 @@ def _get_file_listing( if f.get('size') is not None: entry['size'] = f['size'] entries.append(entry) - + if verbose: total_size = sum(e.get('size', 0) or 0 for e in entries) if total_size > 0: print(f" Found {len(entries)} files ({utils.format_size(total_size, human_readable=True)})") else: print(f" Found {len(entries)} files") - - return entries + if unhashed: + print(f" {len(unhashed)} git-tracked file(s) carry no hash") + + return entries, unhashed +def _cache_root(cache: Optional[str]) -> Optional[str]: + """Root of the cache to work in: the dir that contains ``files/md5``.""" + if cache: + return cache + cache_dir = utils.get_cache_dir() + if not cache_dir: + return None + base = str(cache_dir) + if base.endswith(('/files/md5', '\\files\\md5')): + return str(Path(base).parent.parent) + return base + + +def _find_source_remote(repo_url: str, verbose: bool = False) -> Optional[str]: + """Path to the source repo's remote, if it is on this filesystem. + + Used both to size objects and to publish a rebuilt manifest, so look it up + once. Returns None for cloud remotes or when the lookup fails -- neither is + fatal, it just means sizes have to come from elsewhere. + """ + from . import remote as remote_mod + try: + local_remote = remote_mod.find_local_remote_from_repo(repo_url) + except Exception as e: + if verbose: + print(f" Could not resolve source remote: {e}") + return None + return local_remote[1] if local_remote else None + + +def _total_size_from_entries( + entries: List[Dict[str, any]], + *cache_roots: Optional[str], + verbose: bool = False, +) -> Optional[int]: + """Total bytes for a manifest, or None if any entry cannot be accounted for. + + ``dvc list --size`` against a repo *URL* generally reports no sizes at all: + a ``.dir`` manifest records only md5 and relpath, so the only place a size + exists is the object itself. Hence the fallback -- stat the object in a + locally-readable cache or remote, which is authoritative because the object + *is* the content. + + Returns None if even one entry is unsized, rather than a partial total: an + undercount reads as fact downstream, where a missing size reads as unknown + (issue #182). + """ + total = 0 + unsized = 0 + for entry in entries: + size = entry.get('size') + if size is None: + size = cache_ops.object_size(entry['md5'], *cache_roots) + if size is None: + unsized += 1 + else: + total += size + + if unsized: + if verbose: + print( + f" Could not size {unsized} of {len(entries)} object(s) " + f"from the source listing or any local cache" + ) + return None + return total + + def _write_dir_to_cache( manifest_content: bytes, dest_cache: str, @@ -379,20 +535,30 @@ def _update_dvc_file( new_rev: Optional[str] = None, new_path: Optional[str] = None, size: Optional[int] = None, + clear_size: bool = False, nfiles: Optional[int] = None, + new_rev_spec: Optional[str] = None, verbose: bool = False, ) -> bool: """Update the .dvc file with new hash, size, nfiles, and optionally new revision/path. - + Args: dvc_path: Path to the .dvc file. new_hash: New outs.md5 hash (with .dir suffix if directory). new_rev: New deps.repo.rev_lock (None to keep current). new_path: New deps.path if source path has changed (None to keep current). - size: Total size in bytes (None to omit). + size: Total size in bytes (None to leave the recorded size alone). + clear_size: Delete ``outs.size`` instead of leaving a value that + belongs to the previous hash. Set this when the data changed but + its size could not be determined -- a stale size is a false + statement that propagates downstream, where a missing one is + merely unknown (issue #182). nfiles: Number of files for directories (None to omit). + new_rev_spec: New deps.repo.rev, i.e. the revision *spec* the caller + asked for. When None, a recorded spec that would contradict + ``new_rev`` is dropped rather than left to roll the import back. verbose: Print progress messages. - + Returns: True if file was modified. """ @@ -423,7 +589,17 @@ def _update_dvc_file( old_str = utils.format_size(old_size, True) if old_size else 'None' new_str = utils.format_size(size, True) print(f" Updated size: {old_str} → {new_str}") - + elif clear_size and 'size' in outs: + # The size we hold describes the old hash. Keeping it would + # assert something untrue about the new data. + old_size = outs.pop('size') + modified = True + print( + f" Removed outs.size ({utils.format_size(old_size, True)}): " + f"could not determine the size of the new data. " + f"Run 'dvc update {dvc_path}' or 'dt du' to record it." + ) + # Update nfiles if provided (for directories) if nfiles is not None: old_nfiles = outs.get('nfiles') @@ -438,15 +614,41 @@ def _update_dvc_file( deps = data.get('deps', []) for dep in deps: repo = dep.get('repo', {}) - if repo: - old_rev = repo.get('rev_lock') or repo.get('rev', '') - if old_rev != new_rev: - repo['rev_lock'] = new_rev + if not repo: + continue + + old_rev = repo.get('rev_lock') or repo.get('rev', '') + if old_rev != new_rev: + repo['rev_lock'] = new_rev + modified = True + if verbose: + old_str = f"{old_rev[:12]}..." if old_rev else "None" + print(f" Updated rev_lock: {old_str} → {new_rev[:12]}...") + + # Keep deps.repo.rev consistent with the lock. rev is the spec + # and rev_lock its resolution, so a rev pinned to a different + # commit makes the file assert two revisions at once -- and the + # next `dvc update` believes rev, rolling the import back to + # the commit we just moved away from (issue #182). + old_spec = repo.get('rev') + if new_rev_spec: + if old_spec != new_rev_spec: + repo['rev'] = new_rev_spec modified = True if verbose: - old_str = f"{old_rev[:12]}..." if old_rev else "None" - print(f" Updated rev_lock: {old_str} → {new_rev[:12]}...") - + print(f" Updated rev: {old_spec or 'None'} → {new_rev_spec}") + elif (old_spec and _is_sha_like(old_spec) + and not new_rev.lower().startswith(old_spec.lower())): + del repo['rev'] + modified = True + print( + f" Removed pinned rev ({old_spec}): it no longer matches " + f"rev_lock {new_rev[:12]}..., and would have rolled this " + f"import back on the next 'dvc update'. The import now " + f"tracks the source's default branch; re-pin with " + f"'dt update --rev ' if that is not what you want." + ) + # Update deps.path if source path has changed if new_path: deps = data.get('deps', []) @@ -632,16 +834,33 @@ def update( source_path = info.path path_changed = False + # The spec to record in deps.repo.rev. An explicit --rev is what the + # caller now tracks; 'HEAD' is not a spec worth recording (it means + # "wherever the source is now"), so it leaves rev to be reconciled + # against the new lock. + rev_spec = rev if (rev and rev != 'HEAD') else None + if rev: - # Explicit revision specified - target_rev = rev - if target_rev == 'HEAD': + # Explicit revision specified. Record the spec, but lock to a full + # sha: rev_lock is the *resolution* of the spec, so writing a + # branch name there would leave the import unpinned. + if rev == 'HEAD': target_rev = _get_head_rev(clone_path) + else: + resolved = _resolve_rev(clone_path, rev) + if not resolved: + results.append((str(target_path), False, + f"Revision '{rev}' does not resolve in {info.repo_url}")) + continue + target_rev = resolved if not show_status: - print(f" Target rev: {target_rev[:12]}... (specified)") + spec_note = f" ({rev} → sha)" if rev_spec and rev_spec != target_rev else "" + print(f" Target rev: {target_rev[:12]}...{spec_note} (specified)") else: - # Smart detection: check for changes - head_rev = _get_head_rev(clone_path) + # Smart detection: check for changes. Advance along the revision + # this import tracks, which is deps.repo.rev when it names a + # branch or tag -- not simply whatever the clone has checked out. + head_rev = _tracked_tip(clone_path, info.rev_spec, verbose=verbose) if rebuild: # --rebuild: skip change detection, just rebuild at locked rev @@ -665,6 +884,19 @@ def update( current_hash=info.current_hash, verbose=verbose ) + # A path that mixes git-tracked files with DVC outs cannot be + # rebuilt at all, so say that instead of reporting a change. + if changes.mixed_tree: + if not show_status: + print() + print(utils.describe_mixed_tree( + changes.mixed_tree, info.path, info.repo_url, + action='rebuild', + )) + results.append((str(target_path), False, + "Path mixes git-tracked files with DVC outs")) + continue + # Handle diff errors (e.g., unknown revision) if changes.diff_error: if not show_status: @@ -745,30 +977,52 @@ def update( # Get file listing from source try: - entries = _get_file_listing( + entries, unhashed = _get_file_listing( info.repo_url, source_path, target_rev, verbose=verbose ) except UpdateError as e: results.append((str(target_path), False, str(e))) continue - + + # Reached via --rev/--rebuild, which skip change detection. Refuse + # rather than build a manifest that disagrees with dvc's -- and, since + # the .dir is pushed upstream, plant that disagreement in a shared + # remote where no dvc operation would ever produce it (#182). + if unhashed: + if not show_status: + print() + print(utils.describe_mixed_tree( + unhashed, source_path, info.repo_url, action='rebuild' + )) + results.append((str(target_path), False, + "Path mixes git-tracked files with DVC outs")) + continue + if not entries: # No entries found - error results.append((str(target_path), False, "No files found at source path")) continue + # Where sizes can be read from: the source repo's remote if it is on + # this filesystem, then the cache we are writing into. + cache_base = _cache_root(cache) + source_remote = _find_source_remote(info.repo_url, verbose=verbose) + size_roots = (source_remote, cache_base) + # Check if this is a single file or directory import # Single file: exactly 1 entry where path is just the filename source_filename = Path(source_path).name is_single_file = ( - len(entries) == 1 and + len(entries) == 1 and entries[0]['relpath'] == source_filename ) - + if is_single_file: # Single file import - use the md5 directly (no .dir) file_hash = entries[0]['md5'] - file_size = entries[0].get('size') # May be None + file_size = _total_size_from_entries( + entries, *size_roots, verbose=verbose + ) # May be None if the object is not locally readable if verbose: size_str = f" ({utils.format_size(file_size, True)})" if file_size else "" print(f" Single file import: {file_hash[:12]}...{size_str}") @@ -782,11 +1036,15 @@ def update( ) if needs_update: - new_rev = target_rev if target_rev != info.locked_rev else None + # Also pass the rev when only the spec changed, so an explicit + # --rev is recorded even if it resolves to the current lock. + new_rev = target_rev if (target_rev != info.locked_rev or rev_spec) else None _update_dvc_file( target_path, file_hash, new_rev, new_path=source_path if path_changed else None, size=file_size, + clear_size=file_size is None and file_hash != info.current_hash, + new_rev_spec=rev_spec, verbose=verbose ) if path_changed: @@ -802,54 +1060,47 @@ def update( results.append((str(target_path), True, "Already up to date")) continue - # Directory import - compute totals from entries + # Directory import nfiles = len(entries) - total_size = sum(e.get('size', 0) or 0 for e in entries) - # Only set total_size if we have size data for at least some files - has_size_data = any(e.get('size') is not None for e in entries) - + + if not cache_base: + results.append((str(target_path), False, "DVC cache not configured")) + continue + + # Total the payload. None means "could not be determined", which is + # written as an absent size rather than a stale or partial one. + total_size = _total_size_from_entries(entries, *size_roots, verbose=verbose) + # Build .dir manifest manifest_content = utils.build_dir_manifest(entries) - - # Get cache path - use explicit or primary - if cache: - cache_base = cache - else: - cache_dir = utils.get_cache_dir() - if not cache_dir: - results.append((str(target_path), False, "DVC cache not configured")) - continue - - cache_base = str(cache_dir) - if cache_base.endswith('/files/md5') or cache_base.endswith('\\files\\md5'): - cache_base = str(Path(cache_base).parent.parent) - + # Write to cache dir_hash, dir_file = _write_dir_to_cache(manifest_content, cache_base, verbose) - + # Update .dvc file with size, nfiles, and path metadata - new_rev = target_rev if target_rev != info.locked_rev else None + # Also pass the rev when only the spec changed, so an explicit + # --rev is recorded even if it resolves to the current lock. + new_rev = target_rev if (target_rev != info.locked_rev or rev_spec) else None _update_dvc_file( target_path, dir_hash, new_rev, new_path=source_path if path_changed else None, - size=total_size if has_size_data else None, + size=total_size, + clear_size=total_size is None and dir_hash != info.current_hash, nfiles=nfiles, + new_rev_spec=rev_spec, verbose=verbose ) - + # Always push to source remote so fetch can find it - from . import remote as remote_mod - try: - local_remote = remote_mod.find_local_remote_from_repo(info.repo_url) - if local_remote: - remote_path = Path(local_remote[1]) - _push_dir_to_remote(dir_file, remote_path, dir_hash, verbose) - except Exception as e: - if verbose: - print(f" Warning: Could not push to source remote: {e}") - + if source_remote: + try: + _push_dir_to_remote(dir_file, Path(source_remote), dir_hash, verbose) + except Exception as e: + if verbose: + print(f" Warning: Could not push to source remote: {e}") + updated_targets.append(str(target_path)) - size_str = f", {utils.format_size(total_size, True)}" if has_size_data else "" + size_str = f", {utils.format_size(total_size, True)}" if total_size is not None else "" if path_changed: results.append((str(target_path), True, f"Moved to {source_path} ({nfiles} files{size_str})")) else: diff --git a/dt/utils.py b/dt/utils.py index b65e959..40eac4a 100644 --- a/dt/utils.py +++ b/dt/utils.py @@ -10,7 +10,7 @@ import socket import subprocess from pathlib import Path -from typing import Any, Dict, List, Optional, Tuple, Union +from typing import Any, Dict, Iterable, List, Optional, Tuple, Union from dvc.repo import Repo from dvc_data.hashfile.hash import file_md5 as _dvc_file_md5 @@ -1376,6 +1376,152 @@ def md5_bytes(data: bytes) -> str: return hashlib.md5(data).hexdigest() +# The pattern that keeps DVC's own bookkeeping out of other repos' payloads. +# See ensure_dvcignore for why. +DVCIGNORE_GITIGNORE_PATTERN = '.gitignore' + +_DVCIGNORE_HEADER = ( + "# DVC's repo filesystem presents git-tracked files and DVC outs as one\n" + "# tree, and excludes only *.dvc, dvc.yaml, dvc.lock and .dvcignore from it.\n" + "# So `dvc import `, where some/dir is not itself an\n" + "# out, hashes the .gitignore files DVC generated here and folds them into\n" + "# the importer's payload, nfiles and size. Ignoring them keeps this repo's\n" + "# bookkeeping out of other people's data.\n" +) + + +def ensure_dvcignore(repo_path: Optional[Path] = None) -> Tuple[Path, bool]: + """Ensure .dvcignore excludes .gitignore files. + + DVC writes a ``.gitignore`` next to everything it tracks, then -- when + another repo imports a *directory* that is not itself an out -- hashes that + generated file as if it were data. The importer's manifest, ``nfiles`` and + ``size`` all silently include it. A single ``.dvcignore`` pattern here + prevents that for every future importer, and costs nothing: DVC keeps + maintaining the ignore files, it just stops treating them as content. + + Appends rather than replaces, and is a no-op if the pattern is already + present, so it is safe to run on an existing repo. + + Args: + repo_path: Repository root. Defaults to :func:`find_project_root`. + + Returns: + Tuple of (path to .dvcignore, True if the file was changed). + """ + if repo_path is None: + repo_path = find_project_root() + dvcignore = Path(repo_path) / '.dvcignore' + + existing = dvcignore.read_text() if dvcignore.exists() else '' + if DVCIGNORE_GITIGNORE_PATTERN in {l.strip() for l in existing.splitlines()}: + return dvcignore, False + + if existing: + if not existing.endswith('\n'): + existing += '\n' + existing += '\n' + else: + existing = _DVCIGNORE_HEADER + existing += f"{DVCIGNORE_GITIGNORE_PATTERN}\n" + dvcignore.write_text(existing) + return dvcignore, True + + +def is_dvc_metadata_file(path: str) -> bool: + """True for the bookkeeping files DVC keeps out of imported payloads. + + Mirrors ``dvc.fs.dvc._is_dvc_file``, the exclusion list DVC's repo + filesystem applies when it walks a tree: ``*.dvc``, ``dvc.yaml``, + ``dvc.lock`` and ``.dvcignore``. Notably absent from that list -- and hence + from this one -- is ``.gitignore``, which is why DVC's own generated ignore + files end up inside other repos' data. See :func:`ensure_dvcignore`. + """ + name = Path(path).name + return name.endswith('.dvc') or name in ('dvc.yaml', 'dvc.lock', '.dvcignore') + + +def unhashed_listing_paths(files: Iterable[Dict[str, Any]]) -> List[str]: + """Paths in a ``dvc list`` result that the source repo records no hash for. + + These are the git-tracked files inside a directory that is not itself a + DVC out: ``dvc list`` reports them with ``md5: null`` and ``isout: false``. + ``dvc import`` hashes them from the git worktree and includes them in the + payload; nothing that works from hashes alone can, so their presence means + any manifest we build will disagree with DVC's (issue #182). + + DVC's own bookkeeping files are excluded, because ``dvc import`` excludes + them too -- a directory of outs and their ``.dvc`` files is still one we + can rebuild faithfully. + """ + paths = [] + for f in files: + if f.get('isdir'): + continue + if f.get('md5'): + continue + path = f.get('path') or f.get('relpath') + if path and not is_dvc_metadata_file(path): + paths.append(path) + return paths + + +def describe_mixed_tree( + paths: List[str], + source_path: str, + repo_url: str, + action: str, + limit: int = 10, +) -> str: + """Explain why a path cannot be turned into a .dir manifest, and what to do. + + Args: + paths: The unhashed (git-tracked) paths, from + :func:`unhashed_listing_paths`. + source_path: The path being imported, inside the source repo. + repo_url: The source repository. + action: Verb for the refused operation, e.g. 'import' or 'rebuild'. + limit: How many paths to name before summarising the rest. + """ + shown = '\n'.join(f" {p}" for p in paths[:limit]) + if len(paths) > limit: + shown += f"\n ... and {len(paths) - limit} more" + + only_gitignores = all( + Path(p).name == '.gitignore' for p in paths + ) + + lines = [ + f"Cannot {action} '{source_path}' from {repo_url}: it is not a single " + f"DVC out.", + f"{len(paths)} file(s) there are git-tracked, so the source repo " + f"records no hash for them:", + shown, + "", + "dvc import hashes those from the git worktree and counts them in the " + "payload; dt works from hashes alone, so the manifest it built would " + "disagree with dvc's in hash, nfiles and size.", + "", + "Options:", + ] + if only_gitignores: + lines += [ + f" - Those are DVC's own generated ignore files. In {repo_url}, " + f"add '.gitignore' to .dvcignore ('dt init' now does this) and " + f"commit, then retry. Note this changes the directory's hash, " + f"nfiles and size, as it removes them from the payload.", + ] + lines += [ + f" - Point at the out(s) instead, e.g. a subdirectory of " + f"'{source_path}' that is itself dvc-tracked.", + f" - Make '{source_path}' a single out in {repo_url} " + f"('dvc add {source_path}').", + f" - Or use plain 'dvc import'/'dvc update' for this path, which " + f"hashes the git-tracked files too.", + ] + return '\n'.join(lines) + + def build_dir_manifest(entries: List[Dict[str, str]]) -> bytes: """Build a .dir manifest in DVC's canonical JSON format. diff --git a/pyproject.toml b/pyproject.toml index 2197dd0..13ac11b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "dvc-tools" -version = "0.23.0" +version = "0.24.0" description = "Convenient tools for working with DVC in HPC environments" readme = "README.md" requires-python = ">=3.8" diff --git a/tests/unit/test_doctor.py b/tests/unit/test_doctor.py index 5610e16..9ad41a8 100644 --- a/tests/unit/test_doctor.py +++ b/tests/unit/test_doctor.py @@ -374,3 +374,36 @@ def test_readonly_default_root_fails(self, tmp_path): assert 'Default remote root not writable' in r.message finally: os.chmod(a, 0o755) + + +class TestCheckDvcignore: + """Flag repos whose .gitignore files can leak into importers' data (#182).""" + + def test_passes_when_pattern_present(self, tmp_path): + (tmp_path / '.dvcignore').write_text("# why\n.gitignore\n") + with patch.object(doctor, 'check_in_dvc_repo', return_value=(True, tmp_path)): + result = doctor.check_dvcignore() + + assert result.passed is True + + def test_fails_when_dvcignore_missing(self, tmp_path): + with patch.object(doctor, 'check_in_dvc_repo', return_value=(True, tmp_path)): + result = doctor.check_dvcignore() + + assert result.passed is False + assert '.gitignore' in result.message + assert '.dvcignore' in result.help_text + + def test_fails_when_pattern_absent(self, tmp_path): + (tmp_path / '.dvcignore').write_text("raw/scratch\n") + with patch.object(doctor, 'check_in_dvc_repo', return_value=(True, tmp_path)): + result = doctor.check_dvcignore() + + assert result.passed is False + + def test_skips_outside_a_dvc_repo(self): + with patch.object(doctor, 'check_in_dvc_repo', return_value=(False, None)): + result = doctor.check_dvcignore() + + assert result.passed is True + assert 'skipped' in result.message.lower() diff --git a/tests/unit/test_import_data.py b/tests/unit/test_import_data.py index ede007f..06aa609 100644 --- a/tests/unit/test_import_data.py +++ b/tests/unit/test_import_data.py @@ -817,6 +817,102 @@ def test_no_download_file_lacks_hash_and_size(self, tmp_path): assert out['hash'] == 'md5' +class TestCreateDvcFileOmitsUnknownSize: + """No size at all beats a size of zero (#182). + + `size: 0` on a multi-GB import reads as an empty payload and silently + undercounts anything that sums recorded sizes. + """ + + def test_size_field_absent_when_none(self, tmp_path): + dvc_file = import_mod.create_dvc_file( + dest_path=tmp_path, + name="data", + md5="abc123.dir", + size=None, + nfiles=2, + repo_url="git@github.com:org/repo.git", + repo_path="data", + rev_lock="rev123", + ) + + out = yaml.safe_load(Path(dvc_file).read_text())['outs'][0] + assert 'size' not in out + assert out['nfiles'] == 2 + assert out['md5'] == 'abc123.dir' + + +class TestObjectSizeAcrossLayouts: + """Sizing must survive a half-migrated remote (#182). + + The field case: a registry holding some objects under `files/md5/` and + others still at the bare `/` v2 path. Checkout coped, because it + falls back to v2; the size lookup did not, so every v2-resident object + contributed zero. + """ + + BLOB = b'z' * 4096 + + def _md5(self): + import hashlib + return hashlib.md5(self.BLOB).hexdigest() + + def test_finds_v3_object(self, tmp_path): + md5 = self._md5() + obj = tmp_path / 'files' / 'md5' / md5[:2] / md5[2:] + obj.parent.mkdir(parents=True) + obj.write_bytes(self.BLOB) + + assert import_mod.get_file_size_from_cache(str(tmp_path), md5) == 4096 + + def test_finds_v2_object(self, tmp_path): + """The regression: this returned None, and the caller made it 0.""" + md5 = self._md5() + obj = tmp_path / md5[:2] / md5[2:] + obj.parent.mkdir(parents=True) + obj.write_bytes(self.BLOB) + + assert import_mod.get_file_size_from_cache(str(tmp_path), md5) == 4096 + + def test_finds_v2_dir_object(self, tmp_path): + md5 = self._md5() + obj = tmp_path / md5[:2] / f"{md5[2:]}.dir" + obj.parent.mkdir(parents=True) + obj.write_bytes(self.BLOB) + + assert import_mod.get_file_size_from_cache(str(tmp_path), f"{md5}.dir") == 4096 + + def test_missing_object_reports_none(self, tmp_path): + assert import_mod.get_file_size_from_cache(str(tmp_path), 'f' * 32) is None + + def test_sum_reports_which_objects_are_unsized(self, tmp_path): + md5 = self._md5() + obj = tmp_path / md5[:2] / md5[2:] + obj.parent.mkdir(parents=True) + obj.write_bytes(self.BLOB) + + total, missing = import_mod._sum_object_sizes( + [md5, 'f' * 32], str(tmp_path) + ) + + assert total == 4096 + assert missing == ['f' * 32] + + def test_second_root_is_consulted(self, tmp_path): + """The source remote may be missing what the local cache already has.""" + md5 = self._md5() + local = tmp_path / 'cache' + obj = local / 'files' / 'md5' / md5[:2] / md5[2:] + obj.parent.mkdir(parents=True) + obj.write_bytes(self.BLOB) + + total, missing = import_mod._sum_object_sizes( + [md5], str(tmp_path / 'remote'), str(local) + ) + + assert (total, missing) == (4096, []) + + # ============================================================================= # create_dir_file -- shared, content-addressed caches (#175) # ============================================================================= @@ -929,3 +1025,84 @@ def test_silent_when_the_existing_object_is_intact(self, tmp_path, capsys): capsys.readouterr() import_mod.create_dir_file(self.ENTRIES, str(tmp_path)) assert capsys.readouterr().err == '' + + +# ============================================================================= +# Refusing a path that is not a single out (#182) +# ============================================================================= + +class TestMixedTreeGuard: + """A directory mixing git-tracked files with outs cannot be imported. + + `dvc import` hashes the git-tracked files from the worktree and folds them + into the payload; dt works from hashes alone. Building a manifest anyway + produced one that disagreed with dvc's -- and, before this guard, the + hashless entry travelled on until something dereferenced it and raised + AttributeError. + """ + + LISTING_MIXED = [ + {'path': '.gitignore', 'isout': False, 'md5': None}, + {'path': 'in_house.dvc', 'isout': False, 'md5': None}, + {'path': 'in_house/a.txt', 'isout': True, 'md5': 'a' * 32}, + ] + LISTING_CLEAN = [ + {'path': 'in_house.dvc', 'isout': False, 'md5': None}, + {'path': 'in_house/a.txt', 'isout': True, 'md5': 'a' * 32}, + ] + + def _run(self, listing, tmp_path, monkeypatch, path='data/annotations'): + monkeypatch.chdir(tmp_path) + with patch.object(utils, 'check_dvc'), \ + patch.object(tmp_mod, 'clone_repo', return_value=tmp_path / 'clone'), \ + patch.object(tmp_mod, 'resolve_repository_url', return_value='git@x:o/r.git'), \ + patch.object(import_mod.remote_mod, 'find_local_remote_from_repo', + return_value=('local', str(tmp_path / 'remote'))), \ + patch.object(import_mod, 'configure_clone_cache'), \ + patch.object(import_mod, 'list_files', return_value=listing), \ + patch('subprocess.run', return_value=MagicMock(returncode=0, stdout='abc\n')): + return import_mod.import_data( + repository='r', path=path, checkout=False + ) + + def test_refuses_and_names_the_stray_file(self, tmp_path, monkeypatch): + with pytest.raises(ImportError) as exc: + self._run(self.LISTING_MIXED, tmp_path, monkeypatch) + + msg = str(exc.value) + assert 'not a single DVC out' in msg + assert '.gitignore' in msg + assert '.dvcignore' in msg, "name the fix for a bare .gitignore" + assert 'in_house.dvc' not in msg, "dvc excludes .dvc files from payloads too" + + def test_a_directory_of_outs_still_imports(self, tmp_path, monkeypatch): + """The .dvc files present alongside outs must not trip the guard.""" + blob = b'data' + md5 = 'a' * 32 + obj = tmp_path / 'remote' / 'files' / 'md5' / md5[:2] / md5[2:] + obj.parent.mkdir(parents=True) + obj.write_bytes(blob) + + dvc_file, _ = self._run(self.LISTING_CLEAN, tmp_path, monkeypatch) + + out = yaml.safe_load(Path(dvc_file).read_text())['outs'][0] + assert out['nfiles'] == 1, "only the tracked file belongs in the manifest" + assert out['size'] == len(blob) + assert out['md5'].endswith('.dir'), "a directory keeps its .dir manifest" + + def test_single_file_import_gets_no_dir(self, tmp_path, monkeypatch): + """The lone entry IS the requested path, so it is a plain file out.""" + md5 = 'b' * 32 + obj = tmp_path / 'remote' / md5[:2] / md5[2:] + obj.parent.mkdir(parents=True) + obj.write_bytes(b'hello') + + dvc_file, _ = self._run( + [{'path': 'samples.h5ad', 'isout': True, 'md5': md5}], + tmp_path, monkeypatch, path='data/samples.h5ad', + ) + + out = yaml.safe_load(Path(dvc_file).read_text())['outs'][0] + assert out['md5'] == md5 + assert 'nfiles' not in out + assert out['size'] == 5 diff --git a/tests/unit/test_init.py b/tests/unit/test_init.py index b8a85b6..226cb3a 100644 --- a/tests/unit/test_init.py +++ b/tests/unit/test_init.py @@ -15,6 +15,7 @@ install_dvc_hooks, get_dvc_autostage, init_dt_directory, + init_dvcignore, init_project, ) from dt.errors import InitError @@ -280,6 +281,40 @@ def test_autostages_when_dvc_autostage_enabled(self, tmp_path): assert git_add_called +class TestInitDvcignore: + """A new repo should not export its own .gitignore files as data (#182).""" + + def test_seeds_the_gitignore_pattern(self, tmp_path): + with patch("dt.init.get_dvc_autostage", return_value=False): + changed = init_dvcignore(tmp_path, verbose=False) + + assert changed is True + assert '.gitignore' in (tmp_path / '.dvcignore').read_text().splitlines() + + def test_is_idempotent(self, tmp_path): + with patch("dt.init.get_dvc_autostage", return_value=False): + init_dvcignore(tmp_path, verbose=False) + assert init_dvcignore(tmp_path, verbose=False) is False + + def test_autostages_when_enabled(self, tmp_path): + with patch("dt.init.get_dvc_autostage", return_value=True): + with patch("dt.init.subprocess.run") as mock_run: + mock_run.return_value = MagicMock(returncode=0) + init_dvcignore(tmp_path, verbose=False) + + assert any('add' in str(c) for c in mock_run.call_args_list) + + def test_does_not_stage_when_unchanged(self, tmp_path): + with patch("dt.init.get_dvc_autostage", return_value=True): + with patch("dt.init.subprocess.run") as mock_run: + mock_run.return_value = MagicMock(returncode=0) + init_dvcignore(tmp_path, verbose=False) + mock_run.reset_mock() + init_dvcignore(tmp_path, verbose=False) + + mock_run.assert_not_called() + + # ============================================================================= # init_project tests # ============================================================================= diff --git a/tests/unit/test_update.py b/tests/unit/test_update.py index 14ad9e8..d9fe6ce 100644 --- a/tests/unit/test_update.py +++ b/tests/unit/test_update.py @@ -191,3 +191,230 @@ def test_idempotent_when_already_present(self, tmp_path): dest = remote / "files" / "md5" / md5[:2] / f"{md5[2:]}.dir" assert not os.path.islink(dest) + +# ============================================================================= +# outs.size must never be left describing the previous hash (#182) +# ============================================================================= + +def _write_import_dvc(path, outs, deps_repo): + """Write a minimal import .dvc file.""" + path.write_text(yaml.dump({ + 'frozen': True, + 'deps': [{'path': 'data/thing', 'repo': deps_repo}], + 'outs': [outs], + })) + return path + + +class TestSizeIsNeverStale: + """A size that belongs to the old hash is a false statement, not a default. + + Regression guard for issue #182: `dvc list --size` against a repo URL + returns no sizes, so the size argument was None, and the old code read that + as "leave it alone" -- retaining a figure 3x the real directory. + """ + + def test_clear_size_removes_stale_value(self, tmp_path): + dvc = _write_import_dvc( + tmp_path / "thing.dvc", + {'md5': 'old.dir', 'size': 534483329917, 'nfiles': 169, 'path': 'thing'}, + {'url': 'git@example.com:o/r.git', 'rev_lock': 'a' * 40}, + ) + + modified = update._update_dvc_file( + dvc, 'new.dir', size=None, clear_size=True, nfiles=1092 + ) + + out = yaml.safe_load(dvc.read_text())['outs'][0] + assert modified is True + assert 'size' not in out, "stale size must be removed, not retained" + assert out['md5'] == 'new.dir' + assert out['nfiles'] == 1092 + + def test_size_left_alone_without_clear_flag(self, tmp_path): + """The rev_lock-only fast path must not disturb a valid size.""" + dvc = _write_import_dvc( + tmp_path / "thing.dvc", + {'md5': 'same.dir', 'size': 4096, 'nfiles': 2, 'path': 'thing'}, + {'url': 'git@example.com:o/r.git', 'rev_lock': 'a' * 40}, + ) + + update._update_dvc_file(dvc, 'same.dir', new_rev='b' * 40) + + out = yaml.safe_load(dvc.read_text())['outs'][0] + assert out['size'] == 4096 + + def test_backfills_size_by_stat_when_listing_has_none(self, tmp_path): + """No sizes from the source listing: stat the objects instead.""" + remote = tmp_path / "remote" + blob = b'x' * 1234 + md5 = utils.md5_bytes(blob) + # v2 layout, to cover the half-migrated remotes seen in the field. + obj = remote / md5[:2] / md5[2:] + obj.parent.mkdir(parents=True) + obj.write_bytes(blob) + + entries = [{'md5': md5, 'relpath': 'a.fq'}] + assert update._total_size_from_entries(entries, str(remote)) == 1234 + + def test_returns_none_when_any_object_is_unsized(self, tmp_path): + """A partial total would read as fact downstream, so refuse to give one.""" + remote = tmp_path / "remote" + blob = b'y' * 10 + md5 = utils.md5_bytes(blob) + obj = remote / 'files' / 'md5' / md5[:2] / md5[2:] + obj.parent.mkdir(parents=True) + obj.write_bytes(blob) + + entries = [{'md5': md5, 'relpath': 'a'}, {'md5': 'f' * 32, 'relpath': 'b'}] + assert update._total_size_from_entries(entries, str(remote)) is None + + def test_listing_sizes_win_when_present(self, tmp_path): + entries = [{'md5': 'a' * 32, 'relpath': 'a', 'size': 7}, + {'md5': 'b' * 32, 'relpath': 'b', 'size': 8}] + assert update._total_size_from_entries(entries, str(tmp_path)) == 15 + + +# ============================================================================= +# deps.repo.rev must not contradict rev_lock (#182) +# ============================================================================= + +class TestRevSpecConsistency: + """`rev` is the spec, `rev_lock` its resolution -- they must agree. + + Regression guard for issue #182: advancing rev_lock while leaving a pinned + `rev` behind made the next plain `dvc update` resolve `rev` and roll the + import back to the commit just updated away from. + """ + + def test_stale_pinned_sha_is_dropped(self, tmp_path): + dvc = _write_import_dvc( + tmp_path / "thing.dvc", + {'md5': 'h.dir', 'size': 1, 'path': 'thing'}, + {'url': 'git@example.com:o/r.git', + 'rev': 'ec71bce', + 'rev_lock': 'ec71bcee429cee141a6893a593beddca10f7774e'}, + ) + + update._update_dvc_file(dvc, 'h.dir', new_rev='8024f5cd5fef7a123765ae289fa6471c7d6398a4') + + repo = yaml.safe_load(dvc.read_text())['deps'][0]['repo'] + assert repo['rev_lock'] == '8024f5cd5fef7a123765ae289fa6471c7d6398a4' + assert 'rev' not in repo, "a pin that contradicts rev_lock must go" + + def test_pinned_sha_kept_when_lock_still_matches_it(self, tmp_path): + """The short sha is a prefix of the new lock: no contradiction.""" + full = 'ec71bcee429cee141a6893a593beddca10f7774e' + dvc = _write_import_dvc( + tmp_path / "thing.dvc", + {'md5': 'h.dir', 'size': 1, 'path': 'thing'}, + {'url': 'git@example.com:o/r.git', 'rev': 'ec71bce', 'rev_lock': 'old'}, + ) + + update._update_dvc_file(dvc, 'h.dir', new_rev=full) + + repo = yaml.safe_load(dvc.read_text())['deps'][0]['repo'] + assert repo['rev'] == 'ec71bce' + assert repo['rev_lock'] == full + + def test_branch_spec_survives_a_lock_advance(self, tmp_path): + """rev_lock moving under a tracked branch is normal, not a conflict.""" + dvc = _write_import_dvc( + tmp_path / "thing.dvc", + {'md5': 'h.dir', 'size': 1, 'path': 'thing'}, + {'url': 'git@example.com:o/r.git', 'rev': 'seed', 'rev_lock': 'a' * 40}, + ) + + update._update_dvc_file(dvc, 'h.dir', new_rev='b' * 40) + + repo = yaml.safe_load(dvc.read_text())['deps'][0]['repo'] + assert repo['rev'] == 'seed' + assert repo['rev_lock'] == 'b' * 40 + + def test_explicit_spec_is_recorded(self, tmp_path): + dvc = _write_import_dvc( + tmp_path / "thing.dvc", + {'md5': 'h.dir', 'size': 1, 'path': 'thing'}, + {'url': 'git@example.com:o/r.git', 'rev_lock': 'a' * 40}, + ) + + update._update_dvc_file( + dvc, 'h.dir', new_rev='b' * 40, new_rev_spec='seed' + ) + + repo = yaml.safe_load(dvc.read_text())['deps'][0]['repo'] + assert repo['rev'] == 'seed' + assert repo['rev_lock'] == 'b' * 40 + + +class TestIsShaLike: + """Distinguishing an immutable pin from a moving ref.""" + + @pytest.mark.parametrize('rev', ['ec71bce', 'a' * 40, 'ABCDEF1']) + def test_sha_like(self, rev): + assert update._is_sha_like(rev) is True + + @pytest.mark.parametrize('rev', ['main', 'seed', 'v1.2.3', 'feat/abc', '', 'abc', 'a' * 41]) + def test_not_sha_like(self, rev): + assert update._is_sha_like(rev) is False + + + +# ============================================================================= +# Refusing to rebuild a path that is not a single out (#182) +# ============================================================================= + +class TestMixedTreeRefusal: + """A rebuilt .dir that omits git-tracked files disagrees with dvc's. + + Worse, `dt update` pushes it to the *source* remote -- so the divergence + lands in a shared registry as an object no dvc operation would produce. + """ + + LISTING = [ + {'path': '.gitignore', 'isout': False, 'md5': None, 'size': 18}, + {'path': 'in_house.dvc', 'isout': False, 'md5': None, 'size': 100}, + {'path': 'in_house/a.txt', 'isout': True, 'md5': 'a' * 32, 'size': 13}, + ] + + def test_listing_reports_unhashed_paths(self): + with patch('dt.update.subprocess.run') as mock_run: + mock_run.return_value = MagicMock( + returncode=0, stdout=__import__('json').dumps(self.LISTING) + ) + entries, unhashed = update._get_file_listing('repo', 'p', 'rev') + + assert [e['relpath'] for e in entries] == ['in_house/a.txt'] + assert unhashed == ['.gitignore'], "the .dvc file is not a divergence" + + def test_change_detection_reports_the_mixed_tree(self): + with patch.object(update, '_compute_source_hash', + return_value=(None, 0, ['.gitignore'])): + changes = update._check_source_changes( + Path('.'), 'data/annotations', 'a' * 40, 'b' * 40, + repo_url='repo', current_hash='old.dir', + ) + + assert changes.mixed_tree == ['.gitignore'] + assert 'git-tracked' in changes.diff_summary + + def test_update_refuses_and_does_not_touch_the_dvc_file(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + dvc = _write_import_dvc( + tmp_path / "thing.dvc", + {'md5': 'old.dir', 'size': 31534770, 'nfiles': 169, 'path': 'thing'}, + {'url': 'git@example.com:o/r.git', 'rev_lock': 'a' * 40}, + ) + before = dvc.read_text() + + with patch.object(update.tmp_mod, 'clone_repo', return_value=tmp_path), \ + patch.object(update, '_tracked_tip', return_value='b' * 40), \ + patch.object(update, '_compute_source_hash', + return_value=(None, 0, ['.gitignore'])), \ + patch.object(update, '_push_dir_to_remote') as push: + results = update.update(targets=[str(dvc)], verbose=False) + + assert results[0][1] is False + assert 'git-tracked' in results[0][2] + assert dvc.read_text() == before, "a refusal must not rewrite metadata" + push.assert_not_called() diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 6908c07..648d1cc 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -738,3 +738,124 @@ def test_get_cache_root_strips(self): def test_get_cache_root_none_outside_repo(self): with patch.object(utils, 'get_cache_dir', return_value=None): assert utils.get_cache_root() is None + + +# ============================================================================= +# .dvcignore seeding and mixed-tree detection (#182) +# ============================================================================= + +class TestEnsureDvcignore: + """DVC's generated .gitignore files must not become other repos' data. + + `dvc import `, where dir is not itself an out, hashes the + git-tracked files it finds -- including the .gitignore DVC wrote there -- + and folds them into the importer's payload, nfiles and size. One + .dvcignore pattern in the source repo prevents that. + """ + + def test_creates_with_explanation(self, tmp_path): + path, changed = utils.ensure_dvcignore(tmp_path) + + assert changed is True + assert path == tmp_path / '.dvcignore' + text = path.read_text() + assert '.gitignore' in text.splitlines() + assert text.startswith('#'), "the pattern needs its rationale beside it" + + def test_is_idempotent(self, tmp_path): + utils.ensure_dvcignore(tmp_path) + before = (tmp_path / '.dvcignore').read_text() + + path, changed = utils.ensure_dvcignore(tmp_path) + + assert changed is False + assert path.read_text() == before + + def test_appends_without_clobbering(self, tmp_path): + existing = "raw/scratch\n*.tmp\n" + (tmp_path / '.dvcignore').write_text(existing) + + _, changed = utils.ensure_dvcignore(tmp_path) + + text = (tmp_path / '.dvcignore').read_text() + assert changed is True + assert text.startswith(existing), "existing patterns must survive" + assert '.gitignore' in text.splitlines() + + def test_tolerates_missing_trailing_newline(self, tmp_path): + (tmp_path / '.dvcignore').write_text("raw/scratch") + + utils.ensure_dvcignore(tmp_path) + + lines = (tmp_path / '.dvcignore').read_text().splitlines() + assert 'raw/scratch' in lines + assert '.gitignore' in lines + + +class TestIsDvcMetadataFile: + """Mirrors dvc.fs.dvc._is_dvc_file -- note .gitignore is NOT on it.""" + + @pytest.mark.parametrize('path', [ + 'data.dvc', 'a/b/data.dvc', 'dvc.yaml', 'a/dvc.lock', '.dvcignore', + ]) + def test_metadata(self, path): + assert utils.is_dvc_metadata_file(path) is True + + @pytest.mark.parametrize('path', [ + '.gitignore', 'a/b/.gitignore', 'README.md', 'data/x.fq', 'dvc.yamlx', + ]) + def test_not_metadata(self, path): + assert utils.is_dvc_metadata_file(path) is False + + +class TestUnhashedListingPaths: + """Which entries in a `dvc list` result make a faithful manifest impossible.""" + + def test_flags_git_tracked_files(self): + files = [ + {'path': '.gitignore', 'isout': False, 'md5': None}, + {'path': 'README.md', 'isout': False, 'md5': None}, + {'path': 'in_house/a.txt', 'isout': True, 'md5': 'a' * 32}, + ] + assert utils.unhashed_listing_paths(files) == ['.gitignore', 'README.md'] + + def test_ignores_dvc_files_and_dirs(self): + """A directory of outs plus their .dvc files is still rebuildable.""" + files = [ + {'path': 'in_house.dvc', 'isout': False, 'md5': None}, + {'path': 'imerit.dvc', 'isout': False, 'md5': None}, + {'path': 'sub', 'isdir': True, 'md5': None}, + {'path': 'in_house/a.txt', 'isout': True, 'md5': 'a' * 32}, + ] + assert utils.unhashed_listing_paths(files) == [] + + def test_empty_for_a_clean_out(self): + files = [{'path': 'a.txt', 'isout': True, 'md5': 'a' * 32}] + assert utils.unhashed_listing_paths(files) == [] + + +class TestDescribeMixedTree: + """The refusal has to say what to do about it.""" + + def test_names_the_files_and_the_dvcignore_remedy(self): + msg = utils.describe_mixed_tree( + ['.gitignore'], 'data/annotations', + 'git@github.com:swarbricklab/images.git', action='rebuild', + ) + assert 'data/annotations' in msg + assert '.gitignore' in msg + assert '.dvcignore' in msg, "for a bare .gitignore, name the real fix" + assert 'dvc update' in msg + + def test_omits_dvcignore_advice_for_other_files(self): + msg = utils.describe_mixed_tree( + ['README.md'], 'data/annotations', 'repo', action='import', + ) + assert 'README.md' in msg + assert '.dvcignore' not in msg, "adding it would not help here" + + def test_summarises_a_long_list(self): + msg = utils.describe_mixed_tree( + [f'f{i}.txt' for i in range(25)], 'd', 'repo', action='import', + ) + assert '... and 15 more' in msg