On Mon, May 9, 2011 at 10:03 AM, Jeff King <peff@xxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html