Git development
 help / color / mirror / Atom feed
* Re: [PATCH] Add pack.depth option to git-pack-objects and change default depth to 50
From: Theodore Tso @ 2007-05-08 16:30 UTC (permalink / raw)
  To: Nicolas Pitre; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.LFD.0.99.0705081136050.24220@xanadu.home>

On Tue, May 08, 2007 at 11:38:46AM -0400, Nicolas Pitre wrote:
> On Tue, 8 May 2007, Theodore Ts'o wrote:
> 
> > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> 
> I'd prefer if tests were performed on the performance impact before 
> changing the default depth.  If done separately from this patch then the 
> commit log could contain those results as well.

The following results are on a recent git repository, using time to
record the real, user, and sys times on the two commands: "git-gc
--no-reuse-delta --window=X --depth=Y" and "git log --pretty=oneline
-S'object' > /dev/null".  All of these tests were done with a hot
cache, so disk speed didn't enter into the calculations.

                git-gc                     git log -S'object'   pack size
w=10,d=10       27.1s/25.2s/0.3s           20.8s/20.4s/0.1s     15292k
w=10,d=30	23.8s/22.3s/0.2s           21.2s/20.9s/0.1s     12996k
w=10,d=50       24.8s/22.4s/0.4s           21.8s/21.2s/0.1s     12340k
w=100,d=100     24.1s/22.8s/0.3s           22.4s/21.8s/0.2s     11772k

w=30,d=10       45.0s/43.1s/0.4s           20.8s/20.5s/0.1s     14388k
w=30,d=30       35.8s/34.1s/0.3s           21.6s/21.1s/0.1s     11800k
w=30,d=50       34.6s/33.0s/0.3s           22.1s/21.4s/0.1s     11376k
w=30,d=100      34.0s/32.2s/0.3s           22.2s/21.6s/0.1s     11012k

w=50,d=10       56.1s/54.3s/0.4s           21.3s/20.5s/0.1s     14224k
w=50,d=30       47.2s/45.4s/0.4s           21.6s/21.0s/0.1s     11496k      
w=50,d=50       44.5s/43.0s/0.3s           21.7s/21.2s/0.1s     11108k
w=50,d=100      44.3s/42.7s/0.4s           22.4s/21.7s/0.1s     10824k

So a couple of things immediately become evident.  First of all, as
Junio predicted, changing --depth makes no difference to the git-gc or
git log times.  The latter is thanks to the delta chaching.  Secondly,
changing --depth does make a signficiant difference to the pack size.

Finally, --window does help somewhat in reducing the pack size, but it
_significantly_ increases the time to calculate the pack.

My conclusion given this quick benchmark is that it seems to me that
changing the defaults of --depth to 50, and keeping --window at 10, is
a reasonable thing to do.

Regards,

						- Ted

^ permalink raw reply

* Re: [PATCH] Add pack.depth option to git-pack-objects and change default depth to 50
From: Johannes Schindelin @ 2007-05-08 16:49 UTC (permalink / raw)
  To: Theodore Tso; +Cc: Nicolas Pitre, Junio C Hamano, Git Mailing List
In-Reply-To: <20070508163044.GA15796@thunk.org>

Hi,

On Tue, 8 May 2007, Theodore Tso wrote:

> On Tue, May 08, 2007 at 11:38:46AM -0400, Nicolas Pitre wrote:
> > On Tue, 8 May 2007, Theodore Ts'o wrote:
> > 
> > > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> > 
> > I'd prefer if tests were performed on the performance impact before 
> > changing the default depth.  If done separately from this patch then the 
> > commit log could contain those results as well.
> 
> The following results are on a recent git repository, using time to
> record the real, user, and sys times on the two commands: "git-gc
> --no-reuse-delta --window=X --depth=Y" and "git log --pretty=oneline
> -S'object' > /dev/null".  All of these tests were done with a hot
> cache, so disk speed didn't enter into the calculations.
> 
>                 git-gc                     git log -S'object'   pack size
> w=10,d=10       27.1s/25.2s/0.3s           20.8s/20.4s/0.1s     15292k
> w=10,d=30	23.8s/22.3s/0.2s           21.2s/20.9s/0.1s     12996k
> w=10,d=50       24.8s/22.4s/0.4s           21.8s/21.2s/0.1s     12340k
> w=100,d=100     24.1s/22.8s/0.3s           22.4s/21.8s/0.2s     11772k
> 
> w=30,d=10       45.0s/43.1s/0.4s           20.8s/20.5s/0.1s     14388k
> w=30,d=30       35.8s/34.1s/0.3s           21.6s/21.1s/0.1s     11800k
> w=30,d=50       34.6s/33.0s/0.3s           22.1s/21.4s/0.1s     11376k
> w=30,d=100      34.0s/32.2s/0.3s           22.2s/21.6s/0.1s     11012k
> 
> w=50,d=10       56.1s/54.3s/0.4s           21.3s/20.5s/0.1s     14224k
> w=50,d=30       47.2s/45.4s/0.4s           21.6s/21.0s/0.1s     11496k      
> w=50,d=50       44.5s/43.0s/0.3s           21.7s/21.2s/0.1s     11108k
> w=50,d=100      44.3s/42.7s/0.4s           22.4s/21.7s/0.1s     10824k
> 
> So a couple of things immediately become evident.  First of all, as
> Junio predicted, changing --depth makes no difference to the git-gc or
> git log times.  The latter is thanks to the delta chaching.  Secondly,
> changing --depth does make a signficiant difference to the pack size.
> 
> Finally, --window does help somewhat in reducing the pack size, but it
> _significantly_ increases the time to calculate the pack.
> 
> My conclusion given this quick benchmark is that it seems to me that
> changing the defaults of --depth to 50, and keeping --window at 10, is
> a reasonable thing to do.

I'd be happier if that test was done on _at least_ the kernel repo, if not 
something larger, _plus_ having the numbers on page faults. Swapping can 
kill performance substantially...

git.git is small.

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH] Add pack.depth option to git-pack-objects and change default depth to 50
From: Dana How @ 2007-05-08 17:07 UTC (permalink / raw)
  To: Theodore Tso; +Cc: Nicolas Pitre, Junio C Hamano, Git Mailing List, danahow
In-Reply-To: <20070508163044.GA15796@thunk.org>

On 5/8/07, Theodore Tso <tytso@mit.edu> wrote:
> The following results are on a recent git repository, using time to
> record the real, user, and sys times on the two commands: "git-gc
> --no-reuse-delta --window=X --depth=Y" and "git log --pretty=oneline
> -S'object' > /dev/null".  All of these tests were done with a hot
> cache, so disk speed didn't enter into the calculations.
>
> ...
>
> So a couple of things immediately become evident.  First of all, as
> Junio predicted, changing --depth makes no difference to the git-gc or
> git log times.  The latter is thanks to the delta chaching.  Secondly,
> changing --depth does make a signficiant difference to the pack size.
>
> Finally, --window does help somewhat in reducing the pack size, but it
> _significantly_ increases the time to calculate the pack.
>
> My conclusion given this quick benchmark is that it seems to me that
> changing the defaults of --depth to 50, and keeping --window at 10, is
> a reasonable thing to do.

If you still have the packfiles around,  the times for some non-pickaxe
git-log commands would be interesting,  like from git-log's man page:
       git log v2.6.12.. include/scsi drivers/scsi
       git log --since="2 weeks ago" -- gitk
These operations would be more dominated by processing smaller objects.

Thanks,
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell

^ permalink raw reply

* Re: [PATCH] wcwidth redeclaration
From: Amos Waterland @ 2007-05-08 17:18 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0705081202080.4167@racer.site>

On Tue, May 08, 2007 at 12:03:47PM +0200, Johannes Schindelin wrote:
> Thanks indeed. I thought that a static function could shadow a global 
> function, but probably not with a different signature. Just out of 
> curiosity: what is the signature of wcwidth in your string.h?

This is it:

        extern int      wcwidth(wchar_t);

^ permalink raw reply

* Re: [PATCH] Add pack.depth option to git-pack-objects and change default depth to 50
From: Nicolas Pitre @ 2007-05-08 17:35 UTC (permalink / raw)
  To: Theodore Tso; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <20070508163044.GA15796@thunk.org>

On Tue, 8 May 2007, Theodore Tso wrote:

