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 git repository discovery in npm version #221

Closed
wants to merge 1 commit into from
Closed

Fix git repository discovery in npm version #221

wants to merge 1 commit into from

Conversation

reegnz
Copy link

@reegnz reegnz commented Jul 25, 2019

When package.json is not in the git root, then npm version bumps didn't
make a commit.

Issue was with how npm version discovered if it's in a git repository: it
assumed that it's a git repository only if .git folder is at the same
level as the package.json.

In reality you cannot make such assumptions, as the .git directory is
implementation detail.

Proper way of discovering if you're in a git directory is invoking
git rev-parse --git-dir.

@reegnz reegnz requested a review from a team as a code owner July 25, 2019 09:37
@reegnz
Copy link
Author

reegnz commented Jul 25, 2019

I also opened a thread about it here , but found out that it's an easy fix, so I did the change instead. :)

When package.json is not in the git root, then npm version bumps didn't
make a commit.

Issue was with how npm version discovered if it's in a git repository: it
assumed that it's a git repository only if .git folder is at the same
level as the package.json.

In reality you cannot make such assumptions, as the .git directory is
implementation detail.

Proper way of discovering if you're in a git directory is invoking
`git rev-parse --git-dir`.
@isaacs
Copy link
Contributor

isaacs commented Aug 2, 2019

Can you update the test as well? They're failing: https://travis-ci.com/npm/cli/jobs/219563951#L1661

These look directly related to changes to lib/version.js. Is the behavior change intentional?

@reegnz
Copy link
Author

reegnz commented Aug 6, 2019

@isaacs it wasn't intentional, will try to figure out why those tests fail.

@darcyclarke darcyclarke added pr: needs tests requires tests before merging Priority Backlog a "backlogged" item that will be tracked in a Project Board labels Nov 19, 2019
@ruyadorno ruyadorno added the semver:patch semver patch level for changes label Aug 14, 2020
@darcyclarke darcyclarke added the Release 6.x work is associated with a specific npm 6 release label Sep 1, 2020
@zerog2k
Copy link

zerog2k commented Sep 24, 2020

I understand there is an argument against doing git tag if this is a package in a subdir of a repo with other packages (with potentially separate versioning -- although here, I would expect users to explicitly disable git tagging?). So if this doesn't get merged, I +1 @reegnz idea to emit warning and/or update npm help version with note about not git tagging in subdirs.

@ljharb
Copy link
Contributor

ljharb commented Sep 24, 2020

@zerog2k you can customize the tag prefix in each subdir with .npmrc, and then npm version in the subdir will produce the right tag.

@darcyclarke darcyclarke removed Priority Backlog a "backlogged" item that will be tracked in a Project Board pr: needs tests requires tests before merging labels Oct 1, 2020
@darcyclarke darcyclarke modified the milestone: OSS - Sprint 17 Oct 5, 2020
@ivos
Copy link

ivos commented May 10, 2021

Pardon my ignorance, but I don't understand what is the resolution of this issue. Is is (going to be) merged in? In what version of npm should I expect it to land?

There's a label Release 6.x assigned, but although my npm --version says 7.6.3, it still does not seem to be fixed.

@4-life
Copy link

4-life commented Sep 10, 2021

@ivos doesn't merged. Original issue is still not resolved npm/npm#9111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants