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.

Sign up for GitHub

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

Jump to bottom

Fix #864 (fetching deleted/moved branches in colocated repos) #1600

Merged
merged 7 commits into from
May 18, 2023
Merged

Fix #864 (fetching deleted/moved branches in colocated repos) #1600

merged 7 commits into from
May 18, 2023

Conversation

ilyagr
Copy link
Collaborator

@ilyagr ilyagr commented May 11, 2023 •
edited
Loading

Fixes #864 ("In a git colocated repo, commits are not rebased off of a deleted/moved branch")

Update: Some technical details from the commit description:

This bug concerns the way import_refs that gets called by fetch computes
the heads that should be visible after the import.

Previously, the list of such heads was computed before local branches were
updated based on changes to the remote branches. So, commits that should have
been abandoned based on this update of the local branches weren't properly
abandoned.

Now, import_refs tracks the heads that need to be visible because of some ref
in a mapping keyed by the ref. If the ref moves or is deleted, the
corresponding heads are updated.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • (n/a) I have updated the config schema (src/config-schema.json)
  • I have added tests to cover my changes

@ilyagr ilyagr force-pushed the fetchdel branch 10 times, most recently from 5ce7dca to 0caa03b Compare May 13, 2023 23:44
@ilyagr ilyagr linked an issue May 13, 2023 that may be closed by this pull request
In a git colocated repo, commits are not rebased off of a deleted/moved branch #864
Closed
@ilyagr ilyagr marked this pull request as ready for review May 13, 2023 23:47
@ilyagr
Copy link
Collaborator Author

ilyagr commented May 14, 2023

I added some no-op commits that mildly polish import-refs/export-refs, to preserve some minor improvements I made in #1487.

I'm happy to move those (the last three commits) to a separate PR. There will likely be a separate PR in any case to add some tests I added in #1487.

@ilyagr ilyagr force-pushed the fetchdel branch 7 times, most recently from d444bcc to 522f553 Compare May 14, 2023 05:11
@ilyagr
Copy link
Collaborator Author

ilyagr commented May 14, 2023 •
edited
Loading

I decided to change the "root cause" test to make it more clear why this crazy bug only affects colocated repos. I'm also reordering the commits a bit. (Update: this is all done)

@ilyagr ilyagr force-pushed the fetchdel branch 5 times, most recently from 05a80a7 to df46428 Compare May 14, 2023 05:32
@ilyagr ilyagr mentioned this pull request May 15, 2023
Closed
@ilyagr
Copy link
Collaborator Author

ilyagr commented May 16, 2023

I'll update this to address #864 (comment) as well.

@ilyagr ilyagr marked this pull request as draft May 16, 2023 04:51
@ilyagr ilyagr changed the title Fix #864 Fix #864 (fetching deleted/moved branches in colocated repos) May 17, 2023
@ilyagr ilyagr marked this pull request as ready for review May 17, 2023 01:16
@ilyagr
Copy link
Collaborator Author

ilyagr commented May 17, 2023

I believe it's all fixed. 🎉

@ilyagr ilyagr force-pushed the fetchdel branch 3 times, most recently from 2842632 to b297489 Compare May 17, 2023 02:44
martinvonz
martinvonz approved these changes May 17, 2023
View reviewed changes
Copy link
Owner

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

lib/tests/test_git.rs Show resolved Hide resolved
lib/tests/test_git.rs Outdated Show resolved Hide resolved
lib/tests/test_git.rs Outdated Show resolved Hide resolved
lib/src/git.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
lib/src/git.rs Show resolved Hide resolved
@ilyagr ilyagr force-pushed the fetchdel branch 2 times, most recently from ea27bd6 to 8e14835 Compare May 17, 2023 22:32
@ilyagr
lib/tests/test_git.rs: New test to demonstrate martinvonz#864's root …
5da02f8 
…cause
@ilyagr
Tests demonstrating a similar bug with moved rather than deleted branch
c798868
@ilyagr
git.rs: properly abandon commits from moved/deleted branches on remote (
ac70fce 
martinvonz#864)

This bug concerns the way `import_refs` that gets called by `fetch` computes
the heads that should be visible after the import.

Previously, the list of such heads was computed *before* local branches were
updated based on changes to the remote branches. So, commits that should have
been abandoned based on this update of the local branches weren't properly
abandoned.

Now, `import_refs` tracks the heads that need to be visible because of some ref
in a mapping keyed by the ref. If the ref moves or is deleted, the
corresponding heads are updated.

Fixes martinvonz#864
@ilyagr
import_refs/export_refs: refactor conversion from branch names to ref…
3d4f2b0 
…s (no-op)

This is supposed to make `import_refs` and `export_refs` a little less prone to typos
@ilyagr
export_refs: add or edit some comments (no-op)
fdc0b06 
This is supposed to make `export_refs` a little more readable.
@ilyagr ilyagr enabled auto-merge (rebase) May 18, 2023 00:50
@ilyagr ilyagr merged commit db8fcf9 into martinvonz:main May 18, 2023
@ilyagr ilyagr mentioned this pull request May 18, 2023
Closed
2 tasks
@ilyagr ilyagr deleted the fetchdel branch May 18, 2023 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinvonz martinvonz martinvonz approved these changes

Assignees
No one assigned
Labels
colocated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

In a git colocated repo, commits are not rebased off of a deleted/moved branch
2 participants
@ilyagr @martinvonz