Fix #922 and uncover additional bugs #1487

ilyagr · 2023-04-06T02:21:32Z

Fixes #922.

This is not a complete fix, some of the commits demonstrate bugs that are yet to be fix. However, in practice this helps a lot.

This is a little unfinished, but the rest can be done in subsequent PRs (or in this one, if people prefer). In any case, it seems ready to review. For example, this PR mostly changes jj git export and only changes jj git import a little.

See https://github.com/martinvonz/jj/labels/colocated for yet more bugs to fix.

Checklist

If applicable:

I have updated CHANGELOG.md
(not planned) 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

martinvonz

I've only reviewed the first few commits yet. I'll try to finish it tomorrow. Thanks again for working on this!

lib/src/simple_op_store.rs

lib/src/git.rs

CHANGELOG.md

lib/src/git.rs

tests/test_git_colocated.rs

lib/src/git.rs

ilyagr · 2023-04-17T23:19:59Z

The PR was updated to set up a special proto. The old version is at https://github.com/ilyagr/jj/tree/export-old. It's been reorganized some, especially in the first several commits, but is quite spiritually similar to what it was before.

martinvonz · 2023-04-17T23:32:13Z

The PR was updated to set up a special proto.

Thank you! I'm glad you got it to work. Sorry that I didn't think about it earlier.

ilyagr · 2023-04-17T23:44:47Z

I'm actually glad I tried the version with the View first. I now know that known bugs are unrelated to moving from View to the proto.

ilyagr · 2023-04-17T23:54:26Z

Also, I added locking. Turns out you've already done all the work. 🎉

lib/src/protos/git_ref_view.proto

martinvonz · 2023-04-18T05:56:00Z

lib/src/git.rs

+    // TODO 2: Conceptually, this state should contain all the information in the
+    // repo that's not stored by git and should not be affected by `jj undo`. In
+    // particular, jj's last seen view of remote-tracking branches should be stored
+    // here. It is currently affected by `undo`, which can cause minor issues.


I would say that the file should contain all the information in the repo that is also stored by git, i.e. the information we need to sync back and forth. Is that correct?

What I meant to do here is to contrast information like the contents of commits -- for which the git repo is authoritative and the only place they are stored -- and the "last seen" git state (which not literally stored by git, as it is different from the current git state)

I'm thinking about how to clarify this comment. Perhaps we should say something about storing information that can be potentially conflicted between jj and git. That's what makes it necessary to sync it back and forth, as you said.

Maybe something like this:

Conceptually, this state should contain all the information in the repo that can be updated by either git or jj. For example, branches should be stored here, but remote-tracking branches and other git refs should not.

Actually, what problem does "It is currently affected by undo, which can cause minor issues." refer to?

what problem does "It is currently affected by undo, which can cause minor issues." refer to?

Mainly, jj undo after jj git fetch/push can move remote tracking branches. If we use the remote tracking branches for --force-with-lease, it could become a bigger problem.

An example that I mean to turn into a test (unless I already have):

jj git push --branch a jj branch s a -r somewhere jj git push --branch a jj op restore @--

At this point, another jj git push --branch a should push the branch a, but I think it won't because it will consider the branch unchanged.

At this point, another jj git push --branch a should push the branch a, but I think it won't because it will consider the branch unchanged.

IIRC, git push doesn't check the 'local' value of the remote branch (refs/remotes/my-remote/my-branch on the local clone); it only checks the 'remote' value of the remote branch (refs/heads/my-branch on the remote). So if jj git push replicates git push closely (that depends on libgit2 and whether we added extra steps), it should push the branch.

Ah sorry, I forgot about the --force-with-lease argument, in which case, the 'local' value is checked, yes. I agree that that would be confusing.

martinvonz · 2023-04-18T06:01:31Z

lib/src/git.rs

+    // TODO 2.5: If remote branches were stored here, they should no longer be
+    // stored in jj's normal View and the format of the file would no longer be a
+    // View object. This would allow us to treat the state exported to the git repo


I see this file as a way of detecting what has changed in git since last time. I suppose you can think of it much like a merge base used when merging git's current state with jj's current state. So we would still need to store the current state in the view, I think.

I think we can consider removing the git_refs we track, but that's a separate question of cost/benefit (do users care enough about seeing them in log output?). I don't want to remove the tracking of remote branches in general, especially since that's not specific to the git backend, so we can't rely on git's state.

lib/src/git.rs

martinvonz · 2023-04-18T15:30:52Z

lib/src/git.rs

+/// Reads the last seen state of git refs from a file, updates it with the
+/// provided function, and writes the result back to disk.
+///
+/// The state currently tracks only the local branches.


Do you think we need to track remote-tracking branches too? Since they can't be modified directly (only via import and undo/restore), I feel like we shouldn't track them and instead consider the Git repo the source of truth.

This became a confusing question for me once I started thinking about the issue of concurrency and distributed filesystems, where we can't really trust git. Ignoring those questions, you are probably right.

