You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@maven.apache.org by Tibor Digana <ti...@apache.org> on 2017/06/03 07:03:47 UTC
Code review. Please approve new branches.
Hi all,
We have a patch received from users group. I have improved it a bit, added
Javadoc and pushed the code to branch [1].
Jira issue related [2].
[1]:
https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git;a=shortlog;h=refs/heads/SUREFIRE-1376
[2]: https://issues.apache.org/jira/browse/SUREFIRE-1376
Build passed successfully, mvn install -P run-its.
Can I push it to master?
--
Cheers
Tibor
Re: Code review. Please approve new branches.
Posted by Guillaume Boué <gb...@apache.org>.
I've ran successfully the ITs on FreeBSD under JDK 7 and 8, so +1 from me.
Would it make sense to add an IT for SUREFIRE-1376?
Guillaume
Le 03/06/2017 à 13:53, Tibor Digana a écrit :
> Hi Michael,
>
> I have pushed branch SUREFIRE-1380_2, see [1], and separated the previous
> ticket SUREFIRE-1380 to two: SUREFIRE-1380 and SUREFIRE-1381.
>
> The branch SUREFIRE-1380_2 is for Jira SUREFIRE-1380.
> [1]:
> https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git;a=shortlog;h=refs/heads/SUREFIRE-1380_2
>
>
>
> On Sat, Jun 3, 2017 at 12:54 PM, Michael Osipov <mi...@apache.org> wrote:
>
>> Am 2017-06-03 um 12:36 schrieb Tibor Digana:
>>
>>> Michael, I will split SUREFIRE-1380 in two tickets on tomorrow evening.
>>> Another flush is necessary because the method InputStream.read() is called
>>> in a loop and the flush should be called after last byte. If another
>>> thread
>>> marks the stream to be closed asynchronously, then the bytes can be lost.
>>> Therefore flushing if stream has been closed in the intermediate time
>>> between these two threads. Maybe not clear to understand, we can have a
>>> look deeper.
>>>
>> Please add this profound description to the ticket itself. It will help to
>> understand the motivation.
>>
>>
>> On Sat, Jun 3, 2017 at 12:31 PM, Tibor Digana <tibor.digana@googlemail.com
>>> wrote:
>>>
>>> The changes in SUREFIRE-1376 are done.
>>>> On Sat, Jun 3, 2017 at 11:52 AM, Michael Osipov <mi...@apache.org>
>>>> wrote:
>>>>
>>>> Am 2017-06-03 um 10:56 schrieb Tibor Digana:
>>>>> I have added a new branch with small change only, SUREFIRE-1380.
>>>>>> https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git
>>>>>> ;a=shortlog;h=refs/heads/SUREFIRE-1380
>>>>>>
>>>>>>
>>>>> I am not happy with this: you mix two different taks in one issue,
>>>>> refactoring and flush. There is no explanation why another flush is
>>>>> necessary or what the benefit will be, i.e., don't fix things which
>>>>> aren't
>>>>> broken.
>>>>>
>>>>> WDYT?
>>>>>
>>>>>
>>>>> Michael
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>>>>> For additional commands, e-mail: dev-help@maven.apache.org
>>>>>
>>>>>
>>>>>
>>>> --
>>>> Cheers
>>>> Tibor
>>>>
>>>>
>>>
>>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>> For additional commands, e-mail: dev-help@maven.apache.org
>>
>>
>
---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Code review. Please approve new branches.
Posted by Michael Osipov <mi...@apache.org>.
Am 2017-06-03 um 13:53 schrieb Tibor Digana:
> Hi Michael,
>
> I have pushed branch SUREFIRE-1380_2, see [1], and separated the previous
> ticket SUREFIRE-1380 to two: SUREFIRE-1380 and SUREFIRE-1381.
>
> The branch SUREFIRE-1380_2 is for Jira SUREFIRE-1380.
> [1]:
> https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git;a=shortlog;h=refs/heads/SUREFIRE-1380_2
Looks both fine now to me.
> On Sat, Jun 3, 2017 at 12:54 PM, Michael Osipov <mi...@apache.org> wrote:
>
>> Am 2017-06-03 um 12:36 schrieb Tibor Digana:
>>
>>> Michael, I will split SUREFIRE-1380 in two tickets on tomorrow evening.
>>> Another flush is necessary because the method InputStream.read() is called
>>> in a loop and the flush should be called after last byte. If another
>>> thread
>>> marks the stream to be closed asynchronously, then the bytes can be lost.
>>> Therefore flushing if stream has been closed in the intermediate time
>>> between these two threads. Maybe not clear to understand, we can have a
>>> look deeper.
>>>
>>
>> Please add this profound description to the ticket itself. It will help to
>> understand the motivation.
>>
>>
>> On Sat, Jun 3, 2017 at 12:31 PM, Tibor Digana <tibor.digana@googlemail.com
>>>>
>>> wrote:
>>>
>>> The changes in SUREFIRE-1376 are done.
>>>>
>>>> On Sat, Jun 3, 2017 at 11:52 AM, Michael Osipov <mi...@apache.org>
>>>> wrote:
>>>>
>>>> Am 2017-06-03 um 10:56 schrieb Tibor Digana:
>>>>>
>>>>> I have added a new branch with small change only, SUREFIRE-1380.
>>>>>> https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git
>>>>>> ;a=shortlog;h=refs/heads/SUREFIRE-1380
>>>>>>
>>>>>>
>>>>> I am not happy with this: you mix two different taks in one issue,
>>>>> refactoring and flush. There is no explanation why another flush is
>>>>> necessary or what the benefit will be, i.e., don't fix things which
>>>>> aren't
>>>>> broken.
>>>>>
>>>>> WDYT?
>>>>>
>>>>>
>>>>> Michael
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>>>>> For additional commands, e-mail: dev-help@maven.apache.org
>>>>>
>>>>>
>>>>>
>>>>
>>>> --
>>>> Cheers
>>>> Tibor
>>>>
>>>>
>>>
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>> For additional commands, e-mail: dev-help@maven.apache.org
>>
>>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Code review. Please approve new branches.
Posted by Tibor Digana <ti...@googlemail.com>.
Hi Michael,
I have pushed branch SUREFIRE-1380_2, see [1], and separated the previous
ticket SUREFIRE-1380 to two: SUREFIRE-1380 and SUREFIRE-1381.
The branch SUREFIRE-1380_2 is for Jira SUREFIRE-1380.
[1]:
https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git;a=shortlog;h=refs/heads/SUREFIRE-1380_2
On Sat, Jun 3, 2017 at 12:54 PM, Michael Osipov <mi...@apache.org> wrote:
> Am 2017-06-03 um 12:36 schrieb Tibor Digana:
>
>> Michael, I will split SUREFIRE-1380 in two tickets on tomorrow evening.
>> Another flush is necessary because the method InputStream.read() is called
>> in a loop and the flush should be called after last byte. If another
>> thread
>> marks the stream to be closed asynchronously, then the bytes can be lost.
>> Therefore flushing if stream has been closed in the intermediate time
>> between these two threads. Maybe not clear to understand, we can have a
>> look deeper.
>>
>
> Please add this profound description to the ticket itself. It will help to
> understand the motivation.
>
>
> On Sat, Jun 3, 2017 at 12:31 PM, Tibor Digana <tibor.digana@googlemail.com
>> >
>> wrote:
>>
>> The changes in SUREFIRE-1376 are done.
>>>
>>> On Sat, Jun 3, 2017 at 11:52 AM, Michael Osipov <mi...@apache.org>
>>> wrote:
>>>
>>> Am 2017-06-03 um 10:56 schrieb Tibor Digana:
>>>>
>>>> I have added a new branch with small change only, SUREFIRE-1380.
>>>>> https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git
>>>>> ;a=shortlog;h=refs/heads/SUREFIRE-1380
>>>>>
>>>>>
>>>> I am not happy with this: you mix two different taks in one issue,
>>>> refactoring and flush. There is no explanation why another flush is
>>>> necessary or what the benefit will be, i.e., don't fix things which
>>>> aren't
>>>> broken.
>>>>
>>>> WDYT?
>>>>
>>>>
>>>> Michael
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>>>> For additional commands, e-mail: dev-help@maven.apache.org
>>>>
>>>>
>>>>
>>>
>>> --
>>> Cheers
>>> Tibor
>>>
>>>
>>
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
> For additional commands, e-mail: dev-help@maven.apache.org
>
>
--
Cheers
Tibor
Re: Code review. Please approve new branches.
Posted by Michael Osipov <mi...@apache.org>.
Am 2017-06-03 um 12:36 schrieb Tibor Digana:
> Michael, I will split SUREFIRE-1380 in two tickets on tomorrow evening.
> Another flush is necessary because the method InputStream.read() is called
> in a loop and the flush should be called after last byte. If another thread
> marks the stream to be closed asynchronously, then the bytes can be lost.
> Therefore flushing if stream has been closed in the intermediate time
> between these two threads. Maybe not clear to understand, we can have a
> look deeper.
Please add this profound description to the ticket itself. It will help
to understand the motivation.
> On Sat, Jun 3, 2017 at 12:31 PM, Tibor Digana <ti...@googlemail.com>
> wrote:
>
>> The changes in SUREFIRE-1376 are done.
>>
>> On Sat, Jun 3, 2017 at 11:52 AM, Michael Osipov <mi...@apache.org>
>> wrote:
>>
>>> Am 2017-06-03 um 10:56 schrieb Tibor Digana:
>>>
>>>> I have added a new branch with small change only, SUREFIRE-1380.
>>>> https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git
>>>> ;a=shortlog;h=refs/heads/SUREFIRE-1380
>>>>
>>>
>>> I am not happy with this: you mix two different taks in one issue,
>>> refactoring and flush. There is no explanation why another flush is
>>> necessary or what the benefit will be, i.e., don't fix things which aren't
>>> broken.
>>>
>>> WDYT?
>>>
>>>
>>> Michael
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>>> For additional commands, e-mail: dev-help@maven.apache.org
>>>
>>>
>>
>>
>> --
>> Cheers
>> Tibor
>>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Code review. Please approve new branches.
Posted by Tibor Digana <ti...@googlemail.com>.
Michael, I will split SUREFIRE-1380 in two tickets on tomorrow evening.
Another flush is necessary because the method InputStream.read() is called
in a loop and the flush should be called after last byte. If another thread
marks the stream to be closed asynchronously, then the bytes can be lost.
Therefore flushing if stream has been closed in the intermediate time
between these two threads. Maybe not clear to understand, we can have a
look deeper.
On Sat, Jun 3, 2017 at 12:31 PM, Tibor Digana <ti...@googlemail.com>
wrote:
> The changes in SUREFIRE-1376 are done.
>
> On Sat, Jun 3, 2017 at 11:52 AM, Michael Osipov <mi...@apache.org>
> wrote:
>
>> Am 2017-06-03 um 10:56 schrieb Tibor Digana:
>>
>>> I have added a new branch with small change only, SUREFIRE-1380.
>>> https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git
>>> ;a=shortlog;h=refs/heads/SUREFIRE-1380
>>>
>>
>> I am not happy with this: you mix two different taks in one issue,
>> refactoring and flush. There is no explanation why another flush is
>> necessary or what the benefit will be, i.e., don't fix things which aren't
>> broken.
>>
>> WDYT?
>>
>>
>> Michael
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>> For additional commands, e-mail: dev-help@maven.apache.org
>>
>>
>
>
> --
> Cheers
> Tibor
>
--
Cheers
Tibor
Re: Code review. Please approve new branches.
Posted by Michael Osipov <mi...@apache.org>.
Looks good now.
Am 2017-06-03 um 12:31 schrieb Tibor Digana:
> The changes in SUREFIRE-1376 are done.
>
> On Sat, Jun 3, 2017 at 11:52 AM, Michael Osipov <mi...@apache.org> wrote:
>
>> Am 2017-06-03 um 10:56 schrieb Tibor Digana:
>>
>>> I have added a new branch with small change only, SUREFIRE-1380.
>>> https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git
>>> ;a=shortlog;h=refs/heads/SUREFIRE-1380
>>>
>>
>> I am not happy with this: you mix two different taks in one issue,
>> refactoring and flush. There is no explanation why another flush is
>> necessary or what the benefit will be, i.e., don't fix things which aren't
>> broken.
>>
>> WDYT?
>>
>>
>> Michael
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>> For additional commands, e-mail: dev-help@maven.apache.org
>>
>>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Code review. Please approve new branches.
Posted by Tibor Digana <ti...@googlemail.com>.
The changes in SUREFIRE-1376 are done.
On Sat, Jun 3, 2017 at 11:52 AM, Michael Osipov <mi...@apache.org> wrote:
> Am 2017-06-03 um 10:56 schrieb Tibor Digana:
>
>> I have added a new branch with small change only, SUREFIRE-1380.
>> https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git
>> ;a=shortlog;h=refs/heads/SUREFIRE-1380
>>
>
> I am not happy with this: you mix two different taks in one issue,
> refactoring and flush. There is no explanation why another flush is
> necessary or what the benefit will be, i.e., don't fix things which aren't
> broken.
>
> WDYT?
>
>
> Michael
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
> For additional commands, e-mail: dev-help@maven.apache.org
>
>
--
Cheers
Tibor
Re: Code review. Please approve new branches.
Posted by Michael Osipov <mi...@apache.org>.
Am 2017-06-03 um 10:56 schrieb Tibor Digana:
> I have added a new branch with small change only, SUREFIRE-1380.
> https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git;a=shortlog;h=refs/heads/SUREFIRE-1380
I am not happy with this: you mix two different taks in one issue,
refactoring and flush. There is no explanation why another flush is
necessary or what the benefit will be, i.e., don't fix things which
aren't broken.
WDYT?
Michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: Code review. Please approve new branches.
Posted by Tibor Digana <ti...@apache.org>.
I have added a new branch with small change only, SUREFIRE-1380.
https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git;a=shortlog;h=refs/heads/SUREFIRE-1380
On Sat, Jun 3, 2017 at 9:03 AM, Tibor Digana <ti...@apache.org> wrote:
> Hi all,
>
> We have a patch received from users group. I have improved it a bit, added
> Javadoc and pushed the code to branch [1].
>
> Jira issue related [2].
>
> [1]: https://git-wip-us.apache.org/repos/asf?p=maven-surefire.
> git;a=shortlog;h=refs/heads/SUREFIRE-1376
>
> [2]: https://issues.apache.org/jira/browse/SUREFIRE-1376
>
> Build passed successfully, mvn install -P run-its.
> Can I push it to master?
>
> --
> Cheers
> Tibor
>
Re: Code review. Please approve new branches.
Posted by Michael Osipov <mi...@apache.org>.
Am 2017-06-03 um 09:03 schrieb Tibor Digana:
> Hi all,
>
> We have a patch received from users group. I have improved it a bit, added
> Javadoc and pushed the code to branch [1].
>
> Jira issue related [2].
>
> [1]:
> https://git-wip-us.apache.org/repos/asf?p=maven-surefire.git;a=shortlog;h=refs/heads/SUREFIRE-1376
>
> [2]: https://issues.apache.org/jira/browse/SUREFIRE-1376
>
> Build passed successfully, mvn install -P run-its.
> Can I push it to master?
Two issues:
1. Why normalizePath()? You don't normalize anything but simply escape it.
2. It be nice to have a test for MAX_PATH at least. UNC won't be
testable in a portable way.
Michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org