Make removing the last ] more full-proof

Make removing the last ] more full-proof



Make removal of last ] more full-proof

Change-Id: I002e5ed513066a214fd6353f66e05bf5458f6ad4

BUG=7115
TEST= No file, with --subfolder
  No file, without --subfolder
  Empty file
  File with one config already in it, with --subfolder
  File with one config and garbage at the end, with --subfolder
  File with one config and newlines at the end, with --subfolder
  File with one config, without subfolder

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=43c2280

[Hung-Te, 2010-11-10 05:10:27]

After taking an overview of this CL, I think the intension of the code is to
"remove every empty / unknown lines, and to close (or start a new one) config".

Current while-loops seems not very easy to understand its purpose at a first
glance, so maybe rewriting that by two-phase is better.

What I mean is,two blocks of code that looks like:

# remove trailing blank or garbage lines until a closing "]"
while [ -s ...miniomaha.conf ]; do
  last_line="$(last -1 ...miniomaha.conf)"
  if [ -z "$last_line" -o "$last_line" != "]" ]; then
    sed -i '$d' ...miniomaha.conf
  fi
done
# create or re-open the configuration by removing ']'
if [ "$(last -1 ...miniomaha.conf)" = "]" ]; then
 sed -i '$d' ...
else
 echo "config = [" > ...miniomaha.conf
fi

http://codereview.chromium.org/4688004/diff/4001/make_factory_package.sh
File make_factory_package.sh (right):

http://codereview.chromium.org/4688004/diff/4001/make_factory_package.sh#newc...
make_factory_package.sh:169: while [ true ]
you don't need the []. Simply a "while true; do" works for you.

http://codereview.chromium.org/4688004/diff/4001/make_factory_package.sh#newc...
make_factory_package.sh:172: if [ -z `tail -1 ${OMAHA_DIR}/miniomaha.conf` ];
Please use -z "$(tail -l "${OMAHA_DIR}/miniomaha.conf")".
`` should be replaced by $() as in coding style guide.

Also the "if" block should be "if [ ... ]; then".

http://codereview.chromium.org/4688004/diff/4001/make_factory_package.sh#newc...
make_factory_package.sh:175: mv /tmp/tmp.txt ${OMAHA_DIR}/miniomaha.conf
Please replace this by
sed -i '$d' "${OMAHA_DIR}/miniomaha.conf"

http://codereview.chromium.org/4688004/diff/4001/make_factory_package.sh#newc...
make_factory_package.sh:180: then
"if []; then" is better.

http://codereview.chromium.org/4688004/diff/4001/make_factory_package.sh#newc...
make_factory_package.sh:182: mv /tmp/tmp.txt ${OMAHA_DIR}/miniomaha.conf
Please replace sed+mv with "sed -i"

http://codereview.chromium.org/4688004/diff/4001/make_factory_package.sh#newc...
make_factory_package.sh:186: mv /tmp/tmp.txt ${OMAHA_DIR}/miniomaha.conf
Please replace sed+mv by "sed -i"

http://codereview.chromium.org/4688004/diff/4001/make_factory_package.sh#newc...
make_factory_package.sh:193: echo -e "config = [" > ${OMAHA_DIR}/miniomaha.conf
You don't need the "-e" since you don't have escape sequence in your parameter

[Hung-Te, 2010-11-10 05:13:47]

sorry, there should be a "else; break" in my psuedo code,after "if [ -z
$lastline ... sed -i ..." before fi.

well, that's just for demonstration :)

[krisr, 2010-11-10 07:40:50]

PTAL

On Tue, Nov 9, 2010 at 9:13 PM, <hungte@chromium.org> wrote:

> sorry, there should be a "else; break" in my psuedo code,after "if [ -z
> $lastline ... sed -i ..." before fi.
>
> well, that's just for demonstration :)
>
>
> http://codereview.chromium.org/4688004/
>

[Hung-Te, 2010-11-10 08:41:09]

Thanks for the modification! Please also fill the TEST= field with how you
tested this (I suppose there should be case of : (1) empty / non-exist
miniomaha.conf (2) existing miniomaha.conf with ']', and blank lines / garbage
after the ']'.

http://codereview.chromium.org/4688004/diff/3002/make_factory_package.sh
File make_factory_package.sh (right):

http://codereview.chromium.org/4688004/diff/3002/make_factory_package.sh#newc...
make_factory_package.sh:169: while true
If the checking condition is always true here, I think an empty file will fall
into endless loop by the check if [ -z "..." ].

Would you mind to check and verify that?

A "while [ -s "${OMAHA_DIR}/miniomaha.conf" ]; do " may be better

http://codereview.chromium.org/4688004/diff/3002/make_factory_package.sh#newc...
make_factory_package.sh:173: sed -i '$d' ${OMAHA_DIR}/miniomaha.conf
Please quote the file as "${OMAHA_DIR}/miniomana.conf" when you want to use
${OMAHA_DIR}, because you don't know if people will get a space in the folder
name OMAHA_DIR.

This also applies to all reference to ${OMAHA_DIR}/miniomaha.conf below.

http://codereview.chromium.org/4688004/diff/3002/make_factory_package.sh#newc...
make_factory_package.sh:174: else
This else can be merged with next if as "elif", which makes the logic more
clear.

http://codereview.chromium.org/4688004/diff/3002/make_factory_package.sh#newc...
make_factory_package.sh:177: if [ `tail -1 ${OMAHA_DIR}/miniomaha.conf` != ']'
]; then
Please change `tail ... conf` with $(tail ..."${OMAHA....conf")

http://codereview.chromium.org/4688004/diff/3002/make_factory_package.sh#newc...
make_factory_package.sh:186: if [ `tail -1 ${OMAHA_DIR}/miniomaha.conf` == ']'
]; then
== is bash special syntax and not POSIX compatible. Please use '=' instead.

Also please use $(tail ...) instead of `tail ... conf`

[krisr, 2010-11-11 20:24:44]

PTAL

How do I update the primary description for the TEST= ?

On Wed, Nov 10, 2010 at 12:41 AM, <hungte@chromium.org> wrote:

> Thanks for the modification! Please also fill the TEST= field with how you
> tested this (I suppose there should be case of : (1) empty / non-exist
> miniomaha.conf (2) existing miniomaha.conf with ']', and blank lines /
> garbage
> after the ']'.
>
>
> http://codereview.chromium.org/4688004/diff/3002/make_factory_package.sh
>
> File make_factory_package.sh (right):
>
>
>
http://codereview.chromium.org/4688004/diff/3002/make_factory_package.sh#newc...
>
> make_factory_package.sh:169: while true
> If the checking condition is always true here, I think an empty file
> will fall into endless loop by the check if [ -z "..." ].
>
> Would you mind to check and verify that?
>
> A "while [ -s "${OMAHA_DIR}/miniomaha.conf" ]; do " may be better
>
>
>
http://codereview.chromium.org/4688004/diff/3002/make_factory_package.sh#newc...
> make_factory_package.sh:173: sed -i '$d' ${OMAHA_DIR}/miniomaha.conf
> Please quote the file as "${OMAHA_DIR}/miniomana.conf" when you want to
> use ${OMAHA_DIR}, because you don't know if people will get a space in
> the folder name OMAHA_DIR.
>
> This also applies to all reference to ${OMAHA_DIR}/miniomaha.conf below.
>
>
>
http://codereview.chromium.org/4688004/diff/3002/make_factory_package.sh#newc...
> make_factory_package.sh:174: else
> This else can be merged with next if as "elif", which makes the logic
> more clear.
>
>
>
http://codereview.chromium.org/4688004/diff/3002/make_factory_package.sh#newc...
> make_factory_package.sh:177: if [ `tail -1 ${OMAHA_DIR}/miniomaha.conf`
> != ']' ]; then
> Please change `tail ... conf` with $(tail ..."${OMAHA....conf")
>
>
>
http://codereview.chromium.org/4688004/diff/3002/make_factory_package.sh#newc...
> make_factory_package.sh:186: if [ `tail -1 ${OMAHA_DIR}/miniomaha.conf`
> == ']' ]; then
> == is bash special syntax and not POSIX compatible. Please use '='
> instead.
>
> Also please use $(tail ...) instead of `tail ... conf`
>
>
> http://codereview.chromium.org/4688004/
>

[Hung-Te, 2010-11-12 00:27:09]

You can click the "Edit issue" on code review site to change the TEST= field.

I think the code looks pretty fine now. LGTM.

http://codereview.chromium.org/4688004/diff/16001/make_factory_package.sh
File make_factory_package.sh (right):

http://codereview.chromium.org/4688004/diff/16001/make_factory_package.sh#new...
make_factory_package.sh:181: if [ "$(tail -1 "${OMAHA_DIR}/miniomaha.conf")" '='
']' ]; then
You don't need the single quote for the =

[Nick Sanders, 2010-11-12 07:41:43]

lgtm