Skip to content
This repository has been archived by the owner on Nov 18, 2021. It is now read-only.

Use a diffing algorithm for PRs that shows code moves more succinctly than one add and remove hunk #729

Open
tommyjcarpenter opened this issue Jul 26, 2016 · 6 comments

Comments

@tommyjcarpenter
Copy link

PRs that move code around, even within the same function, appear very onerous on Github, even when they don't do anything else. I have created a very basic PR to demonstrate this: https://github.com/tommyjcarpenter/github_test/pull/1/commits/2afb07ec5c6b56724bd10c6b56386299493bbb43. All the repo does is define two functions, and PRs a change to move the first below the second. The diff on github shows 20 lines removed and 21 added. One would assume the diff could be shown as a trivial "code move".

Now imagine this with a lot more functions and a lot more trivial code moves.

It seems git itself IS able to detect such changes: http://stackoverflow.com/questions/12590947/using-git-diff-to-detect-code-movement-how-to-use-diff-options

So, is there a way to swap out the diffing algorithm such that PRs like this don't look so onerous? Does github use it's own internal algorithm or does it use your default diffing algorithm?

(EDIT: this also appears to make account-level contributions on Github a bit misleading: someone that just moves code around may be shown to make a huge number of additions and deletions to a repository, thus giving the impression that they are a large contributor, when in fact they didn't contribute any functionality)

@cirosantilli cirosantilli changed the title Github's diffing algorithm for PRs: can you swap it? Use a diffing algorithm for PRs that shows code moves more succinctly than one add and remove hunk Jul 29, 2016
@cirosantilli
Copy link
Collaborator

cirosantilli commented Jul 29, 2016

I couldn't find such diffing algorithm in Git in that SO post, can you give the git command that does it, and your git version?

@tommyjcarpenter
Copy link
Author

See first answer here:
http://stackoverflow.com/questions/6412516/configuring-diff-tool-with-gitconfig

On Friday, July 29, 2016, Ciro Santilli 六四事件 法轮功 包卓轩 <
notifications@github.com> wrote:

I couldn't find such diffing algorithm in that SO post, can you give the
git command that does it, and your git version?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#729 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AI-5dTQ_SfuQaZmQmb6T1KfKRVOD0aEfks5qaaOlgaJpZM4JVkum
.

@maxlk
Copy link

maxlk commented Jan 16, 2019

@cirosantilli now git has new --color-moved option: https://blog.github.com/2018-04-05-git-217-released/

@chiphogg
Copy link

Phabricator has the ability to flag lines as moved or copied within a single diff. It's hard to overstate how much this helps refactoring.

@wedamija
Copy link

wedamija commented Jun 1, 2020

It'd be great to have an option to apply --color-moved=dimmed_zebra to a pr. I can do this locally, but would make it much easier to review prs that involve refactoring in the github ui.

@pardis-integral
Copy link

This is one of the biggest pains I've had since I left Google to work at a startup.

I find that I waste my time re-reading moved lines or expend precious mental energy verifying that they are in fact simply moved and nothing else about them has changed.

A simple refactoring task incurs such a high cost to reviewers that I think most people either don't do them or don't review the resulting PR carefully and hope that the code is well-tested enough that it doesn't matter.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants