Git development
 help / color / mirror / Atom feed
* Re: [PATCH] i18n: po for zh_cn
From: Frederik Schwarzer @ 2012-02-02 10:04 UTC (permalink / raw)
  To: git

Am Donnerstag, 2. Februar 2012, 09:15:38 schrieben Sie:

(damn, my mail programme always answers to the sender only ...)

Hi,

> Jiang Xin <worldhello.net@gmail.com> writes:
> > 2012/2/2 Junio C Hamano <gitster@pobox.com>:
> > ...
> > 
> >>  (6) From time to time, the l10n coordinator will pull from
> >>  "git.git" when
> >>  
> >>     meaningful number of changes are made to the translatable
> >>     strings.  I hope this would happen much less often than
> >>     once per week, preferably much less frequently. The l10n
> >>     coordinator updates po/git.pot and makes a commit, and
> >>     notifies the l10n teams.
> > 
> > notifies the l10n teams using another mailing list maybe.
> 
> I personally think using the regular git@vger.kernel.org list would
> be preferrable for this.  People who translate would want to learn
> the reasoning that led to the final phrasing of the messages to
> come up with usable translation, and following the main list would
> be one way to do so. Having to follow two separate list will be an
> unnecessary burden.
> 
> But the choice of how to coordinate his or her work with the l10n
> teams is entirely up to the l10n coordinator.

As I see it there are two kinds of people doing translations. 
Developers who also translate and people who want to translate 
software but are not involved in the development.

Translations of the former group are in many cases suboptimal. But the 
latter group will not follow this mailing list. They are not 
interested in all the details. They live in their room, get a piece of 
paper and translate it. In case something is unclear, they aks. But as 
it is now, it would look something like this:
Translators receive 100+ emails per day and are supposed to filter out 
the one email every 10 days that carries useful information for their 
work. In practice I guess interested Translators (who are not 
interested in every code detail) will unsubscribe after a few days and 
then miss all the fun.

A git-i18n mailing list could coordinate that. It would not be a list 
for l10n teams to do their internal coordination, but for the i18n 
coordinator to notify l10n teams about updated POT files (he might 
even merge PO files) and for l10n teams to ask about strings they are 
unsure about. These questions would then be digested by the i18n 
coordinator and brought to the attention of the developers if needed.

How does that sound?

Regards

PS: I would have even put all the POT/PO stuff in an extra Git 
repository. Working with translation is decoupled from development 
cleanly by the msginit/msgmerge process but well, it might not be woth 
that separation for a single project. :)

^ permalink raw reply

* Re: General support for ! in git-config values
From: Jeff King @ 2012-02-02  9:54 UTC (permalink / raw)
  To: demerphq; +Cc: Junio C Hamano, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <CANgJU+X2dRP__PFAywGEisDS3xyF7fSszSQG6BO61j2TMKL3Qg@mail.gmail.com>

On Thu, Feb 02, 2012 at 10:44:05AM +0100, demerphq wrote:

> The general design of git seems to me to be based around providing
> building blocks that people can use to build new and interesting tools
> on top of, and so it seems counter to that philosophy to reject an
> feature based on speculative security issues that really can't be
> decided in advance but must instead be decided on a case by case
> basis.

I can't speak for Junio, but I am certainly not rejecting it. Only
saying that it needs to be thought through, and the utility weighed
against the costs. So far I haven't seen an actual patch to comment on
(or even a proposed syntax beyond starting a string with "!", which I
think is a non-starter due to conflicting with existing uses), nor have
I seen a concrete use case (you mentioned pulling the name/email from
ldap, but you also mentioned that there are lots of other ways of
solving that particular problem, so it's not especially compelling).

I'd be happy to hear a more concrete proposal.

-Peff

^ permalink raw reply

* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02  9:46 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, SZEDER Gábor
In-Reply-To: <CAMP44s0nD4p9=fwpLwchmGJ123onLmRaSPmOL+cYpTFCJ-jwXw@mail.gmail.com>

Felipe Contreras wrote:
> On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> See [1] for details.  If there's no obvious explanation, I'd suggest
>> contacting the postmaster.
>
> But there's nothing like the taboo words in the mail:
> http://vger.kernel.org/majordomo-taboos.txt

Why are you telling me?  I am not the postmaster.  I can't do anything
to investigate it or fix it.

^ permalink raw reply

* Re: General support for ! in git-config values
From: demerphq @ 2012-02-02  9:44 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Ævar Arnfjörð, Git Mailing List
In-Reply-To: <20120202023857.GA11745@sigill.intra.peff.net>

On 2 February 2012 03:38, Jeff King <peff@peff.net> wrote:
> 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.

Like I said, I do not think this should be enabled by default, I think
it should be possible to enable it config wide. So unless this
scenario involves getting the owner of the locked down repo to enable
a config option they know nothing about, in which case I would say
there are easier attacks -- someone that stupid probably could be
talked into telling you their root password. :-)

> It's not an insurmountable problem. There could be options to turn it
> on, or turn it off, or whatever.

My thought exactly. Anyone paranoid about security would never enable
this feature. Those who are comfortable with the security issues
could.

> 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).

I think that since it could be set up to be determined by the user,
that it would be no more dangerous than any other option.

> But those are the sorts of ramifications that
> need to be thought through.

I understand that. All I can say is $work uses git on a pretty large
scale, 100+ devs etc, and we use it to manage our rollout processes
which we use a lot (I cant say how often but a lot). So if it would be
useful to us it probably would be useful to others.

> (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?).

Again I would say this is not git's problem. If it should not be run
multiple times it is up to the user to figure out an alternative.

The general design of git seems to me to be based around providing
building blocks that people can use to build new and interesting tools
on top of, and so it seems counter to that philosophy to reject an
feature based on speculative security issues that really can't be
decided in advance but must instead be decided on a case by case
basis.

cheers,
Yves





-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

^ permalink raw reply

* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-02-02  9:38 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, SZEDER Gábor
In-Reply-To: <20120202091059.GE3823@burratino>

On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (dropping Shawn from cc list, since I don't think he's touched the
>  completion code for years)
> Felipe Contreras wrote:
>> On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> Patches didn't hit the list again.  Any idea why?
>>
>> No. A bug in list software?
>>
>> I didn't get any warning or error.
>
> Except in response to HTML attachments, I've never seen vger return any
> explanation when it decides a message is spam.  It just discards them.

Which is wrong, because when it discards them wrongly nobody knows why.

> See [1] for details.  If there's no obvious explanation, I'd suggest
> contacting the postmaster.

But there's nothing like the taboo words in the mail:
http://vger.kernel.org/majordomo-taboos.txt

-- 
Felipe Contreras

^ permalink raw reply

* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02  9:10 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, SZEDER Gábor
In-Reply-To: <CAMP44s3FxUmnpQevoV2ARJpWK9CJ16zXDmpJRDOLHNW6RdSc5Q@mail.gmail.com>

(dropping Shawn from cc list, since I don't think he's touched the
 completion code for years)
Felipe Contreras wrote:
> On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> Patches didn't hit the list again.  Any idea why?
>
> No. A bug in list software?
>
> I didn't get any warning or error.

Except in response to HTML attachments, I've never seen vger return any
explanation when it decides a message is spam.  It just discards them.

See [1] for details.  If there's no obvious explanation, I'd suggest
contacting the postmaster.

Hope that helps, and sorry for the fuss,
Jonathan

[1] http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v3 2/4] completion: simplify __git_remotes
From: Jonathan Nieder @ 2012-02-02  9:05 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <1328145320-14071-1-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras wrote:

>   completion: simplify __git_remotes
>   completion: remove unused code

In the same spirit of stealing potential shortlog lines from Junio:

-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Subject: completion: use ls -1 instead of rolling a loop to do that ourselves

This simplifies the code a great deal.  In particular, it allows us to
get rid of __git_shopt, which is used only in this fuction to enable
'nullglob' in zsh.

