cros_workon: check selected packages if they are cros-workon, related fixes

cros_workon: check selected packages if they are cros-workon, related fixes

Override locally warn/error functions to not interfere with stdout of canonicalize_name.
Future FIXME in common.sh.

[djmm, 2010-07-13 01:09:14]

http://codereview.chromium.org/2985003/diff/1/2
File cros_workon (right):

http://codereview.chromium.org/2985003/diff/1/2#newcode87
cros_workon:87: if [ "$?" = "1" ]; then
You can use the form:
if ! pkgfile=$(${EQUERY} which $1)

http://codereview.chromium.org/2985003/diff/1/2#newcode122
cros_workon:122: ATOM_LIST=$(show_live_ebuilds)
Short form:
ATOM_LIST=${ATOM_LIST:-$(show_live_ebuilds)}

[zbehan, 2010-07-13 02:50:00]

On Mon, Jul 12, 2010 at 6:09 PM, <djmm@chromium.org> wrote:

>
> http://codereview.chromium.org/2985003/diff/1/2
> File cros_workon (right):
>
> http://codereview.chromium.org/2985003/diff/1/2#newcode87
> cros_workon:87: if [ "$?" = "1" ]; then
> You can use the form:
> if ! pkgfile=$(${EQUERY} which $1)
>

Yep. Though I tried to keep things not overcomplicated, but I can change
this.


> http://codereview.chromium.org/2985003/diff/1/2#newcode122
> cros_workon:122: ATOM_LIST=$(show_live_ebuilds)
> Short form:
>
> ATOM_LIST=${ATOM_LIST:-$(show_live_ebuilds)}
>

This won't provide you with the else branch though. But I noticed another
slight detail wrt that. (will fix)

[Mandeep Singh Baines, 2010-07-13 20:09:37]

http://codereview.chromium.org/2985003/diff/6001/7001
File cros_workon (left):

http://codereview.chromium.org/2985003/diff/6001/7001#oldcode80
cros_workon:80: [ -n "${name}" ] || return 1
Why get rid of the return value?

http://codereview.chromium.org/2985003/diff/6001/7001
File cros_workon (right):

http://codereview.chromium.org/2985003/diff/6001/7001#newcode22
cros_workon:22: echo -e "${V_BOLD_GREEN}INFO   : $1${V_VIDOFF}" 1>&2
Instead of duplicate implementation, why not:

info "My message" 1>&2

http://codereview.chromium.org/2985003/diff/6001/7001#newcode124
cros_workon:124: if expr match "${ATOM_LIST}" "^ *$" &> /dev/null; then
This is difficult for most folks to parse. Why not just use -n or -z?

Previously we failed on any invalid atom. Now we fail only if all atoms were
invalid.

[zbehan, 2010-07-13 20:30:04]

On Tue, Jul 13, 2010 at 1:09 PM, <msb@chromium.org> wrote:

>
> http://codereview.chromium.org/2985003/diff/6001/7001
> File cros_workon (left):
>
> http://codereview.chromium.org/2985003/diff/6001/7001#oldcode80
> cros_workon:80: [ -n "${name}" ] || return 1
> Why get rid of the return value?
>

I was basically torn between the behaviour when one wrong atom sends the
whole thing to hell, and I ultimately chose warning for each invalid atom
(invalid for whatever reason), while the whole thing works and processes the
correct atoms. Therefore an empty return value (with a warning printed
through a different channel) becomes a perfectly valid code path. That's why
there's a check for empty ATOM_LIST at the end and this check removed.


> http://codereview.chromium.org/2985003/diff/6001/7001
> File cros_workon (right):
>
> http://codereview.chromium.org/2985003/diff/6001/7001#newcode22
> cros_workon:22: echo -e "${V_BOLD_GREEN}INFO   : $1${V_VIDOFF}" 1>&2
> Instead of duplicate implementation, why not:
>
> info "My message" 1>&2
>

This is a very good point. I haven't thought of that. I kind of believe that
from philosophical point of view, it should be common.sh to be modified,
because printing messages like this to stderr is a standard behaviour of
pretty much every elementary unix command. But for now, this will certainly
suffice.


> http://codereview.chromium.org/2985003/diff/6001/7001#newcode124
> cros_workon:124: if expr match "${ATOM_LIST}" "^ *$" &> /dev/null; then
> This is difficult for most folks to parse. Why not just use -n or -z?
>

