Skip to content

change-id: avoid prefix collision when creating a new revision #8146

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.

Sign up for GitHub

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

Draft
glehmann wants to merge 1 commit into
base: main
Choose a base branch
from
+107 −39
Draft

change-id: avoid prefix collision when creating a new revision #8146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions cli/src/cli_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2354,6 +2354,17 @@ to the current parents may contain changes from multiple commits.

Ok(advanceable_bookmarks)
}

pub fn generate_new_change_id(&self) -> ChangeId {
let id_prefix_index = self
.id_prefix_context()
.populate(self.repo().base_repo())
.unwrap();
id_prefix_index.generate_new_change_id(
self.settings().get_rng(),
self.repo().store().change_id_length(),
)
}
}

#[cfg(feature = "git")]
Expand Down Expand Up @@ -2472,6 +2483,17 @@ impl WorkspaceCommandTransaction<'_> {
self.helper.env.parse_template(ui, &language, template_text)
}

pub fn generate_new_change_id(&self) -> ChangeId {
let id_prefix_context = self
.id_prefix_context
.get_or_init(|| self.helper.env.new_id_prefix_context());
let id_prefix_index = id_prefix_context.populate(self.repo()).unwrap();
id_prefix_index.generate_new_change_id(
self.settings().get_rng(),
self.repo().store().change_id_length(),
)
}

