Allow passing --remove to cargo dev setup <SUBCOMMAND>

[yoav-lavi]

changelog: none

Allows passing --remove to cargo dev setup <SUBCOMMAND> as an alternative to cargo dev remove ...

Fixes #8663

[rust-highfive]

r? @xFrednet

(rust-highfive has picked a reviewer for you, use r? to override)

[xFrednet]

The changes look good to me. I think that setup XYZ --remove reads a bit weird, but I'm no UI designer, and it's worth the addition, if it helps at least one contributor.

Could you squash your commits? Afterwards, this can be merged 🙃

[yoav-lavi]

@xFrednet Fair enough, it might be a bit weird.

I think what's throwing me off here is that it's verb noun rather than noun verb (due to the options being subcommands rather than arbitrary values) so I would possibly expect it to be cargo dev git-hooks install / remove (as in git commit --amend, gh repo clone, vercel secrets add etc.).

You see verb noun more in cases with arbitrary values I think, e.g. cargo install <CRATE>, vercel login <EMAIL>, git branch <NAME>.

That being said, it's a matter of personal preference and it may need some more thought / input from others

[xFrednet]

I chose this order for two reasons:

1. rustc's x.py file also has a setup command, which opens an interactive TUI to set up the project.
2. I wanted all setup and remove commands to be grouped together. git commit --amend is a good ui. Git also only has a single job. clippy_dev is a weird mixture of several tools, which can make it a bit messy IMO.

I don't think that there is a right or wrong here. Alternatively, we could add a command to the setup and setup <object> that mentions the remove command.

Since this is an internal Clippy-specific tool, I think just adding --remove as a kind of alias is alright. If you say that this is more intuitive for you, then there might be more users thinking the same 🙃

[yoav-lavi]

@xFrednet I understand, just to clarify I didn't mean to say that the current method is wrong, just to explain why I personally saw the need to open the PR.

This is possibly a silly question but since I've had some issues with this:

I'm used to working with "squash and merge" or just normal commits and haven't needed to squash on my end up until now (and the previous PRs to clippy), what specifically am I expected to do when asked to squash my commits? The guides I've followed didn't really work as expected.

Edit: also, it seems like this (asking to squash) is said on every PR, perhaps a bot could just post that when approving? To make reviewers' lives easier

[xFrednet]

Hey @yoav-lavi, it's not a stupid question. I only learned about squashing from this project. 🙃

Squashing can basically combine multiple commits into one. There are a few ways to do it, and this is my way:

# On your branch
git rebase -i HEAD~6

The 6 is the number of commits you want to effect. This will open your editor with something like this:

pick 6c1216f1f Allow passing --remove to `cargo dev setup <SUBCOMMAND>`
pick 62d94c754 add missing args
pick 7d42ae967 unque name not needed
pick c2c3a3e63 more descriptive help
pick bf0212b28 formatting fixes
pick 3260bbf9a missing quote

Now you can indicate if and how you would like to change commits. In this case, you can use s to keep the other commit messages in the description in the base commit or f to just add the changes to the previous commit. I would do the following:

pick 6c1216f1f Allow passing --remove to `cargo dev setup <SUBCOMMAND>`
f 62d94c754 add missing args
f 7d42ae967 unque name not needed
f c2c3a3e63 more descriptive help
f bf0212b28 formatting fixes
f 3260bbf9a missing quote

and then save and close the file. If you pick s for squashing, you'll also get the option to change the commit message from the base commit. Afterwards, you just need to force push your changes:

git push --force-with-lease
# I have an alias for this and other git utilities. Here is my alias, if you want it
# alias git-push='git push --force-with-lease'

For a deeper understanding, I would recommend reading the documentation, but this should you though almost everything. Also note, that force pushing overwrites the remote branch. In this case, we want this, but I recommend being careful with force pushes 🙃

also, it seems like this is said on every PR, perhaps a bot could just post that when approving? To make reviewers' lives easier

The goal of squashing is to create a good history. With larger PRs it makes sense to split the implementation into several well named commits. They can also help with the review, to separate old changed from new ones. It's possible to automatically squash with @bors but that sadly looses the commit author. There is an open issue to fix this :)

For me, it depends on the type of commits that are present. Here we have one well named one and several commits that just fix something. That's why I asked you to squash. But I appreciate you watching out for us 🙃

[yoav-lavi]

@xFrednet thanks! I appreciate the detailed explanation. That's more or less what I've been doing using the guide I found, the problem mainly arises when pulling from master at some point, but it worked correctly on this attempt (although I tried again before seeing your message so the commit message might be a combined one rather than the first, wasn't aware of f).

[xFrednet]

You're welcome! Are you referring to conflicts? Those are sadly more difficult to fix with rebasing in comparison to merging :/ .

I've heard that you can just merge and then squash the merge commit. But don't quote me on that ^^

[xFrednet]

The changes and commit look good to me, thank you for the update and squashing the commits 🙃

@bors r+

[bors]

📌 Commit ffde78b has been approved by xFrednet

[bors]

⌛ Testing commit ffde78b with merge c7e6863...

[yoav-lavi]

I think the issue was mainly having X new commits that also need to be squashed, but I don't remember exactly what happened last time around. Will let you know if I notice it again. I wonder if bors could use commit impersonation to fix the issue, but the commits would not be verified (not sure if that's an issue)

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing c7e6863 to master...

[xFrednet]

Sure, you can ping me the next time when you have a problem 🙃

I wonder if bors could use commit impersonation to fix the issue, but the commits would not be verified (not sure if that's an issue)

That's the main thing holding this feature back. They've discussed impersonating the user, but that would mark them as unverified. I'm sadly not sure what the best solution would be. This works well enough for now 🙃

[flip1995]

We should add "how to squash / rebase" instructions to our documentation. I always recommend to check the git diff with the remote branch and make sure it is empty before force pushing to the remote branch.

Having an alias for force pushing is really bad style in my opinion, because it makes it easier to do by accident. My alias is git push --fwl 😄 😝

[xFrednet]

It's definitely something that should be handled carefully. How can you add aliases for flags like that? 🙃