However, if a repo is on Dropbox and the remote-tracking branch in git gets corrupted, the current behavior of creating a conflict is nice. Or should we just lose the remote tracking branch position in that case? This becomes more important when we try to do something like --force-with-lease.

I'm also not sure if the same answer is good for colocated and non-colocated repositories.

Also, what you said here seems to contradict what you said in #1487 (comment). Does this mean you are in the same uncertain position I am, or did I misunderstand your point?

I agree that this is confusing, but I don't see the contradiction between these two comments. I think both are saying that we should store our own concept of remote-tracking branches in the view object but probably not in the new file introduced by this PR.

if a repo is on Dropbox and the remote-tracking branch in git gets corrupted, the current behavior of creating a conflict is nice

Yes, I agree. Ah, it was probably my "Do you think we need to track remote-tracking branches too?" that sounded like I was saying that we shouldn't even track them in our model? I meant it in the context of the new file introduced by this PR, i.e. that I don't think we need to track remote-tracking branches in that file.

Makes sense?

sounded like I was saying that we shouldn't even track them in our model?

Yes, that's what I was thinking about.

I don't think we need to track remote-tracking branches in that file. Makes sense?

I think it makes sense in practice; that's what the reality would be after this PR. I'm not 100% sure it works in theory :). In other words, it makes sense in the short term, and I'm still confused about the long term.

Mostly, that's because I was hoping that we could have a framework where exported branches and remote-tracking branches are treated in the same way. That may simply be a flawed idea, but I haven't adjusted my mind to a consistent theory of how it should work otherwise. I guess we forget jj's view of remote tracking branches whenever there is a conflict or git loses this information and wait for the next fetch. Hopefully this will work OK for our version of --force-with-lease.

I guess what I'm saying in the last paragraph is that I'm not sure there's a behavior that keeps the remote-tracking branches in jj's model, makes sense, doesn't track the conflict base for conflits between jj and git, works with undo (or can we compromise here?), and generates benefit compared to only tracking them in git. I'm also not sure it doesn't exist. Finally, I'm additionally not sure how the current jj behavior measures up on the last two points.

lib/src/git.rs

The only change of behavior here is that an empty `last_seen_git_refs` file is created on export. It is also read but ignored. The goal is to encapsulate the changes in behavior in the next commit.

…file The lock is called "git_refs.lock" since it should also protect against concurrent modification of refs in the backing Git repo.

…-vcs#922 This commit essentially reverts 8a440d8 AKA jj-vcs@4dfd765 The problem that commit is that the Git repo is, unaffected by `jj undo`. So, it is important for JJ's view of the git refs to be unaffected by `jj undo`. Fixes jj-vcs#922. Also creates a bug that is fixed in a follow-up commit

This TODO may address the bugs demoed by subsequent commits.

If it doesn't exist, generate it from git refs as jj did before. This should work as long as the last operation wasn't "undo" or "restore". This is a temporary behavior designed for people upgrading from older jj. It may also be useful if the file gets corrupted by concurrent operations that ignore the lock (e.g. Dropbox). It's unclear if it should be kept or removed.

This will make it more similar to the test introduced in the next commit.

At this point, it works more or less correctly. However, in the next commit adding another `jj push` breaks this test.

…-undo test This change should not affect the test at all, but does.

…push This reveals another bug I found.

ilyagr · 2023-05-18T01:13:21Z

All parts of this PR are now obsolete:

The general approach was superseded by the approach cli: when restoring to a previous operation, don't roll back git_refs #1541, though it's unclear at the moment what version of that might be submitted.
The tests added mostly just work with cli: when restoring to a previous operation, don't roll back git_refs #1541 + analogous change to remote branches
The various cleanups moved to Fix #864 (fetching deleted/moved branches in colocated repos) #1600 and are now submitted.

ilyagr force-pushed the export branch from 106db05 to a6c8c2a Compare April 6, 2023 02:22

ilyagr added the colocated label Apr 6, 2023

ilyagr force-pushed the export branch 4 times, most recently from 1488965 to 26f10ba Compare April 6, 2023 21:33

ilyagr marked this pull request as ready for review April 6, 2023 21:33

ilyagr force-pushed the export branch from 26f10ba to 8670ead Compare April 6, 2023 21:36

martinvonz reviewed Apr 7, 2023

View reviewed changes

lib/src/simple_op_store.rs Outdated Show resolved Hide resolved

lib/src/git.rs Outdated Show resolved Hide resolved

lib/src/git.rs Outdated Show resolved Hide resolved

lib/src/git.rs Outdated Show resolved Hide resolved

lib/src/git.rs Outdated Show resolved Hide resolved

ilyagr commented Apr 7, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

martinvonz reviewed Apr 8, 2023

View reviewed changes

lib/src/git.rs Outdated Show resolved Hide resolved

tests/test_git_colocated.rs Outdated Show resolved Hide resolved

tests/test_git_colocated.rs Outdated Show resolved Hide resolved

ilyagr force-pushed the export branch from 8670ead to 16a9447 Compare April 9, 2023 04:40

ilyagr commented Apr 10, 2023

