Re: Review Request 45057: Made `unzip` overwrite existing files without prompting.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45057/#review124468
---



Patch looks great!

Reviews applied: [45057]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 20, 2016, 7:50 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45057/
> ---
> 
> (Updated March 20, 2016, 7:50 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4885
> https://issues.apache.org/jira/browse/MESOS-4885
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `unzip` overwrite existing files without prompting.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 45057: Made `unzip` overwrite existing files without prompting.

[Tomasz Janiszewski]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45057/
---

(Updated March 20, 2016, 7:50 p.m.)


Review request for mesos and Jie Yu.


Bugs: MESOS-4885
https://issues.apache.org/jira/browse/MESOS-4885


Repository: mesos


Description
---

Made `unzip` overwrite existing files without prompting.


Diffs (updated)
-

  src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
  src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 

Diff: https://reviews.apache.org/r/45057/diff/


Testing
---


Thanks,

Tomasz Janiszewski

Re: Review Request 45057: Made `unzip` overwrite existing files without prompting.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45057/#review124442
---



Patch looks great!

Reviews applied: [45057]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 20, 2016, 12:05 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45057/
> ---
> 
> (Updated March 20, 2016, 12:05 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4885
> https://issues.apache.org/jira/browse/MESOS-4885
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `unzip` overwrite existing files without prompting.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 45057: Made `unzip` overwrite existing files without prompting.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45057/#review124439
---


Fix it, then Ship it!




Ship It!


src/tests/fetcher_tests.cpp (line 703)


should we replace `af083b2d` to `440a6aa5` here?


- haosdent huang


On March 20, 2016, 12:05 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45057/
> ---
> 
> (Updated March 20, 2016, 12:05 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4885
> https://issues.apache.org/jira/browse/MESOS-4885
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `unzip` overwrite existing files without prompting.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 45057: Made `unzip` overwrite existing files without prompting.

[Tomasz Janiszewski]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45057/
---

(Updated March 20, 2016, 12:05 a.m.)


Review request for mesos and Jie Yu.


Bugs: MESOS-4885
https://issues.apache.org/jira/browse/MESOS-4885


Repository: mesos


Description
---

Made `unzip` overwrite existing files without prompting.


Diffs (updated)
-

  src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
  src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 

Diff: https://reviews.apache.org/r/45057/diff/


Testing
---


Thanks,

Tomasz Janiszewski

Re: Review Request 45057: Made `unzip` overwrite existing files without prompting.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45057/#review124422
---


Fix it, then Ship it!




Thanks for the patch! This looks great! I made some comments on the first test, 
but the same comments should apply to other tests as well. Once the path is 
updated, I'll commit it. Thanks!


src/tests/fetcher_tests.cpp (line 642)


2 lines apart



src/tests/fetcher_tests.cpp (lines 647 - 649)


Can you create a temp file under os::getcwd()? The TemporaryDirectoryTest 
fixture will cleanup the current working directory after the test 
finishes/failed.

If the test fails, the existing code will leak the tmp zip file.



src/tests/fetcher_tests.cpp (line 648)


Kill this line.



src/tests/fetcher_tests.cpp (lines 657 - 660)


Indentation here should be 4:

```
ASSERT_SOME(...
"UES."
""
"...").get()));
```



src/tests/fetcher_tests.cpp (line 661)


Insert a new line above. We typically insert a new line after a multi-line 
statement.



src/tests/fetcher_tests.cpp (line 682)


s/extract/extractedFile/

Please use path::join(os::getcwd(), "hello");



src/tests/fetcher_tests.cpp (line 687)


This is not necessary if you put the zip file under os::getcwd() as I 
mentioned above.



src/tests/fetcher_tests.cpp (line 689)


2 lines apart please.



src/tests/fetcher_tests.cpp (line 694)


Ditto. Kill this line.


- Jie Yu


On March 19, 2016, 10:26 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45057/
> ---
> 
> (Updated March 19, 2016, 10:26 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4885
> https://issues.apache.org/jira/browse/MESOS-4885
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `unzip` overwrite existing files without prompting.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 45057: Made `unzip` overwrite existing files without prompting.

[Tomasz Janiszewski]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45057/
---

(Updated March 19, 2016, 10:26 a.m.)


Review request for mesos and Jie Yu.


Bugs: MESOS-4885
https://issues.apache.org/jira/browse/MESOS-4885


Repository: mesos


Description
---

Made `unzip` overwrite existing files without prompting.


Diffs (updated)
-

  src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
  src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 

Diff: https://reviews.apache.org/r/45057/diff/


Testing
---


Thanks,

Tomasz Janiszewski

Re: Review Request 45057: Made `unzip` overwrite existing files without prompting.

[Tomasz Janiszewski]

> On March 19, 2016, 7:08 a.m., haosdent huang wrote:
> >

In fact zipfile containg duplicates is valid zip. There is a problem with 
extracting files, becouse `unzip` will prompt user how to deal with it. Forcing 
overwrite makes unzip works like `tar` and `gunzip`. When zip file is invalid 
or somehow broken (e.g., readable but CRC doesn't match) unzip will do 
extraction but return non zero. I added test case for it.


- Tomasz


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45057/#review124380
---


On March 18, 2016, 10:29 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45057/
> ---
> 
> (Updated March 18, 2016, 10:29 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4885
> https://issues.apache.org/jira/browse/MESOS-4885
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `unzip` overwrite existing files without prompting.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 45057: Made `unzip` overwrite existing files without prompting.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45057/#review124380
---




src/launcher/fetcher.cpp (line 77)


This would ignore the error when the zip file is broken. Is it possible to 
return `Error` instead of supress the error here?



src/tests/fetcher_tests.cpp (line 652)


Seems not all Linux distributions contains `zip` command.



src/tests/fetcher_tests.cpp (line 674)


I suggest use `string` instead of `auto` here.



src/tests/fetcher_tests.cpp (line 690)


Could you add comments why use this content?

According my test, this content is a zip file which have contains two A 
file.



src/tests/fetcher_tests.cpp (line 717)


Suggest use `string` instead of `auto` as well.


- haosdent huang


On March 18, 2016, 10:29 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45057/
> ---
> 
> (Updated March 18, 2016, 10:29 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4885
> https://issues.apache.org/jira/browse/MESOS-4885
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `unzip` overwrite existing files without prompting.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Review Request 45057: Made `unzip` overwrite existing files without prompting.

[Tomasz Janiszewski]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45057/
---

Review request for mesos and Jie Yu.


Bugs: MESOS-4885
https://issues.apache.org/jira/browse/MESOS-4885


Repository: mesos


Description
---

Made `unzip` overwrite existing files without prompting.


Diffs
-

  src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
  src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 

Diff: https://reviews.apache.org/r/45057/diff/


Testing
---


Thanks,

Tomasz Janiszewski