[jn: squashed with a patch that actually gets rid of __git_shopt]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
The diffstat says it all.

 contrib/completion/git-completion.bash |   37 +-------------------------------
 1 files changed, 1 insertions(+), 36 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 78be1958..4612dde9 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -644,12 +644,7 @@ __git_refs_remotes ()
 __git_remotes ()
 {
 	local i ngoff IFS=$'\n' d="$(__gitdir)"
-	__git_shopt -q nullglob || ngoff=1
-	__git_shopt -s nullglob
-	for i in "$d/remotes"/*; do
-		echo ${i#$d/remotes/}
-	done
-	[ "$ngoff" ] && __git_shopt -u nullglob
+	test -d "$d/remotes" && ls -1 "$d/remotes"
 	for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
 		i="${i#remote.}"
 		echo "${i/.url*/}"
@@ -2733,33 +2728,3 @@ if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
 complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
 	|| complete -o default -o nospace -F _git git.exe
 fi
-
-if [[ -n ${ZSH_VERSION-} ]]; then
-	__git_shopt () {
-		local option
-		if [ $# -ne 2 ]; then
-			echo "USAGE: $0 (-q|-s|-u) <option>" >&2
-			return 1
-		fi
-		case "$2" in
-		nullglob)
-			option="$2"
-			;;
-		*)
-			echo "$0: invalid option: $2" >&2
-			return 1
-		esac
-		case "$1" in
-		-q)	setopt | grep -q "$option" ;;
-		-u)	unsetopt "$option" ;;
-		-s)	setopt "$option" ;;
-		*)
-			echo "$0: invalid flag: $1" >&2
-			return 1
-		esac
-	}
-else
-	__git_shopt () {
-		shopt "$@"
-	}
-fi
-- 
1.7.9

^ permalink raw reply related

* Re: How to find and analyze bad merges?
From: norbert.nemec @ 2012-02-02  9:01 UTC (permalink / raw)
  To: git
In-Reply-To: <7vd39xy7it.fsf@alter.siamese.dyndns.org>

Am 02.02.12 09:16, schrieb Junio C Hamano:
> "norbert.nemec"<norbert.nemec@native-instruments.de>  writes:
>
>> a colleague of mine happened to produce a bad merge by unintenionally
>> picking the version of the remote branch ("R") for all conflicting
>> files. Effectively, he eliminated a whole bunch of bugfixes that were
>> already on his local branch ("L").
>>
>> Obviously this was a mistake on his side, but hey: everyone makes
>> mistakes. The real problem is to find this problem afterwards,
>> possibly weeks later, when you suddenly realize that a bug that you
>> had fixed suddenly reappears.
>
> Bisect?

This is not the point: My colleague knew exactly which commit contained 
the bugfix. The trouble was finding out why this bugfix disappeared even 
though everything indicated that it was cleanly merged into the current 
branch.

^ permalink raw reply

* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02  8:48 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, SZEDER Gábor, Junio C Hamano
In-Reply-To: <1328145320-14071-1-git-send-email-felipe.contreras@gmail.com>

Hi,

Felipe Contreras wrote:

> Felipe Contreras (4):
>   completion: be nicer with zsh

Since I can't find this patch in the mail archive, I'll reply here.
Luckily the most important bit is above already.

I think I mentioned before that this subject line is what will appear
in the shortlog and the shortlog is all that some people will see of
the changelog, so it should include a self-contained description of
the impact of the patch.

However, clearly I did not say it clearly enough. :)  I guess it's
better to take a cue from storytellers and show rather than tell.
(Please don't take this as a precedent --- I will not always be doing
the style fixes myself, and sometimes will consider a patch to scratch
someone else's itch not worth the trouble and work on something else.)

-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Thu, 2 Feb 2012 03:15:17 +0200
Subject: completion: avoid default value assignment on : true command

zsh versions from 4.3.0 to present (4.3.15) do not correctly propagate
the SH_WORD_SPLIT option into the subshell in ${foo:=$(bar)}
expressions.  For example, after running

	emulate sh
	fn () {
		var='one two'
		printf '%s\n' $var
	}
	x=$(fn)
	: ${y=$(fn)}

printing "$x" results in two lines as expected, but printing "$y"
results in a single line because $var is expanded as a single word
when evaluating fn to compute y.

So avoid the construct, and use an explicit 'test -n "$foo" ||
foo=$(bar)' instead.  This fixes a bug tht caused all commands to be
treated as porcelain and show up in "git <TAB><TAB>" completion,
because the list of all commands was treated as a single word in
__git_list_porcelain_commands and did not match any of the patterns
that would usually cause plumbing to be excluded.

[jn: clarified commit message, indentation style fix]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 contrib/completion/git-completion.bash |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 78be1958..d7965daf 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -676,7 +676,8 @@ __git_merge_strategies=
 # is needed.
 __git_compute_merge_strategies ()
 {
-	: ${__git_merge_strategies:=$(__git_list_merge_strategies)}
+	[ -n "$__git_merge_strategies" ] ||
+	__git_merge_strategies=$(__git_list_merge_strategies)
 }
 
 __git_complete_revlist_file ()
@@ -854,7 +855,8 @@ __git_list_all_commands ()
 __git_all_commands=
 __git_compute_all_commands ()
 {
-	: ${__git_all_commands:=$(__git_list_all_commands)}
+	[ -n "$__git_all_commands" ] ||
+	__git_all_commands=$(__git_list_all_commands)
 }
 
 __git_list_porcelain_commands ()
@@ -947,7 +949,8 @@ __git_porcelain_commands=
 __git_compute_porcelain_commands ()
 {
 	__git_compute_all_commands
-	: ${__git_porcelain_commands:=$(__git_list_porcelain_commands)}
+	[ -n "$__git_porcelain_commands" ] ||
+	__git_porcelain_commands=$(__git_list_porcelain_commands)
 }
 
 __git_pretty_aliases ()
-- 
1.7.9

^ permalink raw reply related

* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-02-02  8:34 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, SZEDER Gábor, Shawn O. Pearce
In-Reply-To: <20120202081622.GB3823@burratino>

On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> And yet another bug in zsh[1] causes a mismatch; zsh seems to have
>> problem emulating wordspliting, but only on the command substitution.
>
> Patches didn't hit the list again.  Any idea why?

No. A bug in list software?

I didn't get any warning or error.

-- 
Felipe Contreras

^ permalink raw reply

* Re: [PATCH 0/9] respect binary attribute in grep
From: Jeff King @ 2012-02-02  8:30 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
	Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>

On Thu, Feb 02, 2012 at 03:17:47AM -0500, Jeff King wrote:

> I implemented all of the other optimizations I mentioned except the
> "only stream the first few bytes when auto-detecting binary-ness" one.
> However, it should be easy to do on top of these changes. I need to
> re-visit the similar change to diff_filespec_is_binary, and I'll do both
> at the same time.

Oh, and I didn't even think about implementing streaming grep.  The
context-finding code relies on being able to backtrack through the file
in memory. We _could_ implement streaming only for binary files (i.e.,
when we will just print "Binary file foo matches"). However, I suspect
people with big binary files will want to be using "-I" anyway, so as to
avoid even pulling the data from disk at all.

We might eventually want to add a config-option version of "-I" for
people who have repositories of mixed source code and large binary
assets.

-Peff

^ permalink raw reply

* [PATCH 9/9] grep: pre-load userdiff drivers when threaded
From: Jeff King @ 2012-02-02  8:24 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
	Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>

The low-level grep_source code will automatically load the
userdiff driver to see whether a file is binary. However,
when we are threaded, it will load the drivers in a
non-deterministic order, handling each one as its assigned
thread happens to be scheduled.

Meanwhile, the attribute lookup code (which underlies the
userdiff driver lookup) is optimized to handle paths in
sequential order (because they tend to share the same
gitattributes files). Multi-threading the lookups destroys
the locality and makes this optimization less effective.

We can fix this by pre-loading the userdiff driver in the
main thread, before we hand off the file to a worker thread.
My best-of-five for "git grep foo" on the linux-2.6
repository went from:

  real    0m0.391s
  user    0m1.708s
  sys     0m0.584s

to:

  real    0m0.360s
  user    0m1.576s
  sys     0m0.572s

Not a huge speedup, but it's quite easy to do. The only
trick is that we shouldn't perform this optimization if "-a"
was used, in which case we won't bother checking whether
the files are binary at all.

Signed-off-by: Jeff King <peff@peff.net>
---
The speedup is especially unimpressive when you consider that it won't
grow as the grep load grows. This is a pretty fast grep. If you used a
real regex, the whole thing would take even longer, and you will still
only be shaving off a few tens of milliseconds. So I wouldn't be
heart-broken if this patch was dropped. I included it because it's easy
to do, and maybe somebody with a slower machine would find the absolute
time difference more noticeable.

 builtin/grep.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index bc85a20..9fc3e95 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -85,8 +85,8 @@ static pthread_cond_t cond_result;
 
 static int skip_first_line;
 
-static void add_work(enum grep_source_type type, const char *name,
-		     const void *id)
+static void add_work(struct grep_opt *opt, enum grep_source_type type,
+		     const char *name, const void *id)
 {
 	grep_lock();
 
@@ -95,6 +95,8 @@ static void add_work(enum grep_source_type type, const char *name,
 	}
 
 	grep_source_init(&todo[todo_end].source, type, name, id);
+	if (opt->binary != GREP_BINARY_TEXT)
+		grep_source_load_driver(&todo[todo_end].source);
 	todo[todo_end].done = 0;
 	strbuf_reset(&todo[todo_end].out);
 	todo_end = (todo_end + 1) % ARRAY_SIZE(todo);
@@ -333,7 +335,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
 
 #ifndef NO_PTHREADS
 	if (use_threads) {
-		add_work(GREP_SOURCE_SHA1, pathbuf.buf, sha1);
+		add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, sha1);
 		strbuf_release(&pathbuf);
 		return 0;
 	} else
@@ -362,7 +364,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 
 #ifndef NO_PTHREADS
 	if (use_threads) {
-		add_work(GREP_SOURCE_FILE, buf.buf, filename);
+		add_work(opt, GREP_SOURCE_FILE, buf.buf, filename);
 		strbuf_release(&buf);
 		return 0;
 	} else
-- 
1.7.9.3.gc3fce1.dirty

^ permalink raw reply related

* [PATCH 8/9] grep: load file data after checking binary-ness
From: Jeff King @ 2012-02-02  8:21 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
	Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>

Usually we load each file to grep into memory, check whether
it's binary, and then either grep it (the default) or not
(if "-I" was given).

In the "-I" case, we can skip loading the file entirely if
it is marked as binary via gitattributes. On my giant
3-gigabyte media repository, doing "git grep -I foo" went
from:

  real    0m0.712s
  user    0m0.044s
  sys     0m4.780s

to:

  real    0m0.026s
  user    0m0.016s
  sys     0m0.020s

Obviously this is an extreme example. The repo is almost
entirely binary files, and you can see that we spent all of
our time asking the kernel to read() the data. However, with
a cold disk cache, even avoiding a few binary files can have
an impact.

Signed-off-by: Jeff King <peff@peff.net>
---
 grep.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/grep.c b/grep.c
index a50d161..3821400 100644
--- a/grep.c
+++ b/grep.c
@@ -1019,9 +1019,6 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 	}
 	opt->last_shown = 0;
 
