-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Git Log API. #2046
Add support for Git Log API. #2046
Conversation
All your comments were fixed |
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
Hi, do you have more comments to fix ? |
@@ -710,44 +721,163 @@ public void init(InitRequest request) throws GitException { | |||
} | |||
} | |||
|
|||
/** @see org.exoplatform.ide.git.server.GitConnection#log(org.exoplatform.ide.git.shared.LogRequest) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is invalid link
7d27f3e
to
e2b3485
Compare
@vinokurig |
*/ | ||
LogRequest withRevisionRangeUntil(String revisionRangeUntil); | ||
|
||
/** @return the integer value of the number of commits that will be skipped when calling log API */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the integer value of the number of commits that will be skipped when calling log command ?
} | ||
|
||
private List<DiffCommitFile> getCommitDiffFiles(RevCommit revCommit, String pattern) throws IOException { | ||
List<DiffEntry> diffs = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not necessary to initialize this variable as null
Could you please add tests for revision range in Log and for new fields in Revision |
8d524d7
to
297a3b0
Compare
@vinokurig |
I will review, as soon as possible |
/** @return the file change type*/ | ||
String getChangeType(); | ||
|
||
/** set the file change type*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start the javadoc with capital letter here and in other javadocs
void setRevisionRangeSince(String revisionRangeSince); | ||
|
||
/** | ||
* Create a LogRequest object based on a given revision range since. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a {@link LogRequest} object based on a given revision range since.
here and in other javadocs
/** @return revision range until */ | ||
String getRevisionRangeUntil(); | ||
|
||
/** @set revision range until */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove @
* | ||
* @param oldPath | ||
* file previous location | ||
* @return a DiffCommitFile object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide more info here and in other javadocs like this.
May be something like this: {@link DiffCommitFile} object that contains information about changes of the committed file
|
||
/** Set a list of DiffCommitFile objects, which describes the changes in the commit files */ | ||
void setDiffCommitFile(List<DiffCommitFile> v); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v
is not a good name for parameter
} | ||
|
||
@Test(dataProvider = "GitConnectionFactory", dataProviderClass = GitConnectionFactoryProvider.class) | ||
public void testLogWithSkipAndMaxCount(GitConnectionFactory connectionFactory) throws GitException, IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between this test and previous two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because i added three tests:
- testLogSkip - to check log with skip option
- testLogMaxCount - to check log with maxCount option
3)testLogWithSkipAndMaxCount - to check log with skip and maxCount together
In addition i added more tests
297a3b0
to
652c4f0
Compare
@vinokurig |
* | ||
* @param revisionRangeSince | ||
* revision range since | ||
* @return a {@link LogRequest} object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be better to remove javadoc fields like this in this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why ?
Do you mean to remove all javadocs from all the with[...] methods ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that @return a {@link LogRequest} object
tells nothing except methods return type.
As for me it would be better to remove such fields only in this class, or try to provide more information there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other Ok fo rme @sleshchenko?
* | ||
* @param branch | ||
* branch name | ||
* @return a Revision object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
/** | ||
* Create a Revision object based on a given branch name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace Revision
to {@link Revision} here and in other javadocs
/** | ||
* Parameter which shows that this revision is a fake revision (i.e. TO for Exception) | ||
* | ||
* @return boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide more info or remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not a parameter that we added , so i don't know what is the purpose of this parameter ?
It was there before i added my code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return boolean
looks bad for me
652c4f0
to
dbdacf0
Compare
@vinokurig |
return gitUsers; | ||
} | ||
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove spaces here
* Contributors: | ||
* Codenvy, S.A. - initial API and implementation | ||
*******************************************************************************/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra line
* | ||
* @author Shimon Ben Yair. | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra line
*/ | ||
DiffCommitFile withOldPath(String oldPath); | ||
|
||
/** @return the file new location*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | ||
* Parameter which shows that this revision is a fake revision (i.e. TO for Exception) | ||
* | ||
* @return boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return boolean
looks bad for me
dbdacf0
to
f4ad4cc
Compare
@vinokurig |
*/ | ||
DiffCommitFile withOldPath(String oldPath); | ||
|
||
/** @return the file new location */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment, it should be something like this: Returns new location of the file.
Please take a look to other javadocs like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Add support for skip and maxCount when calling the LOG API. Add support to get the log for a specific folder or file in the repository. For every commit that is returned when calling the LOG API, the following data was added: The author of the commit A list of branch names that the commit is related to. A list of file names that were changed/deleted/added in the commit. A list of commit parents (to enable for example clients to paint a graph) Change-Id: I2cc486cc49787c681b031067728b7a33f8fb11e7 Signed-off-by: Shimon Ben.Yair <shimon.ben.yair@sap.com>
f4ad4cc
to
e290997
Compare
@vinokurig |
1 similar comment
@vinokurig |
QA is in progress |
@vinokurig could you please merge this PR |
Add support for skip and maxCount when calling the LOG API. Add support to get the log for a specific folder or file in the repository. For every commit that is returned when calling the LOG API, the following data was added: The author of the commit A list of branch names that the commit is related to. A list of file names that were changed/deleted/added in the commit. A list of commit parents (to enable for example clients to paint a graph) Change-Id: I2cc486cc49787c681b031067728b7a33f8fb11e7 Signed-off-by: Shimon Ben.Yair <shimon.ben.yair@sap.com>
What does this PR do?
What issues does this PR fix or reference?
Previous Behavior
Remove this section if not relevant
New Behavior
This content may also be included in the release notes.
Tests written?
Yes/No
Docs requirements?
Include the content for all the docs changes required.
Please review Che's Contributing Guide for best practices.
Add support for skip and maxCount when calling the LOG API.
Add support to get the log for a specific folder or file in the repository.
For every commit that is returned when calling the LOG API, the following data was added:
The author of the commit
A list of branch names that the commit is related to.
A list of file names that were changed/deleted/added in the commit.
A list of commit parents (to enable for example clients to paint a graph)
Signed-off-by: Shimon Ben.Yair shimon.ben.yair@sap.com