Re: fatal: ambiguous message

[Jim Meyering]

Eric Blake wrote:
> [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:
>> Hi Jonathan,
>>
>> On Sun, Jan 2, 2011 at 10:34 AM, Jonathan Nieder <address@hidden> 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:
>
>> 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:
>
>> 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.

Thanks for forwarding that here.
Here's a lightly tested patch to do what you suggest.
I tried to keep it minimal, since what we're doing here
is solely to accommodate very old versions of git.

To test it, without instrumentation you'd have to install a very
old version of git; instead I tweaked conditionals and case-stmts
to exercise the code in question and changed the commit-list
command to fail;  Then, in the gnulib directory, I ran this:

    $ build-aux/git-version-gen /dev/null
    build-aux/git-version-gen: WARNING: git rev-list failed
    UNKNOWN-dirty%

>From eca07fb7cb4ae50102c19cd1b1a6ce54b5319b11 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 3 Jan 2011 19:35:19 +0100
Subject: [PATCH] git-version-gen: handle failed "git rev-list"

* build-aux/git-version-gen: Rather than leaking a "fatal" error
from git and proceeding as if it had succeeded but printed no SHA1
checksums, suppress the diagnostic and handle the failure.
---
 ChangeLog                 |    5 +++++
 build-aux/git-version-gen |    8 ++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 749ad91..109b128 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2011-01-03  Jim Meyering  <address@hidden>

+       git-version-gen: handle failed "git rev-list"
+       * build-aux/git-version-gen: Rather than leaking a "fatal" error
+       from git and proceeding as if it had succeeded but printed no SHA1
+       checksums, suppress the diagnostic and handle the failure.
+
        git-version-gen: include command name in one more diagnostic
        * build-aux/git-version-gen: When the required .tarball-version file
        was missing or unreadable, you might see the diagnostic from "cat",
diff --git a/build-aux/git-version-gen b/build-aux/git-version-gen
index c337673..dd893f9 100755
--- a/build-aux/git-version-gen
+++ b/build-aux/git-version-gen
@@ -1,6 +1,6 @@
 #!/bin/sh
 # Print a version string.
-scriptversion=2011-01-03.10; # UTC
+scriptversion=2011-01-03.18; # UTC

 # Copyright (C) 2007-2011 Free Software Foundation, Inc.
 #
@@ -122,8 +122,12 @@ 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`
+           commit_list=`git rev-list "$vtag"..HEAD 2>/dev/null` \
+               || { commit_list=failed;
+                    echo "$0: WARNING: git rev-list failed" 1>&2; }
+           numcommits=`echo "$commit_list" | wc -l`
            v=`echo "$v" | sed "s/\(.*\)-\(.*\)/\1-$numcommits-\2/"`;
+           test "$commit_list" = failed && v=UNKNOWN
            ;;
     esac

--
1.7.3.4