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(utils): Use safe relative path when calling git diff #1323

Merged
merged 1 commit into from
Mar 14, 2018

Conversation

Vunovati
Copy link
Contributor

@Vunovati Vunovati commented Mar 14, 2018

When location is the same as the cwd, return '.' instead of empty string
otherwise git diff will fail

refs #1322

Description

Using path.relative is not enough for git params as when comparing two identical paths (path.relative) it will return an empty string

Motivation and Context

Fixes #1322

How Has This Been Tested?

I added a test case and was not able to reproduce it as in #1322 anymore

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.

When location is the same as the cwd, return '.' instead of empty string
otherwise git diff will fail

refs lerna#1322
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.

Thanks for reporting this and identifying the fix!

@@ -134,7 +134,9 @@ function describeTag(ref, opts) {
}

function diffSinceIn(committish, location, opts) {
const formattedLocation = slash(path.relative(opts.cwd, location));
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd rather just leave off the -- <formattedLocation> args if formattedLocation is empty. The git command works the same way in that case.

diff --git a/core/git-utils/index.js b/core/git-utils/index.js
index bb11df0f..720ae11a 100644
--- a/core/git-utils/index.js
+++ b/core/git-utils/index.js
@@ -134,15 +134,17 @@ function describeTag(ref, opts) {
 }
 
 function diffSinceIn(committish, location, opts) {
+  const args = ["diff", "--name-only", committish];
   const formattedLocation = slash(path.relative(opts.cwd, location));
 
+  if (formattedLocation) {
+    // avoid same-directory path.relative() === ""
+    args.push("--", formattedLocation);
+  }
+
   log.silly("diffSinceIn", committish, formattedLocation);
 
-  const diff = ChildProcessUtilities.execSync(
-    "git",
-    ["diff", "--name-only", committish, "--", formattedLocation],
-    opts
-  );
+  const diff = ChildProcessUtilities.execSync("git", args, opts);
   log.silly("diff", diff);
 
   return diff;

@@ -259,6 +259,21 @@ describe("GitUtilities", () => {
opts
);
});

it("returns list of files changed since commit at location when location equals cwd", () => {
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the test!

@evocateur evocateur changed the title fix(utils): use safe relative path on calling git fix(utils): Use safe relative path when calling git diff Mar 14, 2018
@evocateur evocateur merged commit 619c477 into lerna:master Mar 14, 2018
@evocateur
Copy link
Member

@Vunovati Thanks for the fix!

@Vunovati
Copy link
Contributor Author

@evocateur do you know when we can expect a new beta release containing the fix?

I cannot reference it as a git or github dev dependency ("lerna": lerna/lerna#master") in my package.json due to this repo's project structure which prevents it from being installed that way. I can't think of any other options. Also, Thank you for merging this.

@evocateur
Copy link
Member

@Vunovati Sorry, meant to release it yesterday. Just published v3.0.0-beta.3, thanks again for the contribution!

@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.

Lerna updated not working when a package is in the lerna root dir
2 participants