> On Tue, May 08, 2007 at 11:38:46AM -0400, Nicolas Pitre wrote:
> > On Tue, 8 May 2007, Theodore Ts'o wrote:
> > 
> > > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> > 
> > I'd prefer if tests were performed on the performance impact before 
> > changing the default depth.  If done separately from this patch then the 
> > commit log could contain those results as well.
> 
> The following results are on a recent git repository, using time to
> record the real, user, and sys times on the two commands: "git-gc
> --no-reuse-delta --window=X --depth=Y" and "git log --pretty=oneline
> -S'object' > /dev/null".  All of these tests were done with a hot
> cache, so disk speed didn't enter into the calculations.
[...]
> My conclusion given this quick benchmark is that it seems to me that
> changing the defaults of --depth to 50, and keeping --window at 10, is
> a reasonable thing to do.

Effectively.

I'd still prefer to see the default changed in a patch of its own 
though.


Nicolas

^ permalink raw reply

* Re: [PATCH] [TRIVIAL] Change default man page path to /usr/share/man
From: Brian Gernhardt @ 2007-05-08 17:44 UTC (permalink / raw)
  To: Uwe Kleine-König; +Cc: Ismail Dönmez, git
In-Reply-To: <20070508150220.GA18860@informatik.uni-freiburg.de>


On May 8, 2007, at 11:02 AM, Uwe Kleine-König wrote:

> Personally I prefer FHS, so I have
>
> 	mandir=$(prefix)/share/man
>
> in my config.mak.

