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.

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

Jump to bottom

Implement a rename subcommand for the branch command. #2699

Merged
merged 1 commit into from
Dec 16, 2023
Merged

Implement a rename subcommand for the branch command. #2699

merged 1 commit into from
Dec 16, 2023

Conversation

essiene
Copy link
Contributor

@essiene essiene commented Dec 13, 2023 •
edited
Loading

Summary

This is really a simple change that does the following in a transaction:

  • Set the new branch name to point to the same commit as the old branch name.
  • Set the old branch name to point to no commit (hence deleting the old name).

Before it starts, it confirms that the new branch name is not already in use.

Changes

  1. Add Rename variant to BranchSubcommands enum.
  2. Create BranchRenameArgs struct needed by BranchSubcommands::Rename
  3. Implement cmd_branch_rename

Fixed: #1457

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)

    N/A

  • I have updated the config schema (cli/src/config-schema.json)

    N/A

  • I have added tests to cover my changes

@essiene essiene mentioned this pull request Dec 13, 2023
Closed
martinvonz
martinvonz reviewed Dec 13, 2023
View reviewed changes
Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

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

Thanks!

cli/src/commands/branch.rs Outdated Show resolved Hide resolved
cli/src/commands/branch.rs Outdated Show resolved Hide resolved
cli/src/commands/branch.rs Outdated Show resolved Hide resolved
yuja
yuja reviewed Dec 16, 2023
View reviewed changes
cli/src/commands/branch.rs Outdated Show resolved Hide resolved
cli/src/commands/branch.rs Outdated Show resolved Hide resolved
cli/src/commands/branch.rs Show resolved Hide resolved
cli/src/commands/branch.rs Outdated Show resolved Hide resolved
@essiene essiene requested review from yuja and martinvonz December 16, 2023 12:33
@essiene
Copy link
Contributor Author

essiene commented Dec 16, 2023

Should I bother with snapshot tests for this? $ cargo insta test doesn't fail, and I expect it shouldn't, since I'm adding a new subcommands rather than changing an existing one.

Thoughts?

@yuja
Copy link
Contributor

yuja commented Dec 16, 2023

Should I bother with snapshot tests for this?

Yes, please. You can add some to test_branch_command.rs.

@essiene
Copy link
Contributor Author

essiene commented Dec 16, 2023

Should I bother with snapshot tests for this?

Yes, please. You can add some to test_branch_command.rs.

Ok. I finally figured out cargo insta and I added tests.

PTAL.

@essiene essiene marked this pull request as ready for review December 16, 2023 16:17
martinvonz
martinvonz reviewed Dec 16, 2023
View reviewed changes
cli/src/commands/branch.rs Outdated Show resolved Hide resolved
cli/src/commands/branch.rs Outdated Show resolved Hide resolved
cli/src/commands/branch.rs Outdated Show resolved Hide resolved
cli/tests/test_branch_command.rs Outdated Show resolved Hide resolved
@essiene
Implement a rename subcommand for the branch command.
3b30a86 
This is really a simple change that does the following in a transaction:
* Set the new branch name to point to the same commit as the old branch name.
* Set the old branch name to point to no commit (hence deleting the old name).

Before it starts, it confirms that the new branch name is not already in use.
martinvonz
martinvonz approved these changes Dec 16, 2023
View reviewed changes
Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Feel free to merge.

@essiene essiene requested a review from martinvonz December 16, 2023 18:30
@essiene
Copy link
Contributor Author

essiene commented Dec 16, 2023

Looks good, thanks! Feel free to merge.

Yay! \o/

Thanks for the detailed and patient explanations. On to the next ;-)

@essiene essiene enabled auto-merge (rebase) December 16, 2023 18:32
@essiene essiene merged commit 35b8dad into jj-vcs:main Dec 16, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuja yuja Awaiting requested review from yuja

@martinvonz martinvonz Awaiting requested review from martinvonz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FR: jj branch rename
3 participants
@essiene @yuja @martinvonz