just playing with tangled
0
fork

Configure Feed

Select the types of activity you want to include in your feed.

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

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

+10 -2
+10 -2
lib/src/git.rs
··· 372 372 /// Reads the last seen state of git refs from a file, updates it with the 373 373 /// provided function, and writes the result back to disk. 374 374 /// 375 + /// This cannot be done concurrently, and is thus done under a lock. 376 + /// 375 377 /// The state currently tracks only the local branches. 376 378 fn with_last_seen_refs<Ret, Err, Err2: From<GitRefViewError> + From<Err>>( 377 379 repo_path: PathBuf, 378 380 f: impl FnOnce(&GitRefView) -> Result<(GitRefView, Ret), Err>, 379 381 ) -> Result<Ret, Err2> { 380 - // Short-term TODO 1: Modification of this file, as well as the git refs in the 381 - // git storage, should be protected by a lock. 382 + // TODO 1: It might be better to lock using a libgit2 lock or a git-native lock, 383 + // if feasible. 384 + // 385 + // See also https://github.com/git/git/blob/f285f68a/lockfile.h. 382 386 // 383 387 // TODO 2: Conceptually, this state should contain all the information in the 384 388 // repo that's not stored by git and should not be affected by `jj undo`. In ··· 390 394 // View object. This would allow us to treat the state exported to the git repo 391 395 // similar to any other remote and unify `jj git push` and `jj git export` to a 392 396 // large degree. 397 + let lock = crate::lock::FileLock::lock(repo_path.join("git_refs.lock")); 393 398 let last_seen_refs_path = repo_path.join("git_last_seen_refs"); 394 399 let old_view = GitRefView::read_view_from_file(last_seen_refs_path.clone()) 395 400 // TODO: Do different things on errors other than "not found"? ··· 397 402 .unwrap_or(GitRefView::default()); 398 403 let (new_view, ret) = f(&old_view)?; 399 404 new_view.write_view_to_file(last_seen_refs_path)?; 405 + // TODO: Create a test that checks the lock lives long enough. I don't know if 406 + // removing the following line would break such a test. 407 + drop(lock); 400 408 Ok(ret) 401 409 } 402 410