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

Fix the test_git_colocated_fetch_deleted_branch test #1599

Merged
merged 2 commits into from
May 11, 2023

Conversation

ilyagr
Copy link
Contributor

@ilyagr ilyagr commented May 11, 2023

Cc: #864

@ilyagr ilyagr marked this pull request as ready for review May 11, 2023 03:49
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

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

Before, HEAD@git was at change e1f4 mentioned in the test. So, in spite
of what the comment in the test says, that change should NOT have been
abandoned after the fetch and the test did not demonstrate a bug.

The change LGTM, but I'm not certain that the tested scenario was wrong. It might be better to abandon the deleted branch and rebase off the working copy commit (as it would be done in non-colocated repo), but that would disagree with the change 20eb9ec.

@ilyagr
Copy link
Contributor Author

ilyagr commented May 11, 2023

I'm not sure which behavior is best here, but the fact that it's not obvious is a good point. Thanks for the reference; I'll put it in my commit description before merging this.

Before, HEAD@git was at change `e1f4` mentioned in the test. So, as long as we
consider the behavior added in 20eb9ec to be correct, that change should NOT
have been abandoned after the fetch, in spite of what the comment in the test
says. In other words, the test did NOT demonstrate a bug before this commit.

Now, the test properly demonstrates the bug.

Cc jj-vcs#864
@ilyagr ilyagr enabled auto-merge (rebase) May 11, 2023 23:19
@ilyagr ilyagr merged commit 0a51c5f into jj-vcs:main May 11, 2023
@ilyagr ilyagr deleted the fixtest branch May 11, 2023 23:35
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