Skip to content
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

Docs: swap order of @ and main in jj new doc comment #5096

Merged

Conversation

david-crespo
Copy link
Contributor

@david-crespo david-crespo commented Dec 13, 2024

I followed the recommendation in the jj new doc to use jj new main @ to make a merge commit and ended up with a merge commit that GitHub did not like. The PR diff included both the relevant changes from that branch plus everything I merged in. @papertigers pointed out that swapping the two args produces a merge commit GitHub understands better. Happy to add a line explaining that the order matters, but it might be too much detail for this spot. The linked doc https://martinvonz.github.io/jj/latest/working-copy/ also does not explain this.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

Copy link

google-cla bot commented Dec 13, 2024

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.

@david-crespo david-crespo changed the title Docs: swap order of @ and main in jj new doc comment Docs: swap order of @ and main in jj new doc comment Dec 13, 2024
@david-crespo david-crespo force-pushed the crespo/docs-swap-merge-args branch from 82fba7d to b9b907f Compare December 13, 2024 19:22
Copy link
Collaborator

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

LG, but can you comply with our commit style here and maybe add the PR text as a motivation too.

cli/src/commands/new.rs Show resolved Hide resolved
cli/src/commands/new.rs Outdated Show resolved Hide resolved
cli/tests/[email protected] Outdated Show resolved Hide resolved
I followed the recommendation in the `jj new` doc to use `jj new main @`
to make a merge commit and ended up with a merge commit that GitHub did
not like. The PR diff included both the relevant changes from that
branch plus everything I merged in. @papertigers pointed out that
swapping the two args produces a merge commit GitHub understands better.
Happy to add a line explaining that the order matters, but it might be
too much detail for this spot. The linked doc
https://martinvonz.github.io/jj/latest/working-copy/ also does not
explain this.
@david-crespo david-crespo force-pushed the crespo/docs-swap-merge-args branch from b9b907f to 720c53a Compare December 13, 2024 22:19
@PhilipMetzger
Copy link
Collaborator

and feel free to resolve all comments that you've adressed

@PhilipMetzger PhilipMetzger merged commit bf24be2 into martinvonz:main Dec 13, 2024
18 checks passed
@david-crespo david-crespo deleted the crespo/docs-swap-merge-args branch December 14, 2024 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants