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

Keep non-patch subject parts when importing repository #1115

Merged
merged 2 commits into from Nov 15, 2017

Conversation

Projects
None yet
2 participants
@koenpunt
Contributor

koenpunt commented Nov 14, 2017

Description

Currently when you import a repo that follows a commit message convention that prefixes commits with something between brackets; e.g. [TICKET-123] something changed, the prefix is stripped due to how git am (or actually git mailinfo) works.

Motivation and Context

Since these prefixes are relevant, since they often refer to a project management system, or define the category of the commit, they should not be stripped.

Especially since the README states the following:

Original commit authors, dates and messages are preserved.

How Has This Been Tested?

I ran the git am command in the ImportCommand.js, and tested with the command arguments, where the --keep-non-patch argument (forwarded as git mailinfo -b ...) resulted in the desired effect of only the [PATCH] prefix being stripped, and other prefixes remained.

I still have to try and see if I can add an assertion to the tests for this behavior.
Added a test.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

koenpunt added some commits Nov 14, 2017

@koenpunt

This comment has been minimized.

Show comment
Hide comment
@koenpunt

koenpunt Nov 14, 2017

Contributor

There's 1 test failing but I don't believe that one's related to my changes..

Contributor

koenpunt commented Nov 14, 2017

There's 1 test failing but I don't believe that one's related to my changes..

@evocateur

This comment has been minimized.

Show comment
Hide comment
@evocateur

evocateur Nov 15, 2017

Member

Yeah, that's an environmental problem with npm on Windows, as far as I can tell. I kicked it again to see if we win the dice roll.

Member

evocateur commented Nov 15, 2017

Yeah, that's an environmental problem with npm on Windows, as far as I can tell. I kicked it again to see if we win the dice roll.

@evocateur

Looks great, thanks!

@@ -171,7 +171,7 @@ export default class ImportCommand extends Command {
//
// Fall back to three-way merge, which can help with duplicate commits
// due to merge history.
ChildProcessUtilities.exec("git", ["am", "-3"], this.execOpts, (err) => {
ChildProcessUtilities.exec("git", ["am", "-3", "--keep-non-patch"], this.execOpts, (err) => {

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment