threads / discuss / 26175

Re: fatal: ambiguous message

Subject: Re: fatal: ambiguous message

## tl;dr

4 messages between Jan 2, 2011 and Jan 3, 2011.

replies: 3people: 3as markdown or json

Bruce Korb· Jan 2, 2011, 18:03 UTC · lore
On 12/31/10 08:06, Eric Blake wrote:
Show 6 quoted lines
> On 12/30/2010 06:37 PM, Bruce Korb wrote:
>> Hi,
>>
>> Is this fatal?  If so, how come it continued?
> 
> It's fatal to git-version-gen, which did not continue.

git-version-gen has but two fatal conditions: invalid arguments yielding a usage message and an unreadable "tarball version file". That is not this message, but might be clarified with:

    v=`cat $tarball_version_file 2>&1` || {
        echo "$0 error: unreadable tarball version file $1:  $v" >&2
        exit 1
    }
In any event, the invocation is:
   ./git-version-gen .tarball-version
and the file ".tarball-version" does not exist, hence git-version-gen
should not fail at all.  So, this message says, "fatal: ..."
and comes from git and all three "git" invocations redirect stderr to
/dev/null.  The fact that we see it is a git bug.  Error messages
should be directed to stderr and thus written to /dev/null.

So, git-version-gen is correct to continue, but git should fail with a message that names the program that fails ("git") and should direct the message to stderr.

Note to GIT list: the message in question:
    fatal: ambiguous argument 'v0.1..HEAD': unknown revision \
       or path not in the working tree.
Thanks!  Regards, Bruce
Jonathan Nieder· Jan 2, 2011, 18:34 UTC · re: Bruce Korb · lore
Hi,
Context: http://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=blob;f=build-aux/git-version-gen
Bruce Korb wrote:
>                          So, this message says, "fatal: ..."
> and comes from git and all three "git" invocations redirect stderr to
> /dev/null.  The fact that we see it is a git bug.  Error messages
> should be directed to stderr and thus written to /dev/null.
Were you been able to reproduce that outside the script?
> So, git-version-gen is correct to continue, but git should fail
> with a message that names the program that fails ("git") and
> should direct the message to stderr.

No thoughts on this part. git has at least four kinds of message it sends to stderr (fatal:, warning:, error:, and usage:) but I am not sure it is useful to distinguish them ---

	git add: pathspec 'nonsense' did not match any files
might be nicer.
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
Bruce Korb· Jan 3, 2011, 00:16 UTC · re: Jonathan Nieder · lore
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....

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> '
+    git status >/dev/null 2>&1 || {
+        printf UNKNOWN
+        exit 0
+    }
 
-# First see if there is a tarball-only version file.
-# then try "git describe", then default.
-if test -f $tarball_version_file
-then
-    v=`cat $tarball_version_file` || exit 1
-    case $v in
-	*$nl*) v= ;; # reject multi-line output
-	[0-9]*) ;;
-	*) v= ;;
+    test "`git log -1 --pretty=format:x . 2>&1`" = x || {
+        printf UNKNOWN
+        exit 0
+    }
+
+    X=`git describe --abbrev=4 --match='v*' HEAD || \
+        git describe --abbrev=4 HEAD` || {
+        printf UNKNOWN
+        exit 0
+    }
+
+    case "$X" in
+    v[0-9]* ) : ;;
+    * )
+        printf UNKNOWN
+        exit 0
+        ;;
     esac
-    test -z "$v" \
-	&& echo "$0: WARNING: $tarball_version_file seems to be damaged" 1>&2
-fi
 
-if test -n "$v"
-then
-    : # use $v
-# Otherwise, if there is at least one git commit involving the working
-# directory, and "git describe" output looks sensible, use that to
-# derive a version string.
-elif test "`git log -1 --pretty=format:x . 2>&1`" = x \
-    && v=`git describe --abbrev=4 --match='v*' HEAD 2>/dev/null \
-	  || git describe --abbrev=4 HEAD 2>/dev/null` \
-    && v=`printf '%s\n' "$v" | sed "$tag_sed_script"` \
-    && case $v in
-	 v[0-9]*) ;;
-	 *) (exit 1) ;;
-       esac
-then
     # Is this a new git that lists number of commits since the last
     # tag or the previous older version that did not?
     #   Newer: v6.10-77-g0f8faeb
     #   Older: v6.10-g0f8faeb
-    case $v in
+    case $X in
 	*-*-*) : git describe is okay three part flavor ;;
 	*-*)
 	    : git describe is older two part flavor
 	    # Recreate the number of commits and rewrite such that the
 	    # result is the same as if we were using the newer version
 	    # of git describe.
-	    vtag=`echo "$v" | sed 's/-.*//'`
+	    vtag=`echo "$X" | sed 's/-.*//'`
 	    numcommits=`git rev-list "$vtag"..HEAD | wc -l`
-	    v=`echo "$v" | sed "s/\(.*\)-\(.*\)/\1-$numcommits-\2/"`;
+	    X=`echo "$X" | sed "s/\(.*\)-\(.*\)/\1-$numcommits-\2/"`;
 	    ;;
     esac
 
+    # Don't declare a version "dirty" merely because a time stamp has changed.
+    silent_git update-index --refresh >/dev/null 2>&1
+
+    dirty=`git diff-index --name-only HEAD` || dirty=
+    case "$dirty" in
+    '') ;;
+    *) # Append the suffix only if there isn't one already.
+	case $X in
+	  *-dirty) ;;
+	  *) X="$X-dirty" ;;
+	esac
+        ;;
+    esac
+
     # Change the first '-' to a '.', so version-comparing tools work properly.
     # Remove the "g" in git describe's output string, to save a byte.
-    v=`echo "$v" | sed 's/-/./;s/\(.*\)-g/\1-/'`;
+    echo "$X" | sed 's/^v//;s/-/./;s/\(.*\)-g/\1-/'
+}
+
+# First see if there is a tarball-only version file.
+# then try "git describe", then default.
+if test -f $tarball_version_file
+then
+    v=`cat $tarball_version_file` || exit 1
+    case $v in
+	*$nl*) v= ;; # reject multi-line output
+	[0-9]*) ;;
+	*) v= ;;
+    esac
+    test -z "$v" \
+	&& echo "$0: WARNING: $tarball_version_file seems to be damaged" 1>&2
 else
-    v=UNKNOWN
+    v=
 fi
 
-v=`echo "$v" |sed 's/^v//'`
-
-# Don't declare a version "dirty" merely because a time stamp has changed.
-git update-index --refresh > /dev/null 2>&1
+if test -n "$v"
+then
+    : # use $v
 
-dirty=`sh -c 'git diff-index --name-only HEAD' 2>/dev/null` || dirty=
-case "$dirty" in
-    '') ;;
-    *) # Append the suffix only if there isn't one already.
-	case $v in
-	  *-dirty) ;;
-	  *) v="$v-dirty" ;;
-	esac ;;
-esac
+else
+    # Otherwise, if there is at least one git commit involving the
+    # working directory, and "git describe" output looks sensible, use
+    # that to derive a version string.
+    #
+    v=`get_ver` 2>/dev/null
+fi
 
 # Omit the trailing newline, so that m4_esyscmd can use the result directly.
 echo "$v" | tr -d "$nl"


_______________________________________________
Autoconf mailing list
Autoconf@gnu.org
http://lists.gnu.org/mailman/listinfo/autoconf
Eric Blake· Jan 3, 2011, 15:04 UTC · re: Bruce Korb · lore

[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

← back to recent threads