Skip to content

Conversation

@stimberm
Copy link
Contributor

@stimberm stimberm commented Dec 7, 2025

Removing packages from the file system can affect the outcome of a diff involving the working copy. This can come up during checkout or discard-and-load repair actions.

Consider the following scenario:

  • working copy is on branch A, with package P
  • there exists a branch B removing P
  • using git directly, checkout B
  • use discard-and-load to load B in the image

The expectation is that the preview shows the removal of P, but instead the diff is empty (or contains changes to other packages), and after proceeding with the checkout, package P is still present in the image.

This commit fixes that by

  • removing a general check on the existence of files. Both packages and files do their own existence checks.
  • switching the existince check for packages to looking at the reference commit and the image instead of the file system.

Removing packages from the file system can affect the outcome of a diff
involving the working copy. This can come up during checkout or
discard-and-load repair actions.

Consider the following scenario:
  - working copy is on branch A, with package P
  - there exists a branch B removing P
  - using git directly, checkout B
  - use discard-and-load to load B in the image

The expectation is that the preview shows the removal of P, but instead
the diff is empty (or contains changes to other packages), and after
proceeding with the checkout, package P is still present in the image.

This commit fixes that by
  - removing a general check on the existence of files. Both packages
    and files do their own existence checks.
  - switching the existince check for packages to looking at the
    reference commit and the image instead of the file system.
@stimberm
Copy link
Contributor Author

stimberm commented Dec 7, 2025

I am somewhat out of my depth with this one. I don't really know whether this fix is correct and can cause other issues.

The general check for file existence has been added here 1787eb5#diff-6752517f621353082ec42733368b670965b7be2996acc2e1843f2fff18763460R6.

It looks to me like packages, files, and folders, all do their own checks already.

I'm not sure about the version includesPackageNamed: currentSegment either.
A comment says it combines packages in the commit and the image, but the implementation seems to require packages to be loaded, thereby excluding non-loaded package present in the commit.

includesPackageNamed: aString 
	"true if is in image or is in commit"
	^ self includesInWorkingCopyPackageNamed: aString

includesInWorkingCopyPackageNamed: aString 
	^ (self packagesDictionary includesKey: aString)
		and: [ (self packageNamed: aString) isLoaded ]

@jecisc
Copy link
Contributor

jecisc commented Dec 8, 2025

I will try to take a look at the code at some point. Idk when exactly since I'm also pretty new to this part of the system.

In the meantime if someone with more knowledge wants to take a look it would be great.

I would like also to spend some time to improve Iceberg's CI so that we run the test again

@jecisc
Copy link
Contributor

jecisc commented Dec 17, 2025

Sorry I still did not find the time to explore the code and see if the change seems right to me

@stimberm
Copy link
Contributor Author

Sorry I still did not find the time to explore the code and see if the change seems right to me

No problem, this is not blocking me. Just something I noticed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants