Attention is currently required from: Martin Roth, Piotr Król. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59169 )
Change subject: utils: Add initial version of "remove_unused_code" script ......................................................................
Patch Set 1:
(28 comments)
Patchset:
PS1:
Raul, When you have the time, could you do a review of the bash?
Sure :)
File util/scripts/rm_unused_code:
https://review.coreboot.org/c/coreboot/+/59169/comment/1cce47bc_7cef8db8 PS1, Line 35: show_version() { : echo : _echo_color "${BOLD}${GREEN}" "${PROGNAME} version ${VERSION}" 1 : echo : } nit: I would move this under _echo_color
https://review.coreboot.org/c/coreboot/+/59169/comment/4e50b860_4967b6be PS1, Line 41: _echo_color Can you add a comment describing the args.
https://review.coreboot.org/c/coreboot/+/59169/comment/331db0f0_27dcf2ca PS1, Line 50: test -n "${VERBOSE}" You have `-e set above. Won't this cause the script to bail if this is evaluates to false? I think you need to use an `if [[ -n "${VERBOSE}" ]];
https://review.coreboot.org/c/coreboot/+/59169/comment/3ea9f649_701938db PS1, Line 55: ( Why the subshell?
https://review.coreboot.org/c/coreboot/+/59169/comment/6f1d17be_45b97c3f PS1, Line 55: >&2 Shouldn't this go at the end?
https://review.coreboot.org/c/coreboot/+/59169/comment/97fd0737_6f88dafb PS1, Line 59: $(getopt -l version,help,debug,nocolor,blddir: -o b:DhV -- "$@") I never remember, do we need to quote this?
https://review.coreboot.org/c/coreboot/+/59169/comment/413e4f00_3cf75c97 PS1, Line 60: getopt_ret=$? Since you have -e, I think the script will bail when the line above fails.
https://review.coreboot.org/c/coreboot/+/59169/comment/7991fa75_af1152a2 PS1, Line 72: BLD_DIR Do you need to `shift` after pulling the arg off?
https://review.coreboot.org/c/coreboot/+/59169/comment/a04f1e3a_9e6ec9ce PS1, Line 115: ${dir} quote
https://review.coreboot.org/c/coreboot/+/59169/comment/06479c7e_cc999de8 PS1, Line 120: find "${dir}" -depth -type d -exec /bin/bash -c \ : 'if echo "$1" | grep -q "$2"; then exit; fi && : if [[ "$(cd "$1" && find . -maxdepth 1 | grep -v "./Makefile")" = "." ]]; then : rm -rf "$1"; fi' shell {} \ : ${REQUIRED_MAKEFILES} ; Could you invert this?
while read -r FILE; do ... Do processing on each file done < <(find "${dir}" -depth -type d)
https://review.coreboot.org/c/coreboot/+/59169/comment/dce6629f_f416daf2 PS1, Line 125: ${dir} ""
https://review.coreboot.org/c/coreboot/+/59169/comment/8980931b_4d771ca0 PS1, Line 189: { You could replace the {} these with () to make the function invoke the subshell.
https://review.coreboot.org/c/coreboot/+/59169/comment/5ba92ec7_93fd3fc1 PS1, Line 196: # find "${BLD_DIR}" -type f -exec touch -a --date="2020-01-01 00:00:00" {} ; ?
https://review.coreboot.org/c/coreboot/+/59169/comment/5c237e9c_ecf0f87b PS1, Line 205: KEEP_FILES Can you declare KEEP_FILES as an array above,then use "${KEEP_FILES[@]}"
https://review.coreboot.org/c/coreboot/+/59169/comment/76c0bfac_58ba65c0 PS1, Line 206: find Why do we need the find? Can this be replaced with `touch "${BLD_DIR}/${file}"`?
https://review.coreboot.org/c/coreboot/+/59169/comment/bd663892_919836f1 PS1, Line 211: $1 ""
https://review.coreboot.org/c/coreboot/+/59169/comment/1935f8cf_d251c6d7 PS1, Line 214: ${CONFIG_FILE} ""
https://review.coreboot.org/c/coreboot/+/59169/comment/604290d3_5408b95a PS1, Line 221: || exit 1 Your `-e` should take care this, but it doesn't hurt.
https://review.coreboot.org/c/coreboot/+/59169/comment/fb59d18d_ce9f8f3e PS1, Line 228: I think you can add a `-path ./.git -prune` to the find command and avoid the grep.
https://review.coreboot.org/c/coreboot/+/59169/comment/2fb51a67_41177700 PS1, Line 228: MODIFIED_FILES Could use `readarray MODIFIED_FILES < <(find ...)`
https://review.coreboot.org/c/coreboot/+/59169/comment/8a6350e6_fdf2700e PS1, Line 229: (echo "${MODIFIED_FILES}" | wc -l) If you use an array you can use "${#MODIFIED_FILES[@]}"
https://review.coreboot.org/c/coreboot/+/59169/comment/0651a1f3_83b0f3e0 PS1, Line 236: -exec rm {} ; -delete
https://review.coreboot.org/c/coreboot/+/59169/comment/d4f96c21_48dfeeb3 PS1, Line 249: CLEAN_DIR_LIST I would make it an array: "${CLEAN_DIR_LIST[@]}"
https://review.coreboot.org/c/coreboot/+/59169/comment/17fc0576_a74cbbb2 PS1, Line 252: -exec rm {} ; -delete
https://review.coreboot.org/c/coreboot/+/59169/comment/86f8e177_56588ae7 PS1, Line 264: && Shouldn't need the && with `-e`
https://review.coreboot.org/c/coreboot/+/59169/comment/f69219c2_36335847 PS1, Line 279: ${HASH} ""
https://review.coreboot.org/c/coreboot/+/59169/comment/eea9553d_7c890aa8 PS1, Line 285: ${HASH} ""
-
Raul Rangel (Code Review)