WIP: gerrit: Use base to upload exact revisions#9384
Draft
freundTech wants to merge 3 commits intojj-vcs:mainfrom
Draft
WIP: gerrit: Use base to upload exact revisions#9384freundTech wants to merge 3 commits intojj-vcs:mainfrom
base to upload exact revisions#9384freundTech wants to merge 3 commits intojj-vcs:mainfrom
Conversation
freundTech
force-pushed
the
push-qxpyuntolnvq
branch
2 times, most recently
from
cbb8246 to
f7430b4
Compare
This gets rid of some code duplication
freundTech
force-pushed
the
push-qxpyuntolnvq
branch
from
f7430b4 to
55cfd13
Compare
Contributor
|
please expand on the commit motivations and move the fixes line into the one which "completes" the work. |
Contributor
There was a problem hiding this comment.
nit: this is missing a config.md entry explaining what it does.
Comment on lines
-472
to
-476
| // If you have the changes main -> A -> B, and then run `jj gerrit upload -r B`, | ||
| // then that uploads both A and B. Thus, we need to ensure that A also | ||
| // has a Change-ID. | ||
| // We make an assumption here that all immutable commits already have a | ||
| // Change-ID. |
Contributor
There was a problem hiding this comment.
nit: its probably useful to preserve that comment
Comment on lines
+423
to
+424
| while let Some(current_commit) = queue.pop_front() { | ||
| let parents = index.parents(¤t_commit)?; |
Contributor
There was a problem hiding this comment.
nit: this results in N index lookups/calls which can be expensive, so offering a stream-based implementation would definitely better for Google and ERSC. cc @martinvonz for an opinion.
| fn heads(&self, candidates: &mut dyn Iterator<Item = &CommitId>) -> IndexResult<Vec<CommitId>>; | ||
|
|
||
| /// Returns the parents of commit ID `commit_id` | ||
| fn parents(&self, commit_id: &CommitId) -> IndexResult<Vec<CommitId>>; |
Contributor
There was a problem hiding this comment.
nit: This is related to my other comment, but maybe a variant which takes an iterator of CommitIds is better here for alternate backends which aren't Git and then also returns a stream.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #9364.
Work in progress.
This adds the changes proposed in #9364. Because they are breaking changes they are hidden behind a config option for now. The default value of this config option could then be flipped after a deprecation period.
This still needs a bit more testing and probably a few edge case fixes. Opening as draft PR to enable design discussions. Not yet for code review (I know there's a bunch of stuff to fix).
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)how it works, how it's organized), including any code drafted by an LLM.
an eye towards deleting anything that is irrelevant, clarifying anything
that is confusing, and adding details that are relevant. This includes,
for example, commit descriptions, PR descriptions, and code comments.