Re: [PATCH] only warn about ambiguous refs if stderr is a tty

On Mon, May 09, 2011 at 09:51:18AM +0200, Erik Faye-Lund wrote:

If there's a branch (either local or remote) called 'HEAD'
commands that take a ref currently emits a warning, no matter
if the output is going to a TTY or not.

Fix this by making sure we only output this warning when stderr
is a TTY. Other git commands or scripts should not care about
this ambiguity.

This fix prevents gitk from barfing when given no arguments and
there's a branch called 'HEAD'.

This feels wrong. Gitk should not care about messages on stderr, for
exactly the reason that they may be harmless warnings (if anything, it
should show them to the user in a dialog).

My understanding is that this is a tcl thing, but I just think it's
insane.

In 2f8acdb ('core.warnambiguousrefs: warns when "name" is used
and both "name" branch and tag exists.'), a check for collisions
of refs was introduced. It does not seem to me like the intention
was to check HEAD for ambiguty (because the commit talks about
branches and tags), but it does.

Because HEAD cannot be disambiguated like branches and tags can,
this can lead to an annoying warning, or even an error in the case
of gitk.

This is a separate issue, isn't it? Gitk should probably handle
ambiguous ref warnings better, no matter what the name.  And if
ambiguous HEAD warnings are considered too annoying, they should be
squelched for everyone. I don't personally have an opinion on the
latter, though.

A branch called HEAD can be 'injected' into another user's repo
through remotes, and this can cause annoyance (and in the case of
gitk, brokenness) just by pulling the wrong remote. Yuck.

Can you give an example? If I am fetching your refs into
refs/remotes/$remote/*, how does that create an ambiguity?

The particular problem of gitk can be fixed by making gitk able
to parse the warning, and probably forwarding it to the user.
This strikes me as The Right Thing To Do(tm), but is outside of my
gitk and TCL/TK skills.

Agreed. And also outside my tcl skills. :)

One question is if ANY warnings should be output to stderr if it's
not a TTY. My guess is that there probably are some classes of
warnings that should, but the vast majority should probably not.

I disagree. If I do:

  git foo 2>errors

I would certainly expect any relevant errors to end up in that file. As
for why I would do that, two cases I can think of offhand are:

  1. Test scripts, which use this extensively.

  2. Sometimes cron jobs will capture chatty output in a file and show
     it only in the case of some error condition.

Another question is if we should come up with a way of
disambiguating HEAD. Perhaps having something like 'refs/HEAD'
will do?

Yeah, if we disambiguate, I would be tempted to say that "HEAD" always
unambiguously refers to "HEAD". And "refs/HEAD" should already
work, no?

So, to recap: The way I see it, these are our options:

 1) Discard this specific warning when stderr isn't a TTY (i.e
    what this patch does)
 2) Discard all warnings when stderr isn't a TTY
 3) Make gitk understand and forward warnings to the user
 4) Have gitk explicitly ignore ambiuous refs
 5) Come up with a way to disambiguate HEAD, and use that instead
    by default
 6) Force HEAD to never be ambiguous
 7) Leave things as they are

I think 3) + 5) might be the most sane solution.

Agreed.

I think I have made it clear that I am not in favor of this approach,
but if we were to do it, it is too late to be calling isatty(2) here.
You need to also check pager_in_use(), as we may have redirected stderr
into the pager's pipe.

-Peff