Re: fatal: ambiguous message

From: Eric Blake <eblake@redhat.com>
Date: Jan 3, 2011, 15:04 UTC
Message-ID: <4D21E574.50404@redhat.com>
In-Reply-To: <4D211555.1040502@gmail.com>

[redirecting to bug-gnulib as the owner of the git-version-gen script in question; replies can drop other lists]

On 01/02/2011 05:16 PM, Bruce Korb wrote:

Show 13 quoted lines

> Hi Jonathan,
> 
> On Sun, Jan 2, 2011 at 10:34 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Were you been able to reproduce that outside the script?
> 
> No, I was blind to the invocation.  You found it.  I was looking
> without seeing.  Thank you.
> 
> Given that shells without functions can be considered sufficiently
> obsolete to not be a consideration, perhaps a better solution is
> to put the I-don't-care-about-error-messages code into a separate
> function with stderr redirected.  Doing that turned out messier
> than I had hoped....

Jonathan's patch:

Show 13 quoted lines

> diff --git a/build-aux/git-version-gen b/build-aux/git-version-gen
> index 5617eb8..119d7aa 100755
> --- a/build-aux/git-version-gen
> +++ b/build-aux/git-version-gen
> @@ -119,7 +119,7 @@ then
>  	    # result is the same as if we were using the newer version
>  	    # of git describe.
>  	    vtag=`echo "$v" | sed 's/-.*//'`
> -	    numcommits=`git rev-list "$vtag"..HEAD | wc -l`
> +	    numcommits=`git rev-list "$vtag"..HEAD 2>/dev/null | wc -l`
>  	    v=`echo "$v" | sed "s/\(.*\)-\(.*\)/\1-$numcommits-\2/"`;
>  	    ;;
>      esac

makes sense to suppress the error message from leaking (whether or not git can be improved to have the error message claim which program is issuing the message); but there's still the nagging issue that because git output is fed to a pipe, there's no way to check $? to see if git failed, in order to properly react to that situation.

Bruce's patch mixes refactoring with bug fixing, making it a bit harder to read, and introduced a bug in its own right:

Show 21 quoted lines

> diff --git a/build-aux/git-version-gen b/build-aux/git-version-gen
> index c278f6a..8a238b0 100755
> --- a/build-aux/git-version-gen
> +++ b/build-aux/git-version-gen
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>  # Print a version string.
> -scriptversion=2010-10-13.20; # UTC
> +scriptversion=2011-01-03.00; # UTC
>  
>  # Copyright (C) 2007-2011 Free Software Foundation, Inc.
>  #
> @@ -78,76 +78,96 @@ tag_sed_script="${2:-s/x/x/}"
>  nl='
>  '
>  
> -# Avoid meddling by environment variable of the same name.
> -v=
> +get_ver()
> +{
> +    local PS4='>gv> '

Portable scripts CANNOT use local (since POSIX does not require it), and setting PS4 is not commonly done in portable scripting.

I'll probably end up writing yet a third approach, which collects git rev-list output into a temporary variable in order to correctly detect failures, without refactoring into a helper function.

-- 
Eric Blake   eblake@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org