Add support for first-parent arg when fetching the list of changes from git#391
Add support for first-parent arg when fetching the list of changes from git#391zambamingi wants to merge 7 commits intojenkinsci:masterfrom
Conversation
| return includes(rev.name()); | ||
| } | ||
|
|
||
| public ChangelogCommand firstParent() { |
There was a problem hiding this comment.
Please add @Override and in the JGit implementation too.
There was a problem hiding this comment.
thanks for catching this, I addressed this in my latest commit
| return firstParent(true); | ||
| } | ||
|
|
||
| public ChangelogCommand firstParent(boolean firstParent) { |
There was a problem hiding this comment.
Please add @Override and in the JGit implementation too.
| ChangelogCommand includes(ObjectId rev); | ||
|
|
||
| /** | ||
| * Sets the firstParent private variable to true |
There was a problem hiding this comment.
I suggest to document the behaviour that is triggered by setting the variable value, not just that the variable is set. The former is more useful for the user of this API.
| ChangelogCommand firstParent(); | ||
|
|
||
| /** | ||
| * Sets the firstParent private variable to boolean param |
There was a problem hiding this comment.
See the comment for firstParent().
| /** | ||
| * Sets the firstParent private variable to boolean param | ||
| * | ||
| * @param firstParent a {@link java.lang.boolean} object. |
There was a problem hiding this comment.
Please don't include the type in description again (even if there are some existing Javadocs like this} - java.lang.boolean is not even valid.
| } | ||
|
|
||
| public ChangelogCommand firstParent(boolean firstParent) { | ||
| return this; |
There was a problem hiding this comment.
Please either implement this functionality in JGit (preferred) or log a warning that this parameter is not supported in the JGit implementation. Otherwise the user has no idea when enabling this feature why it's not working as expected.
There was a problem hiding this comment.
thanks for the suggestion, I addressed this in my latest commit
| String mergeMessage = "Merge message to be tested."; | ||
| w.git.merge().setMessage(mergeMessage).setGitPluginFastForwardMode(MergeCommand.GitPluginFastForwardMode.NO_FF).setRevisionToMerge(w.git.getHeadRev(w.repoPath(), "branch-1")).execute(); | ||
|
|
||
| if (w.git instanceof CliGitAPIImpl) { |
There was a problem hiding this comment.
I suggest to use @NotImplementedInJGit on the test method instead of this instanceof check. The former is more descriptive.
There was a problem hiding this comment.
thanks for the suggestion, I addressed this in my latest commit
|
Assigned label 'Changelog' and milestone 'Later' so that it can be revisited when reviewing changelog related pull requests |
3 participants
This is the git-client-plugin side of the changes to implement https://issues.jenkins-ci.org/browse/JENKINS-50366. There are changes in the git-plugin as well, I'll submitted a pull request there as well.