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

[jira-importer]

Jakob Vad Nielsen opened SCM-1027 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

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:

git diff --cached HEAD~2 HEAD~1 

From git doc:

git diff [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]
           This form is to view the changes you staged for the next commit relative to the named <commit>. Typically you would want comparison with the latest commit, so if you do not give <commit>, it defaults to HEAD. If HEAD does not exist (e.g. unborn branches) and <commit> 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 HEAD3 and HEAD2), 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