[BUG (maybe)] git rev-parse --verify --quiet isn't quiet

[BUG (maybe)] git rev-parse --verify --quiet isn't quiet

[Øystein Walle]

Hi,

I noticed this while writing a small script for myself:

    $ git rev-parse --verify --quiet '@{u}'
    fatal: No upstream configured for branch 'local'

The functions that get run when rev-parse does its thing all return a
pointer to a strbuf, return the length of the modified buf or something
to that effect. However get_upstream_branch() calls die() which writes
to stderr unconditionally.

Call hierarchy in the above case: 

die()
  get_upstream_branch()
    interpret_upstream_mark()
      interpret_branch_name()
        substitute_branch_name()
          dwim_ref()
            get_sha1_basic()
              get_sha1_1()
                get_sha1_with_context_1()
                  get_sha1_with_context()
                    get_sha1()
                      cmd_rev_parse()

Passing quiet = 1 all the way down I assume isn't feasible. Changing
get_upstream_branch() to not die but rather signify an error could work.
But interpret_branch_name() has four different callers belonging to
different parts of Git.

I marked this as "maybe" because I guess it's fair to say that rev-parse
'@{u}' is not the correct way to check whether a branch as a configured
upstream. Furthermore even if it writes to stderr the exit status is
still non-zero so the only way to get bitten by this is if you doing
something like:

    foo=$(git rev-parse --verify --quiet $bar 2>&1) # pointless redirection

*and* you check whether the exit code is equal to 1, not unequal to zero.

On the other hand, it does break the expectations that the documentation
and the other uses of git rev-parse one encounters give. At least for
me. It should be able to verify that $foo is valid, and it should do so
quietly.

On a related note:

    $ git branch origin/master
    $ git rev-parse --verify --quiet origin/master
    warning: refname 'origin/master' is ambiguous.
    7c07808349fd0fc2c61a169833eeb55163cf3df4   


Best regards,
Øsse

Re: [BUG (maybe)] git rev-parse --verify --quiet isn't quiet

[Junio C Hamano]

Øystein Walle <oystwa@gmail.com> writes:

> I noticed this while writing a small script for myself:
>
>     $ git rev-parse --verify --quiet '@{u}'
>     fatal: No upstream configured for branch 'local'
>
> The functions that get run when rev-parse does its thing all return a
> pointer to a strbuf, return the length of the modified buf or something
> to that effect. However get_upstream_branch() calls die() which writes
> to stderr unconditionally.

I would suspect that this may be fine.

"rev-parse --verify" makes sure the named object exists, but in this
case @{u} does not even name any object, does it?

> On a related note:
>
>     $ git branch origin/master
>     $ git rev-parse --verify --quiet origin/master
>     warning: refname 'origin/master' is ambiguous.
>     7c07808349fd0fc2c61a169833eeb55163cf3df4   

And this warning is in the same vein, I would suspect.

Re: [BUG (maybe)] git rev-parse --verify --quiet isn't quiet

[Junio C Hamano]

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

> I would suspect that this may be fine.
>
> "rev-parse --verify" makes sure the named object exists, but in this
> case @{u} does not even name any object, does it?

Hmph, but "rev-parse --verify no-such-branch" does *not* name any
object, we would want to see it barf, and we probably would want to
be able to squelch the message.  So it is unclear if @{u} barfing is
a good idea.

What is the reason why it is inpractical to pass 'quiet' down the
callchain?

Re: [BUG (maybe)] git rev-parse --verify --quiet isn't quiet

[Øystein Walle]

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

> 
> Junio C Hamano <gitster <at> pobox.com> writes:
> 
> > I would suspect that this may be fine.
> >
> > "rev-parse --verify" makes sure the named object exists, but in this
> > case  <at> {u} does not even name any object, does it?
> 
> Hmph, but "rev-parse --verify no-such-branch" does *not* name any
> object, we would want to see it barf, and we probably would want to
> be able to squelch the message.  So it is unclear if  <at> {u} barfing is
> a good idea.
> 

That was my counter-argument :) The "vibe" I get from rev-parse --verify
--quiet is that it should handle anything.

> 
> What is the reason why it is inpractical to pass 'quiet' down the
> callchain?
> 

Maybe I'm missing something obvious, but wouldn't that mean changing the
signature of 9 different functions, and consequently all of their calls
throughout Git? 

That's perhaps not a good argument. Who cares whether a diff is small or
large if it fixes a bug properly?  But most (or all) of those functions
do not concern themselves with printing stuff so maybe an additional
"quiet?" argument would look foreign in most places and make the code
harder to read.

Øsse