[
https://issues.apache.org/jira/browse/SCM-1027?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jakob Vad Nielsen updated SCM-1027:
-----------------------------------
Description:
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]
{code: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;
} {code}
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:
{code:java}
git diff --cached HEAD~2 HEAD~1 {code}
>From git doc:
{code:java}
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. {code}
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:
# Introduce a boolean parameter to the exeucteDiffCommand. Like
`includeCached` or `includeStagedChanges`. This way I could at least chose to
not include staged.
# 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.
was:
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]
{code: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;
} {code}
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, when that flag is set. And it has been like since the flag was
introduced in Git, long time ago. Giving a endRevision throws an exception, as
the git runtime fails with two arguments.
Reproduce by running this command in any repo:
{code:java}
git diff --cached HEAD~2 HEAD~1 {code}
>From git doc:
{code:java}
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. {code}
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:
# Introduce a boolean parameter to the exeucteDiffCommand. Like
`includeCached` or `includeStagedChanges`. This way I could at least chose to
not include staged.
# 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.
> Wrong implementation of git diff, with --cached argument when endRevision set
> -----------------------------------------------------------------------------
>
> Key: SCM-1027
> URL: https://issues.apache.org/jira/browse/SCM-1027
> Project: Maven SCM
> Issue Type: Bug
> Components: maven-scm-provider-gitexe, maven-scm-provider-jgit
> Affects Versions: 1.13.0, 2.0.0, 2.0.1, 2.1.0
> Reporter: Jakob Vad Nielsen
> Priority: Major
>
> 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]
> {code: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;
> } {code}
> 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:
> {code:java}
> git diff --cached HEAD~2 HEAD~1 {code}
> From git doc:
> {code:java}
> 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. {code}
> 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:
> # Introduce a boolean parameter to the exeucteDiffCommand. Like
> `includeCached` or `includeStagedChanges`. This way I could at least chose to
> not include staged.
> # 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.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
