[
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
only triggered when specifying 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 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 ato 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 exeuteDiffCommand. Like `includeCached`
or `includeStagedChanges`. This way I could at least chose to not include
staged.
# Have two diff commands. DiffCommand and DiffCached.
I guess both variants in some way would introduce a breaking change, unless
additional 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. This bug appears to have been there for a very long time. It is
only triggered when specifying 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 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 ato 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 exeuteDiffCommand. Like `includeCached`
or `includeStagedChanges`. This way I could at least chose to not include
staged.
# Have two diff commands. DiffCommand and DiffCached.
I guess both variants in some way would introduce a breaking change, unless
additional 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 only triggered when specifying 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 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 ato 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 exeuteDiffCommand. Like
> `includeCached` or `includeStagedChanges`. This way I could at least chose to
> not include staged.
> # Have two diff commands. DiffCommand and DiffCached.
> I guess both variants in some way would introduce a breaking change, unless
> additional 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)
