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

fix: preserve merge states in submodules #769

Merged
merged 4 commits into from
Jan 20, 2020
Merged

Conversation

iiroj
Copy link
Member

@iiroj iiroj commented Jan 20, 2020

This PR fixes the behaviour of lint-staged during merge conflicts in git submodules. The issue was that in a git submodule, the .git directory is actually a file containing a reference to the actual directory (that is located inside the parent repo's .git directory).

The fix includes detecting whether the current repo is a submodule by running git rev-parse --show-superproject-working-tree, which will only output a path when run inside a submodule. When this is true, the true .git location is read from the submodule's file. This resolved path is then passed to the main gitWorkflow instead of it assuming that the .git directory is always at the root of the repo/submodule.

Fixes #768

Thanks to @armata007 for the bug report.

@codecov
Copy link

codecov bot commented Jan 20, 2020

Codecov Report

Merging #769 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #769   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     13           
  Lines         429    439   +10     
  Branches       97    100    +3     
=====================================
+ Hits          429    439   +10
Impacted Files Coverage Δ
lib/runAll.js 100% <100%> (ø) ⬆️
lib/resolveGitRepo.js 100% <100%> (ø)
lib/gitWorkflow.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 072924f...10d6a11. Read the comment docs.


const buffer = await readBufferFromFile(defaultDir)
const dotGit = buffer.toString()
const gitConfigDir = path.resolve(dotGit.replace(/^gitdir: /, ''))

Choose a reason for hiding this comment

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

I believe it does not work properly because of this line. It takes path relative to node_modules or package.json (not sure), and from this going up a directory. I think it should be something like
const gitConfigDir = path.resolve(gitDir + '/' + dotGit.replace(/^gitdir: /, ''))
but probably better written

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this. I tested it myself locally and the path in .git seems to be relative to the gitDir, so that would be better indeed.

Choose a reason for hiding this comment

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

one more thing, in my case
dotGit.replace(/^gitdir: /, '') contains new line at the end, so it tries to read from
path/path/path
MERGE_MSG
when i add .trim() after replace it reads from proper path.

With this change and above change to path it works great in my case 🥇

Copy link
Member Author

Choose a reason for hiding this comment

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

Those following newlines could use some trimming in other places as well, let me fix... thanks for testing!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! Maybe a re-test just in case?

Choose a reason for hiding this comment

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

Works great now

@iiroj iiroj merged commit e646b2c into master Jan 20, 2020
@iiroj iiroj deleted the submodule-merge-conflict branch January 20, 2020 20:06
@okonet
Copy link
Collaborator

okonet commented Jan 20, 2020

🎉 This PR is included in version 10.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

MERGE_HEAD is lost when repository is using submodules feature
3 participants