Skip to contentSkip to conversation

fatal: ambiguous message

DISCUSSION Jonathan Nieder 3 participants 4 messages Jun 15, 2016 2 messages not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw
Quoted 6 lines · Eric Blake
Quoted 3 lines · Bruce Korb

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

#3 lore raw

Hi,

Context: http://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=blob;f=build-aux/git-version-gen

Quoted 4 lines · Bruce Korb

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?

Quoted 3 lines

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.

build-aux/git-version-gen +1 -1
index 5617eb8..119d7aa 100755
@@ -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
#4 lore raw

Hi Jonathan,

Quoted 1 line · Jonathan Nieder

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....

build-aux/git-version-gen +68 -48
index c278f6a..8a238b0 100755
@@ -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
[email protected]
http://lists.gnu.org/mailman/listinfo/autoconf

#5 lore raw

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

Quoted 5 lines · Bruce Korb

Hi Jonathan,

Quoted 1 line · Jonathan Nieder

Were you been able to reproduce that outside the script?

Quoted 8 lines

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:

build-aux/git-version-gen then · line 125

Quoted 13 lines
build-aux/git-version-gen +1 -1
index 5617eb8..119d7aa 100755
@@ -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:

Quoted 13 lines
build-aux/git-version-gen +1 -1
index c278f6a..8a238b0 100755
@@ -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.
 #

build-aux/git-version-gen tag_sed_script="${2:-s/x/x/}" · line 83

Quoted 8 lines
build-aux/git-version-gen +3 -2
@@ -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.

Signature · 2 lines
Eric Blake   [email protected]    +1-801-349-2682
Libvirt virtualization library http://libvirt.org