[PATCH 0/2] the cat-file -e option doesn't work as documented

[PATCH 0/2] the cat-file -e option doesn't work as documented

[Ævar Arnfjörð Bjarmason]

The -e option to cat-file will emit output, after promising not to.

We should take either 1/2 or 2/2, but not both. I'm partial to just
documenting the existing behavior and dropping 2/2, it's useful to
know if you passed in something that didn't look like a SHA-1.

But if others disagree we can drop 1/2 and take 2/2. Up to you.

Ævar Arnfjörð Bjarmason (2):
  cat-file doc: document that -e will return some output
  cat-file: -e should not emit output on stderr

 Documentation/git-cat-file.txt | 7 ++++---
 builtin/cat-file.c             | 8 ++++++--
 t/t1006-cat-file.sh            | 7 +++++++
 3 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.15.1.424.g9478a66081

[PATCH 1/2] cat-file doc: document that -e will return some output

[Ævar Arnfjörð Bjarmason]

The -e option added in 7950571ad7 ("A few more options for
git-cat-file", 2005-12-03) has always errored out with message on
stderr saying that the provided object is malformed, currently:

    $ git cat-file -e malformed; echo $?
    fatal: Not a valid object name malformed
    128

A careful reader of this documentation would be mislead into thinking
the could write:

    if ! git cat-file -e "$object" [...]

As opposed to:

    if ! git cat-file -e "$object" 2>/dev/null [...]

To check whether some arbitrary $object string was both valid, and
pointed to an object that exists.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-cat-file.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index fb09cd69d6..f90f09b03f 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -42,8 +42,9 @@ OPTIONS
 	<object>.
 
 -e::
-	Suppress all output; instead exit with zero status if <object>
-	exists and is a valid object.
+	Exit with zero status if <object> exists and is a valid
+	object. If <object> is of an invalid format exit with non-zero and
+	emits an error on stderr.
 
 -p::
 	Pretty-print the contents of <object> based on its type.
@@ -168,7 +169,7 @@ If `-t` is specified, one of the <type>.
 
 If `-s` is specified, the size of the <object> in bytes.
 
-If `-e` is specified, no output.
+If `-e` is specified, no output, unless the <object> is malformed.
 
 If `-p` is specified, the contents of <object> are pretty-printed.
 
-- 
2.15.1.424.g9478a66081

Re: [PATCH 1/2] cat-file doc: document that -e will return some output

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The -e option added in 7950571ad7 ("A few more options for
> git-cat-file", 2005-12-03) has always errored out with message on
> stderr saying that the provided object is malformed, currently:
>
>     $ git cat-file -e malformed; echo $?
>     fatal: Not a valid object name malformed
>     128
>
> A careful reader of this documentation would be mislead into thinking
> the could write:
>
>     if ! git cat-file -e "$object" [...]

It is arguable if such a reader is careful or careless.  I'd rather drop
s/careful // there ;-)

> As opposed to:
>
>     if ! git cat-file -e "$object" 2>/dev/null [...]
>
> To check whether some arbitrary $object string was both valid, and
> pointed to an object that exists.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/git-cat-file.txt | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Re: [PATCH 1/2] cat-file doc: document that -e will return some output

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> The -e option added in 7950571ad7 ("A few more options for
>> git-cat-file", 2005-12-03) has always errored out with message on
>> stderr saying that the provided object is malformed, currently:
>>
>>     $ git cat-file -e malformed; echo $?
>>     fatal: Not a valid object name malformed
>>     128
>>
>> A careful reader of this documentation would be mislead into thinking
>> the could write:
>>
>>     if ! git cat-file -e "$object" [...]
>
> It is arguable if such a reader is careful or careless.  I'd rather drop
> s/careful // there ;-)

Actually the phrasing around here was a bit strange, and I ended up
rewriting a bit more.

    cat-file doc: document that -e will return some output
    
    The -e option added in 7950571ad7 ("A few more options for
    git-cat-file", 2005-12-03) has always errored out with message on
    stderr saying that the provided object is malformed, like this:
    
        $ git cat-file -e malformed; echo $?
        fatal: Not a valid object name malformed
        128
    
    A reader of this documentation may be misled into thinking that
    
        if ! git cat-file -e "$object" [...]
    
    as opposed to:
    
        if ! git cat-file -e "$object" 2>/dev/null [...]
    
    is sufficient to implement a truly silent test that checks whether
    some arbitrary $object string was both valid, and pointed to an
    object that exists.
    
    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

[PATCH 2/2] cat-file: -e should not emit output on stderr

[Ævar Arnfjörð Bjarmason]

Change "cat-file -e some-garbage" to work as documented. Before it
would emit:

    $ git cat-file -e some-garbage; echo $?
    fatal: Not a valid object name some-garbage
    128

Now:

    $ ./git-cat-file -e some-garbage; echo $?
    1

This is a change to longstanding behavior established in
7950571ad7 ("A few more options for git-cat-file", 2005-12-03) when
the option was initially added, but we should go with the promise made
in the documentation.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/cat-file.c  | 8 ++++++--
 t/t1006-cat-file.sh | 7 +++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f5fa4fd75a..75991788af 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -65,8 +65,12 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		flags |= OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
 
 	if (get_oid_with_context(obj_name, GET_OID_RECORD_PATH,
-				 &oid, &obj_context))
-		die("Not a valid object name %s", obj_name);
+				 &oid, &obj_context)) {
+		if (opt == 'e')
+			return 1;
+		else
+			die("Not hello a valid object name %s", obj_name);
+	}
 
 	if (!path)
 		path = obj_context.path;
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index b19f332694..c05a899bc4 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -201,6 +201,13 @@ do
     '
 done
 
+test_expect_success 'Providing -e should suppress all error output' '
+	! git cat-file -e some-garbage >stdout 2>stderr &&
+	>expect &&
+	test_cmp expect stdout &&
+	test_cmp expect stderr
+'
+
 for opt in t s e p
 do
     test_expect_success "Passing -$opt with --follow-symlinks fails" '
-- 
2.15.1.424.g9478a66081

Re: [PATCH 0/2] the cat-file -e option doesn't work as documented

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The -e option to cat-file will emit output, after promising not to.
>
> We should take either 1/2 or 2/2, but not both. I'm partial to just
> documenting the existing behavior and dropping 2/2, it's useful to
> know if you passed in something that didn't look like a SHA-1.
>
> But if others disagree we can drop 1/2 and take 2/2. Up to you.
>
> Ævar Arnfjörð Bjarmason (2):
>   cat-file doc: document that -e will return some output
>   cat-file: -e should not emit output on stderr
>
>  Documentation/git-cat-file.txt | 7 ++++---
>  builtin/cat-file.c             | 8 ++++++--
>  t/t1006-cat-file.sh            | 7 +++++++
>  3 files changed, 17 insertions(+), 5 deletions(-)

I am kind of confused.  

When the doc there says "no output", I read it as "no output", and
no other restriction (like suppressing an error diagnosis, which is
not even sent to the standard output stream).

Re: [PATCH 0/2] the cat-file -e option doesn't work as documented

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I am kind of confused.  
>
> When the doc there says "no output", I read it as "no output", and
> no other restriction (like suppressing an error diagnosis, which is
> not even sent to the standard output stream).

Ah, OK, so you were saying with 1/2 that "output" could mean
"something sent to standard error stream" as well and can be helped
with a more explicit clarification.  Makes sense to me (sort-of).