fix: git diff no-relative - #1620

🦋 Changeset detected

Latest commit: fd740d1

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.66%. Comparing base (131d445) to head (61acf4f).

@@           Coverage Diff           @@
##             main    #1620   +/-   ##
=======================================
  Coverage   80.66%   80.66%           
=======================================
  Files          54       54           
  Lines        2255     2255           
  Branches      681      680    -1     
=======================================
  Hits         1819     1819           
  Misses        431      431           
  Partials        5        5           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Seems fine to me. Should we also add --no-relative here too?

changesets/packages/git/src/index.ts

Line 198 in 5c2bac5

const cmd = await spawn("git", ["diff", "--name-only", divergedAt], { cwd });

As long as the cwd is always in the root, it should be fine. (I can add it just in case) But with getChangedChangesetFilesSinceRef it first CDs into the .changesets directory, meaning the cwd is not in the root

As long as the cwd is always in the root, it should be fine. (I can add it just in case) But with getChangedChangesetFilesSinceRef it first CDs into the .changesets directory, meaning the cwd is not in the root

Ah I didn't realize that the cwd passed was the .changeset directory directly. I thought it didn't need to since there's already the .changeset regex next.

I wonder if we should not do that and let cwd be the actual current working directory, but I don't think we need to figure that now and I'm happy with the PR's fix for now.

As long as the cwd is always in the root, it should be fine. (I can add it just in case) But with getChangedChangesetFilesSinceRef it first CDs into the .changesets directory, meaning the cwd is not in the root

Ah I didn't realize that the cwd passed was the .changeset directory directly. I thought it didn't need to since there's already the .changeset regex next.

I wonder if we should not do that and let cwd be the actual current working directory, but I don't think we need to figure that now and I'm happy with the PR's fix for now.

Oh that's true, as the cwd is in the .changeset directory already, we could technically get rid of the filter for the files in the directory. But the cwd is already set outside of the function, so in case the function gets called without the cwd, it might still be nice to have in place

Shouldn't this be --relative={relative root path from repo root}, rather than --no-relative?

With --no-relative, I still get repo relative roots:

aubron@127 % git diff --name-only --no-relative
libraries/javascript/{path}/.changeset/config.json
libraries/javascript/{path}/package.json
libraries/javascript/{path}/pnpm-lock.yaml

This still kills status in CI:

DEBUG: New changeset hashes: [
  "libraries/javascript/{path}/spotty-pumas-behave.md"
]
DEBUG: New hashes: [
  "javascript"
]
DEBUG: Filtered changeset files: []
DEBUG: Changesets: []

Hi @Aubron

Let's continue the conversation on here, as it allows us more characters & code blocks :)

First of all, is this an issue you were experiencing before as well or only since 2.29.1?
Second, what's the full command you run?

This is something I've seen in various old versions of the status cli, today was my opportunity to try and understand why it happens:

This happens when your repo includes your .changesets directory not in the root of the repo:

- libraries
  - my cool library
    - .changesets

• the status command gets your list of changed packages, and then reconciles that with your release plan
• This calls 'getChangesets', which (for me in this configuration) returns filename only paths:

export default async function getChangesets(
  cwd: string,
  sinceRef?: string
): Promise<Array<NewChangeset>> {
  console.log("DEBUG: Getting changesets for directory: " + cwd);
  let changesetBase = path.join(cwd, ".changeset");
  let contents: string[];
  try {
    contents = await fs.readdir(changesetBase);
  } catch (err) {
    if ((err as any).code === "ENOENT") {
      throw new Error("There is no .changeset directory in this project");
    }
    throw err;
  }

  console.log("DEBUG: Found changeset files: " + JSON.stringify(contents, null, 2));

DEBUG: Found changeset files: [
  "README.md",
  "config.json",
  "funny-seas-cross.md",
  "spotty-pumas-behave.md"
]

• This calls filterChangesetsSinceRef, which returns relative paths to the repo root:

const newChangesets = await git.getChangedChangesetFilesSinceRef({
    cwd: changesetBase,
    ref: sinceRef,
  });
  console.log("DEBUG: New changeset hashes: " + JSON.stringify(newChangesets, null, 2));

DEBUG: New changeset hashes: [
  "libraries/javascript/{path}/.changeset/spotty-pumas-behave.md"
]

this is then reduced to its first slice via .split, which for me, returns a partial path:

  const newHashes = newChangesets.map((c) => c.split("/")[1]);
  console.log("DEBUG: New hashes: " + JSON.stringify(newHashes, null, 2));

DEBUG: New hashes: [
  "javascript"
]

This causes the changeset resolution to fail, since it can't find the changesets in the set of hashes

And to confirm, I think our circumstances must be similar-but-not-the-same, and touching the same files.

Ah yes, I might see what's wrong. The filter basically splits on "/" and takes item at position 1 (in your case "javascript"). Let me create a fix for you :)

For example, I have fixed this locally to the project by patching that c.split("/")[1] to instead be the last element in the split, but I assume that solution will break in some other configuration

EDIT: Thanks! Would be nice for someone who has just been touching this code to fix, I'm very worried about breaking others' configs

#1636 Should do it, I think :)

Note that --no-relative was introduced in git 2.28.0 so this fails in earlier versions of git such as 2.25.1 with the slightly misleading error 'Error: Failed to diff against {divergedAt}. Is {divergedAt} a valid ref?'