[Feature request] Use "--color-moved" diffing algorithm in PRs

[MartinSandeCosta]

PRs that move code around, even within the same function, show n lines removed and n added. One would assume this diff could be shown as a trivial "code move". This scales poorly when dealing with multiple function trivial code-moves.

This also appears to make contributions on Github a bit misleading: moving code around may give the impression that a certain contributor makes huge number of additions and deletions to a repository, when in fact they didn't contribute to any functionality whatsoever.

As of now, git itself IS able to detect such changes: https://git-scm.com/docs/git-diff

--color-moved[=<mode>]
Moved lines of code are colored differently. It can be changed by the diff.colorMoved configuration setting.

[wedamija]

Please do this. The one thing I miss most about phabricator is that it can differentiate between moved vs actually changed code.

[brendanwalters]

I've seen this feature request in a variety of places that aren't github's official feedback (e.g. Twitter) - it's quite popular among those who know about this git feature, which has been around a while and even appeared in the GitHub blog in 2018.

[vinkay215]

I mean the same as you

[bramjoosten]

Yup, now I'm discouraged to move code blocks into separate files, because the actual change info is lost with the moving of a code block. even though it's nicer in the long run. No one in my team does this in favor of clear PR's, but we do end up with long files for this reason.

[vinkay215]

I mean the same as you

[0xdevalias]

