Skip to content

Commit

Permalink
git.rs: Fix bug where a forgotten branch couldn't be fetched
Browse files Browse the repository at this point in the history 
The fix is slightly messy, but I don't know a better way without reworking
the logic of `jj git fetch` and `jj git push` in-depth.

This bug was originally reported by @arxanas in a [Discord discussion].

[Discord discussion]: https://discord.com/channels/968932220549103686/969829516539228222/1114997445949136936

Summary:

> How do I proceed here to check out the remote branch that I know exists?
> 
> ```
> $ jj git fetch --branch 'arxanas/fsmonitor' --remote origin
> Nothing changed.
> $ jj checkout arxanas/fsmonitor                            
> Error: Revision "arxanas/fsmonitor" doesn't exist
> $ jj checkout arxanas/fsmonitor@origin
> Error: Revision "arxanas/fsmonitor@origin" doesn't exist
> $ git show origin/arxanas/fsmonitor                        
> commit bac43af (origin/arxanas/fsmonitor, refs/jj/keep/bac43af9fbaade36a2374f4b2a7457ca4caa05ee)
> Author: Waleed Khan <[email protected]>
> Date:   Fri Jun 10 18:58:21 2022 -0700
> 
>     fsmonitor: add `.watchmanconfig` to repo
>     
>     This identifies the directory as Watchman-enabled. Additional config settings can go in this file.
> 
> diff --git a/.watchmanconfig b/.watchmanconfig
> new file mode 100644
> index 00000000..0967ef42
> --- /dev/null
> +++ b/.watchmanconfig
> @@ -0,0 +1 @@
> +{}
> ```
  • Loading branch information
