* Re: [PATCH] i18n: po for zh_cn
From: Jiang Xin @ 2012-02-02 3:56 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, Git List
In-Reply-To: <CACBZZX6mHQ8NLWiBjp5OtDpFAc1=s9XfzHY5HhQ-26pqLXQ3bg@mail.gmail.com>
2012/2/2 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
> On Wed, Feb 1, 2012 at 16:50, Jiang Xin <worldhello.net@gmail.com> wrote:
>> Git can speak Chinese now.
>
> That's very nice, and it passes the msgfmt --check test as well, so
> all looks good.
One bug, the filename should be zh_CN.po, not zh_cn.po. On my Mac,
the filesystem is case insensitive, locale works fine, but in Linux won't.
I will send the patch again with correct filename and with translates
for news message from Git master branch.
(previous l10n is against git v1.7.9, i.e. the maint branch)
>
> As an aside doesn't the bug / workaround described in
> https://github.com/git/git/blob/master/gettext.c#L35 impact chinese
> especially badly? I'd expect any place where we use strerror() to show
> up as total gibberish.
The perror test program does impact chinese too.
On Linux with well translated system error messages, with the following output:
$ ./test
test: ??????
On Mac without translated system error messages, output is all ascii.
--
Jiang Xin
^ permalink raw reply
* Re: General support for ! in git-config values
From: Jeff King @ 2012-02-02 2:38 UTC (permalink / raw)
To: demerphq; +Cc: Junio C Hamano, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <CANgJU+WCdsF+igCWoueYcChYBvNyj5je_kvWorCBOgh5D7Bb9g@mail.gmail.com>
On Thu, Feb 02, 2012 at 02:57:14AM +0100, demerphq wrote:
> > Not really.
> >
> > I do not think whatever "utility" value outweighs the hassle of having to
> > think through the ramifications (including but not limited to security) of
> > running arbitrary user command every time a value is looked up.
>
> Why is that your problem? If I have to enable it then isn't that my choice?
>From a security perspective, you want to make sure that people who
aren't interested in your feature don't accidentally trigger it. E.g.,
imagine I currently run a locked-down git repo but execute some commands
on your behalf, and I allow you to set a few "known safe" config options
like user.email. Even though I am not interested in your feature,
respecting "!rm -rf /" in the user.email you give me would be a bad
thing.
It's not an insurmountable problem. There could be options to turn it
on, or turn it off, or whatever. Or we could shrug and say that config
is already dangerous to let other people set (which it is already, but
only for some options). But those are the sorts of ramifications that
need to be thought through.
(Another one is that with our current strategy, we actually read and
parse the config files multiple times. Should your program get run many
times?).
-Peff
^ permalink raw reply
* Re: General support for ! in git-config values
From: demerphq @ 2012-02-02 2:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð, Jeff King, Git Mailing List
In-Reply-To: <7v39au1073.fsf@alter.siamese.dyndns.org>
On 2 February 2012 02:41, Junio C Hamano <gitster@pobox.com> wrote:
> demerphq <demerphq@gmail.com> writes:
>
>>> When you say "alias.co = !git checkout --foo-bar", you are not saying
>>> "instead of usual aliases like 'alias.co = checkout' that defines what
>>> string replaces the 'co' alias, please run !whatever command, capture
>>> its output, and that is the _value_ of alias.co variable, i.e. the command
>>> line that is going to be run".
>>
>> I think I see your point. Using the same "!" notation for "execute
>> this command and exit" and "execute this command and use its result as
>> the config value" might be confusing.
>
> I am afraid that you are not seeing it. In "alias.co = !whatever", bang
> does *NOT* mean "execute this command and exit".
Then I have been unclear. I understand what this does:
$ git config alias.foo '!echo foo'
$ git foo
foo
And if you were trying to say that having "!" denote "execute this
command and use its return as the config value" would be confusing as
it is different from its meaning for aliases then I see your point.
--
perl -Mre=debug -e "/just|another|perl|hacker/"
^ permalink raw reply
* Re: [PATCH] Don't search files with an unset "grep" attribute
From: Junio C Hamano @ 2012-02-02 2:03 UTC (permalink / raw)
To: Jeff King; +Cc: Conrad Irwin, git, Nguyen Thai Ngoc Duy, Dov Grobgeld
In-Reply-To: <20120201232027.GA32119@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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.
Correct.
> 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.
Nice.
> ... However, for large files, it might be nice to have a streaming grep
> interface anyway, and (3) could be part of that.
Even nicer ;-)
^ permalink raw reply
* [PATCH v3] i18n: format_tracking_info "Your branch is behind" message
From: Jiang Xin @ 2012-02-02 2:02 UTC (permalink / raw)
To: Git List, avarab, Junio C Hamano; +Cc: Jiang Xin
Function format_tracking_info in remote.c is called by
wt_status_print_tracking in wt-status.c, which will print
branch tracking message in git-status. git-checkout also
show these messages through it's report_tracking function.
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
remote.c | 32 +++++++++++++++++++++-----------
1 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/remote.c b/remote.c
index 73a38..af597 100644
--- a/remote.c
+++ b/remote.c
@@ -1572,19 +1572,29 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
base = branch->merge[0]->dst;
base = shorten_unambiguous_ref(base, 0);
if (!num_theirs)
- strbuf_addf(sb, "Your branch is ahead of '%s' "
- "by %d commit%s.\n",
- base, num_ours, (num_ours == 1) ? "" : "s");
+ strbuf_addf(sb,
+ Q_("Your branch is ahead of '%s' by %d commit.\n",
+ "Your branch is ahead of '%s' by %d commits.\n",
+ num_ours),
+ base, num_ours);
else if (!num_ours)
- strbuf_addf(sb, "Your branch is behind '%s' "
- "by %d commit%s, "
- "and can be fast-forwarded.\n",
- base, num_theirs, (num_theirs == 1) ? "" : "s");
+ strbuf_addf(sb,
+ Q_("Your branch is behind '%s' by %d commit, "
+ "and can be fast-forwarded.\n",
+ "Your branch is behind '%s' by %d commits, "
+ "and can be fast-forwarded.\n",
+ num_theirs),
+ base, num_theirs);
else
- strbuf_addf(sb, "Your branch and '%s' have diverged,\n"
- "and have %d and %d different commit(s) each, "
- "respectively.\n",
- base, num_ours, num_theirs);
+ strbuf_addf(sb,
+ Q_("Your branch and '%s' have diverged,\n"
+ "and have %d and %d different commit each, "
+ "respectively.\n",
+ "Your branch and '%s' have diverged,\n"
+ "and have %d and %d different commits each, "
+ "respectively.\n",
+ num_theirs),
+ base, num_ours, num_theirs);
return 1;
}
--
1.7.9.109.gd927b
^ permalink raw reply
* Re: General support for ! in git-config values
From: demerphq @ 2012-02-02 1:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð, Jeff King, Git Mailing List
In-Reply-To: <7v7h06109t.fsf@alter.siamese.dyndns.org>
On 2 February 2012 02:39, Junio C Hamano <gitster@pobox.com> wrote:
> demerphq <demerphq@gmail.com> writes:
>
>> On 2 February 2012 01:27, Junio C Hamano <gitster@pobox.com> wrote:
>>> demerphq <demerphq@gmail.com> writes:
>>>
>>>> user.name=!ldap_full_name
>>>> user.email=!ldap_email
>>>
>>> I didn't get the impression that that was what he was proposing, but
>>> regardless of who's suggesting it, the above does not make any sense,
>>> from the syntax point of view.
>>
>> Does that mean that from a utility point of view it does?
>
> Not really.
>
> I do not think whatever "utility" value outweighs the hassle of having to
> think through the ramifications (including but not limited to security) of
> running arbitrary user command every time a value is looked up.
Why is that your problem? If I have to enable it then isn't that my choice?
--
perl -Mre=debug -e "/just|another|perl|hacker/"
^ permalink raw reply
* Re: [PATCH v2] i18n: format_tracking_info "Your branch is behind" message
From: Junio C Hamano @ 2012-02-02 1:50 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, avarab
In-Reply-To: <1328146639-87863-1-git-send-email-worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Function format_tracking_info in remote.c is called by
> wt_status_print_tracking in wt-status.c, which will print
> branch tracking message in git-status. git-checkout also
> show these messages through it's report_tracking function.
>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> ---
> remote.c | 48 +++++++++++++++++++++++++++++++++++++-----------
> 1 files changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 73a38..45ac1 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1572,19 +1572,45 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
> base = branch->merge[0]->dst;
> base = shorten_unambiguous_ref(base, 0);
> if (!num_theirs)
> - strbuf_addf(sb, "Your branch is ahead of '%s' "
> - "by %d commit%s.\n",
> - base, num_ours, (num_ours == 1) ? "" : "s");
> + strbuf_addf(sb,
> + Q_(
> + /* The singular version */
> + "Your branch is ahead of '%s' by %d commit.\n",
> + /* The plural version */
> + "Your branch is ahead of '%s' by %d commits.\n",
> + /* Give ngettext() the count */
> + num_ours),
> + base,
> + num_ours);
It may be just me, but I think
strbuf_addf(sb,
Q_("Your branch is ahead of '%s' by %d commit.\n",
"Your branch is ahead of '%s' by %d commits.\n",
num_ours),
base, num_ours);
would be far easier to read than these repetitive comments.
It allows you to see it more clearly that the two strings and one variable
are given to Q_(), and the last two, base and num_ours, are the values to
be formatted by whatever the format string Q_() would choose.
When the strings are longer, an extra indent would probably help, e.g.
strbuf_addf(sb,
Q_("Your branch is behind '%s' by %d commit, "
"and can be fast-forwarded.\n",
"Your branch is behind '%s' by %d commits, "
"and can be fast-forwarded.\n",
num_theirs),
base, num_theirs);
^ permalink raw reply
* warning: tip of 'next' rewound post 1.7.9
From: Junio C Hamano @ 2012-02-02 1:44 UTC (permalink / raw)
To: git
I've finished today's integration run, and the tip of 'next' is now
rewound.
The draft release notes to the next release calls it 1.7.10 but the name
is tentative. We may want to switch to two dotted-decimal scheme and call
it 2.0 or something, but I haven't decided it yet.
^ permalink raw reply
* Re: General support for ! in git-config values
From: Junio C Hamano @ 2012-02-02 1:41 UTC (permalink / raw)
To: demerphq; +Cc: Ævar Arnfjörð, Jeff King, Git Mailing List
In-Reply-To: <CANgJU+XQWdFmfmBJ4KX4GBz1a=TrVbp9BphGmhEb5Gphmzogjw@mail.gmail.com>
demerphq <demerphq@gmail.com> writes:
>> When you say "alias.co = !git checkout --foo-bar", you are not saying
>> "instead of usual aliases like 'alias.co = checkout' that defines what
>> string replaces the 'co' alias, please run !whatever command, capture
>> its output, and that is the _value_ of alias.co variable, i.e. the command
>> line that is going to be run".
>
> I think I see your point. Using the same "!" notation for "execute
> this command and exit" and "execute this command and use its result as
> the config value" might be confusing.
I am afraid that you are not seeing it. In "alias.co = !whatever", bang
does *NOT* mean "execute this command and exit".
^ permalink raw reply
* Re: General support for ! in git-config values
From: Junio C Hamano @ 2012-02-02 1:39 UTC (permalink / raw)
To: demerphq; +Cc: Ævar Arnfjörð, Jeff King, Git Mailing List
In-Reply-To: <CANgJU+XQWdFmfmBJ4KX4GBz1a=TrVbp9BphGmhEb5Gphmzogjw@mail.gmail.com>
demerphq <demerphq@gmail.com> writes:
> On 2 February 2012 01:27, Junio C Hamano <gitster@pobox.com> wrote:
>> demerphq <demerphq@gmail.com> writes:
>>
>>> user.name=!ldap_full_name
>>> user.email=!ldap_email
>>
>> I didn't get the impression that that was what he was proposing, but
>> regardless of who's suggesting it, the above does not make any sense,
>> from the syntax point of view.
>
> Does that mean that from a utility point of view it does?
Not really.
I do not think whatever "utility" value outweighs the hassle of having to
think through the ramifications (including but not limited to security) of
running arbitrary user command every time a value is looked up.
^ permalink raw reply
* [PATCH v2] i18n: format_tracking_info "Your branch is behind" message
From: Jiang Xin @ 2012-02-02 1:37 UTC (permalink / raw)
To: Git List, avarab; +Cc: Jiang Xin
Function format_tracking_info in remote.c is called by
wt_status_print_tracking in wt-status.c, which will print
branch tracking message in git-status. git-checkout also
show these messages through it's report_tracking function.
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
remote.c | 48 +++++++++++++++++++++++++++++++++++++-----------
1 files changed, 37 insertions(+), 11 deletions(-)
diff --git a/remote.c b/remote.c
index 73a38..45ac1 100644
--- a/remote.c
+++ b/remote.c
@@ -1572,19 +1572,45 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
base = branch->merge[0]->dst;
base = shorten_unambiguous_ref(base, 0);
if (!num_theirs)
- strbuf_addf(sb, "Your branch is ahead of '%s' "
- "by %d commit%s.\n",
- base, num_ours, (num_ours == 1) ? "" : "s");
+ strbuf_addf(sb,
+ Q_(
+ /* The singular version */
+ "Your branch is ahead of '%s' by %d commit.\n",
+ /* The plural version */
+ "Your branch is ahead of '%s' by %d commits.\n",
+ /* Give ngettext() the count */
+ num_ours),
+ base,
+ num_ours);
else if (!num_ours)
- strbuf_addf(sb, "Your branch is behind '%s' "
- "by %d commit%s, "
- "and can be fast-forwarded.\n",
- base, num_theirs, (num_theirs == 1) ? "" : "s");
+ strbuf_addf(sb,
+ Q_(
+ /* The singular version */
+ "Your branch is behind '%s' by %d commit, "
+ "and can be fast-forwarded.\n",
+ /* The plural version */
+ "Your branch is behind '%s' by %d commits, "
+ "and can be fast-forwarded.\n",
+ /* Give ngettext() the count */
+ num_theirs),
+ base,
+ num_theirs);
else
- strbuf_addf(sb, "Your branch and '%s' have diverged,\n"
- "and have %d and %d different commit(s) each, "
- "respectively.\n",
- base, num_ours, num_theirs);
+ strbuf_addf(sb,
+ Q_(
+ /* The singular version */
+ "Your branch and '%s' have diverged,\n"
+ "and have %d and %d different commit each, "
+ "respectively.\n",
+ /* The plural version */
+ "Your branch and '%s' have diverged,\n"
+ "and have %d and %d different commits each, "
+ "respectively.\n",
+ /* Give ngettext() the count */
+ num_theirs),
+ base,
+ num_ours,
+ num_theirs);
return 1;
}
--
1.7.9.109.gd927b
^ permalink raw reply
* [PATCH v3 0/4] completion: couple of cleanups
From: Felipe Contreras @ 2012-02-02 1:15 UTC (permalink / raw)
To: git; +Cc: SZEDER Gábor, Jonathan Nieder, Felipe Contreras
And an improvement for zsh.
Junio: I see you already picked most of them for 'pu', but I've made further changes based on the feedback:
* completion: be nicer with zsh
Improved the code-style
* completion: simplify __gitcomp*
Fix
Improved commit message
Cheers.
Felipe Contreras (4):
completion: be nicer with zsh
completion: simplify __git_remotes
completion: remove unused code
completion: simplify __gitcomp*
contrib/completion/git-completion.bash | 66 +++++---------------------------
1 files changed, 10 insertions(+), 56 deletions(-)
--
1.7.9
^ permalink raw reply
* Re: General support for ! in git-config values
From: demerphq @ 2012-02-02 1:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð, Jeff King, Git Mailing List
In-Reply-To: <7vliom13lm.fsf@alter.siamese.dyndns.org>
On 2 February 2012 01:27, Junio C Hamano <gitster@pobox.com> wrote:
> demerphq <demerphq@gmail.com> writes:
>
>> 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.
>>
>> user.name=!ldap_full_name
>> user.email=!ldap_email
>
> I didn't get the impression that that was what he was proposing, but
> regardless of who's suggesting it, the above does not make any sense,
> from the syntax point of view.
Does that mean that from a utility point of view it does?
As for the suggestion and who is making it, this came up because Ævar
and I were discussing git-deploy and a scenario much like I described,
where we use tags to mark out rollouts and we have multiple users
using a common staging server and git repo to do the rollouts from, we
want the tags to have proper details, so we have to go through some
hoops to ensure that users have properly configured things before the
do their first rollout. Since Ævar was aware of the "!" notation for
aliases and thought it made some kind of sense to reuse the syntax
elsewhere he decided to see what people thought. I was not aware of
the notation until he pointed it out. I don't use git-alias.
> When you say "alias.co = !git checkout --foo-bar", you are not saying
> "instead of usual aliases like 'alias.co = checkout' that defines what
> string replaces the 'co' alias, please run !whatever command, capture
> its output, and that is the _value_ of alias.co variable, i.e. the command
> line that is going to be run".
I think I see your point. Using the same "!" notation for "execute
this command and exit" and "execute this command and use its result as
the config value" might be confusing.
Do you have a suggestion for a better syntax? Backtick quoting perhaps?
cheers,
yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
^ permalink raw reply
* Re: [PATCH 1/2] grep: let grep_buffer callers specify a binary flag
From: Jeff King @ 2012-02-02 0:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Conrad Irwin, git, Nguyen Thai Ngoc Duy, Dov Grobgeld
In-Reply-To: <7vhaza12ol.fsf@alter.siamese.dyndns.org>
On Wed, Feb 01, 2012 at 04:47:38PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > 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.
>
> Hrm, I would have expected a patch that turns "const char *name" into a
> structure that has name and drv as its members, so that later we can tell
> the function more about the nature of the contents. Or a separate pointer
> to drv in place of your "binary" flag word.
Hmm. Yeah, I would be OK with that, as it's really encapsulating the
idea of "stuff we want to tell grep about this buffer".
What I really didn't want to do was pass the userdiff driver directly,
as that feels way too much like an implementation detail (and while it
can be used to avoid further lookups, it doesn't seem to make a
difference in practice -- see the following patch). And passing it
around became a big messy chore.
But if it were a "struct grep_context" that encapsulated those things, I
think it would be much nicer (it could even carry a pointer to "struct
grep_opt" in it, making the code _more_ pleasant to read, not less).
I'll take a look at re-working it that way.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] grep: let grep_buffer callers specify a binary flag
From: Junio C Hamano @ 2012-02-02 0:47 UTC (permalink / raw)
To: Jeff King; +Cc: Conrad Irwin, git, Nguyen Thai Ngoc Duy, Dov Grobgeld
In-Reply-To: <20120201232109.GA2652@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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.
Hrm, I would have expected a patch that turns "const char *name" into a
structure that has name and drv as its members, so that later we can tell
the function more about the nature of the contents. Or a separate pointer
to drv in place of your "binary" flag word.
I am not saying that your patch is wrong. It was just somewhat unexpected
that "binary" is the only additional thing we want to tell the function.
^ permalink raw reply
* Re: [PATCH v2] Use correct grammar in diffstat summary line
From: Jonathan Nieder @ 2012-02-02 0:31 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>
Jonathan Nieder wrote:
> Nguyễn Thái Ngọc Duy wrote:
>> --- 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.
... and here's a patch making that change for convenience (also untested).
---
t/t5150-request-pull.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index 34c482f1..ec5f7368 100755
--- 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/ {
a\\
DIFFSTAT
b
--
1.7.9
^ permalink raw reply related
* Re: [PATCH v2] Use correct grammar in diffstat summary line
From: Jonathan Nieder @ 2012-02-02 0:27 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>
Jonathan Nieder wrote:
> [...]
>> --- 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. :)
Patch for squashing (untested) below.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Documentation/gitcore-tutorial.txt | 2 +-
Documentation/gittutorial-2.txt | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
index c27d086f..fb0d5692 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -1004,7 +1004,7 @@ Updating from ae3a2da... to a80b4aa....
Fast-forward (no commit created; -m option ignored)
example | 1 +
hello | 1 +
- 2 files changed, 2 insertions(+), 0 deletions(-)
+ 2 files changed, 2 insertions(+)
----------------
Because your branch did not contain anything more than what had
diff --git a/Documentation/gittutorial-2.txt b/Documentation/gittutorial-2.txt
index f1e4422a..e00a4d21 100644
--- a/Documentation/gittutorial-2.txt
+++ b/Documentation/gittutorial-2.txt
@@ -34,12 +34,12 @@ $ echo 'hello world' > file.txt
$ git add .
$ git commit -a -m "initial commit"
[master (root-commit) 54196cc] initial commit
- 1 files changed, 1 insertions(+), 0 deletions(-)
+ 1 file changed, 1 insertion(+)
create mode 100644 file.txt
$ echo 'hello world!' >file.txt
$ git commit -a -m "add emphasis"
[master c4d59f3] add emphasis
- 1 files changed, 1 insertions(+), 1 deletions(-)
+ 1 file changed, 1 insertion(+), 1 deletion(-)
------------------------------------------------
What are the 7 digits of hex that git responded to the commit with?
--
1.7.9
^ permalink raw reply related
* Re: General support for ! in git-config values
From: Junio C Hamano @ 2012-02-02 0:27 UTC (permalink / raw)
To: demerphq; +Cc: Ævar Arnfjörð, Jeff King, Git Mailing List
In-Reply-To: <CANgJU+X+UZmycwE6xkJ-zHfT7ai6nV9zbeR4WYnAXczL5JtqjA@mail.gmail.com>
demerphq <demerphq@gmail.com> writes:
> 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.
>
> user.name=!ldap_full_name
> user.email=!ldap_email
I didn't get the impression that that was what he was proposing, but
regardless of who's suggesting it, the above does not make any sense,
from the syntax point of view.
When you say "alias.co = !git checkout --foo-bar", you are not saying
"instead of usual aliases like 'alias.co = checkout' that defines what
string replaces the 'co' alias, please run !whatever command, capture
its output, and that is the _value_ of alias.co variable, i.e. the command
line that is going to be run".
^ 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
* 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
* 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: [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 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 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
* [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
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