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

jj op undo/restore no longer restore git-tracking branches by default #1700

Merged
merged 6 commits into from
Jul 3, 2023

Conversation

ilyagr
Copy link
Collaborator

@ilyagr ilyagr commented Jun 14, 2023

This is attempt number ~3-4 to fix #922 and is heavily based on #1541 (two more attempts happened as two versions of #1487). There is a long discussion in that PR, the plan for this PR specifically is discussed towards the end.

The first 2 commits are somewhat unrelated and could be moved to a separate PR. They add tests pertaining to the question of whether we should import/export remote-tracking branches by default in normal repos (see discussion in #1739). It is unfortunate that changes to import/export & git-tracking branches affect push and fetch. However, I thought it might be interesting which of these tests are and are not changed by this PR.

See also #1741 for what tests change if we have this behavior in non-colocated repos as well (making this equivalent to #1541).

This may still be a bit unpolished, and I may have gone a bit overboard with the tests. Oh well.

I also discovered some minor unexpected behaviors and some mysteries about the test behavior still remain, mostly having to do with fetch/push behavior. We've been discussing what to do about these in #1739, I think it's a separate issue. I think they are acceptable (mostly changing from one weird behavior to another); I'll look at them again and suggestions are welcome.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • (maybe in followup PR) I have updated the documentation (README.md, docs/, demos/)
  • (not planned) I have updated the config schema (src/config-schema.json)
  • I have added tests to cover my changes

@ilyagr ilyagr force-pushed the coloc-what branch 2 times, most recently from d3a9cbb to 5a83a87 Compare June 14, 2023 02:53
@ilyagr ilyagr force-pushed the coloc-what branch 6 times, most recently from de62154 to 3b36d9e Compare June 25, 2023 03:20
@ilyagr ilyagr changed the title Attempt number 3 (?) to fix #922 jj op undo/restore no longer restore git-tracking branches by default Jun 26, 2023
@ilyagr ilyagr marked this pull request as ready for review June 26, 2023 04:32
@ilyagr ilyagr force-pushed the coloc-what branch 3 times, most recently from 291fd1e to 774c919 Compare June 26, 2023 06:32
@ilyagr ilyagr requested a review from yuja June 29, 2023 22:12
@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 29, 2023

@yuja @martinvonz I'm not sure you noticed this PR is ready for review. It's not in perfect shape, but I think it'd benefit from other people's eyes.

Let me know if it would benefit from explaining something in more detail, either in commit messages, tests, or comments.

As I said in the description, the first two commits add tests that become a separate puzzle and (I would say) demonstrate new bugs. I could separate them in a separate PR.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 30, 2023

It turns out test_git_push_undo_colocated (one of the puzzling tests from the first two commits) is also affected by #1771. The test change in the merge of the two PRs is:

diff --git a/tests/test_undo.rs b/tests/test_undo.rs
index eda4b2f1..506111fe 100644
--- a/tests/test_undo.rs
+++ b/tests/test_undo.rs
@@ -220,8 +220,12 @@ fn test_git_push_undo_colocated() {
     // was undone, but keeps track of the latest local version. TODO: Would this
     // be improved if `jj git export` exported remote-tracking branches?
     insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
-    main: 0a3e99f08a48 CC
-      @origin (ahead by 1 commits, behind by 1 commits): 0cffb6146141 AA
+    main (conflicted):
+      - 0cffb6146141 AA
+      + 0a3e99f08a48 CC
+      + 8c05de152218 BB
+      @git (behind by 1 commits): 0a3e99f08a48 CC
+      @origin (behind by 1 commits): 8c05de152218 BB
     "###);
 }

Anyway, we should probably focus on the last four commits.

Copy link
Collaborator

@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.

The change LGTM, many thanks!

Just repeating my comment: I don't think --what should be a user-facing option, but being experimental seems fine.

tests/common/mod.rs Show resolved Hide resolved
tests/test_undo.rs Show resolved Hide resolved
tests/test_undo.rs Show resolved Hide resolved
src/commands/operation.rs Outdated Show resolved Hide resolved
src/commands/operation.rs Outdated Show resolved Hide resolved
src/commands/operation.rs Show resolved Hide resolved
@ilyagr ilyagr force-pushed the coloc-what branch 4 times, most recently from eb55da2 to d64f5dc Compare July 3, 2023 19:10
The original test is copied from @martinvonz 's [PR draft] (thanks!).

The three versions show differences in behavior due to import/export
of remote-tracking branches, and due to repo being colocated.

The former is relevant for [the discussion] of whether `jj git export` should
export remote-tracking branches. The latter will change in a follow-up commit.

Outstanding TODO: check if we have similar tests for undoing `fetch`

[PR draft]: martinvonz#1541
[the discussion]: martinvonz#1739
@ilyagr
Copy link
Collaborator Author

ilyagr commented Jul 3, 2023

Thank you very much, Yuya!

🎉

@ilyagr ilyagr enabled auto-merge (rebase) July 3, 2023 19:14
@ilyagr ilyagr merged commit 8645153 into martinvonz:main Jul 3, 2023
@ilyagr ilyagr deleted the coloc-what branch July 3, 2023 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jj op restore didn't work in a colocated repo, was undone by jj git import.
2 participants