The Perl man files Git(3pm) and private-Error(3pm) seem to ignore  
that setting though.  :-(

I just have a "mybuild" target in config.mak that calls make all,  
doc, test, install, and install-doc before cleaning up files from the  
wrong places (man instead of share/man, Perl's System dir on OS X)  
and running "stow -R" to catch new files.

~~ Brian

^ permalink raw reply

* Re: git pull failure, truncated object
From: Nicolas Pitre @ 2007-05-08 17:51 UTC (permalink / raw)
  To: Bill Lear; +Cc: git
In-Reply-To: <17984.39776.771508.873437@lisa.zopyra.com>

On Tue, 8 May 2007, Bill Lear wrote:

> On Tuesday, May 8, 2007 at 11:41:14 (-0400) Nicolas Pitre writes:
> >On Tue, 8 May 2007, Bill Lear wrote:
> >
> >> He did a git-gc, twice, and retried.  Still failed.
> >> 
> >> So, he called me in and we tried to see if the server was acting up
> >> --- perhaps an NFS problem, as we've had those before, but got very
> >> different error messages.  Watched the log file from git-daemon, and
> >> saw nothing.  Finally we took a look at the local repos
> >> .git/objects/4b, and 4b93eb81265ea4f2b436618a4b1c3bea2bedf06d was of
> >> length 0.
> >> 
> >> So, I looked in the man page of git-gc and thought to try --prune,
> >> as this was not an active repository.  This worked, and then
> >> the pull did as well.
> >> 
> >> I'm wondering why git-gc did not at least warn us of this problem when
> >> we tried it.  It appeared to us that git-gc gave our repo a clean bill
> >> of health, and so we turned our attention to the remote and
> >> investigated there, instead of continuing in the local repo.
> >
> >git-gc != git-fsck.
> 
> Indeed, as is now clear to me.  Would it be prudent to have git-gc
> run a quick git-fsck internally and warn if things are not in a kosher
> state?

No.  git-fsck is a potentially expensive operation and it is up to you 
to remember that git-gc isn't about repository sanity. git-gc only 
repacks things and if it encounters an object that is corrupt it will 
abort and leave your object store as is.  In your case the corrupted 
object wasn't one that needed to be repacked which explains why git-gc 
succeeded.


Nicolas

^ permalink raw reply

* Re: [PATCH] Add pack.depth option to git-pack-objects and change default depth to 50
From: Theodore Tso @ 2007-05-08 18:09 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Nicolas Pitre, Junio C Hamano, Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0705081848330.4167@racer.site>

On Tue, May 08, 2007 at 06:49:47PM +0200, Johannes Schindelin wrote:
> I'd be happier if that test was done on _at least_ the kernel repo, if not 
> something larger, _plus_ having the numbers on page faults. Swapping can 
> kill performance substantially...

Given how the delta cache works, I really don't think it's going to
matter.  In any case, my laptop has 2gigs of memory, and the kernel
pack file is only 134megs, so you're not going to see any major page
faults....

In any case, here is a quick run:

                git-gc                     git-log -S'object'     
             real/user/sys/min.faults   real/user/sys/min.faults  pack size
w=10,d=10    4:31/257.7/6.2/391711      5:53/326.9/1.7/255156     155940k
w=10,d=30    4:16/242.7/6.5/378193      5:39/331.6/2.3/437283     143144k
w=10,d=50    4:29/250.1/6.7/554493      5:43/334.5/1.9/362574     140080k

You'll note that it's the same thing; git-gc, git-log doesn't change
much, while the pack size decreases as --depth increases.  We're only
seeing at 10% decrease in the pack size, compared to the 20% decrease
with the git repository, but that's probably because of the HTML and
man branches, which no doubt delta compress really, really well.

I can run a full set of benchmarks, varying both --window and --depth,
and also including a non-pickaxe git-log test as requested, but not
until tonight.  I really don't think we'll see any surprises compared
to the earlier runs, though.

After all, if we just stop and think about how the delta caching
works, and how the repacking algorithm works, it's pretty clear that
there shouldn't be any scaling issues with increasing --depth, and
that increasing --window is just going to be painful, and these should
hold true regardless of the size of the repo.

					- Ted

^ permalink raw reply

* Re: [PATCH] Add pack.depth option to git-pack-objects and change default depth to 50
From: Nicolas Pitre @ 2007-05-08 18:46 UTC (permalink / raw)
  To: Theodore Tso; +Cc: Johannes Schindelin, Junio C Hamano, Git Mailing List
In-Reply-To: <20070508180947.GB15796@thunk.org>

On Tue, 8 May 2007, Theodore Tso wrote:

> After all, if we just stop and think about how the delta caching
> works, and how the repacking algorithm works, it's pretty clear that
> there shouldn't be any scaling issues with increasing --depth, and
> that increasing --window is just going to be painful, and these should
> hold true regardless of the size of the repo.

The window size has absolutely no effect on the runtime pack access, 
except maybe for the increased number of deltas.  It is really a pack 
time cost.  The delta depth is the opposite: it has no effect on the 
packing time, but it has the potential to slow down runtime access.  But 
the delta base cache is apparently working really well to mitigate that 
cost, as long as it is big enough of course.


Nicolas

^ permalink raw reply

* git-http-fetch Segmentation fault
From: Luiz Fernando N. Capitulino @ 2007-05-08 19:27 UTC (permalink / raw)
  To: Git Mailing List; +Cc: boiko


 Hi,

 A friend of mine reported an easy to reproduce segmentation fault
when cloning through http from his repository:

"""
~/ git clone http://people.mandriva.com/~boiko/mandrivamenu.git
got 299cdadd846913a052df361e973a947622f23198
walk 299cdadd846913a052df361e973a947622f23198
...
got 0ecd10d9d6ab020c2469a961777854afda705776
/home/lcapitulino/git//bin/git-clone: line 33: 22353 Segmentation fault      (core dumped) git-http-fetch $v -a -w "$tname" "$sha1" "$1"
"""

 Sometimes it shows up as a corrupted double-linked list, detected by
glibc:

"""
*** glibc detected *** git-http-fetch: corrupted double-linked list: 0x080a5680 ***
"""

 It's reproducible with Junio's master and maint branches (3082ac and
53a582 respectivelly).

 BTW, At line 406 (the trap command), git-clone script removes the
directory it was fetching. This removes the core dump file too.
We should not remove the directory if there's a core dump in there.

-- 
Luiz Fernando N. Capitulino

^ permalink raw reply

* Re: [PATCH] git-gui: Call changes "Staged" and "Unstaged" in file list titles.
From: Junio C Hamano @ 2007-05-08 19:29 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Johannes Sixt, git
In-Reply-To: <20070508143859.GO11311@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Yes.  But...
>  
>>  git-gui/git-gui.sh |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> That's the wrong path.  Clearly you made this patch in git.git and
> not the git-gui.git repository itself.  Fortunately for me I added
> -3 to git-am out of habit:
>
>   $ git am -3 -s gg
>   Applying 'git-gui: Call changes "Staged" and "Unstaged" in file list titles.'
>   
>   error: git-gui/git-gui.sh: does not exist in index
>   Using index info to reconstruct a base tree...
>   Falling back to patching base and 3-way merge...
>   Renamed git-gui/git-gui.sh => git-gui.sh
>   Wrote tree 8e034a94d9533af707279766e174b78ddf3874cc
>   Committed: a1a4975824e8f9f88a8c96ae908c488a2c6047c5
>
> Yay Git!  Go go gadget rename detection!  ;-)
>
> I didn't even realize the git-gui/ prefix on the path in the patch
> until after I saw the rename detection message.

Yeah, -3 is a magic, isn't it?

For the benefit of people who are unfamiliar with the internal,
here is how it works.

 - The patch Hannes gave Shawn had a change to
   git-gui/git-gui.sh, like this:

    git-gui/git-gui.sh |    4 ++--
    1 files changed, 2 insertions(+), 2 deletions(-)

   diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
   index 60e79ca..45d251e 100755
    --- a/git-gui/git-gui.sh
    +++ b/git-gui/git-gui.sh
    @@ -5494,7 +5494,7 @@ pack .vpane -anchor n -side top -fill both -expand 1


 - Shawn had a tree that looked like this:

        GIT-VERSION-GEN
        Makefile
        git-gui.sh
        lib/blame.tcl
        lib/branch.tcl
        ...

 - The patch obviously does not apply, as Shawn does not have
   git-gui/git-gui.sh.  However, Shawn instructed "git am" to
   fall back on 3-way merge, which prepared two "fake" trees.

   Fake tree 1 (the ancestor) is prepared by reading the index
   line to figure out what blob the patch applies to.  The
   answer is 60e79ca, which Shawn has in his repository.  The
   resulting tree looks like this:

        040000 tree 0df2c7b...  git-gui
        100755 blob 60e79ca...  git-gui/git-gui.sh

   Fake tree 2 (update) is prepared by applying the patch to the
   ancestor fake tree.

        040000 tree 950e50d...	git-gui
        100755 blob 45d251e...	git-gui/git-gui.sh

 - Then, it falls back to the 3-way merge, using the fake tree 1
   as the common ancestor, Shawn's repository as "our" tree, and
   fake tree 2 as the update being merged into "our" tree.

   To this merge, most of the paths in "our" tree appear as if
   Shawn added them since the common ancestor, except that
   git-gui.sh at the toplevel in "our" tree was moved up by
   Shawn from git-gui subdirectory (and then possibly further
   modified), while git-gui/git-gui.sh was modified by Hannes.

   At the path level, this is the classic case of "one side
   (Shawn) modifies, while the other side (Hannes) kept it at
   the place it was before", 3-way merge at the path level takes
   Shawn's change (which is to "move the file up one level").

   At the same time, at the contents level, this is just a
   normal "common ancestor 60e79ca was modified to Shawn's
   git-gui.sh on one side while Hannes's side modified it to
   45d251e", and the contents are merged using the file level
   3-way merge.  The result is left in git-gui.sh.

   All the other paths look as if Shawn added them to the common
   ancestor that did not have any of them while Hannes did not
   do any such change, so the merge algorithm takes Shawn's
   "additions".

So in the end, Shawn ends up with most of his paths intact,
except that Hannes's patch which were meant to apply to
git-gui/git-gui.sh gets applied to his git-gui.sh.

^ permalink raw reply

* Re: minimize_url in git-svn?
From: Eric Wong @ 2007-05-08 19:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Seth Falcon
In-Reply-To: <7v4pmn4oxj.fsf_-_@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:
> I was trying to run git-svn against this:
> 
> 	https://repo.socialtext.net:8999/svn/socialtext/trunk
> 
> This is an open source project [*1*] and the trunk is supposed
> to be readable by everybody, but it seems that anything outside
> that area needs authentication.  If I mimick the example in
> git-svn.txt manual page to clone from there, it creates trunk,
> trunk/.git, and then asks for password:
> 
> 	$ URL=https://repo.socialtext.net:8999/svn/socialtext/trunk
> 	$ git-svn clone $URL
>         Authentication realm: <https://repo.socialtext.net:8999> Auth for SVN
> 	Password for 'junio': ^C
> 
> I've narrowed it down to this part of git-svn.  If I tell it not
> to bother "minimiz"ing the URL, it seems to import without
> stepping outside of the URL it was given.
> 
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1038,7 +1038,8 @@
>  		}
>  		$self->{repo_id} = $existing;
>  	} else {
> -		my $min_url = Git::SVN::Ra->new($url)->minimize_url;
> +		my $ra = Git::SVN::Ra->new($url);
> +		my $min_url = $url; # $ra->minimize_url;
>  		$existing = find_existing_remote($min_url, $r);
>  		if ($existing) {
>  			unless ($no_write) {

That should be fine.

> Two and half questions.
> 
>  * What does minimize do, and why is it necessary?

I try to connect to the root (or closer to the root) of the repository.
This allows branches and tags to be tracked more effectively without
needing reconnects.  There's a reparent function in SVN 1.4, but it
doesn't work correctly with svn:// repos last I checked (1.4.3)

>  * The resulting git-svn remote tracking branch (and 'master')
>    seems to check out fine, but I do not know what damage the
>    hack to avoid minimizing is causing.  Are there any?  I see
>    many 0{40} lines in trunk/.git/svn/git-svn/.rev_db.* file,
>    and also many lines in unhandled.log file (+empty_dir,
>    +file_prop, and +dir_prop).  Are these something to worry
>    about?

Nope.  unhandled.log is strictly informational.

.rev_db is offset-based database.  Revision numbers to git commits can
be looked up using (SVN revision * 41).

If the project has really high revision numbers (like gcc) or lots of
tags, it's a space-killer.  I've been meaning to add an optional SQLite
alternative to .rev_db for people tracking those projects.  Patches
welcome :)

>  * Assuming there aren't any damage, or maybe some damage that
>    would cause minor decreased functionality/interoperability,
>    would it perhaps make sense to optionally allow skipping the
>    minimizing to avoid this problem?  Would it make sense, or is
>    the setting at socialtext site too esoteric and it isn't
>    worth to worry about?

It *should* be automatically detecting the highest level up it can
access and stop there.  In your case, there's obviously something
broken in my code :(

I've definitely tested this as working against Seth Falcon's hedgehog
repo (URL is somewhere in the archives).  I also setup a test repository
somewhere that I can double-check against.

> [Footnote]
> 
> *1* http://www.socialtext.net/stoss/index.cgi?developing_with_a_dev_env

I'll try to take a look at that in the next few days.  I also have
segfaults to fix that I haven't gotten to :(

-- 
Eric Wong

^ permalink raw reply

* [PATCH] Alter sha1close() 3rd argument to request flush only
From: Dana How @ 2007-05-08 19:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, danahow


update=0 suppressed writing the final SHA-1 but was not used.
Now final=0 suppresses SHA-1 finalization, SHA-1 writing,
and closing -- in other words,  only flush the buffer.

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 csum-file.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/csum-file.c b/csum-file.c
index 7c806ad..9913cb5 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -29,18 +29,20 @@ static void sha1flush(struct sha1file *f, unsigned int count)
 	}
 }
 
-int sha1close(struct sha1file *f, unsigned char *result, int update)
+int sha1close(struct sha1file *f, unsigned char *result, int final)
 {
 	unsigned offset = f->offset;
 	if (offset) {
 		SHA1_Update(&f->ctx, f->buffer, offset);
 		sha1flush(f, offset);
+		f->offset = 0;
 	}
+	if (!final)
+		return 0;	/* only want to flush (no checksum write, no close) */
 	SHA1_Final(f->buffer, &f->ctx);
 	if (result)
 		hashcpy(result, f->buffer);
-	if (update)
-		sha1flush(f, 20);
+	sha1flush(f, 20);
 	if (close(f->fd))
 		die("%s: sha1 file error on close (%s)", f->name, strerror(errno));
 	free(f);
-- 
1.5.2.rc0.787.g0014

^ permalink raw reply related

* [PATCH 0/4] git-repack --max-pack-size
From: Dana How @ 2007-05-08 20:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, danahow


This patchset adds git-fast-import's "--max-pack-size=N"
option,  with the same behavior,  to git-repack.

This latest patchset includes the following new updates:
* sha1close() changes split out as an earlier patch;
* rebased on top of recent changes to pack-objects;
* fixed delta reuse _reporting_ buglet when deltas split;
* temporary packfiles built in GIT_OBJECT_DIRECTORY not GIT_DIR;
* updated documentation.

^ permalink raw reply

* [PATCH 1/4] git-repack --max-pack-size: new file statics and code restructuring
From: Dana How @ 2007-05-08 20:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, danahow


Add "pack_size_limit", the limit specified by --max-pack-size,
"written_list", the list of objects written to the current pack,
and "nr_written", the number of objects in written_list.
Put "base_name" at file scope again and add forward declarations.
Move write_index_file() call from cnd_pack_objects() to
write_pack_file() since only the latter will know how
many times to call write_index_file().

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 builtin-pack-objects.c |   68 +++++++++++++++++++++++++----------------------
 1 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 7bff8ea..d832e2d 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -52,7 +52,8 @@ struct object_entry {
  * nice "minimum seek" order.
  */
 static struct object_entry *objects;
-static uint32_t nr_objects, nr_alloc, nr_result;
+static struct object_entry **written_list;
+static uint32_t nr_objects, nr_alloc, nr_result, nr_written;
 
 static int non_empty;
 static int no_reuse_delta;
@@ -61,9 +62,11 @@ static int incremental;
 static int allow_ofs_delta;
 static const char *pack_tmp_name, *idx_tmp_name;
 static char tmpname[PATH_MAX];
+static const char *base_name;
 static unsigned char pack_file_sha1[20];
 static int progress = 1;
 static int window = 10;
+static uint32_t pack_size_limit;
 static int pack_to_stdout;
 static int num_preferred_base;
 static struct progress progress_state;
@@ -565,7 +568,11 @@ static int open_object_dir_tmp(const char *path)
     return mkstemp(tmpname);
 }
 
-static off_t write_pack_file(void)
+/* forward declarations for write_pack_file */
+static void write_index_file(off_t last_obj_offset, unsigned char *sha1);
+static int adjust_perm(const char *path, mode_t mode);
+
+static void write_pack_file(void)
 {
 	uint32_t i;
 	struct sha1file *f;
@@ -607,7 +614,32 @@ static off_t write_pack_file(void)
 		die("wrote %u objects while expecting %u", written, nr_result);
 	sha1close(f, pack_file_sha1, 1);
 
-	return last_obj_offset;
+	if (!pack_to_stdout) {
+			unsigned char object_list_sha1[20];
+			mode_t mode = umask(0);
+
+			umask(mode);
+			mode = 0444 & ~mode;
+
+			write_index_file(last_obj_offset, object_list_sha1);
+			snprintf(tmpname, sizeof(tmpname), "%s-%s.pack",
+				 base_name, sha1_to_hex(object_list_sha1));
+			if (adjust_perm(pack_tmp_name, mode))
+				die("unable to make temporary pack file readable: %s",
+				    strerror(errno));
+			if (rename(pack_tmp_name, tmpname))
+				die("unable to rename temporary pack file: %s",
+				    strerror(errno));
+			snprintf(tmpname, sizeof(tmpname), "%s-%s.idx",
+				 base_name, sha1_to_hex(object_list_sha1));
+			if (adjust_perm(idx_tmp_name, mode))
+				die("unable to make temporary index file readable: %s",
+				    strerror(errno));
+			if (rename(idx_tmp_name, tmpname))
+				die("unable to rename temporary index file: %s",
+				    strerror(errno));
+			puts(sha1_to_hex(object_list_sha1));
+	}
 }
 
 static int sha1_sort(const void *_a, const void *_b)
@@ -1588,8 +1620,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	int use_internal_rev_list = 0;
 	int thin = 0;
 	uint32_t i;
-	off_t last_obj_offset;
-	const char *base_name = NULL;
 	const char **rp_av;
 	int rp_ac_alloc = 64;
 	int rp_ac;
@@ -1740,33 +1770,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		fprintf(stderr, "Result has %u objects.\n", nr_result);
 	if (nr_result)
 		prepare_pack(window, depth);
-	last_obj_offset = write_pack_file();
-	if (!pack_to_stdout) {
-		unsigned char object_list_sha1[20];
-		mode_t mode = umask(0);
-
-		umask(mode);
-		mode = 0444 & ~mode;
-
-		write_index_file(last_obj_offset, object_list_sha1);
-		snprintf(tmpname, sizeof(tmpname), "%s-%s.pack",
-			 base_name, sha1_to_hex(object_list_sha1));
-		if (adjust_perm(pack_tmp_name, mode))
-			die("unable to make temporary pack file readable: %s",
-			    strerror(errno));
-		if (rename(pack_tmp_name, tmpname))
-			die("unable to rename temporary pack file: %s",
-			    strerror(errno));
-		snprintf(tmpname, sizeof(tmpname), "%s-%s.idx",
-			 base_name, sha1_to_hex(object_list_sha1));
-		if (adjust_perm(idx_tmp_name, mode))
-			die("unable to make temporary index file readable: %s",
-			    strerror(errno));
-		if (rename(idx_tmp_name, tmpname))
-			die("unable to rename temporary index file: %s",
-			    strerror(errno));
-		puts(sha1_to_hex(object_list_sha1));
-	}
+	write_pack_file();
 	if (progress)
 		fprintf(stderr, "Total %u (delta %u), reused %u (delta %u)\n",
 			written, written_delta, reused, reused_delta);
-- 
1.5.2.rc0.787.g0014

^ permalink raw reply related

* [PATCH 2/4] git-repack --max-pack-size: write_{object,one}() respect pack limit
From: Dana How @ 2007-05-08 20:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, danahow


With --max-pack-size,  generate the appropriate write limit
for each object and check against it before each group of writes.
Update delta usability rules to handle base being in a previously-
written pack.  Inline sha1write_compress() so we know the
exact size of the written data when it needs to be compressed.
Detect and return write "failure".

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 builtin-pack-objects.c |  135 ++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 108 insertions(+), 27 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index d832e2d..fbff8fa 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -399,16 +399,31 @@ static int revalidate_loose_object(struct object_entry *entry,
 }
 
 static unsigned long write_object(struct sha1file *f,
-				  struct object_entry *entry)
+				  struct object_entry *entry,
+				  off_t write_offset)
 {
 	unsigned long size;
 	enum object_type type;
 	void *buf;
 	unsigned char header[10];
+	unsigned char dheader[10];
 	unsigned hdrlen;
 	off_t datalen;
 	enum object_type obj_type;
 	int to_reuse = 0;
+	/* write limit if limited packsize and not first object */
+	unsigned long limit = pack_size_limit && nr_written ?
+				pack_size_limit - write_offset : 0;
+				/* no if no delta */
+	int usable_delta =	!entry->delta ? 0 :
+				/* yes if unlimited packfile */
+				!pack_size_limit ? 1 :
+				/* no if base written to previous pack */
+				entry->delta->offset == (off_t)-1 ? 0 :
+				/* otherwise double-check written to this
+				 * pack,  like we do below
+				 */
+				entry->delta->offset ? 1 : 0;
 
 	if (!pack_to_stdout)
 		crc32_begin(f);
@@ -417,7 +432,9 @@ static unsigned long write_object(struct sha1file *f,
 	if (! entry->in_pack)
 		to_reuse = 0;	/* can't reuse what we don't have */
 	else if (obj_type == OBJ_REF_DELTA || obj_type == OBJ_OFS_DELTA)
-		to_reuse = 1;	/* check_object() decided it for us */
+				/* check_object() decided it for us ... */
+		to_reuse = usable_delta;
+				/* ... but pack split may override that */
 	else if (obj_type != entry->in_pack_type)
 		to_reuse = 0;	/* pack has delta which is unusable */
 	else if (entry->delta)
@@ -436,6 +453,10 @@ static unsigned long write_object(struct sha1file *f,
 			if (revalidate_loose_object(entry, map, mapsize))
 				die("corrupt loose object %s",
 				    sha1_to_hex(entry->sha1));
+			if (limit && mapsize + 20 >= limit) {
+				munmap(map, mapsize);
+				return 0;
+			}
 			sha1write(f, map, mapsize);
 			munmap(map, mapsize);
 			written++;
@@ -447,24 +468,48 @@ static unsigned long write_object(struct sha1file *f,
 	}
 
 	if (!to_reuse) {
+		z_stream stream;
+		unsigned long maxsize;
+		void *out;
 		buf = read_sha1_file(entry->sha1, &type, &size);
 		if (!buf)
 			die("unable to read %s", sha1_to_hex(entry->sha1));
 		if (size != entry->size)
 			die("object %s size inconsistency (%lu vs %lu)",
 			    sha1_to_hex(entry->sha1), size, entry->size);
-		if (entry->delta) {
+		if (usable_delta) {
 			buf = delta_against(buf, size, entry);
 			size = entry->delta_size;
 			obj_type = (allow_ofs_delta && entry->delta->offset) ?
 				OBJ_OFS_DELTA : OBJ_REF_DELTA;
+		} else {
+			/*
+			 * recover real object type in case
+			 * check_object() wanted to re-use a delta,
+			 * but we couldn't since base was in previous split pack
+			 */
+			obj_type = type;
 		}
+		/* compress the data to store and put compressed length in datalen */
+		memset(&stream, 0, sizeof(stream));
+		deflateInit(&stream, zlib_compression_level);
+		maxsize = deflateBound(&stream, size);
+		out = xmalloc(maxsize);
+		/* Compress it */
+		stream.next_in = buf;
+		stream.avail_in = size;
+		stream.next_out = out;
+		stream.avail_out = maxsize;
+		while (deflate(&stream, Z_FINISH) == Z_OK)
+			/* nothing */;
+		deflateEnd(&stream);
+		datalen = stream.total_out;
+		deflateEnd(&stream);
 		/*
 		 * The object header is a byte of 'type' followed by zero or
 		 * more bytes of length.
 		 */
 		hdrlen = encode_header(obj_type, size, header);
-		sha1write(f, header, hdrlen);
 
 		if (obj_type == OBJ_OFS_DELTA) {
 			/*
@@ -473,21 +518,41 @@ static unsigned long write_object(struct sha1file *f,
 			 * base from this object's position in the pack.
 			 */
 			off_t ofs = entry->offset - entry->delta->offset;
-			unsigned pos = sizeof(header) - 1;
-			header[pos] = ofs & 127;
+			unsigned pos = sizeof(dheader) - 1;
+			dheader[pos] = ofs & 127;
 			while (ofs >>= 7)
-				header[--pos] = 128 | (--ofs & 127);
-			sha1write(f, header + pos, sizeof(header) - pos);
-			hdrlen += sizeof(header) - pos;
+				dheader[--pos] = 128 | (--ofs & 127);
+			if (limit && hdrlen + sizeof(dheader) - pos + datalen + 20 >= limit) {
+				free(out);
+				free(buf);
+				return 0;
+			}
+			sha1write(f, header, hdrlen);
+			sha1write(f, dheader + pos, sizeof(dheader) - pos);
+			hdrlen += sizeof(dheader) - pos;
 		} else if (obj_type == OBJ_REF_DELTA) {
 			/*
 			 * Deltas with a base reference contain
 			 * an additional 20 bytes for the base sha1.
 			 */
+			if (limit && hdrlen + 20 + datalen + 20 >= limit) {
+				free(out);
+				free(buf);
+				return 0;
+			}
+			sha1write(f, header, hdrlen);
 			sha1write(f, entry->delta->sha1, 20);
 			hdrlen += 20;
+		} else {
+			if (limit && hdrlen + datalen + 20 >= limit) {
+				free(out);
+				free(buf);
+				return 0;
+			}
+			sha1write(f, header, hdrlen);
 		}
-		datalen = sha1write_compressed(f, buf, size);
+		sha1write(f, out, datalen);
+		free(out);
 		free(buf);
 	}
 	else {
@@ -502,20 +567,6 @@ static unsigned long write_object(struct sha1file *f,
 			reused_delta++;
 		}
 		hdrlen = encode_header(obj_type, entry->size, header);
-		sha1write(f, header, hdrlen);
-		if (obj_type == OBJ_OFS_DELTA) {
-			off_t ofs = entry->offset - entry->delta->offset;
-			unsigned pos = sizeof(header) - 1;
-			header[pos] = ofs & 127;
-			while (ofs >>= 7)
-				header[--pos] = 128 | (--ofs & 127);
-			sha1write(f, header + pos, sizeof(header) - pos);
-			hdrlen += sizeof(header) - pos;
-		} else if (obj_type == OBJ_REF_DELTA) {
-			sha1write(f, entry->delta->sha1, 20);
-			hdrlen += 20;
-		}
-
 		offset = entry->in_pack_offset;
 		revidx = find_packed_object(p, offset);
 		datalen = revidx[1].offset - offset;
@@ -524,6 +575,29 @@ static unsigned long write_object(struct sha1file *f,
 			die("bad packed object CRC for %s", sha1_to_hex(entry->sha1));
 		offset += entry->in_pack_header_size;
 		datalen -= entry->in_pack_header_size;
+		if (obj_type == OBJ_OFS_DELTA) {
+			off_t ofs = entry->offset - entry->delta->offset;
+			unsigned pos = sizeof(dheader) - 1;
+			dheader[pos] = ofs & 127;
+			while (ofs >>= 7)
+				dheader[--pos] = 128 | (--ofs & 127);
+			if (limit && hdrlen + sizeof(dheader) - pos + datalen + 20 >= limit)
+				return 0;
+			sha1write(f, header, hdrlen);
+			sha1write(f, dheader + pos, sizeof(dheader) - pos);
+			hdrlen += sizeof(dheader) - pos;
+		} else if (obj_type == OBJ_REF_DELTA) {
+			if (limit && hdrlen + 20 + datalen + 20 >= limit)
+				return 0;
+			sha1write(f, header, hdrlen);
+			sha1write(f, entry->delta->sha1, 20);
+			hdrlen += 20;
+		} else {
+			if (limit && hdrlen + datalen + 20 >= limit)
+				return 0;
+			sha1write(f, header, hdrlen);
+		}
+
 		if (!pack_to_stdout && p->index_version == 1 &&
 		    check_pack_inflate(p, &w_curs, offset, datalen, entry->size))
 			die("corrupt packed object for %s", sha1_to_hex(entry->sha1));
@@ -531,7 +605,7 @@ static unsigned long write_object(struct sha1file *f,
 		unuse_pack(&w_curs);
 		reused++;
 	}
-	if (entry->delta)
+	if (usable_delta)
 		written_delta++;
 	written++;
 	if (!pack_to_stdout)
@@ -550,11 +624,18 @@ static off_t write_one(struct sha1file *f,
 		return offset;
 
 	/* if we are deltified, write out base object first. */
-	if (e->delta)
+	if (e->delta) {
 		offset = write_one(f, e->delta, offset);
+		if (!offset)
+			return 0;
+	}
 
 	e->offset = offset;
-	size = write_object(f, e);
+	size = write_object(f, e, offset);
+	if (!size) {
+		e->offset = 0;
+		return 0;
+	}
 
 	/* make sure off_t is sufficiently large not to wrap */
 	if (offset > offset + size)
-- 
1.5.2.rc0.787.g0014

^ permalink raw reply related

* [PATCH 3/4] git-repack --max-pack-size: split packs as asked by write_{object,one}()
From: Dana How @ 2007-05-08 20:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, danahow


Rewrite write_pack_file() to break to a new packfile
whenever write_object/write_one request it,  and
correct the header's object count in the previous packfile.
Change write_index_file() to write an index
for just the objects in the most recent packfile.

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 builtin-pack-objects.c |  120 ++++++++++++++++++++++++++++-------------------
 1 files changed, 71 insertions(+), 49 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index fbff8fa..2932b88 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -636,6 +636,7 @@ static off_t write_one(struct sha1file *f,
 		e->offset = 0;
 		return 0;
 	}
+	written_list[nr_written++] = e;
 
 	/* make sure off_t is sufficiently large not to wrap */
 	if (offset > offset + size)
@@ -655,47 +656,57 @@ static int adjust_perm(const char *path, mode_t mode);
 
 static void write_pack_file(void)
 {
-	uint32_t i;
+	uint32_t i = 0, j;
 	struct sha1file *f;
-	off_t offset, last_obj_offset = 0;
+	off_t offset, offset_one, last_obj_offset = 0;
 	struct pack_header hdr;
-	int do_progress = progress;
-
-	if (pack_to_stdout) {
-		f = sha1fd(1, "<stdout>");
-		do_progress >>= 1;
-	} else {
-		int fd = open_object_dir_tmp("tmp_pack_XXXXXX");
-		if (fd < 0)
-			die("unable to create %s: %s\n", tmpname, strerror(errno));
-		pack_tmp_name = xstrdup(tmpname);
-		f = sha1fd(fd, pack_tmp_name);
-	}
+	int do_progress = progress >> pack_to_stdout;
+	uint32_t nr_remaining = nr_result;
 
 	if (do_progress)
 		start_progress(&progress_state, "Writing %u objects...", "", nr_result);
+	written_list = xmalloc(nr_objects * sizeof(struct object_entry *));
 
-	hdr.hdr_signature = htonl(PACK_SIGNATURE);
-	hdr.hdr_version = htonl(PACK_VERSION);
-	hdr.hdr_entries = htonl(nr_result);
-	sha1write(f, &hdr, sizeof(hdr));
-	offset = sizeof(hdr);
-	if (!nr_result)
-		goto done;
-	for (i = 0; i < nr_objects; i++) {
-		last_obj_offset = offset;
-		offset = write_one(f, objects + i, offset);
-		if (do_progress)
-			display_progress(&progress_state, written);
-	}
-	if (do_progress)
-		stop_progress(&progress_state);
- done:
-	if (written != nr_result)
-		die("wrote %u objects while expecting %u", written, nr_result);
-	sha1close(f, pack_file_sha1, 1);
+	do {
+		if (pack_to_stdout) {
+			f = sha1fd(1, "<stdout>");
+		} else {
+			int fd = open_object_dir_tmp("tmp_pack_XXXXXX");
+			if (fd < 0)
+				die("unable to create %s: %s\n", tmpname, strerror(errno));
+			pack_tmp_name = xstrdup(tmpname);
+			f = sha1fd(fd, pack_tmp_name);
+		}
 
-	if (!pack_to_stdout) {
+		hdr.hdr_signature = htonl(PACK_SIGNATURE);
+		hdr.hdr_version = htonl(PACK_VERSION);
+		hdr.hdr_entries = htonl(nr_remaining);
+		sha1write(f, &hdr, sizeof(hdr));
+		offset = sizeof(hdr);
+		nr_written = 0;
+		for (; i < nr_objects; i++) {
+			last_obj_offset = offset;
+			offset_one = write_one(f, objects + i, offset);
+			if (!offset_one)
+				break;
+			offset = offset_one;
+			if (do_progress)
+				display_progress(&progress_state, written);
+		}
+
+		/*
+		 * Did we write the wrong # entries in the header?
+		 * If so, rewrite it like in fast-import
+		 */
+		if (pack_to_stdout || nr_written == nr_remaining) {
+			sha1close(f, pack_file_sha1, 1);
+		} else {
+			sha1close(f, pack_file_sha1, 0);
+			fixup_pack_header_footer(f->fd, pack_file_sha1, pack_tmp_name, nr_written);
+			close(f->fd);
+		}
+
+		if (!pack_to_stdout) {
 			unsigned char object_list_sha1[20];
 			mode_t mode = umask(0);
 
@@ -720,7 +731,26 @@ static void write_pack_file(void)
 				die("unable to rename temporary index file: %s",
 				    strerror(errno));
 			puts(sha1_to_hex(object_list_sha1));
+		}
+
+		/* mark written objects as written to previous pack */
+		for (j = 0; j < nr_written; j++) {
+			written_list[j]->offset = (off_t)-1;
+		}
+		nr_remaining -= nr_written;
+	} while (nr_remaining && i < nr_objects);
+
+	free(written_list);
+	if (do_progress)
+		stop_progress(&progress_state);
+	if (written != nr_result)
+		die("wrote %u objects while expecting %u", written, nr_result);
+	for (j = 0; i < nr_objects; i++) {
+		struct object_entry *e = objects + i;
+		j += !e->offset && !e->preferred_base;
 	}
+	if (j)
+		die("wrote %u objects as expected but %u unwritten", written, j);
 }
 
 static int sha1_sort(const void *_a, const void *_b)
@@ -747,18 +777,11 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)
 	idx_tmp_name = xstrdup(tmpname);
 	f = sha1fd(fd, idx_tmp_name);
 
-	if (nr_result) {
-		uint32_t j = 0;
-		sorted_by_sha =
-			xcalloc(nr_result, sizeof(struct object_entry *));
-		for (i = 0; i < nr_objects; i++)
-			if (!objects[i].preferred_base)
-				sorted_by_sha[j++] = objects + i;
-		if (j != nr_result)
-			die("listed %u objects while expecting %u", j, nr_result);
-		qsort(sorted_by_sha, nr_result, sizeof(*sorted_by_sha), sha1_sort);
+	if (nr_written) {
+		sorted_by_sha = written_list;
+		qsort(sorted_by_sha, nr_written, sizeof(*sorted_by_sha), sha1_sort);
 		list = sorted_by_sha;
-		last = sorted_by_sha + nr_result;
+		last = sorted_by_sha + nr_written;
 	} else
 		sorted_by_sha = list = last = NULL;
 
@@ -796,7 +819,7 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)
 
 	/* Write the actual SHA1 entries. */
 	list = sorted_by_sha;
-	for (i = 0; i < nr_result; i++) {
+	for (i = 0; i < nr_written; i++) {
 		struct object_entry *entry = *list++;
 		if (index_version < 2) {
 			uint32_t offset = htonl(entry->offset);
@@ -811,7 +834,7 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)
 
 		/* write the crc32 table */
 		list = sorted_by_sha;
-		for (i = 0; i < nr_objects; i++) {
+		for (i = 0; i < nr_written; i++) {
 			struct object_entry *entry = *list++;
 			uint32_t crc32_val = htonl(entry->crc32);
 			sha1write(f, &crc32_val, 4);
@@ -819,7 +842,7 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)
 
 		/* write the 32-bit offset table */
 		list = sorted_by_sha;
-		for (i = 0; i < nr_objects; i++) {
+		for (i = 0; i < nr_written; i++) {
 			struct object_entry *entry = *list++;
 			uint32_t offset = (entry->offset <= index_off32_limit) ?
 				entry->offset : (0x80000000 | nr_large_offset++);
@@ -844,7 +867,6 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)
 
 	sha1write(f, pack_file_sha1, 20);
 	sha1close(f, NULL, 1);
-	free(sorted_by_sha);
 	SHA1_Final(sha1, &ctx);
 }
 
-- 
1.5.2.rc0.787.g0014

^ permalink raw reply related

* Re: git-http-fetch Segmentation fault
From: Junio C Hamano @ 2007-05-08 20:04 UTC (permalink / raw)
  To: Luiz Fernando N. Capitulino; +Cc: Git Mailing List, boiko
In-Reply-To: <20070508162735.6c530a70@localhost>

"Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> escreveu:

>  A friend of mine reported an easy to reproduce segmentation fault
> when cloning through http from his repository:
>
> """
> ~/ git clone http://people.mandriva.com/~boiko/mandrivamenu.git
> got 299cdadd846913a052df361e973a947622f23198
> walk 299cdadd846913a052df361e973a947622f23198
> ...
> got 0ecd10d9d6ab020c2469a961777854afda705776
> /home/lcapitulino/git//bin/git-clone: line 33: 22353 Segmentation fault      (core dumped) git-http-fetch $v -a -w "$tname" "$sha1" "$1"
> """
>
>  Sometimes it shows up as a corrupted double-linked list, detected by
> glibc:

Does not seem to reproduce for me on my x86_64 box nor an i386
box I happened to have access to.  Both run Debian etch.

Here is an excerpt from a valgrind run on the x86_64 box.  It
does seem to find a handful "problematic" accesses, but all seem
to be in the system libraries.  I did not get any errors on the
i386 box.

: gitster v/master; valgrind git-http-fetch -v -a -w remotes/origin/master 299cdadd846913a052df361e973a947622f23198 http://people.mandriva.com/~boiko/mandrivamenu.git/

==2120== Conditional jump or move depends on uninitialised value(s)
==2120==    at 0x4010AEE: (within /lib/ld-2.3.6.so)
==2120==    by 0x4006CB6: (within /lib/ld-2.3.6.so)
==2120==    by 0x51FF260: (within /lib/libc-2.3.6.so)
==2120==    by 0x400B7D0: (within /lib/ld-2.3.6.so)
==2120==    by 0x51FECF7: _dl_open (in /lib/libc-2.3.6.so)
==2120==    by 0x5201497: (within /lib/libc-2.3.6.so)
==2120==    by 0x400B7D0: (within /lib/ld-2.3.6.so)
==2120==    by 0x52014E1: __libc_dlopen_mode (in /lib/libc-2.3.6.so)
==2120==    by 0x51DC456: __nss_lookup_function (in /lib/libc-2.3.6.so)
==2120==    by 0x51B6FCE: (within /lib/libc-2.3.6.so)
==2120==    by 0x51B7C06: getaddrinfo (in /lib/libc-2.3.6.so)
==2120==    by 0x4ED31E2: (within /usr/lib/libcurl.so.3.0.0)
==2120== 
==2120== Conditional jump or move depends on uninitialised value(s)
==2120==    at 0x4010AF9: (within /lib/ld-2.3.6.so)
==2120==    by 0x4006CB6: (within /lib/ld-2.3.6.so)
==2120==    by 0x51FF260: (within /lib/libc-2.3.6.so)
==2120==    by 0x400B7D0: (within /lib/ld-2.3.6.so)
==2120==    by 0x51FECF7: _dl_open (in /lib/libc-2.3.6.so)
==2120==    by 0x5201497: (within /lib/libc-2.3.6.so)
==2120==    by 0x400B7D0: (within /lib/ld-2.3.6.so)
==2120==    by 0x52014E1: __libc_dlopen_mode (in /lib/libc-2.3.6.so)
==2120==    by 0x51DC456: __nss_lookup_function (in /lib/libc-2.3.6.so)
==2120==    by 0x51B6FCE: (within /lib/libc-2.3.6.so)
==2120==    by 0x51B7C06: getaddrinfo (in /lib/libc-2.3.6.so)
==2120==    by 0x4ED31E2: (within /usr/lib/libcurl.so.3.0.0)
==2120== 
==2120== Conditional jump or move depends on uninitialised value(s)
==2120==    at 0x4010B04: (within /lib/ld-2.3.6.so)
==2120==    by 0x4006CB6: (within /lib/ld-2.3.6.so)
==2120==    by 0x51FF260: (within /lib/libc-2.3.6.so)
==2120==    by 0x400B7D0: (within /lib/ld-2.3.6.so)
==2120==    by 0x51FECF7: _dl_open (in /lib/libc-2.3.6.so)
==2120==    by 0x5201497: (within /lib/libc-2.3.6.so)
==2120==    by 0x400B7D0: (within /lib/ld-2.3.6.so)
==2120==    by 0x52014E1: __libc_dlopen_mode (in /lib/libc-2.3.6.so)
==2120==    by 0x51DC456: __nss_lookup_function (in /lib/libc-2.3.6.so)
==2120==    by 0x51B6FCE: (within /lib/libc-2.3.6.so)
==2120==    by 0x51B7C06: getaddrinfo (in /lib/libc-2.3.6.so)
==2120==    by 0x4ED31E2: (within /usr/lib/libcurl.so.3.0.0)
==2120== 
==2120== Conditional jump or move depends on uninitialised value(s)
==2120==    at 0x4010C61: (within /lib/ld-2.3.6.so)
==2120==    by 0x4006E47: (within /lib/ld-2.3.6.so)
==2120==    by 0x51FF260: (within /lib/libc-2.3.6.so)
==2120==    by 0x400B7D0: (within /lib/ld-2.3.6.so)
==2120==    by 0x51FECF7: _dl_open (in /lib/libc-2.3.6.so)
==2120==    by 0x5201497: (within /lib/libc-2.3.6.so)
==2120==    by 0x400B7D0: (within /lib/ld-2.3.6.so)
==2120==    by 0x52014E1: __libc_dlopen_mode (in /lib/libc-2.3.6.so)
==2120==    by 0x51DC456: __nss_lookup_function (in /lib/libc-2.3.6.so)
==2120==    by 0x51B6FCE: (within /lib/libc-2.3.6.so)
==2120==    by 0x51B7C06: getaddrinfo (in /lib/libc-2.3.6.so)
==2120==    by 0x4ED31E2: (within /usr/lib/libcurl.so.3.0.0)
==2120== 
==2120== Invalid read of size 8
==2120==    at 0x4010C54: (within /lib/ld-2.3.6.so)
==2120==    by 0x4008D38: (within /lib/ld-2.3.6.so)
==2120==    by 0x4004CF2: (within /lib/ld-2.3.6.so)
==2120==    by 0x4006D6C: (within /lib/ld-2.3.6.so)
==2120==    by 0x51FF260: (within /lib/libc-2.3.6.so)
==2120==    by 0x400B7D0: (within /lib/ld-2.3.6.so)
==2120==    by 0x51FECF7: _dl_open (in /lib/libc-2.3.6.so)
==2120==    by 0x5201497: (within /lib/libc-2.3.6.so)
==2120==    by 0x400B7D0: (within /lib/ld-2.3.6.so)
==2120==    by 0x52014E1: __libc_dlopen_mode (in /lib/libc-2.3.6.so)
==2120==    by 0x51DC456: __nss_lookup_function (in /lib/libc-2.3.6.so)
==2120==    by 0x51B6FCE: (within /lib/libc-2.3.6.so)
==2120==  Address 0x5F096F0 is 16 bytes inside a block of size 23 alloc'd
==2120==    at 0x4A1B858: malloc (vg_replace_malloc.c:149)
==2120==    by 0x4007113: (within /lib/ld-2.3.6.so)
==2120==    by 0x51FF260: (within /lib/libc-2.3.6.so)
==2120==    by 0x400B7D0: (within /lib/ld-2.3.6.so)
==2120==    by 0x51FECF7: _dl_open (in /lib/libc-2.3.6.so)
==2120==    by 0x5201497: (within /lib/libc-2.3.6.so)
==2120==    by 0x400B7D0: (within /lib/ld-2.3.6.so)
==2120==    by 0x52014E1: __libc_dlopen_mode (in /lib/libc-2.3.6.so)
==2120==    by 0x51DC456: __nss_lookup_function (in /lib/libc-2.3.6.so)
==2120==    by 0x51B6FCE: (within /lib/libc-2.3.6.so)
==2120==    by 0x51B7C06: getaddrinfo (in /lib/libc-2.3.6.so)
==2120==    by 0x4ED31E2: (within /usr/lib/libcurl.so.3.0.0)
...
==2120== 
==2120== IN SUMMARY: 6 errors from 5 contexts (suppressed: 9 from 1)
==2120== 

^ permalink raw reply

* [PATCH 4/4] git-repack --max-pack-size: add option parsing to enable feature
From: Dana How @ 2007-05-08 20:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, danahow


Add --max-pack-size parsing and usage messages.
Upgrade git-repack.sh to handle multiple packfile names,
and build packfiles in GIT_OBJECT_DIRECTORY not GIT_DIR.
Update documentation.

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 Documentation/git-pack-objects.txt |    5 +++++
 Documentation/git-repack.txt       |    5 +++++
 builtin-pack-objects.c             |    9 ++++++++-
 git-repack.sh                      |   14 ++++++++------
 4 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index d9e11c6..e762cc7 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -85,6 +85,11 @@ base-name::
 	times to get to the necessary object.
 	The default value for both --window and --depth is 10.
 
+--max-pack-size=<n>::
+	Maximum size of each output packfile, expressed in MiB.
+	If specified,  multiple packfiles may be created.
+	The default is unlimited.
+
 --incremental::
 	This flag causes an object already in a pack ignored
 	even if it appears in the standard input.
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index d39abc1..c6be673 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -65,6 +65,11 @@ OPTIONS
 	to be applied that many times to get to the necessary object.
 	The default value for both --window and --depth is 10.
 
+--max-pack-size=<n>::
+	Maximum size of each output packfile, expressed in MiB.
+	If specified,  multiple packfiles may be created.
+	The default is unlimited.
+
 
 Configuration
 -------------
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 2932b88..8824793 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -15,7 +15,7 @@
 #include "progress.h"
 
 static const char pack_usage[] = "\
-git-pack-objects [{ -q | --progress | --all-progress }] \n\
+git-pack-objects [{ -q | --progress | --all-progress }] [--max-pack-size=N] \n\
 	[--local] [--incremental] [--window=N] [--depth=N] \n\
 	[--no-reuse-delta] [--delta-base-offset] [--non-empty] \n\
 	[--revs [--unpacked | --all]*] [--reflog] [--stdout | base-name] \n\
@@ -1754,6 +1754,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			incremental = 1;
 			continue;
 		}
+		if (!prefixcmp(arg, "--max-pack-size=")) {
+			char *end;
+			pack_size_limit = strtoul(arg+16, &end, 0) * 1024 * 1024;
+			if (!arg[16] || *end)
+				usage(pack_usage);
+			continue;
+		}
 		if (!prefixcmp(arg, "--window=")) {
 			char *end;
 			window = strtoul(arg+9, &end, 0);
diff --git a/git-repack.sh b/git-repack.sh
index ddfa8b4..44c12a7 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2005 Linus Torvalds
 #
 
-USAGE='[-a] [-d] [-f] [-l] [-n] [-q] [--window=N] [--depth=N]'
+USAGE='[-a] [-d] [-f] [-l] [-n] [-q] [--max-pack-size=N] [--window=N] [--depth=N]'
 SUBDIRECTORY_OK='Yes'
 . git-sh-setup
 
@@ -18,6 +18,7 @@ do
 	-q)	quiet=-q ;;
 	-f)	no_reuse_delta=--no-reuse-delta ;;
 	-l)	local=--local ;;
+	--max-pack-size=*) extra="$extra $1" ;;
 	--window=*) extra="$extra $1" ;;
 	--depth=*) extra="$extra $1" ;;
 	*)	usage ;;
@@ -35,7 +36,7 @@ true)
 esac
 
 PACKDIR="$GIT_OBJECT_DIRECTORY/pack"
-PACKTMP="$GIT_DIR/.tmp-$$-pack"
+PACKTMP="$GIT_OBJECT_DIRECTORY/.tmp-$$-pack"
 rm -f "$PACKTMP"-*
 trap 'rm -f "$PACKTMP"-*' 0 1 2 3 15
 
@@ -62,11 +63,12 @@ case ",$all_into_one," in
 esac
 
 args="$args $local $quiet $no_reuse_delta$extra"
-name=$(git-pack-objects --non-empty --all --reflog $args </dev/null "$PACKTMP") ||
+names=$(git-pack-objects --non-empty --all --reflog $args </dev/null "$PACKTMP") ||
 	exit 1
-if [ -z "$name" ]; then
+if [ -z "$names" ]; then
 	echo Nothing new to pack.
-else
+fi
+for name in $names ; do
 	chmod a-w "$PACKTMP-$name.pack"
 	chmod a-w "$PACKTMP-$name.idx"
 	if test "$quiet" != '-q'; then
@@ -92,7 +94,7 @@ else
 		exit 1
 	}
 	rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"
-fi
+done
 
 if test "$remove_redundant" = t
 then
-- 
1.5.2.rc0.787.g0014

^ permalink raw reply related

* Re: git-http-fetch Segmentation fault
From: Luiz Fernando N. Capitulino @ 2007-05-08 20:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, boiko
In-Reply-To: <7vfy672iao.fsf@assigned-by-dhcp.cox.net>

Em Tue, 08 May 2007 13:04:47 -0700
Junio C Hamano <junkio@cox.net> escreveu:

| "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> escreveu:
| 
| >  A friend of mine reported an easy to reproduce segmentation fault
| > when cloning through http from his repository:
| >
| > """
| > ~/ git clone http://people.mandriva.com/~boiko/mandrivamenu.git
| > got 299cdadd846913a052df361e973a947622f23198
| > walk 299cdadd846913a052df361e973a947622f23198
| > ...
| > got 0ecd10d9d6ab020c2469a961777854afda705776
| > /home/lcapitulino/git//bin/git-clone: line 33: 22353 Segmentation fault      (core dumped) git-http-fetch $v -a -w "$tname" "$sha1" "$1"
| > """
| >
| >  Sometimes it shows up as a corrupted double-linked list, detected by
| > glibc:
| 
| Does not seem to reproduce for me on my x86_64 box nor an i386
| box I happened to have access to.  Both run Debian etch.

 Forgot to say that you have to try a few times to reproduce.

 Boiko, can you try it on another distro? NSL maybe? I only run
Mandriva on my machines.

-- 
Luiz Fernando N. Capitulino

^ permalink raw reply

* Re: git-http-fetch Segmentation fault
From: Luiz Fernando N. Capitulino @ 2007-05-08 20:23 UTC (permalink / raw)
  To: Luiz Fernando N. Capitulino; +Cc: Junio C Hamano, Git Mailing List, boiko
In-Reply-To: <20070508171310.7e21f5ef@localhost>

Em Tue, 8 May 2007 17:13:10 -0300
"Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> escreveu:

| Em Tue, 08 May 2007 13:04:47 -0700
| Junio C Hamano <junkio@cox.net> escreveu:
| 
| | "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> escreveu:
| | 
| | >  A friend of mine reported an easy to reproduce segmentation fault
| | > when cloning through http from his repository:
| | >
| | > """
| | > ~/ git clone http://people.mandriva.com/~boiko/mandrivamenu.git
| | > got 299cdadd846913a052df361e973a947622f23198
| | > walk 299cdadd846913a052df361e973a947622f23198
| | > ...
| | > got 0ecd10d9d6ab020c2469a961777854afda705776
| | > /home/lcapitulino/git//bin/git-clone: line 33: 22353 Segmentation fault      (core dumped) git-http-fetch $v -a -w "$tname" "$sha1" "$1"
| | > """
| | >
| | >  Sometimes it shows up as a corrupted double-linked list, detected by
| | > glibc:
| | 
| | Does not seem to reproduce for me on my x86_64 box nor an i386
| | box I happened to have access to.  Both run Debian etch.
| 
|  Forgot to say that you have to try a few times to reproduce.

 And, looks like you need http_proxy set too.

 Sorry for the bad report. :-|

-- 
Luiz Fernando N. Capitulino

^ permalink raw reply

* git-clean fails to remove a file whose name contains \\, ", or \n, TAB, etc.
From: Jim Meyering @ 2007-05-08 20:32 UTC (permalink / raw)
  To: git

Not that it matters (or maybe this is a feature :-), because people
who create such files in their working directory deserve what they
get, Eh? :-)

But if leaving it, then perhaps git-clean should at least warn
that it's not doing its job (i.e. remove the uses of rm's "-f").