-	if (grep_source_load(gs) < 0)
-		return 0;
-
 	switch (opt->binary) {
 	case GREP_BINARY_DEFAULT:
 		if (grep_source_is_binary(gs))
@@ -1042,6 +1039,9 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 
 	try_lookahead = should_lookahead(opt);
 
+	if (grep_source_load(gs) < 0)
+		return 0;
+
 	bol = gs->buf;
 	left = gs->size;
 	while (left) {
-- 
1.7.9.3.gc3fce1.dirty

^ permalink raw reply related

* [PATCH 7/9] grep: respect diff attributes for binary-ness
From: Jeff King @ 2012-02-02  8:21 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
	Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@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.

Signed-off-by: Jeff King <peff@peff.net>
---
 grep.c                 |   16 ++++++++++++++--
 grep.h                 |    1 +
 t/t7008-grep-binary.sh |   24 ++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/grep.c b/grep.c
index bb18569..a50d161 100644
--- a/grep.c
+++ b/grep.c
@@ -1024,11 +1024,11 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 
 	switch (opt->binary) {
 	case GREP_BINARY_DEFAULT:
-		if (buffer_is_binary(gs->buf, gs->size))
+		if (grep_source_is_binary(gs))
 			binary_match_only = 1;
 		break;
 	case GREP_BINARY_NOMATCH:
-		if (buffer_is_binary(gs->buf, gs->size))
+		if (grep_source_is_binary(gs))
 			return 0; /* Assume unmatch */
 		break;
 	case GREP_BINARY_TEXT:
@@ -1350,3 +1350,15 @@ void grep_source_load_driver(struct grep_source *gs)
 		gs->driver = userdiff_find_by_name("default");
 	grep_attr_unlock();
 }
+
+int grep_source_is_binary(struct grep_source *gs)
+{
+	grep_source_load_driver(gs);
+	if (gs->driver->binary != -1)
+		return gs->driver->binary;
+
+	if (!grep_source_load(gs))
+		return buffer_is_binary(gs->buf, gs->size);
+
+	return 0;
+}
diff --git a/grep.h b/grep.h
index 73b28c2..36e49d8 100644
--- a/grep.h
+++ b/grep.h
@@ -152,6 +152,7 @@ int grep_source_load(struct grep_source *gs);
 void grep_source_clear_data(struct grep_source *gs);
 void grep_source_clear(struct grep_source *gs);
 void grep_source_load_driver(struct grep_source *gs);
+int grep_source_is_binary(struct grep_source *gs);
 
 int grep_source(struct grep_opt *opt, struct grep_source *gs);
 
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

* [PATCH 6/9] grep: cache userdiff_driver in grep_source
From: Jeff King @ 2012-02-02  8:20 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
	Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>

Right now, grep only uses the userdiff_driver for one thing:
looking up funcname patterns for "-p" and "-W".  As new uses
for userdiff drivers are added to the grep code, we want to
minimize attribute lookups, which can be expensive.

It might seem at first that this would also optimize multiple
lookups when the funcname pattern for a file is needed
multiple times. However, the compiled funcname pattern is
already cached in struct grep_opt's "priv" member, so
multiple lookups are already suppressed.

Signed-off-by: Jeff King <peff@peff.net>
---
 grep.c |   22 ++++++++++++++++------
 grep.h |    4 ++++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/grep.c b/grep.c
index 2a3fe7c..bb18569 100644
--- a/grep.c
+++ b/grep.c
@@ -841,12 +841,9 @@ static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bo
 {
 	xdemitconf_t *xecfg = opt->priv;
 	if (xecfg && !xecfg->find_func) {
-		struct userdiff_driver *drv;
-		grep_attr_lock();
-		drv = userdiff_find_by_path(gs->name);
-		grep_attr_unlock();
-		if (drv && drv->funcname.pattern) {
-			const struct userdiff_funcname *pe = &drv->funcname;
+		grep_source_load_driver(gs);
+		if (gs->driver->funcname.pattern) {
+			const struct userdiff_funcname *pe = &gs->driver->funcname;
 			xdiff_set_find_func(xecfg, pe->pattern, pe->cflags);
 		} else {
 			xecfg = opt->priv = NULL;
@@ -1237,6 +1234,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
 	gs->name = name ? xstrdup(name) : NULL;
 	gs->buf = NULL;
 	gs->size = 0;
+	gs->driver = NULL;
 
 	switch (type) {
 	case GREP_SOURCE_FILE:
@@ -1340,3 +1338,15 @@ int grep_source_load(struct grep_source *gs)
 	}
 	die("BUG: invalid grep_source type");
 }
+
+void grep_source_load_driver(struct grep_source *gs)
+{
+	if (gs->driver)
+		return;
+
+	grep_attr_lock();
+	gs->driver = userdiff_find_by_path(gs->name);
+	if (!gs->driver)
+		gs->driver = userdiff_find_by_name("default");
+	grep_attr_unlock();
+}
diff --git a/grep.h b/grep.h
index 8bf3001..73b28c2 100644
--- a/grep.h
+++ b/grep.h
@@ -9,6 +9,7 @@ typedef int pcre_extra;
 #endif
 #include "kwset.h"
 #include "thread-utils.h"
+#include "userdiff.h"
 
 enum grep_pat_token {
 	GREP_PATTERN,
@@ -141,6 +142,8 @@ struct grep_source {
 
 	char *buf;
 	unsigned long size;
+
+	struct userdiff_driver *driver;
 };
 
 void grep_source_init(struct grep_source *gs, enum grep_source_type type,
@@ -148,6 +151,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
 int grep_source_load(struct grep_source *gs);
 void grep_source_clear_data(struct grep_source *gs);
 void grep_source_clear(struct grep_source *gs);
+void grep_source_load_driver(struct grep_source *gs);
 
 int grep_source(struct grep_opt *opt, struct grep_source *gs);
 
-- 
1.7.9.3.gc3fce1.dirty

^ permalink raw reply related

* [PATCH 5/9] grep: drop grep_buffer's "name" parameter
From: Jeff King @ 2012-02-02  8:20 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
	Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>

Before the grep_source interface existed, grep_buffer was
used by two types of callers:

  1. Ones which pulled a file into a buffer, and then wanted
     to supply the file's name for the output (i.e.,
     git grep).

  2. Ones which really just wanted to grep a buffer (i.e.,
     git log --grep).

Callers in set (1) should now be using grep_source. Callers
in set (2) always pass NULL for the "name" parameter of
grep_buffer. We can therefore get rid of this now-useless
parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
This one isn't necessary, obviously, but I think it's a nice clean-up
after the last two patches.

 grep.c     |    4 ++--
 grep.h     |    2 +-
 revision.c |    1 -
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 8204ca2..2a3fe7c 100644
--- a/grep.c
+++ b/grep.c
@@ -1215,12 +1215,12 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs)
 	return grep_source_1(opt, gs, 0);
 }
 
-int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size)
+int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)
 {
 	struct grep_source gs;
 	int r;
 
-	grep_source_init(&gs, GREP_SOURCE_BUF, name, NULL);
+	grep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL);
 	gs.buf = buf;
 	gs.size = size;
 
diff --git a/grep.h b/grep.h
index e386ca4..8bf3001 100644
--- a/grep.h
+++ b/grep.h
@@ -127,7 +127,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, char *buf, unsigned long size);
 
 struct grep_source {
 	char *name;
diff --git a/revision.c b/revision.c
index c97d834..819ff01 100644
--- a/revision.c
+++ b/revision.c
@@ -2149,7 +2149,6 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
 	if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)
 		return 1;
 	return grep_buffer(&opt->grep_filter,
-			   NULL, /* we say nothing, not even filename */
 			   commit->buffer, strlen(commit->buffer));
 }
 
