cros_workon: print out short summary of what has been done for start/stop

cros_workon: print out short summary of what has been done for start/stop

BUG=6325
TEST=cros_workon start and stop for board and host and see it work

Change-Id: Ic0680a273391fdf93b6ebbe0e34497807f31f240

[kliegs1, 2010-09-02 00:56:38]

A bit worried that the outputs don't depend on the command succeeding.  This
could lead to confused users.

-jon

On Sep 1, 2010 8:34 PM, <zbehan@chromium.org> wrote:
> Reviewers: anush_google, msb_google.com, David James, robotboy1,
>
> Description:
> cros_workon: print out short summary of what has been done for start/stop
>
> BUG=6325
> TEST=cros_workon start and stop for board and host and see it work
>
> Change-Id: Ic0680a273391fdf93b6ebbe0e34497807f31f240
>
> Please review this at http://codereview.chromium.org/3352002/show
>
> SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git
>
> Affected files:
> M cros_workon
>
>
> Index: cros_workon
> diff --git a/cros_workon b/cros_workon
> index
>
128933a84a540d971db8302fe5a5d4d17043592a..545ac65a1144aad384dff8c2e5141f68d1685cc7

> 100755
> --- a/cros_workon
> +++ b/cros_workon
> @@ -59,10 +59,12 @@ if [ -n "${FLAGS_board}" ]; then
> BOARD_DIR=/build/"${FLAGS_board}" # --board specified
> EQUERYCMD=equery-"${FLAGS_board}"
> EBUILDCMD=ebuild-"${FLAGS_board}"
> + BOARD_STR="${FLAGS_board}"
> else
> BOARD_DIR="" # --host specified
> EQUERYCMD=equery
> EBUILDCMD=ebuild
> + BOARD_STR="host"
> fi
>
> WORKON_DIR=${CHROOT_TRUNK_DIR}/.config/cros_workon
> @@ -181,29 +183,37 @@ regen_manifest_and_sync() {
> # src_unpack step fetches the package source for local development.
> ebuild_to_live () {
> local atoms=$1
> + local atoms_success=""
>
> for atom in ${atoms}; do
> if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
> sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
> + atoms_success="${atoms_success} ${atom}"
> else
> warn "Already working on ${atom}"
> fi
> done
> + [ -n "${atoms_success/ /}" ] && \
> + info "Started working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
> }
>
> # Move a live development ebuild back to stable.
> ebuild_to_stable () {
> local atoms=$1
> + local atoms_success=""
>
> for atom in ${atoms}; do
> if grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
> sudo bash -c "grep -v '^[~=]${atom}-9999\$' \"${WORKON_FILE}\" > \
> \"${WORKON_FILE}+\""
> sudo mv "${WORKON_FILE}+" "${WORKON_FILE}"
> + atoms_success="${atoms_success} ${atom}"
> else
> warn "Not working on ${atom}"
> fi
> done
> + [ -n "${atoms_success/ /}" ] && \
> + info "Stopped working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
> }
>
> # Run a command on all or a set of repos.
>
>

[zbehan, 2010-09-02 02:08:23]

They do.

On Wed, Sep 1, 2010 at 5:56 PM, Jonathan Kliegman <kliegs@google.com> wrote:

> A bit worried that the outputs don't depend on the command succeeding.
> This could lead to confused users.
>
> -jon
>
> On Sep 1, 2010 8:34 PM, <zbehan@chromium.org> wrote:
> > Reviewers: anush_google, msb_google.com, David James, robotboy1,
> >
> > Description:
> > cros_workon: print out short summary of what has been done for start/stop
> >
> > BUG=6325
> > TEST=cros_workon start and stop for board and host and see it work
> >
> > Change-Id: Ic0680a273391fdf93b6ebbe0e34497807f31f240
> >
> > Please review this at http://codereview.chromium.org/3352002/show
> >
> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git
> >
> > Affected files:
> > M cros_workon
> >
> >
> > Index: cros_workon
> > diff --git a/cros_workon b/cros_workon
> > index
> >
>
128933a84a540d971db8302fe5a5d4d17043592a..545ac65a1144aad384dff8c2e5141f68d1685cc7
>
> > 100755
> > --- a/cros_workon
> > +++ b/cros_workon
> > @@ -59,10 +59,12 @@ if [ -n "${FLAGS_board}" ]; then
> > BOARD_DIR=/build/"${FLAGS_board}" # --board specified
> > EQUERYCMD=equery-"${FLAGS_board}"
> > EBUILDCMD=ebuild-"${FLAGS_board}"
> > + BOARD_STR="${FLAGS_board}"
> > else
> > BOARD_DIR="" # --host specified
> > EQUERYCMD=equery
> > EBUILDCMD=ebuild
> > + BOARD_STR="host"
> > fi
> >
> > WORKON_DIR=${CHROOT_TRUNK_DIR}/.config/cros_workon
> > @@ -181,29 +183,37 @@ regen_manifest_and_sync() {
> > # src_unpack step fetches the package source for local development.
> > ebuild_to_live () {
> > local atoms=$1
> > + local atoms_success=""
> >
> > for atom in ${atoms}; do
> > if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
> > sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
> > + atoms_success="${atoms_success} ${atom}"
> > else
> > warn "Already working on ${atom}"
> > fi
> > done
> > + [ -n "${atoms_success/ /}" ] && \
> > + info "Started working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
> > }
> >
> > # Move a live development ebuild back to stable.
> > ebuild_to_stable () {
> > local atoms=$1
> > + local atoms_success=""
> >
> > for atom in ${atoms}; do
> > if grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
> > sudo bash -c "grep -v '^[~=]${atom}-9999\$' \"${WORKON_FILE}\" > \
> > \"${WORKON_FILE}+\""
> > sudo mv "${WORKON_FILE}+" "${WORKON_FILE}"
> > + atoms_success="${atoms_success} ${atom}"
> > else
> > warn "Not working on ${atom}"
> > fi
> > done
> > + [ -n "${atoms_success/ /}" ] && \
> > + info "Stopped working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
> > }
> >
> > # Run a command on all or a set of repos.
> >
> >
>

[Mandeep Singh Baines, 2010-09-02 03:25:03]

Not a fan of chatty tools.

This violates "silence is golden". If you really want to do this,
add a -v option.

zbehan@chromium.org (zbehan@chromium.org) wrote:
> Reviewers: anush_google, msb_google.com, David James, robotboy1,
> 
> Description:
> cros_workon: print out short summary of what has been done for start/stop
> 
> BUG=6325
> TEST=cros_workon start and stop for board and host and see it work
> 
> Change-Id: Ic0680a273391fdf93b6ebbe0e34497807f31f240
> 
> Please review this at http://codereview.chromium.org/3352002/show
> 
> SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git
> 
> Affected files:
>   M cros_workon
> 
> 
> Index: cros_workon
> diff --git a/cros_workon b/cros_workon
> index
128933a84a540d971db8302fe5a5d4d17043592a..545ac65a1144aad384dff8c2e5141f68d1685cc7
> 100755
> --- a/cros_workon
> +++ b/cros_workon
> @@ -59,10 +59,12 @@ if [ -n "${FLAGS_board}" ]; then
>    BOARD_DIR=/build/"${FLAGS_board}" # --board specified
>    EQUERYCMD=equery-"${FLAGS_board}"
>    EBUILDCMD=ebuild-"${FLAGS_board}"
> +  BOARD_STR="${FLAGS_board}"
>  else
>    BOARD_DIR="" # --host specified
>    EQUERYCMD=equery
>    EBUILDCMD=ebuild
> +  BOARD_STR="host"
>  fi
> 
>  WORKON_DIR=${CHROOT_TRUNK_DIR}/.config/cros_workon
> @@ -181,29 +183,37 @@ regen_manifest_and_sync() {
>  # src_unpack step fetches the package source for local development.
>  ebuild_to_live () {
>    local atoms=$1
> +  local atoms_success=""
> 
>    for atom in ${atoms}; do
>      if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>        sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
> +      atoms_success="${atoms_success} ${atom}"
>      else
>        warn "Already working on ${atom}"
>      fi
>    done
> +  [ -n "${atoms_success/ /}" ] && \
> +    info "Started working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
>  }
> 
>  # Move a live development ebuild back to stable.
>  ebuild_to_stable () {
>    local atoms=$1
> +  local atoms_success=""
> 
>    for atom in ${atoms}; do
>      if grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>        sudo bash -c "grep -v '^[~=]${atom}-9999\$' \"${WORKON_FILE}\" > \
>            \"${WORKON_FILE}+\""
>        sudo mv "${WORKON_FILE}+" "${WORKON_FILE}"
> +      atoms_success="${atoms_success} ${atom}"
>      else
>        warn "Not working on ${atom}"
>      fi
>    done
> +  [ -n "${atoms_success/ /}" ] && \
> +    info "Stopped working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
>  }
> 
>  # Run a command on all or a set of repos.
> 
> 

[zbehan, 2010-09-02 04:25:36]

The explanation is in bug 6325, and I think this is a very valid concern.
The currently quiet operation for people who didn't realise that they're
supposed pass a --board or --host, and have a default board setup, is
unpretty, confusing and user-unfriendly.

On Wed, Sep 1, 2010 at 8:24 PM, Mandeep Singh Baines <msb@chromium.org>wrote:

> Not a fan of chatty tools.
>
> This violates "silence is golden". If you really want to do this,
> add a -v option.
>
> zbehan@chromium.org (zbehan@chromium.org) wrote:
> > Reviewers: anush_google, msb_google.com, David James, robotboy1,
> >
> > Description:
> > cros_workon: print out short summary of what has been done for start/stop
> >
> > BUG=6325
> > TEST=cros_workon start and stop for board and host and see it work
> >
> > Change-Id: Ic0680a273391fdf93b6ebbe0e34497807f31f240
> >
> > Please review this at http://codereview.chromium.org/3352002/show
> >
> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git
> >
> > Affected files:
> >   M cros_workon
> >
> >
> > Index: cros_workon
> > diff --git a/cros_workon b/cros_workon
> > index
>
128933a84a540d971db8302fe5a5d4d17043592a..545ac65a1144aad384dff8c2e5141f68d1685cc7
> > 100755
> > --- a/cros_workon
> > +++ b/cros_workon
> > @@ -59,10 +59,12 @@ if [ -n "${FLAGS_board}" ]; then
> >    BOARD_DIR=/build/"${FLAGS_board}" # --board specified
> >    EQUERYCMD=equery-"${FLAGS_board}"
> >    EBUILDCMD=ebuild-"${FLAGS_board}"
> > +  BOARD_STR="${FLAGS_board}"
> >  else
> >    BOARD_DIR="" # --host specified
> >    EQUERYCMD=equery
> >    EBUILDCMD=ebuild
> > +  BOARD_STR="host"
> >  fi
> >
> >  WORKON_DIR=${CHROOT_TRUNK_DIR}/.config/cros_workon
> > @@ -181,29 +183,37 @@ regen_manifest_and_sync() {
> >  # src_unpack step fetches the package source for local development.
> >  ebuild_to_live () {
> >    local atoms=$1
> > +  local atoms_success=""
> >
> >    for atom in ${atoms}; do
> >      if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
> >        sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
> > +      atoms_success="${atoms_success} ${atom}"
> >      else
> >        warn "Already working on ${atom}"
> >      fi
> >    done
> > +  [ -n "${atoms_success/ /}" ] && \
> > +    info "Started working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
> >  }
> >
> >  # Move a live development ebuild back to stable.
> >  ebuild_to_stable () {
> >    local atoms=$1
> > +  local atoms_success=""
> >
> >    for atom in ${atoms}; do
> >      if grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
> >        sudo bash -c "grep -v '^[~=]${atom}-9999\$' \"${WORKON_FILE}\" > \
> >            \"${WORKON_FILE}+\""
> >        sudo mv "${WORKON_FILE}+" "${WORKON_FILE}"
> > +      atoms_success="${atoms_success} ${atom}"
> >      else
> >        warn "Not working on ${atom}"
> >      fi
> >    done
> > +  [ -n "${atoms_success/ /}" ] && \
> > +    info "Stopped working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
> >  }
> >
> >  # Run a command on all or a set of repos.
> >
> >
>

[kliegs1, 2010-09-02 04:45:54]

They print messages even on failure.

  for atom in ${atoms}; do
    if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
      sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
+      atoms_success="${atoms_success} ${atom}"
    else
      warn "Already working on ${atom}"
    fi
  done
+  [ -n "${atoms_success/ /}" ] && \
+    info "Started working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
 }


Whether or not the line is written into ${WORKON_FILE} it will echo as if
it succeeded.

Similarly for the other sections.

If its going to output information on success I'd like it to actually
validate that it did what was expected, not just that it parsed the inputs
properly.

On Thu, Sep 2, 2010 at 12:25 AM, Zdenek Behan <zbehan@chromium.org> wrote:

> The explanation is in bug 6325, and I think this is a very valid concern.
> The currently quiet operation for people who didn't realise that they're
> supposed pass a --board or --host, and have a default board setup, is
> unpretty, confusing and user-unfriendly.
>
>
> On Wed, Sep 1, 2010 at 8:24 PM, Mandeep Singh Baines <msb@chromium.org>wrote:
>
>> Not a fan of chatty tools.
>>
>> This violates "silence is golden". If you really want to do this,
>> add a -v option.
>>
>> zbehan@chromium.org (zbehan@chromium.org) wrote:
>> > Reviewers: anush_google, msb_google.com, David James, robotboy1,
>> >
>> > Description:
>> > cros_workon: print out short summary of what has been done for
>> start/stop
>> >
>> > BUG=6325
>> > TEST=cros_workon start and stop for board and host and see it work
>> >
>> > Change-Id: Ic0680a273391fdf93b6ebbe0e34497807f31f240
>> >
>> > Please review this at http://codereview.chromium.org/3352002/show
>> >
>> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git
>> >
>> > Affected files:
>> >   M cros_workon
>> >
>> >
>> > Index: cros_workon
>> > diff --git a/cros_workon b/cros_workon
>> > index
>>
128933a84a540d971db8302fe5a5d4d17043592a..545ac65a1144aad384dff8c2e5141f68d1685cc7
>> > 100755
>> > --- a/cros_workon
>> > +++ b/cros_workon
>> > @@ -59,10 +59,12 @@ if [ -n "${FLAGS_board}" ]; then
>> >    BOARD_DIR=/build/"${FLAGS_board}" # --board specified
>> >    EQUERYCMD=equery-"${FLAGS_board}"
>> >    EBUILDCMD=ebuild-"${FLAGS_board}"
>> > +  BOARD_STR="${FLAGS_board}"
>> >  else
>> >    BOARD_DIR="" # --host specified
>> >    EQUERYCMD=equery
>> >    EBUILDCMD=ebuild
>> > +  BOARD_STR="host"
>> >  fi
>> >
>> >  WORKON_DIR=${CHROOT_TRUNK_DIR}/.config/cros_workon
>> > @@ -181,29 +183,37 @@ regen_manifest_and_sync() {
>> >  # src_unpack step fetches the package source for local development.
>> >  ebuild_to_live () {
>> >    local atoms=$1
>> > +  local atoms_success=""
>> >
>> >    for atom in ${atoms}; do
>> >      if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>> >        sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
>> > +      atoms_success="${atoms_success} ${atom}"
>> >      else
>> >        warn "Already working on ${atom}"
>> >      fi
>> >    done
>> > +  [ -n "${atoms_success/ /}" ] && \
>> > +    info "Started working on \`${atoms_success/ /}' for
>> \`${BOARD_STR}'"
>> >  }
>> >
>> >  # Move a live development ebuild back to stable.
>> >  ebuild_to_stable () {
>> >    local atoms=$1
>> > +  local atoms_success=""
>> >
>> >    for atom in ${atoms}; do
>> >      if grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>> >        sudo bash -c "grep -v '^[~=]${atom}-9999\$' \"${WORKON_FILE}\" >
>> \
>> >            \"${WORKON_FILE}+\""
>> >        sudo mv "${WORKON_FILE}+" "${WORKON_FILE}"
>> > +      atoms_success="${atoms_success} ${atom}"
>> >      else
>> >        warn "Not working on ${atom}"
>> >      fi
>> >    done
>> > +  [ -n "${atoms_success/ /}" ] && \
>> > +    info "Stopped working on \`${atoms_success/ /}' for
>> \`${BOARD_STR}'"
>> >  }
>> >
>> >  # Run a command on all or a set of repos.
>> >
>> >
>>
>
>

[zbehan, 2010-09-02 05:06:30]

So i think one of us must be confused here. What I believe this code does
is:
- Allow partial success, ie. if all packages pass the "existence" check and
get into the quoted function, they will all be attempted workon, and the
ones that succeed (ie. are not already being worked on) will start to be
worked on.
- atoms_success variable contains the list of packages that were
successfully worked on, ie. the ones for which an appropripate entry in
WORKON_FILE was created
- If atoms_success is empty, nothing is printed

If you workon 3 packages, 1 of which is already being worked on, only the
two will be printed, if all 3 are already being worked on, nothing is
printed.
If you workon a package that does not exist and doesn't pass the basic
`equery which <package>' check, nothing at all happens.

On Wed, Sep 1, 2010 at 9:45 PM, Jonathan Kliegman <kliegs@google.com> wrote:

> They print messages even on failure.
>
>   for atom in ${atoms}; do
>     if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>       sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
> +      atoms_success="${atoms_success} ${atom}"
>     else
>       warn "Already working on ${atom}"
>     fi
>   done
> +  [ -n "${atoms_success/ /}" ] && \
> +    info "Started working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
>  }
>
>
> Whether or not the line is written into ${WORKON_FILE} it will echo as if
> it succeeded.
>
> Similarly for the other sections.
>
> If its going to output information on success I'd like it to actually
> validate that it did what was expected, not just that it parsed the inputs
> properly.
>
> On Thu, Sep 2, 2010 at 12:25 AM, Zdenek Behan <zbehan@chromium.org> wrote:
>
>> The explanation is in bug 6325, and I think this is a very valid concern.
>> The currently quiet operation for people who didn't realise that they're
>> supposed pass a --board or --host, and have a default board setup, is
>> unpretty, confusing and user-unfriendly.
>>
>>
>> On Wed, Sep 1, 2010 at 8:24 PM, Mandeep Singh Baines <msb@chromium.org>wrote:
>>
>>> Not a fan of chatty tools.
>>>
>>> This violates "silence is golden". If you really want to do this,
>>> add a -v option.
>>>
>>> zbehan@chromium.org (zbehan@chromium.org) wrote:
>>> > Reviewers: anush_google, msb_google.com, David James, robotboy1,
>>> >
>>> > Description:
>>> > cros_workon: print out short summary of what has been done for
>>> start/stop
>>> >
>>> > BUG=6325
>>> > TEST=cros_workon start and stop for board and host and see it work
>>> >
>>> > Change-Id: Ic0680a273391fdf93b6ebbe0e34497807f31f240
>>> >
>>> > Please review this at http://codereview.chromium.org/3352002/show
>>> >
>>> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git
>>> >
>>> > Affected files:
>>> >   M cros_workon
>>> >
>>> >
>>> > Index: cros_workon
>>> > diff --git a/cros_workon b/cros_workon
>>> > index
>>>
128933a84a540d971db8302fe5a5d4d17043592a..545ac65a1144aad384dff8c2e5141f68d1685cc7
>>> > 100755
>>> > --- a/cros_workon
>>> > +++ b/cros_workon
>>> > @@ -59,10 +59,12 @@ if [ -n "${FLAGS_board}" ]; then
>>> >    BOARD_DIR=/build/"${FLAGS_board}" # --board specified
>>> >    EQUERYCMD=equery-"${FLAGS_board}"
>>> >    EBUILDCMD=ebuild-"${FLAGS_board}"
>>> > +  BOARD_STR="${FLAGS_board}"
>>> >  else
>>> >    BOARD_DIR="" # --host specified
>>> >    EQUERYCMD=equery
>>> >    EBUILDCMD=ebuild
>>> > +  BOARD_STR="host"
>>> >  fi
>>> >
>>> >  WORKON_DIR=${CHROOT_TRUNK_DIR}/.config/cros_workon
>>> > @@ -181,29 +183,37 @@ regen_manifest_and_sync() {
>>> >  # src_unpack step fetches the package source for local development.
>>> >  ebuild_to_live () {
>>> >    local atoms=$1
>>> > +  local atoms_success=""
>>> >
>>> >    for atom in ${atoms}; do
>>> >      if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>>> >        sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
>>> > +      atoms_success="${atoms_success} ${atom}"
>>> >      else
>>> >        warn "Already working on ${atom}"
>>> >      fi
>>> >    done
>>> > +  [ -n "${atoms_success/ /}" ] && \
>>> > +    info "Started working on \`${atoms_success/ /}' for
>>> \`${BOARD_STR}'"
>>> >  }
>>> >
>>> >  # Move a live development ebuild back to stable.
>>> >  ebuild_to_stable () {
>>> >    local atoms=$1
>>> > +  local atoms_success=""
>>> >
>>> >    for atom in ${atoms}; do
>>> >      if grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>>> >        sudo bash -c "grep -v '^[~=]${atom}-9999\$' \"${WORKON_FILE}\" >
>>> \
>>> >            \"${WORKON_FILE}+\""
>>> >        sudo mv "${WORKON_FILE}+" "${WORKON_FILE}"
>>> > +      atoms_success="${atoms_success} ${atom}"
>>> >      else
>>> >        warn "Not working on ${atom}"
>>> >      fi
>>> >    done
>>> > +  [ -n "${atoms_success/ /}" ] && \
>>> > +    info "Stopped working on \`${atoms_success/ /}' for
>>> \`${BOARD_STR}'"
>>> >  }
>>> >
>>> >  # Run a command on all or a set of repos.
>>> >
>>> >
>>>
>>
>>
>

[kliegs1, 2010-09-02 06:36:23]

What I meant was that if the sudo command fails for whatever reason or the
"echo ... " fails that the script will still print "success".

The script is determining success based on if the atoms are possible to be
worked on, not if they are actually marked correctly.

If we're going to print success messages it should be done at the end of the
script with a confirmation.  So the script would store which atoms should be
changed, it reads out the data files and confirms the status and goes "xxx
succesfully marked as yyy" or similar.  Anything else can be confusing to an
end user unless it states "attempting to mark xxx as yyy"

-Jon

On Thu, Sep 2, 2010 at 1:06 AM, Zdenek Behan <zbehan@chromium.org> wrote:

> So i think one of us must be confused here. What I believe this code does
> is:
> - Allow partial success, ie. if all packages pass the "existence" check and
> get into the quoted function, they will all be attempted workon, and the
> ones that succeed (ie. are not already being worked on) will start to be
> worked on.
> - atoms_success variable contains the list of packages that were
> successfully worked on, ie. the ones for which an appropripate entry in
> WORKON_FILE was created
> - If atoms_success is empty, nothing is printed
>
> If you workon 3 packages, 1 of which is already being worked on, only the
> two will be printed, if all 3 are already being worked on, nothing is
> printed.
> If you workon a package that does not exist and doesn't pass the basic
> `equery which <package>' check, nothing at all happens.
>
> On Wed, Sep 1, 2010 at 9:45 PM, Jonathan Kliegman <kliegs@google.com>wrote:
>
>> They print messages even on failure.
>>
>>   for atom in ${atoms}; do
>>     if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>>       sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
>> +      atoms_success="${atoms_success} ${atom}"
>>     else
>>       warn "Already working on ${atom}"
>>     fi
>>   done
>> +  [ -n "${atoms_success/ /}" ] && \
>> +    info "Started working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
>>  }
>>
>>
>> Whether or not the line is written into ${WORKON_FILE} it will echo as if
>> it succeeded.
>>
>> Similarly for the other sections.
>>
>> If its going to output information on success I'd like it to actually
>> validate that it did what was expected, not just that it parsed the inputs
>> properly.
>>
>> On Thu, Sep 2, 2010 at 12:25 AM, Zdenek Behan <zbehan@chromium.org>wrote:
>>
>>> The explanation is in bug 6325, and I think this is a very valid
>>> concern. The currently quiet operation for people who didn't realise that
>>> they're supposed pass a --board or --host, and have a default board setup,
>>> is unpretty, confusing and user-unfriendly.
>>>
>>>
>>> On Wed, Sep 1, 2010 at 8:24 PM, Mandeep Singh Baines
<msb@chromium.org>wrote:
>>>
>>>> Not a fan of chatty tools.
>>>>
>>>> This violates "silence is golden". If you really want to do this,
>>>> add a -v option.
>>>>
>>>> zbehan@chromium.org (zbehan@chromium.org) wrote:
>>>> > Reviewers: anush_google, msb_google.com, David James, robotboy1,
>>>> >
>>>> > Description:
>>>> > cros_workon: print out short summary of what has been done for
>>>> start/stop
>>>> >
>>>> > BUG=6325
>>>> > TEST=cros_workon start and stop for board and host and see it work
>>>> >
>>>> > Change-Id: Ic0680a273391fdf93b6ebbe0e34497807f31f240
>>>> >
>>>> > Please review this at http://codereview.chromium.org/3352002/show
>>>> >
>>>> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git
>>>> >
>>>> > Affected files:
>>>> >   M cros_workon
>>>> >
>>>> >
>>>> > Index: cros_workon
>>>> > diff --git a/cros_workon b/cros_workon
>>>> > index
>>>>
128933a84a540d971db8302fe5a5d4d17043592a..545ac65a1144aad384dff8c2e5141f68d1685cc7
>>>> > 100755
>>>> > --- a/cros_workon
>>>> > +++ b/cros_workon
>>>> > @@ -59,10 +59,12 @@ if [ -n "${FLAGS_board}" ]; then
>>>> >    BOARD_DIR=/build/"${FLAGS_board}" # --board specified
>>>> >    EQUERYCMD=equery-"${FLAGS_board}"
>>>> >    EBUILDCMD=ebuild-"${FLAGS_board}"
>>>> > +  BOARD_STR="${FLAGS_board}"
>>>> >  else
>>>> >    BOARD_DIR="" # --host specified
>>>> >    EQUERYCMD=equery
>>>> >    EBUILDCMD=ebuild
>>>> > +  BOARD_STR="host"
>>>> >  fi
>>>> >
>>>> >  WORKON_DIR=${CHROOT_TRUNK_DIR}/.config/cros_workon
>>>> > @@ -181,29 +183,37 @@ regen_manifest_and_sync() {
>>>> >  # src_unpack step fetches the package source for local development.
>>>> >  ebuild_to_live () {
>>>> >    local atoms=$1
>>>> > +  local atoms_success=""
>>>> >
>>>> >    for atom in ${atoms}; do
>>>> >      if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>>>> >        sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
>>>> > +      atoms_success="${atoms_success} ${atom}"
>>>> >      else
>>>> >        warn "Already working on ${atom}"
>>>> >      fi
>>>> >    done
>>>> > +  [ -n "${atoms_success/ /}" ] && \
>>>> > +    info "Started working on \`${atoms_success/ /}' for
>>>> \`${BOARD_STR}'"
>>>> >  }
>>>> >
>>>> >  # Move a live development ebuild back to stable.
>>>> >  ebuild_to_stable () {
>>>> >    local atoms=$1
>>>> > +  local atoms_success=""
>>>> >
>>>> >    for atom in ${atoms}; do
>>>> >      if grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>>>> >        sudo bash -c "grep -v '^[~=]${atom}-9999\$' \"${WORKON_FILE}\"
>>>> > \
>>>> >            \"${WORKON_FILE}+\""
>>>> >        sudo mv "${WORKON_FILE}+" "${WORKON_FILE}"
>>>> > +      atoms_success="${atoms_success} ${atom}"
>>>> >      else
>>>> >        warn "Not working on ${atom}"
>>>> >      fi
>>>> >    done
>>>> > +  [ -n "${atoms_success/ /}" ] && \
>>>> > +    info "Stopped working on \`${atoms_success/ /}' for
>>>> \`${BOARD_STR}'"
>>>> >  }
>>>> >
>>>> >  # Run a command on all or a set of repos.
>>>> >
>>>> >
>>>>
>>>
>>>
>>
>

[zbehan, 2010-09-02 06:53:05]

I see, that makes sense.

Would it help if i changed that into
 sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\"" && \
      atoms_success="${atoms_success} ${atom}"
?

On Wed, Sep 1, 2010 at 11:35 PM, Jonathan Kliegman <kliegs@google.com>wrote:

> What I meant was that if the sudo command fails for whatever reason or the
> "echo ... " fails that the script will still print "success".
>
> The script is determining success based on if the atoms are possible to be
> worked on, not if they are actually marked correctly.
>
> If we're going to print success messages it should be done at the end of
> the script with a confirmation.  So the script would store which atoms
> should be changed, it reads out the data files and confirms the status and
> goes "xxx succesfully marked as yyy" or similar.  Anything else can be
> confusing to an end user unless it states "attempting to mark xxx as yyy"
>
> -Jon
>
>
> On Thu, Sep 2, 2010 at 1:06 AM, Zdenek Behan <zbehan@chromium.org> wrote:
>
>> So i think one of us must be confused here. What I believe this code does
>> is:
>> - Allow partial success, ie. if all packages pass the "existence" check
>> and get into the quoted function, they will all be attempted workon, and the
>> ones that succeed (ie. are not already being worked on) will start to be
>> worked on.
>> - atoms_success variable contains the list of packages that were
>> successfully worked on, ie. the ones for which an appropripate entry in
>> WORKON_FILE was created
>> - If atoms_success is empty, nothing is printed
>>
>> If you workon 3 packages, 1 of which is already being worked on, only the
>> two will be printed, if all 3 are already being worked on, nothing is
>> printed.
>> If you workon a package that does not exist and doesn't pass the basic
>> `equery which <package>' check, nothing at all happens.
>>
>> On Wed, Sep 1, 2010 at 9:45 PM, Jonathan Kliegman <kliegs@google.com>wrote:
>>
>>> They print messages even on failure.
>>>
>>>   for atom in ${atoms}; do
>>>     if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>>>       sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
>>> +      atoms_success="${atoms_success} ${atom}"
>>>     else
>>>       warn "Already working on ${atom}"
>>>     fi
>>>   done
>>> +  [ -n "${atoms_success/ /}" ] && \
>>> +    info "Started working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
>>>  }
>>>
>>>
>>> Whether or not the line is written into ${WORKON_FILE} it will echo as if
>>> it succeeded.
>>>
>>> Similarly for the other sections.
>>>
>>> If its going to output information on success I'd like it to actually
>>> validate that it did what was expected, not just that it parsed the inputs
>>> properly.
>>>
>>> On Thu, Sep 2, 2010 at 12:25 AM, Zdenek Behan <zbehan@chromium.org>wrote:
>>>
>>>> The explanation is in bug 6325, and I think this is a very valid
>>>> concern. The currently quiet operation for people who didn't realise that
>>>> they're supposed pass a --board or --host, and have a default board setup,
>>>> is unpretty, confusing and user-unfriendly.
>>>>
>>>>
>>>> On Wed, Sep 1, 2010 at 8:24 PM, Mandeep Singh Baines
<msb@chromium.org>wrote:
>>>>
>>>>> Not a fan of chatty tools.
>>>>>
>>>>> This violates "silence is golden". If you really want to do this,
>>>>> add a -v option.
>>>>>
>>>>> zbehan@chromium.org (zbehan@chromium.org) wrote:
>>>>> > Reviewers: anush_google, msb_google.com, David James, robotboy1,
>>>>> >
>>>>> > Description:
>>>>> > cros_workon: print out short summary of what has been done for
>>>>> start/stop
>>>>> >
>>>>> > BUG=6325
>>>>> > TEST=cros_workon start and stop for board and host and see it work
>>>>> >
>>>>> > Change-Id: Ic0680a273391fdf93b6ebbe0e34497807f31f240
>>>>> >
>>>>> > Please review this at http://codereview.chromium.org/3352002/show
>>>>> >
>>>>> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git
>>>>> >
>>>>> > Affected files:
>>>>> >   M cros_workon
>>>>> >
>>>>> >
>>>>> > Index: cros_workon
>>>>> > diff --git a/cros_workon b/cros_workon
>>>>> > index
>>>>>
128933a84a540d971db8302fe5a5d4d17043592a..545ac65a1144aad384dff8c2e5141f68d1685cc7
>>>>> > 100755
>>>>> > --- a/cros_workon
>>>>> > +++ b/cros_workon
>>>>> > @@ -59,10 +59,12 @@ if [ -n "${FLAGS_board}" ]; then
>>>>> >    BOARD_DIR=/build/"${FLAGS_board}" # --board specified
>>>>> >    EQUERYCMD=equery-"${FLAGS_board}"
>>>>> >    EBUILDCMD=ebuild-"${FLAGS_board}"
>>>>> > +  BOARD_STR="${FLAGS_board}"
>>>>> >  else
>>>>> >    BOARD_DIR="" # --host specified
>>>>> >    EQUERYCMD=equery
>>>>> >    EBUILDCMD=ebuild
>>>>> > +  BOARD_STR="host"
>>>>> >  fi
>>>>> >
>>>>> >  WORKON_DIR=${CHROOT_TRUNK_DIR}/.config/cros_workon
>>>>> > @@ -181,29 +183,37 @@ regen_manifest_and_sync() {
>>>>> >  # src_unpack step fetches the package source for local development.
>>>>> >  ebuild_to_live () {
>>>>> >    local atoms=$1
>>>>> > +  local atoms_success=""
>>>>> >
>>>>> >    for atom in ${atoms}; do
>>>>> >      if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>>>>> >        sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
>>>>> > +      atoms_success="${atoms_success} ${atom}"
>>>>> >      else
>>>>> >        warn "Already working on ${atom}"
>>>>> >      fi
>>>>> >    done
>>>>> > +  [ -n "${atoms_success/ /}" ] && \
>>>>> > +    info "Started working on \`${atoms_success/ /}' for
>>>>> \`${BOARD_STR}'"
>>>>> >  }
>>>>> >
>>>>> >  # Move a live development ebuild back to stable.
>>>>> >  ebuild_to_stable () {
>>>>> >    local atoms=$1
>>>>> > +  local atoms_success=""
>>>>> >
>>>>> >    for atom in ${atoms}; do
>>>>> >      if grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
>>>>> >        sudo bash -c "grep -v '^[~=]${atom}-9999\$' \"${WORKON_FILE}\"
>>>>> > \
>>>>> >            \"${WORKON_FILE}+\""
>>>>> >        sudo mv "${WORKON_FILE}+" "${WORKON_FILE}"
>>>>> > +      atoms_success="${atoms_success} ${atom}"
>>>>> >      else
>>>>> >        warn "Not working on ${atom}"
>>>>> >      fi
>>>>> >    done
>>>>> > +  [ -n "${atoms_success/ /}" ] && \
>>>>> > +    info "Stopped working on \`${atoms_success/ /}' for
>>>>> \`${BOARD_STR}'"
>>>>> >  }
>>>>> >
>>>>> >  # Run a command on all or a set of repos.
>>>>> >
>>>>> >
>>>>>
>>>>
>>>>
>>>
>>
>

[Mandeep Singh Baines, 2010-09-02 17:21:30]

Zdenek Behan (zbehan@chromium.org) wrote:
> The explanation is in bug 6325, and I think this is a very valid concern.
> The currently quiet operation for people who didn't realise that they're
> supposed pass a --board or --host, and have a default board setup, is

Yeah, if we didn't support a default-board there wouldn't be all this
confusion. In hindsight, default board support was a bad idea. Too late
to undo that so I guess this CL is necessary. I'm a little worried that
this once simple script is now inching closer to 300 lines.

> unpretty, confusing and user-unfriendly.
> 
> On Wed, Sep 1, 2010 at 8:24 PM, Mandeep Singh Baines <msb@chromium.org>wrote:
> 
> > Not a fan of chatty tools.
> >
> > This violates "silence is golden". If you really want to do this,
> > add a -v option.
> >
> > zbehan@chromium.org (zbehan@chromium.org) wrote:
> > > Reviewers: anush_google, msb_google.com, David James, robotboy1,
> > >
> > > Description:
> > > cros_workon: print out short summary of what has been done for start/stop
> > >
> > > BUG=6325
> > > TEST=cros_workon start and stop for board and host and see it work
> > >
> > > Change-Id: Ic0680a273391fdf93b6ebbe0e34497807f31f240
> > >
> > > Please review this at http://codereview.chromium.org/3352002/show
> > >
> > > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git
> > >
> > > Affected files:
> > >   M cros_workon
> > >
> > >
> > > Index: cros_workon
> > > diff --git a/cros_workon b/cros_workon
> > > index
> >
128933a84a540d971db8302fe5a5d4d17043592a..545ac65a1144aad384dff8c2e5141f68d1685cc7
> > > 100755
> > > --- a/cros_workon
> > > +++ b/cros_workon
> > > @@ -59,10 +59,12 @@ if [ -n "${FLAGS_board}" ]; then
> > >    BOARD_DIR=/build/"${FLAGS_board}" # --board specified
> > >    EQUERYCMD=equery-"${FLAGS_board}"
> > >    EBUILDCMD=ebuild-"${FLAGS_board}"
> > > +  BOARD_STR="${FLAGS_board}"
> > >  else
> > >    BOARD_DIR="" # --host specified
> > >    EQUERYCMD=equery
> > >    EBUILDCMD=ebuild
> > > +  BOARD_STR="host"
> > >  fi
> > >
> > >  WORKON_DIR=${CHROOT_TRUNK_DIR}/.config/cros_workon
> > > @@ -181,29 +183,37 @@ regen_manifest_and_sync() {
> > >  # src_unpack step fetches the package source for local development.
> > >  ebuild_to_live () {
> > >    local atoms=$1
> > > +  local atoms_success=""
> > >
> > >    for atom in ${atoms}; do
> > >      if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
> > >        sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
> > > +      atoms_success="${atoms_success} ${atom}"
> > >      else
> > >        warn "Already working on ${atom}"
> > >      fi
> > >    done
> > > +  [ -n "${atoms_success/ /}" ] && \
> > > +    info "Started working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
> > >  }
> > >
> > >  # Move a live development ebuild back to stable.
> > >  ebuild_to_stable () {
> > >    local atoms=$1
> > > +  local atoms_success=""
> > >
> > >    for atom in ${atoms}; do
> > >      if grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
> > >        sudo bash -c "grep -v '^[~=]${atom}-9999\$' \"${WORKON_FILE}\" > \
> > >            \"${WORKON_FILE}+\""
> > >        sudo mv "${WORKON_FILE}+" "${WORKON_FILE}"
> > > +      atoms_success="${atoms_success} ${atom}"
> > >      else
> > >        warn "Not working on ${atom}"
> > >      fi
> > >    done
> > > +  [ -n "${atoms_success/ /}" ] && \
> > > +    info "Stopped working on \`${atoms_success/ /}' for \`${BOARD_STR}'"
> > >  }
> > >
> > >  # Run a command on all or a set of repos.
> > >
> > >
> >

[zbehan, 2010-09-02 17:35:50]

I think you should set your bar of reasonableness to 400 lines. You can only
write crappy scripts that don't test its inputs properly and don't handle
all corner cases properly in under 300. ;)

On Thu, Sep 2, 2010 at 10:21 AM, Mandeep Singh Baines <msb@chromium.org>wrote:

> Zdenek Behan (zbehan@chromium.org) wrote:
> > The explanation is in bug 6325, and I think this is a very valid concern.
> > The currently quiet operation for people who didn't realise that they're
> > supposed pass a --board or --host, and have a default board setup, is
>
> Yeah, if we didn't support a default-board there wouldn't be all this
> confusion. In hindsight, default board support was a bad idea. Too late
> to undo that so I guess this CL is necessary. I'm a little worried that
> this once simple script is now inching closer to 300 lines.
>
> > unpretty, confusing and user-unfriendly.
> >
> > On Wed, Sep 1, 2010 at 8:24 PM, Mandeep Singh Baines <msb@chromium.org
> >wrote:
> >
> > > Not a fan of chatty tools.
> > >
> > > This violates "silence is golden". If you really want to do this,
> > > add a -v option.
> > >
> > > zbehan@chromium.org (zbehan@chromium.org) wrote:
> > > > Reviewers: anush_google, msb_google.com, David James, robotboy1,
> > > >
> > > > Description:
> > > > cros_workon: print out short summary of what has been done for
> start/stop
> > > >
> > > > BUG=6325
> > > > TEST=cros_workon start and stop for board and host and see it work
> > > >
> > > > Change-Id: Ic0680a273391fdf93b6ebbe0e34497807f31f240
> > > >
> > > > Please review this at http://codereview.chromium.org/3352002/show
> > > >
> > > > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git
> > > >
> > > > Affected files:
> > > >   M cros_workon
> > > >
> > > >
> > > > Index: cros_workon
> > > > diff --git a/cros_workon b/cros_workon
> > > > index
> > >
>
128933a84a540d971db8302fe5a5d4d17043592a..545ac65a1144aad384dff8c2e5141f68d1685cc7
> > > > 100755
> > > > --- a/cros_workon
> > > > +++ b/cros_workon
> > > > @@ -59,10 +59,12 @@ if [ -n "${FLAGS_board}" ]; then
> > > >    BOARD_DIR=/build/"${FLAGS_board}" # --board specified
> > > >    EQUERYCMD=equery-"${FLAGS_board}"
> > > >    EBUILDCMD=ebuild-"${FLAGS_board}"
> > > > +  BOARD_STR="${FLAGS_board}"
> > > >  else
> > > >    BOARD_DIR="" # --host specified
> > > >    EQUERYCMD=equery
> > > >    EBUILDCMD=ebuild
> > > > +  BOARD_STR="host"
> > > >  fi
> > > >
> > > >  WORKON_DIR=${CHROOT_TRUNK_DIR}/.config/cros_workon
> > > > @@ -181,29 +183,37 @@ regen_manifest_and_sync() {
> > > >  # src_unpack step fetches the package source for local development.
> > > >  ebuild_to_live () {
> > > >    local atoms=$1
> > > > +  local atoms_success=""
> > > >
> > > >    for atom in ${atoms}; do
> > > >      if ! grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
> > > >        sudo bash -c "echo \"=${atom}-9999\" >> \"${WORKON_FILE}\""
> > > > +      atoms_success="${atoms_success} ${atom}"
> > > >      else
> > > >        warn "Already working on ${atom}"
> > > >      fi
> > > >    done
> > > > +  [ -n "${atoms_success/ /}" ] && \
> > > > +    info "Started working on \`${atoms_success/ /}' for
> \`${BOARD_STR}'"
> > > >  }
> > > >
> > > >  # Move a live development ebuild back to stable.
> > > >  ebuild_to_stable () {
> > > >    local atoms=$1
> > > > +  local atoms_success=""
> > > >
> > > >    for atom in ${atoms}; do
> > > >      if grep -qx "[~=]${atom}-9999" "${WORKON_FILE}" ; then
> > > >        sudo bash -c "grep -v '^[~=]${atom}-9999\$' \"${WORKON_FILE}\"
> > \
> > > >            \"${WORKON_FILE}+\""
> > > >        sudo mv "${WORKON_FILE}+" "${WORKON_FILE}"
> > > > +      atoms_success="${atoms_success} ${atom}"
> > > >      else
> > > >        warn "Not working on ${atom}"
> > > >      fi
> > > >    done
> > > > +  [ -n "${atoms_success/ /}" ] && \
> > > > +    info "Stopped working on \`${atoms_success/ /}' for
> \`${BOARD_STR}'"
> > > >  }
> > > >
> > > >  # Run a command on all or a set of repos.
> > > >
> > > >
> > >
>

[Mandeep Singh Baines, 2010-09-02 23:57:18]

LGTM

[kliegs, 2010-09-03 15:14:44]

The $? response on grep -v behaves differently than without -v
Grep returns 0 if it find any lines to output, 1 if it does.
So testing if a line is successfully filtered with grep using the return code
isn't effective.

Your best bet is to do the copy and then check the output file for existence. 
As well check if the grep command itself worked (i.e. confirm no error on
writing the output).

[zbehan, 2010-09-03 16:50:10]

The point is, we're testing for error coming out of grep, not whether it
found anything, hence testing against -lt 2.

On Fri, Sep 3, 2010 at 8:14 AM, <kliegs@chromium.org> wrote:

> The $? response on grep -v behaves differently than without -v
> Grep returns 0 if it find any lines to output, 1 if it does.
> So testing if a line is successfully filtered with grep using the return
> code
> isn't effective.
>
> Your best bet is to do the copy and then check the output file for
> existence.
> As well check if the grep command itself worked (i.e. confirm no error on
> writing the output).
>
>
>
>
> http://codereview.chromium.org/3352002/show
>

[kliegs, 2010-09-03 17:13:48]

kliegs@kliegs-chrome:~/foo/greps$ sudo bash -c "grep -v 'd' bar >
/fdsa/asdf"
bash: /fdsa/asdf: No such file or directory
kliegs@kliegs-chrome:~/foo/greps$ echo $?
1

This also gives a value less than 2 but didn't succeed.

I do apologize for being a pest on this but the new workflow is already a
bit unclear for many (well - me at least).  So adding messages intended to
help but which could confuse the users (especially if they did something
wrong) could lead to a lot of waste time and clobbered commits.

-Jon

On Fri, Sep 3, 2010 at 12:49 PM, Zdenek Behan <zbehan@chromium.org> wrote:

> The point is, we're testing for error coming out of grep, not whether it
> found anything, hence testing against -lt 2.
>
>
> On Fri, Sep 3, 2010 at 8:14 AM, <kliegs@chromium.org> wrote:
>
>> The $? response on grep -v behaves differently than without -v
>> Grep returns 0 if it find any lines to output, 1 if it does.
>> So testing if a line is successfully filtered with grep using the return
>> code
>> isn't effective.
>>
>> Your best bet is to do the copy and then check the output file for
>> existence.
>> As well check if the grep command itself worked (i.e. confirm no error on
>> writing the output).
>>
>>
>>
>>
>> http://codereview.chromium.org/3352002/show
>>
>
>

[zbehan, 2010-09-03 21:00:01]

Jonathan, Try this.

On Fri, Sep 3, 2010 at 1:59 PM, <zbehan@chromium.org> wrote:

> http://codereview.chromium.org/3352002/show
>

[kliegs, 2010-09-03 22:33:18]

My sed is a bit weak and its late so not sure about the changes from grep to
sed.

I like the addition of the warning lines, however.  Would want to look at
this a bit more next work day to make sure I parsed SED correctly - too late
on a Friday for me to feel good about reviewing properly.

On Fri, Sep 3, 2010 at 4:59 PM, Zdenek Behan <zbehan@chromium.org> wrote:

> Jonathan, Try this.
>
>
> On Fri, Sep 3, 2010 at 1:59 PM, <zbehan@chromium.org> wrote:
>
>> http://codereview.chromium.org/3352002/show
>>
>
>

[Mandeep Singh Baines, 2010-09-03 23:14:56]

LGTM

[Mandeep Singh Baines, 2010-09-03 23:17:38]

I think we should get this in now. If there are follow-up comments, they can be
addressed in a follow-up CL. A lot of folks have been asking for this. It'll
really help with usability.

[kliegs, 2010-09-04 00:20:49]

LGTM with nits/caveats that can be resolved in separate CL's

This fixes the main concerns I have so signing off.  I'm just not positive the
sed commands work (inability to test/verify - they look fine I'm just not
confident in my ability to identify a problem with them tonight).

The nit I have is that regen_manifest_and_sync doesn't handle errors properly. 
zbehan is opening a separate bug to address that issue.