Fix gerrit upload with mixed explicit and implicit change-ids #8219
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
HoelzelJon
force-pushed
the
push-zppnpxxynnpz
branch
2 times, most recently
from
e677fe6 to
f085f39
Compare
|
Can you review this one, @thoughtpolice? |
|
And maybe @matts1 also wants to take a look |
cli/tests/test_gerrit_upload.rs
Outdated
| "}, | ||
| ) | ||
| .unwrap(); | ||
| let output = local_dir.run_jj(["describe", "c"]); |
There was a problem hiding this comment.
nit: you can use describe c -m instead of the edit script.
There was a problem hiding this comment.
oh, I can! For some reason, I had thought that using -m wouldn't work for a multiline description string. thanks!
| if trailers.is_empty() { "\n\n" } else { "\n" }, | ||
| gerrit_change_id | ||
| ) | ||
| }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Also, let me know if it's reasonable that I've added myself to the |
HoelzelJon
force-pushed
the
push-zppnpxxynnpz
branch
from
f085f39 to
f688f4b
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The following commits do not follow our format for subject lines:
- e9b5c9c: fix typo in comment
Commits should have a subject line following the format <topic>: <description>. Please review the commit guidelines for more information.
The 'gerrit upload' command has logic to handle commits with the "Change-Id" explicitly set within the commit message, and commits without the Change-Id explicitly set. This commit adds a test for the behavior when all commits have the Change-Id explicitly set. Also fix typo in existing test comment
HoelzelJon
force-pushed
the
push-zppnpxxynnpz
branch
from
f688f4b to
835fa2e
Compare
github-actions
bot
dismissed
their stale review
All commits are now correctly formatted. Thank you for your contribution!
The 'gerrit upload' cli command is meant to support uploading commits that have an explicit "Change-Id: ..." footer, and those that don't. In the latter case, a temporary copy of the commit being uploaded will be created, with the Change-Id set based on the jj change id. Prior to this commit, there was a bug in this implementation when a chain of commits contained some with Change-Id footers and some without. Say we have commits a->b->c, where b has no Change-Id footer but c does (this case is what's now tested in the test_gerrit_upload_local case). The old logic would: - create a new version of b (let's call it b*), with a CommitId footer, and populate the 'old_to_new' map with 'b -> b*' - not create a new version of c, and populate the 'old_to_new' map with 'c -> c' - push the 'new' version of c, which is c itself. However, c has b as a parent, not b*, so this fails to send to gerrit because b has no Commit-Id footer! After this commit, we create a new temporary commit for each commit being uploaded, so that we can properly point each of them to their expected parents. Note that if all commits being pushed already have Commit-Id footers, then it is theoretically not necessary to create any temporary commits. However, this would require additional bespoke logic for this case, and it's not clear that this would lead to any significant benefit (presumably, the gerrit push itself will be the main performance bottleneck when running this command). This is covered by the 'test_gerrit_upload_local_explicit_change_ids' case, where we see the testcase is now uploading different commit hashes to gerrit.
The prior commit in this PR was done of my own accord but during work hours, so adding myself to this list.
HoelzelJon
force-pushed
the
push-zppnpxxynnpz
branch
from
835fa2e to
a02f780
Compare
yuja
left a comment
yuja
left a comment
There was a problem hiding this comment.
Also, let me know if it's reasonable that I've added myself to the
paid_contributors.md.
I think that's good because the purpose of this list is to spot conflicts of interest.
BTW, I didn't notice CLA issue isn't resolved yet. Some of the contributors wouldn't be allowed to review this PR.
| Change-Id: Idfac1e8c149efddf5c7a286f787b43886a6a6964 | ||
| commit 887a7016ec03a904835da1059543d8cc34b6ba76 | ||
| commit 22bbfff17a982c9b8f18cc8cbcd59ec833827c41 |
There was a problem hiding this comment.
Oh, so this caught the bug. Maybe we need to copy the original predecessors to rewrite to identical commit?
3 participants
This should address the issue described in #8006
This is my first contribution; please let me know if there's anything I've overlooked here. Thanks!
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/) -- N/Acli/src/config-schema.json) -- N/A