@ilyagr
ilyagr committed Jun 20, 2023
1 parent d581569 commit fc9a791
Show file tree
Hide file tree
Showing 4 changed files with 133 additions and 23 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
remote, just like in a non-colocated repo.
[#864](https://github.com/martinvonz/jj/issues/864)

* `jj git fetch` can now restore branches forgotten by `jj branch forget`.
[#1714](https://github.com/martinvonz/jj/pull/1714)

## [0.7.0] - 2023-02-16

### Breaking changes
Expand Down
83 changes: 72 additions & 11 deletions lib/src/git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,36 @@ fn prevent_gc(git_repo: &git2::Repository, id: &CommitId) -> Result<(), git2::Er
Ok(())
}

#[derive(Clone, Debug, PartialEq)]
pub enum ImportMethod {
Skip,
Merge,
/// Used by `jj git fetch` so that it can resurrect forgotten branches, even
/// if they did not move on the remote.
///
/// See `test_branch_forget_fetched_branch` in test_branch_command.rs for a
/// demonstration.
///
/// This is needed because:
///
/// - `jj branch forget` deletes the jj's remote-tracking branches, but not
/// the refs tracking the git repo's remote-tracking branches. This is
/// crucial in colocated branches, where deleting that ref would cause a
/// subsequent `jj git import` to immediately resurrect the branch.
///
/// - `jj git fetch` needs to fetch such branches, otherwise it is very
/// difficult for the user to recover a forgotten branch that they know
/// exists on the remote.
///
/// Another long-term solution would be for `jj git push/fetch` to neither
/// rely on nor modify the local git repo's remote-tracking branches in
/// a normal jj repo. Then, 'jj git fetch' would respond correctly when
/// jj's remote-tracking branches are removed. In a colocated repo, it
/// would be `jj git export`'s job to update the git repo's
/// remote-tracking branches.
MergeOrResurrect,
}

/// Reflect changes made in the underlying Git repo in the Jujutsu repo.
///
/// This function detects conflicts (if both Git and JJ modified a branch) and
Expand All @@ -95,7 +125,7 @@ pub fn import_refs(
git_repo: &git2::Repository,
git_settings: &GitSettings,
) -> Result<(), GitImportError> {
import_some_refs(mut_repo, git_repo, git_settings, |_| true)
import_some_refs(mut_repo, git_repo, git_settings, |_| ImportMethod::Merge)
}

/// Reflect changes made in the underlying Git repo in the Jujutsu repo.
Expand All @@ -106,7 +136,7 @@ pub fn import_some_refs(
mut_repo: &mut MutableRepo,
git_repo: &git2::Repository,
git_settings: &GitSettings,
git_ref_filter: impl Fn(&str) -> bool,
git_ref_filter: impl Fn(&str) -> ImportMethod,
) -> Result<(), GitImportError> {
let store = mut_repo.store().clone();
let mut jj_view_git_refs = mut_repo.view().git_refs().clone();
Expand Down Expand Up @@ -144,9 +174,10 @@ pub fn import_some_refs(
continue;
}
let full_name = git_repo_ref.name().unwrap().to_string();
if let Some(RefName::RemoteBranch { branch, remote: _ }) = parse_git_ref(&full_name) {
let ref_name = parse_git_ref(&full_name);
if let Some(RefName::RemoteBranch { branch, remote: _ }) = &ref_name {
// "refs/remotes/origin/HEAD" isn't a real remote-tracking branch
if &branch == "HEAD" {
if branch == "HEAD" {
continue;
}
}
Expand All @@ -159,13 +190,35 @@ pub fn import_some_refs(
};
let id = CommitId::from_bytes(git_commit.id().as_bytes());
pinned_git_heads.insert(full_name.to_string(), vec![id.clone()]);
if !git_ref_filter(&full_name) {
continue;
}

let allow_resurrection = match git_ref_filter(&full_name) {
ImportMethod::Skip => continue,
ImportMethod::Merge => false,
ImportMethod::MergeOrResurrect => true,
};
// TODO: Make it configurable which remotes are publishing and update public
// heads here.
let old_target = jj_view_git_refs.remove(&full_name);
let new_target = Some(RefTarget::Normal(id.clone()));
let old_target = jj_view_git_refs.remove(&full_name);
let old_target = if allow_resurrection
&& new_target == old_target
&& ref_name
.map(|ref_name| mut_repo.view().get_ref(&ref_name).is_none())
.unwrap_or(false)
{
// Normally, if the jj local ref is None and `new_target==old_target`, merging
// these three will result in the jj local ref staying nonexistent.
//
// Changing the `old_target` to None will cause the jj local ref to get
// resurrected from `new_target`. Since we only do this when the local ref does
// not exist, this will never cause conflicts.
//
// This behavior will occur for remote-tracking branches during a `jj git
// fetch`. For motivation, see the docstring of ImportMethod::MergeOrResurrect.
None
} else {
old_target
};
if new_target != old_target {
prevent_gc(git_repo, &id)?;
mut_repo.set_git_ref(full_name.clone(), RefTarget::Normal(id.clone()));
Expand All @@ -175,7 +228,7 @@ pub fn import_some_refs(
}
}
for (full_name, target) in jj_view_git_refs {
if git_ref_filter(&full_name) {
if git_ref_filter(&full_name) != ImportMethod::Skip {
mut_repo.remove_git_ref(&full_name);
changed_git_refs.insert(full_name, (Some(target), None));
} else {
Expand Down Expand Up @@ -488,10 +541,18 @@ pub fn fetch(
mut_repo,
git_repo,
git_settings,
move |git_ref_name: &str| -> bool { regex.is_match(git_ref_name) },
move |git_ref_name: &str| -> ImportMethod {
if regex.is_match(git_ref_name) {
ImportMethod::MergeOrResurrect
} else {
ImportMethod::Skip
}
},
)
} else {
import_refs(mut_repo, git_repo, git_settings)
import_some_refs(mut_repo, git_repo, git_settings, |_| {
ImportMethod::MergeOrResurrect
})
}
.map_err(|err| match err {
GitImportError::InternalGitError(source) => GitFetchError::InternalGitError(source),
Expand Down
57 changes: 50 additions & 7 deletions lib/tests/test_git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@ use jujutsu_lib::backend::{
};
use jujutsu_lib::commit::Commit;
use jujutsu_lib::commit_builder::CommitBuilder;
use jujutsu_lib::git;
use jujutsu_lib::git::{GitFetchError, GitPushError, GitRefUpdate};
use jujutsu_lib::git::{self, GitFetchError, GitPushError, GitRefUpdate, ImportMethod};
use jujutsu_lib::git_backend::GitBackend;
use jujutsu_lib::op_store::{BranchTarget, RefTarget};
use jujutsu_lib::repo::{MutableRepo, ReadonlyRepo, Repo};
Expand Down Expand Up @@ -71,6 +70,14 @@ fn get_git_repo(repo: &Arc<ReadonlyRepo>) -> git2::Repository {
.git_repo_clone()
}

fn into_import_method(select: bool) -> ImportMethod {
if select {
ImportMethod::Merge
} else {
ImportMethod::Skip
}
}

#[test]
fn test_import_refs() {
let settings = testutils::user_settings();
Expand Down Expand Up @@ -690,7 +697,7 @@ fn test_import_some_refs() {
// Import branches feature1, feature2, and feature3.
let mut tx = repo.start_transaction(&settings, "test");
git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| {
ref_name.starts_with("refs/remotes/origin/feature")
into_import_method(ref_name.starts_with("refs/remotes/origin/feature"))
})
.unwrap();
tx.mut_repo().rebase_descendants(&settings).unwrap();
Expand Down Expand Up @@ -755,7 +762,7 @@ fn test_import_some_refs() {
delete_git_ref(&git_repo, "refs/remotes/origin/feature4");
let mut tx = repo.start_transaction(&settings, "test");
git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| {
ref_name == "refs/remotes/origin/feature2"
into_import_method(ref_name == "refs/remotes/origin/feature2")
})
.unwrap();
tx.mut_repo().rebase_descendants(&settings).unwrap();
Expand All @@ -771,7 +778,7 @@ fn test_import_some_refs() {
// corresponding commit should stay because it is reachable from feature2.
let mut tx = repo.start_transaction(&settings, "test");
git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| {
ref_name == "refs/remotes/origin/feature1"
into_import_method(ref_name == "refs/remotes/origin/feature1")
})
.unwrap();
// No descendant should be rewritten.
Expand All @@ -788,7 +795,7 @@ fn test_import_some_refs() {
// feature4 should be left alone even though it is no longer in git.
let mut tx = repo.start_transaction(&settings, "test");
git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| {
ref_name == "refs/remotes/origin/feature3"
into_import_method(ref_name == "refs/remotes/origin/feature3")
})
.unwrap();
// No descendant should be rewritten
Expand All @@ -804,7 +811,7 @@ fn test_import_some_refs() {
// Import feature4: both the head and the branch will disappear.
let mut tx = repo.start_transaction(&settings, "test");
git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| {
ref_name == "refs/remotes/origin/feature4"
into_import_method(ref_name == "refs/remotes/origin/feature4")
})
.unwrap();
// No descendant should be rewritten
Expand All @@ -818,6 +825,42 @@ fn test_import_some_refs() {
jj_id(&commit_feat2),
};
assert_eq!(*view.heads(), expected_heads);

// Now, let's forget feature2
let mut tx = repo.start_transaction(&settings, "test");
tx.mut_repo().remove_branch("feature2");
assert_eq!(tx.mut_repo().rebase_descendants(&settings).unwrap(), 0);
let repo = tx.commit();

assert_eq!(repo.view().branches().len(), 0);

// A normal import does not recreate it since the local git repo didn't change
let mut tx = repo.start_transaction(&settings, "test");
git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| {
if ref_name == "refs/remotes/origin/feature2" {
ImportMethod::Merge
} else {
ImportMethod::Skip
}
})
.unwrap();
let repo = tx.commit();

assert_eq!(repo.view().branches().len(), 0);

// A "resurrection" import does recreate it
let mut tx = repo.start_transaction(&settings, "test");
git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| {
if ref_name == "refs/remotes/origin/feature2" {
ImportMethod::MergeOrResurrect
} else {
ImportMethod::Skip
}
})
.unwrap();
let repo = tx.commit();

assert_eq!(repo.view().branches().len(), 1);
}

fn git_ref(git_repo: &git2::Repository, name: &str, target: Oid) {
Expand Down
13 changes: 8 additions & 5 deletions tests/test_branch_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,13 +233,16 @@ fn test_branch_forget_fetched_branch() {
"###);
insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @"");

// Short-term TODO: Fix this BUG. It should be possible to fetch `feature1`
// again.
// On the other hand, `jj git fetch` does resurrect the branch. This is
// important, as otherwise there wouldn't be a good way to resurrect it in
// non-colocated repos.
//
// See the docstring of `ImportMethod::MergeOrResurrect` for technical details.
let stdout = test_env.jj_cmd_success(&repo_path, &["git", "fetch", "--remote=origin"]);
insta::assert_snapshot!(stdout, @r###"
Nothing changed.
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
feature1: 9f01a0e04879 message
"###);
insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @"");
}

// TODO: Test `jj branch list` with a remote named `git`
Expand Down

0 comments on commit fc9a791

Please sign in to comment.