On Thu, Jun 06, 2013 at 10:58:25AM +0200, Laszlo Ersek wrote:
> comments below
>
> On 05/31/13 18:39, Jeff Cody wrote:
>
> > +usage() {
> > + echo ""
> > + echo "$0 [OPTIONS]"
> > + echo "$desc"
> > + echo ""
> > + echo "OPTIONS:"
> > + echo " -r git range
> > + optional; default is '$range'
> > + "
> > + echo " -c configure options
> > + optional; default is '$config_opt'
> > + "
> > + echo " -m make options
> > + optional; default is '$make_opt'
> > + "
> > + echo " -d log dir
> > + optional; default is '$logdir'
> > + "
> > + echo " -l log filename
> > + optional; default is output-PROCID, where PROCID is the bash
> > process id
> > + note: you may specify a full path for the log filename here,
> > and exclude the
> > + -d option.
> > + "
> > + echo " -f force a git reset and clean
> > + this will cause a 'git reset --hard; git clean -fdx' to be run
> > between checkouts.
> > + !! WARNING: This may cause data loss in your git tree.
> > + READ THE git-clean and git-reset man pages and make
> > + sure you understand the implications of
> > + 'git clean -fdx' and 'git reset --hard' before
> > using !!
> > + "
> > + echo " -h help"
> > +}
>
> Sorry for not noticing this before: is there any reason for the trailing
> spaces on most lines in the usage text?
>
No, not particularly, mainly just formatting with an extra newline,
and trying to keep the code somewhat lined up.
> Plus I suggest lower-casing the "THE" in "READ THE".
>
I agree.
> > +
> > +while getopts ":r:c:m:l:d:hf" opt
> > +do
> > + case $opt in
> > + r) range=$OPTARG
> > + ;;
> > + c) config_opt=$OPTARG
> > + ;;
> > + m) make_opt=$OPTARG
> > + ;;
> > + d) logdir=$OPTARG
> > + ;;
> > + l) logfile=$OPTARG
> > + ;;
> > + f) force_clean='y'
> > + ;;
> > + h) usage
> > + exit
> > + ;;
> > + \?) echo "Unknown option: -$OPTARG" >&2
> > + usage
> > + exit 1
> > + ;;
> > + esac
> > +done
> > +
> > +# append a '/' to logdir if $logdir was specified without one
> > +[[ -n "$logdir" ]] && [[ ${logdir:${#logdir}-1} != "/" ]] &&
> > logdir="${logdir}/"
> > +
> > +logfile="${logdir}${logfile}"
> > +
> > +head=`git rev-parse HEAD`
> > +total=`git rev-list "$range" |wc -l`
> > +
> > +echo "log output: $logfile"
> > +
> > +rm -f "$logfile"
>
> rm -f -- "$logfile" is safer, but I doubt anyone would pass a pathname
> starting with "-"...
You are right, and no reason not to use '--', so I should add that in.
>
> > +date > "$logfile"
> > +echo "git compile check for $range." >> "$logfile"
> > +echo "* configure options='$config_opt'" >> "$logfile"
> > +echo "* make options='$make_opt'" >> "$logfile"
> > +echo "Performing a test compile on $total patches" | tee -a "$logfile"
> > +echo "-------------------------------------------------------------" >>
> > "$logfile"
> > +echo "" | tee -a "$logfile"
> > +
> > +clean_repo() {
> > + if [[ $force_clean == 'y' ]]
> > + then
> > + git reset --hard >> "$logfile" 2>&1 || true
> > + git clean -fdx -e "$logfile" >> "$logfile" 2>&1 || true
> > + fi
> > +}
>
> Does "-e" mean "except"? It's not supported on RHEL-6.
>
Yes - this way it will preserve the log output (the default log path
is in the current working directory). Looks like the -e option was
not introduced until 1.7.3, and RHEL-6 is on 1.7.1.
I'll add a check for the git version, and drop the -e if git is too
old to support it (and in that case, the user will need to make sure
if they supply the -f option to this script that they specify a
different log location than the cwd).
> > +
> > +# we want to cleanup and return the git tree back to the previous head
> > +trap cleanup EXIT
> > +
> > +cleanup() {
> > + echo ""
> > + echo -n "Cleaning up..."
> > + clean_repo
> > + git checkout $head > /dev/null 2>&1
> > + echo "done."
> > +}
> > +
> > +cnt=1
> > +# don't pipe the git job into read, to avoid subshells
> > +while read hash
> > +do
> > + txt=`git log --pretty=tformat:"%h: %s" $hash^!`
> > + echo "${cnt}/${total}: compiling: $txt" | tee -a "$logfile"
> > + let cnt=$cnt+1;
> > + echo "####################" >> "$logfile"
> > + clean_repo
> > + make clean > /dev/null 2>&1 || true
> > + git checkout $hash >> "$logfile" 2>&1 && \
> > + ./configure $config_opt >> "$logfile" 2>&1 && \
> > + make $make_opt >> "$logfile" 2>&1 ||
> > + (
> > + echo "" | tee -a "$logfile"
> > + echo "ERROR: commit $hash failed to build!" | tee -a "$logfile"
> > + git show --stat $hash | tee -a "$logfile"
> > + exit 1
> > + )
> > +done < <(git log $range --pretty=tformat:"%H" --reverse)
> > +
> > +echo "
> > +All patches in $range compiled successfully!" | tee -a "$logfile"
> > +exit 0
> >
>
> I think my remarks are not important, the script should work in any
> practical environment. We should get this merged and small fixes can be
> posted incrementally if needed.
>
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
>
Thanks. I can either do the above changes for a v2, or as follow on
patches.
