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

Add --flatten option to use when merge conflicts cannot be imported #1019

Merged
merged 3 commits into from Sep 26, 2017
Merged

Add --flatten option to use when merge conflicts cannot be imported #1019

merged 3 commits into from Sep 26, 2017

Conversation

arthakdoo
Copy link
Contributor

@arthakdoo arthakdoo commented Sep 19, 2017

Add option "--flatten" to import each merge as a single change the merge introduced

Description

Instead of doing "git format-patch", patches can be created by "git log" with a certain set of options that mimic format-patch, but with git log some additional options are available, like "--first-parent", which only follows main branch, so that side branch commits are not taken in separately, but instead only the merge commit is imported containing all changes from all branches commits that were merged there.

Motivation and Context

This is required for situations when there were merge commits containing merge conflict resolutions in imported project. In the current implementation, "lerna import" fails because "git format-patch" does not include merge commits and its resolution, so such projects can't be imported. If "--flatten" is provided with the proposed solution, these problematic projects are successfully imported.

The issue was first reported in #730, and I personally had this issue so I had to fix it in order to use Lerna. Now I'm sharing the solution.

How Has This Been Tested?

Tested with all sorts of importable repos, with several branches merged, with binary content, with conflicts and without conflicts, with my option and with the original option. Also all auto tests pass and one automatic test added.

Types of changes

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

Checklist:

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

@arthakdoo
Copy link
Contributor Author

I think the AppVeyor build is failing for its own reasons. I don't see any relationship of its output to my changes. Can somebody have a look at this change please? It's been 7 days now, I'd appreciate some feedback

@evocateur
Copy link
Member

Sorry, yeah, the appveyor build is flapping a lot. I've been extremely busy, I'll take a look when I can.

Copy link
Member

@evocateur evocateur left a comment

Choose a reason for hiding this comment

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

Very minor tweaks and we're good to merge. Thanks again for your patience, @dmaksimovic.

createPatchForCommit(sha) {
let patch = null;
if (this.options.flatten) {
const diff = this.externalExecSync("git", ["log", "--reverse", "--first-parent", "-p",
Copy link
Member

Choose a reason for hiding this comment

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

I'd appreciate if the array of args was spread one-per-line for readability, thanks.

@@ -105,24 +117,39 @@ export default class ImportCommand extends Command {
return ChildProcessUtilities.execSync(cmd, args, this.externalExecOpts);
}

createPatchForCommit(sha) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for extracting this method :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, it was my pleasure. Changes applied as requested

const branchName = "conflict_branch";
const conflictedFileName = "conflicted-file.txt";
const conflictedFile = path.join(externalDir, conflictedFileName);
await fs.writeFile(conflictedFile, "initial content");
Copy link
Member

Choose a reason for hiding this comment

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

Please put newlines before the fs.writeFile() bits so the logical steps are a bit easier to follow, thanks.

@evocateur evocateur changed the title Introduce flatten import to use when merge conflicts cannot be imported Add --flatten option to use when merge conflicts cannot be imported Sep 26, 2017
@evocateur evocateur merged commit 6af47ba into lerna:master Sep 26, 2017
@evocateur
Copy link
Member

Thanks!

@arthakdoo
Copy link
Contributor Author

Cheers!

@lock
Copy link

lock bot commented Dec 27, 2018

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants