* Re: People unaware of the importance of "git gc"?
From: Shawn O. Pearce @ 2007-09-06 2:52 UTC (permalink / raw)
To: Russ Dill; +Cc: git, Junio C Hamano, Nicolas Pitre
In-Reply-To: <loom.20070906T044017-727@post.gmane.org>
Russ Dill <Russ.Dill@gmail.com> wrote:
> > Ok, how about doing something like this?
>
> git add? merge? rebase? No, I have a sneakier place to invoke gc.
>
> Whenever $EDITOR gets invoked. Heck, whenever git is waiting for any user input,
> do some gc in the background, it'd just have to be incremental so that we could
> pick up where we left off.
Heh. That is a really good idea. I've been thinking about doing
some automatic generational style GC type repacking controls in
git-gui, and doing them when git-gui is sitting idle and has not
been used in the past couple of minutes.
This is along the same vein of thought. I like it. Often it
takes me a while to come up with a good commit message even if
I am using command line commit.
But git-rebase/git-am can cause a huge number of objects to be
created, especially if you are pushing a large stack of patches
around. So it may still be a good idea to trigger `gc --auto`
at the end of those operations.
> Similarly, you could mix it in with git pull/push so that while we are waiting
> on the network, we can do some packing.
Here's a better thought:
If we are pushing somewhere, and the push size is "large-ish" and
we aren't pushing a thin pack (its currently considered not nice
to the remote end so it doesn't happen by default) and the objects
we are packing are mostly all loose maybe we should also save a
copy of that packfile locally, then prune *only* those loose objects
back.
Not every git user pushes their work. But many do. And those
that push usually will do so in bursts, are already expecting to
wait for the network latency, and usually are pushing the majority
of the things that are loose. Such users will probably never see
the `gc --auto` trip in places like commit/am/merge as they would
already be clearing their ODB with the push.
--
Shawn.
^ permalink raw reply
* Re: People unaware of the importance of "git gc"?
From: Shawn O. Pearce @ 2007-09-06 2:56 UTC (permalink / raw)
To: Steven Grimm
Cc: Junio C Hamano, Nicolas Pitre, Nix, Linus Torvalds,
Git Mailing List
In-Reply-To: <46DF6AA8.60804@midwinter.com>
Steven Grimm <koreth@midwinter.com> wrote:
> Shawn O. Pearce wrote:
> >But this suffers from the same fate if the user sets gc.auto too
> >small and doesn't realize that the reason Git is always repacking
> >is because over the last 6 months they have been unlucky enough to
> >stage the magic number of unreachable blobs into the 17 directory
> >and they have *never* run `git gc --prune` because the auto thing
> >is working just fine for them and they don't realize they need to
> >prune every once in a blue moon.
>
> Check the modification times on those files and don't count ones that
> are older than the last git-gc run, maybe? That'd take care of the problem.
Eh, that could mean a bunch of stat calls that it would be nice
to avoid. The counter Junio (and git-gui) implements just does
a readdir(). Reasonably cheap.
Maybe just save a ".git/gc_last_auto" with the last object count
of .git/objects/17, after repacking. If the count is over the
gc.auto limit *and* is still over the limit after subtracting the
".git/gc_last_auto" value then consider that auto is required.
This way the file is only consulted if we are really thinking
about running a repack, and its only written to if we actually do
the repack. So we only take the extra penalty if we are going to
be taking a *really* big extra penalty by repacking.
--
Shawn.
^ permalink raw reply
* Re: Calculating tree nodes
From: Shawn O. Pearce @ 2007-09-06 3:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jon Smirl, Git Mailing List
In-Reply-To: <7vbqcinxdb.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
> > There's nothing stopping us from creating additional indexes.
> > ...
> > But we can also store the notes alongside the commits in the
> > packfile, so that if the data for the commit has been paged in
...
>
> I would agree with your main thrust "nobody prevents you from
> building additional index", but on a tangent, I am skeptical
> about adding too much to pack v4. Especially "clustering the
> notes" part.
...
> Now, hopefully many operations do not need notes either,
> although notes themselves can store _anything_ so each of them
> could be large and/or each commit could have large number of
> them. I suspect clustering notes along with the commit they
> annotate would break the locality of access for common case.
I'm inclined to agree.
Its something I've thought about doing. I haven't even prototyped
code for it. Let alone shown numbers that say one way or the other.
One of the notes proposals was talking about having lots of different
classes of notes. E.g. a "Signed-off-by" class and a "build and test
log results" class.
The former would generally be very small and may even want to be
shown most of the time the commit body is displayed (e.g. in gitk,
git-log). These would be good candidates to cluster alongside the
commit. Indeed they are clustered there today, just hung inside
of the commit object itself. Nobody is bitching about the hit they
cause on the common case of `pack-objects`. :)
The latter (build and test log) would generally be very large.
We would *not* want to cluster them. But we might want to store
next to the commit a very small pointer to the note itself. Such
as the note's SHA-1. Or its offset within the packfile's index.
This would make locating those notes very cheap, while not having
a huge impact on the common case of commit traversal.
Likewise we might want to pack a tag's SHA-1 alongside of the commit
it points at, as parsing the commit would immediately give us all
annotated tags that refer to that commit. Tags are (usually) few
and far between. But tools like git-describe are commonly used and
would benefit from not needing to build the commit->tag hashtable.
OK, well, git-describe cheats and uses the struct object hashtable,
but whatever.
You get my point. I think. And I got yours about not making the
common case worse than it already is today.
--
Shawn.
^ permalink raw reply
* [PATCH] Don't allow contrib/workdir/git-new-workdir to trash existing dirs
From: Shawn O. Pearce @ 2007-09-06 3:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Recently I found that doing a sequence like the following:
git-new-workdir a b
...
git-new-workdir a b
by accident will cause a (and now also b) to have an infinite cycle
in its refs directory. This is caused by git-new-workdir trying
to create the "refs" symlink over again, only during the second
time it is being created within a's refs directory and is now also
pointing back at a's refs.
This causes confusion in git as suddenly branches are named things
like "refs/refs/refs/refs/refs/refs/refs/heads/foo" instead of the
more commonly accepted "refs/heads/foo". Plenty of commands start
to see ambiguous ref names and others just take ages to compute.
git-clone has the same safety check, so git-new-workdir should
behave just like it.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
contrib/workdir/git-new-workdir | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir
index c6e154a..2838546 100755
--- a/contrib/workdir/git-new-workdir
+++ b/contrib/workdir/git-new-workdir
@@ -48,6 +48,12 @@ then
"a complete repository."
fi
+# don't recreate a workdir over an existing repository
+if test -e "$new_workdir"
+then
+ die "destination directory '$new_workdir' already exists."
+fi
+
# make sure the the links use full paths
git_dir=$(cd "$git_dir"; pwd)
--
1.5.3.1.840.g0fedbc
^ permalink raw reply related
* [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags
From: Shawn O. Pearce @ 2007-09-06 4:21 UTC (permalink / raw)
To: Junio C Hamano, Carlos Rica; +Cc: git
If the user has misconfigured `user.signingkey` in their .git/config
or just doesn't have any secret keys on their keyring and they ask
for a signed tag with `git tag -s` we better make sure the resulting
tag was actually signed by gpg.
Prior versions of builtin git-tag allowed this failure to slip
by without error as they were not checking the return value of
the finish_command() so they did not notice when gpg exited with
an error exit status. They also did not fail if gpg produced an
empty output or if read_in_full received an error from the read
system call while trying to read the pipe back from gpg.
Finally we did not actually honor any return value from the do_sign
function as it returns ssize_t but was being stored into an unsigned
long. This caused the compiler to optimize out the die condition,
allowing git-tag to continue along and create the tag object.
With these issues fixed `git-tag -s` will now fail to create the
tag and will report a non-zero exit status to its caller, thereby
allowing automated helper scripts to detect (and recover from)
failure if gpg is not working properly.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
I think this and my prior contrib/workdir patch should both go into
maint. This one in particular; it hurt us today when an automated
tool that runs `git tag -s` didn't notice the GnuPG problems.
builtin-tag.c | 8 +++++---
t/t7004-tag.sh | 7 +++++++
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/builtin-tag.c b/builtin-tag.c
index 348919c..aadf850 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -217,7 +217,8 @@ static ssize_t do_sign(char *buffer, size_t size, size_t max)
gpg.close_in = 0;
len = read_in_full(gpg.out, buffer + size, max - size);
- finish_command(&gpg);
+ if (finish_command(&gpg) || !len || len < 0)
+ return error("gpg failed to sign the tag");
if (len == max - size)
return error("could not read the entire signature from gpg.");
@@ -310,9 +311,10 @@ static void create_tag(const unsigned char *object, const char *tag,
size += header_len;
if (sign) {
- size = do_sign(buffer, size, max_size);
- if (size < 0)
+ ssize_t r = do_sign(buffer, size, max_size);
+ if (r < 0)
die("unable to sign the tag");
+ size = r;
}
if (write_sha1_file(buffer, size, tag_type, result) < 0)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 606d4f2..0d07bc3 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -990,6 +990,13 @@ test_expect_success \
git diff expect actual
'
+# try to sign with bad user.signingkey
+git config user.signingkey BobTheMouse
+test_expect_failure \
+ 'git-tag -s fails if gpg is misconfigured' \
+ 'git tag -s -m tail tag-gpg-failure'
+git config --unset user.signingkey
+
# try to verify without gpg:
rm -rf gpghome
--
1.5.3.1.840.g0fedbc
^ permalink raw reply related
* Re: [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags
From: Shawn O. Pearce @ 2007-09-06 4:26 UTC (permalink / raw)
To: Junio C Hamano, Carlos Rica; +Cc: git
In-Reply-To: <20070906042115.GA343@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> wrote:
> If the user has misconfigured `user.signingkey` in their .git/config
> or just doesn't have any secret keys on their keyring and they ask
> for a signed tag with `git tag -s` we better make sure the resulting
> tag was actually signed by gpg.
...
> I think this and my prior contrib/workdir patch should both go into
> maint. This one in particular; it hurt us today when an automated
> tool that runs `git tag -s` didn't notice the GnuPG problems.
I'm sorry, despite the subject of this email this is actually a
*one* patch series. The 2/2 is because I applied and tested this
on top of the contrib/workdir patch I was talking about above and
my email sending script thought it was a two patch series.
--
Shawn.
^ permalink raw reply
* [PATCH] Include a git-push example for creating a remote branch
From: Shawn O. Pearce @ 2007-09-06 4:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Many users get confused when `git push origin master:foo` works
when foo already exists on the remote repository but are confused
when foo doesn't exist as a branch and this form does not create
the branch foo.
This new example highlights the trick of including refs/heads/
in front of the desired branch name to create a branch.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
Documentation/git-push.txt | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 0dd9caf..7b8e075 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -117,6 +117,12 @@ git push origin master:satellite/master::
the ref that matches `satellite/master` (most likely, it would
be `refs/remotes/satellite/master`) in `origin` repository with it.
+git push origin master:refs/heads/experimental::
+ Create the branch `experimental` in the `origin` repository
+ by copying the current `master` branch. This form is usually
+ needed to create a new branch in the remote repository as
+ there is no `experimental` branch to match.
+
Author
------
Written by Junio C Hamano <junkio@cox.net>, later rewritten in C
--
1.5.3.1.840.g0fedbc
^ permalink raw reply related
* Re: [RFC] Convert builin-mailinfo.c to use The Better String Library.
From: Dmitry Kakurin @ 2007-09-06 4:48 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git
In-Reply-To: <vpq642pkoln.fsf@bauges.imag.fr>
[ snip ]
When I first looked at Git source code two things struck me as odd:
1. Pure C as opposed to C++. No idea why. Please don't talk about
portability, it's BS.
2. Brute-force, direct string manipulation. It's both verbose and
error-prone. This makes it hard to follow high-level code logic.
- Dmitry
^ permalink raw reply
* Re: [PATCH] Include a git-push example for creating a remote branch
From: Miles Bader @ 2007-09-06 4:54 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20070906044408.GA588@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> Many users get confused when `git push origin master:foo` works
> when foo already exists on the remote repository but are confused
> when foo doesn't exist as a branch and this form does not create
> the branch foo.
Hmm, what _does_ it do in that case...?
-Miles
--
If you can't beat them, arrange to have them beaten. [George Carlin]
^ permalink raw reply
* Re: [RFC] Convert builin-mailinfo.c to use The Better String Library.
From: Shawn O. Pearce @ 2007-09-06 4:59 UTC (permalink / raw)
To: Dmitry Kakurin; +Cc: Matthieu Moy, Git
In-Reply-To: <4AFD7EAD1AAC4E54A416BA3F6E6A9E52@ntdev.corp.microsoft.com>
Dmitry Kakurin <dmitry.kakurin@gmail.com> wrote:
> When I first looked at Git source code two things struck me as odd:
> 1. Pure C as opposed to C++. No idea why. Please don't talk about
> portability, it's BS.
Git's creator (Linus) codes in C, not C++. He has at various times
stated reasons why he does not use C++. I'm sure one can find such
messages with a bit of searching on mailing lists that he frequents.
He has his reasons. I also happen to agree with at least some
of them. :)
Git evolved from that initial prototype that Linus created. I'm not
sure how much code survives from that initial few versions that
Linus managed before Junio took over, but nobody wanted to rewrite
things that already work so it just stayed in C.
"If it works, don't fix it."
C works. We (now) have 83,215 lines of it. Its not going away
anytime soon in Git. It is also a relatively simple language that
a large number of open source programmers know. This makes it easy
for them to get involved in the project. Instead of say Haskell,
which has a smaller community. Or Tcl/Tk as we recently found out
in the Git User Survey. :-\
--
Shawn.
^ permalink raw reply
* Re: [PATCH] Include a git-push example for creating a remote branch
From: Shawn O. Pearce @ 2007-09-06 5:01 UTC (permalink / raw)
To: Miles Bader; +Cc: Junio C Hamano, git
In-Reply-To: <buoodggo0l2.fsf@dhapc248.dev.necel.com>
Miles Bader <miles.bader@necel.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> > Many users get confused when `git push origin master:foo` works
> > when foo already exists on the remote repository but are confused
> > when foo doesn't exist as a branch and this form does not create
> > the branch foo.
>
> Hmm, what _does_ it do in that case...?
error: dst refspec experimental does not match any existing ref on the remote and does not start with refs/.
--
Shawn.
^ permalink raw reply
* Re: [RFC] Convert builin-mailinfo.c to use The Better String Library.
From: Miles Bader @ 2007-09-06 5:03 UTC (permalink / raw)
To: Dmitry Kakurin; +Cc: Matthieu Moy, Git
In-Reply-To: <4AFD7EAD1AAC4E54A416BA3F6E6A9E52@ntdev.corp.microsoft.com>
Dmitry Kakurin <dmitry.kakurin@gmail.com> writes:
> When I first looked at Git source code two things struck me as odd:
> 1. Pure C as opposed to C++. No idea why. Please don't talk about
> portability, it's BS.
Just to piss you off.
-Miles
--
Love is a snowmobile racing across the tundra. Suddenly it flips over,
pinning you underneath. At night the ice weasels come. --Nietzsche
^ permalink raw reply
* [PATCH] git-apply: do not read past the end of buffer
From: Junio C Hamano @ 2007-09-06 5:06 UTC (permalink / raw)
To: git; +Cc: Linus Torvalds
When the preimage we are patching is shorter than what the patch
text expects, we tried to match the buffer contents at the
"original" line with the fragment in full, without checking we
have enough data to match in the preimage. This caused the size
of a later memmove() to wrap around and attempt to scribble
almost the entire address space. Not good.
The code that follows the part this patch touches tries to match
the fragment with line offsets. Curiously, that code does not
have the problem --- it guards against reading past the end of
the preimage.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This is an ancient part of the code; I am somewhat surprised
that nobody has ever reported it earlier.
builtin-apply.c | 3 +-
t/t4123-apply-shrink.sh | 58 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+), 1 deletions(-)
create mode 100755 t/t4123-apply-shrink.sh
diff --git a/builtin-apply.c b/builtin-apply.c
index 25b1447..976ec77 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1514,7 +1514,8 @@ static int find_offset(const char *buf, unsigned long size, const char *fragment
}
/* Exact line number? */
- if (!memcmp(buf + start, fragment, fragsize))
+ if ((start + fragsize <= size) &&
+ !memcmp(buf + start, fragment, fragsize))
return start;
/*
diff --git a/t/t4123-apply-shrink.sh b/t/t4123-apply-shrink.sh
new file mode 100755
index 0000000..984157f
--- /dev/null
+++ b/t/t4123-apply-shrink.sh
@@ -0,0 +1,58 @@
+#!/bin/sh
+
+test_description='apply a patch that is larger than the preimage'
+
+. ./test-lib.sh
+
+cat >F <<\EOF
+1
+2
+3
+4
+5
+6
+7
+8
+999999
+A
+B
+C
+D
+E
+F
+G
+H
+I
+J
+
+EOF
+
+test_expect_success setup '
+
+ git add F &&
+ mv F G &&
+ sed -e "s/1/11/" -e "s/999999/9/" -e "s/H/HH/" <G >F &&
+ git diff >patch &&
+ sed -e "/^\$/d" <G >F &&
+ git add F
+
+'
+
+test_expect_success 'apply should fail gracefully' '
+
+ if git apply --index patch
+ then
+ echo Oops, should not have succeeded
+ false
+ else
+ status=$?
+ echo "Status was $status"
+ if test -f .git/index.lock
+ then
+ echo Oops, should not have crashed
+ false
+ fi
+ fi
+'
+
+test_done
^ permalink raw reply related
* Re: Calculating tree nodes
From: Junio C Hamano @ 2007-09-06 5:21 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Jon Smirl, Git Mailing List
In-Reply-To: <20070906032026.GO18160@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> The latter (build and test log) would generally be very large.
> We would *not* want to cluster them. But we might want to store
> next to the commit a very small pointer to the note itself. Such
> as the note's SHA-1. Or its offset within the packfile's index.
> This would make locating those notes very cheap, while not having
> a huge impact on the common case of commit traversal.
>
> Likewise we might want to pack a tag's SHA-1 alongside of the commit
> it points at, as parsing the commit would immediately give us all
> annotated tags that refer to that commit. Tags are (usually) few
> and far between. But tools like git-describe are commonly used and
> would benefit from not needing to build the commit->tag hashtable.
I am not so sure.
It is a good idea to have an internal API that takes an object
and gives back a set of objects (be they notes to commit or tags
point at object) related to it, but you need to always deal with
loose objects and v2 packs, so you will have to traverse all the
tags and find what they point at _anyway_ to implement the API.
Having a specialized table in v4 does not mean you can look at
that table and nothing else, so I do not immediately see a gain.
^ permalink raw reply
* Re: Significant performance waste in git-svn and friends
From: Mike Hommey @ 2007-09-06 5:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd4wwj16d.fsf@gitster.siamese.dyndns.org>
On Wed, Sep 05, 2007 at 01:40:42PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Mike Hommey <mh@glandium.org> writes:
>
> > The same things obviously apply to git-cvsimport and other scripts
> > calling git-hash-object a lot.
>
> I *obviously* hate this patch, as it makes this Porcelain
> command to be aware of the internal representation too much.
The patch was not supposed to be applied. I said it was lame ;)
It was more of a proof of concept.
Anyways, thinking a bit more about it, I was wondering if it wouldn't be
a good idea to have Git.pm have a "native" implementation (by native I
mean a .so module) for low-level plumbing tools such as hash-object,
cat-file and such.
Obviously, reinventing the wheel is not good, so this native
implementation would be using a "git library" API, such as what has
been done under SoC (though I don't know if this API exposes low-level
plumbing functions)
> I wonder if letting fast-import handle the object creation is an
> option, though.
It could, probably. The reason I didn't use it is that it was way
quicker to hack a 10 lines patch to create the blobs by hand than it
would have been to fork a fast-import object at the correct place during
git-svn initialization and piping to it at the appropriate times.
My goal was only to check how faster this would make it not to fork a
git-hash-object per blob.
Mike
^ permalink raw reply
* Re: People unaware of the importance of "git gc"?
From: David Kastrup @ 2007-09-06 5:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nicolas Pitre, Nix, Steven Grimm, Linus Torvalds,
Git Mailing List
In-Reply-To: <7v1wdciy3w.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Nicolas Pitre <nico@cam.org> writes:
>
>>> This patch does not add invocation of the "auto repacking". It
>>> is left to key Porcelain commands that could produce tons of
>>> loose objects to add a call to "git gc --auto" after they are
>>> done their work. Obvious candidates are:
>>>
>>> git add
>>
>> Nope! 'git add' creates loose objects which are not yet reachable from
>> anywhere. They won't get repacked until a commit is made.
>
> Bzzt, I am releaved to see you are sometimes wrong ;-)
>
> They are reachable from the index and are not subject to
> pruning.
Hm. Isn't it possible to work with several index files at once? I
seem to remember that even git-add does this itself. So what is it
that protects objects in such a temporary index from being garbage
collected by a different git process running on the same repository?
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: [PATCH] Include a git-push example for creating a remote branch
From: Miles Bader @ 2007-09-06 5:57 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20070906050127.GS18160@spearce.org>
On 9/6/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> Miles Bader <miles.bader@necel.com> wrote:
> > > Many users get confused when `git push origin master:foo` works
> > > when foo already exists on the remote repository but are confused
> > > when foo doesn't exist as a branch and this form does not create
> > > the branch foo.
> >
> > Hmm, what _does_ it do in that case...?
>
> error: dst refspec experimental does not match any existing ref on the remote and does not start with refs/.
Hmm, I'm assuming people don't want to default to just creating a new
remote ref ('cause it might be too easy to muck up a remote archive
that way), but it seems like it would be nice to have a "prettier" way
to create a remote ref than explicitly giving the whole ref path on
the remote side.
Maybe I"m weird, but I tend to think of the refs/... syntax as being
for "only if you're doing something funny" cases.
I'm thinking of something like:
... create new local branch "zoink" ...
git push --create origin zoink
-Miles
--
Do not taunt Happy Fun Ball.
^ permalink raw reply
* [PATCH] straighten the list of objects to deltify
From: Nicolas Pitre @ 2007-09-06 6:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Not all objects are subject to deltification, so avoid carrying those
along, and provide the real count to progress display.
Signed-off-by: Nicolas Pitre <nico@cam.org>
---
builtin-pack-objects.c | 77 ++++++++++++++++++++++++++----------------------
1 files changed, 42 insertions(+), 35 deletions(-)
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index e64e3a0..b1c64be 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1313,12 +1313,6 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
if (trg_entry->type != src_entry->type)
return -1;
- /* We do not compute delta to *create* objects we are not
- * going to pack.
- */
- if (trg_entry->preferred_base)
- return -1;
-
/*
* We do not bother to try a delta that we discarded
* on an earlier try, but only when reusing delta data.
@@ -1443,43 +1437,24 @@ static void free_unpacked(struct unpacked *n)
n->depth = 0;
}
-static void find_deltas(struct object_entry **list, int window, int depth)
+static void find_deltas(struct object_entry **list, unsigned list_size,
+ unsigned nr_deltas, int window, int depth)
{
- uint32_t i = nr_objects, idx = 0, count = 0, processed = 0;
+ uint32_t i = list_size, idx = 0, count = 0, processed = 0;
unsigned int array_size = window * sizeof(struct unpacked);
struct unpacked *array;
int max_depth;
- if (!nr_objects)
- return;
array = xmalloc(array_size);
memset(array, 0, array_size);
if (progress)
- start_progress(&progress_state, "Deltifying %u objects...", "", nr_result);
+ start_progress(&progress_state, "Deltifying %u objects...", "", nr_deltas);
do {
struct object_entry *entry = list[--i];
struct unpacked *n = array + idx;
int j, best_base = -1;
- if (!entry->preferred_base)
- processed++;
-
- if (progress)
- display_progress(&progress_state, processed);
-
- if (entry->delta)
- /* This happens if we decided to reuse existing
- * delta from a pack. "!no_reuse_delta &&" is implied.
- */
- continue;
-
- if (entry->size < 50)
- continue;
-
- if (entry->no_try_delta)
- continue;
-
free_unpacked(n);
n->entry = entry;
@@ -1491,6 +1466,15 @@ static void find_deltas(struct object_entry **list, int window, int depth)
count--;
}
+ /* We do not compute delta to *create* objects we are not
+ * going to pack.
+ */
+ if (entry->preferred_base)
+ goto next;
+
+ if (progress)
+ display_progress(&progress_state, ++processed);
+
/*
* If the current object is at pack edge, take the depth the
* objects that depend on the current object into account
@@ -1565,18 +1549,41 @@ static void find_deltas(struct object_entry **list, int window, int depth)
static void prepare_pack(int window, int depth)
{
struct object_entry **delta_list;
- uint32_t i;
+ uint32_t i, n, nr_deltas;
get_object_details();
- if (!window || !depth)
+ if (!nr_objects || !window || !depth)
return;
delta_list = xmalloc(nr_objects * sizeof(*delta_list));
- for (i = 0; i < nr_objects; i++)
- delta_list[i] = objects + i;
- qsort(delta_list, nr_objects, sizeof(*delta_list), type_size_sort);
- find_deltas(delta_list, window+1, depth);
+ nr_deltas = n = 0;
+
+ for (i = 0; i < nr_objects; i++) {
+ struct object_entry *entry = objects + i;
+
+ if (entry->delta)
+ /* This happens if we decided to reuse existing
+ * delta from a pack. "!no_reuse_delta &&" is implied.
+ */
+ continue;
+
+ if (entry->size < 50)
+ continue;
+
+ if (entry->no_try_delta)
+ continue;
+
+ if (!entry->preferred_base)
+ nr_deltas++;
+
+ delta_list[n++] = entry;
+ }
+
+ if (nr_deltas) {
+ qsort(delta_list, n, sizeof(*delta_list), type_size_sort);
+ find_deltas(delta_list, n, nr_deltas, window+1, depth);
+ }
free(delta_list);
}
--
1.5.3.1.844.g0a05-dirty
^ permalink raw reply related
* [PATCH] localize window memory usage accounting
From: Nicolas Pitre @ 2007-09-06 6:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <11890591912193-git-send-email-nico@cam.org>
This is to help threadification of delta searching.
Signed-off-by: Nicolas Pitre <nico@cam.org>
---
builtin-pack-objects.c | 28 ++++++++++++++--------------
1 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index b1c64be..b8495bf 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -78,7 +78,6 @@ static unsigned long delta_cache_size = 0;
static unsigned long max_delta_cache_size = 0;
static unsigned long cache_max_small_delta_size = 1000;
-static unsigned long window_memory_usage = 0;
static unsigned long window_memory_limit = 0;
/*
@@ -1300,7 +1299,7 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
* one.
*/
static int try_delta(struct unpacked *trg, struct unpacked *src,
- unsigned max_depth)
+ unsigned max_depth, unsigned long *mem_usage)
{
struct object_entry *trg_entry = trg->entry;
struct object_entry *src_entry = src->entry;
@@ -1356,7 +1355,7 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
if (sz != trg_size)
die("object %s inconsistent object length (%lu vs %lu)",
sha1_to_hex(trg_entry->idx.sha1), sz, trg_size);
- window_memory_usage += sz;
+ *mem_usage += sz;
}
if (!src->data) {
src->data = read_sha1_file(src_entry->idx.sha1, &type, &sz);
@@ -1366,7 +1365,7 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
if (sz != src_size)
die("object %s inconsistent object length (%lu vs %lu)",
sha1_to_hex(src_entry->idx.sha1), sz, src_size);
- window_memory_usage += sz;
+ *mem_usage += sz;
}
if (!src->index) {
src->index = create_delta_index(src->data, src_size);
@@ -1376,7 +1375,7 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
warning("suboptimal pack - out of memory");
return 0;
}
- window_memory_usage += sizeof_delta_index(src->index);
+ *mem_usage += sizeof_delta_index(src->index);
}
delta_buf = create_delta(src->index, trg->data, trg_size, &delta_size, max_size);
@@ -1423,18 +1422,19 @@ static unsigned int check_delta_limit(struct object_entry *me, unsigned int n)
return m;
}
-static void free_unpacked(struct unpacked *n)
+static unsigned long free_unpacked(struct unpacked *n)
{
- window_memory_usage -= sizeof_delta_index(n->index);
+ unsigned long freed_mem = sizeof_delta_index(n->index);
free_delta_index(n->index);
n->index = NULL;
if (n->data) {
+ freed_mem += n->entry->size;
free(n->data);
n->data = NULL;
- window_memory_usage -= n->entry->size;
}
n->entry = NULL;
n->depth = 0;
+ return freed_mem;
}
static void find_deltas(struct object_entry **list, unsigned list_size,
@@ -1443,7 +1443,7 @@ static void find_deltas(struct object_entry **list, unsigned list_size,
uint32_t i = list_size, idx = 0, count = 0, processed = 0;
unsigned int array_size = window * sizeof(struct unpacked);
struct unpacked *array;
- int max_depth;
+ unsigned long mem_usage = 0;
array = xmalloc(array_size);
memset(array, 0, array_size);
@@ -1453,16 +1453,16 @@ static void find_deltas(struct object_entry **list, unsigned list_size,
do {
struct object_entry *entry = list[--i];
struct unpacked *n = array + idx;
- int j, best_base = -1;
+ int j, max_depth, best_base = -1;
- free_unpacked(n);
+ mem_usage -= free_unpacked(n);
n->entry = entry;
while (window_memory_limit &&
- window_memory_usage > window_memory_limit &&
+ mem_usage > window_memory_limit &&
count > 1) {
uint32_t tail = (idx + window - count) % window;
- free_unpacked(array + tail);
+ mem_usage -= free_unpacked(array + tail);
count--;
}
@@ -1497,7 +1497,7 @@ static void find_deltas(struct object_entry **list, unsigned list_size,
m = array + other_idx;
if (!m->entry)
break;
- ret = try_delta(n, m, max_depth);
+ ret = try_delta(n, m, max_depth, &mem_usage);
if (ret < 0)
break;
else if (ret > 0)
--
1.5.3.1.844.g0a05-dirty
^ permalink raw reply related
* [PATCH] rearrange delta search progress reporting
From: Nicolas Pitre @ 2007-09-06 6:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <11890591923123-git-send-email-nico@cam.org>
This is to help threadification of the delta search code, with a bonus
consistency check.
Signed-off-by: Nicolas Pitre <nico@cam.org>
---
builtin-pack-objects.c | 23 ++++++++++++++---------
1 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index b8495bf..9d56592 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1438,17 +1438,15 @@ static unsigned long free_unpacked(struct unpacked *n)
}
static void find_deltas(struct object_entry **list, unsigned list_size,
- unsigned nr_deltas, int window, int depth)
+ int window, int depth, unsigned *processed)
{
- uint32_t i = list_size, idx = 0, count = 0, processed = 0;
+ uint32_t i = list_size, idx = 0, count = 0;
unsigned int array_size = window * sizeof(struct unpacked);
struct unpacked *array;
unsigned long mem_usage = 0;
array = xmalloc(array_size);
memset(array, 0, array_size);
- if (progress)
- start_progress(&progress_state, "Deltifying %u objects...", "", nr_deltas);
do {
struct object_entry *entry = list[--i];
@@ -1472,8 +1470,9 @@ static void find_deltas(struct object_entry **list, unsigned list_size,
if (entry->preferred_base)
goto next;
+ (*processed)++;
if (progress)
- display_progress(&progress_state, ++processed);
+ display_progress(&progress_state, *processed);
/*
* If the current object is at pack edge, take the depth the
@@ -1536,9 +1535,6 @@ static void find_deltas(struct object_entry **list, unsigned list_size,
idx = 0;
} while (i > 0);
- if (progress)
- stop_progress(&progress_state);
-
for (i = 0; i < window; ++i) {
free_delta_index(array[i].index);
free(array[i].data);
@@ -1581,8 +1577,17 @@ static void prepare_pack(int window, int depth)
}
if (nr_deltas) {
+ unsigned nr_done = 0;
+ if (progress)
+ start_progress(&progress_state,
+ "Deltifying %u objects...", "",
+ nr_deltas);
qsort(delta_list, n, sizeof(*delta_list), type_size_sort);
- find_deltas(delta_list, n, nr_deltas, window+1, depth);
+ find_deltas(delta_list, n, window+1, depth, &nr_done);
+ if (progress)
+ stop_progress(&progress_state);
+ if (nr_done != nr_deltas)
+ die("inconsistency with delta count");
}
free(delta_list);
}
--
1.5.3.1.844.g0a05-dirty
^ permalink raw reply related
* [PATCH] basic threaded delta search
From: Nicolas Pitre @ 2007-09-06 6:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <11890591923270-git-send-email-nico@cam.org>
this is still rough, hence it is disabled by default. You need to compile
with "make THREADED_DELTA_SEARCH=1 ..." at the moment.
Threading is done on different portions of the object list to be
deltified. This is currently done by spliting the list into n parts and
then a thread is spawned for each of them. A better method would consist
of spliting the list into more smaller parts and have the n threads
pick the next part available.
Signed-off-by: Nicolas Pitre <nico@cam.org>
---
Makefile | 8 +++++
builtin-pack-objects.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 90 insertions(+), 1 deletions(-)
diff --git a/Makefile b/Makefile
index 51af531..a92fb31 100644
--- a/Makefile
+++ b/Makefile
@@ -122,6 +122,9 @@ all::
# If not set it defaults to the bare 'wish'. If it is set to the empty
# string then NO_TCLTK will be forced (this is used by configure script).
#
+# Define THREADED_DELTA_SEARCH if you have pthreads and wish to exploit
+# parallel delta searching when packing objects.
+#
GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -662,6 +665,11 @@ ifdef NO_HSTRERROR
COMPAT_OBJS += compat/hstrerror.o
endif
+ifdef THREADED_DELTA_SEARCH
+ BASIC_CFLAGS += -DTHREADED_DELTA_SEARCH
+ EXTLIBS += -lpthread
+endif
+
ifeq ($(TCLTK_PATH),)
NO_TCLTK=NoThanks
endif
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 9d56592..1bcee23 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -15,6 +15,10 @@
#include "list-objects.h"
#include "progress.h"
+#ifdef THREADED_DELTA_SEARCH
+#include <pthread.h>
+#endif
+
static const char pack_usage[] = "\
git-pack-objects [{ -q | --progress | --all-progress }] \n\
[--max-pack-size=N] [--local] [--incremental] \n\
@@ -1290,6 +1294,25 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
return 0;
}
+#ifdef THREADED_DELTA_SEARCH
+
+static pthread_mutex_t read_mutex = PTHREAD_MUTEX_INITIALIZER;
+#define read_lock() pthread_mutex_lock(&read_mutex)
+#define read_unlock() pthread_mutex_unlock(&read_mutex)
+
+static pthread_mutex_t progress_mutex = PTHREAD_MUTEX_INITIALIZER;
+#define progress_lock() pthread_mutex_lock(&progress_mutex)
+#define progress_unlock() pthread_mutex_unlock(&progress_mutex)
+
+#else
+
+#define read_lock() 0
+#define read_unlock() 0
+#define progress_lock() 0
+#define progress_unlock() 0
+
+#endif
+
/*
* We search for deltas _backwards_ in a list sorted by type and
* by size, so that we see progressively smaller and smaller files.
@@ -1348,7 +1371,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
/* Load data if not already done */
if (!trg->data) {
+ read_lock();
trg->data = read_sha1_file(trg_entry->idx.sha1, &type, &sz);
+ read_unlock();
if (!trg->data)
die("object %s cannot be read",
sha1_to_hex(trg_entry->idx.sha1));
@@ -1358,7 +1383,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
*mem_usage += sz;
}
if (!src->data) {
+ read_lock();
src->data = read_sha1_file(src_entry->idx.sha1, &type, &sz);
+ read_unlock();
if (!src->data)
die("object %s cannot be read",
sha1_to_hex(src_entry->idx.sha1));
@@ -1470,9 +1497,11 @@ static void find_deltas(struct object_entry **list, unsigned list_size,
if (entry->preferred_base)
goto next;
+ progress_lock();
(*processed)++;
if (progress)
display_progress(&progress_state, *processed);
+ progress_unlock();
/*
* If the current object is at pack edge, take the depth the
@@ -1542,6 +1571,58 @@ static void find_deltas(struct object_entry **list, unsigned list_size,
free(array);
}
+#ifdef THREADED_DELTA_SEARCH
+
+struct thread_params {
+ pthread_t thread;
+ struct object_entry **list;
+ unsigned list_size;
+ int window;
+ int depth;
+ unsigned *processed;
+};
+
+static void *threaded_find_deltas(void *arg)
+{
+ struct thread_params *p = arg;
+ if (p->list_size)
+ find_deltas(p->list, p->list_size,
+ p->window, p->depth, p->processed);
+ return NULL;
+}
+
+#define NR_THREADS 8
+
+static void ll_find_deltas(struct object_entry **list, unsigned list_size,
+ int window, int depth, unsigned *processed)
+{
+ struct thread_params p[NR_THREADS];
+ int i, ret;
+
+ for (i = 0; i < NR_THREADS; i++) {
+ unsigned sublist_size = list_size / (NR_THREADS - i);
+ p[i].list = list;
+ p[i].list_size = sublist_size;
+ p[i].window = window;
+ p[i].depth = depth;
+ p[i].processed = processed;
+ ret = pthread_create(&p[i].thread, NULL,
+ threaded_find_deltas, &p[i]);
+ if (ret)
+ die("unable to create thread: %s", strerror(ret));
+ list += sublist_size;
+ list_size -= sublist_size;
+ }
+
+ for (i = 0; i < NR_THREADS; i++) {
+ pthread_join(p[i].thread, NULL);
+ }
+}
+
+#else
+#define ll_find_deltas find_deltas
+#endif
+
static void prepare_pack(int window, int depth)
{
struct object_entry **delta_list;
@@ -1583,7 +1664,7 @@ static void prepare_pack(int window, int depth)
"Deltifying %u objects...", "",
nr_deltas);
qsort(delta_list, n, sizeof(*delta_list), type_size_sort);
- find_deltas(delta_list, n, window+1, depth, &nr_done);
+ ll_find_deltas(delta_list, n, window+1, depth, &nr_done);
if (progress)
stop_progress(&progress_state);
if (nr_done != nr_deltas)
--
1.5.3.1.844.g0a05-dirty
^ permalink raw reply related
* my recent patches
From: Nicolas Pitre @ 2007-09-06 6:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
I forgot to have them numbered, so here is the proper order:
1- "[PATCH] straighten the list of objects to deltify"
2- "[PATCH] localize window memory usage accounting"
3- "[PATCH] rearrange delta search progress reporting"
4- "[PATCH] basic threaded delta search"
Based on top of "next".
Nicolas
^ permalink raw reply
* Re: [PATCH] basic threaded delta search
From: David Kastrup @ 2007-09-06 6:19 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Junio C Hamano, git
In-Reply-To: <1189059193250-git-send-email-nico@cam.org>
Nicolas Pitre <nico@cam.org> writes:
> this is still rough, hence it is disabled by default. You need to compile
> with "make THREADED_DELTA_SEARCH=1 ..." at the moment.
>
> Threading is done on different portions of the object list to be
> deltified. This is currently done by spliting the list into n parts and
> then a thread is spawned for each of them. A better method would consist
> of spliting the list into more smaller parts and have the n threads
> pick the next part available.
In my experience, the worst performance hit happens when the real
memory is exhausted and things start paging. Threading on different
portions of the object list is going to exacerbate the problem in the
areas where it hits worst.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags
From: Junio C Hamano @ 2007-09-06 6:20 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Carlos Rica, git
In-Reply-To: <20070906042653.GQ18160@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> "Shawn O. Pearce" <spearce@spearce.org> wrote:
>> If the user has misconfigured `user.signingkey` in their .git/config
>> or just doesn't have any secret keys on their keyring and they ask
>> for a signed tag with `git tag -s` we better make sure the resulting
>> tag was actually signed by gpg.
> ...
>> I think this and my prior contrib/workdir patch should both go into
>> maint. This one in particular; it hurt us today when an automated
>> tool that runs `git tag -s` didn't notice the GnuPG problems.
>
> I'm sorry, despite the subject of this email this is actually a
> *one* patch series. The 2/2 is because I applied and tested this
> on top of the contrib/workdir patch I was talking about above and
> my email sending script thought it was a two patch series.
This seems to fail the test depending on the order processes
happen to be scheduled. I haven't looked at it closely yet.
^ permalink raw reply
* Re: [PATCH] basic threaded delta search
From: Nicolas Pitre @ 2007-09-06 6:23 UTC (permalink / raw)
To: David Kastrup; +Cc: Junio C Hamano, git
In-Reply-To: <85ps0wfh8h.fsf@lola.goethe.zz>
On Thu, 6 Sep 2007, David Kastrup wrote:
> Nicolas Pitre <nico@cam.org> writes:
>
> > this is still rough, hence it is disabled by default. You need to compile
> > with "make THREADED_DELTA_SEARCH=1 ..." at the moment.
> >
> > Threading is done on different portions of the object list to be
> > deltified. This is currently done by spliting the list into n parts and
> > then a thread is spawned for each of them. A better method would consist
> > of spliting the list into more smaller parts and have the n threads
> > pick the next part available.
>
> In my experience, the worst performance hit happens when the real
> memory is exhausted and things start paging. Threading on different
> portions of the object list is going to exacerbate the problem in the
> areas where it hits worst.
Just don't use it if you don't have 1) a SMP machine and 2) enough ram.
Threading will be enabled with a command line argument at some point.
Nicolas
^ 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