From: Erik Faye-Lund Date: Mon, 09 May 2011 08:41:02 GMT Subject: Re: [PATCH] only warn about ambiguous refs if stderr is a tty Message-ID: In-Reply-To: <20110509080315.GA6205@sigill.intra.peff.net> On Mon, May 9, 2011 at 10:03 AM, Jeff King wrote: > 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). I agree. >> 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. > I agree; it's possible to squelch them already with the core.warnambiguousrefs config, so people who want to live with branched called 'HEAD' can already get around it. >> 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? > Actually, this is just something I read out of Stephen's report and was too lazy to double check. It's not possible to do, because refs/remotes/* does not seem to be checked for ambiguity. Thanks for setting me straight :) >> 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. > I was talking about warnings, not errors. But I can also see that one would sometimes want warnings even when not connected to a tty, but perhaps only when -v is specified? >> 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". While that would touch less code, my gut tells me it's a bit more fragile. But perhaps you're right; I can't come up with any real arguments (i.e use cases that I care about) on top of my head. > And "refs/HEAD" should already work, no? No: $ git init foo $ cd foo/ $ echo "foo" > bar $ git add bar $ git commit -m. [master (root-commit) fc0cbef] . warning: LF will be replaced by CRLF in bar. The file will have its original line endings in your working directory. 1 files changed, 1 insertions(+), 0 deletions(-) create mode 100644 bar $ git show refs/HEAD fatal: ambiguous argument 'refs/HEAD': unknown revision or path not in the working tree. Use '--' to separate paths from revisions >> diff --git a/sha1_name.c b/sha1_name.c >> index faea58d..c7e855e 100644 >> --- a/sha1_name.c >> +++ b/sha1_name.c >> @@ -391,7 +391,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1) >>       if (!refs_found) >>               return -1; >> >> -     if (warn_ambiguous_refs && refs_found > 1) >> +     if (warn_ambiguous_refs && refs_found > 1 && isatty(2)) >>               warning(warn_msg, len, str); >> > > 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. Good point. I doubt I'll update the patch in this direction though, since I agree it's not the right approach.