Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When performing a pull, sync IRIS with repository changes using diff output rather than command output #517

Merged
merged 4 commits into from
Oct 4, 2024

Conversation

raymond-rebbeck
Copy link
Contributor

This is intended as a hopefully superior and simpler alternative to #515 to address the same issue, though only for git pull for now. Files are not always able to be reliably imported when their names are truncated in the git pull stats summary.

The truncated git pull stat summary output is no longer used and diff output is used instead. This should allow all file types to be handled without requiring any new or custom IRIS logic. The diff is performed after the git pull to avoid a potential double fetch and also a potential race condition if the remote changes between the diff and pull taking place.

Also indirectly addresses, for pulls only, the following likely unreported/unknown issues associated with the truncated name expansion logic that I have discovered in recent days, by no longer using that logic:

  • Generated classes are no longer inappropriately considered as possible classes
  • Lookup tables containing spaces in their names (e.g. A B C.lut) are now able to be successfully imported. Other files where this is possible may be similarly improved.

Have tested the following scenarios locally and seems to work well:

  • Expert mode git pull
  • Basic mode sync
  • Pull from within interoperability production
  • Pull using pull.csp

Have not made changes to the WebUI related logic in RunGitCommandWithInput as there did not seem to currently be any actual uses of it for pulls.

Nothing has been done for rebase as it appears to only be used within the context of MergeDefaultRemoteBranch which already performs a similar diff after the rebase takes place and does not seem to be affected by the same importing issues.

Nothing has been done for merge which may be affected by the same import issue. It appears that it is only used from WebUI but is not a feature that I have ever actually used so have not spent much time looking into it.

@isc-tleavitt
Copy link
Collaborator

@raymond-rebbeck thank you again - this looks awesome.

@isc-pbarton I think there was some reason we didn't use diff for the pull command with Henry's work over the summer, but suspect that it boils down to the timing issue that this PR properly considers.

Based on the better structured output format for renames (and the proper handling of them in ParseDiffStream), I suspect that this PR will also resolve #496, so unassigned it from @isc-etamarch.

@isc-tleavitt isc-tleavitt linked an issue Oct 4, 2024 that may be closed by this pull request
@isc-tleavitt
Copy link
Collaborator

Merging this, will really kick the tires on it as part of our pre-release testing for 2.6.0.

@isc-tleavitt isc-tleavitt merged commit 93ebd68 into intersystems:main Oct 4, 2024
1 check passed
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.

Sync operation does not properly handle file rename
2 participants