Code review. Please approve new branches.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Code review. Please approve new branches.

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?

--
Cheers
Tibor
Reply | Threaded
Open this post in threaded view
|

Re: Code review. Please approve new branches.

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


On Sat, Jun 3, 2017 at 9:03 AM, Tibor Digana <[hidden email]> 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
>
Reply | Threaded
Open this post in threaded view
|

Re: Code review. Please approve new branches.

michaelo
In reply to this post by Tibor Digana
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Code review. Please approve new branches.

michaelo
In reply to this post by Tibor Digana
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Code review. Please approve new branches.

gboue
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 <[hidden email]> 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 <[hidden email]
>>> wrote:
>>>
>>> The changes in SUREFIRE-1376 are done.
>>>> On Sat, Jun 3, 2017 at 11:52 AM, Michael Osipov <[hidden email]>
>>>> 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: [hidden email]
>>>>> For additional commands, e-mail: [hidden email]
>>>>>
>>>>>
>>>>>
>>>> --
>>>> Cheers
>>>> Tibor
>>>>
>>>>
>>>
>>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>


---
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: [hidden email]
For additional commands, e-mail: [hidden email]