[SCM-1027] Wrong implementation of git diff --cached (when endRevision set)

**[Jakob Vad Nielsen](https://issues.apache.org/jira/secure/ViewProfile.jspa?name=lazeetown)** opened **[SCM-1027](https://issues.apache.org/jira/browse/SCM-1027?redirect=false)** and commented

Hi,

The `createCommandLine` function In `{}GitDiffCommand.java{`}, has a buggy implementation. The bug appears to have been there for a very long time. It is triggered when giving both `startRevision` and `endRevision` to this method (via `{}executeDiffCommand{`})

[link to the method](https://github.com/apache/maven-scm/blob/master/maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gitexe/src/main/java/org/apache/maven/scm/provider/git/gitexe/command/diff/GitDiffCommand.java#L75)

```java
public static Commandline createCommandLine(File workingDirectory, ScmVersion startVersion, ScmVersion endVersion, boolean cached) {

Commandline cl = GitCommandLineUtils.getBaseGitCommandLine(workingDirectory, "diff");       
if (cached) {
  cl.createArg().setValue("--cached");
  }       
if (startVersion != null && StringUtils.isNotEmpty(startVersion.getName())) {
  cl.createArg().setValue(startVersion.getName());
  }
  if (endVersion != null && StringUtils.isNotEmpty(endVersion.getName())) {
  cl.createArg().setValue(endVersion.getName());
  }       
return cl;
}
```

The problem is that both startRevision and endRevision are added as arguments to  git diff  when the --cached flag is set. Git diff only supports one argument in that case. It has been like that since the flag was introduced in Git, long time ago. Setting an endRevision throws an exception, as the git runtime fails with two arguments.

Reproduce by running this command in any repo:

```java
git diff --cached HEAD~2 HEAD~1
```

From git doc:

```java
git diff [] --cached [--merge-base] [] [--] [...]
           This form is to view the changes you staged for the next commit relative to the named . Typically you would want comparison with the latest commit, so if you do not give , it defaults to HEAD. If HEAD does not exist (e.g. unborn branches) and is not
           given, it shows all staged changes.  --staged is a synonym of --cached.
```

The easy fix would be to make sure the endVersion argument is not added when the boolean cached argument is set to true.

But I do not think this is actually the problem. When I want to do a diff between two revisions, I do not want to get a diff that also includes staged changes (which is what --cached actually does). Actually, I do not want it at all. I suspect that this diff command was introduced to solve special case for the Maven Release Plugin, where you want to diff against a specific commit AND make sure there are no staged commits. And then solved both scenarios in the same executeDiffCommand.

In order for me to be able to use this code in my plugin (where I want to diff between for example HEAD~3 and HEAD~2), I think there are two possible solutions:
1. Introduce a boolean parameter to the exeucteDiffCommand. Like `includeCached` or `includeStagedChanges`. This way I could at least chose to not include staged.
2. Have two diff commands. DiffCommand and DiffCachedCommand.

I guess both variants in some way would introduce a breaking change, unless new commands are added in addition to the existing one. 

Just wanted to let you know. I can help out with a PR if this is something you want to fix. If so, we need to agree on what the right solution would be.

 

---

**Affects:** 1.13.0, 2.0.0, 2.0.1, 2.1.0

Start with maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gitexe/src/main/java/org/apache/maven/scm/provider/git/gitexe/command/diff/GitDiffCommand.java, especially createCommandLine and executeDiffCommand. Reproduce the reported git diff --cached HEAD~2 HEAD~1 failure, then resolve whether cached diffs should accept one revision or whether ordinary two-revision diffs need a separate option or command.

Written by the indexing model from the issue text.