* Re: git auto-repack is broken...
From: Nicolas Pitre @ 2011-12-08 0:18 UTC (permalink / raw)
To: Jeff King
Cc: Brandon Casey, Junio C Hamano, Linus Torvalds,
Ævar Arnfjörð, Git Mailing List
In-Reply-To: <20111207225318.GA21852@sigill.intra.peff.net>
On Wed, 7 Dec 2011, Jeff King wrote:
> On Wed, Dec 07, 2011 at 05:12:14PM -0500, Nicolas Pitre wrote:
>
> > Maybe FETCH_HEAD should have a reflog too?
>
> That might be nice. However, there is a complication, in that FETCH_HEAD
> may contain many sha1s, but each reflog entry only has room for a single
> sha1 transition. You could obviously encode it as a series of reflog
> entries, but then "git show FETCH_HEAD@{1}" is not very meaningful.
What does "git show FETCH_HEAD" do now? If it shows only one
(presumably the first) SHA1 then its reflog doesn't have to be smarter,
which would properly cover most cases already. I certainly never did a
multi-ref fetch myself.
Nicolas
^ permalink raw reply
* Re: [PATCH 1/4] test: add missing "&&" after echo command
From: Jeff King @ 2011-12-07 23:11 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, git, Vijay Lakshminarayanan, Viresh Kumar,
Shiraz HASHIM
In-Reply-To: <20111207231002.GD21852@sigill.intra.peff.net>
On Wed, Dec 07, 2011 at 06:10:03PM -0500, Jeff King wrote:
> On Wed, Dec 07, 2011 at 08:45:40AM -0600, Jonathan Nieder wrote:
>
> > diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> > index 3ad04363..da75abc1 100755
> > --- a/t/t7501-commit.sh
> > +++ b/t/t7501-commit.sh
> > @@ -60,7 +60,7 @@ test_expect_success \
> >
> > test_expect_success \
> > "next commit" \
> > - "echo 'bongo bongo bongo' >file \
> > + "echo 'bongo bongo bongo' >file && \
> > git commit -m next -a"
>
> Patch is obviously correct, but isn't the "\" here just superfluous and
> error-prone? Maybe it should just be dropped from the new version (and
> possibly from other tests in t7501).
Oh, nevermind. I just read your patch 3, which does that and much more.
I approve.
-Peff
^ permalink raw reply
* Re: [PATCH 1/4] test: add missing "&&" after echo command
From: Jeff King @ 2011-12-07 23:10 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, git, Vijay Lakshminarayanan, Viresh Kumar,
Shiraz HASHIM
In-Reply-To: <20111207144540.GB30157@elie.hsd1.il.comcast.net>
On Wed, Dec 07, 2011 at 08:45:40AM -0600, Jonathan Nieder wrote:
> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> index 3ad04363..da75abc1 100755
> --- a/t/t7501-commit.sh
> +++ b/t/t7501-commit.sh
> @@ -60,7 +60,7 @@ test_expect_success \
>
> test_expect_success \
> "next commit" \
> - "echo 'bongo bongo bongo' >file \
> + "echo 'bongo bongo bongo' >file && \
> git commit -m next -a"
Patch is obviously correct, but isn't the "\" here just superfluous and
error-prone? Maybe it should just be dropped from the new version (and
possibly from other tests in t7501).
-Peff
^ permalink raw reply
* Re: Odd issue - The Diffs That WILL NOT DIE.
From: Jeff King @ 2011-12-07 23:06 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: Chris Patti, git
In-Reply-To: <20111207225827.GB641@centaur.lab.cmartin.tk>
On Wed, Dec 07, 2011 at 11:58:27PM +0100, Carlos Martín Nieto wrote:
> If you want to use OSX to develop this project, you'll have to either
> rename one of those files or set your filesystem to be case-sensitive
> (and unset core.ignorecase afterwards). From what I've heard, the OS
> itself will work fine with a case-sensitive filesystem, but not all
> applications might. YMMV.
I've never done it, but my understanding is that for HFS+, going
case-sensitive is not a simple flip of a switch, but you have to
actually make a new filesystem. Given that complexity, and the fact that
some other apps might not like it, your best bet might be to create a
new case-sensitive filesystem in a loopback file, and then mount that
just for this project.
I'm not sure of the exact commands under OS X, but I'm sure some
googling could probably turn up a solution.
-Peff
^ permalink raw reply
* Re: Odd issue - The Diffs That WILL NOT DIE.
From: Jeff King @ 2011-12-07 23:03 UTC (permalink / raw)
To: Chris Patti; +Cc: Carlos Martín Nieto, git
In-Reply-To: <CAJ8P3RB=Gj-QCe6meqXSZ7N8+PnfNxSD8omUxT6dDh00bUf0QQ@mail.gmail.com>
On Wed, Dec 07, 2011 at 05:24:01PM -0500, Chris Patti wrote:
> Yup, you nailed it. The files in question are CloudSponge.php
> (deleted) and Cloudsponge.php (still being actively maintained).
> [...]
> Actually I'm wrong on that count, but in an interesting way.
>
> Both CloudSponge.php and Cloudsponge.php exist and are *not* deleted
> in the remote repository, but on OSX only Cloudsponge.php shows up on
> the filesystem, yet CloudSponge.php is being reported as modified.
>
> Turns out two of our other developers are also seeing this behavior.
OK. Then it's not a bug[1], but rather an incompatibility issue. Your
repository stores two files which differ only in case, but your
underlying case-insensitive filesystem is incapable of storing both of
them simultaneously.
There's really no way around it except to either use a case-sensitive
filesystem, rename your files (or if one if obsolete, get rid of it).
[1] Arguably git could be better at noticing and reporting this issue,
but that wouldn't change the fundamental problem.
> I am seeing the same behavior with 1.7.7.4 which I backrevved to
> yesterday while troubleshooting this issue. Can you suggest an older
> version for me to try next?
No, you'll see the same behavior in all versions of git (I was mostly
concerned that it was a regression new in v1.7.8, as I fixed some
case-sensitivity bugs recently).
> I'm not sure how I would git bisect in this case, I'd need to have all
> the different git revs installed in order to do that right? (I'm
> relatively new to git bisect, just figured it out the other day).
You can't bisect, because you don't have a version that actually worked.
But if you had, you would do it by cloning and building the git
repository itself, then starting a bisection like:
git clone git://git.kernel.org/pub/scm/git/git.git
git bisect start
git bisect bad v1.7.8
git bisect good v1.7.7.4 ;# if it had turned out to be OK
and then repeatedly build and try your test with each version of git,
until if finally narrowed it down to a single offending commit.
-Peff
^ permalink raw reply
* Re: Odd issue - The Diffs That WILL NOT DIE.
From: Carlos Martín Nieto @ 2011-12-07 22:58 UTC (permalink / raw)
To: Chris Patti; +Cc: Jeff King, git
In-Reply-To: <CAJ8P3RA48W7ZiABvjkn_KkU-JPnCnaF_X_WK0wPtToph3DGDvg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]
On Wed, Dec 07, 2011 at 05:30:19PM -0500, Chris Patti wrote:
> Actually I'm wrong on that count, but in an interesting way.
>
> Both CloudSponge.php and Cloudsponge.php exist and are *not* deleted
> in the remote repository, but on OSX only Cloudsponge.php shows up on
> the filesystem, yet CloudSponge.php is being reported as modified.
>
> Turns out two of our other developers are also seeing this behavior.
HFS+ (the filesystem used by OSX) is case insensitive unless you
toggle some magic switch, so git set core.ignorecase to true on clone
(the config manpage claims its done on init or clone, in any case)
which makes it assume that Cloudsponge.php and CloudSponge.php are the
same file (which in a case-insensitive filesystem is true).
If you want to use OSX to develop this project, you'll have to either
rename one of those files or set your filesystem to be case-sensitive
(and unset core.ignorecase afterwards). From what I've heard, the OS
itself will work fine with a case-sensitive filesystem, but not all
applications might. YMMV.
cmn
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: git auto-repack is broken...
From: Jeff King @ 2011-12-07 22:53 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Brandon Casey, Junio C Hamano, Linus Torvalds,
Ævar Arnfjörð, Git Mailing List
In-Reply-To: <alpine.LFD.2.02.1112071709250.2907@xanadu.home>
On Wed, Dec 07, 2011 at 05:12:14PM -0500, Nicolas Pitre wrote:
> Maybe FETCH_HEAD should have a reflog too?
That might be nice. However, there is a complication, in that FETCH_HEAD
may contain many sha1s, but each reflog entry only has room for a single
sha1 transition. You could obviously encode it as a series of reflog
entries, but then "git show FETCH_HEAD@{1}" is not very meaningful.
-Peff
^ permalink raw reply
* Re: Debugging git-commit slowness on a large repo
From: Joshua Redstone @ 2011-12-07 22:48 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Carlos Martín Nieto, Tomas Carnecky, Junio C Hamano,
git@vger.kernel.org
In-Reply-To: <CACsJy8Dbd+v+8FzvQS9a4C8DQSxQGgqQNGaLhL1cHv-yMnaCJQ@mail.gmail.com>
Hi Duy,
Thanks for the documentation link.
git ls-files shows 100k files, which matches # of files in the working
tree ('find . -type f -print | wc -l').
I added a 'git read-tree HEAD' before the git-add, and a 'git write-tree'
after the add. With that, the commit time slowed down to 8 seconds per
commit, plus 4 more seconds for the read-tree/add/write-tree ops. The
read-tree/add/write-tree each took about a second.
As an experiment, I also tried removing the 'git read-tree' and just
having the git-write-tree. That sped up commits to 0.6 seconds, but the
overall time for add/write-tree/commit was still 3 to 6 seconds.
For comparison, without the read-tree and write-tree, commits take about 1
second and add/commit in total takes about 2 seconds.
It surprises me that the presence of git read-tree or write-tree would
slow things down so much.
Josh
On 12/6/11 6:08 PM, "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> wrote:
>On Wed, Dec 7, 2011 at 8:48 AM, Joshua Redstone <joshua.redstone@fb.com>
>wrote:
>> I tried doing a 'git read-tree HEAD' before each 'git add ; git commit'
>> iteration, and the time for git-commit jumped from about 1 second to
>>about
>> 8 seconds. That is a pretty dramatic slowdown. Any idea why? I wonder
>> if that's related to the overall commit slowness.
>
>How big is your working directory? "git ls-files | wc -l" should show
>it. Try "git read-tree HEAD; git add; git write-tree" and see if the
>write-tree part takes as much time as commit. write-tree is mainly
>about cache-tree generation.
>
>> @Carlos and/or @Junio, can you point me at any docs/code to understand
>> what a tree-cache is and how it differs from the index? I did a google
>> search for [git tree-cache index], but nothing popped out.
>
>Have a look at Documentation/technical/index-format.txt. Cache tree
>extension is near the end.
>--
>Duy
^ permalink raw reply
* Re: Odd issue - The Diffs That WILL NOT DIE.
From: Chris Patti @ 2011-12-07 22:30 UTC (permalink / raw)
To: Jeff King; +Cc: Carlos Martín Nieto, git
In-Reply-To: <CAJ8P3RB=Gj-QCe6meqXSZ7N8+PnfNxSD8omUxT6dDh00bUf0QQ@mail.gmail.com>
Actually I'm wrong on that count, but in an interesting way.
Both CloudSponge.php and Cloudsponge.php exist and are *not* deleted
in the remote repository, but on OSX only Cloudsponge.php shows up on
the filesystem, yet CloudSponge.php is being reported as modified.
Turns out two of our other developers are also seeing this behavior.
-Chris
On Wed, Dec 7, 2011 at 5:24 PM, Chris Patti <cpatti@gmail.com> wrote:
> On Wed, Dec 7, 2011 at 5:03 PM, Jeff King <peff@peff.net> wrote:
>> On Wed, Dec 07, 2011 at 11:54:26AM -0500, Chris Patti wrote:
>>
>>> OK. Let me give you a very specific series of commands, sorry about
>>> the poor question / report (Not convinced it's a bug, probably pilot
>>> error?)
>>>
>>> If my understanding of the way Git works is correct, there should be
>>> NO pending diffs in a freshly cloned repository, yes?
>>
>> Yes. It's probably a bug, perhaps related to the case-insensitive
>> filesystem (we've seen similar weird "phantom changes right after clone"
>> bugs before).
>>
>>> 11:35][admin@Hiram-Abiff-2:~/src]$ rm -rf framework/
>>> [11:37][admin@Hiram-Abiff-2:~/src]$
>>> [11:44][admin@Hiram-Abiff-2:~/src]$ git clone
>>> ssh://git.bluestatedigital.com/home/git/framework.git
>>> Cloning into 'framework'...
>>> remote: Counting objects: 378540, done.
>>> remote: Compressing objects: 100% (100469/100469), done.
>>> remote: Total 378540 (delta 261046), reused 374685 (delta 258447)
>>> Receiving objects: 100% (378540/378540), 148.33 MiB | 2.08 MiB/s, done.
>>> Resolving deltas: 100% (261046/261046), done.
>>> [11:51][admin@Hiram-Abiff-2:~/src]$ cd framework/
>>> [11:51][admin@Hiram-Abiff-2:~/src/framework(master)]$ git diff
>>> diff --git a/app/modules/Core/controllers/CloudSponge.php b/app/modules/Core/con
>>> index 615a7b3..911d456 100644
>>> --- a/app/modules/Core/controllers/CloudSponge.php
>>> +++ b/app/modules/Core/controllers/CloudSponge.php
>>
>> Are there other files in the repository that differ from this path only
>> in capitalization? Can you show us the output of "git ls-files"?
>>
>> Is it possible to make this repo public, or at least available privately
>> to git developers?
>>
>> You mentioned v1.7.8. Do you see the bug with other git versions? If
>> not, can you try bisecting?
>>
>> -Peff
>
> Yup, you nailed it. The files in question are CloudSponge.php
> (deleted) and Cloudsponge.php (still being actively maintained).
>
> I am seeing the same behavior with 1.7.7.4 which I backrevved to
> yesterday while troubleshooting this issue. Can you suggest an older
> version for me to try next?
>
> I'm not sure how I would git bisect in this case, I'd need to have all
> the different git revs installed in order to do that right? (I'm
> relatively new to git bisect, just figured it out the other day).
>
> Thanks,
> -Chris
>
> --
> Christopher Patti - Geek At Large | GTalk: cpatti@gmail.com | AIM:
> chrisfeohpatti | P: (260) 54PATTI
> "Technology challenges art, art inspires technology." - John Lasseter, Pixar
--
Christopher Patti - Geek At Large | GTalk: cpatti@gmail.com | AIM:
chrisfeohpatti | P: (260) 54PATTI
"Technology challenges art, art inspires technology." - John Lasseter, Pixar
^ permalink raw reply
* Re: Odd issue - The Diffs That WILL NOT DIE.
From: Chris Patti @ 2011-12-07 22:24 UTC (permalink / raw)
To: Jeff King; +Cc: Carlos Martín Nieto, git
In-Reply-To: <20111207220345.GA21596@sigill.intra.peff.net>
On Wed, Dec 7, 2011 at 5:03 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Dec 07, 2011 at 11:54:26AM -0500, Chris Patti wrote:
>
>> OK. Let me give you a very specific series of commands, sorry about
>> the poor question / report (Not convinced it's a bug, probably pilot
>> error?)
>>
>> If my understanding of the way Git works is correct, there should be
>> NO pending diffs in a freshly cloned repository, yes?
>
> Yes. It's probably a bug, perhaps related to the case-insensitive
> filesystem (we've seen similar weird "phantom changes right after clone"
> bugs before).
>
>> 11:35][admin@Hiram-Abiff-2:~/src]$ rm -rf framework/
>> [11:37][admin@Hiram-Abiff-2:~/src]$
>> [11:44][admin@Hiram-Abiff-2:~/src]$ git clone
>> ssh://git.bluestatedigital.com/home/git/framework.git
>> Cloning into 'framework'...
>> remote: Counting objects: 378540, done.
>> remote: Compressing objects: 100% (100469/100469), done.
>> remote: Total 378540 (delta 261046), reused 374685 (delta 258447)
>> Receiving objects: 100% (378540/378540), 148.33 MiB | 2.08 MiB/s, done.
>> Resolving deltas: 100% (261046/261046), done.
>> [11:51][admin@Hiram-Abiff-2:~/src]$ cd framework/
>> [11:51][admin@Hiram-Abiff-2:~/src/framework(master)]$ git diff
>> diff --git a/app/modules/Core/controllers/CloudSponge.php b/app/modules/Core/con
>> index 615a7b3..911d456 100644
>> --- a/app/modules/Core/controllers/CloudSponge.php
>> +++ b/app/modules/Core/controllers/CloudSponge.php
>
> Are there other files in the repository that differ from this path only
> in capitalization? Can you show us the output of "git ls-files"?
>
> Is it possible to make this repo public, or at least available privately
> to git developers?
>
> You mentioned v1.7.8. Do you see the bug with other git versions? If
> not, can you try bisecting?
>
> -Peff
Yup, you nailed it. The files in question are CloudSponge.php
(deleted) and Cloudsponge.php (still being actively maintained).
I am seeing the same behavior with 1.7.7.4 which I backrevved to
yesterday while troubleshooting this issue. Can you suggest an older
version for me to try next?
I'm not sure how I would git bisect in this case, I'd need to have all
the different git revs installed in order to do that right? (I'm
relatively new to git bisect, just figured it out the other day).
Thanks,
-Chris
--
Christopher Patti - Geek At Large | GTalk: cpatti@gmail.com | AIM:
chrisfeohpatti | P: (260) 54PATTI
"Technology challenges art, art inspires technology." - John Lasseter, Pixar
^ permalink raw reply
* Re: Auto update submodules after merge and reset
From: Jens Lehmann @ 2011-12-07 22:23 UTC (permalink / raw)
To: Andreas T.Auer; +Cc: git
In-Reply-To: <jbnadt$hf8$1@dough.gmane.org>
Am 07.12.2011 10:07, schrieb Andreas T.Auer:
> Jens Lehmann wrote:
>
>> Am 30.11.2011 01:55, schrieb Max Krasnyansky:
>> I'm working on a patch series to teach Git to optionally update the
>> submodules work trees on checkout, reset merge and so on, but I'm not
>> there yet.
>>
>>> I'm thinking about adding a config option that would enable automatic
>>> submodule update but wanted to see if there is some fundamental reason
>>> why it would not be accepted.
> Because there is no good way to do so. It would be fine when you just track
> the submodules "read-only", but if you are actually working on submodules,
> it is a bad idea to always get a detached HEAD.
YMMV. We get along *really* well with this because all developers know that
if they want to hack on a submodule, they have to create a branch in there
first (and if they forget to do that, git status and friends will tell them).
What bugs us is that submodule HEADs don't follow what is checked out (or
merged, or reset ...) in the superproject. We had some really nasty
mismerges because of that, so we need the option to enable it.
> It is also a bad idea to
> merge or rebase on the currently checkedout branch.
As I'm no user of update=merge|rebase, I have no first hand experience on
that. But people do use those settings, no?
> Because if you are
> working on a maint branch in the submodule and then you checkout a pu branch
> in the superproject, because you have forgotten that maint branch in the
> submodule then all the proposed updates go to the maintenance branch -> bad.
Nope, checkout will fail and not do anything as it will detect changes in
the submodule to be updated by the checkout (just as it would do with a
regular file).
> So auto-update is not easy.
No, it is, and it works really well. But it might not fit your use case.
> But below I describe an idea that might solve
> these issues and help auto-udpate to work in a sane way.
>
>> I think adding something like an "submodule.autoupdate" config makes lots
>> of sense, but IMO it should affect all work tree updating porcelain
>> commands, not just merge.
>
> I was thinking about submodule integration and had the idea to bind a
> submodule to the superproject by having special references in the submodule
> like refs/super/master, refs/super/featureX... So these references are like
> tracking branches for the refs/heads/* of the superproject.
Having stuff in the submodule reference branches in the superproject
sounds upside down, as a superproject has (and should have) zero knowledge
about the superproject (as it could have many different of them).
> If you have tracking branches, the supermodule can just update the
> corresponding branch. If this branch is currently checkedout and the work
> area is clean, then the work area is updated, too. If there is currently a
> local branch or a diffent super-branch checked out then the working area
> should be considered "detached" from the superproject and not updated.
This sounds a lot like the "follow branch tip" model we discussed
recently (which could be configured via .gitmodules), but I'm not sure
you really are in the same boat here.
> With this concept you could even switch branches in the superproject and the
> attached submodules follow - still having no detached HEAD. When you want to
> do some local work on the submodule you checkout a local branch and merge
> back into the super branch later.
You lost me here. How can you merge a submodule branch into one of the
superproject?
> The head of that super branch might have
> changed by the update procedure meanwhile, but that is fine, then you just
> have a merge instead of a fast-forward.
>
> Another nice feature would be a recursive commit. So all changed index files
> in the _attached_ submodules would first be committed in their submodules
> and then the superproject commits too - all with one command. Currently it
> feels a little bit like CVS - commit one file(submodule), commit the other
> file(submodule) and then apply a label(commit the superproject) to keep the
> changes together.
>
> If the submodule is not attached the commit in the superproject can still
> detect changes that have been made to the corresponding tracking branch and
> pick these up.
>
> As a summary: Tracking submodule branches in the superproject instead of
> only the current HEAD of the submodule gives you more freedom to install
> sane auto-update procedures.
But we would want to have a deterministic update procedure, no? (And what
has more freedom than a detached HEAD? ;-)
> Even though it will raise a lot of detailed
> questions like "should the refs/super/* be pushed/pulled when syncing the
> submodule repositories".
I doubt that is a good idea, as that might conflict with the same submodule
sitting in a different superproject. But I'm interested to hear how you
want to solve that.
^ permalink raw reply
* Re: [PATCH 15/15] t3040 (subprojects-basic): modernize style
From: Jonathan Nieder @ 2011-12-07 22:21 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323286611-4806-16-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Put the opening quote starting each test on the same line as the
> test_expect_* invocation. While at it:
[...]
> - Use <<\-EOF in preference to <<EOF to save readers the trouble of
> looking for variable interpolations.
I think you mean <<-\EOF. :)
[...]
> - Chain commands with &&. Breaks in a test assertion's && chain can
> potentially hide failures from earlier commands in the chain.
>
> - Use test_expect_code() in preference to checking the exit status of
> various statements by hand.
I guess these two are the motivation?
> Inspired-by: Jonathan Nieder <jrnieder@gmail.com>
Oh, dear.
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
[...]
> --- a/t/t3040-subprojects-basic.sh
> +++ b/t/t3040-subprojects-basic.sh
> @@ -3,81 +3,81 @@
> test_description='Basic subproject functionality'
> . ./test-lib.sh
>
> -test_expect_success 'Super project creation' \
> - ': >Makefile &&
> - git add Makefile &&
> - git commit -m "Superproject created"'
> -
> -
> -cat >expected <<EOF
[...]
It would be easier to read if each preimage test assertion were next to
the corresponding postimage test assertion. Does "git diff --patience"
do better?
> -test_expect_success 'Super project creation' \
> - ': >Makefile &&
> - git add Makefile &&
> - git commit -m "Superproject created"'
> -
> -
> +test_expect_success 'setup: create superproject' '
> + : >Makefile &&
> + git add Makefile &&
> + git commit -m "Superproject created"
> +'
> +
Ok, makes sense.
> -cat >expected <<EOF
> -:000000 160000 00000... A sub1
> -:000000 160000 00000... A sub2
> -EOF
> -test_expect_success 'create subprojects' \
> - 'mkdir sub1 &&
> - ( cd sub1 && git init && : >Makefile && git add * &&
> - git commit -q -m "subproject 1" ) &&
> +test_expect_success 'setup: create subprojects' '
> + mkdir sub1 &&
> + ( cd sub1 && git init && : >Makefile && git add * &&
> + git commit -q -m "subproject 1" ) &&
If cleaning up the style anyway, I would write this as
mkdir sub1 &&
(
cd sub1 &&
git init &&
>Makefile &&
git add Makefile &&
git commit -m "subproject 1"
)
Or
mkdir sub1 &&
(
cd sub1 &&
git init &&
test_commit subproject-1 Makefile
)
But leaving it alone like you did is probably better. ;-)
[...]
> - git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
> - test_cmp expected current'
> -
> -git branch save HEAD
> -
> + git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
> + git branch save HEAD &&
> + cat >expected <<-\EOF &&
> + :000000 160000 00000... A sub1
> + :000000 160000 00000... A sub2
> + EOF
> + test_cmp expected current
> +'
> +
At first I wondered if "git branch save HEAD" is logically part of the
same test. After all, it's just meant as a baseline for use by later
tests.
After thinking about it for a few seconds, though, that's exactly what
this test is about. Maybe it would have been clearer if the two setup
tests were combined into one (but please don't take this advice too
seriously; I'm just musing).
> -test_expect_success 'check if fsck ignores the subprojects' \
> - 'git fsck --full'
> +test_expect_success 'check if fsck ignores the subprojects' '
> + git fsck --full
> +'
Does this test imply that one of the subprojects is broken somehow?
> -
> -test_expect_success 'check if commit in a subproject detected' \
> - '( cd sub1 &&
> - echo "all:" >>Makefile &&
> - echo " true" >>Makefile &&
> - git commit -q -a -m "make all" ) && {
> - git diff-files --exit-code
> - test $? = 1
> - }'
> +
> +test_expect_success 'check if commit in a subproject detected' '
> + ( cd sub1 &&
> + echo "all:" >>Makefile &&
> + echo " true" >>Makefile &&
> + git commit -q -a -m "make all" ) &&
> + test_expect_code 1 git diff-files --exit-code
> +'
Nice. Style again: I'd be tempted to reformat as
(
cd sub1 &&
echo "all:" >>Makefile &&
...
) &&
test_expect_code 1 git diff-files --exit-code
to make the subshell scope a little clearer, but exercising restraint
like you did may be better.
[...]
>
> # the index must contain the object name the HEAD of the
> # subproject sub1 was at the point "save"
> -test_expect_success 'checkout in superproject' \
> - 'git checkout save &&
> - git diff-index --exit-code --raw --cached save -- sub1'
> +test_expect_success 'checkout in superproject' '
> + git checkout save &&
> + git diff-index --exit-code --raw --cached save -- sub1
> +'
Thanks much. The result really is a little easier on the eyes, and
the changes look safe.
^ permalink raw reply
* Re: git auto-repack is broken...
From: Nicolas Pitre @ 2011-12-07 22:12 UTC (permalink / raw)
To: Brandon Casey
Cc: Jeff King, Junio C Hamano, Linus Torvalds,
Ævar Arnfjörð, Git Mailing List
In-Reply-To: <CA+sFfMdeVoz8XU5j4hNn1qCHHzaiDi0Bw=QbbuU3cwT9mMPZOA@mail.gmail.com>
On Sat, 3 Dec 2011, Brandon Casey wrote:
> Linus's scenario of fetching a lot of stuff that never actually makes
> it into the reflogs is still a valid problem. I'm not sure that
> people who don't know what they are doing are going to run into this
> problem though. Since he fetches a lot of stuff without ever checking
> it out or creating a branch from it, potentially many objects become
> unreferenced every time FETCH_HEAD changes.
Maybe FETCH_HEAD should have a reflog too?
Nicolas
^ permalink raw reply
* Re: Yo dawg, I heard you like trees...
From: Jeff King @ 2011-12-07 22:12 UTC (permalink / raw)
To: Sebastian Morr; +Cc: Phil Hord, git
In-Reply-To: <20111207155411.GB2003@thinkpad>
On Wed, Dec 07, 2011 at 04:54:11PM +0100, Sebastian Morr wrote:
> > Git uses a DAG. The A stands for "acyclic". Loops are not allowed.
>
> I'm aware of that. It's acyclic by design, but is this actually enforced
> by the code? Or does it simply trust that no loops will ever occur,
> because it's so improbable?
The latter. And not just "improbable", but "so improbable that trying to
do it on purpose should still take billions of years".
Assuming sha1 isn't totally broken, of course.
> After Andrew's response I investigated a bit, and it seems I
> overvalued the attempts to "break" SHA-1. Wikipedia quotes a 2008
> attack, that can create a collision with 2^51 hash function calls.
According to wikipedia, it _may_ produce collisions in 2^51 to 2^57.
Worrisome numbers, certainly, but 2^51 is probably within our ability to
compute if a big project is undertaken. Yet to my knowledge nobody has
actually created such a collision. So the attack is still theoretical at
this point, and there's no good way to create a loop within the git DAG
(or within a tree).
-Peff
^ permalink raw reply
* Re: Odd issue - The Diffs That WILL NOT DIE.
From: Jeff King @ 2011-12-07 22:03 UTC (permalink / raw)
To: Chris Patti; +Cc: Carlos Martín Nieto, git
In-Reply-To: <CAJ8P3RCPt9Kwi1F7_TEkZQhkm1mwR_TFKhYszS5LL50kXU8oNQ@mail.gmail.com>
On Wed, Dec 07, 2011 at 11:54:26AM -0500, Chris Patti wrote:
> OK. Let me give you a very specific series of commands, sorry about
> the poor question / report (Not convinced it's a bug, probably pilot
> error?)
>
> If my understanding of the way Git works is correct, there should be
> NO pending diffs in a freshly cloned repository, yes?
Yes. It's probably a bug, perhaps related to the case-insensitive
filesystem (we've seen similar weird "phantom changes right after clone"
bugs before).
> 11:35][admin@Hiram-Abiff-2:~/src]$ rm -rf framework/
> [11:37][admin@Hiram-Abiff-2:~/src]$
> [11:44][admin@Hiram-Abiff-2:~/src]$ git clone
> ssh://git.bluestatedigital.com/home/git/framework.git
> Cloning into 'framework'...
> remote: Counting objects: 378540, done.
> remote: Compressing objects: 100% (100469/100469), done.
> remote: Total 378540 (delta 261046), reused 374685 (delta 258447)
> Receiving objects: 100% (378540/378540), 148.33 MiB | 2.08 MiB/s, done.
> Resolving deltas: 100% (261046/261046), done.
> [11:51][admin@Hiram-Abiff-2:~/src]$ cd framework/
> [11:51][admin@Hiram-Abiff-2:~/src/framework(master)]$ git diff
> diff --git a/app/modules/Core/controllers/CloudSponge.php b/app/modules/Core/con
> index 615a7b3..911d456 100644
> --- a/app/modules/Core/controllers/CloudSponge.php
> +++ b/app/modules/Core/controllers/CloudSponge.php
Are there other files in the repository that differ from this path only
in capitalization? Can you show us the output of "git ls-files"?
Is it possible to make this repo public, or at least available privately
to git developers?
You mentioned v1.7.8. Do you see the bug with other git versions? If
not, can you try bisecting?
-Peff
^ permalink raw reply
* Re: [PATCH 14/15] t1006 (cat-file): use test_cmp
From: Jonathan Nieder @ 2011-12-07 22:01 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323286611-4806-15-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Use test_cmp in preference to repeatedly comparing command outputs by
> hand.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
> t/t1006-cat-file.sh | 53 +++++++++++---------------------------------------
> 1 files changed, 12 insertions(+), 41 deletions(-)
I guess test_cmp didn't exist yet when this code was written, though
that doesn't explain why "diff" or "cmp" was not used.
Doesn't this miss some other instances of the
test $expect = $actual
idiom in the same test script? What distinguishes the cases that you
fixed from the ones skipped, and is it worth an inconsistent style to
convert some without the others?
^ permalink raw reply
* Re: [PATCH 13/15] t3030 (merge-recursive): use test_expect_code
From: Jonathan Nieder @ 2011-12-07 21:57 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323286611-4806-14-git-send-email-artagnon@gmail.com>
Patches 9-12 look good.
Ramkumar Ramachandra wrote:
> Use test_expect_code in preference to repeatedly checking exit codes
> by hand.
[...]
> +++ b/t/t3030-merge-recursive.sh
> @@ -285,17 +285,7 @@ test_expect_success 'merge-recursive simple' '
> rm -fr [abcd] &&
> git checkout -f "$c2" &&
>
> - git merge-recursive "$c0" -- "$c2" "$c1"
> - status=$?
> - case "$status" in
> - 1)
> - : happy
> - ;;
> - *)
> - echo >&2 "why status $status!!!"
> - false
> - ;;
> - esac
> + test_expect_code 1 git merge-recursive "$c0" -- "$c2" "$c1"
Yeah, this style is kind of repetitive. Worse, if "git checkout"
fails with status 1, it would yield a false success. Thanks for
fixing it.
^ permalink raw reply
* Re: Git Submodule Problem - Bug?
From: Jens Lehmann @ 2011-12-07 21:56 UTC (permalink / raw)
To: Manuel Koller; +Cc: Heiko Voigt, Fredrik Gustafsson, Thomas Rast, git
In-Reply-To: <CAPUobv1QnuAT76=yGDM-KKjoiXCzMt0jCda0LdYxAjN49qmAgA@mail.gmail.com>
Am 07.12.2011 09:21, schrieb Manuel Koller:
>> How about this:
>>
>> The user issues 'git submodule add foo' and we discover that there is
>> already a local clone under the name foo. Git then asks something like
>> this
>>
>> Error when adding: There is already a local submodule under the
>> name 'foo'.
>>
>> You can either rename the submodule to be added to a different
>> name or manually remove the local clone underneath
>> .git/modules/foo. If you want to remove the local clone please
>> quit now.
>>
>> We strongly suggest that you give each submodule a unique name.
>> Note: This name is independent from the path it is bound to.
>>
>> What do you want me to do ([r]ename it, [Q]uit) ?
>>
>> When the user chooses 'rename' git will prompt for a new name.
>>
>> If we are going to support the remove use case with add we additionally
>> need some logic to deal with it during update (which is not supported
>> yet AFAIK). But we probably need this support anyway since between
>> removal and adding a new submodule under the same can be a long time.
>> If users switch between such ancient history and the new history we
>> would have the same conflict.
>>is_submodule_modified()
>> We could of course just error out and tell the user that he has to give
>> the submodule an uniqe name. If the user does not do so leave it to him
>> to deal with the situation manually.
>>
>> What do you think?
>>
>> Cheers Heiko
>
> Prompt to choose another name would be fine I guess - but it solves
> the problem only if the submodule has been initialized already. There
> could be a submodule of the same name in another branch, which I
> haven't checked out yet, for example. The user would have to be forced
> choose a unique name for every submodule.
Which seems pretty much impossible in a distributed system ...
> Anyway, it seems impossible to handle a name clash automatically,
> since there are good reasons to have different urls for the same
> submodule.> Having read the thread linked by Junio, the only way out
> seems to be a kind of url rewrite scheme and using the url as name.
> Doesn't it solve all the problems?
>
> - the url is more or less unique (there are problems now if we have to
> different submodules at the same path, which is much more likely to
> happen than a different repository at the same url some time in the
> future)
> - after a change of the submodule's url, we can still check out old
> commits in a comfortable way
> - we could have the same submodule at different paths, but downloaded only once
> - the user is not forced to do anything, but the .gitmodule config can
> still be overruled if necessary
Hmm, using the URL has the downside that when one URL is just a fork of
the other we might have most of the repo duplicated in the .git/modules
directory ... but if it solves the problem of having a totally different
submodule cloned into the same path it might be worth it.
^ permalink raw reply
* Re: [PATCH 08/15] t3200 (branch): fix && chaining
From: Jonathan Nieder @ 2011-12-07 21:55 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323286611-4806-9-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -22,7 +22,7 @@ test_expect_success \
>
> test_expect_success \
> 'git branch --help should not have created a bogus branch' '
> - git branch --help </dev/null >/dev/null 2>/dev/null;
> + git branch --help </dev/null >/dev/null 2>/dev/null &&
> test_path_is_missing .git/refs/heads/--help
Won't this break when running tests for the first time, before the git
manpages are installed?
> '
>
> @@ -88,7 +88,7 @@ test_expect_success \
> test_expect_success \
> 'git branch -m n/n n should work' \
> 'git branch -l n/n &&
> - git branch -m n/n n
> + git branch -m n/n n &&
> test_path_is_file .git/logs/refs/heads/n'
Good catch.
^ permalink raw reply
* Re: [PATCH 07/15] t1510 (worktree): fix && chaining
From: Jonathan Nieder @ 2011-12-07 21:51 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323286611-4806-8-git-send-email-artagnon@gmail.com>
Patches 5 and 6 look safe, though I haven't tested them.
Ramkumar Ramachandra wrote:
> --- a/t/t1501-worktree.sh
> +++ b/t/t1501-worktree.sh
> @@ -48,7 +48,7 @@ test_expect_success 'setup: helper for testing rev-parse' '
> '
>
> test_expect_success 'setup: core.worktree = relative path' '
> - unset GIT_WORK_TREE;
> + unset GIT_WORK_TREE &&
On some shells, like /usr/xpg4/bin/sh on Solaris, unset returns nonzero
status when the variable passed was already unset. Will this work on
such platforms, or does it need to be changed to use sane_unset?
^ permalink raw reply
* [PATCH] diff/status: print submodule path when looking for changes fails
From: Jens Lehmann @ 2011-12-07 21:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Seth Robertson, git
In-Reply-To: <201112061930.pB6JUuDx004171@no.baka.org>
diff and status run "git status --porcelain" inside each populated
submodule to see if it contains changes (unless told not to do so via
config or command line option). When that fails, e.g. due to a corrupt
submodule .git directory, it just prints "git status --porcelain failed"
or "Could not run git status --porcelain" without giving the user a clue
where that happened.
Add '"in submodule %s", path' to these error strings to tell the user
where exactly the problem occurred.
Reported-by: Seth Robertson <in-gitvger@baka.org>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
Am 06.12.2011 20:30, schrieb Seth Robertson:
> Someone on #git just encountered a problem where `git init && git add . &&
> git status` was failing with a message about a corrupted index.
>
> error: bad index file sha1 signature
> fatal: index file corrupt
> fatal: git status --porcelain failed
>
> This confused everyone for a while, until he provided access to the
> directory to play with. I eventually tracked it down to a directory
> in the tree which already had a .git directory in it. Unfortunately,
> that .git repo was corrupted and was the one returning the message
> about a corrupted index. The problem is that the error message we
> were seeing did not provide any direct hints that submodules were
> involved or that the problem was not at the top level (`git status
> --porcelain` is admittedly an indirect hint to both). Here is a
> recipe to reproduce a similar problem:
>
> (mkdir -p z/foo; cd z/foo; git init; echo A>A; git add A; git commit -m A; cd ..; echo B>B; rm -f foo/.git/objects/*/*; git init; git add .; git status)
Thanks for the report and the recipe to reproduce it.
> Providing an expanded error message which clarifies that this is
> failing in a submodule directory makes everything clear.
>
> ----------------------------------------------------------------------
> --- submodule.c~ 2011-12-02 14:25:08.000000000 -0500
> +++ submodule.c 2011-12-06 14:13:00.554413432 -0500
> @@ -714,7 +714,7 @@
> close(cp.out);
>
> if (finish_command(&cp))
> - die("git status --porcelain failed");
> + die("git status --porcelain failed in submodule directory %s", path);
>
> strbuf_release(&buf);
> return dirty_submodule;
> ----------------------------------------------------------------------
Makes lots of sense.
> Do more error messages in submodule.c need adjusting? It seems likely.
It looks like only the die() after the start_command() in the same
is_submodule_modified() function would also need to print the path.
The only other place that dies after starting a command inside a
submodule is in submodule_needs_pushing(), and it already says:
die("Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s",
...
So let's do the same in is_submodule_modified().
submodule.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/submodule.c b/submodule.c
index 52cdcc6..68c1ba9 100644
--- a/submodule.c
+++ b/submodule.c
@@ -689,7 +689,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
cp.out = -1;
cp.dir = path;
if (start_command(&cp))
- die("Could not run git status --porcelain");
+ die("Could not run 'git status --porcelain' in submodule %s", path);
len = strbuf_read(&buf, cp.out, 1024);
line = buf.buf;
@@ -714,7 +714,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
close(cp.out);
if (finish_command(&cp))
- die("git status --porcelain failed");
+ die("'git status --porcelain' failed in submodule %s", path);
strbuf_release(&buf);
return dirty_submodule;
--
1.7.8.111.gd3732
^ permalink raw reply related
* Re: [PATCH 04/15] t1007 (hash-object): fix && chaining
From: Jonathan Nieder @ 2011-12-07 21:47 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323286611-4806-5-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
I think there's room for at least one line of description about why
one would want to do this.
IMHO if the patches are only being sent to me, Junio, and the
mailing list (and not cc-ed to different people), then there's no
reason to split them up when they have the same topic.
Aside from that, patches 1-3 look good. Now for this one:
[...]
> --- a/t/t1007-hash-object.sh
> +++ b/t/t1007-hash-object.sh
> @@ -154,13 +154,13 @@ test_expect_success 'check that --no-filters option works with --stdin-paths' '
> pop_repo
>
> for args in "-w --stdin" "--stdin -w"; do
> - push_repo
> + push_repo &&
>
> test_expect_success "hash from stdin and write to database ($args)" '
> test $example_sha1 = $(git hash-object $args < example)
> - '
> + ' &&
I don't see how this would have any effect. Is it intended?
^ permalink raw reply
* Re: [PATCH] userdiff: allow * between cpp funcname words
From: Johannes Sixt @ 2011-12-07 21:13 UTC (permalink / raw)
To: Thomas Rast; +Cc: Junio C Hamano, Jeff King, git
In-Reply-To: <201112070904.28212.trast@student.ethz.ch>
Am 07.12.2011 09:04, schrieb Thomas Rast:
> Junio C Hamano wrote:
>> Thomas Rast <trast@student.ethz.ch> writes:
>>
>>> Actually (sadly) I'll have to revise it. It doesn't match much of C++
>>> either, and I haven't yet come up with a reasonable regex that
>>> matches, say,
>>>
>>> foo::Bar<int>::t& Baz::operator<<(
>>>
>>> which I would call ludicrous, but it's valid C++.
>>
>> Heh, I'd rather not see us go that route, which would either end up
>> implementing a C++ parser or reverting the heuristics back to "non-blank
>> at the beginning of the line" that was already reasonably useful.
>
> Well, there are many things that we deliberately do not match right
> now and for which that's a good thing:
>
> label:
> public:
> void declaration_only(...);
> int global_variable;
>
> At some point I was wondering whether it would be better to just
> declare a non-match for '.*;' and '^[A-Za-z_][A-Za-z_0-9]+:', and
> otherwise match all '^[A-Za-z].*\(' but I may be missing something.
The current cpp pattern doesn't work that well with C++. Since it
requires a blank before a name before the opening parentheses, it
doesn't catch constructors:
Foo::Foo()
and it should fail for GNU style C function definitions as well (I
didn't test):
void
do_the_foo()
I'll run this pattern for a while:
diff.cpp.xfuncname=!^[a-zA-Z_][a-zA-Z_0-9]*[[:space:]]*:[[:space:]]*$
^[a-zA-Z_].*
BTW, your match pattern requires an opening parenthesis; it would not
catch class definitions.
-- Hannes
^ permalink raw reply
* Re: [PATCH 4/2] grep: turn off threading for non-worktree
From: Jeff King @ 2011-12-07 20:45 UTC (permalink / raw)
To: J. Bruce Fields
Cc: René Scharfe, Thomas Rast, git, Eric Herman, Junio C Hamano
In-Reply-To: <20111207201105.GA22995@fieldses.org>
On Wed, Dec 07, 2011 at 03:11:05PM -0500, J. Bruce Fields wrote:
> > $ time git grep --threads=8 'a.*b' HEAD >/dev/null
> > real 0m8.655s
> > user 0m23.817s
> > sys 0m0.480s
>
> Dumb question (I missed the beginning of the conversation): what kind of
> storage are you using, and is the data already cached?
Sorry, I should have been clear: all of those numbers are with a warm
cache. So this is measuring only CPU.
> I seem to recall part of the motivation for the multithreading being
> NFS, where the goal isn't so much to keep CPU's busy as it is to keep
> the network busy.
>
> Probably a bigger problem for something like "git status" which I think
> ends up doing a series of stat's (which can each require a round trip to
> the server in the NFS case), as it is a problem for something like
> git-grep that's also doing reads.
>
> Just a plea for considering the IO cost as well when making these kinds
> of decisions....
This system has a decent-quality SSD, so the I/O timings are perhaps
not as interesting as they might otherwise be. But here are cold cache
numbers (each run after 'echo 3 >/proc/sys/vm/drop_caches'):
HEAD, --threads=0: 4.956s
HEAD, --threads=8: 9.917s
working tree, --threads=0: 17.444s
working tree, --threads=8: 6.462s
So when pulling from the object db, threads are still a huge loss
(because the data is compressed, the SSD is fast, and we spend a lot of
CPU time inflating; so it ends up close to the warm cache results). But
for the working tree, the I/O parallelism is a huge win.
So at least on my system, cold cache vs. warm cache leads to the same
conclusion. "git grep --threads=8 ... HEAD" might still be a win on slow
disks or NFS, though.
-Peff
^ permalink raw reply
* Re: [PATCH 4/2] grep: turn off threading for non-worktree
From: J. Bruce Fields @ 2011-12-07 20:11 UTC (permalink / raw)
To: Jeff King
Cc: René Scharfe, Thomas Rast, git, Eric Herman, Junio C Hamano
In-Reply-To: <20111207044242.GB10765@sigill.intra.peff.net>
On Tue, Dec 06, 2011 at 11:42:42PM -0500, Jeff King wrote:
> On Wed, Dec 07, 2011 at 12:01:37AM +0100, René Scharfe wrote:
>
> > Reading of git objects needs to be protected by an exclusive lock
> > and cannot be parallelized. Searching the read buffers can be done
> > in parallel, but for simple expressions threading is a net loss due
> > to its overhead, as measured by Thomas. Turn it off unless we're
> > searching in the worktree.
>
> Based on my earlier numbers, I was going to complain that we should
> also be checking the "simple expressions" assumption here, as time spent
> in the actual regex might be important.
>
> However, after trying to repeat my experiment, I think the numbers I
> posted earlier were misleading. For example, using my "more complex"
> regex of 'a.*b':
>
> $ time git grep --threads=8 'a.*b' HEAD >/dev/null
> real 0m8.655s
> user 0m23.817s
> sys 0m0.480s
Dumb question (I missed the beginning of the conversation): what kind of
storage are you using, and is the data already cached?
I seem to recall part of the motivation for the multithreading being
NFS, where the goal isn't so much to keep CPU's busy as it is to keep
the network busy.
Probably a bigger problem for something like "git status" which I think
ends up doing a series of stat's (which can each require a round trip to
the server in the NFS case), as it is a problem for something like
git-grep that's also doing reads.
Just a plea for considering the IO cost as well when making these kinds
of decisions....
(Which maybe you already do, apologies again for just naively dropping
into the middle of a thread.)
--b.
>
> Look at that sweet, sweet parallelism. It's a quad-core with
> hyperthreading, so we're not getting the 8x speedup we might hope for
> (presumably due to lock contention on extracting blobs), but hey, 3x
> isn't bad. Except, wait:
>
> $ time git grep --threads=0 'a.*b' HEAD >/dev/null
> real 0m7.651s
> user 0m7.600s
> sys 0m0.048s
>
> We can get 1x on a single core, but the total time is lower! This
> processor is an i7 with "turbo boost", which means it clocks higher in
> single-core mode than when multiple cores are active. So the numbers I
> posted earlier were misleading. Yes, we got parallelism, but at the cost
> of knocking the clock speed down for a net loss.
>
> The sweet spot for me seems to be:
>
> $ time git grep --threads=2 'a.*b' HEAD >/dev/null
> real 0m6.303s
> user 0m11.129s
> sys 0m0.220s
>
> I'd be curious to see results from somebody with a quad-core (or more)
> without turbo boost; I suspect that threading may have more benefit
> there, even though we have some lock contention for blobs.
>
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -1048,7 +1048,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> > nr_threads = 0;
> > #else
> > if (nr_threads == -1)
> > - nr_threads = (online_cpus() > 1) ? THREADS : 0;
> > + nr_threads = (online_cpus() > 1 && !list.nr) ? THREADS : 0;
> >
> > if (nr_threads > 0) {
> > opt.use_threads = 1;
>
> This doesn't kick in for "--cached", which has the same performance
> characteristics as grepping a tree. I think you want to add "&& !cached" to
> the conditional.
>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
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