-- 
1.7.9.3.gc3fce1.dirty

^ permalink raw reply related

* [PATCH 4/9] convert git-grep to use grep_source interface
From: Jeff King @ 2012-02-02  8:19 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
	Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>

The grep_source interface (as opposed to grep_buffer) will
eventually gives us a richer interface for telling the
low-level grep code about our buffers. Eventually this will
lead to things like better binary-file handling. For now, it
lets us drop a lot of now-redundant code.

The conversion is mostly straight-forward. One thing to note
is that the memory ownership rules for "struct grep_source"
are different than the "struct work_item" found here (the
former will copy things like the filename, rather than
taking ownership). Therefore you will also see some slight
tweaking of when filename buffers are released.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/grep.c |  142 +++++++++-----------------------------------------------
 1 files changed, 23 insertions(+), 119 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index f4402fa..bc85a20 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -29,25 +29,12 @@ static int use_threads = 1;
 #define THREADS 8
 static pthread_t threads[THREADS];
 
-static void *load_sha1(const unsigned char *sha1, unsigned long *size,
-		       const char *name);
-static void *load_file(const char *filename, size_t *sz);
-
-enum work_type {WORK_SHA1, WORK_FILE};
-
 /* We use one producer thread and THREADS consumer
  * threads. The producer adds struct work_items to 'todo' and the
  * consumers pick work items from the same array.
  */
 struct work_item {
-	enum work_type type;
-	char *name;
-
-	/* if type == WORK_SHA1, then 'identifier' is a SHA1,
-	 * otherwise type == WORK_FILE, and 'identifier' is a NUL
-	 * terminated filename.
-	 */
-	void *identifier;
+	struct grep_source source;
 	char done;
 	struct strbuf out;
 };
@@ -98,7 +85,8 @@ static pthread_cond_t cond_result;
 
 static int skip_first_line;
 
-static void add_work(enum work_type type, char *name, void *id)
+static void add_work(enum grep_source_type type, const char *name,
+		     const void *id)
 {
 	grep_lock();
 
@@ -106,9 +94,7 @@ static void add_work(enum work_type type, char *name, void *id)
 		pthread_cond_wait(&cond_write, &grep_mutex);
 	}
 
-	todo[todo_end].type = type;
-	todo[todo_end].name = name;
-	todo[todo_end].identifier = id;
+	grep_source_init(&todo[todo_end].source, type, name, id);
 	todo[todo_end].done = 0;
 	strbuf_reset(&todo[todo_end].out);
 	todo_end = (todo_end + 1) % ARRAY_SIZE(todo);
@@ -136,21 +122,6 @@ static struct work_item *get_work(void)
 	return ret;
 }
 
-static void grep_sha1_async(struct grep_opt *opt, char *name,
-			    const unsigned char *sha1)
-{
-	unsigned char *s;
-	s = xmalloc(20);
-	memcpy(s, sha1, 20);
-	add_work(WORK_SHA1, name, s);
-}
-
-static void grep_file_async(struct grep_opt *opt, char *name,
-			    const char *filename)
-{
-	add_work(WORK_FILE, name, xstrdup(filename));
-}
-
 static void work_done(struct work_item *w)
 {
 	int old_done;
@@ -177,8 +148,7 @@ static void work_done(struct work_item *w)
 
 			write_or_die(1, p, len);
 		}
-		free(w->name);
-		free(w->identifier);
+		grep_source_clear(&w->source);
 	}
 
 	if (old_done != todo_done)
@@ -201,25 +171,8 @@ static void *run(void *arg)
 			break;
 
 		opt->output_priv = w;
-		if (w->type == WORK_SHA1) {
-			unsigned long sz;
-			void* data = load_sha1(w->identifier, &sz, w->name);
-
-			if (data) {
-				hit |= grep_buffer(opt, w->name, 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);
-				free(data);
-			}
-		} else {
-			assert(0);
-		}
-
+		hit |= grep_source(opt, &w->source);
+		grep_source_clear_data(&w->source);
 		work_done(w);
 	}
 	free_grep_patterns(arg);
@@ -365,23 +318,10 @@ static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type
 	return data;
 }
 
-static void *load_sha1(const unsigned char *sha1, unsigned long *size,
-		       const char *name)
-{
-	enum object_type type;
-	void *data = lock_and_read_sha1_file(sha1, &type, size);
-
-	if (!data)
-		error(_("'%s': unable to read %s"), name, sha1_to_hex(sha1));
-
-	return data;
-}
-
 static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
 		     const char *filename, int tree_name_len)
 {
 	struct strbuf pathbuf = STRBUF_INIT;
-	char *name;
 
 	if (opt->relative && opt->prefix_length) {
 		quote_path_relative(filename + tree_name_len, -1, &pathbuf,
@@ -391,87 +331,51 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
 		strbuf_addstr(&pathbuf, filename);
 	}
 
-	name = strbuf_detach(&pathbuf, NULL);
-
 #ifndef NO_PTHREADS
 	if (use_threads) {
-		grep_sha1_async(opt, name, sha1);
+		add_work(GREP_SOURCE_SHA1, pathbuf.buf, sha1);
+		strbuf_release(&pathbuf);
 		return 0;
 	} else
 #endif
 	{
+		struct grep_source gs;
 		int hit;
-		unsigned long sz;
-		void *data = load_sha1(sha1, &sz, name);
-		if (!data)
-			hit = 0;
-		else
-			hit = grep_buffer(opt, name, data, sz);
 
-		free(data);
-		free(name);
-		return hit;
-	}
-}
+		grep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, sha1);
+		strbuf_release(&pathbuf);
+		hit = grep_source(opt, &gs);
 
