Re: fatal: ambiguous message

Re: fatal: ambiguous message

[Bruce Korb]

On 12/31/10 08:06, Eric Blake wrote:
> 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

Re: fatal: ambiguous message

[Jonathan Nieder]

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

Re: fatal: ambiguous message

[Bruce Korb]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

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


[-- Attachment #2: gvg.patch --]
[-- Type: text/x-patch, Size: 4381 bytes --]

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"

[-- Attachment #3: Type: text/plain, Size: 134 bytes --]

_______________________________________________
Autoconf mailing list
Autoconf@gnu.org
http://lists.gnu.org/mailman/listinfo/autoconf

Re: fatal: ambiguous message

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2793 bytes --]

[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 <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:

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]