git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Erik Faye-Lund <kusmabite@gmail.com>
Date
May 9, 2011, 08:41 UTC
Message-ID
<BANLkTimR_S-px-MfRy0pKGrjxOgSC_=e=A@mail.gmail.com>
In-Reply-To
<20110509080315.GA6205@sigill.intra.peff.net>
On Mon, May 9, 2011 at 10:03 AM, Jeff King <peff@peff.net> wrote:
Show 16 quoted lines
> 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.
Show 16 quoted lines
>> 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.

Show 7 quoted lines
>> 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 :)

Show 16 quoted lines
>> 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?

Show 6 quoted lines
>> 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
Show 17 quoted lines
>> 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.

Previous: Jeff KingNext: Jeff King
Message 35 of 40 in “Creating remote branch called HEAD corrupts remote clones”
  1. Stephen KellyJan 17, 2011
  2. Stephen KellyJan 20, 2011
  3. Thomas RastJan 20, 2011
  4. Stephen KellyJan 20, 2011
  5. Erik Faye-LundJan 20, 2011
  6. Stephen KellyJan 20, 2011
  7. Felipe ContrerasJan 20, 2011
  8. Wesley J. LandakerJan 20, 2011
  9. Junio C HamanoJan 20, 2011
  10. Jeff KingJan 20, 2011
  11. Junio C HamanoJan 20, 2011
  12. Jeff KingJan 20, 2011
  13. Felipe ContrerasJan 20, 2011
  14. Junio C HamanoJan 21, 2011
  15. Felipe ContrerasJan 22, 2011
  16. Stephen KellyFeb 20, 2011
  17. Stephen KellyApr 26, 2011
  18. Felipe ContrerasApr 26, 2011
  19. Stephen KellyApr 27, 2011
  20. Felipe ContrerasApr 27, 2011
  21. Stephen KellyApr 27, 2011
  22. Felipe ContrerasApr 27, 2011
  23. Stephen KellyApr 27, 2011
  24. Felipe ContrerasApr 27, 2011
  25. Erik Faye-LundApr 27, 2011
  26. Erik Faye-LundApr 27, 2011
  27. Stephen KellyMay 2, 2011
  28. Erik Faye-LundMay 2, 2011
  29. Felipe ContrerasMay 3, 2011
  30. Stephen KellyMay 3, 2011
  31. Felipe ContrerasMay 3, 2011
  32. Erik Faye-LundMay 4, 2011
  33. only warn about ambiguous refs if stderr is a ttyErik Faye-Lund, May 9, 2011
  34. Jeff KingMay 9, 2011
  35. Erik Faye-LundMay 9, 2011
  36. Jeff KingMay 9, 2011
  37. Erik Faye-LundMay 9, 2011
  38. Jeff KingMay 9, 2011
  39. Junio C HamanoMay 9, 2011
  40. Jeff KingMay 9, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.