search

danger / danger-js

⚠️ Stop saying "you forgot to …" in code review
http://danger.systems/js/
MIT License
5.26k stars 368 forks source link

[BUG] Incompatible with git config diff.noprefix #1302

Closed lode closed 2 years ago

lode commented 2 years ago

Describe the bug When having git configured to hide prefixes on file names in the diff output (https://git-scm.com/docs/git-config#Documentation/git-config.txt-diffnoprefix) fetching a list of changed files fails.

To Reproduce Steps to reproduce the behavior:

  1. Run danger local (maybe also works with ci)
  2. See nice output
  3. Run git config --global diff.noprefix "true"
  4. Run danger local (maybe also works with ci)
  5. See an error
Error:  TypeError: Cannot read properties of null (reading 'map')
    at parseFile (<...>/node_modules/parse-diff/index.js:159:13)
    ...

Expected behavior It should work also when people configure git in a different way. I think it is also possible, depending on how we get the diff output. At least running git diff --src-prefix="a/" --dst-prefix="b/" forces needed prefixes even though the global config is set to remove them.

Your Environment software version
danger.js 10.9.0
node 16.17.0
npm 8.15.0
Operating System Ubuntu 22.04

Additional context If of any interest, I'm using danger.git.fileMatch, danger.git.created_files, danger.git.modified_files, danger.git.structuredDiffForFile in my dangerfile. However, it seems to trigger regardless, even on an empty dangerfile.

orta commented 2 years ago

Yeah, perhaps the command we run under the hood can be explicit in requiring the prefixes from git, I'm open to PRs there

lode commented 2 years ago

:+1: I'm willing to try a PR, ~though I couldn't find where this was ran. Can you give me a pointer?~

lode commented 2 years ago

~Diving a little deeper.~

~If I understand correctly this is called in https://github.com/danger/danger-js/blob/main/source/platforms/git/diffToGitJSONDSL.ts. It relies on parseDiff, and also does an assumption itself on the prefixes: https://github.com/danger/danger-js/blob/main/source/platforms/git/diffToGitJSONDSL.ts#L20.~

~I get the feeling this should first be fixed in https://github.com/sergeyt/parse-diff. Is that correct?~

lode commented 2 years ago

Sorry for the monologue, but I think I found a place in this repo. I'll try a little and will stop spamming here for a while :)