-
Notifications
You must be signed in to change notification settings - Fork 698
docs: add working with gerrit section #7396
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.
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
base: aseipp/push-uytvkxyqyspn
Are you sure you want to change the base?
docs: add working with gerrit section #7396
Conversation
This in general looks good to me, but your patch is failing the CI because the commit title not the PR title should be |
Currently there are instructions on how to setup jj to work with GitHub, but not Gerrit, which seems to be very popular with the jj community. Add a gerrit.md file to help users discover the jj gerrit subcommand.
586bc96
to
9342158
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks pretty good, but 3 main focus areas:
- User guides should focus on how a user interacts with something, rather than the technical details of how it works under the hood
- You make a lot of assumptions about the user's workflow, in particular
@-
vs@
. JJ does not distinguish these. You're welcome to use@-
in your examples, but we should avoid making assumptions and saying things like "@-
is the last non-empty commit" - One thing it appears you've missed in how the command works is that when you push to gerrit, you are pushing not just a single commit, but a stack of changes. So your usage of
trunk()..@-
are equivalent to@-
|
||
```shell | ||
# Option 1: Start JJ in an existing Git repo with Gerrit remotes | ||
$ jj git init --colocate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should be an option 3: jj git clone
``` | ||
|
||
You can configure default values in your repository config by appending the | ||
below to `.jj/repo/config.toml`, like so: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a note that this is generally only useful for option 2
## Basic workflow | ||
|
||
`jj gerrit upload` takes one or more revsets, ensures each selected commit has a | ||
Gerrit-compatible `Change-Id:` footer (adding one if missing), and pushes the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is phrased from a technical perspective of how it works, rather than from the perspective of a user. I suggest:
jj gerrit upload
takes one or more revsets, and uploads the stack of commits ending in them to gerrit. Each JJ change will map to a single gerrit change based on the JJ change id. This should be what you want most of the time, but if you want to associate a JJ change with a specific change already uploaded to gerrit, you can copy the Change-Id
footer from gerrit to the bottom of the commit description in JJ.
### upload a single change | ||
|
||
```shell | ||
# upload the last real commit (@-) for review to main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should call this the "last real commit". @
is a real commit too (in the sense that it has a commit object associated with it). Your interpretation of "last real commit" also assumes the squash workflow, rather than the edit workflow, both of which work just fine with jj gerrit upload
.
See the [revsets](./revsets.md) guide for more. | ||
|
||
> Warning | ||
> The working-copy commit `@` is empty and is rejected. Use `@-` or another |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is making another assumption about workflows that isn't necessarily true for everyone. Empty commits are rejected. I usually upload @
, and my @
is non-empty.
|
||
## Updating changes after review | ||
|
||
To address review feedback, amend or rewrite your commits, then run `jj gerrit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/amend or rewrite your commits/update your revisions.
Amend is a bit of an overloaded term, and we work with revisions / changes in jj, not commits
## Updating changes after review | ||
|
||
To address review feedback, amend or rewrite your commits, then run `jj gerrit | ||
upload` again with the same revsets. Because the `Change-Id` footer is preserved, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this is a user guide, can we remove the "Because the change-ID footer is preserved". This is another one of those things that the user doesn't need to know. All they need to know is "I say upload, and it updates an existing gerrit change because I already uploaded it"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change-Id
is a trailer. I believe gerrit used to use the term footer, but it seems now git officially supports them, they're trailers, and JJ also uses the term trailer.
Since we've already mentioned the trailer earlier (well, see my previous comment) we could probably omit repeating it here?
## Target branch and remote selection | ||
|
||
You must specify the target branch for review with `--for <branch>` or by | ||
configuring `[gerrit].default_for`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the technical details. From a user's perspective, they simply need to know:
- Please run
jj config set --user gerrit.default_for <branch name>
to set your default branch across all repos - Please run
jj config set --repo gerrit.default_for <branch name>
to set your default branch for this specific repo. - Use
--for <branch name>
to override this for one specific occasion.
You must specify the target branch for review with `--for <branch>` or by | ||
configuring `[gerrit].default_for`. | ||
|
||
The remote used to push is determined as follows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, more technical details. What the user needs to know
- If your upload isn't going to gerrit, run
jj config set --repo gerrit.default_remote <gerrit remote name>
- To upload to a specific remote as a one-off thing, use
--remote <remote name>
without changing anything or contacting the remote. | ||
|
||
```shell | ||
$ jj gerrit upload -r 'trunk()..@-' --for main --dry-run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned before, trunk()..@-
isn't required
I've updated the docstrings for the commands in the original pr, since the behaviour around stacked commits was very non-obvious. |
resulting heads to `refs/for/<branch>` on your Gerrit remote. | ||
|
||
> Note | ||
> Gerrit identifies and updates changes by `Change-Id`. When you reupload a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth being explicit and mentioning that Change-Id
is a commit trailer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just minor stuff
- `-r 'trunk()..@-'`: everything on top of trunk | ||
- `-r 'A..B'`: commits reachable from `B` but not `A` | ||
|
||
See the [revsets](./revsets.md) guide for more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove ./
as we generally use relative links
$ jj gerrit upload -r 'xcv' | ||
``` | ||
|
||
## Future: native Jujutsu metadata in Gerrit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Matt since we already have a familiar wording in the FAQ. The thing Daniel could do is link out to Gerrits design doc though.
client. In practice, that means small, clean commits that evolve over | ||
time—exactly how Gerrit wants you to work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client. In practice, that means small, clean commits that evolve over | |
time—exactly how Gerrit wants you to work. | |
client. In practice, that means small, clean commits that evolve over time, | |
exactly how Gerrit wants you to work. |
$ jj git init --colocate | ||
|
||
# Option 2: Add a Gerrit remote to a JJ repo | ||
$ jj git remote add gerrit ssh://gerrit.example.com:29418/your/project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ jj git remote add gerrit ssh://gerrit.example.com:29418/your/project | |
$ jj git remote add gerrit https://review.gerrithub.io/yourname/yourproject |
If you use Git/HTTPS is going to be more similar to a real production use; also using GerritHub.io could help people experimenting it without having to setup Gerrit at all.
4. A Git remote named `gerrit`, if present | ||
5. Otherwise, the command errors | ||
|
||
## Updating changes after review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Updating changes after review | |
## Uploading a new patch-set to an existing Change |
|
||
## Updating changes after review | ||
|
||
To address review feedback, amend or rewrite your commits, then run `jj gerrit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To address review feedback, amend or rewrite your commits, then run `jj gerrit | |
To rework an existing Change, amending the content or rewrite the commit comments, then run `jj gerrit |
The Gerrit project is exploring support for native Jujutsu metadata ("jj | ||
headers") so Gerrit could understand Jujutsu change identity without requiring | ||
`Change-Id` footers. Until then, `jj gerrit upload` will add a Gerrit-compatible | ||
`Change-Id` when needed and push to `refs/for/<branch>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this is pretty much up in the air and subject to change; I'd just remove it for now.
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: empty lines
@@ -23,4 +23,5 @@ You may want to jump to: | |||
- [Installation and Setup](install-and-setup.md) | |||
- [Tutorial and Birds-Eye View](tutorial.md) | |||
- [Working with GitHub](github.md) | |||
- [Working with Gerrit](gerrit.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just sort them alphabetically to avoid being biased on the order.
@@ -92,6 +92,7 @@ nav: | |||
- 'Installation and Setup': 'install-and-setup.md' | |||
- 'Tutorial and Birds-Eye View': 'tutorial.md' | |||
- 'Working with GitHub': 'github.md' | |||
- 'Working with Gerrit': 'gerrit.md' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: let's sort them alphabetically.
This PR depends on @matts1 change that addst the Gerrit upload
command.
I should add, I'm very new to JJ and this is my first contribution.
Please let me know if I did anything wrong.
Checklist
If applicable:
CHANGELOG.md
README.md
,docs/
,demos/
)cli/src/config-schema.json
)