pub fn finish(self, ui: &Ui, description: impl Into<String>) -> Result<(), CommandError> {
if !self.tx.repo().has_changes() {
writeln!(ui.status(), "Nothing changed.")?;
Expand Down
2 changes: 2 additions & 0 deletions cli/src/commands/new.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,11 @@ pub(crate) fn cmd_new(

let mut tx = workspace_command.start_transaction();
let merged_tree = merge_commit_trees(tx.repo(), &parent_commits).block_on()?;
let change_id = tx.generate_new_change_id();
let mut commit_builder = tx
.repo_mut()
.new_commit(parent_commit_ids, merged_tree)
.set_change_id(change_id)
.detach();
let mut description = join_message_paragraphs(&args.message_paragraphs);
if !description.is_empty() {
Expand Down
78 changes: 39 additions & 39 deletions cli/tests/test_squash_command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,8 +467,8 @@ fn test_squash_from_to() {
work_dir.write_file("file2", "f\n");
// Test the setup
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ 0fac1124d1ad f
○ 4ebe104a0e4e e
@ 20ee6d1e7970 f
○ e5cef3db24a3 e
○ dc71a460d5d6 d
│ ○ ee0b260ffc44 c
│ ○ e31bf988d7c9 b
Expand All @@ -491,14 +491,14 @@ fn test_squash_from_to() {
let output = work_dir.run_jj(["squash", "--from", "c"]);
insta::assert_snapshot!(output, @r"
------- stderr -------
Working copy (@) now at: kmkuslsw 941ab024 f | (no description set)
Parent commit (@-) : znkkpsqq 4ebe104a e | (no description set)
Working copy (@) now at: yptpptny 1bd346c6 f | (no description set)
Parent commit (@-) : uuzqqzqu e5cef3db e | (no description set)
Added 0 files, modified 1 files, removed 0 files
[EOF]
");
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ 941ab024b3f8 f
○ 4ebe104a0e4e e
@ 1bd346c685ba f
○ e5cef3db24a3 e
○ dc71a460d5d6 d
│ ○ e31bf988d7c9 b c
├─╯
Expand All @@ -524,15 +524,15 @@ fn test_squash_from_to() {
let output = work_dir.run_jj(["squash", "--from", "@--"]);
insta::assert_snapshot!(output, @r"
------- stderr -------
Working copy (@) now at: kmkuslsw c102d2c4 f | (no description set)
Parent commit (@-) : znkkpsqq beb7c033 e | (no description set)
Working copy (@) now at: yptpptny de2de8c2 f | (no description set)
Parent commit (@-) : uuzqqzqu d9d24d72 e | (no description set)
[EOF]
");
// The change has been removed from the source (the change pointed to by 'd'
// became empty and was abandoned)
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ c102d2c4e165 f
○ beb7c0338f7c e
@ de2de8c25312 f
○ d9d24d72b085 e
│ ○ ee0b260ffc44 c
│ ○ e31bf988d7c9 b
├─╯
Expand All @@ -554,14 +554,14 @@ fn test_squash_from_to() {
insta::assert_snapshot!(output, @r"
------- stderr -------
Rebased 1 descendant commits
Working copy (@) now at: kmkuslsw 1bc21d4e f | (no description set)
Working copy (@) now at: yptpptny 1c5c6afa f | (no description set)
Parent commit (@-) : vruxwmqv 8b6b080a d e | (no description set)
[EOF]
");
// The change has been removed from the source (the change pointed to by 'e'
// became empty and was abandoned)
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ 1bc21d4e92d6 f
@ 1c5c6afae007 f
○ 8b6b080ab587 d e
│ ○ ee0b260ffc44 c
│ ○ e31bf988d7c9 b
Expand Down Expand Up @@ -856,8 +856,8 @@ fn test_squash_from_multiple() {
work_dir.write_file("file", "f\n");
// Test the setup
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ 65e53f39b4d6 f
○ 7dc592781647 e
@ be45b16bca62 f
○ 6b07f868b05a e
├─┬─╮
│ │ ○ fed4d1a2e491 b
│ ○ │ d7e94ec7e73e c
Expand All @@ -872,11 +872,11 @@ fn test_squash_from_multiple() {

// Squash a few commits sideways
let output = work_dir.run_jj(["squash", "--from=b", "--from=c", "--into=d"]);
insta::assert_snapshot!(output, @r###"
insta::assert_snapshot!(output, @r"
------- stderr -------
Rebased 2 descendant commits
Working copy (@) now at: kpqxywon 0b695306 f | (no description set)
Parent commit (@-) : yostqsxw ff064d52 e | (no description set)
Working copy (@) now at: vzqnnsmr ca2f46ec f | (no description set)
Parent commit (@-) : wmwvqwsz 9b7a17f7 e | (no description set)
New conflicts appeared in 1 commits:
yqosqzyt 61130da4 d | (conflict) (no description set)
Hint: To resolve the conflicts, start by creating a commit on top of
Expand All @@ -886,10 +886,10 @@ fn test_squash_from_multiple() {
Once the conflicts are resolved, you can inspect the result with `jj diff`.
Then run `jj squash` to move the resolution into the conflicted commit.
[EOF]
"###);
");
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ 0b6953066ee0 f
○ ff064d529578 e
@ ca2f46ec9064 f
○ 9b7a17f7609c e
├─╮
× │ 61130da4e714 d
├─╯
Expand Down Expand Up @@ -919,13 +919,13 @@ fn test_squash_from_multiple() {
insta::assert_snapshot!(output, @r"
------- stderr -------
Rebased 1 descendant commits
Working copy (@) now at: xznxytkn ec32238b (empty) (no description set)
Parent commit (@-) : yostqsxw 5298eef6 e f | (no description set)
Working copy (@) now at: xznxytkn 922c633e (empty) (no description set)
Parent commit (@-) : wmwvqwsz ee0db625 e f | (no description set)
[EOF]
");
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ ec32238b2be5 (empty)
○ 5298eef6bca5 e f
@ 922c633ef376 (empty)
○ ee0db6253f50 e f
├─╮
○ │ 8acbb71558d5 d
├─╯
Expand Down Expand Up @@ -1000,8 +1000,8 @@ fn test_squash_from_multiple_partial() {
work_dir.write_file("file2", "f\n");
// Test the setup
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ 4558bd852475 f
○ e2db96b2e57a e
@ 463a1aa77462 f
○ 3e60cb5b3666 e
├─┬─╮
│ │ ○ f2c9709f39e9 b
│ ○ │ aa908686a197 c
Expand All @@ -1016,11 +1016,11 @@ fn test_squash_from_multiple_partial() {

// Partially squash a few commits sideways
let output = work_dir.run_jj(["squash", "--from=b|c", "--into=d", "file1"]);
insta::assert_snapshot!(output, @r###"
insta::assert_snapshot!(output, @r"
------- stderr -------
Rebased 2 descendant commits
Working copy (@) now at: kpqxywon a724910c f | (no description set)
Parent commit (@-) : yostqsxw 1bc405e1 e | (no description set)
Working copy (@) now at: vzqnnsmr 8ef3674e f | (no description set)
Parent commit (@-) : wmwvqwsz 166fb3eb e | (no description set)
New conflicts appeared in 1 commits:
yqosqzyt 7ddfe685 d | (conflict) (no description set)
Hint: To resolve the conflicts, start by creating a commit on top of
Expand All @@ -1030,10 +1030,10 @@ fn test_squash_from_multiple_partial() {
Once the conflicts are resolved, you can inspect the result with `jj diff`.
Then run `jj squash` to move the resolution into the conflicted commit.
[EOF]
"###);
");
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ a724910cd361 f
○ 1bc405e12b68 e
@ 8ef3674ee02b f
○ 166fb3eb9514 e
├─┬─╮
│ │ ○ e9db15b956c4 b
│ ○ │ 83cbe51db94d c
Expand Down Expand Up @@ -1084,13 +1084,13 @@ fn test_squash_from_multiple_partial() {
insta::assert_snapshot!(output, @r"
------- stderr -------
Rebased 1 descendant commits
Working copy (@) now at: kpqxywon b5a40c15 f | (no description set)
Parent commit (@-) : yostqsxw 5dea187c e | (no description set)
Working copy (@) now at: vzqnnsmr 7d7c487a f | (no description set)
Parent commit (@-) : wmwvqwsz 59425fbc e | (no description set)
[EOF]
");
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ b5a40c154611 f
○ 5dea187c414d e
@ 7d7c487ac4e1 f
○ 59425fbc3dff e
├─┬─╮
│ │ ○ 8b9afc05ca07 b
│ ○ │ 5630471a8fd5 c
Expand Down Expand Up @@ -1152,7 +1152,7 @@ fn test_squash_from_multiple_partial_no_op() {
// Test the setup
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ fdb92bc249a0 d
│ ○ 0dc8cb72859d c
│ ○ 774827326ca0 c
├─╯
│ ○ b1a17f79a1a5 b
├─╯
Expand All @@ -1173,7 +1173,7 @@ fn test_squash_from_multiple_partial_no_op() {
");
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ 6dfc239e2ba3 d
│ ○ 0dc8cb72859d c
│ ○ 774827326ca0 c
├─╯
○ 93d495c46d89 a
◆ 000000000000 (empty)
Expand Down Expand Up @@ -1204,7 +1204,7 @@ fn test_squash_from_multiple_partial_no_op() {
");
insta::assert_snapshot!(get_log_output(&work_dir), @r"
@ fdb92bc249a0 d
│ ○ 0dc8cb72859d c
│ ○ 774827326ca0 c
├─╯
│ ○ b1a17f79a1a5 b
├─╯
Expand Down
44 changes: 44 additions & 0 deletions lib/src/id_prefix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use crate::revset::RevsetResolutionError;
use crate::revset::SymbolResolver;
use crate::revset::SymbolResolverExtension;
use crate::revset::UserRevsetExpression;
use crate::settings::JJRng;
use crate::view::View;

#[derive(Debug, Error)]
Expand Down Expand Up @@ -275,6 +276,49 @@ impl IdPrefixIndex<'_> {
}
repo.shortest_unique_change_id_prefix_len(change_id)
}

pub fn generate_new_change_id(
&self,
rng: Arc<JJRng>,
repo_change_id_length: usize,
) -> ChangeId {
if let Some(indexes) = self.indexes {
// generate a set of change-ids, and keep the one that produce the shortest
// prefix
let mut change_id = None;
let mut change_id_length = repo_change_id_length + 1;
// 47 for a .95 probability to find the last free 1-length prefix (1-(15/16)^47)
for _ in 0..47 {
let id = rng.new_change_id(repo_change_id_length);
let length = prefix_length(&id, indexes);
if length == 1 {
// it can't be shorter, stop there
return id;
}
if length < change_id_length {
change_id = Some(id);
change_id_length = length;
}
}
change_id.expect("a change-id to be there")
} else {
rng.new_change_id(repo_change_id_length)
}
}
}

fn prefix_length(change_id: &ChangeId, indexes: &Indexes) -> usize {
let change_id_str = change_id.to_string();
for length in 1..change_id_str.len() {
let prefix = HexPrefix::try_from_reverse_hex(&change_id_str[0..length]).unwrap();
let resolution = indexes
.change_index
.resolve_prefix_to_key(&*indexes.commit_change_ids, &prefix);
if let PrefixResolution::NoMatch = resolution {
return length;
}
}
return change_id_str.len();
}

fn disambiguate_prefix_with_refs(view: &View, id_sym: &str, min_len: usize) -> usize {
Expand Down
Loading