From: Jakub Narebski <jnareb@gmail.com>
Date: Thu, 16 Feb 2012 09:15:27 GMT
Subject: Re: [PATCH v2] git-latexdiff: new command in contrib, to use latexdiff and Git
Message-ID: <m3hayrrvf9.fsf@localhost.localdomain>
In-Reply-To: <1329381560-15853-1-git-send-email-Matthieu.Moy@imag.fr>

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> +verbose "Auto-detecting PDF viewer"
> +for command in xdg-open evince okular xpdf acroread; do
> +    if [ "$PDFVIEWER" = "" ]; then
> +	if command -v "$command" >/dev/null 2>&1; then
> +	    PDFVIEWER="$command"
> +	else
> +	    verbose_progress
> +	fi
> +    fi
> +done
> +verbose_done "$PDFVIEWER"

Why we autodetect PDF viewer unconditionally?  Why we do not stop on
first detected viewer rather than last?  Why not

  +if [ "$PDFVIEWER" = "" ]; then
  +	verbose "Auto-detecting PDF viewer"
  +	for command in xdg-open evince okular xpdf acroread; do
  +		if command -v "$command" >/dev/null 2>&1; then
  +			PDFVIEWER=$command
  +			break
  +		else
  +			verbose_progress
  +		fi
  +	done
  +	verbose_done "$PDFVIEWER"
  +fi

Nb. Documentation/CodingGuidelines says:

  For shell scripts specifically (not exhaustive):
  
  [...]
  
   - We prefer "test" over "[ ... ]".

I know that 'contrib/' is more relaxed...
-- 
Jakub Narebski