To reproduce, run these commands:

nl='
'
git-init > /dev/null && touch "x\\n\"$nl" && git-clean && ls -b

Here's the output I get:

    Removing "x\\n\"\n"
    .git/  x\\n"\n

git-clean.sh needs to strip off leading and trailing double quotes (easy)
as well as convert escapees back to originals (not easy as you might
think, in sh) before running rm.  Good excuse to rewrite it in perl.

^ permalink raw reply

* Re: git-clean fails to remove a file whose name contains \\, ", or  \n, TAB, etc.
From: Randal L. Schwartz @ 2007-05-08 20:51 UTC (permalink / raw)
  To: Jim Meyering; +Cc: git
In-Reply-To: <87ps5bhx8t.fsf@rho.meyering.net>

>>>>> "Jim" == Jim Meyering <jim@meyering.net> writes:

Jim> Not that it matters (or maybe this is a feature :-), because people
Jim> who create such files in their working directory deserve what they
Jim> get, Eh? :-)

The problem is the newline in the string, since

  git-ls-files --others --directory $excl ${excl_info:+"$excl_info"} -- "$@" |
  while read -r file; do

is using newline as a delimiter.  Any file with a newline would mess this up.

Not being a shell programming expert, is there a way we could use -z and xargs
-0 here instead?

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