-z [ ! -n ] tests for empty string, and does not always cope with multiple
whitespace (possibly different types of whitespace). In particular if you do
[ -z "${ATOM_LIST}" ], it will most certainly consider merely one space as a
non-empty string. You can go with [ -z ${ATOM_LIST} ], but the semantic of
that is pretty fuzzy and sometimes fails with weird messages about invalid
number of arguments, depending on the type of if check. The regexp match
accepts any amount of spaces and is easilly extensible through a common
(regexp) notation. Why do you think it's difficult to parse?


> Previously we failed on any invalid atom. Now we fail only if all atoms
> were invalid.


You think failing for any atom is a better behaviour? I suppose I can leave
that out, though I find it much better to just skip the errors. In
particular if you have a long list of ebuilds, and the process stops at the
first defect, it might take several iterations to get through the whole list
if you have more mistakes, so it would be nice if the whole thing could fail
but check the complete list anyway and print out ALL the errors, not just
the first. This is a common frustration with portage where you have to
iteratively wait to resolve the complete dependency tree and fix errors one
by one. Admittedly, the problem is of several orders of magnitude smaller
scale in this little script.

[Mandeep Singh Baines, 2010-07-13 21:37:27]

On Tue, Jul 13, 2010 at 1:29 PM, Zdenek Behan <zbehan@chromium.org> wrote:
> On Tue, Jul 13, 2010 at 1:09 PM, <msb@chromium.org> wrote:
>>
>> http://codereview.chromium.org/2985003/diff/6001/7001
>> File cros_workon (left):
>>
>> http://codereview.chromium.org/2985003/diff/6001/7001#oldcode80
>> cros_workon:80: [ -n "${name}" ] || return 1
>> Why get rid of the return value?
>
>
> I was basically torn between the behaviour when one wrong atom sends the
> whole thing to hell, and I ultimately chose warning for each invalid atom
> (invalid for whatever reason), while the whole thing works and processes the
> correct atoms. Therefore an empty return value (with a warning printed
> through a different channel) becomes a perfectly valid code path. That's why
> there's a check for empty ATOM_LIST at the end and this check removed.
>
>>
>> http://codereview.chromium.org/2985003/diff/6001/7001
>> File cros_workon (right):
>>
>> http://codereview.chromium.org/2985003/diff/6001/7001#newcode22
>> cros_workon:22: echo -e "${V_BOLD_GREEN}INFO   : $1${V_VIDOFF}" 1>&2
>> Instead of duplicate implementation, why not:
>>
>> info "My message" 1>&2
>
>
> This is a very good point. I haven't thought of that. I kind of believe that
> from philosophical point of view, it should be common.sh to be modified,
> because printing messages like this to stderr is a standard behaviour of
> pretty much every elementary unix command. But for now, this will certainly
> suffice.
>
>>
>> http://codereview.chromium.org/2985003/diff/6001/7001#newcode124
>> cros_workon:124: if expr match "${ATOM_LIST}" "^ *$" &> /dev/null; then
>> This is difficult for most folks to parse. Why not just use -n or -z?
>
> -z [ ! -n ] tests for empty string, and does not always cope with multiple
> whitespace (possibly different types of whitespace). In particular if you do
> [ -z "${ATOM_LIST}" ], it will most certainly consider merely one space as a
> non-empty string. You can go with [ -z ${ATOM_LIST} ], but the semantic of
> that is pretty fuzzy and sometimes fails with weird messages about invalid
> number of arguments, depending on the type of if check. The regexp match
> accepts any amount of spaces and is easilly extensible through a common
> (regexp) notation. Why do you think it's difficult to parse?
>
>>
>> Previously we failed on any invalid atom. Now we fail only if all atoms
>> were invalid.
>
>
> You think failing for any atom is a better behaviour? I suppose I can leave
> that out, though I find it much better to just skip the errors. In
> particular if you have a long list of ebuilds, and the process stops at the
> first defect, it might take several iterations to get through the whole list
> if you have more mistakes, so it would be nice if the whole thing could fail
> but check the complete list anyway and print out ALL the errors, not just
> the first. This is a common frustration with portage where you have to
> iteratively wait to resolve the complete dependency tree and fix errors one
> by one. Admittedly, the problem is of several orders of magnitude smaller
> scale in this little script.

Yeah, I prefer failure on bad input. If its not possible to do exactly
what the user has asked it is best to do nothing to avoid doing something
unintened.

[Mandeep Singh Baines, 2010-07-13 21:49:37]

LGTM