Re: [PATCH 1/2] gnu-web-doc-update: check the requirements.

[Jim Meyering]

Akim Demaille wrote:
> * build-aux/gnu-web-doc-update (find_tool): Import from bootstrap.
> ($CVS, $CVSU, $GIT, $RSYNC, $XARGS): New.
> ---
>  ChangeLog                    |  6 ++++
>  build-aux/gnu-web-doc-update | 68 
> +++++++++++++++++++++++++++++++++++---------
>  2 files changed, 61 insertions(+), 13 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 27ea1ef..0913c0d 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2012-07-19  Akim Demaille  <address@hidden>
> +
> +     gnu-web-doc-update: check the requirements.
> +     * build-aux/gnu-web-doc-update (find_tool): Import from bootstrap.
> +     ($CVS, $CVSU, $GIT, $RSYNC, $XARGS): New.
...

Thanks for doing this.
I agree that it'd be nice to factor out the find_tool
function one way or another, but am not terribly gung-ho
on getting it done right away.  If you go ahead and push this,
please add a warning that this is duplicate code both here
and in bootstrap.

> +# find_tool ENVVAR NAMES...
> +# -------------------------
> +# Search for a required program.  Use the value of ENVVAR, if set,
> +# otherwise find the first of the NAMES that can be run (i.e.,
> +# supports --version).  If found, set ENVVAR to the program name,
> +# die otherwise.
...

> +# Requirements: everything required to bootstrap your package, plus
> +# these.
> +find_tool CVS cvs
> +find_tool CVSU cvsu
> +find_tool GIT git
> +find_tool RSYNC rsync
> +find_tool XARGS gxargs xargs
> +
>  builddir=.
>  while test $# != 0
>  do
> @@ -93,15 +133,15 @@ version=$(cat $prev) || die "$ME: no $prev file?"
>  pkg=$(sed -n 's/^PACKAGE = \(.*\)/\1/p' $builddir/Makefile) \
>    || die "$ME: no Makefile?"
>  tmp_branch=web-doc-$version-$$
> -current_branch=$(git branch | sed -ne '/^\* /{s///;p;q;}')
> +current_branch=$($GIT branch | sed -ne '/^\* /{s///;p;q;}')

>From an aesthetics/readability perspective, I dislike using variables
like $GIT, $CVS, etc. and would prefer to see functions or aliases that
use the canonical names (git, cvs), but the aesthetics of this little
script aren't worth worrying about, IMHO, so you're welcome to push this
change as-is.  I.e., it's probably not worth the hassle of adding the
file-usable-shell-and-re-exec infrastructure that we use in init.sh.

>  cleanup()
>  {
>    __st=$?
>    rm -rf "$tmp"
> -  git checkout "$current_branch"
> -  git submodule update --recursive
> -  git branch -d $tmp_branch
> +  $GIT checkout "$current_branch"
> +  $GIT submodule update --recursive
> +  $GIT branch -d $tmp_branch
>    exit $__st

TL;DR:  ACK (with comments documenting find_tool duplication) ;-)