^ permalink raw reply

* Re: git-clean fails to remove a file whose name contains \\, ", or  \n, TAB, etc.
From: Junio C Hamano @ 2007-05-08 20:53 UTC (permalink / raw)
  To: Randal L. Schwartz; +Cc: Jim Meyering, git
In-Reply-To: <86k5vj9gzu.fsf@blue.stonehenge.com>

merlyn@stonehenge.com (Randal L. Schwartz) writes:

>>>>>> "Jim" == Jim Meyering <jim@meyering.net> writes:
>
> Jim> Not that it matters (or maybe this is a feature :-), because people
> Jim> who create such files in their working directory deserve what they
> Jim> get, Eh? :-)
>
> The problem is the newline in the string, since
>
>   git-ls-files --others --directory $excl ${excl_info:+"$excl_info"} -- "$@" |
>   while read -r file; do
>
> is using newline as a delimiter.  Any file with a newline would mess this up.
>
> Not being a shell programming expert, is there a way we could use -z and xargs
> -0 here instead?

Funny that we had this discussed on this list a few days ago
;-).

^ permalink raw reply

* Re: [PATCH] diff: release blobs after generating textual diff.
From: Junio C Hamano @ 2007-05-08 20:58 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: Nicolas Pitre, git
In-Reply-To: <200705062336.28201.robin.rosenberg.lists@dewire.com>

Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:

