fix: version does not git-commit nor git-tag when package in subdir

[timothybonci]

Change the git detection to use git.find in order to walk up through directories to find the git repo.

Add use case for detecting if you're in a subdirectory that is a workspace, and make that case the exception for creating the commit and tag.

References

Fixes #2010

[lukekarrys]

Thanks for the new PR for this @timothybonci! It looks good so far, but CI is showing a few more tests in test/lib/commands/version.js that are failing now. Would you be able to look at those too?

[lukekarrys]

libnpmversion tests are passing but cli tests for commands/version are failing.

[timothybonci]

Yep, I missed that test locally. Thanks for kicking off the workflows, and thanks for fixing the git config in the .github ci. I'll fire up the debugger.

[timothybonci]

Aha, there was a bug in the internal caller in lib/commands/version.js where we were passing the flag instead of the option property, so we were never properly overriding gitTagVersion with false. These tests were passing before because we'd short circuit the doGit check on git.is returning false. Now that we use git.find and find the repo, we pass to the next check for doGit, which properly fails. Now that it's fixed, we short circuit on the very first check for gitTagVersion, and correctly don't attempt any git operations.

Long story short, I think I fixed it. I'd appreciate another run of the workflows.

[timothybonci]

@lukekarrys would you please approve the test workflows to test the latest changes?

[timothybonci]

Rebased to latest and fixed the two outstanding lint issues.

[timothybonci]

Rebased to latest again. @lukekarrys could you review?

[lukekarrys]

Sorry for the delay in reviewing this @timothybonci. I am out for a few weeks and will review it when I get back. Thanks for your continued work on this!

[1oglop1]

@lukekarrys are there any updates on this? It has been quite a few weeks. As a newcomer to NPM, I wasted a lot of time trying to figure out why this feature is not working according to docs :(.

[martinmiglio]

bumping because I need this feature.

[markhougaard]

This would be extremely helpful to get merged. I've wasted a few days trying to understand why this behavior is different from the official docs. Workarounds are nice, but it's better to get it fixed properly.

[wraithgar]

Typically we try to avoid unit tests like those written in test/is-workspace-safe.js

Our testing approach is that we need to be able to reach it all from the surface area of the module itself. Unit testing in npm has historically led to a lot of dead code and unneeded complexity handling cases that were impossible in whole.

It's fine to test those things in a separate file for organization but they need to reach those lines of code through libnpmversion itself, not by requiring the module directly.

ETA: it looks like this repo is still all unit tests. We're not asking you to implement this so the unit test will have to do for now

[Stehsegler]

We're also urgently waiting for this bugfix.

@lukekarrys @wraithgar - If I understand the latest comment correctly, the PR is now ready to be merged? If not, what still needs to be done?

Thank you all for your engagement in this product!

[dmcweeney]

@lukekarrys @wraithgar @timothybonci Wondering if there will be an progress made on this PR for this bug fix?

[wraithgar]

I think what's left is to fix the merge conflicts and to make sure we're ok with the tag it will make. Workspaces need to be tagged differently than the root project because without any changes they will collide with each other and the root project (since the tag is only the version, it's not namespaced in any way)

[timothybonci]

After a year of silence, I'm not going to contribute further. I'm relinquishing any possible claims I may have on anything I've written in these commits/MR. I wish you all the best of luck.