Re: Review Request 129400: Avoid potential access of .last() on empty list

2016-11-22 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129400/
---

(Updated Nov. 22, 2016, 10:42 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Plasma.


Repository: kpackage


Description
---

locateAll can return nothing, therefore we can't always call .last() on
it.

Existing code should be able to handle an empty package root as the user
could also also have provided a broken package root as an argument.


Diffs
-

  src/kpackagetool/kpackagetool.cpp 6135e14ba717d579553036f9ed7874813046a1db 

Diff: https://git.reviewboard.kde.org/r/129400/diff/


Testing
---

Ran kpackagetool --generate-index -g

Previously asserted. No longer asserted.


Thanks,

David Edmundson

Re: Review Request 129400: Avoid potential access of .last() on empty list

2016-11-15 Thread Marco Martin 


> On Nov. 14, 2016, 1:40 p.m., Aleix Pol Gonzalez wrote:
> > src/kpackagetool/kpackagetool.cpp, line 502
> > 
> >
> > Then just change to `QStandardPath::locate`?
> 
> David Edmundson wrote:
> Sounds sensible to me. Though I don't really know this code.
> Marco, should I do that?

err, comment in the review below was intended as answer for this thread (damn 
ui ;)


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129400/#review100834
---


On Nov. 14, 2016, 1:35 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129400/
> ---
> 
> (Updated Nov. 14, 2016, 1:35 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> locateAll can return nothing, therefore we can't always call .last() on
> it.
> 
> Existing code should be able to handle an empty package root as the user
> could also also have provided a broken package root as an argument.
> 
> 
> Diffs
> -
> 
>   src/kpackagetool/kpackagetool.cpp 6135e14ba717d579553036f9ed7874813046a1db 
> 
> Diff: https://git.reviewboard.kde.org/r/129400/diff/
> 
> 
> Testing
> ---
> 
> Ran kpackagetool --generate-index -g
> 
> Previously asserted. No longer asserted.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 129400: Avoid potential access of .last() on empty list

2016-11-15 Thread Marco Martin 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129400/#review100858
---


Fix it, then Ship it!




other than that, ship it


src/kpackagetool/kpackagetool.cpp (line 502)


no, locate would return the first most suitable it found, that is usually 
the local one under ~/.local

whuile this being with global option we need the system one instead. I 
don't see an option in qstandardpaths to ignore the local stuff, so using the 
last one of the things returned by locateAll is the better heuristic i found 
for having this behavior


- Marco Martin


On Nov. 14, 2016, 1:35 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129400/
> ---
> 
> (Updated Nov. 14, 2016, 1:35 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> locateAll can return nothing, therefore we can't always call .last() on
> it.
> 
> Existing code should be able to handle an empty package root as the user
> could also also have provided a broken package root as an argument.
> 
> 
> Diffs
> -
> 
>   src/kpackagetool/kpackagetool.cpp 6135e14ba717d579553036f9ed7874813046a1db 
> 
> Diff: https://git.reviewboard.kde.org/r/129400/diff/
> 
> 
> Testing
> ---
> 
> Ran kpackagetool --generate-index -g
> 
> Previously asserted. No longer asserted.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 129400: Avoid potential access of .last() on empty list

2016-11-15 Thread David Edmundson 


> On Nov. 14, 2016, 1:40 p.m., Aleix Pol Gonzalez wrote:
> > src/kpackagetool/kpackagetool.cpp, line 502
> > 
> >
> > Then just change to `QStandardPath::locate`?

Sounds sensible to me. Though I don't really know this code.
Marco, should I do that?


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129400/#review100834
---


On Nov. 14, 2016, 1:35 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129400/
> ---
> 
> (Updated Nov. 14, 2016, 1:35 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> locateAll can return nothing, therefore we can't always call .last() on
> it.
> 
> Existing code should be able to handle an empty package root as the user
> could also also have provided a broken package root as an argument.
> 
> 
> Diffs
> -
> 
>   src/kpackagetool/kpackagetool.cpp 6135e14ba717d579553036f9ed7874813046a1db 
> 
> Diff: https://git.reviewboard.kde.org/r/129400/diff/
> 
> 
> Testing
> ---
> 
> Ran kpackagetool --generate-index -g
> 
> Previously asserted. No longer asserted.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 129400: Avoid potential access of .last() on empty list

2016-11-14 Thread Aleix Pol Gonzalez 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129400/#review100834
---




src/kpackagetool/kpackagetool.cpp (line 502)


Then just change to `QStandardPath::locate`?


- Aleix Pol Gonzalez


On Nov. 14, 2016, 2:35 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129400/
> ---
> 
> (Updated Nov. 14, 2016, 2:35 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> locateAll can return nothing, therefore we can't always call .last() on
> it.
> 
> Existing code should be able to handle an empty package root as the user
> could also also have provided a broken package root as an argument.
> 
> 
> Diffs
> -
> 
>   src/kpackagetool/kpackagetool.cpp 6135e14ba717d579553036f9ed7874813046a1db 
> 
> Diff: https://git.reviewboard.kde.org/r/129400/diff/
> 
> 
> Testing
> ---
> 
> Ran kpackagetool --generate-index -g
> 
> Previously asserted. No longer asserted.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Review Request 129400: Avoid potential access of .last() on empty list

2016-11-14 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129400/
---

Review request for KDE Frameworks and Plasma.


Repository: kpackage


Description
---

locateAll can return nothing, therefore we can't always call .last() on
it.

Existing code should be able to handle an empty package root as the user
could also also have provided a broken package root as an argument.


Diffs
-

  src/kpackagetool/kpackagetool.cpp 6135e14ba717d579553036f9ed7874813046a1db 

Diff: https://git.reviewboard.kde.org/r/129400/diff/


Testing
---

Ran kpackagetool --generate-index -g

Previously asserted. No longer asserted.


Thanks,

David Edmundson