-static void *load_file(const char *filename, size_t *sz)
-{
-	struct stat st;
-	char *data;
-	int i;
-
-	if (lstat(filename, &st) < 0) {
-	err_ret:
-		if (errno != ENOENT)
-			error(_("'%s': %s"), filename, strerror(errno));
-		return NULL;
-	}
-	if (!S_ISREG(st.st_mode))
-		return NULL;
-	*sz = xsize_t(st.st_size);
-	i = open(filename, O_RDONLY);
-	if (i < 0)
-		goto err_ret;
-	data = xmalloc(*sz + 1);
-	if (st.st_size != read_in_full(i, data, *sz)) {
-		error(_("'%s': short read %s"), filename, strerror(errno));
-		close(i);
-		free(data);
-		return NULL;
+		grep_source_clear(&gs);
+		return hit;
 	}
-	close(i);
-	data[*sz] = 0;
-	return data;
 }
 
 static int grep_file(struct grep_opt *opt, const char *filename)
 {
 	struct strbuf buf = STRBUF_INIT;
-	char *name;
 
 	if (opt->relative && opt->prefix_length)
 		quote_path_relative(filename, -1, &buf, opt->prefix);
 	else
 		strbuf_addstr(&buf, filename);
-	name = strbuf_detach(&buf, NULL);
 
 #ifndef NO_PTHREADS
 	if (use_threads) {
-		grep_file_async(opt, name, filename);
+		add_work(GREP_SOURCE_FILE, buf.buf, filename);
+		strbuf_release(&buf);
 		return 0;
 	} else
 #endif
 	{
+		struct grep_source gs;
 		int hit;
-		size_t sz;
-		void *data = load_file(filename, &sz);
-		if (!data)
-			hit = 0;
-		else
-			hit = grep_buffer(opt, name, data, sz);
 
-		free(data);
-		free(name);
+		grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename);
+		strbuf_release(&buf);
+		hit = grep_source(opt, &gs);
+
+		grep_source_clear(&gs);
 		return hit;
 	}
 }
-- 
1.7.9.3.gc3fce1.dirty

^ permalink raw reply related

* [PATCH 3/9] grep: refactor the concept of "grep source" into an object
From: Jeff King @ 2012-02-02  8:19 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
	Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>

The main interface to the low-level grep code is
grep_buffer, which takes a pointer to a buffer and a size.
This is convenient and flexible (we use it to grep commit
bodies, files on disk, and blobs by sha1), but it makes it
hard to pass extra information about what we are grepping
(either for correctness, like overriding binary
auto-detection, or for optimizations, like lazily loading
blob contents).

Instead, let's encapsulate the idea of a "grep source",
including the buffer, its size, and where the data is coming
from. This is similar to the diff_filespec structure used by
the diff code (unsurprising, since future patches will
implement some of the same optimizations found there).

The diffstat is slightly scarier than the actual patch
content. Most of the modified lines are simply replacing
access to raw variables with their counterparts that are now
in a "struct grep_source". Most of the added lines were
taken from builtin/grep.c, which partially abstracted the
idea of grep sources (for file vs sha1 sources).

Instead of dropping the now-redundant code, this patch
leaves builtin/grep.c using the traditional grep_buffer
interface (which now wraps the grep_source interface). That
makes it easy to test that there is no change of behavior
(yet).

Signed-off-by: Jeff King <peff@peff.net>
---
 grep.c |  198 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 grep.h |   22 +++++++
 2 files changed, 186 insertions(+), 34 deletions(-)

diff --git a/grep.c b/grep.c
index db58a29..8204ca2 100644
--- a/grep.c
+++ b/grep.c
@@ -837,13 +837,13 @@ pthread_mutex_t grep_read_mutex;
 #define grep_attr_unlock()
 #endif
 