View reviewed changes

lib/src/git.rs Outdated Show resolved Hide resolved

ilyagr force-pushed the export branch 2 times, most recently from 0eae94b to 2907628 Compare April 10, 2023 06:27

ilyagr marked this pull request as draft April 10, 2023 15:35

ilyagr force-pushed the export branch 2 times, most recently from 30688f0 to 7a94558 Compare April 17, 2023 23:18

ilyagr force-pushed the export branch 2 times, most recently from 4d6c281 to 4360256 Compare April 17, 2023 23:28

ilyagr force-pushed the export branch from 4360256 to 26fae54 Compare April 17, 2023 23:39

ilyagr marked this pull request as ready for review April 17, 2023 23:39

ilyagr force-pushed the export branch from 26fae54 to fcfb89a Compare April 18, 2023 06:22

martinvonz reviewed Apr 18, 2023

View reviewed changes

ilyagr force-pushed the export branch 2 times, most recently from 570de0d to e0d297b Compare April 18, 2023 22:32

ilyagr force-pushed the export branch 3 times, most recently from 149b273 to 599cc7c Compare April 19, 2023 03:04

ilyagr mentioned this pull request Apr 19, 2023

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

Closed

yuja mentioned this pull request Apr 22, 2023

cli: when restoring to a previous operation, don't roll back git_refs #1541

Closed

4 tasks

ilyagr marked this pull request as draft April 28, 2023 01:19

ilyagr force-pushed the export branch from 599cc7c to 06f43f4 Compare May 1, 2023 21:23

ilyagr added 12 commits May 2, 2023 14:47

git export/import: Create a proto to track git's state

31e1b13

Create a type to wrap the proto (no-op)

abb3a01

git export: plumb proto through git.rs without using it (almost no-op)

b4d2d18

The only change of behavior here is that an empty `last_seen_git_refs` file is created on export. It is also read but ignored. The goal is to encapsulate the changes in behavior in the next commit.

git import/export: hold a lock when modifying the git_last_seen_refs …

0333c60

…file The lock is called "git_refs.lock" since it should also protect against concurrent modification of refs in the backing Git repo.

git import: Store branches on import as well

2952b1c

git fetch and git push: add a TODO about with_last_seen_git_refs

3dceb79

This TODO may address the bugs demoed by subsequent commits.

test_git_colocated: minor changes to test_git_colocated_squash_undo

3131646

This will make it more similar to the test introduced in the next commit.

test_git_colocated.rs: add a squash_push_undo test

0e204b3

At this point, it works more or less correctly. However, in the next commit adding another `jj push` breaks this test.

test_git_colocated.rs: Demo bug by adding another push to squash-push…

390de4b

…-undo test This change should not affect the test at all, but does.

test_git_colocated.rs: Demo bug with undoing branch creation after a …

6d23be7

…push This reveals another bug I found.

ilyagr force-pushed the export branch from 06f43f4 to 6d23be7 Compare May 2, 2023 22:15

ilyagr mentioned this pull request May 14, 2023

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

Merged

3 tasks

ilyagr closed this May 18, 2023

ilyagr mentioned this pull request Jun 26, 2023

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

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #922 and uncover additional bugs #1487

Fix #922 and uncover additional bugs #1487

ilyagr commented Apr 6, 2023 •

edited

Loading

martinvonz left a comment

ilyagr commented Apr 17, 2023 •

edited

Loading

martinvonz commented Apr 17, 2023

ilyagr commented Apr 17, 2023

ilyagr commented Apr 17, 2023

martinvonz Apr 18, 2023

ilyagr Apr 18, 2023 •

edited

Loading

martinvonz Apr 18, 2023

ilyagr Apr 18, 2023 •

edited

Loading

chooglen Apr 19, 2023 •

edited

Loading

martinvonz Apr 18, 2023

martinvonz Apr 18, 2023

ilyagr Apr 18, 2023

ilyagr Apr 18, 2023

martinvonz Apr 18, 2023

ilyagr Apr 18, 2023

ilyagr Apr 18, 2023 •

edited

Loading

ilyagr commented May 18, 2023

Fix #922 and uncover additional bugs #1487

Fix #922 and uncover additional bugs #1487

Conversation

ilyagr commented Apr 6, 2023 • edited Loading

Checklist

martinvonz left a comment

Choose a reason for hiding this comment

ilyagr commented Apr 17, 2023 • edited Loading

martinvonz commented Apr 17, 2023

ilyagr commented Apr 17, 2023

ilyagr commented Apr 17, 2023

Choose a reason for hiding this comment

ilyagr Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyagr Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

chooglen Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyagr Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

ilyagr commented May 18, 2023

ilyagr commented Apr 6, 2023 •

edited

Loading

ilyagr commented Apr 17, 2023 •

edited

Loading

ilyagr Apr 18, 2023 •

edited

Loading

ilyagr Apr 18, 2023 •

edited

Loading

chooglen Apr 19, 2023 •

edited

Loading

ilyagr Apr 18, 2023 •

edited

Loading