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