(Crossposted to https://gist.github.com/0xdevalias/1245fe66ac6953d490114b417a0075e3 for posterity)

Omg yes, please! This is one of my biggest pet peeves/code review slowdowns when performing large refactors to cleanup a legacy codebase.

The following were some manual review notes/commands I added to a recent PR to assist others who were reviewing:

The following git diff command will make it easier to see what actually changed in the code here, as opposed to what just moved around without being changed; to make review/etc easier. Dimmed code in the diff has been moved, but not changed.

This will get all the changes made in this branch, using devalias/improve-first-use as the base:

git \
  -c color.diff.oldMoved="white dim" \
  -c color.diff.oldMovedAlternative="white dim" \
  -c color.diff.newMoved="white dim" \
  -c color.diff.newMovedAlternative="white dim" \
  -c color.diff.newMovedDimmed="white dim" \
  -c color.diff.newMovedAlternativeDimmed="white dim" \
  diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --minimal \
  devalias/improve-first-use HEAD

This will get all changes made for a specific commit hash (55a259f), as compared to the commit hash just before it (55a259f~):

git \
  -c color.diff.oldMoved="white dim" \
  -c color.diff.oldMovedAlternative="white dim" \
  -c color.diff.newMoved="white dim" \
  -c color.diff.newMovedAlternative="white dim" \
  -c color.diff.newMovedDimmed="white dim" \
  -c color.diff.newMovedAlternativeDimmed="white dim" \
  diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --minimal \
  55a259f~ 55a259f

This will get all changes made for a specific commit hash (55a259f), as compared to the commit hash just before it (55a259f~), only for the specific file/path specified:

git \
  -c color.diff.oldMoved="white dim" \
  -c color.diff.oldMovedAlternative="white dim" \
  -c color.diff.newMoved="white dim" \
  -c color.diff.newMovedAlternative="white dim" \
  -c color.diff.newMovedDimmed="white dim" \
  -c color.diff.newMovedAlternativeDimmed="white dim" \
  diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --minimal \
  55a259f~ 55a259f -- oauth/vendor/slack/index.js

You can simplify this for future use by adding it to your git aliases in your ~/.gitconfig, something like the following:

[alias]
  diff-refactor = \
    -c color.diff.oldMoved='white dim' \
    -c color.diff.oldMovedAlternative='white dim' \
    -c color.diff.newMoved='white dim' \
    -c color.diff.newMovedAlternative='white dim' \
    -c color.diff.newMovedDimmed='white dim' \
    -c color.diff.newMovedAlternativeDimmed='white dim' \
    diff --ignore-blank-lines --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --minimal

Which would then allow you to use it as just:

⇒ git diff-refactor

⇒ git diff-refactor devalias/improve-first-use HEAD

⇒ git diff-refactor 55a259f~ 55a259f

⇒ git diff-refactor 55a259f~ 55a259f -- oauth/vendor/slack/index.js

You could potentially add the following as well, though i'm not sure if they will do anything new/useful when combined with --color-moved:

--ignore-all-space --ignore-space-change --ignore-space-at-eol --ignore-cr-at-eol --ignore-blank-lines

[0xdevalias]

I wonder how hard it would be to take the output from git diff --color-moved, parse it, and then render it in a GitHub comment/similar..?

Some potentially useful references:

• https://stackoverflow.com/questions/11509830/how-to-add-color-to-githubs-readme-md-file/73613687#73613687

  • Now since May 2022, Github can accept LATEX code on Markdown, so you can use the \color{namecolor} inside the $$$$

  • https://github.blog/2022-05-19-math-support-in-markdown/
  • https://en.wikibooks.org/wiki/LaTeX/Colors
• Bash color codes: https://gist.github.com/vratiu/9780109
• https://stackoverflow.com/questions/1380333/highlighting-added-deleted-lines-ignoring-moves-in-a-patch-file
  • https://github.com/l0b0/diff-ignore-moved-lines/blob/master/diff-ignore-moved-lines.sh

---

Super hacky, but this uses sed to try and strip out parts of the diff based on the color codes:

git \
  -c color.diff.oldMoved='white dim' \
  -c color.diff.oldMovedAlternative='white dim' \
  -c color.diff.newMoved='white dim' \
  -c color.diff.newMovedAlternative='white dim' \
  -c color.diff.newMovedDimmed='white dim' \
  -c color.diff.newMovedAlternativeDimmed='white dim' \
  diff --ignore-blank-lines --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --minimal --color HEAD~1 HEAD | sed -r "s/\x1B\[2;37m\+(.*)\x1B\[m/\1/g" | sed -r "s/\x1B\[2m-(.*)\x1B\[m/\1/g" | sed -r "s/\x1B\[([0-9]{1,2}(;[0-9]{1,2})?)?[mGK]//g"

Strip 'dim white' +:

sed -r "s/\x1B\[2;37m\+(.*)\x1B\[m/\1/g"

Strip 'dim' -, keeping the content between:

sed -r "s/\x1B\[2m-(.*)\x1B\[m/\1/g"

Strip all colour codes:

sed -r "s/\x1B\[([0-9]{1,2}(;[0-9]{1,2})?)?[mGK]//g"

If we then put that colour stripped output into a standard GitHub code block with diff formatting, it will only use the + and - lines to determine the colouring to show, and so will highlight the 'actual' changes in red/green.

[0xdevalias]

Another quick little git alias to add to the aliases in my original post (Ref):

diff-refactor-commit = !"f() { git diff-refactor ${1}~1 ${1}; }; f"

Original alias for reference:

You can simplify this for future use by adding it to your git aliases in your ~/.gitconfig, something like the following:

[alias]
  diff-refactor = \
    -c color.diff.oldMoved='white dim' \
    -c color.diff.oldMovedAlternative='white dim' \
    -c color.diff.newMoved='white dim' \
    -c color.diff.newMovedAlternative='white dim' \
    -c color.diff.newMovedDimmed='white dim' \
    -c color.diff.newMovedAlternativeDimmed='white dim' \
    diff --ignore-blank-lines --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --minimal

Which would then allow you to use it as just:

⇒ git diff-refactor

⇒ git diff-refactor devalias/improve-first-use HEAD

⇒ git diff-refactor 55a259f~ 55a259f

⇒ git diff-refactor 55a259f~ 55a259f -- oauth/vendor/slack/index.js

[0xdevalias]

Some similar tangential references to me musing on --color-moved type things over the years:

• Explore AST based diff tools 0xdevalias/chatgpt-source-watch#3 (comment)

VSCode recently added a new feature called move code detection. The algorithm is surprisingly simple: after normal diff, calculate similarity(deletion, insertion) for every deletion & insertion pair. The code can be found here https://github.com/microsoft/vscode/blob/166097a20cbd06d10d255ef561837c439f372de3/src/vs/editor/common/diff/defaultLinesDiffComputer/computeMovedLines.ts#L44-L85.

But intuitively demonstrating the moving relationship on TUI might be a challenge.

Originally posted by @QuarticCat in Wilfred/difftastic#508 (comment)

• Explore AST based diff tools 0xdevalias/chatgpt-source-watch#3 (comment)

Including this one, where I started digging into the git source code to understand how it implements --color-moved 'moved detection'; which honestly seems simple enough that it could probably be implemented in a browser extension / user script / similar given the full added/removed diff of a PR:

• Explore AST based diff tools 0xdevalias/chatgpt-source-watch#3 (comment)

[soniacasas]

Any news about this?

[biajoeknee]

any news?

[skwasnia]

[philippotto]

As an alternative, you can use VS Code to review while highlighting moved code. It's available behind an experimental flag. It also works in githubs online version. So, simply press . in an open PR and review there. Announcement of the feature: https://code.visualstudio.com/updates/v1_82#_moved-code-detection

[Timmmm]

That only works within the same file. Phabricator shows lines that are moved between files.

[AHJiapengZhang]

Indeed, it doesn't work when functions are moved across files.

[vinkay215]

Any news about this?

[AHJiapengZhang]

Also vote for this feature. Current Vscode is able to detect line move inside a file. But not so when functions are moved across files. @hediet

[biajoeknee]

you know this isn't being prioritized because the squeaky wheel gets the oil and not enough wheels are squeaking for this.

and the reason not enough wheels are squeaking for this is because the majority of developers are not that scrutinizing (line-by-line) when it comes to code review, so the introduction of this feature wouldn't change many developers user experience much if at all.

[mberndt123]

This would be a huge boon for code review.