fix(diff): make --parent default to current version and skip non-meaningful ancestors#10526
Open
davidfirst wants to merge 2 commits into
Open
fix(diff): make --parent default to current version and skip non-meaningful ancestors#10526davidfirst wants to merge 2 commits into
davidfirst wants to merge 2 commits into
Conversation
PR Summary by QodoFix
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. Default version not imported
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
239
to
+245
| const idList = ComponentIdList.fromArray(idsToImport); | ||
| await this.scope.legacyScope.scopeImporter.importWithoutDeps(idList, { cache: true, reason: 'to show diff' }); | ||
| if (diffOpts.compareToParent) { | ||
| if (!version) throw new BitError('--parent flag expects to get version'); | ||
| if (toVersion) throw new BitError('--parent flag expects to get only one version'); | ||
| if (!version) version = component.id.version; | ||
| const versionObject = await modelComponent.loadVersion(version, repository); | ||
| const parent = versionObject.parents[0]; | ||
| let parentRef = versionObject.parents[0]; |
There was a problem hiding this comment.
1. Default version not imported 🐞 Bug ☼ Reliability
In ComponentCompareMain.computeDiff(), the initial importWithoutDeps() runs before --parent defaults version to component.id.version, so loadVersion(version) can run without importing that resolved version first. If the current version object isn’t already present in the local object store, modelComponent.loadVersion() will throw and bit diff <comp> --parent will fail.
Agent Prompt
### Issue description
`computeDiff()` imports `idsToImport` before defaulting `version` for `--parent`. When `--parent` is used without an explicit version, the resolved `version` may not be imported, and `modelComponent.loadVersion(version, repository)` can throw if the object is missing locally.
### Issue Context
`ModelComponent.loadVersion()` throws when the ref/object can’t be found/loaded from the repository. Because the defaulting happens after the initial import step, the resolved version is not guaranteed to exist locally.
### Fix Focus Areas
- scopes/component/component-compare/component-compare.main.runtime.ts[235-245]
- scopes/scope/objects/models/model-component.ts[908-913]
### Suggested fix
- Resolve the effective `version`/`toVersion` (including the `--parent` defaulting) **before** building `idsToImport` and calling `importWithoutDeps()`.
- Alternatively, after `if (!version) version = component.id.version;` add an `importWithoutDeps()` call for `component.id.changeVersion(version)` before calling `loadVersion(version, ...)`.
- Keep the existing per-parent import inside the ancestor loop as-is (or batch import if desired), but ensure the initially diffed `toVersion` is present first.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+261
to
+283
| while (parentRef) { | ||
| const parentTag = modelComponent.getTagOfRefIfExists(parentRef); | ||
| const parentVersion: string = parentTag || parentRef.toString(); | ||
| await this.scope.legacyScope.scopeImporter.importWithoutDeps( | ||
| ComponentIdList.fromArray([component.id.changeVersion(parentVersion)]), | ||
| { cache: true, reason: 'to show diff' } | ||
| ); | ||
| const parentObject = await modelComponent.loadVersion(parentVersion, repository); | ||
| version = parentVersion; | ||
| if (!parentObject.hidden) { | ||
| if (parentTag) break; | ||
| const parentDiff = await this.diffBetweenVersionsObjects( | ||
| modelComponent, | ||
| parentObject, | ||
| versionObject, | ||
| parentVersion, | ||
| toVersion, | ||
| diffOpts | ||
| ); | ||
| if (parentDiff.hasDiff) break; | ||
| } | ||
| parentRef = parentObject.parents[0]; | ||
| } |
There was a problem hiding this comment.
2. Repeated parent diff computation 🐞 Bug ➹ Performance
The new --parent ancestor walk can compute a full diff against multiple successive ancestors (load versions, materialize files, run getFilesDiff) before finding a meaningful parent. On components with long chains of non-hidden, untagged ancestors with identical content, this can make `bit diff --parent` unexpectedly slow.
Agent Prompt
### Issue description
The `--parent` walk-up loop may invoke `diffBetweenVersionsObjects()` multiple times, doing full file diff generation (temp writes + git diff) per ancestor just to answer the yes/no question “is content identical?”.
### Issue Context
This is only triggered when:
- the ancestor is not hidden,
- the ancestor is a snap (no tag), and
- the content is identical across multiple ancestors.
In that shape of history, the loop performs repeated expensive work.
### Fix Focus Areas
- scopes/component/component-compare/component-compare.main.runtime.ts[253-283]
- scopes/component/component-compare/component-compare.main.runtime.ts[319-352]
- components/legacy/component-diff/components-diff.ts[60-101]
### Suggested fix
- Add a lightweight “content equality” check for the ancestor-walk decision:
- e.g., compare per-file hashes and paths from `modelFilesToSourceFiles()` (without calling `getOneFileDiff()` / `git diff`) to decide if the parent is identical.
- Only when a non-identical ancestor is found, compute the full diff once for output.
- Keep the “never skip tag ancestor by content” rule by applying the fast equality check only when `parentTag` is falsy.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Code Review by Qodo
1. Default version not imported
|
Comment on lines
240
to
244
| await this.scope.legacyScope.scopeImporter.importWithoutDeps(idList, { cache: true, reason: 'to show diff' }); | ||
| if (diffOpts.compareToParent) { | ||
| if (!version) throw new BitError('--parent flag expects to get version'); | ||
| if (toVersion) throw new BitError('--parent flag expects to get only one version'); | ||
| if (!version) version = component.id.version; | ||
| const versionObject = await modelComponent.loadVersion(version, repository); |
There was a problem hiding this comment.
1. Default version not imported 🐞 Bug ☼ Reliability
In computeDiff(), when --parent is used without an explicit version, version is defaulted to component.id.version only after importWithoutDeps() runs, so the current version may not be imported before modelComponent.loadVersion(version) executes and can throw VersionNotFoundOnFS if the object is missing locally.
Agent Prompt
### Issue description
`bit diff <comp> --parent` now defaults the version to `component.id.version`, but this happens **after** the initial `scopeImporter.importWithoutDeps()` call (which imports only explicit CLI versions). If the current version object is not already in the local object store, `modelComponent.loadVersion(version, repository)` may throw (e.g. `VersionNotFoundOnFS`).
### Issue Context
- `computeDiff()` imports versions from `idsToImport` before entering the `compareToParent` branch.
- When the user omits the version, `idsToImport` is empty, so nothing is imported.
- `ModelComponent.loadVersion()` throws when the ref exists but the object file is missing locally.
### Fix Focus Areas
- scopes/component/component-compare/component-compare.main.runtime.ts[235-244]
### Proposed fix
Either:
1) Move the `--parent` defaulting logic earlier (before building `idsToImport` and calling `importWithoutDeps`), so the defaulted `version` is included in `idsToImport`, or
2) After `version` is defaulted inside the `compareToParent` branch, explicitly `importWithoutDeps([component.id.changeVersion(version)])` before calling `loadVersion(version, repository)`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes several issues with
bit diff --parent:--parentwithout a version threw an error; it now defaults to the component's current version._tag) on a merged lane snap, the tag is identical to that snap, so diffing against the immediate parent showed "no diff". The diff now walks up the history, skipping hidden ancestors and snap ancestors with identical content (the latter covers releases with rebuilt artifacts, where the merged snap is not marked hidden). A tag ancestor is never skipped by content, so a legit tag created with--unmodifiedstill shows no diff.--parenton the first version now shows all files as added.Added e2e coverage for all the above.