-static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol)
+static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol)
 {
 	xdemitconf_t *xecfg = opt->priv;
 	if (xecfg && !xecfg->find_func) {
 		struct userdiff_driver *drv;
 		grep_attr_lock();
-		drv = userdiff_find_by_path(name);
+		drv = userdiff_find_by_path(gs->name);
 		grep_attr_unlock();
 		if (drv && drv->funcname.pattern) {
 			const struct userdiff_funcname *pe = &drv->funcname;
@@ -866,33 +866,33 @@ static int match_funcname(struct grep_opt *opt, const char *name, char *bol, cha
 	return 0;
 }
 
-static void show_funcname_line(struct grep_opt *opt, const char *name,
-			       char *buf, char *bol, unsigned lno)
+static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,
+			       char *bol, unsigned lno)
 {
-	while (bol > buf) {
+	while (bol > gs->buf) {
 		char *eol = --bol;
 
-		while (bol > buf && bol[-1] != '\n')
+		while (bol > gs->buf && bol[-1] != '\n')
 			bol--;
 		lno--;
 
 		if (lno <= opt->last_shown)
 			break;
 
-		if (match_funcname(opt, name, bol, eol)) {
-			show_line(opt, bol, eol, name, lno, '=');
+		if (match_funcname(opt, gs, bol, eol)) {
+			show_line(opt, bol, eol, gs->name, lno, '=');
 			break;
 		}
 	}
 }
 
-static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,
+static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,
 			     char *bol, char *end, unsigned lno)
 {
 	unsigned cur = lno, from = 1, funcname_lno = 0;
 	int funcname_needed = !!opt->funcname;
 
-	if (opt->funcbody && !match_funcname(opt, name, bol, end))
+	if (opt->funcbody && !match_funcname(opt, gs, bol, end))
 		funcname_needed = 2;
 
 	if (opt->pre_context < lno)
@@ -901,14 +901,14 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,
 		from = opt->last_shown + 1;
 
 	/* Rewind. */
-	while (bol > buf &&
+	while (bol > gs->buf &&
 	       cur > (funcname_needed == 2 ? opt->last_shown + 1 : from)) {
 		char *eol = --bol;
 
-		while (bol > buf && bol[-1] != '\n')
+		while (bol > gs->buf && bol[-1] != '\n')
 			bol--;
 		cur--;
-		if (funcname_needed && match_funcname(opt, name, bol, eol)) {
+		if (funcname_needed && match_funcname(opt, gs, bol, eol)) {
 			funcname_lno = cur;
 			funcname_needed = 0;
 		}
@@ -916,7 +916,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,
 
 	/* We need to look even further back to find a function signature. */
 	if (opt->funcname && funcname_needed)
-		show_funcname_line(opt, name, buf, bol, cur);
+		show_funcname_line(opt, gs, bol, cur);
 
 	/* Back forward. */
 	while (cur < lno) {
@@ -924,7 +924,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,
 
 		while (*eol != '\n')
 			eol++;
-		show_line(opt, bol, eol, name, cur, sign);
+		show_line(opt, bol, eol, gs->name, cur, sign);
 		bol = eol + 1;
 		cur++;
 	}
@@ -991,11 +991,10 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)
 	fwrite(buf, size, 1, stdout);
 }
 
-static int grep_buffer_1(struct grep_opt *opt, const char *name,
-			 char *buf, unsigned long size, int collect_hits)
+static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)
 {
-	char *bol = buf;
-	unsigned long left = size;
+	char *bol;
+	unsigned long left;
 	unsigned lno = 1;
 	unsigned last_hit = 0;
 	int binary_match_only = 0;
@@ -1023,13 +1022,16 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 	}
 	opt->last_shown = 0;
 
+	if (grep_source_load(gs) < 0)
+		return 0;
+
 	switch (opt->binary) {
 	case GREP_BINARY_DEFAULT:
-		if (buffer_is_binary(buf, size))
+		if (buffer_is_binary(gs->buf, gs->size))
 			binary_match_only = 1;
 		break;
 	case GREP_BINARY_NOMATCH:
-		if (buffer_is_binary(buf, size))
+		if (buffer_is_binary(gs->buf, gs->size))
 			return 0; /* Assume unmatch */
 		break;
 	case GREP_BINARY_TEXT:
@@ -1043,6 +1045,8 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 
 	try_lookahead = should_lookahead(opt);
 
+	bol = gs->buf;
+	left = gs->size;
 	while (left) {
 		char *eol, ch;
 		int hit;
@@ -1091,14 +1095,14 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 			if (opt->status_only)
 				return 1;
 			if (opt->name_only) {
-				show_name(opt, name);
+				show_name(opt, gs->name);
 				return 1;
 			}
 			if (opt->count)
 				goto next_line;
 			if (binary_match_only) {
 				opt->output(opt, "Binary file ", 12);
-				output_color(opt, name, strlen(name),
+				output_color(opt, gs->name, strlen(gs->name),
 					     opt->color_filename);
 				opt->output(opt, " matches\n", 9);
 				return 1;
@@ -1107,23 +1111,23 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 			 * pre-context lines, we would need to show them.
 			 */
 			if (opt->pre_context || opt->funcbody)
-				show_pre_context(opt, name, buf, bol, eol, lno);
+				show_pre_context(opt, gs, bol, eol, lno);
 			else if (opt->funcname)
-				show_funcname_line(opt, name, buf, bol, lno);
-			show_line(opt, bol, eol, name, lno, ':');
+				show_funcname_line(opt, gs, bol, lno);
+			show_line(opt, bol, eol, gs->name, lno, ':');
 			last_hit = lno;
 			if (opt->funcbody)
 				show_function = 1;
 			goto next_line;
 		}
-		if (show_function && match_funcname(opt, name, bol, eol))
+		if (show_function && match_funcname(opt, gs, bol, eol))
 			show_function = 0;
 		if (show_function ||
 		    (last_hit && lno <= last_hit + opt->post_context)) {
 			/* If the last hit is within the post context,
 			 * we need to show this line.
 			 */
-			show_line(opt, bol, eol, name, lno, '-');
+			show_line(opt, bol, eol, gs->name, lno, '-');
 		}
 
 	next_line:
@@ -1141,7 +1145,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 		return 0;
 	if (opt->unmatch_name_only) {
 		/* We did not see any hit, so we want to show this */
-		show_name(opt, name);
+		show_name(opt, gs->name);
 		return 1;
 	}
 
@@ -1155,7 +1159,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 	 */
 	if (opt->count && count) {
 		char buf[32];
-		output_color(opt, name, strlen(name), opt->color_filename);
+		output_color(opt, gs->name, strlen(gs->name), opt->color_filename);
 		output_sep(opt, ':');
 		snprintf(buf, sizeof(buf), "%u\n", count);
 		opt->output(opt, buf, strlen(buf));
@@ -1190,23 +1194,149 @@ 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_source(struct grep_opt *opt, struct grep_source *gs)
 {
 	/*
 	 * 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_source_1(opt, gs, 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_source_1(opt, gs, 1);
 
 	if (!chk_hit_marker(opt->pattern_expression))
 		return 0;
 
-	return grep_buffer_1(opt, name, buf, size, 0);
+	return grep_source_1(opt, gs, 0);
+}
+
+int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size)
+{
+	struct grep_source gs;
+	int r;
+
+	grep_source_init(&gs, GREP_SOURCE_BUF, name, NULL);
+	gs.buf = buf;
+	gs.size = size;
+
+	r = grep_source(opt, &gs);
+
+	grep_source_clear(&gs);
+	return r;
+}
+
+void grep_source_init(struct grep_source *gs, enum grep_source_type type,
+		      const char *name, const void *identifier)
+{
+	gs->type = type;
+	gs->name = name ? xstrdup(name) : NULL;
+	gs->buf = NULL;
+	gs->size = 0;
+
+	switch (type) {
+	case GREP_SOURCE_FILE:
+		gs->identifier = xstrdup(identifier);
+		break;
+	case GREP_SOURCE_SHA1:
+		gs->identifier = xmalloc(20);
+		memcpy(gs->identifier, identifier, 20);
+		break;
+	case GREP_SOURCE_BUF:
+		gs->identifier = NULL;
+	}
+}
+
+void grep_source_clear(struct grep_source *gs)
+{
+	free(gs->name);
+	gs->name = NULL;
+	free(gs->identifier);
+	gs->identifier = NULL;
+	grep_source_clear_data(gs);
+}
+
+void grep_source_clear_data(struct grep_source *gs)
+{
+	switch (gs->type) {
+	case GREP_SOURCE_FILE:
+	case GREP_SOURCE_SHA1:
+		free(gs->buf);
+		gs->buf = NULL;
+		gs->size = 0;
+		break;
+	case GREP_SOURCE_BUF:
+		/* leave user-provided buf intact */
+		break;
+	}
+}
+
+static int grep_source_load_sha1(struct grep_source *gs)
+{
+	enum object_type type;
+
+	grep_read_lock();
+	gs->buf = read_sha1_file(gs->identifier, &type, &gs->size);
+	grep_read_unlock();
+
+	if (!gs->buf)
+		return error(_("'%s': unable to read %s"),
+			     gs->name,
+			     sha1_to_hex(gs->identifier));
+	return 0;
+}
+
+static int grep_source_load_file(struct grep_source *gs)
+{
+	const char *filename = gs->identifier;
+	struct stat st;
+	char *data;
+	size_t size;
+	int i;
+
+	if (lstat(filename, &st) < 0) {
+	err_ret:
+		if (errno != ENOENT)
+			error(_("'%s': %s"), filename, strerror(errno));
+		return -1;
+	}
+	if (!S_ISREG(st.st_mode))
+		return -1;
+	size = xsize_t(st.st_size);
+	i = open(filename, O_RDONLY);
+	if (i < 0)
+		goto err_ret;
+	data = xmalloc(size + 1);
+	if (st.st_size != read_in_full(i, data, size)) {
+		error(_("'%s': short read %s"), filename, strerror(errno));
+		close(i);
+		free(data);
+		return -1;
+	}
+	close(i);
+	data[size] = 0;
+
+	gs->buf = data;
+	gs->size = size;
+	return 0;
+}
+
+int grep_source_load(struct grep_source *gs)
+{
+	if (gs->buf)
+		return 0;
+
+	switch (gs->type) {
+	case GREP_SOURCE_FILE:
+		return grep_source_load_file(gs);
+	case GREP_SOURCE_SHA1:
+		return grep_source_load_sha1(gs);
+	case GREP_SOURCE_BUF:
+		return gs->buf ? 0 : -1;
+	}
+	die("BUG: invalid grep_source type");
 }
diff --git a/grep.h b/grep.h
index 4f1b025..e386ca4 100644
--- a/grep.h
+++ b/grep.h
@@ -129,6 +129,28 @@ 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);
 
+struct grep_source {
+	char *name;
+
+	enum grep_source_type {
+		GREP_SOURCE_SHA1,
+		GREP_SOURCE_FILE,
+		GREP_SOURCE_BUF,
+	} type;
+	void *identifier;
+
+	char *buf;
+	unsigned long size;
+};
+
+void grep_source_init(struct grep_source *gs, enum grep_source_type type,
+		      const char *name, const void *identifier);
+int grep_source_load(struct grep_source *gs);
+void grep_source_clear_data(struct grep_source *gs);
+void grep_source_clear(struct grep_source *gs);
+
+int grep_source(struct grep_opt *opt, struct grep_source *gs);
+
 extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);
 extern int grep_threads_ok(const struct grep_opt *opt);
 
-- 
1.7.9.3.gc3fce1.dirty

^ permalink raw reply related

* [PATCH 2/9] grep: move sha1-reading mutex into low-level code
From: Jeff King @ 2012-02-02  8:18 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
	Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>

The multi-threaded git-grep code needs to serialize access
to the thread-unsafe read_sha1_file call. It does this with
a mutex that is local to builtin/grep.c.

Let's instead push this down into grep.c, where it can be
used by both builtin/grep.c and grep.c. This will let us
safely teach the low-level grep.c code tricks that involve
reading from the object db.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/grep.c |   29 ++++++-----------------------
 grep.c         |    6 ++++++
 grep.h         |   17 +++++++++++++++++
 3 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 06983f9..f4402fa 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -85,21 +85,6 @@ static inline void grep_unlock(void)
 		pthread_mutex_unlock(&grep_mutex);
 }
 
-/* Used to serialize calls to read_sha1_file. */
-static pthread_mutex_t read_sha1_mutex;
-
-static inline void read_sha1_lock(void)
-{
-	if (use_threads)
-		pthread_mutex_lock(&read_sha1_mutex);
-}
-
-static inline void read_sha1_unlock(void)
-{
-	if (use_threads)
-		pthread_mutex_unlock(&read_sha1_mutex);
-}
-
 /* Signalled when a new work_item is added to todo. */
 static pthread_cond_t cond_add;
 
@@ -254,7 +239,7 @@ static void start_threads(struct grep_opt *opt)
 	int i;
 
 	pthread_mutex_init(&grep_mutex, NULL);
-	pthread_mutex_init(&read_sha1_mutex, NULL);
+	pthread_mutex_init(&grep_read_mutex, NULL);
 	pthread_mutex_init(&grep_attr_mutex, NULL);
 	pthread_cond_init(&cond_add, NULL);
 	pthread_cond_init(&cond_write, NULL);
@@ -303,7 +288,7 @@ static int wait_all(void)
 	}
 
 	pthread_mutex_destroy(&grep_mutex);
-	pthread_mutex_destroy(&read_sha1_mutex);
+	pthread_mutex_destroy(&grep_read_mutex);
 	pthread_mutex_destroy(&grep_attr_mutex);
 	pthread_cond_destroy(&cond_add);
 	pthread_cond_destroy(&cond_write);
@@ -313,8 +298,6 @@ static int wait_all(void)
 	return hit;
 }
 #else /* !NO_PTHREADS */
-#define read_sha1_lock()
-#define read_sha1_unlock()
 
 static int wait_all(void)
 {
@@ -376,9 +359,9 @@ static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type
 {
 	void *data;
 
-	read_sha1_lock();
+	grep_read_lock();
 	data = read_sha1_file(sha1, type, size);
-	read_sha1_unlock();
+	grep_read_unlock();
 	return data;
 }
 
@@ -617,10 +600,10 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
 		struct strbuf base;
 		int hit, len;
 
-		read_sha1_lock();
+		grep_read_lock();
 		data = read_object_with_reference(obj->sha1, tree_type,
 						  &size, NULL);
-		read_sha1_unlock();
+		grep_read_unlock();
 
 		if (!data)
 			die(_("unable to read tree (%s)"), sha1_to_hex(obj->sha1));
diff --git a/grep.c b/grep.c
index 7a67c2f..db58a29 100644
--- a/grep.c
+++ b/grep.c
@@ -826,6 +826,12 @@ static inline void grep_attr_unlock(void)
 	if (grep_use_locks)
 		pthread_mutex_unlock(&grep_attr_mutex);
 }
+
+/*
+ * Same as git_attr_mutex, but protecting the thread-unsafe object db access.
+ */
+pthread_mutex_t grep_read_mutex;
+
 #else
 #define grep_attr_lock()
 #define grep_attr_unlock()
diff --git a/grep.h b/grep.h
index 3653bb3..4f1b025 100644
--- a/grep.h
+++ b/grep.h
@@ -139,6 +139,23 @@ extern int grep_threads_ok(const struct grep_opt *opt);
  */
 extern int grep_use_locks;
 extern pthread_mutex_t grep_attr_mutex;
+extern pthread_mutex_t grep_read_mutex;
+
+static inline void grep_read_lock(void)
+{
+	if (grep_use_locks)
+		pthread_mutex_lock(&grep_read_mutex);
+}
+
+static inline void grep_read_unlock(void)
+{
+	if (grep_use_locks)
+		pthread_mutex_unlock(&grep_read_mutex);
+}
+
+#else
+#define grep_read_lock()
+#define grep_read_unlock()
 #endif
 
 #endif
-- 
1.7.9.3.gc3fce1.dirty

^ permalink raw reply related

* [PATCH 1/9] grep: make locking flag global
From: Jeff King @ 2012-02-02  8:18 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
	Dov Grobgeld
In-Reply-To: <20120202081747.GA10271@sigill.intra.peff.net>

The low-level grep code traditionally didn't care about
threading, as it doesn't do any threading itself and didn't
call out to other non-thread-safe code.  That changed with
0579f91 (grep: enable threading with -p and -W using lazy
attribute lookup, 2011-12-12), which pushed the lookup of
funcname attributes (which is not thread-safe) into the
low-level grep code.

As a result, the low-level code learned about a new global
"grep_attr_mutex" to serialize access to the attribute code.
A multi-threaded caller (e.g., builtin/grep.c) is expected
to initialize the mutex and set "use_threads" in the
grep_opt structure. The low-level code only uses the lock if
use_threads is set.

However, putting the use_threads flag into the grep_opt
struct is not the most logical place. Whether threading is
in use is not something that matters for each call to
grep_buffer, but is instead global to the whole program
(i.e., if any thread is doing multi-threaded grep, every
other thread, even if it thinks it is doing its own
single-threaded grep, would need to use the locking).  In
practice, this distinction isn't a problem for us, because
the only user of multi-threaded grep is "git-grep", which
does nothing except call grep.

This patch turns the opt->use_threads flag into a global
flag. More important than the nit-picking semantic argument
above is that this means that the locking functions don't
need to actually have access to a grep_opt to know whether
to lock. Which in turn can make adding new locks simpler, as
we don't need to pass around a grep_opt.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/grep.c |    4 ++--
 grep.c         |   18 ++++++++++--------
 grep.h         |    2 +-
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 5c2ae94..06983f9 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -259,6 +259,7 @@ static void start_threads(struct grep_opt *opt)
 	pthread_cond_init(&cond_add, NULL);
 	pthread_cond_init(&cond_write, NULL);
 	pthread_cond_init(&cond_result, NULL);
+	grep_use_locks = 1;
 
 	for (i = 0; i < ARRAY_SIZE(todo); i++) {
 		strbuf_init(&todo[i].out, 0);
@@ -307,6 +308,7 @@ static int wait_all(void)
 	pthread_cond_destroy(&cond_add);
 	pthread_cond_destroy(&cond_write);
 	pthread_cond_destroy(&cond_result);
+	grep_use_locks = 0;
 
 	return hit;
 }
@@ -1030,8 +1032,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	use_threads = 0;
 #endif
 
-	opt.use_threads = use_threads;
-
 #ifndef NO_PTHREADS
 	if (use_threads) {
 		if (!(opt.name_only || opt.unmatch_name_only || opt.count)
diff --git a/grep.c b/grep.c
index 486230b..7a67c2f 100644
--- a/grep.c
+++ b/grep.c
@@ -807,26 +807,28 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 }
 
 #ifndef NO_PTHREADS
+int grep_use_locks;
+
 /*
  * This lock protects access to the gitattributes machinery, which is
  * not thread-safe.
  */
 pthread_mutex_t grep_attr_mutex;
 
-static inline void grep_attr_lock(struct grep_opt *opt)
+static inline void grep_attr_lock(void)
 {
-	if (opt->use_threads)
+	if (grep_use_locks)
 		pthread_mutex_lock(&grep_attr_mutex);
 }
 
-static inline void grep_attr_unlock(struct grep_opt *opt)
+static inline void grep_attr_unlock(void)
 {
-	if (opt->use_threads)
+	if (grep_use_locks)
 		pthread_mutex_unlock(&grep_attr_mutex);
 }
 #else
-#define grep_attr_lock(opt)
-#define grep_attr_unlock(opt)
+#define grep_attr_lock()
+#define grep_attr_unlock()
 #endif
 
 static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol)
@@ -834,9 +836,9 @@ static int match_funcname(struct grep_opt *opt, const char *name, char *bol, cha
 	xdemitconf_t *xecfg = opt->priv;
 	if (xecfg && !xecfg->find_func) {
 		struct userdiff_driver *drv;
-		grep_attr_lock(opt);
+		grep_attr_lock();
 		drv = userdiff_find_by_path(name);
-		grep_attr_unlock(opt);
+		grep_attr_unlock();
 		if (drv && drv->funcname.pattern) {
 			const struct userdiff_funcname *pe = &drv->funcname;
 			xdiff_set_find_func(xecfg, pe->pattern, pe->cflags);
diff --git a/grep.h b/grep.h
index fb205f3..3653bb3 100644
--- a/grep.h
+++ b/grep.h
@@ -116,7 +116,6 @@ struct grep_opt {
 	int show_hunk_mark;
 	int file_break;
 	int heading;
-	int use_threads;
 	void *priv;
 
 	void (*output)(struct grep_opt *opt, const void *data, size_t size);
@@ -138,6 +137,7 @@ extern int grep_threads_ok(const struct grep_opt *opt);
  * Mutex used around access to the attributes machinery if
  * opt->use_threads.  Must be initialized/destroyed by callers!
  */
+extern int grep_use_locks;
 extern pthread_mutex_t grep_attr_mutex;
 #endif
 
-- 
1.7.9.3.gc3fce1.dirty

^ permalink raw reply related

* [PATCH 0/9] respect binary attribute in grep
From: Jeff King @ 2012-02-02  8:17 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, Conrad Irwin, git, Nguyen Thai Ngoc Duy,
	Dov Grobgeld
In-Reply-To: <20120202005209.GA6883@sigill.intra.peff.net>

[+cc Thomas, as I am mangling some of his recent work with my
     refactoring]

On Wed, Feb 01, 2012 at 07:52:09PM -0500, Jeff King wrote:

> > 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'll take a look at re-working it that way.

Thanks for a dose of sanity. The result turned out much easier to read
(and explain in the commit messages, as it was simple to break into
smaller commits). In particular, the "don't read binary-marked files at
all with -I" optimization became very natural.

I implemented all of the other optimizations I mentioned except the
"only stream the first few bytes when auto-detecting binary-ness" one.
However, it should be easy to do on top of these changes. I need to
re-visit the similar change to diff_filespec_is_binary, and I'll do both
at the same time.

The patches are:

  [1/9]: grep: make locking flag global
  [2/9]: grep: move sha1-reading mutex into low-level code
  [3/9]: grep: refactor the concept of "grep source" into an object
  [4/9]: convert git-grep to use grep_source interface
  [5/9]: grep: drop grep_buffer's "name" parameter
  [6/9]: grep: cache userdiff_driver in grep_source

    These are all refactoring that should have no behavior change.

  [7/9]: grep: respect diff attributes for binary-ness

    This is the point of the series. :)

  [8/9]: grep: load file data after checking binary-ness
  [9/9]: grep: pre-load userdiff drivers when threaded

    And these two are simple optimizations.

-Peff

^ permalink raw reply

* Re: How to find and analyze bad merges?
From: Junio C Hamano @ 2012-02-02  8:16 UTC (permalink / raw)
  To: norbert.nemec; +Cc: git
In-Reply-To: <jgdgcv$h8n$1@dough.gmane.org>

"norbert.nemec" <norbert.nemec@native-instruments.de> writes:

> a colleague of mine happened to produce a bad merge by unintenionally
> picking the version of the remote branch ("R") for all conflicting
> files. Effectively, he eliminated a whole bunch of bugfixes that were
> already on his local branch ("L").
>
> Obviously this was a mistake on his side, but hey: everyone makes
> mistakes. The real problem is to find this problem afterwards,
> possibly weeks later, when you suddenly realize that a bug that you
> had fixed suddenly reappears.

Bisect?

^ permalink raw reply

* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-02-02  8:16 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, SZEDER Gábor, Shawn O. Pearce
In-Reply-To: <1328145320-14071-2-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras wrote:

> And yet another bug in zsh[1] causes a mismatch; zsh seems to have
> problem emulating wordspliting, but only on the command substitution.

Patches didn't hit the list again.  Any idea why?

^ permalink raw reply

* Re: [PATCH] i18n: po for zh_cn
From: Junio C Hamano @ 2012-02-02  8:15 UTC (permalink / raw)
  To: Jiang Xin; +Cc: Frederik Schwarzer, git
In-Reply-To: <CANYiYbEG+QqBQi6Xagk-bmRoz3So_f0HVtZRb8+T0f9hXcuq4w@mail.gmail.com>

Jiang Xin <worldhello.net@gmail.com> writes:

> 2012/2/2 Junio C Hamano <gitster@pobox.com>:
> ...
>>  (6) From time to time, the l10n coordinator will pull from "git.git" when
>>     meaningful number of changes are made to the translatable strings.  I
>>     hope this would happen much less often than once per week, preferably
>>     much less frequently. The l10n coordinator updates po/git.pot and
>>     makes a commit, and notifies the l10n teams.
>
> notifies the l10n teams using another mailing list maybe.

I personally think using the regular git@vger.kernel.org list would be
preferrable for this.  People who translate would want to learn the
reasoning that led to the final phrasing of the messages to come up with
usable translation, and following the main list would be one way to do
so. Having to follow two separate list will be an unnecessary burden.

But the choice of how to coordinate his or her work with the l10n teams is
entirely up to the l10n coordinator.

>> And i18n patches that modify the _() markings in the source code, will
>> only go through "git.git" tree in the normal channel, IOW, this mailing
>> list.
>
> Yeah,  works on the codes but not the po files still send to this list.

Many if not all of the po files in gitk and git-gui were discussed on this
list and integrated to the tree with the usual "format-patch to am" review
flow just fine.  You keep saying that l10n patches does not work, but that
is patently wrong.

It is a different matter if it is the most convenient for l10n patches for
both the l10n folks and others to review and discuss on this list, or on
web based services like GitHub.

^ permalink raw reply

* How to find and analyze bad merges?
From: norbert.nemec @ 2012-02-02  8:10 UTC (permalink / raw)
  To: git

Hi there,

a colleague of mine happened to produce a bad merge by unintenionally 
picking the version of the remote branch ("R") for all conflicting 
files. Effectively, he eliminated a whole bunch of bugfixes that were 
already on his local branch ("L").

Obviously this was a mistake on his side, but hey: everyone makes 
mistakes. The real problem is to find this problem afterwards, possibly 
weeks later, when you suddenly realize that a bug that you had fixed 
suddenly reappears.

A "git log" on the whole repository shows both branches R and L.
A "git show" on the bugfix commit shows the bugfix as you expect it.

BUT:
A "git log" on the file itself shows neither the problematic merge nor 
the bugfix commit. Git considers the merge of this file trivial because 
the content is identical to that of parent R. Therefore, whatever 
happened on branch L is not considered relevant history of the file.

FURTHERMORE:
A "git show" of the merge itself does not show the conflicting file 
either. Obviously, "git show" on a merge decides which files are 
relevant not based on conflicts but based on resolutions.

To sort out what happened, you first need to have a suspicion and then 
dig fairly deep in the manuals to set the correct options to show what 
happened.

I think, both "git log" and "git show" should by default be a bit more 
conservative in hiding "insignificant" merges:
* In "git log" a branch should only be hidden if it never touched the file.
* In "git show" a merge should display all files that did have a 
conflict independent of the resolution. (I am open to discuss whether 
auto-resolvable conflicts should be displayed by default. Non-trivial 
conflicts definitely should)

Greetings,
Norbert

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox