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

Autosquash Improvements #394

Closed
wants to merge 2 commits into from
Closed

Conversation

JP-Ellis
Copy link

There are three main changes here:

  1. Expanded the autosquash prefixes from just fixup! to now include squash! and amend! . This ensures better compliance with the Git docs in git-rebase.
  2. To go with (1), aliased fixup with autosquash. This was done both in the CLI and in the config. Eventually, it may be worth deprecating fixup in favour of autosquash; however, I am leaving that as a "future change".
  3. When autosquash commits are allowed and an autosquash commit is detected, the remaining checks at done on the remainder of the commit message.

I understand that the change from "fixup" to "autosquash" might be a bit of a big change; however, this should be all backwards compatible. Happy to revise the PR if this is a step too far.

Resolves #392

When the `--autosquash` command is used during a rebase, three prefixes can be
used:

- `fixup! `
- `squash! `
- `amend! `

See [the docs](https://git-scm.com/docs/git-rebase) here.

The `no_fixup` config has been expanded to include all three options (this will
need some comms to ensure no unexpended behaviour).

Furthermore, an alias has been added under `no_autosquash`.

Longer term, it may be worth considering replacing `no_fixup` with
`no_autosquash`.

Signed-off-by: JP-Ellis <[email protected]>
When autosquash commits are allowed, the prefix is removed before
validating the rest of the commit (such as line length, conventional,
etc.).

Signed-off-by: JP-Ellis <[email protected]>
Comment on lines +66 to +67
#[serde(alias = "fixup")]
Autosquash(Autosquash),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a breaking change. Our json output would be switching from emitting type = "fixup" messages to type = "autosquash".

@epage
Copy link
Collaborator

epage commented Sep 16, 2024

This PR is listed as making 3 changes but it does so in 2 commits. Please be sure to make your commits atomic. The first two changes are different enough from the third, that really, this should be at least 2 PRs.

Comment on lines 26 to 29
if config.no_autosquash() {
failed |= check_autosquash(source, message, report)?;
failed |= autosquash_result;
} else if let Some(idx) = idx {
message = &message[idx..];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we always strip?

Ok(true)
} else {
Ok(false)
) -> Result<(bool, Option<usize>), anyhow::Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we instead return the new message? We could use strip_prefix rather than starts_with.

@JP-Ellis
Copy link
Author

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.

Allow for fixup! {conventional}
2 participants