Skip to content

Fix gerrit upload with mixed explicit and implicit change-ids #8219

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

Open
HoelzelJon wants to merge 3 commits into
base: main
Choose a base branch
from
+232 −19
Open

Fix gerrit upload with mixed explicit and implicit change-ids #8219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
051464c
gerrit: add test for all-explicit change-ids for upload command
HoelzelJon Dec 5, 2025
eb73ede
gerrit: handle mixed explicit & implicit Change-Ids in upload
HoelzelJon Nov 19, 2025
a02f780
docs: Add myself as paid contributor (under IMC Trading)
HoelzelJon Dec 4, 2025
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

* Broken symlink on Windows. [#6934](https://github.com/jj-vcs/jj/issues/6934).

* `jj gerrit upload` now correctly handles mixed explicit and implicit
Change-Ids in chains of commits ([#8219](https://github.com/jj-vcs/jj/pull/8219))

## [0.36.0] - 2025-12-03

### Release highlights
Expand Down
32 changes: 15 additions & 17 deletions cli/src/commands/gerrit/upload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ pub fn cmd_gerrit_upload(

// The user can choose to explicitly set their own change-ID to
// override the default change-ID based on the jj change-ID.
if let Some(trailer) = change_id_trailers.first() {
let new_description = if let Some(trailer) = change_id_trailers.first() {
// Check the change-id format is correct.
if trailer.value.len() != 41 || !trailer.value.starts_with('I') {
// Intentionally leave the invalid change IDs as-is.
Expand All @@ -272,22 +272,20 @@ pub fn cmd_gerrit_upload(
)?;
}

// map the old commit to itself
old_to_new.insert(original_commit.id().clone(), original_commit);
continue;
}

// Gerrit change id is 40 chars, jj change id is 32, so we need padding.
// To be consistent with `format_gerrit_change_id_trailer``, we pad with
// 6a6a6964 (hex of "jjid").
let gerrit_change_id = format!("I{}6a6a6964", original_commit.change_id().hex());

let new_description = format!(
"{}{}Change-Id: {}\n",
original_commit.description().trim(),
if trailers.is_empty() { "\n\n" } else { "\n" },
gerrit_change_id
);
original_commit.description().to_owned()
} else {
// Gerrit change id is 40 chars, jj change id is 32, so we need padding.
// To be consistent with `format_gerrit_change_id_trailer``, we pad with
// 6a6a6964 (hex of "jjid").
let gerrit_change_id = format!("I{}6a6a6964", original_commit.change_id().hex());

format!(
"{}{}Change-Id: {}\n",
original_commit.description().trim(),
if trailers.is_empty() { "\n\n" } else { "\n" },
gerrit_change_id
)
};
Copy link
Contributor

@yuja yuja Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test that will upload series that contains all preset Change-Id trailers? In that case, the temporarily rewritten commits should be identical to the original ones. That would probably work because we reset committer timestamps, but it seems we don't have tests.

Copy link
Author

@HoelzelJon HoelzelJon Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a pre-commit that splits out two separate tests for all-preset and all-implied Change-Ids, and then in the main commit, I've added a new test case for the relevant mixed case


let new_parents = original_commit
.parents()
Expand Down
213 changes: 211 additions & 2 deletions cli/tests/test_gerrit_upload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ fn test_gerrit_upload_dryrun() {
}

#[test]
fn test_gerrit_upload_local() {
fn test_gerrit_upload_local_implicit_change_ids() {
let test_env = TestEnvironment::default();
test_env
.run_jj_in(".", ["git", "init", "--colocate", "remote"])
Expand All @@ -104,7 +104,7 @@ fn test_gerrit_upload_local() {
create_commit(&local_dir, "b", &["a@origin"]);
create_commit(&local_dir, "c", &["b"]);

// The output should only mentioned commit IDs from the log output above (no
// The output should only mention commit IDs from the log output above (no
// temporary commits)
let output = local_dir.run_jj(["log", "-r", "all()"]);
insta::assert_snapshot!(output, @r###"
Expand Down Expand Up @@ -170,3 +170,212 @@ fn test_gerrit_upload_local() {
[EOF]
"###);
}

#[test]
fn test_gerrit_upload_local_explicit_change_ids() {
let test_env = TestEnvironment::default();
test_env
.run_jj_in(".", ["git", "init", "--colocate", "remote"])
.success();
let remote_dir = test_env.work_dir("remote");
create_commit(&remote_dir, "a", &[]);

test_env
.run_jj_in(".", ["git", "clone", "remote", "local"])
.success();
let local_dir = test_env.work_dir("local");
create_commit(&local_dir, "b", &["a@origin"]);

// Add an explicit Change-Id footer to b
let output = local_dir.run_jj([
"describe",
"b",
"-m",
"b\n\nChange-Id: Id39b308212fe7e0b746d16c13355f3a90712d7f9\n",
]);
insta::assert_snapshot!(output, @r###"
------- stderr -------
Working copy (@) now at: mzvwutvl 887a7016 b | b
Parent commit (@-) : rlvkpnrz 7d980be7 a@origin | a
[EOF]
"###);

create_commit(&local_dir, "c", &["b"]);

// Add an explicit Change-Id footer to c
let output = local_dir.run_jj([
"describe",
"c",
"-m",
"c\n\nChange-Id: Idfac1e8c149efddf5c7a286f787b43886a6a6964\n",
]);
insta::assert_snapshot!(output, @r###"
------- stderr -------
Working copy (@) now at: vruxwmqv 27c0bdd0 c | c
Parent commit (@-) : mzvwutvl 887a7016 b | b
[EOF]
"###);

// The output should only mention commit IDs from the log output above (no
// temporary commits)
let output = local_dir.run_jj(["log", "-r", "all()"]);
insta::assert_snapshot!(output, @r###"
@ vruxwmqv test.user@example.com 2001-02-03 08:05:16 c 27c0bdd0
│ c
○ mzvwutvl test.user@example.com 2001-02-03 08:05:13 b 887a7016
│ b
◆ rlvkpnrz test.user@example.com 2001-02-03 08:05:09 a@origin 7d980be7
│ a
◆ zzzzzzzz root() 00000000
[EOF]
"###);

let output = local_dir.run_jj(["gerrit", "upload", "-r", "c", "--remote-branch=main"]);
insta::assert_snapshot!(output, @r###"
------- stderr -------

Found 1 heads to push to Gerrit (remote 'origin'), target branch 'main'

Pushing vruxwmqv 27c0bdd0 c | c
[EOF]
"###);

// The output should be unchanged because we only add Change-Id trailers
// transiently
let output = local_dir.run_jj(["log", "-r", "all()"]);
insta::assert_snapshot!(output, @r###"
@ vruxwmqv test.user@example.com 2001-02-03 08:05:16 c 27c0bdd0
│ c
○ mzvwutvl test.user@example.com 2001-02-03 08:05:13 b 887a7016
│ b
◆ rlvkpnrz test.user@example.com 2001-02-03 08:05:09 a@origin 7d980be7
│ a
◆ zzzzzzzz root() 00000000
[EOF]
"###);

// There's no particular reason to run this with jj util exec, it's just that
// the infra makes it easier to run this way.
let output = remote_dir.run_jj(["util", "exec", "--", "git", "log", "refs/for/main"]);
insta::assert_snapshot!(output, @r###"
commit ab2262125f7e2db50ec2646e949beba54219d345
Author: Test User <test.user@example.com>
Date: Sat Feb 3 04:05:14 2001 +0700

c

Change-Id: Idfac1e8c149efddf5c7a286f787b43886a6a6964

commit 22bbfff17a982c9b8f18cc8cbcd59ec833827c41
Copy link
Contributor

@yuja yuja Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so this caught the bug. Maybe we need to copy the original predecessors to rewrite to identical commit?

Author: Test User <test.user@example.com>
Date: Sat Feb 3 04:05:11 2001 +0700

b

Change-Id: Id39b308212fe7e0b746d16c13355f3a90712d7f9

commit 7d980be7a1d499e4d316ab4c01242885032f7eaf
Author: Test User <test.user@example.com>
Date: Sat Feb 3 04:05:08 2001 +0700

a
[EOF]
"###);
}

#[test]
fn test_gerrit_upload_local_mixed_change_ids() {
let test_env = TestEnvironment::default();
test_env
.run_jj_in(".", ["git", "init", "--colocate", "remote"])
.success();
let remote_dir = test_env.work_dir("remote");
create_commit(&remote_dir, "a", &[]);

test_env
.run_jj_in(".", ["git", "clone", "remote", "local"])
.success();
let local_dir = test_env.work_dir("local");
create_commit(&local_dir, "b", &["a@origin"]);
create_commit(&local_dir, "c", &["b"]);

// Add an explicit Change-Id footer to c but not b
let output = local_dir.run_jj([
"describe",
"c",
"-m",
"c\n\nChange-Id: Id39b308212fe7e0b746d16c13355f3a90712d7f9\n",
]);
insta::assert_snapshot!(output, @r###"
------- stderr -------
Working copy (@) now at: yqosqzyt 8d46d915 c | c
Parent commit (@-) : mzvwutvl 3bcb28c4 b | b
[EOF]
"###);

// The output should only mention commit IDs from the log output above (no
// temporary commits)
let output = local_dir.run_jj(["log", "-r", "all()"]);
insta::assert_snapshot!(output, @r###"
@ yqosqzyt test.user@example.com 2001-02-03 08:05:15 c 8d46d915
│ c
○ mzvwutvl test.user@example.com 2001-02-03 08:05:12 b 3bcb28c4
│ b
◆ rlvkpnrz test.user@example.com 2001-02-03 08:05:09 a@origin 7d980be7
│ a
◆ zzzzzzzz root() 00000000
[EOF]
"###);

let output = local_dir.run_jj(["gerrit", "upload", "-r", "c", "--remote-branch=main"]);
insta::assert_snapshot!(output, @r###"
------- stderr -------

Found 1 heads to push to Gerrit (remote 'origin'), target branch 'main'

Pushing yqosqzyt 8d46d915 c | c
[EOF]
"###);

// The output should be unchanged because we only add Change-Id trailers
// transiently
let output = local_dir.run_jj(["log", "-r", "all()"]);
insta::assert_snapshot!(output, @r###"
@ yqosqzyt test.user@example.com 2001-02-03 08:05:15 c 8d46d915
│ c
○ mzvwutvl test.user@example.com 2001-02-03 08:05:12 b 3bcb28c4
│ b
◆ rlvkpnrz test.user@example.com 2001-02-03 08:05:09 a@origin 7d980be7
│ a
◆ zzzzzzzz root() 00000000
[EOF]
"###);

// There's no particular reason to run this with jj util exec, it's just that
// the infra makes it easier to run this way.
let output = remote_dir.run_jj(["util", "exec", "--", "git", "log", "refs/for/main"]);
insta::assert_snapshot!(output, @r###"
commit 015df2b1d38bdc71ae7ef24c2889100e39d34ef8
Author: Test User <test.user@example.com>
Date: Sat Feb 3 04:05:13 2001 +0700

c

Change-Id: Id39b308212fe7e0b746d16c13355f3a90712d7f9

commit 81b723522d1c1a583a045eab5bfb323e45e6198d
Author: Test User <test.user@example.com>
Date: Sat Feb 3 04:05:11 2001 +0700

b

Change-Id: Id043564ef93650b06a70f92f9d91912b6a6a6964

commit 7d980be7a1d499e4d316ab4c01242885032f7eaf
Author: Test User <test.user@example.com>
Date: Sat Feb 3 04:05:08 2001 +0700

a
[EOF]
"###);
}
3 changes: 3 additions & 0 deletions docs/paid_contributors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,6 @@ See [contribution docs](contributing.md#code-reviews) for details on this policy
* steadmon
* tbodt
* zygoloid

## IMC Trading
* HoelzelJon