> söndag 06 maj 2007 skrev Nicolas Pitre:
>> On Sun, 6 May 2007, Junio C Hamano wrote:
>> 
>> > This reduces the memory pressure when dealing with many paths.
>> > 
>> > An unscientific test of running "diff-tree --stat --summary -M"
>> > between v2.6.19 and v2.6.20-rc1 in the linux kernel repository
>> > indicates that the number of minor faults are reduced by 2/3
>> > (153k vs 49k).
>> > 
>> > Signed-off-by: Junio C Hamano <junkio@cox.net>
>> > ---
>> > 
>> >  * This is still a WIP, not in the sense that it breaks anything
>> >    (it doesn't seem to), but in the sense that it is not known
>> >    if it is useful in general and would make that much of a
>> >    difference with a project much larger than the kernel.
>> 
>> This can only be good.  People are really starting to use Git with 
>> gigantic repos on limited memory hardware.
>
> This did wonders on the usually unreasonable diffs on huge
> repos. The openoffice diff mentioned in the openoffice thread
> went from 6 to ~3 minutes, and most importantly the computer
> was perfectly usable meanwhile. Git memory usage dropped from
> 1,7GB to 400MB.

I've parked a cleaned-up version in 'next'.  Hopefully we can
merge it to 'master' before 1.5.2, as it is not about a new
feature but about fixing a performance 'bug'.

^ permalink raw reply


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