* Re: [PATCH] i18n: diff/apply statistics
From: Jiang Xin @ 2012-02-03 1:23 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Git List
In-Reply-To: <CACsJy8DfdTPfwZJq1_=RzO_f67FoLGJ1Q__Qfqtg+HxKZEMp4g@mail.gmail.com>
2012/2/3 Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
> On Fri, Feb 3, 2012 at 8:12 AM, Jiang Xin <worldhello.net@gmail.com> wrote:
>> 2012/2/2 Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
>>>
>>> There's another patch with similar goal:
>>>
>>> http://thread.gmane.org/gmane.comp.version-control.git/189453/focus=189509
>>
>> It's cool, especially the call of interactive_use, so that
>> translations for the statistics line is only for human, not for scripts/pipe...
>
> That's the idea but they decided to allow translations unconditionally
> in the end.
Ohh, sad, not so cool. Are there any reasons?
--
Jiang Xin
^ permalink raw reply
* Re: Problem pushing to a Novell share
From: Junio C Hamano @ 2012-02-03 1:28 UTC (permalink / raw)
To: Rüdiger Kessel; +Cc: git
In-Reply-To: <CAJ4nRM1K=sCy8_0PG-NADVn4T0XG1ELC7AvtQyW-Dc1cUMzEXQ@mail.gmail.com>
Rüdiger Kessel <ruediger.kessel@gmail.com> writes:
> Trying git_mkstemp_mode() first means trying more 16000 times to create a
> random file before realizing that something might be wrong.
Well, git_mkstemps_mode() does have such a 16k loop, and it tries to
create a unique, unused file this way:
fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
if (fd > 0)
return fd;
/*
* Fatal error (EPERM, ENOSPC etc).
* It doesn't make sense to loop.
*/
if (errno != EEXIST)
break;
If you do not have a directory D and try to create D/tmp_random here,
shouldn't you get an error that is _NOT_ EEXIST and trigger this break?
^ permalink raw reply
* Re: [PATCH] i18n: diff/apply statistics
From: Nguyen Thai Ngoc Duy @ 2012-02-03 1:28 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List
In-Reply-To: <CANYiYbHtJMw25ZDbYRbe8S_GM-gR6okkDwwNXo_yxPhPrHQp+w@mail.gmail.com>
On Fri, Feb 3, 2012 at 8:23 AM, Jiang Xin <worldhello.net@gmail.com> wrote:
> 2012/2/3 Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
>> On Fri, Feb 3, 2012 at 8:12 AM, Jiang Xin <worldhello.net@gmail.com> wrote:
>>> 2012/2/2 Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
>>>>
>>>> There's another patch with similar goal:
>>>>
>>>> http://thread.gmane.org/gmane.comp.version-control.git/189453/focus=189509
>>>
>>> It's cool, especially the call of interactive_use, so that
>>> translations for the statistics line is only for human, not for scripts/pipe...
>>
>> That's the idea but they decided to allow translations unconditionally
>> in the end.
>
> Ohh, sad, not so cool. Are there any reasons?
>
Inconsistency mainly. I don't see anything sad about that though. You
should read that thread.
--
Duy
^ permalink raw reply
* Re: [PATCH] i18n: po for zh_cn
From: Jiang Xin @ 2012-02-03 1:35 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Git List, Nguyen Thai Ngoc Duy
In-Reply-To: <CACBZZX5AceH+pc-teFm=PE9u14PM88GLtLnNg5ENOpNni0Tsig@mail.gmail.com>
2012/2/3 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
> For now we can also avoid this whole issue and just declare that
> anything that's used for interchange or permanently stored
> (git-format-patch output, reflogs, merge commit messages etc.) will be
> in English.
An extreme case is: Hg translated 'hg --versoin' and broke many other software.
For example, redmine had to grep hg version properly using 'LC_ALL=C
hg --version'.
I think Duy‘s patch maybe better for such cases.
http://thread.gmane.org/gmane.comp.version-control.git/189453/focus=189509
The call of interactive_use makes translations only available for human
but not for interchange and permanently stored.
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH, RFC] Fix build problems related to profile-directed optimization
From: Ted Ts'o @ 2012-02-03 2:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Andi Kleen, Clemens Buchacher
In-Reply-To: <7vvcnou40u.fsf@alter.siamese.dyndns.org>
What do you think of this? I'm still running a test build --- "make
PROFILE=BUILD all" takes quite a long time, so this is still an RFC; I
figure there will still be some places where people will point out
more nits to be polished. :-)
(In particular, I just noticed I left the V=1 for debugging purposes
in this version....)
- Ted
>From 4bf14e732216fd1327da2e3c8c6dfc0a3f689e1b Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Thu, 2 Feb 2012 13:56:22 -0500
Subject: [PATCH] Fix build problems related to profile-directed optimization
There was a number of problems I ran into when trying the
profile-directed optimizations added by Andi Kleen in git commit
7ddc2710b9. (This was using gcc 4.4 found on many enterprise
distros.)
1) The -fprofile-generate and -fprofile-use commands are incompatible
with ccache; the code ends up looking in the wrong place for the gcda
files based on the ccache object names.
2) If the makefile notices that CFLAGS are different, it will rebuild
all of the binaries. Hence the recipe originally specified by the
INSTALL file ("make profile-all" followed by "make install") doesn't
work. It will appear to work, but the binaries will end up getting
built with no optimization.
This patch fixes this by using an explicit set of options passed via
the PROFILE variable then using this to directly manipulate CFLAGS and
EXTLIBS.
The developer can run "make PROFILE=BUILD all ; make PROFILE=BUILD
install" to do an automatic two-pass build using the test suite as the
sample workload for the purpose of profiling.
Alternatively, the profiling version of binaries can be built using:
make PROFILE=GEN PROFILE_DIR=/var/cache/profile all
make PROFILE=GEN install
and then after git has been used a number of times, the optimized
version of the binary can be built as follows:
make PROFILE=USE PROFILE_DIR=/var/cache/profile all
make PROFILE=USE install
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andi Kleen <ak@linux.intel.com>
---
INSTALL | 4 ++--
Makefile | 41 ++++++++++++++++++++++++++++++-----------
2 files changed, 32 insertions(+), 13 deletions(-)
diff --git a/INSTALL b/INSTALL
index 6fa83fe..73b654b 100644
--- a/INSTALL
+++ b/INSTALL
@@ -28,8 +28,8 @@ set up install paths (via config.mak.autogen), so you can write instead
If you're willing to trade off (much) longer build time for a later
faster git you can also do a profile feedback build with
- $ make profile-all
- # make prefix=... install
+ $ make --prefix=/usr PROFILE=BUILD all
+ # make --prefix=/usr PROFILE=BUILD install
This will run the complete test suite as training workload and then
rebuild git with the generated profile feedback. This results in a git
diff --git a/Makefile b/Makefile
index c457c34..7d66d5c 100644
--- a/Makefile
+++ b/Makefile
@@ -1772,6 +1772,24 @@ ifdef ASCIIDOC7
export ASCIIDOC7
endif
+### profile feedback build
+#
+
+# Can adjust this to be a global directory if you want to do extended
+# data gathering
+PROFILE_DIR := $(CURDIR)
+
+ifeq "$(PROFILE)" "GEN"
+ CFLAGS += -fprofile-generate=$(PROFILE_DIR) -DNO_NORETURN=1
+ EXTLIBS += -lgcov
+ export CCACHE_DISABLE=t
+ V=1
+else ifneq "$PROFILE" ""
+ CFLAGS += -fprofile-use=$(PROFILE_DIR) -fprofile-correction -DNO_NORETURN=1
+ export CCACHE_DISABLE=t
+ V=1
+endif
+
# Shell quote (do not use $(call) to accommodate ancient setups);
SHA1_HEADER_SQ = $(subst ','\'',$(SHA1_HEADER))
@@ -1828,7 +1846,15 @@ export DIFF TAR INSTALL DESTDIR SHELL_PATH
SHELL = $(SHELL_PATH)
-all:: shell_compatibility_test $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
+all:: shell_compatibility_test
+
+ifeq "$(PROFILE)" "BUILD"
+all:: profile-clean
+ $(MAKE) PROFILE=GEN all
+ $(MAKE) PROFILE=GEN -j1 test
+endif
+
+all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
ifneq (,$X)
$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
endif
@@ -2699,16 +2725,9 @@ cover_db_html: cover_db
### profile feedback build
#
-.PHONY: profile-all profile-clean
-
-PROFILE_GEN_CFLAGS := $(CFLAGS) -fprofile-generate -DNO_NORETURN=1
-PROFILE_USE_CFLAGS := $(CFLAGS) -fprofile-use -fprofile-correction -DNO_NORETURN=1
+.PHONY: profile-clean
profile-clean:
- $(RM) $(addsuffix *.gcda,$(object_dirs))
- $(RM) $(addsuffix *.gcno,$(object_dirs))
+ $(RM) $(addsuffix *.gcda,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))
+ $(RM) $(addsuffix *.gcno,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))
-profile-all: profile-clean
- $(MAKE) CFLAGS="$(PROFILE_GEN_CFLAGS)" all
- $(MAKE) CFLAGS="$(PROFILE_GEN_CFLAGS)" -j1 test
- $(MAKE) CFLAGS="$(PROFILE_USE_CFLAGS)" all
--
1.7.8.11.gefc1f.dirty
^ permalink raw reply related
* Re: How best to handle multiple-authorship commits in GIT?
From: Valerie Aurora @ 2012-02-03 2:18 UTC (permalink / raw)
To: David Howells; +Cc: git@vger.kernel.org, Rusty Russell
In-Reply-To: <15281.1328207789@redhat.com>
On Thu, Feb 2, 2012 at 10:36 AM, David Howells <dhowells@redhat.com> wrote:
> Valerie Aurora <valerie.aurora@gmail.com> wrote:
>
>> There is a line in the kernel doc saying how this should be handled,
>> suggested by Rusty, but it's not being followed.
>
> Do you know where?
A fault in my memory - I asked Rusty for advice on a related problem
and he gave me additional advice, he didn't suggest (so far as I know)
the changes to the kernel docs. Here's what SubmittingPatches has to
say:
"If you are a subsystem or branch maintainer, sometimes you need to slightly
modify patches you receive in order to merge them, because the code is not
exactly the same in your tree and the submitters'. If you stick strictly to
rule (c), you should ask the submitter to rediff, but this is a totally
counter-productive waste of time and energy. Rule (b) allows you to adjust
the code, but then it is very impolite to change one submitter's code and
make him endorse your bugs. To solve this problem, it is recommended that
you add a line between the last Signed-off-by header and yours, indicating
the nature of your changes. While there is nothing mandatory about this, it
seems like prepending the description with your mail and/or name, all
enclosed in square brackets, is noticeable enough to make it obvious that
you are responsible for last-minute changes. Example :
Signed-off-by: Random J Developer <random@developer.example.org>
[lucky@maintainer.example.org: struct foo moved from foo.c to foo.h]
Signed-off-by: Lucky K Maintainer <lucky@maintainer.example.org>
This practise is particularly helpful if you maintain a stable branch and
want at the same time to credit the author, track changes, merge the fix,
and protect the submitter from complaints. Note that under no circumstances
can you change the author's identity (the From header), as it is the one
which appears in the changelog."
And Rusty's practice as of a year or two ago is for "minor mods" to a
patch, to leave the authorship the same, and add a Signed-off-by:
Signed-off-by: Some Upstream Author
Signed-off-by: Maintainer or Merger (rewrote error handling)
And for a complete (meaningful) rewrite such as David has done, he
changes the commit authorship and adds a Signed-off-by for the
original author.
That's existing guidelines and practice.
-VAL
^ permalink raw reply
* Re: how to determine oldest supported version of git
From: Junio C Hamano @ 2012-02-03 4:52 UTC (permalink / raw)
To: Neal Kreitzinger; +Cc: git
In-Reply-To: <jgeekn$of2$1@dough.gmane.org>
"Neal Kreitzinger" <neal@rsss.com> writes:
> What is the best way for me (a git user) to determine what is currently
> the oldest supported version of git (the oldest version still getting
> bugfixes)? IOW, when can I tell that my version of git is no longer
> supported?
"A note from the maintainer" only promises that the latest major release
(as of this writing, 1.7.9) gets regular maintenance releases until the
next major release happens.
When queuing a fix to an old bug, however, I try to build a topic branch
for that fix from as old an release as practical, in order to make sure
that older maintenance tracks could benefit, and I do give updates for
older maintenance tracks when able (but no promises).
For example, during the last cycle leading to 1.7.9, in other words, back
when 1.7.8 was the latest major release, in addition to the maintenance
releases 1.7.8.1, 1.7.8.2, 1.7.8.3 and 1.7.8.4, maintenance releases for
older version of Git were tagged (1.7.6.5, 1.7.7.5, and 1.7.7.6). Note
that 1.7.6 was originally released on June 26th, 2011.
One cycle of major release development is expected to last between 8 to 10
weeks, so keeping two stale maintenance tracks in addition to the latest
maintenance track alive would roughly translate to 6 months shelf life for
an ancient release.
As other people mentioned, if you are on a (probably paid) support plan
from a(n enterprise) distro, asking them would be the best way, and if you
are running Git supplied as part of a distro, the distro would dictate the
version it supplies to you, so asking here would not help very much.
^ permalink raw reply
* Re: General support for ! in git-config values
From: Kyle Moffett @ 2012-02-03 5:08 UTC (permalink / raw)
To: demerphq
Cc: Jeff King, Junio C Hamano, Ævar Arnfjörð,
Git Mailing List
In-Reply-To: <CANgJU+XoZd6x6jdSHszigZaPgi+6H3Nbf4OG7p0y1_=7m+qntA@mail.gmail.com>
On Thu, Feb 2, 2012 at 02:21, demerphq <demerphq@gmail.com> wrote:
>> So far I haven't seen an actual patch to comment on
>> (or even a proposed syntax beyond starting a string with "!", which I
>> think is a non-starter due to conflicting with existing uses),
>
> I understand. I think we will probably use backtick quoting in git-deploy. So
>
> deploy.prefix=`cat /etc/SERVER_ROLE`
>
> will execute cat /etc/SERVER_ROLE and use the results as the value of
> the config option.
Alternatively, you could extend the recent proposal for GIT config
"include" statements so that something like this works:
[include]
exec = echo "deploy.prefix = `cat /etc/SERVER_ROLE`"
exec = /usr/local/bin/git-config-for-ldap-user
Thoughts?
Cheers,
Kyle Moffett
^ permalink raw reply
* Re: How best to handle multiple-authorship commits in GIT?
From: Junio C Hamano @ 2012-02-03 5:11 UTC (permalink / raw)
To: Valerie Aurora; +Cc: David Howells, git@vger.kernel.org, Rusty Russell
In-Reply-To: <CAD-XujkVK=tOtmVS90U0KAutFZ55jxsHMKuuMppXOi-H6ZY=RQ@mail.gmail.com>
Valerie Aurora <valerie.aurora@gmail.com> writes:
> And Rusty's practice as of a year or two ago is for "minor mods" to a
> patch, to leave the authorship the same, and add a Signed-off-by:
>
> Signed-off-by: Some Upstream Author
> Signed-off-by: Maintainer or Merger (rewrote error handling)
>
> And for a complete (meaningful) rewrite such as David has done, he
> changes the commit authorship and adds a Signed-off-by for the
> original author.
>
> That's existing guidelines and practice.
All sounds very sensible. Thanks for a summary.
^ permalink raw reply
* Re: [PATCH, RFC] Fix build problems related to profile-directed optimization
From: Junio C Hamano @ 2012-02-03 6:00 UTC (permalink / raw)
To: Ted Ts'o; +Cc: git, Andi Kleen, Clemens Buchacher
In-Reply-To: <20120203020743.GE1032@thunk.org>
Ted Ts'o <tytso@mit.edu> writes:
> What do you think of this? I'm still running a test build --- "make
> PROFILE=BUILD all" takes quite a long time, so this is still an RFC; I
> figure there will still be some places where people will point out
> more nits to be polished. :-)
>
> (In particular, I just noticed I left the V=1 for debugging purposes
> in this version....)
Thanks.
Three comments:
* I am happy that this version handles this well:
$ make PROFILE=BUILD install
even though you did not advertise as such in INSTALL ;-).
* However, I think "clean" target should remove *.gcda unconditionally.
$ make PROFILE=BUILD install ; make clean ; git clean -n -x | grep gcda
* Running "make PROFILE=BUILD install" immediately after another one,
without "make clean" in between, resulted in full rebuild and test
before the second "install", which somewhat surprised me. I however do
not think this is a big show-stopper problem.
^ permalink raw reply
* Re: General support for ! in git-config values
From: Junio C Hamano @ 2012-02-03 6:11 UTC (permalink / raw)
To: Kyle Moffett
Cc: demerphq, Jeff King, Ævar Arnfjörð,
Git Mailing List
In-Reply-To: <CAGZ=bq++R+X+2r2_zQ4UZ6JvDC9W9_4nF23MQ6+612_Qe2RS4Q@mail.gmail.com>
Kyle Moffett <kyle@moffetthome.net> writes:
> Alternatively, you could extend the recent proposal for GIT config
> "include" statements so that something like this works:
>
> [include]
> exec = echo "deploy.prefix = `cat /etc/SERVER_ROLE`"
> exec = /usr/local/bin/git-config-for-ldap-user
Erh...
Running known stuff from your own .git/config may be justifiable as "at
your own risk", but if we consider sources that are not under your direct
control, such as /etc/gitconfig and whatever your project encourages you
to include from your .git/config,... eek.
^ permalink raw reply
* Re: General support for ! in git-config values
From: Kyle Moffett @ 2012-02-03 7:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: demerphq, Jeff King, Ævar Arnfjörð,
Git Mailing List
In-Reply-To: <7vmx90say8.fsf@alter.siamese.dyndns.org>
On Thu, Feb 2, 2012 at 22:11, Junio C Hamano <gitster@pobox.com> wrote:
> Kyle Moffett <kyle@moffetthome.net> writes:
>
>> Alternatively, you could extend the recent proposal for GIT config
>> "include" statements so that something like this works:
>>
>> [include]
>> exec = echo "deploy.prefix = `cat /etc/SERVER_ROLE`"
>> exec = /usr/local/bin/git-config-for-ldap-user
>
> Erh...
>
> Running known stuff from your own .git/config may be justifiable as "at
> your own risk", but if we consider sources that are not under your direct
> control, such as /etc/gitconfig and whatever your project encourages you
> to include from your .git/config,... eek.
Well yes, but running commands from .git/config is exactly what the OP
requested, and if it applies to .git/config it should also be
applicable to other trusted include sources too, no?
Perhaps allow config files to perform a "trusted" include, EG:
[include]
trusted_exec = /usr/local/bin/site-specific-config-program
blob = v1.0:src/gitconfig
By default, the only files which would be trusted are /etc/gitconfig,
~/.gitconfig, and .git/config (but ONLY if it has the same owner and
mode go-w), and they would only pass trust on to other files if they
use "trusted_*" include lines.
Also, since "include" is intended to introduce a
non-backwards-compatible change in behavior, perhaps a totally
different format should be used, EG:
$include exec_trusted /usr/local/bin/site-specific-config-program
$include blob v1.0:src/gitconfig
Something that would cause noticeable warnings in older versions of
git instead of silently ignoring the desired config includes.
Just a few thoughts.
Cheers,
Kyle Moffett
^ permalink raw reply
* Re: [PATCH/RFC (version B)] gitweb: Allow UTF-8 encoded CGI query parameters and path_info
From: Michal Kiedrowicz @ 2012-02-03 7:39 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <201202022357.29569.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 2 Feb 2012, Jakub Narebski wrote:
> > On Thu, 2 Feb 2012, Michał Kiedrowicz wrote:
> > > Jakub Narebski <jnareb@gmail.com> wrote:
> > >
> > > > Gitweb tries hard to properly process UTF-8 data, by marking
> > > > output from git commands and contents of files as UTF-8 with
> > > > to_utf8() subroutine. This ensures that gitweb would print
> > > > correctly UTF-8 e.g. in 'log' and 'commit' views.
> > > >
> > > > Unfortunately it misses another source of potentially Unicode
> > > > input, namely query parameters. The result is that one cannot
> > > > search for a string containing characters outside US-ASCII.
> > > > For example searching for "Michał Kiedrowicz" (containing
> > > > letter 'ł' - LATIN SMALL LETTER L WITH STROKE, with Unicode
> > > > codepoint U+0142, represented with 0xc5 0x82 bytes in UTF-8 and
> > > > percent-encoded as %C5%81) result in the following incorrect
> > > > data in search field
> > > >
> > > > Michał Kiedrowicz
> > > >
> > > > This is caused by CGI by default treating '0xc5 0x82' bytes as
> > > > two characters in Perl legacy encoding latin-1 (iso-8859-1),
> > > > because 's' query parameter is not processed explicitly as
> > > > UTF-8 encoded string.
> > > >
> > > > The solution used here follows "Using Unicode in a Perl CGI
> > > > script" article on
> > > > http://www.lemoda.net/cgi/perl-unicode/index.html:
> > > >
> > > > use CGI;
> > > > use Encode 'decode_utf8;
> > > > my $value = params('input');
> > > > $value = decode_utf8($value);
> > > >
> > > > This is done when filling %input_params hash; this required to
> > > > move from explicit $cgi->param(<label>) to
> > > > $input_params{<name>} in a few places.
> > >
> > > I'm sorry but this doesn't work for me. I would be happy to help
> > > if you have some questions about it.
> >
> > Strange. http://www.lemoda.net/cgi/perl-unicode/index.html says
> > that those two approaches should be equivalent. The -utf8 pragma
> > version doesn't work for me at all, while this one works in that if
> > finds what it is supposed to, but shows garbage in search form.
>
> Is it what you mean by "this doesn't work for me", i.e. working
> search, garbage in search field?
I mean "garbage in search field". Search works even without the patch
(at least on Debian with git-1.7.7.3, perl-5.10.1 and CGI-3.43; I
don't have my notebook nearby at the moment to check).
>
> > Will investigate.
Thanks for your time spending on this. I wouldn't call this problem
"production critial" but it seems wrong to support UTF-8 everywhere
properly except for one place.
>
> Damn. If we use $cgi->textfield(-name => "s", -value => $searchtext)
> like in gitweb, CGI.pm would read $cgi->param("s") by itself -
> without decoding.
Makes sense. When I tried calling to_utf8() in the line that defines
textfield (this was my first approach to this problem), it haven't
changed anything.
> To skip this we need to pass -force=>1 or
> -override=>1 (i.e. further changes to gitweb).
>
> -utf8 pragma works with more modern CGI.pm, but does not with 3.10.
>
^ permalink raw reply
* [PATCH] Convert isatty() calls to git_isatty()
From: Nguyễn Thái Ngọc Duy @ 2012-02-03 8:35 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
isatty() is used to check for interactive use cases. However if pager is
set up, standard file handles may be redirected and istty() calls later
on no longer reflect the original state.
Convert isatty() calls to git_isatty() and allow git_isatty() to cache
tty info before pager is set up.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/commit.c | 2 +-
builtin/fsck.c | 2 +-
builtin/merge.c | 4 ++--
builtin/pack-objects.c | 2 +-
builtin/pack-redundant.c | 2 +-
builtin/prune-packed.c | 2 +-
builtin/prune.c | 2 +-
builtin/revert.c | 2 +-
builtin/shortlog.c | 4 ++--
builtin/unpack-objects.c | 2 +-
cache.h | 1 +
color.c | 2 +-
| 17 +++++++++++++++++
transport.c | 4 ++--
14 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index eba1377..cc72f13 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -672,7 +672,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
strbuf_addbuf(&sb, &message);
hook_arg1 = "message";
} else if (logfile && !strcmp(logfile, "-")) {
- if (isatty(0))
+ if (git_isatty(0))
fprintf(stderr, _("(reading log message from standard input)\n"));
if (strbuf_read(&sb, 0, 0) < 0)
die_errno(_("could not read log from standard input"));
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 8c479a7..0b4e8cf 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -637,7 +637,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, fsck_opts, fsck_usage, 0);
if (show_progress == -1)
- show_progress = isatty(2);
+ show_progress = git_isatty(2);
if (verbose)
show_progress = 0;
diff --git a/builtin/merge.c b/builtin/merge.c
index 62c7b68..49b9176 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -714,7 +714,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
o.renormalize = option_renormalize;
o.show_rename_progress =
- show_progress == -1 ? isatty(2) : show_progress;
+ show_progress == -1 ? git_isatty(2) : show_progress;
for (x = 0; x < xopts_nr; x++)
if (parse_merge_opt(&o, xopts[x]))
@@ -1129,7 +1129,7 @@ static int default_edit_option(void)
/* Use editor if stdin and stdout are the same and is a tty */
return (!fstat(0, &st_stdin) &&
!fstat(1, &st_stdout) &&
- isatty(0) &&
+ git_isatty(0) &&
st_stdin.st_dev == st_stdout.st_dev &&
st_stdin.st_ino == st_stdout.st_ino &&
st_stdin.st_mode == st_stdout.st_mode);
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 0f2e7b8..4468c84 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2328,7 +2328,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
if (!pack_compression_seen && core_compression_seen)
pack_compression_level = core_compression_level;
- progress = isatty(2);
+ progress = git_isatty(2);
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index f5c6afc..8197c9e 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -646,7 +646,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
/* ignore objects given on stdin */
llist_init(&ignore);
- if (!isatty(0)) {
+ if (!git_isatty(0)) {
while (fgets(buf, sizeof(buf), stdin)) {
sha1 = xmalloc(20);
if (get_sha1_hex(buf, sha1))
diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
index f9463de..498c6e9 100644
--- a/builtin/prune-packed.c
+++ b/builtin/prune-packed.c
@@ -71,7 +71,7 @@ void prune_packed_objects(int opts)
int cmd_prune_packed(int argc, const char **argv, const char *prefix)
{
- int opts = isatty(2) ? VERBOSE : 0;
+ int opts = git_isatty(2) ? VERBOSE : 0;
const struct option prune_packed_options[] = {
OPT_BIT('n', "dry-run", &opts, "dry run", DRY_RUN),
OPT_NEGBIT('q', "quiet", &opts, "be quiet", VERBOSE),
diff --git a/builtin/prune.c b/builtin/prune.c
index 58d7cb8..821772e 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -158,7 +158,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
}
if (show_progress == -1)
- show_progress = isatty(2);
+ show_progress = git_isatty(2);
if (show_progress)
progress = start_progress_delay("Checking connectivity", 0, 0, 2);
diff --git a/builtin/revert.c b/builtin/revert.c
index e6840f2..6f87c9b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -199,7 +199,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
int res;
memset(&opts, 0, sizeof(opts));
- if (isatty(0))
+ if (git_isatty(0))
opts.edit = 1;
opts.action = REPLAY_REVERT;
git_config(git_default_config, NULL);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 37f3193..c77b472 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -289,10 +289,10 @@ parse_done:
log.abbrev = rev.abbrev;
/* assume HEAD if from a tty */
- if (!nongit && !rev.pending.nr && isatty(0))
+ if (!nongit && !rev.pending.nr && git_isatty(0))
add_head_to_pending(&rev);
if (rev.pending.nr == 0) {
- if (isatty(0))
+ if (git_isatty(0))
fprintf(stderr, _("(reading log message from standard input)\n"));
read_from_stdin(&log);
}
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 14e04e6..4aaba69 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -501,7 +501,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)
git_config(git_default_config, NULL);
- quiet = !isatty(2);
+ quiet = !git_isatty(2);
for (i = 1 ; i < argc; i++) {
const char *arg = argv[i];
diff --git a/cache.h b/cache.h
index 9bd8c2d..4073fc9 100644
--- a/cache.h
+++ b/cache.h
@@ -1176,6 +1176,7 @@ extern void setup_pager(void);
extern const char *pager_program;
extern int pager_in_use(void);
extern int pager_use_color;
+extern int git_isatty(int);
extern const char *editor_program;
extern const char *askpass_program;
diff --git a/color.c b/color.c
index e8e2681..7151b48 100644
--- a/color.c
+++ b/color.c
@@ -183,7 +183,7 @@ int git_config_colorbool(const char *var, const char *value)
static int check_auto_color(void)
{
if (color_stdout_is_tty < 0)
- color_stdout_is_tty = isatty(1);
+ color_stdout_is_tty = git_isatty(1);
if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
char *term = getenv("TERM");
if (term && strcmp(term, "dumb"))
--git a/pager.c b/pager.c
index 975955b..a9380ab 100644
--- a/pager.c
+++ b/pager.c
@@ -72,12 +72,17 @@ const char *git_pager(int stdout_is_tty)
void setup_pager(void)
{
const char *pager = git_pager(isatty(1));
+ int i;
if (!pager)
return;
setenv("GIT_PAGER_IN_USE", "true", 1);
+ /* cache tty info */
+ for (i = 0; i <= 2; i++)
+ git_isatty(i);
+
/* spawn the pager */
pager_argv[0] = pager;
pager_process.use_shell = 1;
@@ -110,3 +115,15 @@ int pager_in_use(void)
env = getenv("GIT_PAGER_IN_USE");
return env ? git_config_bool("GIT_PAGER_IN_USE", env) : 0;
}
+
+int git_isatty(int fd)
+{
+ static int tty[3] = { -1, -1, -1 };
+
+ if (fd < 0 || fd > 2)
+ return isatty(fd);
+
+ if (tty[fd] == -1)
+ tty[fd] = isatty(fd);
+ return tty[fd];
+}
diff --git a/transport.c b/transport.c
index cac0c06..af48f7c 100644
--- a/transport.c
+++ b/transport.c
@@ -880,7 +880,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
const char *helper;
struct transport *ret = xcalloc(1, sizeof(*ret));
- ret->progress = isatty(2);
+ ret->progress = git_isatty(2);
if (!remote)
die("No remote provided to transport_get()");
@@ -997,7 +997,7 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
* 2. Don't report progress, if verbosity < 0 (ie. -q/--quiet ).
* 3. Report progress if isatty(2) is 1.
**/
- transport->progress = force_progress || (verbosity >= 0 && isatty(2));
+ transport->progress = force_progress || (verbosity >= 0 && git_isatty(2));
}
int transport_push(struct transport *transport,
--
1.7.8.36.g69ee2
^ permalink raw reply related
* Re: [RFC/PATCH git-remote-bzr] Adapt to new semantics of remote-helper "import" command
From: Gabriel Filion @ 2012-02-03 9:43 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Simon Poirier, Sverre Rabbelier, Jeff King, David Barr,
Dmitry Ivankov, Jelmer Vernooij
In-Reply-To: <20120122054657.GA25103@burratino>
Hello,
On 12-01-22 12:46 AM, Jonathan Nieder wrote:
> Hi Simon and Gabriel,
>
> Here's a rough patch against git://github.com/lelutin/git-remote-bzr.git
> master.
great! thanks for your help.
I must admit that this project never got to completion and is now
getting quite the low priority for my part. I'm no longer working for
the company that was using mainly Bazaar as their VCS, and I've been a
happy git-only user for some time now.
So, I don't have the same incentive to complete the project as I had before.
But I'd be happy to see this get to a point where it's working fine.
> Without this patch, whenever I try to use "git clone bzr::<something>",
> after doing all the work it removes the resulting repo and exits with
> status 141 (SIGPIPE). Maybe the transport-helper should mask SIGPIPE
> when writing the final newline to avoid that.
>
> I'd have prefered to write a patch for remote-bzr that works with
> older versions of git fast-import, too, but it wasn't obvious how.
> Hints welcome.
hmm.. I can wait some time to see if some ideas come out around this,
and commit your patch as-is if there are no comments/reworks.
> BTW, would you mind if I sent a patch to include git-remote-bzr in
> git.git under contrib/?
absolutely not, that'd be great actually :)
I didn't do that up to now, though, since I bumped into so much bugs
that I couldn't work out -- some very bad performance issues, and
problems with handling mark files with bzr-fastimport.
> Thanks for git remote-bzr! I'd be happy for any thoughts you have.
The idea behind git-remote-bzr was to be able to interact with Bazaar
from within your git repository, i.e. to expose remote branches that you
can pull from and push to using the default git commands, without having
to learn to use yet another tool since the remote-helper would be
interfacing with the tool for you.
I have dived for a short period of time into bzrlib, the python library
behind Bazaar, to see how much work it would take to put together a
simplified fast-import client.. and .... wechrk!
It was a huge maze of version-dependant code (the API undergoes big
changes frequently, and backwards compatibility is maintained pretty
far) mixed with a 40-foot-deep class hierarchy. So I quickly gave up on
that idea..
IIRC, at the time I started work on this, Simon and I used
bzr-fastimport[1] because we were able to get farther with this tool. I
haven't used bzr-git[2] that much, though, so I can't comment too
extensively on it. But I would guess that it is maintained more
frequently than bzr-fastimport is, so it could be a better choice for
the backend fast-import client.
I would guess that Jelmer would be happy to help out with interfacing
with bzr-git.
--
Gabriel Filion
^ permalink raw reply
* Re: [PATCH] Convert isatty() calls to git_isatty()
From: Johannes Sixt @ 2012-02-03 9:48 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1328258101-10636-1-git-send-email-pclouds@gmail.com>
Am 2/3/2012 9:35, schrieb Nguyễn Thái Ngọc Duy:
> isatty() is used to check for interactive use cases. However if pager is
> set up, standard file handles may be redirected and istty() calls later
> on no longer reflect the original state.
So what? What's wrong with this behavior?
You converted many cases involving progress indicators. Wouldn't the new
code pipe progress output to the pager where earlier it was not shown if a
pager was present? That is plainly wrong: Progress output is destined only
for the terminal, not for the pager.
-- Hannes
^ permalink raw reply
* Re: [PATCH] Convert isatty() calls to git_isatty()
From: Nguyen Thai Ngoc Duy @ 2012-02-03 9:59 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <4F2BAD8B.1080403@viscovery.net>
2012/2/3 Johannes Sixt <j.sixt@viscovery.net>:
> Am 2/3/2012 9:35, schrieb Nguyễn Thái Ngọc Duy:
>> isatty() is used to check for interactive use cases. However if pager is
>> set up, standard file handles may be redirected and istty() calls later
>> on no longer reflect the original state.
>
> So what? What's wrong with this behavior?
>
> You converted many cases involving progress indicators. Wouldn't the new
> code pipe progress output to the pager where earlier it was not shown if a
> pager was present? That is plainly wrong: Progress output is destined only
> for the terminal, not for the pager.
Yeah right. My mistake.
--
Duy
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-02-03 10:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, SZEDER Gábor
In-Reply-To: <7vhaz8vkhd.fsf@alter.siamese.dyndns.org>
On Fri, Feb 3, 2012 at 2:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Feb 2, 2012 at 9:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>>> However, clearly I did not say it clearly enough. :) I guess it's
>>>> better to take a cue from storytellers and show rather than tell.
>>>
>>> Very big thanks for this ;-)
>>
>> Not a single comment regarding what I said?
>
> What entitles you to force me to refraining from commenting at all until I
> read everything in my mailbox and after waiting for a while to make sure
> there is no more to come to the thread?
Fair enough. Just wondering.
> In any case, "be nicer with zsh" conveys no more meaningful information
> than "this is some patch about zsh".
And that already tells you a lot more than other alternatives.
> Let's try to avoid warm and fuzzy
> words that imply "goodness", e.g. "improve" and "be nicer with" because
> nobody sends a patch to purposefully make Git worse and expects it to be
> applied.
True. Which why I listened to the suggestion from Thomas Rast and
didn't use that, but "completion: work around zsh option propagation
bug" instead.
> I found Jonathan's alternative "avoid default value assignment on : true
> command" at least a bit better for the purpose of jogging the short-term
> memory in the "'git shortlog v1.7.9.. contrib/completion/' tells us that
> we have applied several patches, and I remember that : ${var=word} one!"
> sense. It is not super-useful for the longer term, though.
>
> Here is what I ended up in preparation for queuing the series. I still
> haven't seen any version of 4/4, but please check $gmane/189683 and see if
> that matches what you intended. Also I am assuming $gmane/189606 relayed
> by Jonathan is a squash between your 2 and 3 (which didn't reach me), so
> please advise if that does not match what you want to have.
This is getting ridiculous, now I sent the patches directly to you, is
your pobox.com server also silently dropping them for no reason? I
think this is totally counter-productive. I haven't received any reply
from the vger postmaster, but I guess you should be able to find out
why your host is dropping mails. Am I the only one that has such
issues?
Anyway. I have uploaded all the mails to here:
http://people.freedesktop.org/~felipec/git-patches/
As for $gmane/189683, the changes seem to be correct, but I still
prefer my commit message[1]--which I have written and rewritten many
times now to improve it.
Regarding $gmane/189606, I still prefer my commit message[2], because
it starts with the *purpose* of the patch. As for the changes, they
are correct, and I don't mind squashing them, but they are *two*
logically independent changes; imagine in the future somebody
wants/need to re-enable __git_shopt, well, all they have to do is
revert the second patch. But that's up to you.
Cheers.
[1] http://people.freedesktop.org/~felipec/git-patches/4
[2] http://people.freedesktop.org/~felipec/git-patches/2
--
Felipe Contreras
^ permalink raw reply
* Re: [RFC/PATCH 0/2] Commits with ancient timestamps
From: Thomas Rast @ 2012-02-03 10:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1328218903-5681-1-git-send-email-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> avoid misinterpreting human-written timestamp in other formats, and
> timestamps before 1975 do not have enough number of digits in them.
>
> Here is a two-patch series that may improve the situation.
Doing this just makes me wonder how important exactly the 1970-1975
range is. Is there a notable software history from that era that can be
recovered?
(Your [1/2] does not seem to parse negative offsets from the unix epoch,
so anything before 1970 is still out.)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: Alternates corruption issue
From: Jeff King @ 2012-02-03 12:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, Richard Purdie, GIT Mailing-list, Hart, Darren,
Ashfield, Bruce
In-Reply-To: <7vzkd0u4ik.fsf@alter.siamese.dyndns.org>
On Thu, Feb 02, 2012 at 04:47:31PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > @@ -324,8 +324,11 @@ const char *enter_repo(const char *path, int strict)
> > return NULL;
> > len = strlen(used_path);
> > for (i = 0; suffix[i]; i++) {
> > + struct stat st;
> > strcpy(used_path + len, suffix[i]);
> > - if (!access(used_path, F_OK)) {
> > + if (!stat(used_path, &st) &&
> > + (S_ISREG(st.st_mode) ||
> > + (S_ISDIR(st.st_mode) && is_git_directory(used_path)))) {
>
> Hmm, how would this change interact with
>
> > strcat(validated_path, suffix[i]);
> > break;
> > }
>
> gitfile = read_gitfile(used_path);
>
> that appear after the context in the patch?
It assumes that any file named ".git" is worth reading and selecting.
And then later we actually read_gitfile to find out if it's worth-while.
There is no change of behavior from before the patch, as we would
similarly notice the file (without checking if it's a real gitfile) and
then later read it and possibly fail.
However, with the ordering change, there is a technically a regression
in one case: a random file "foo" next to a repo "foo.git". Saying "git
ls-remote foo" used to prefer "foo.git", and will now select the file
"foo" only to fail.
The code-path in clone's get_repo_path handles this properly (it checks
that the path is really a valid gitfile before finishing the loop). The
gitfile-reading from later in enter_repo could be hoisted into the loop.
If was trying to make a less-invasive change; if we're going to do that
much rewriting, it probably makes sense to factor out the logic from
get_repo_path and have them share the code.
Thanks for noticing. I saw this issue when I was writing the original
version of the patch, and meant to revisit it and at least document it
in the commit message, but I ended up forgetting.
-Peff
^ permalink raw reply
* Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug
From: Jeff King @ 2012-02-03 12:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ben Walton, git
In-Reply-To: <7vr4ycu3ty.fsf@alter.siamese.dyndns.org>
On Thu, Feb 02, 2012 at 05:02:17PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I wonder if a better solution is to use a known-good shell instead of
> > trying to work around problems in a bogus shell.
>
> Yeah, I think that is a better approach.
>
> What prevents us from doing 's|^#! */bin/sh|$#$SHELL_PATH|' on everything
> in t/ directory (I am not suggesting to do this. I just want to know if
> there is a reason we want hardcoded "#!/bin/sh" for some instances).
The quoting is more annoying, because you usually don't want
interpolation on the rest of the lines of your embedded script. So:
cat >foo.sh <<\EOF
#!/bin/sh
echo my arguments are "$@"
EOF
cannot have the mechanical replace you mentioned above. It would need:
cat >foo.sh <<EOF
#!$SHELL_PATH
echo my arguments are "\$@"
EOF
or:
{
echo "#!$SHELL_PATH" &&
cat <<EOF
echo my arguments are "$@"
EOF
} >foo.sh
When I have hard-coded "#!/bin/sh", my thinking is usually "this is less
cumbersome to type and to read, and this script-let is so small that
even Solaris will get it right".
-Peff
^ permalink raw reply
* Re: General support for ! in git-config values
From: Jeff King @ 2012-02-03 12:09 UTC (permalink / raw)
To: Kyle Moffett
Cc: demerphq, Junio C Hamano, Ævar Arnfjörð,
Git Mailing List
In-Reply-To: <CAGZ=bq++R+X+2r2_zQ4UZ6JvDC9W9_4nF23MQ6+612_Qe2RS4Q@mail.gmail.com>
On Thu, Feb 02, 2012 at 09:08:46PM -0800, Kyle Moffett wrote:
> > I understand. I think we will probably use backtick quoting in git-deploy. So
> >
> > deploy.prefix=`cat /etc/SERVER_ROLE`
> >
> > will execute cat /etc/SERVER_ROLE and use the results as the value of
> > the config option.
>
> Alternatively, you could extend the recent proposal for GIT config
> "include" statements so that something like this works:
>
> [include]
> exec = echo "deploy.prefix = `cat /etc/SERVER_ROLE`"
> exec = /usr/local/bin/git-config-for-ldap-user
>
> Thoughts?
I am still undecided on whether the utility of the idea is worth the
potential hassle, but syntactically I like that better. It does put a
little more burden on the called program to handle things like quoting,
though.
-Peff
^ permalink raw reply
* Re: General support for ! in git-config values
From: Jeff King @ 2012-02-03 12:13 UTC (permalink / raw)
To: Junio C Hamano
Cc: Kyle Moffett, demerphq, Ævar Arnfjörð,
Git Mailing List
In-Reply-To: <7vmx90say8.fsf@alter.siamese.dyndns.org>
On Thu, Feb 02, 2012 at 10:11:27PM -0800, Junio C Hamano wrote:
> Kyle Moffett <kyle@moffetthome.net> writes:
>
> > Alternatively, you could extend the recent proposal for GIT config
> > "include" statements so that something like this works:
> >
> > [include]
> > exec = echo "deploy.prefix = `cat /etc/SERVER_ROLE`"
> > exec = /usr/local/bin/git-config-for-ldap-user
>
> Erh...
>
> Running known stuff from your own .git/config may be justifiable as "at
> your own risk", but if we consider sources that are not under your direct
> control, such as /etc/gitconfig and whatever your project encourages you
> to include from your .git/config,... eek.
For normal use, I don't see this as a big deal. They could also be
specifying diff.external, which would run arbitrary code (and who
doesn't run "git diff" once in a while?).
I see it as a bigger issue for sites which serve repositories on behalf
of their users, and already take care never to use porcelain commands
which will run arbitrary code from the config by default (e.g., gitweb
carefully uses diff plumbing for this reason). Introducing such an
option provides a mechanism for users who control the config of the
served repositories to execute code as the user running git-daemon or
gitweb.
-Peff
^ permalink raw reply
* Re: [PATCH v2] Use correct grammar in diffstat summary line
From: Jeff King @ 2012-02-03 12:24 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Junio C Hamano, git, Jonathan Nieder,
Ævar Arnfjörð, Frederik Schwarzer, Brandon Casey
In-Reply-To: <CACsJy8CcBB9OF=8a1hQEpDFzqsrkbpFKnVAcU65h_5Cnym90SQ@mail.gmail.com>
On Fri, Feb 03, 2012 at 08:11:23AM +0700, Nguyen Thai Ngoc Duy wrote:
> >> I take it --summary is un-i18n-able,...
> >
> > ... because?
>
> .. of scripts? We have --numstat for scripts, but I see no alternative
> to --summary. Does anybody parse --summary output?
I would think it is spelled "--raw".
-Peff
^ permalink raw reply
* Re: Breakage in master?
From: Erik Faye-Lund @ 2012-02-03 12:28 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, msysGit, Ævar Arnfjörð
In-Reply-To: <20120202174601.GB30857@sigill.intra.peff.net>
On Thu, Feb 2, 2012 at 6:46 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 02, 2012 at 01:14:19PM +0100, Erik Faye-Lund wrote:
>
>> But here's the REALLY puzzling part: If I add a simple, unused
>> function to diff-lib.c, like this:
>> [...]
>> "git status" starts to error out with that same vsnprintf complaint!
>>
>> ---8<---
>> $ git status
>> # On branch master
>> # Changes not staged for commit:
>> # (use "git add <file>..." to update what will be committed)
>> fatal: BUG: your vsnprintf is broken (returned -1)
>> ---8<---
>
> OK, that's definitely odd.
>
> At the moment of the die() in strbuf_vaddf, what does errno say?
If I apply this patch:
---8<---
diff --git a/strbuf.c b/strbuf.c
index ff0b96b..52dfdd6 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -218,7 +218,7 @@ void strbuf_vaddf(struct strbuf *sb, const char
*fmt, va_list ap)
len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, cp);
va_end(cp);
if (len < 0)
- die("BUG: your vsnprintf is broken (returned %d)", len);
+ die_errno("BUG: your vsnprintf is broken (returned %d)", len);
if (len > strbuf_avail(sb)) {
strbuf_grow(sb, len);
len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
---8<---
Then I get "fatal: BUG: your vsnprintf is broken (returned -1): Result
too large". This goes both for both failure cases I described. I
assume this means errno=ERANGE.
> vsnprintf should generally never be returning -1 (it should return the
> number of characters that would have been written). Since you're on
> Windows, I assume you're using the replacement version in
> compat/snprintf.c.
No. SNPRINTF_RETURNS_BOGUS is only set for the MSVC target, not for
the MinGW target. I'm assuming that means MinGW-runtime has a sane
vsnprintf implementation. But even if I enable SNPRINTF_RETURNS_BOGUS,
the problem occurs. And it's still "Result too large".
So I decided to do a bit of stepping, and it seems libintl takes over
vsnprintf, directing us to libintl_vsnprintf instead. I guess this is
so it can ensure we support reordering the parameters with $1 etc...
And aparently this vsnprintf implementation calls the system vnsprintf
if the format string does not contain '$', and it's using _vsnprintf
rather than vsnprintf on Windows. _vsnprintf is the MSVCRT-version,
and not the MinGW-runtime, which needs SNPRINTF_RETURNS_BOGUS.
So I guess I can patch libintl to call vsnprintf from MinGW-runtime instead.
> All of that would make sense to me, _except_ for your weird "if I add a
> random function, the problem is more reproducible" bit. Which does seem
> like something is invoking undefined behavior (of course, it could be
> that undefined behavior or stack-smashing that is causing vsnprintf to
> report an error). Lacking any better leads, it might be worth pursuing.
Well, now at least I have some better leads, but I'm still not able to
explain the "if I add a random function, the problem is more
reproducible" bit. :(
>> I've bisected the issues down to 5e9637c (i18n: add infrastructure for
>> translating Git with gettext). Trying to apply my unused-function
>> patch on top of this commit starts giving the same "fatal: BUG: your
>> vsnprintf is broken (returned -1)" error. It's ancestor, bc1bbe0(Git
>> 1.7.8-rc2), does not yield any of the issues.
>
> I've looked at 5e9637c, and it really doesn't do anything that looks
> bad. I wonder if your gettext library is buggy. Does compiling with
> NO_GETTEXT help?
Compiling with NO_GETTEXT does make the symptoms go away, but that's
not curing the problem ;)
But, I have a lead now. I'll see if I can find out *why* libintl calls
_vsnprintf on MinGW. I expect it's so the MSVC and the MinGW versions
behave similarly, MSVC doesn't have a sane vsnprintf. Perhaps I should
back-port SNPRINTF_RETURNS_BOGUS-workaround to libintl, so our MSVC
builds doesn't break also?
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox