* Re: [PATCH v5 2/5] gitweb: add project_filter to limit project list to a subdirectory
From: Junio C Hamano @ 2012-02-01 20:55 UTC (permalink / raw)
To: Bernhard R. Link; +Cc: Jakub Narebski, git
In-Reply-To: <20120201165902.GA14706@server.brlink.eu>
"Bernhard R. Link" <brl+git@mail.brlink.eu> writes:
> Look liks a change like that is actually needed...
> ... So it looks for forks in a directory named '1'
Yeah, that was exactly what was causing failures in 9502. Fixed locally
so no further action is required.
Thanks.
^ permalink raw reply
* Re: General support for ! in git-config values
From: Ævar Arnfjörð Bjarmason @ 2012-02-01 21:25 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20120201184020.GA29374@sigill.intra.peff.net>
On Wed, Feb 1, 2012 at 19:40, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 01, 2012 at 06:33:47PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> For a program I'm working on (git-deploy) I'd like to have this as a
>> general facility, i.e. users can specify either:
>>
>> foo.bar = value
>>
>> Or:
>>
>> foo.bar = !cat /some/path
>>
>> I'm wondering why git-config doesn't do this already, if there's no
>> reason in particular I can just patch it in, either as a new option:
>>
>> git config --with-exec --get foo.bar
>
> I'm not clear on what you want --with-exec to do. By default, config
> values are strings. I would expect the "!" to be a special marker that
> the caller would recognize in the string, and then act appropriately.
>
> So if I were implementing git aliases in the shell, the code would look
> like:
>
> v=$(git config alias.$alias)
> case "$v" in
> "")
> die "no such alias: $alias" ;;
> "!*)
> cmd="${v#!}" ;;
> *)
> cmd="git $v" ;;
> esac
> eval "$cmd"
>
> I.e., everything pertaining to "!" happens after we get the config
> string. So what is it that you want "git config --with-exec" to do?
I agree that that's how it should work, I just suggested --with-exec
in case anyone complained about the backwards compatibility issue of
changing the meaning of "!" for existing configs.
^ permalink raw reply
* Re: [PATCH v2] Use correct grammar in diffstat summary line
From: Junio C Hamano @ 2012-02-01 21:26 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Thomas Dickey, Jonathan Nieder, Ævar Arnfjörð,
Frederik Schwarzer, Brandon Casey, dickey
In-Reply-To: <1328100907-20397-1-git-send-email-pclouds@gmail.com>
Nice. Will queue with a minor update to the log message:
commit 3f29ab34372ee11946439da3bde307eb90ad9031
Author: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date: Wed Feb 1 19:55:07 2012 +0700
Use correct grammar in diffstat summary line
"git diff --stat" and "git apply --stat" now learn to print the line
"%d files changed, %d insertions(+), %d deletions(-)" in singular form
whenever applicable. "0 insertions" and "0 deletions" are also omitted
unless they are both zero.
This matches how versions of "diffstat" that are not prehistoric produced
their output, and also makes this line translatable.
[jc: with help from Thomas Dickey in archaeology of "diffstat"]
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
And also this bit on top:
diff --git a/diff.c b/diff.c
index 5c31b36..5f3ce97 100644
--- a/diff.c
+++ b/diff.c
@@ -1329,7 +1329,7 @@ int print_stat_summary(FILE *fp, int files, int insertions, int deletions)
if (!files) {
assert(insertions == 0 && deletions == 0);
- return fputs(_(" no changes\n"), fp);
+ return fputs(_(" 0 files changed\n"), fp);
}
strbuf_addf(&sb,
^ permalink raw reply related
* rebase -i reword converts to pick on pre-commit non-zero exit
From: Neal Kreitzinger @ 2012-02-01 21:27 UTC (permalink / raw)
To: git
When interactive rebase does the reword command it runs the pre-commit hook
for that commit (a). If the pre-commit hook gives a non-zero exit then
interactive rebase picks commit (a) and continues to the next commit (b) in
the rebase-to-do-list. Instead of picking commit (a) when the pre-commit
hook exits non-zero on the reword command, shouldn't interactive rebase
learn to edit commit (a) and tell the user that because the pre-commit hook
exited non-zero they need to either remedy the pre-commit hook violations
and run git commit --amend or run git commit --amend --no-verify to bypass
the pre-commit hook? Otherwise, you have to run another rebase after the
rejected rewords and edit those commits to accomplish the rewords.
v/r,
neal
^ permalink raw reply
* Re: rebase -i reword converts to pick on pre-commit non-zero exit
From: Neal Kreitzinger @ 2012-02-01 21:28 UTC (permalink / raw)
To: git; +Cc: git
In-Reply-To: <jgcaoh$d9q$1@dough.gmane.org>
On 2/1/2012 3:27 PM, Neal Kreitzinger wrote:
> When interactive rebase does the reword command it runs the pre-commit hook
> for that commit (a). If the pre-commit hook gives a non-zero exit then
> interactive rebase picks commit (a) and continues to the next commit (b) in
> the rebase-to-do-list. Instead of picking commit (a) when the pre-commit
> hook exits non-zero on the reword command, shouldn't interactive rebase
> learn to edit commit (a) and tell the user that because the pre-commit hook
> exited non-zero they need to either remedy the pre-commit hook violations
> and run git commit --amend or run git commit --amend --no-verify to bypass
> the pre-commit hook? Otherwise, you have to run another rebase after the
> rejected rewords and edit those commits to accomplish the rewords.
>
git 1.7.1
v/r,
neal
^ permalink raw reply
* rebase -i reword runs pre-commit hook with curious results
From: Neal Kreitzinger @ 2012-02-01 21:50 UTC (permalink / raw)
To: git
I'm confused on why and/or how interactive rebase runs the pre-commit hook
when doing the reword command for commit (a). My pre-commit hook does
keyword expansion on the worktree copy of the modified index files and then
re-adds them to effect a user-date-stamp when committed. However, the
user-date-stamps don't get updated when reword runs the pre-commit hook.
IOW, the pre-commit hook does not get the same results as if I were doing a
commandline git-commit of a modified index. I suppose reword is protecting
the preservation of all the contents of commit (a) except the commit message
which makes sense, but I don't understand how it goes about doing this while
still attempting to somehow honor the pre-commit hook. (git 1.7.1)
v/r,
neal
^ permalink raw reply
* AW: Project structure of .NET-Projects using git submodule or something different
From: Harald Heigl @ 2012-02-01 21:07 UTC (permalink / raw)
To: git
In-Reply-To: <4F29A0BE.8000803@web.de>
Hi, thanks for your answer!
> -----Ursprüngliche Nachricht-----
> Von: Jens Lehmann [mailto:Jens.Lehmann@web.de]
> Gesendet: Mittwoch, 01. Februar 2012 21:30
> An: Harald Heigl
> Cc: git@vger.kernel.org
> Betreff: Re: Project structure of .NET-Projects using git submodule or
> something different
>
> Am 31.01.2012 23:41, schrieb Harald Heigl:
> > Let's assume following Project structure (Dependencies and
> Subdependencies
> > are submodules and submodules of the submodules)
> > Project
> > Dependency 1
> > Dependency 2
> > Dependency 3
> > Dependency 4
> > Dependency 2
> >
> >
> > The problem is if I want to build them I need to build 2+3, then 1, 4
and 2
> > again and then the project. As you may see project 2 is a submodule of
> > dependency 1 and also of project. I don't feel comfortable with this
setup.
> > What do you think?
>
> Hmm, we try to avoid that kind of setup as having checked out different
> versions of the "Dependency 2" submodule could have rather surprising
> effects. We get along really well with "Dependency 2" only being present
> in the superproject and having "Dependency 1" reference that instead of
> having its own copy (So we have submodules which are depending on having
> other submodules right next to them). Then the superproject is responsible
> for tying it all together.
I think you're right, my first thoughts were that if I start a new project I
just "git submodule dependency1" and get all the required dependencies and
the dependencies within the dependencies and so on ... .
With your solution I "git submodule dependency1" and have to think about the
dependencies it depends on. On the other hand we are just a small company
and the number of submodules is not too big and the missing references in a
new project would be easily identifiable, so ... .
And if I want to checkout dependency 1 individually (for whatever reason), I
could still do something like this:
SuperDependency1 (with solution-File)
Dependency1 (as submodule)
Dependency2 (dependency of dependency1 - as submodule)
Dependency3 (dependency of dependency1 - as submodule)
Thanks again, I see my concept causes some trouble ...
Any other thoughts or other workflows with git or with tools build around
git?
Thanks again,
Harald
^ permalink raw reply
* Re: logging disjoint sets of commits in a single command
From: Bryan O'Sullivan @ 2012-02-01 22:01 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git@vger.kernel.org
In-Reply-To: <20120201030300.GA9969@sigill.intra.peff.net>
On 2012-01-31 19:03 , "Jeff King" <peff@peff.net> wrote:
>
>That sounds kind of slow. Is your repository really gigantic?
Bigger than a kernel tree, but not as many commits. Beyond that, can't say.
> Have you packed
>everything?
Yep.
> I'm just curious if there's some other way to make things
>faster.
It would be nice if there was, but the fundamental problem is the lack of
an index from filename to commit. Walking every commit is the limiting
factor, I believe.
> Is the repository publicly available?
No, sorry.
^ permalink raw reply
* Re: [PATCH v3 1/2] Factor find_pack_entry()'s core out
From: Junio C Hamano @ 2012-02-01 22:03 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <alpine.LFD.2.02.1202011056140.2759@xanadu.home>
Nicolas Pitre <nico@fluxnic.net> writes:
>> +static int find_pack_entry_1(const unsigned char *sha1,
>> + struct packed_git *p, struct pack_entry *e)
>
> This looks all goot but the name. Pretty please, try to find something
> that is more descriptive than "1". Suggestions:
> "find_pack_entry_lookup", "find_pack_entry_inner", etc.
Perhaps "find_pack_entry_in_pack(sha1, e, p)"?
That would go well with the caller "find_pack_entry(sha1, e)".
^ permalink raw reply
* Re: [PATCH] Don't search files with an unset "grep" attribute
From: Jeff King @ 2012-02-01 22:14 UTC (permalink / raw)
To: Conrad Irwin; +Cc: Junio C Hamano, git, Nguyen Thai Ngoc Duy, Dov Grobgeld
In-Reply-To: <CAOTq_ptj06aNGsQRjV0fVRxnQFBHmU2FFSXwWDUUk9MM77k2LQ@mail.gmail.com>
On Wed, Feb 01, 2012 at 01:28:47AM -0800, Conrad Irwin wrote:
> > But there's more. Respecting binary attributes does mean looking up
> > attributes for _every_ file. And that has a noticeable impact. My
> > best-of-five for "git grep foo" on linux-2.6 went from 0.302s to 0.392s.
> > Yuck.
>
> The first time I introduced this behaviour[1], I made it conditional
> on a preference — those who wanted "good" grep could set the
> preference, while those who wanted "fast" grep could not. I think
> that's not a good idea, though if the performance issues are
> show-stoppers, I'd suggest the opposite preference (so speed-freaks
> can disable the checks).
I've been able to get somewhat better performance by hoisting the
attribute lookup into the parent thread. That means it happens in order
(which lets the attr code's stack optimizations work), and there's no
lock contention.
I'll post finished patches with numbers in a few minutes.
> Tests from [1] included below in case they're still useful (they pass
> with your change)
Thanks, I'll include them.
-Peff
^ permalink raw reply
* Re: General support for ! in git-config values
From: Junio C Hamano @ 2012-02-01 22:21 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Jeff King, Git Mailing List
In-Reply-To: <CACBZZX5mX55Rh8b2GYv7wKbCCypCkrn5AiM9BpXydgcYxHWdQA@mail.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> I.e., everything pertaining to "!" happens after we get the config
>> string. So what is it that you want "git config --with-exec" to do?
>
> I agree that that's how it should work, I just suggested --with-exec
> in case anyone complained about the backwards compatibility issue of
> changing the meaning of "!" for existing configs.
Now you made me utterly confused.
What "backwards compatibility" issue do you have in mind? If I name
myself '!me' with "user.name = !me", do I suddenly break backwards
compatibility of Git with previous versions somehow? If so how?
The --with-exec option you talk about seems to me the option about the
backward compatibility of the _calling script_ of "git config". The old
version of git-blorl script may have used foo.bar as a mere string, but a
new version of it may (optionally) interpret foo.bar's value in a special
way when it begins with a "!", introducing a backward compatibility issues
to git-blorl script, and users who want to take advantage of that feature
may want to run it as "git-blorl --with-exec", and the relevant part of
the "git-blorl" script might look like this:
foo_bar=$(git config foo.bar)
case "$with_exec,$foo_bar" in
yes,\!*)
foo_bar=$(eval ${foo_bar#\!}) ;;
esac
... then do something interesting using $foo_bar ...
^ permalink raw reply
* Request for git-svn man page
From: Dr. Lars Hanke @ 2012-02-01 22:32 UTC (permalink / raw)
To: git
Please add instructions to the git-svn man page concerning cloning a SVN
repositiory through a proxy. In particular make clear that also the SVN
software has to be configured to use the proxy. See e.g.
http://stackoverflow.com/questions/9057111/git-svn-ignores-http-proxy-debian.
I'd prepare a patch, but I have no clue about SVN, so I'm probably not
the best choice for writing a SVN related manual. ;)
Kind regards,
- lars.
^ permalink raw reply
* Re: [PATCH v3 1/2] Factor find_pack_entry()'s core out
From: Junio C Hamano @ 2012-02-01 22:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nicolas Pitre, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <7vaa522oum.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Nicolas Pitre <nico@fluxnic.net> writes:
>
>>> +static int find_pack_entry_1(const unsigned char *sha1,
>>> + struct packed_git *p, struct pack_entry *e)
>>
>> This looks all goot but the name. Pretty please, try to find something
>> that is more descriptive than "1". Suggestions:
>> "find_pack_entry_lookup", "find_pack_entry_inner", etc.
>
> Perhaps "find_pack_entry_in_pack(sha1, e, p)"?
> That would go well with the caller "find_pack_entry(sha1, e)".
I amended 1/2 (and adjusted 2/2 to match) to call it fill_pack_entry.
Will push out the result tonight.
^ permalink raw reply
* Re: [RFC/PATCH] add update to branch support for "floating submodules"
From: Jens Lehmann @ 2012-02-01 22:37 UTC (permalink / raw)
To: Phil Hord; +Cc: Junio C Hamano, Leif Gruenwoldt, git
In-Reply-To: <CABURp0pSGGT8eyzNad-dNNx49oioAxOPOf3dmqu7M3fnV+PzdA@mail.gmail.com>
Am 31.01.2012 23:50, schrieb Phil Hord:
> What I mean is that a developer may be completely focused on one
> particular submodule (his domain). He does his work in this module,
> and when it's ready he commits and pushes to the server. 'git status'
> shows him that his directory is clean. But this is only because he
> doesn't really know where the submodules top-directories are, so he
> doesn't realize that he has changes in another submodule that he has
> not committed. He has to know to run 'git status' from somewhere in
> the superproject (ostensibly in the root directory of that
> superproject). But he may forget since 'git status' already assured
> him he was done.
<snip>
> I guess what would help here is something like the opposite of 'git
> status' showing the status of descendant submodules; it would help if
> it showed the status of sibling submodules and the superproject as
> well.
Hmm, I really think the fact that submodules are unaware that they
are part of a superproject is a feature. I'd prefer seeing that kind
of problem being tackled by the CI server and/or user education. Or
maybe a pre-commit hook which issues a warning in that case?
^ permalink raw reply
* Re: Bug report: stash in upstream caused remote fetch to fail
From: Andrew Walrond @ 2012-02-01 22:37 UTC (permalink / raw)
To: Thomas Rast; +Cc: git
In-Reply-To: <874nvap9hj.fsf@thomas.inf.ethz.ch>
On Wed, Feb 01, 2012 at 09:46:48PM +0100, Thomas Rast wrote:
>
> Do you, by any chance, still have a copy of the upstream repo before you
> trashed the stash? It would be interesting to know whether there was
> actually some repository corruption going on (that went unnoticed by
> fsck, no less) or if there was a bug in the transmission.
I tried to reproduce the problem but without success :(
Disk corruption is unlikely - raided drives etc etc. More likely stash
related somehow as I was applying patches manually on older versions
checked out in a temp branch, stashing the results then applying the
stash onto master/HEAD.
But why was a remote clone involved with the upstream stash at all I
wonder??
Andrew Walrond
^ permalink raw reply
* [RFC PATCH] gitweb: use CGI with -utf8
From: Michał Kiedrowicz @ 2012-02-01 22:50 UTC (permalink / raw)
To: git; +Cc: Michał Kiedrowicz
I noticed that gitweb tries a lot to properly process UTF-8 data, for
example it prints my name correctly in log and commit information, but
it echos junk in the search field. It looks like:
Michał Kiedrowicz
I don't know CGI well and I never touched gitewb code, but I found this
on http://www.lemoda.net/cgi/perl-unicode/index.html:
use CGI '-utf8';
my $value = params ('input');
I tried it and that fixed my problem. I'm not sure about the
consequences, maybe someone more experienced in CGI might help?
---
gitweb/gitweb.perl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index abb5a79..74d45b1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -10,7 +10,7 @@
use 5.008;
use strict;
use warnings;
-use CGI qw(:standard :escapeHTML -nosticky);
+use CGI qw(:standard :escapeHTML -nosticky -utf8);
use CGI::Util qw(unescape);
use CGI::Carp qw(fatalsToBrowser set_message);
use Encode;
--
1.7.3.4
^ permalink raw reply related
* Re: [PATCH] Don't search files with an unset "grep" attribute
From: Jeff King @ 2012-02-01 23:20 UTC (permalink / raw)
To: Conrad Irwin; +Cc: Junio C Hamano, git, Nguyen Thai Ngoc Duy, Dov Grobgeld
In-Reply-To: <20120201221437.GA19044@sigill.intra.peff.net>
On Wed, Feb 01, 2012 at 05:14:37PM -0500, Jeff King wrote:
> > The first time I introduced this behaviour[1], I made it conditional
> > on a preference — those who wanted "good" grep could set the
> > preference, while those who wanted "fast" grep could not. I think
> > that's not a good idea, though if the performance issues are
> > show-stoppers, I'd suggest the opposite preference (so speed-freaks
> > can disable the checks).
>
> I've been able to get somewhat better performance by hoisting the
> attribute lookup into the parent thread. That means it happens in order
> (which lets the attr code's stack optimizations work), and there's no
> lock contention.
>
> I'll post finished patches with numbers in a few minutes.
OK, here they are. After playing with some options, I'm satisfied this
is a sane way to do it. I don't think it's worth having a config option.
There is a measurable slowdown, but it's simply not that big.
[1/2]: grep: let grep_buffer callers specify a binary flag
[2/2]: grep: respect diff attributes for binary-ness
There are a few optimizations I didn't do that you could put on top:
1. When "-a" is given, we can avoid the attribute lookup altogether.
2. When "-I" is given, we can actually check attributes _before_
loading the file or blob into memory. This can help with very large
binaries.
3. When "-I" is given but we have no attribute, we can stream the
beginning of the file or blob to check for binary-ness, and then
avoid loading the whole thing if it turns out to be binary.
I think (1) and (2) should be easy. Doing (3) is a little messier,
because binary detection happens inside grep_buffer, but we can hoist it
out. However, for large files, it might be nice to have a streaming grep
interface anyway, and (3) could be part of that.
-Peff
^ permalink raw reply
* [PATCH 1/2] grep: let grep_buffer callers specify a binary flag
From: Jeff King @ 2012-02-01 23:21 UTC (permalink / raw)
To: Conrad Irwin; +Cc: Junio C Hamano, git, Nguyen Thai Ngoc Duy, Dov Grobgeld
In-Reply-To: <20120201221437.GA19044@sigill.intra.peff.net>
The caller of grep_buffer may have extra information about
whether a buffer is binary or not (e.g., from configuration).
Let's give them a chance to pass along that information and
override our binary auto-detection.
Callers can still pass "-1" to get the regular
auto-detection (and all callers are converted to do this,
meaning there should be no behavior change yet).
We could maintain source compatibility for callers by adding
a new "grep_buffer_with_flags" and leaving "grep_buffer" as
a wrapper that always passes "-1". But there are only 5
callers of grep_buffer, and only 1 of those (grepping commit
buffers) will not be converted to pass something useful in
the next patch. So it's simpler to just add a "-1" there.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 8 ++++----
grep.c | 23 ++++++++++++++++-------
grep.h | 2 +-
revision.c | 1 +
4 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 9ce064a..e328316 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -221,14 +221,14 @@ static void *run(void *arg)
void* data = load_sha1(w->identifier, &sz, w->name);
if (data) {
- hit |= grep_buffer(opt, w->name, data, sz);
+ hit |= grep_buffer(opt, w->name, -1, data, sz);
free(data);
}
} else if (w->type == WORK_FILE) {
size_t sz;
void* data = load_file(w->identifier, &sz);
if (data) {
- hit |= grep_buffer(opt, w->name, data, sz);
+ hit |= grep_buffer(opt, w->name, -1, data, sz);
free(data);
}
} else {
@@ -421,7 +421,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
if (!data)
hit = 0;
else
- hit = grep_buffer(opt, name, data, sz);
+ hit = grep_buffer(opt, name, -1, data, sz);
free(data);
free(name);
@@ -483,7 +483,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)
if (!data)
hit = 0;
else
- hit = grep_buffer(opt, name, data, sz);
+ hit = grep_buffer(opt, name, -1, data, sz);
free(data);
free(name);
diff --git a/grep.c b/grep.c
index 486230b..e547db2 100644
--- a/grep.c
+++ b/grep.c
@@ -983,8 +983,16 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)
fwrite(buf, size, 1, stdout);
}
+static int grep_buffer_is_binary(char *buf, unsigned long size, int flag)
+{
+ if (flag == -1)
+ flag = buffer_is_binary(buf, size);
+ return flag;
+}
+
static int grep_buffer_1(struct grep_opt *opt, const char *name,
- char *buf, unsigned long size, int collect_hits)
+ int is_binary, char *buf, unsigned long size,
+ int collect_hits)
{
char *bol = buf;
unsigned long left = size;
@@ -1017,11 +1025,11 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
switch (opt->binary) {
case GREP_BINARY_DEFAULT:
- if (buffer_is_binary(buf, size))
+ if (grep_buffer_is_binary(buf, size, is_binary))
binary_match_only = 1;
break;
case GREP_BINARY_NOMATCH:
- if (buffer_is_binary(buf, size))
+ if (grep_buffer_is_binary(buf, size, is_binary))
return 0; /* Assume unmatch */
break;
case GREP_BINARY_TEXT:
@@ -1182,23 +1190,24 @@ static int chk_hit_marker(struct grep_expr *x)
}
}
-int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size)
+int grep_buffer(struct grep_opt *opt, const char *name, int is_binary,
+ char *buf, unsigned long size)
{
/*
* we do not have to do the two-pass grep when we do not check
* buffer-wide "all-match".
*/
if (!opt->all_match)
- return grep_buffer_1(opt, name, buf, size, 0);
+ return grep_buffer_1(opt, name, is_binary, buf, size, 0);
/* Otherwise the toplevel "or" terms hit a bit differently.
* We first clear hit markers from them.
*/
clr_hit_marker(opt->pattern_expression);
- grep_buffer_1(opt, name, buf, size, 1);
+ grep_buffer_1(opt, name, is_binary, buf, size, 1);
if (!chk_hit_marker(opt->pattern_expression))
return 0;
- return grep_buffer_1(opt, name, buf, size, 0);
+ return grep_buffer_1(opt, name, is_binary, buf, size, 0);
}
diff --git a/grep.h b/grep.h
index fb205f3..8447e4c 100644
--- a/grep.h
+++ b/grep.h
@@ -128,7 +128,7 @@ extern void append_grep_pattern(struct grep_opt *opt, const char *pat, const cha
extern void append_header_grep_pattern(struct grep_opt *, enum grep_header_field, const char *);
extern void compile_grep_patterns(struct grep_opt *opt);
extern void free_grep_patterns(struct grep_opt *opt);
-extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size);
+extern int grep_buffer(struct grep_opt *opt, const char *name, int is_binary, char *buf, unsigned long size);
extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);
extern int grep_threads_ok(const struct grep_opt *opt);
diff --git a/revision.c b/revision.c
index c97d834..3dcd968 100644
--- a/revision.c
+++ b/revision.c
@@ -2150,6 +2150,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
return 1;
return grep_buffer(&opt->grep_filter,
NULL, /* we say nothing, not even filename */
+ -1,
commit->buffer, strlen(commit->buffer));
}
--
1.7.9.3.gc3fce1.dirty
^ permalink raw reply related
* [PATCH 2/2] grep: respect diff attributes for binary-ness
From: Jeff King @ 2012-02-01 23:21 UTC (permalink / raw)
To: Conrad Irwin; +Cc: Junio C Hamano, git, Nguyen Thai Ngoc Duy, Dov Grobgeld
In-Reply-To: <20120201221437.GA19044@sigill.intra.peff.net>
There is currently no way for users to tell git-grep that a
particular path is or is not a binary file; instead, grep
always relies on its auto-detection (or the user specifying
"-a" to treat all binary-looking files like text).
This patch teaches git-grep to use the same attribute lookup
that is used by git-diff. We could add a new "grep" flag,
but that is unnecessarily complex and unlikely to be useful.
Despite the name, the "-diff" attribute (or "diff=foo" and
the associated diff.foo.binary config option) are really
about describing the contents of the path. It's simply
historical that diff was the only thing that cared about
these attributes in the past.
And if this simple approach turns out to be insufficient, we
still have a backwards-compatible path forward: we can add a
separate "grep" attribute, and fall back to respecting
"diff" if it is unset.
There are a few things worth nothing about the
implementation.
One is that the attribute lookup happens outside of the
grep.[ch] interface (i.e., outside of grep_buffer). We could
do it at a lower level, which would be slightly more
convenient for callers. However, this interacts badly with
threading. The attribute-lookup code performs best when
lookup order matches the filesystem order (so looking up
"a/b/c" is cheaper if we just looked up "a/b/d" than if we
just did "x/y/z"). Because we issue many simultaneous
requests to grep_buffer, performing the attribute lookup at
that level will cause requests from unrelated paths to
interleave, and we lose the locality that makes the lookup
optimization work.
Instead, in the threaded case we check the attributes as
they are added to the work queue, meaning they are looked up
in the optimal order (in the single threaded case, this is a
non-issue, as we process the files serially in the optimal
order).
Here are a few numbers showing the difference. The first is
a best-of-five time for "git grep foo" on the linux-2.6 repo
before this patch (on a 4-core HT processor, using 8
threads):
real 0m0.306s
user 0m1.512s
sys 0m0.412s
Now here's the time for the same operation with a trial
implementation looking up attributes in grep_buffer,
showing a 31% slowdown:
real 0m0.401s
user 0m1.760s
sys 0m0.636s
And here's the same operation with this patch, with only an
11% slowdown:
real 0m0.339s
user 0m1.444s
sys 0m0.584s
Note that while the percentages are big, the absolute
numbers are pretty small. In particular, this is a very
inexpensive grep to do. A more complex regex should have the
same absolute slowdown from the attribute lookup, but it
would be a much smaller percentage of the total processing
time. So doing it this way is not a huge win, but it does
help on small greps.
The second issue worth noting is that while we do a full
attribute lookup, we pass along only the binary flag to
grep_buffer. When the "-p" flag is given to grep, we will
actually look up the same attributes to find funcname
patterns of matches. We could pass along the pointer to the
userdiff driver for reuse.
However, it's not worth doing this. It clutters the code, as
the driver has to be passed through a large number of helper
functions (and pollutes the grep_buffer interface with
userdiff code). And in my tests, it didn't actually improve
performance. Because we only have to look up the attribute
for a grep hit, in most cases we will only do the funcname
lookup for a small subset of files. The cost of the extra
lookups turns out to be negligible.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 26 ++++++++++++++++++++++----
t/t7008-grep-binary.sh | 24 ++++++++++++++++++++++++
2 files changed, 46 insertions(+), 4 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index e328316..bb38804 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -42,6 +42,7 @@ enum work_type {WORK_SHA1, WORK_FILE};
struct work_item {
enum work_type type;
char *name;
+ int is_binary;
/* if type == WORK_SHA1, then 'identifier' is a SHA1,
* otherwise type == WORK_FILE, and 'identifier' is a NUL
@@ -113,6 +114,14 @@ static pthread_cond_t cond_result;
static int skip_first_line;
+static int path_is_binary(const char *path)
+{
+ struct userdiff_driver *drv = userdiff_find_by_path(path);
+ if (drv)
+ return drv->binary;
+ return -1;
+}
+
static void add_work(enum work_type type, char *name, void *id)
{
grep_lock();
@@ -123,6 +132,11 @@ static void add_work(enum work_type type, char *name, void *id)
todo[todo_end].type = type;
todo[todo_end].name = name;
+
+ pthread_mutex_lock(&grep_attr_mutex);
+ todo[todo_end].is_binary = path_is_binary(name);
+ pthread_mutex_unlock(&grep_attr_mutex);
+
todo[todo_end].identifier = id;
todo[todo_end].done = 0;
strbuf_reset(&todo[todo_end].out);
@@ -221,14 +235,16 @@ static void *run(void *arg)
void* data = load_sha1(w->identifier, &sz, w->name);
if (data) {
- hit |= grep_buffer(opt, w->name, -1, data, sz);
+ hit |= grep_buffer(opt, w->name, w->is_binary,
+ data, sz);
free(data);
}
} else if (w->type == WORK_FILE) {
size_t sz;
void* data = load_file(w->identifier, &sz);
if (data) {
- hit |= grep_buffer(opt, w->name, -1, data, sz);
+ hit |= grep_buffer(opt, w->name, w->is_binary,
+ data, sz);
free(data);
}
} else {
@@ -421,7 +437,8 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
if (!data)
hit = 0;
else
- hit = grep_buffer(opt, name, -1, data, sz);
+ hit = grep_buffer(opt, name, path_is_binary(name),
+ data, sz);
free(data);
free(name);
@@ -483,7 +500,8 @@ static int grep_file(struct grep_opt *opt, const char *filename)
if (!data)
hit = 0;
else
- hit = grep_buffer(opt, name, -1, data, sz);
+ hit = grep_buffer(opt, name, path_is_binary(name),
+ data, sz);
free(data);
free(name);
diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index 917a264..fd6410f 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -99,4 +99,28 @@ test_expect_success 'git grep y<NUL>x a' "
test_must_fail git grep -f f a
"
+test_expect_success 'grep respects binary diff attribute' '
+ echo text >t &&
+ git add t &&
+ echo t:text >expect &&
+ git grep text t >actual &&
+ test_cmp expect actual &&
+ echo "t -diff" >.gitattributes &&
+ echo "Binary file t matches" >expect &&
+ git grep text t >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'grep respects not-binary diff attribute' '
+ echo binQary | q_to_nul >b &&
+ git add b &&
+ echo "Binary file b matches" >expect &&
+ git grep bin b >actual &&
+ test_cmp expect actual &&
+ echo "b diff" >.gitattributes &&
+ echo "b:binQary" >expect &&
+ git grep bin b | nul_to_q >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.9.3.gc3fce1.dirty
^ permalink raw reply related
* Re: [PATCH v2] Use correct grammar in diffstat summary line
From: Jonathan Nieder @ 2012-02-01 23:35 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Ævar Arnfjörð,
Frederik Schwarzer, Brandon Casey
In-Reply-To: <1328100907-20397-1-git-send-email-pclouds@gmail.com>
(dropping Thomas from cc)
Nguyễn Thái Ngọc Duy wrote:
> And this patch's diffstat looks just scary due to test suite's updates.
Not about this patch, but: let's see if we can chisel away at that.
Hidden below are also two minor bug reports about the patch. See (*)
and (**) below.
> --- a/t/t0023-crlf-am.sh
> +++ b/t/t0023-crlf-am.sh
> @@ -12,7 +12,7 @@ Subject: test1
>
> ---
> foo | 1 +
> - 1 files changed, 1 insertions(+), 0 deletions(-)
> + 1 file changed, 1 insertion(+)
> create mode 100644 foo
This patchfile is sample input to "git am", and this patch hunk
changes it to match the modified format-patch output format. But
don't we want "git am" to work with old patches, too? So this hunk is
unnecessary.
[...]
> --- a/t/t1200-tutorial.sh
> +++ b/t/t1200-tutorial.sh
> @@ -156,7 +156,7 @@ Updating VARIABLE..VARIABLE
> FASTFORWARD (no commit created; -m option ignored)
> example | 1 +
> hello | 1 +
> - 2 files changed, 2 insertions(+), 0 deletions(-)
> + 2 files changed, 2 insertions(+)
> EOF
Yes, this one's necessary.
(*) It's reminding us to update the gitcore-tutorial. :)
[...]
> --- a/t/t3300-funny-names.sh
> +++ b/t/t3300-funny-names.sh
> @@ -167,7 +167,7 @@ test_expect_success TABS_IN_FILENAMES 'git diff-tree delete with-funny' \
> test_expect_success TABS_IN_FILENAMES 'setup expect' '
> cat >expected <<\EOF
> "tabs\t,\" (dq) and spaces"
> - 1 files changed, 0 insertions(+), 0 deletions(-)
> + 1 file changed, 0 insertions(+), 0 deletions(-)
> EOF
> '
I'm not sure why this and other tests in the same file use "git apply
--stat" to check their work. The descriptions refer to diff-tree but
the tests themselves use diff-index | apply --stat. Puzzling.
[...]
> --- a/t/t3508-cherry-pick-many-commits.sh
> +++ b/t/t3508-cherry-pick-many-commits.sh
> @@ -38,13 +38,13 @@ test_expect_success 'cherry-pick first..fourth works' '
> cat <<-\EOF >expected &&
> [master OBJID] second
> Author: A U Thor <author@example.com>
> - 1 files changed, 1 insertions(+), 0 deletions(-)
> + 1 file changed, 1 insertion(+)
> [master OBJID] third
> Author: A U Thor <author@example.com>
> - 1 files changed, 1 insertions(+), 0 deletions(-)
> + 1 file changed, 1 insertion(+)
> [master OBJID] fourth
> Author: A U Thor <author@example.com>
> - 1 files changed, 1 insertions(+), 0 deletions(-)
> + 1 file changed, 1 insertion(+)
> EOF
Probably should be split out as a separate test so cherry-pick's
behavior and progress reporting can be tested separately. Aside from
that detail, makes sense.
[...]
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -444,7 +444,7 @@ test_expect_success 'stash show - stashes on stack, stash-like argument' '
> git reset --hard &&
> cat >expected <<-EOF &&
> file | 1 +
> - 1 files changed, 1 insertions(+), 0 deletions(-)
> + 1 file changed, 1 insertion(+)
Hm. "git stash show" defaults to --stat, but these tests are not
really about that. Maybe there should be one test that checks that
the default uses --stat and others could use --numstat or similar?
This will be a problem with GETTEXT_POISON (except your patch bypasses
poison so it got missed --- will send a relevant fix as a reply).
[...]
> +++ b/t/t4013/diff.diff-tree_--cc_--patch-with-stat_--summary_master
> +++ b/t/t4013/diff.diff-tree_--cc_--patch-with-stat_--summary_side
> +++ b/t/t4013/diff.diff-tree_--cc_--patch-with-stat_master
> +++ b/t/t4013/diff.diff-tree_--cc_--stat_--summary_master
[...]
> +++ b/t/t4013/diff.format-patch_--attach_--stdout_initial..side
> +++ b/t/t4013/diff.format-patch_--inline_--stdout_--numbered-files_initial..master
> +++ b/t/t4013/diff.format-patch_--inline_--stdout_--subject-prefix=TESTCASE_initial..master
[...]
Testing the --stat option. Can't really be avoided. (Though maybe
some more modular tests for these combinations can be made some day.)
[...]
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -520,7 +520,7 @@ test_expect_success 'shortlog of cover-letter wraps overly-long onelines' '
> cat > expect << EOF
> ---
> file | 16 ++++++++++++++++
> - 1 files changed, 16 insertions(+), 0 deletions(-)
> + 1 file changed, 16 insertions(+)
Not relevant to the detail being tested, should probably be fuzzed
out.
[...]
> --- a/t/t4030-diff-textconv.sh
> +++ b/t/t4030-diff-textconv.sh
> @@ -86,7 +86,7 @@ test_expect_success 'status -v produces text' '
>
> cat >expect.stat <<'EOF'
> file | Bin 2 -> 4 bytes
> - 1 files changed, 0 insertions(+), 0 deletions(-)
> + 1 file changed, 0 insertions(+), 0 deletions(-)
> EOF
> test_expect_success 'diffstat does not run textconv' '
> echo file diff=fail >.gitattributes &&
Testing diffstat. Would be nice to have an accompanying test of
numstat.
[...]
> --- a/t/t4045-diff-relative.sh
> +++ b/t/t4045-diff-relative.sh
> @@ -33,7 +33,7 @@ check_stat() {
> expect=$1; shift
> cat >expected <<EOF
> $expect | 1 +
> - 1 files changed, 1 insertions(+), 0 deletions(-)
> + 1 file changed, 1 insertion(+)
> EOF
Not the detail being tested, so technically it would be nicer to
fuzz the last line out.
[...]
> --- a/t/t4049-diff-stat-count.sh
> +++ b/t/t4049-diff-stat-count.sh
> @@ -16,7 +16,7 @@ test_expect_success setup '
> cat >expect <<-\EOF
> a | 1 +
> b | 1 +
> - 2 files changed, 2 insertions(+), 0 deletions(-)
> + 2 files changed, 2 insertions(+)
> EOF
> git diff --stat --stat-count=2 >actual &&
> test_cmp expect actual
Testing diffstat. Looks sane.
[...]
> +++ b/t/t4100/t-apply-8.expect
> @@ -1,2 +1,2 @@
> t/t4100-apply-stat.sh | 2 +-
> - 1 files changed, 1 insertions(+), 1 deletions(-)
> + 1 file changed, 1 insertion(+), 1 deletion(-)
[...]
> +++ b/t/t4100/t-apply-9.expect
> @@ -1,2 +1,2 @@
> t/t4100-apply-stat.sh | 2 +-
> - 1 files changed, 1 insertions(+), 1 deletions(-)
> + 1 file changed, 1 insertion(+), 1 deletion(-)
Testing apply --stat. Tests of apply --numstat would be more
interesting, but then we'd still need some tests like these to make
sure --stat is consistent with it, so it still seems sane.
> --- a/t/t5150-request-pull.sh
> +++ b/t/t5150-request-pull.sh
> @@ -95,7 +95,7 @@ test_expect_success 'setup: two scripts for reading pull requests' '
> b
> : diffstat
> n
> - / [0-9]* files changed/ {
> + / [0-9]* files\? changed/ {
Mimicking a human.
(**) Should probably use "*" instead of \? --- \? is a GNU extension,
not a BRE.
[...]
> +++ b/t/t7602-merge-octopus-many.sh
> @@ -57,7 +57,7 @@ Merge made by the 'octopus' strategy.
> c2.c | 1 +
> c3.c | 1 +
> c4.c | 1 +
> - 3 files changed, 3 insertions(+), 0 deletions(-)
> + 3 files changed, 3 insertions(+)
> create mode 100644 c2.c
> create mode 100644 c3.c
> create mode 100644 c4.c
Test is about the "Trying simple merge with" lines, not the final
diffstat summary. Should probably be fuzzed out.
Thanks, that was interesting.
Jonathan
^ permalink raw reply
* Re: [PATCH v3 1/2] Factor find_pack_entry()'s core out
From: Nicolas Pitre @ 2012-02-01 23:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <7v1uqe2ngh.fsf@alter.siamese.dyndns.org>
On Wed, 1 Feb 2012, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Nicolas Pitre <nico@fluxnic.net> writes:
> >
> >>> +static int find_pack_entry_1(const unsigned char *sha1,
> >>> + struct packed_git *p, struct pack_entry *e)
> >>
> >> This looks all goot but the name. Pretty please, try to find something
> >> that is more descriptive than "1". Suggestions:
> >> "find_pack_entry_lookup", "find_pack_entry_inner", etc.
> >
> > Perhaps "find_pack_entry_in_pack(sha1, e, p)"?
> > That would go well with the caller "find_pack_entry(sha1, e)".
>
> I amended 1/2 (and adjusted 2/2 to match) to call it fill_pack_entry.
> Will push out the result tonight.
Looks fine to me.
Nicolas
^ permalink raw reply
* Re: [PATCH] i18n: po for zh_cn
From: Junio C Hamano @ 2012-02-01 23:45 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List
In-Reply-To: <1328111436-58925-1-git-send-email-worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Git can speak Chinese now.
>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> ---
> po/zh_cn.po | 3568 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 个文件被修改, 3568 处添加(+), 0 处删除(-)
> create mode 100644 po/zh_cn.po
I do not mind Chinese in the patch text (i.e. below), but I would have
preferred the above not to be in Chinese, which I do not read---I can
guess what 文件, 添加 and 删除 are, and I can also guess that 个 and 处
are units of counting, but nevertheless...
I haven't decided what to do with this, though. If we are going to get
tons of l10n patches[*1*], it may make sense to have a separate clone of
git.git, with possibly a maintainer other than myself, forked at v1.7.9
and updates _only_ files in po/ hierarchy.
Let me think about how to manage the l10n patches a bit.
Thanks for a patch.
[Footnote]
*1* Ah, another thing, shouldn't the title be labelled as l10n instead of
i18n?
^ permalink raw reply
* Re: [PATCH] i18n: git-commit whence_s "merge/cherry-pick" message
From: Junio C Hamano @ 2012-02-01 23:46 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, avarab
In-Reply-To: <1328116830-61290-1-git-send-email-worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Mark the "merge/cherry-pick" messages in whence_s for translation.
> These messages returned from whence_s function are used as argument
> to build other messages.
>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> ---
> builtin/commit.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index eba137..663e1 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -202,10 +202,10 @@ static const char *whence_s(void)
> case FROM_COMMIT:
> break;
> case FROM_MERGE:
> - s = "merge";
> + s = _("merge");
> break;
> case FROM_CHERRY_PICK:
> - s = "cherry-pick";
> + s = _("cherry-pick");
> break;
> }
builtin/commit.c: In function 'whence_s':
builtin/commit.c:206: error: assignment discards qualifiers from pointer target type
builtin/commit.c:209: error: assignment discards qualifiers from pointer target type
I'll squash the following in. Thanks.
builtin/commit.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 663e1fd..470b4a4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -196,7 +196,7 @@ static void determine_whence(struct wt_status *s)
static const char *whence_s(void)
{
- char *s = "";
+ const char *s = "";
switch (whence) {
case FROM_COMMIT:
^ permalink raw reply related
* Re: General support for ! in git-config values
From: demerphq @ 2012-02-02 0:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð, Jeff King, Git Mailing List
In-Reply-To: <7v62fq2o03.fsf@alter.siamese.dyndns.org>
On 1 February 2012 23:21, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> I.e., everything pertaining to "!" happens after we get the config
>>> string. So what is it that you want "git config --with-exec" to do?
>>
>> I agree that that's how it should work, I just suggested --with-exec
>> in case anyone complained about the backwards compatibility issue of
>> changing the meaning of "!" for existing configs.
>
> Now you made me utterly confused.
>
> What "backwards compatibility" issue do you have in mind? If I name
> myself '!me' with "user.name = !me", do I suddenly break backwards
> compatibility of Git with previous versions somehow? If so how?
(I am piping up because Ævar and I discussed this idea earlier today
while we discussed git-deploy)
If Ævar's proposal was implemented without a --with-exec switch then
user.name = !me
would set your name to the result of executing the "me" command, which
likely would have the same result as not having set your name at all,
but presumably with a warning about a bad command.
> The --with-exec option you talk about seems to me the option about the
> backward compatibility of the _calling script_ of "git config".
By backward compatibility he meant that anything not providing the
--with-exec option to git-config would continue to behave as before,
and anything that did would get the new more powerful behavior, where
! at the front of a value signified that it meant a command to be
executed whose output would be used as the value.
I think it would be convenient to be able to enable this in the config
file itself. Consider a bunch of devs share a box. Their full name and
email address can be located via a LDAP lookup. So if we could enable
Ævar's proposal in the config then we can automagically have their
commits have the correct username and email if we were to put in the
/etc/gitconfig something like this:
config.enable_executable_options=true
user.name=!ldap_full_name
user.email=!ldap_email
So now, user.name and user.email don't have to be configured by the
users. They just get an account on the box and all their commits have
the right details, no fuss.
I am aware that there are other ways of solving /this/ particular
problem, so it specifically is not a big deal, but still I think the
idea makes a lot of sense, I can imagine it being used in a lot of
interesting ways that are not obvious right away.
cheers,
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
^ permalink raw reply
* [RFC/PATCH] i18n: do not define gettext/ngettext in NO_GETTEXT case
From: Jonathan Nieder @ 2012-02-02 0:20 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, Ævar Arnfjörð,
Frederik Schwarzer, Brandon Casey
In-Reply-To: <20120201233515.GC29599@burratino>
gettext.h is responsible for defining the _ and Q_ helpers which are
either simple stubs (in the NO_GETTEXT case) or synonyms for the
libintl functions used to translated git output. As an implementation
artifact, ever since the !NO_GETTEXT case was implemented
(v1.7.9-rc0~42^2, 2011-11-18), they have also defined gettext and
ngettext macros in the NO_GETTEXT case:
#ifdef gettext
# undef gettext
#endif
#define gettext(x) (x)
New readers looking at this header might be tempted to use gettext and
ngettext directly, since they are defined in all cases, instead of
using the _ and Q_ wrappers which look like shortcuts.
However gettext and ngettext bypass the GETTEXT_POISON feature. So
any code using them directly will produce perfectly sensible English
output when run in the test suite with GETTEXT_POISON set, instead of
the poison markers that normally would make it easy to catch output
intended for machines that has been marked for translation by mistake.
Avoid the temptation by _not_ providing fallback definitions for
gettext and ngettext ourselves. This way, the header is less
misleading and code that uses gettext et al directly will fail to
compile when NO_GETTEXT is set so we can catch it early.
We also take the opportunity to avoid a little ifdef-ery by splitting
the NO_GETTEXT and !NO_GETTEXT implementations into different headers.
Unfortunately this involves some duplication of code.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jonathan Nieder wrote:
> This will be a problem with GETTEXT_POISON (except your patch bypasses
> poison so it got missed --- will send a relevant fix as a reply).
Here's a rough patch to make that kind of thing easier to catch. It
might make more sense to do
#ifdef NO_GETTEXT
#define git_gettext_(x) (x)
#define git_ngettext_(msgid, plu, n) ...
#else
#define git_gettext_(x) gettext(x)
#define git_ngettext_(msgid, plu, n) ngettext(msgid, plu, n)
#endif
static inline const char *_(const char *msgid)
{
return poisoning() ? "poison!" : git_gettext_(msgid);
}
...
but I didn't think of it until I had already written this patch.
Anyway, this way you get both approaches for easy comparison. ;-)
Makefile | 3 +++
gettext-libintl.h | 26 ++++++++++++++++++++++++++
gettext-noop.h | 30 ++++++++++++++++++++++++++++++
gettext.h | 53 +++--------------------------------------------------
git-compat-util.h | 6 ++++++
5 files changed, 68 insertions(+), 50 deletions(-)
create mode 100644 gettext-libintl.h
create mode 100644 gettext-noop.h
diff --git a/Makefile b/Makefile
index c457c34f..83e8c0f1 100644
--- a/Makefile
+++ b/Makefile
@@ -1525,6 +1525,9 @@ endif
ifdef NO_GETTEXT
BASIC_CFLAGS += -DNO_GETTEXT
USE_GETTEXT_SCHEME ?= fallthrough
+ LIB_H += gettext-noop.h
+else
+ LIB_H += gettext-libintl.h
endif
ifdef NO_STRCASESTR
COMPAT_CFLAGS += -DNO_STRCASESTR
diff --git a/gettext-libintl.h b/gettext-libintl.h
new file mode 100644
index 00000000..c9199703
--- /dev/null
+++ b/gettext-libintl.h
@@ -0,0 +1,26 @@
+#ifndef GETTEXT_LIBINTL_H
+#define GETTEXT_LIBINTL_H
+
+#include <libintl.h>
+
+#define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))
+
+extern void git_setup_gettext(void);
+
+static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
+{
+ return use_gettext_poison() ? "# GETTEXT POISON #" : gettext(msgid);
+}
+
+static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
+const char *Q_(const char *msgid, const char *plu, unsigned long n)
+{
+ if (use_gettext_poison())
+ return "# GETTEXT POISON #";
+ return ngettext(msgid, plu, n);
+}
+
+/* Mark msgid for translation but do not translate it. */
+#define N_(msgid) msgid
+
+#endif
diff --git a/gettext-noop.h b/gettext-noop.h
new file mode 100644
index 00000000..28843a61
--- /dev/null
+++ b/gettext-noop.h
@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2010-2011 Ævar Arnfjörð Bjarmason
+ *
+ * This is a skeleton no-op implementation of gettext for Git.
+ */
+
+#ifndef GETTEXT_NOOP_H
+#define GETTEXT_NOOP_H
+
+#define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))
+
+static inline void git_setup_gettext(void) { }
+
+static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
+{
+ return use_gettext_poison() ? "# GETTEXT POISON #" : msgid;
+}
+
+static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
+const char *Q_(const char *msgid, const char *plu, unsigned long n)
+{
+ if (use_gettext_poison())
+ return "# GETTEXT POISON #";
+ return n == 1 ? msgid : plu;
+}
+
+/* Mark msgid for translation but do not translate it. */
+#define N_(msgid) msgid
+
+#endif
diff --git a/gettext.h b/gettext.h
index 57ba8bb0..9bc67243 100644
--- a/gettext.h
+++ b/gettext.h
@@ -1,11 +1,3 @@
-/*
- * Copyright (c) 2010-2011 Ævar Arnfjörð Bjarmason
- *
- * This is a skeleton no-op implementation of gettext for Git.
- * You can replace it with something that uses libintl.h and wraps
- * gettext() to try out the translations.
- */
-
#ifndef GETTEXT_H
#define GETTEXT_H
@@ -13,49 +5,10 @@
#error "namespace conflict: '_' or 'Q_' is pre-defined?"
#endif
-#ifndef NO_GETTEXT
-# include <libintl.h>
+#ifdef NO_GETTEXT
+#include "gettext-noop.h"
#else
-# ifdef gettext
-# undef gettext
-# endif
-# define gettext(s) (s)
-# ifdef ngettext
-# undef ngettext
-# endif
-# define ngettext(s, p, n) ((n == 1) ? (s) : (p))
+#include "gettext-libintl.h"
#endif
-#define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))
-
-#ifndef NO_GETTEXT
-extern void git_setup_gettext(void);
-#else
-static inline void git_setup_gettext(void)
-{
-}
-#endif
-
-#ifdef GETTEXT_POISON
-extern int use_gettext_poison(void);
-#else
-#define use_gettext_poison() 0
-#endif
-
-static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
-{
- return use_gettext_poison() ? "# GETTEXT POISON #" : gettext(msgid);
-}
-
-static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
-const char *Q_(const char *msgid, const char *plu, unsigned long n)
-{
- if (use_gettext_poison())
- return "# GETTEXT POISON #";
- return ngettext(msgid, plu, n);
-}
-
-/* Mark msgid for translation but do not translate it. */
-#define N_(msgid) msgid
-
#endif
diff --git a/git-compat-util.h b/git-compat-util.h
index 8f3972cd..ca4a4f19 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -236,6 +236,12 @@ extern char *gitbasename(char *);
#endif
#endif
+#ifdef GETTEXT_POISON
+extern int use_gettext_poison(void);
+#else
+#define use_gettext_poison() 0
+#endif
+
#include "compat/bswap.h"
/* General helper functions */
--
1.7.9
^ 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