Git development
 help / color / mirror / Atom feed
* Re: GIT on Windows
From: Shawn O. Pearce @ 2007-05-09 18:58 UTC (permalink / raw)
  To: Panagiotis Issaris; +Cc: git
In-Reply-To: <loom.20070509T142518-558@post.gmane.org>

Panagiotis Issaris <takis.issaris@uhasselt.be> wrote:
> Any clues on the reason for this crash? Is anyone already using GIT on Windows
> on real repositories?

Yes, I'm using Git on Cygwin on real repositories.  My largest there
is about 60 MiB, ~70,000 objects, 10,000 files.  Myself and about
25 others who are required to use Windows use it every day just fine.

I also use HTTP fetch to download the latest git.git changes so I
can build them locally (I don't use the Cygwin package, I compile it
myself from scratch) but I haven't done a clone over HTTP in ages as
I already have a local git.git clone established in a few locations.
I'd build any new clone from my existing local ones first...

-- 
Shawn.

^ permalink raw reply

* Re: [PATCH] Add --no-reuse-delta option to git-gc
From: Shawn O. Pearce @ 2007-05-09 19:10 UTC (permalink / raw)
  To: Steven Grimm
  Cc: Junio C Hamano, Daniel Barkalow, Theodore Ts'o,
	Git Mailing List
In-Reply-To: <46418E24.9020309@midwinter.com>

Steven Grimm <koreth@midwinter.com> wrote:
> On that note, has any thought been given to looking at other compression 
> algorithms? Gzip is a great high-speed compressor, but there are others 
> out there (some a bit slower, some much slower at both compression and 
> decompression) that produce substantially smaller output.

Its been discussed once before on the list, in very recent history,
but not by a whole lot.  As Junio pointed out, I don't think there
ever really was any discussion of is gzip the best way to deflate the
objects.  I think gzip was just chosen simply because it was readily
available in libz, stable, and has a pretty decent speed/size ratio.
 
> I think it'd be kind of neat to have my .git directory shrink by another 
> 20+%. That's conservative; on maximumcompression.com's test of a mix of 
> different file types including images, gzip compresses 64% and the 
> best-scoring one does 80%. On English text gzip does 71% and the top 
> scorer does 89%. Most of the top-tier compressors are proprietary, but 

Yes.  But in many cases we might actually be able to do even better
by going with a pack-wide dictionary.  Why?

Think about source code structure.  E.g.

  $ git grep --cached 'struct object'| cut -d: -f1|wc -l
     402

So 402 files in git.git use the term 'struct object', and that's just
the current revision I had in my index.  With our current packfile
organization we are likely to store this string at least 402 times.
We'll store it once in each file's delta chain, assuming each
file's blobs largely fall into a single delta chain for that file
(reasonable assumption, but certainly not always true).

That's just one string that does appear somewhat frequently in any
file its used in.  Now try 'unsigned char' (its 944 files, but an
even higher frequency-per-file).

So anyway, for the past year I've been thinking about trying to
implement a blob-level dictionary prototype to see if it helps on a
project like linux-2.6.git, but I haven't gotten to it.  The pack v4
work was about applying that basic dicationary principal to trees
and commits, and I think it pays off nicely there.  Just need to
get it cleaned up, rebased onto current master, and submitted to
the list for wider testing.  ;-)

-- 
Shawn.

^ permalink raw reply

* [RFC] Second parent for reverts
From: Daniel Barkalow @ 2007-05-09 19:20 UTC (permalink / raw)
  To: git

The discussion about having a header to specify, for a revert commit, what 
it reverts made me realize that this header *would* be useful, but that we 
don't need a *new* header for it. I think that the right method is to add 
the parent of the reverted commit as a second parent for the revert.

If you have:

a -> b -> c -> d

And you want to revert b, the most exact flow would be:

a -> b -> c -> d -> e
       \         /
        -> a' ---

I.e., you exactly remove the effects of b to generate a commit that has 
the same tree as a, and then you merge.

But a' doesn't actually take anything from b, since it's reverting all of 
b (unless it's only reverting part of b), and, if b isn't there, it 
doesn't need a commit message, either, so it's not different from a. So 
the flow should be:

a -> b -> c -> d -> e
  \              /
   --------------

And this means blame work correctly: lines that b changed will be blamed 
on a (or an ancestor), because e will match a there and be different from 
d. So I think git-revert should simply add in the reverted patch's parent. 
Does this analysis make sense to other people?

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply

* Re: [PATCH] checkout: allow full refnames for local branches
From: Junio C Hamano @ 2007-05-09 19:37 UTC (permalink / raw)
  To: Lars Hjemli; +Cc: git
In-Reply-To: <8c5c35580705090207y2979aaa5u7ce9de5fa1dfe658@mail.gmail.com>

"Lars Hjemli" <hjemli@gmail.com> writes:

> On 5/9/07, Junio C Hamano <junkio@cox.net> wrote:
>> Lars Hjemli <hjemli@gmail.com> writes:
>>
>> > This teaches git-checkout to strip the prefix 'refs/heads/' from the
>> > supplied <branch> argument
>>
>> Why is this necessary, may I ask?
>>
>
> I'm playing around with a gui frontend, and there I use
> git-for-each-ref to obtain possible arguments for git-checkout. That's
> how I discovered the 'problem', and solved it by stripping
> 'refs/heads/' in my frontend.

Pathspec-less variant of "git checkout" takes two kinds of
parameters and has two flavours in its behaviour:

 (1) an exact branch name, in which case it switches to the
     branch; otherwise

 (2) any arbitrary commit object name, in whch case it checks
     out and detaches HEAD.

A tricky part is that an exact branch name is often a perfectly
valid commit object name, so rule (1) trumps the rule (2).  You
just discovered a way to have a detached HEAD at a commit that
happens to be at an existing branch -- by naming that commit
without using its exact branch name.

An easier way to spell that would be:

	$ git checkout master^0

but

	master^0
        heads/master
        refs/heads/master

are all perfectly good ways to talk about the commit at the tip
of the 'master' branch without spelling it as an exact
branch name (which is 'master').

^ permalink raw reply

* [PATCH] Add --aggressive option to 'git gc'
From: Theodore Tso @ 2007-05-09 19:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Daniel Barkalow, Git Mailing List
In-Reply-To: <7v3b26xvjo.fsf@assigned-by-dhcp.cox.net>

On Wed, May 09, 2007 at 01:15:07AM -0700, Junio C Hamano wrote:
> > Maybe git-gc should have an option for "compress hard"? It seems to me 
> > like a two-sizes-fit-all solution would be good here; "git gc" for daily 
> > use, and "git gc --squeeze" for when you want to make the result as small 
> > as possible, with compute time not being a major factor.
> 
> I think that sounds saner and more user friendly than specific
> knob to tune "window", "depth" and friends which are too
> technical.  It has an added attraction that we can redefine what
> exactly "hard" means later.

OK, here's a patch that does exactly that.  I choose git-gc
--aggressive, since I thought that was more descriptive than --hard or
--squeeze.  Junio, would you be willing to apply this?

					- Ted

=== Cut here ===

Add --aggressive option to 'git gc'

This option causes 'git gc' to more aggressively optimize the
repository at the cost of taking much more wall clock and CPU time.

Today this option causes git-pack-objects to use --no-use-delta
option, and it allows the --window parameter to be set via the
gc.aggressiveWindow configuration parameter.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 Documentation/config.txt |    5 +++++
 Documentation/git-gc.txt |   16 +++++++++++++++-
 builtin-gc.c             |   35 +++++++++++++++++++++++++++++++++--
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ea434af..efcf301 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -384,6 +384,11 @@ format.suffix::
 	`.patch`. Use this variable to change that suffix (make sure to
 	include the dot if you want it).
 
+gc.aggressiveWindow::
+	The window size parameter used in the delta compression
+	algorithm used by 'git gc --aggressive'.  This defaults
+	to 10.
+
 gc.packrefs::
 	`git gc` does not run `git pack-refs` in a bare repository by
 	default so that older dumb-transport clients can still fetch
diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index bc16584..56575e8 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -8,7 +8,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository
 
 SYNOPSIS
 --------
-'git-gc' [--prune]
+'git-gc' [--prune] [--aggressive]
 
 DESCRIPTION
 -----------
@@ -35,6 +35,13 @@ OPTIONS
 	repository at the same time (e.g. never use this option
 	in a cron script).
 
+--aggressive::
+	Usually 'git-gc' runs very quickly while providing good disk
+	space utilization and performance.   This option will cause
+	git-gc to more aggressive optimize the repository at the expense
+	of taking much more time.  The effects of this optimization are
+	persistent, so this option only needs to be sporadically; every
+	few hundred changesets or so.
 
 Configuration
 -------------
@@ -67,6 +74,13 @@ The optional configuration variable 'gc.packrefs' determines if
 is not run in bare repositories by default, to allow older dumb-transport
 clients fetch from the repository,  but this will change in the future.
 
+The optional configuration variable 'gc.aggressiveWindow' controls how
+much time is spent optimizing the delta compression of the objects in
+the repository when the --aggressive option is specified.  The larger
+the value, the more time is spent optimizing the delta compression.  See
+the documentation for the --window' option in gitlink:git-repack[1] for
+more details.  This defaults to 10.
+
 See Also
 --------
 gitlink:git-prune[1]
diff --git a/builtin-gc.c b/builtin-gc.c
index 3b1f8c2..10f92f1 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -15,13 +15,15 @@
 
 #define FAILED_RUN "failed to run %s"
 
-static const char builtin_gc_usage[] = "git-gc [--prune]";
+static const char builtin_gc_usage[] = "git-gc [--prune] [--aggressive]";
 
 static int pack_refs = -1;
+static int aggressive_window = -1;
 
+#define MAX_ADD 10
 static const char *argv_pack_refs[] = {"pack-refs", "--prune", NULL};
 static const char *argv_reflog[] = {"reflog", "expire", "--all", NULL};
-static const char *argv_repack[] = {"repack", "-a", "-d", "-l", NULL};
+static const char *argv_repack[MAX_ADD] = {"repack", "-a", "-d", "-l", NULL};
 static const char *argv_prune[] = {"prune", NULL};
 static const char *argv_rerere[] = {"rerere", "gc", NULL};
 
@@ -34,13 +36,34 @@ static int gc_config(const char *var, const char *value)
 			pack_refs = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "gc.aggressiveWindow")) {
+		aggressive_window = git_config_int(var, value);
+		printf("aggressive_window = %d\n", aggressive_window);
+		return 0;
+	}
 	return git_default_config(var, value);
 }
 
+static append_option(const char **cmd, const char *opt, int max_length)
+{
+	int	i;
+
+	for (i=0; cmd[i]; i++)
+		;
+
+	if (i+2 >= max_length) {
+		fprintf(stderr, "Too many options specified\n");
+		exit(1);
+	}
+	cmd[i++] = opt;
+	cmd[i] = 0;
+}
+
 int cmd_gc(int argc, const char **argv, const char *prefix)
 {
 	int i;
 	int prune = 0;
+	char buf[80];
 
 	git_config(gc_config);
 
@@ -53,6 +76,14 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			prune = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--aggressive")) {
+			append_option(argv_repack, "-f", MAX_ADD);
+			if (aggressive_window > 0) {
+				sprintf(buf, "--window=%d", aggressive_window);
+				append_option(argv_repack, buf, MAX_ADD);
+			}
+			continue;
+		}
 		/* perhaps other parameters later... */
 		break;
 	}
-- 
1.5.2.rc2.22.ga39d

^ permalink raw reply related

* Re: [PATCH] checkout: allow full refnames for local branches
From: Lars Hjemli @ 2007-05-09 19:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8xbxu6u0.fsf@assigned-by-dhcp.cox.net>

On 5/9/07, Junio C Hamano <junkio@cox.net> wrote:
> "Lars Hjemli" <hjemli@gmail.com> writes:
>
> > On 5/9/07, Junio C Hamano <junkio@cox.net> wrote:
> >> Lars Hjemli <hjemli@gmail.com> writes:
> >>
> >> > This teaches git-checkout to strip the prefix 'refs/heads/' from the
> >> > supplied <branch> argument
> >>
> >> Why is this necessary, may I ask?
> >>
> >
> > I'm playing around with a gui frontend, and there I use
> > git-for-each-ref to obtain possible arguments for git-checkout. That's
> > how I discovered the 'problem', and solved it by stripping
> > 'refs/heads/' in my frontend.
>
> Pathspec-less variant of "git checkout" takes two kinds of
> parameters and has two flavours in its behaviour:
>
>  (1) an exact branch name, in which case it switches to the
>      branch; otherwise
>
>  (2) any arbitrary commit object name, in whch case it checks
>      out and detaches HEAD.
>
> A tricky part is that an exact branch name is often a perfectly
> valid commit object name, so rule (1) trumps the rule (2).  You
> just discovered a way to have a detached HEAD at a commit that
> happens to be at an existing branch -- by naming that commit
> without using its exact branch name.

Ok. But if this is intended behaviour, maybe we would want do change
the detach-message in this case:

[~/src/git] next$ git checkout refs/heads/master
Note: moving to "refs/heads/master" which isn't a local branch

(sorry for stealing your time with this unimportant stuff, it just
surprised me that refs/heads/$branch wasn't treated as a local branch
name)

-- 
larsh

^ permalink raw reply

* Re: [PATCH] checkout: allow full refnames for local branches
From: Lars Hjemli @ 2007-05-09 20:01 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20070509185440.GB3141@spearce.org>

On 5/9/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> Lars Hjemli <hjemli@gmail.com> wrote:
> > I'm playing around with a gui frontend, and there I use
> > git-for-each-ref to obtain possible arguments for git-checkout. That's
> > how I discovered the 'problem', and solved it by stripping
> > 'refs/heads/' in my frontend. But then I thought it would be nice if
> > 'git-checkout' did the stripping on my behalf, since this might bite
> > others too :)
>
> If you are building "porcelain" to sit over Git and offer up a pretty
> view of things, I would encourage you to avoid the stock porcelain.
> Don't use git-checkout, its stock porcelain.  Instead go right to
> the plumbing.  The plumbing doesn't really change behavior as often
> (if ever).

Thanks, I probably will (also to avoid the shell scripts, since my
porcelain is aimed at my co-workers who are stuck on windows)

-- 
larsh

^ permalink raw reply

* Re: quick bare clones taking longer?
From: David Miller @ 2007-05-09 20:06 UTC (permalink / raw)
  To: junkio; +Cc: git
In-Reply-To: <7vvef2t36n.fsf@assigned-by-dhcp.cox.net>

From: Junio C Hamano <junkio@cox.net>
Date: Wed, 09 May 2007 08:41:20 -0700

> There is something very wrong.  "-l -s" should never go to the
> "remote: Generating pack..." codepath.  Is that reproducible?

Every single time on master.kernel.org

> Could you try "sh -x git-clone" it?

Sure:

+ unset CDPATH
+ '[' -n '' ']'
+ quiet=
+ local=no
+ use_local=no
+ local_shared=no
+ unset template
+ no_checkout=
+ upload_pack=
+ bare=
+ reference=
+ origin=
+ origin_override=
+ use_separate_remote=t
+ depth=
+ no_progress=
+ test -t 1
+ case "$#,$1" in
+ bare=yes
+ shift
+ case "$#,$1" in
+ no_checkout=yes
+ shift
+ case "$#,$1" in
+ use_local=yes
+ shift
+ case "$#,$1" in
+ local_shared=yes
+ use_local=yes
+ shift
+ case "$#,$1" in
+ break
+ repo=../torvalds/linux-2.6.git
+ test -n ../torvalds/linux-2.6.git
+ test yes = yes
+ test yes = ''
+ no_checkout=yes
+ use_separate_remote=
+ test -z ''
+ origin=origin
++ get_repo_base ../torvalds/linux-2.6.git
+ base=
+ dir=test-2.6.git
+ '[' -z test-2.6.git ']'
+ '[' -e test-2.6.git ']'
+ mkdir -p test-2.6.git
++ cd test-2.6.git
++ pwd
+ D=/home/davem/git/test-2.6.git
+ trap 'err=$?; cd ..; rm -rf "$D"; exit $err' 0
+ case "$bare" in
+ GIT_DIR=/home/davem/git/test-2.6.git
+ export GIT_DIR
+ git-init
Initialized empty Git repository in /home/davem/git/test-2.6.git/
+ test -n ''
+ rm -f /home/davem/git/test-2.6.git/CLONE_HEAD
+ case "$local,$use_local" in
+ case "$repo" in
+ case "$upload_pack" in
+ git-fetch-pack --all -k ../torvalds/linux-2.6.git
remote: Generating pack...

etc.

Oh, /home/davem/git is a soft symlink to
/pub/scm/linux/kernel/git/davem, maybe that is confusing
git to make it think the repo is not local.

^ permalink raw reply

* Re: [RFC] Second parent for reverts
From: Johannes Schindelin @ 2007-05-09 20:07 UTC (permalink / raw)
  To: Daniel Barkalow; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0705091406350.18541@iabervon.org>

Hi,

On Wed, 9 May 2007, Daniel Barkalow wrote:

> The discussion about having a header to specify, for a revert commit, 
> what it reverts made me realize that this header *would* be useful, but 
> that we don't need a *new* header for it. I think that the right method 
> is to add the parent of the reverted commit as a second parent for the 
> revert.

I am not so sure. In a sense, you are correct. But everybody who does "git 
log --no-merges" would no longer see reverts. Which is somewhat incorrect.

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH] checkout: allow full refnames for local branches
From: Shawn O. Pearce @ 2007-05-09 20:11 UTC (permalink / raw)
  To: Lars Hjemli; +Cc: Junio C Hamano, git
In-Reply-To: <8c5c35580705091301s19dcd2e0q20a4c84e3dd23d82@mail.gmail.com>

Lars Hjemli <hjemli@gmail.com> wrote:
> On 5/9/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> >If you are building "porcelain" to sit over Git and offer up a pretty
> >view of things, I would encourage you to avoid the stock porcelain.
> >Don't use git-checkout, its stock porcelain.  Instead go right to
> >the plumbing.  The plumbing doesn't really change behavior as often
> >(if ever).
> 
> Thanks, I probably will (also to avoid the shell scripts, since my
> porcelain is aimed at my co-workers who are stuck on windows)

Are you building a strictly Win32 native GUI?  Or something else?
Can I ask what sort of features you are going after?  (And if
there's a git repository available, feel free to just point me at
it and ignore my questions.)

I'm just curious.  We seem to have a lot of user interface projects
going on at once right now (Eclipse plugin, git-gui, gitk, qgit, tig,
gitweb, blameview) and everyone's been learning from each other.
I think the competition is good, there's no clear right way to do
things here.  As the primary author of git-gui, I do want to try
and keep current with what the others are up to.  ;-)

-- 
Shawn.

^ permalink raw reply

* Re: [PATCH] Add a birdview-on-the-source-code section to the user manual
From: Johannes Schindelin @ 2007-05-09 20:15 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: Daniel Barkalow, Petr Baudis, kha, junio, git
In-Reply-To: <20070509170725.GB23778@fieldses.org>

Hi,

On Wed, 9 May 2007, J. Bruce Fields wrote:

> On Wed, May 09, 2007 at 06:25:01PM +0200, Johannes Schindelin wrote:
> > None. I only suspected them to be carried out in byte order. From what I 
> > know, there are some shifts involved, which might or might not be helped 
> > by 32-bit arithmetic.
> > 
> > I did not really look into it.
> > 
> > From my prior debugging experiences on Intel, though, I automatically 
> > looked for the least significant bytes at the beginning of those "sha1" 
> > variables, and came up empty.
> 
> So, I'm confused about what you actually mean by "big endian" here.  I
> originally assumed that you meant that SHA1's are defined as bit arrays,
> and that the first bit of the SHA1 is in the high-order bit of the first
> byte.  But if you just meant that the first byte of the SHA1 is stored
> in the first byte of the array...  that kind of goes without saying,
> doesn't it?

Hm.

Let me explain it in this way:

If you parse a number, passed to a program, with strtol(argv[1], NULL, 0) 
you would expect something like this on an Intel processor:

Input 0x1234 -> memory 0x34 0x12 0x00 0x00.

On a big endian machine, you'd expect 0x00 0x00 0x12 0x34.

That is what endianness means.

If you tell Git that it should look for commit e83c6516..., it will store 
the sha1 as 0xe8 0x3c 0x65 0x16 ... in memory, no matter which 
endianness the processor has.

Which was positively confusing for me, since I automatically searched for 
the sequence 0x90 0xf2 0x4a 0x60 ... (which is the tail of that hash).

But if all this sounds too confusing, I agree to delete the 
"(big-endian)".

Ciao,
Dscho

^ permalink raw reply

* Re: svn user trying to recover from brain damage
From: Jan Hudec @ 2007-05-09 20:16 UTC (permalink / raw)
  To: Petr Baudis; +Cc: Joshua Ball, git
In-Reply-To: <20070509162259.GY4489@pasky.or.cz>

[-- Attachment #1: Type: text/plain, Size: 2825 bytes --]

On Wed, May 09, 2007 at 18:22:59 +0200, Petr Baudis wrote:
> > What is a merge? My understanding of merge comes from the SVN book,
> > where it was described as diff+apply. Diff takes 2 arguments, and
> > apply takes a 1 argument (if the patch is implicit). However, cg-merge
> > only appears to take one branch. (There again a use of the word
> > branch! Wouldn't commit or revision be a more accurate term?) Why does
> > cg-merge only take one argument? Even if I use the -b switch, I'm
> > still only up to two arguments. Where is the hidden argument?
> 
>   The hidden argument is your current branch. So cg-merge x will merge
> the branch 'x' to your current branch: symbolically, kind of
> 
> 	base=-b argument | base(HEAD, x)
> 	apply(HEAD, diff(base, x))
> 
>   The word 'branch' is used in an attempt to make it all less confusing
> :-). But in fact, you can give cg-merge just id of a commit, it does not
> have to be branch name.

I believe the important thing to explain here is the BASE, as that is really
the missing argument.

Subversion Book describes merge as diff + apply. Diff takes 2 arguments - OLD
and NEW, and apply takes 2 arguments - TARGET and result of diff. That gives
us 3 arguments in total. 2 of them are passed to merge and the third is
current state of working tree.

Now in git (and in any other version control tool), merge is still diff
+ apply[1]. The TARGET is again implied by working tree. The argument to git
merge is the NEW. So where is the OLD missing?

The answer is simple: It is implied by the history! It is the most recent
common ancestor of the NEW and TARGET, or in other words latest revision that
is predecessor of both revisions being merged.

An important distinction between subversion and git here is, that in git BOTH
NEW and TARGET are considered parents of the new commit created by merge.
This means that repeated merges just work without need to look in the logs or
anywhere for what changes need to be applied.

The OLD, NEW and TARGET argument names are derived from diff/patch
terminology. More common (also in git) is to call them BASE, REMOTE and LOCAL
respectively.

It might be interesting to note, that merging is *symetrical* operation.
Swapping the LOCAL and REMOTE will give the same result, except for order in
which parents are recorded in the commit object and the order in which
conflicted sections are written out in case of conflict.

This is property is not in any way special to git. It is fundamental property
of patches. Git just cares very little about the order.

[1] The 3-way merge algorithm is not diff+apply internally, but is
    equivalent to diff+apply with full context (whole file is kept), except
    for way it marks conflicts.

-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply

* Re: [PATCH] deprecate the new loose object header format
From: Dana How @ 2007-05-09 20:16 UTC (permalink / raw)
  To: Nicolas Pitre; +Cc: Junio C Hamano, Git Mailing List, danahow
In-Reply-To: <alpine.LFD.0.99.0705091422130.24220@xanadu.home>

This doesn't just deprecate the format,
it removes the ability to create it.
So to me this patch goes too far.

Also, if we're interested in "simpler", wouldn't
it be better for loose and in-pack objects to be
the same?  I thought that was the point of
!legacy_headers.  Whatever the decision is,
it certainly has little impact on CPU effort
as you point out.

Maybe I'm too conservative because I'm less
famililar with the code and haven't written it.
Personally I oscillate between adding and refactoring
(you haven't seen any refactoring yet ;-) ).

Thanks,

Dana

On 5/9/07, Nicolas Pitre <nico@cam.org> wrote:
> Now that we encourage and actively preserve objects in a packed form
> more agressively than we did at the time the new loose object format and
> core.legacyheaders were introduced, that extra loose object format
> doesn't appear to be worth it anymore.
>
> Because the packing of loose objects has to go through the delta match
> loop anyway, and since most of them should end up being deltified in
> most cases, there is really little advantage to have this parallel loose
> object format as the CPU savings it might provide is rather lost in the
> noise in the end.
>
> This patch gets rid of core.legacyheaders, preserve the legacy format as
> the only writable loose object format and deprecate the other one to
> keep things simpler.
>
> Signed-off-by: Nicolas Pitre <nico@cam.org>
> ---
>
> On Wed, 9 May 2007, Junio C Hamano wrote:
>
> > I agree with your analysis, especially when deeper delta chains
> > are allowed, straight copy of loose object becomes less and less
> > likely.
>
> So here it is, with a nice code reduction:
>
>  Documentation/config.txt |   13 --------
>  builtin-pack-objects.c   |   69 ----------------------------------------------
>  cache.h                  |    2 -
>  config.c                 |    5 ---
>  environment.c            |    1 -
>  sha1_file.c              |   47 +++++++------------------------
>  6 files changed, 11 insertions(+), 126 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ea434af..d6d89ba 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -209,19 +209,6 @@ core.compression::
>         compression, and 1..9 are various speed/size tradeoffs, 9 being
>         slowest.
>
> -core.legacyheaders::
> -       A boolean which
> -       changes the format of loose objects so that they are more
> -       efficient to pack and to send out of the repository over git
> -       native protocol, since v1.4.2.  However, loose objects
> -       written in the new format cannot be read by git older than
> -       that version; people fetching from your repository using
> -       older versions of git over dumb transports (e.g. http)
> -       will also be affected.
> -+
> -To let git use the new loose object format, you have to
> -set core.legacyheaders to false.
> -
>  core.packedGitWindowSize::
>         Number of bytes of a pack file to map into memory in a
>         single mapping operation.  Larger window sizes may allow
> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index 966f843..c74a361 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -346,56 +346,6 @@ static void copy_pack_data(struct sha1file *f,
>         }
>  }
>
> -static int check_loose_inflate(unsigned char *data, unsigned long len, unsigned long expect)
> -{
> -       z_stream stream;
> -       unsigned char fakebuf[4096];
> -       int st;
> -
> -       memset(&stream, 0, sizeof(stream));
> -       stream.next_in = data;
> -       stream.avail_in = len;
> -       stream.next_out = fakebuf;
> -       stream.avail_out = sizeof(fakebuf);
> -       inflateInit(&stream);
> -
> -       while (1) {
> -               st = inflate(&stream, Z_FINISH);
> -               if (st == Z_STREAM_END || st == Z_OK) {
> -                       st = (stream.total_out == expect &&
> -                             stream.total_in == len) ? 0 : -1;
> -                       break;
> -               }
> -               if (st != Z_BUF_ERROR) {
> -                       st = -1;
> -                       break;
> -               }
> -               stream.next_out = fakebuf;
> -               stream.avail_out = sizeof(fakebuf);
> -       }
> -       inflateEnd(&stream);
> -       return st;
> -}
> -
> -static int revalidate_loose_object(struct object_entry *entry,
> -                                  unsigned char *map,
> -                                  unsigned long mapsize)
> -{
> -       /* we already know this is a loose object with new type header. */
> -       enum object_type type;
> -       unsigned long size, used;
> -
> -       if (pack_to_stdout)
> -               return 0;
> -
> -       used = unpack_object_header_gently(map, mapsize, &type, &size);
> -       if (!used)
> -               return -1;
> -       map += used;
> -       mapsize -= used;
> -       return check_loose_inflate(map, mapsize, size);
> -}
> -
>  static unsigned long write_object(struct sha1file *f,
>                                   struct object_entry *entry)
>  {
> @@ -425,25 +375,6 @@ static unsigned long write_object(struct sha1file *f,
>                                  * and we do not need to deltify it.
>                                  */
>
> -       if (!entry->in_pack && !entry->delta) {
> -               unsigned char *map;
> -               unsigned long mapsize;
> -               map = map_sha1_file(entry->sha1, &mapsize);
> -               if (map && !legacy_loose_object(map)) {
> -                       /* We can copy straight into the pack file */
> -                       if (revalidate_loose_object(entry, map, mapsize))
> -                               die("corrupt loose object %s",
> -                                   sha1_to_hex(entry->sha1));
> -                       sha1write(f, map, mapsize);
> -                       munmap(map, mapsize);
> -                       written++;
> -                       reused++;
> -                       return mapsize;
> -               }
> -               if (map)
> -                       munmap(map, mapsize);
> -       }
> -
>         if (!to_reuse) {
>                 buf = read_sha1_file(entry->sha1, &type, &size);
>                 if (!buf)
> diff --git a/cache.h b/cache.h
> index 8e76152..5725bce 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -273,7 +273,6 @@ extern void rollback_lock_file(struct lock_file *);
>  extern int delete_ref(const char *, const unsigned char *sha1);
>
>  /* Environment bits from configuration mechanism */
> -extern int use_legacy_headers;
>  extern int trust_executable_bit;
>  extern int has_symlinks;
>  extern int assume_unchanged;
> @@ -354,7 +353,6 @@ extern int move_temp_to_file(const char *tmpfile, const char *filename);
>  extern int has_sha1_pack(const unsigned char *sha1, const char **ignore);
>  extern int has_sha1_file(const unsigned char *sha1);
>  extern void *map_sha1_file(const unsigned char *sha1, unsigned long *);
> -extern int legacy_loose_object(unsigned char *);
>
>  extern int has_pack_file(const unsigned char *sha1);
>  extern int has_pack_index(const unsigned char *sha1);
> diff --git a/config.c b/config.c
> index 70d1055..298966f 100644
> --- a/config.c
> +++ b/config.c
> @@ -299,11 +299,6 @@ int git_default_config(const char *var, const char *value)
>                 return 0;
>         }
>
> -       if (!strcmp(var, "core.legacyheaders")) {
> -               use_legacy_headers = git_config_bool(var, value);
> -               return 0;
> -       }
> -
>         if (!strcmp(var, "core.compression")) {
>                 int level = git_config_int(var, value);
>                 if (level == -1)
> diff --git a/environment.c b/environment.c
> index 2231659..54e3aba 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -11,7 +11,6 @@
>
>  char git_default_email[MAX_GITNAME];
>  char git_default_name[MAX_GITNAME];
> -int use_legacy_headers = 1;
>  int trust_executable_bit = 1;
>  int has_symlinks = 1;
>  int assume_unchanged;
> diff --git a/sha1_file.c b/sha1_file.c
> index 32244d7..e715527 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -972,7 +972,7 @@ void *map_sha1_file(const unsigned char *sha1, unsigned long *size)
>         return map;
>  }
>
> -int legacy_loose_object(unsigned char *map)
> +static int legacy_loose_object(unsigned char *map)
>  {
>         unsigned int word;
>
> @@ -1034,6 +1034,14 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon
>                 return inflate(stream, 0);
>         }
>
> +
> +       /*
> +        * There used to be a second loose object header format which
> +        * was meant to mimic the in-pack format, allowing for direct
> +        * copy of the object data.  This format turned up not to be
> +        * really worth it and we don't write it any longer.  But we
> +        * can still read it.
> +        */
>         used = unpack_object_header_gently(map, mapsize, &type, &size);
>         if (!used || !valid_loose_object_type[type])
>                 return -1;
> @@ -1962,40 +1970,6 @@ static int write_buffer(int fd, const void *buf, size_t len)
>         return 0;
>  }
>
> -static int write_binary_header(unsigned char *hdr, enum object_type type, unsigned long len)
> -{
> -       int hdr_len;
> -       unsigned char c;
> -
> -       c = (type << 4) | (len & 15);
> -       len >>= 4;
> -       hdr_len = 1;
> -       while (len) {
> -               *hdr++ = c | 0x80;
> -               hdr_len++;
> -               c = (len & 0x7f);
> -               len >>= 7;
> -       }
> -       *hdr = c;
> -       return hdr_len;
> -}
> -
> -static void setup_object_header(z_stream *stream, const char *type, unsigned long len)
> -{
> -       int obj_type, hdrlen;
> -
> -       if (use_legacy_headers) {
> -               while (deflate(stream, 0) == Z_OK)
> -                       /* nothing */;
> -               return;
> -       }
> -       obj_type = type_from_string(type);
> -       hdrlen = write_binary_header(stream->next_out, obj_type, len);
> -       stream->total_out = hdrlen;
> -       stream->next_out += hdrlen;
> -       stream->avail_out -= hdrlen;
> -}
> -
>  int hash_sha1_file(const void *buf, unsigned long len, const char *type,
>                     unsigned char *sha1)
>  {
> @@ -2062,7 +2036,8 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha
>         /* First header.. */
>         stream.next_in = (unsigned char *)hdr;
>         stream.avail_in = hdrlen;
> -       setup_object_header(&stream, type, len);
> +       while (deflate(&stream, 0) == Z_OK)
> +               /* nothing */;
>
>         /* Then the data itself.. */
>         stream.next_in = buf;
>


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

^ permalink raw reply

* Re: [PATCH] Add --aggressive option to 'git gc'
From: Junio C Hamano @ 2007-05-09 20:19 UTC (permalink / raw)
  To: Theodore Tso; +Cc: Daniel Barkalow, Git Mailing List
In-Reply-To: <20070509194839.GB10280@thunk.org>

Theodore Tso <tytso@mit.edu> writes:

> On Wed, May 09, 2007 at 01:15:07AM -0700, Junio C Hamano wrote:
>> > Maybe git-gc should have an option for "compress hard"? It seems to me 
>> > like a two-sizes-fit-all solution would be good here; "git gc" for daily 
>> > use, and "git gc --squeeze" for when you want to make the result as small 
>> > as possible, with compute time not being a major factor.
>> 
>> I think that sounds saner and more user friendly than specific
>> knob to tune "window", "depth" and friends which are too
>> technical.  It has an added attraction that we can redefine what
>> exactly "hard" means later.
>
> OK, here's a patch that does exactly that.  I choose git-gc
> --aggressive, since I thought that was more descriptive than --hard or
> --squeeze.  Junio, would you be willing to apply this?

Willing?  Yes.

It's tricky that it defaults to 10 and still called aggressive.
When the configuration variable is left unspecified, the only
reason it is called aggressive is because it passes '-f' to
repack, right?  It was not very clear at the first sight and I
was about to ask why the default is 10, not higher.

^ permalink raw reply

* Re: [RFC] Second parent for reverts
From: Shawn O. Pearce @ 2007-05-09 20:22 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Daniel Barkalow, git
In-Reply-To: <Pine.LNX.4.64.0705092206540.4167@racer.site>

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Wed, 9 May 2007, Daniel Barkalow wrote:
> 
> > The discussion about having a header to specify, for a revert commit, 
> > what it reverts made me realize that this header *would* be useful, but 
> > that we don't need a *new* header for it. I think that the right method 
> > is to add the parent of the reverted commit as a second parent for the 
> > revert.
> 
> I am not so sure. In a sense, you are correct. But everybody who does "git 
> log --no-merges" would no longer see reverts. Which is somewhat incorrect.

Right.

I've actually done what Daniel just talked about doing in one of my
"production" repositories.  I did it by hand as a developer had
created a bad merge and accidentially reverted 800 files during
that merge.  80 or so commits later along a public non-rewinding
branch coworkers realized things weren't right, and asked me
to fix the mess.  As I wanted to save the blame data when I
reverted-the-revert I did what Daniel suggests.

But since the revert-the-revert wasn't really an interesting point
in history, and neither was the bad merge, I don't really care that
neither shows up with --no-merges.  The original bad merge was a
simple honest mistake made by a developer who was new to Git, and
was only caused because merge-recursive wasn't installed properly
on that system.


As Dscho says, most reverts are interesting points in time.  *Why*
a particular revert was done is important.

And so I have to disagree quite a bit with Daniel's idea, for exactly
that reason.  If I'm looking at a block of code in a file I want to
know why its there.  If blame tells me its a revert of something,
that tells me we tried another path and it didn't work out.  I might
be sitting here looking at this line because I'm thinking of redoing
whatever it was that wasn't good!

So that revert commit message better say why that thing didn't
work out.

If I really do care about the source of that line, I can always
re-run blame on the parent of the reverted commit (hence why ^
is so nice as a suffix on a commit-ish!) and examine the line again.

Hmm.  I should teach git-gui to parse out the revert message and
let you click into its parent.  Simple enough.  Maybe it will be
in 0.7.0.  Maybe it won't be.  ;-)

-- 
Shawn.

^ permalink raw reply

* Re: failing test t9400 (Re: [PATCH] git-update-ref: add --no-deref option for overwriting/detaching ref)
From: Frank Lichtenheld @ 2007-05-09 20:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: skimo, git
In-Reply-To: <7vr6pqt1fu.fsf@assigned-by-dhcp.cox.net>

On Wed, May 09, 2007 at 09:19:01AM -0700, Junio C Hamano wrote:
> Sven Verdoolaege <skimo@kotnet.org> writes:
> > Shouldn't these tests be skipped if I don't have all that stuff installed?
> > There doesn't even seem to be an option to turn off these tests.
> 
> I agree.  We would need something like this, but I have no easy
> way to test it myself, short of uninstalling what I need on the
> box.  As you do not have them, maybe you can give it a quick
> whirl?

Yeah, I totally forgot add such a check, even though I added one for
cvs.

> ---
> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
> index f17be6b..98d6bb4 100755
> --- a/t/t9400-git-cvsserver-server.sh
> +++ b/t/t9400-git-cvsserver-server.sh
> @@ -17,6 +17,11 @@ then
>      test_done
>      exit
>  fi
> +perl -e 'use DBI; use DBD::SQLite' 2>&1 || {

Maybe there is a >/dev/null missing here?

I personally would prefer "perl -MDBI -MDBD::SQLite" stylewise, but I
guess it makes no difference in the end.

> +    test_expect_success 'skipping git-cvsserver tests, Perl SQLite interface unavailable' :
> +    test_done
> +    exit
> +}

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

^ permalink raw reply

* Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.
From: Robin Rosenberg @ 2007-05-09 20:30 UTC (permalink / raw)
  To: Steffen Prohaska; +Cc: git
In-Reply-To: <0056A63A-D511-4FDD-82A6-A13B06E237E9@zib.de>

onsdag 09 maj 2007 skrev Steffen Prohaska:
> The old implementation executed 'cvs status' for each file touched by  
> the patch
> to be applied. The new code calls 'cvs status' only once and parses  
> cvs's
> output to collect status information of all files contained in the  
> cvs working
> copy.
> 
> Runtime is now independent of the number of modified files. A  
> drawback is that
> the new code retrieves status information for all files even if only  
> a few are
> touched. The old implementation may be noticeably faster for small  
> patches to

Ouch, lets see now. My working cvs checkout contains ~25k files and
my typical commit touches 5-20 files. 

A quick (well....) test says cvs status on my checkout takes about
five minutes to execute. Compare this with my typical exportcommit
time of about ten seconds. 

If you really need this, make a switch to select it.

Still we're missing a check for the case that new files/directories have been
added on the server, but are missing from the checkout, or why not run
an update first. If you are commit this number of large files you'll need that
check, or it's hurt a lot when things fail.

> large workingcopies. However, the old implementation doesn't scale if  
> more
> files are touched, especially in remotely located cvs repositories.

How come your commit are so large you'd prefer this behaviour?

-- robin

^ permalink raw reply

* Re: [PATCH] Add a birdview-on-the-source-code section to the user manual
From: J. Bruce Fields @ 2007-05-09 20:32 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Daniel Barkalow, Petr Baudis, kha, junio, git
In-Reply-To: <Pine.LNX.4.64.0705092208230.4167@racer.site>

On Wed, May 09, 2007 at 10:15:19PM +0200, Johannes Schindelin wrote:
> If you parse a number, passed to a program, with strtol(argv[1], NULL, 0) 
> you would expect something like this on an Intel processor:
> 
> Input 0x1234 -> memory 0x34 0x12 0x00 0x00.

Right, but this is something special to integers.  If it made sense for
some strange reason to define the structure carrying a sha1 as int[5]
instead of char[20] then I'd understand the confusion, but char[20] is
totally unambiguous.

> But if all this sounds too confusing, I agree to delete the 
> "(big-endian)".

Yeah, I think that'd be best; thanks.

--b.

^ permalink raw reply

* Re: [PATCH] checkout: allow full refnames for local branches
From: Lars Hjemli @ 2007-05-09 20:38 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20070509201105.GF3141@spearce.org>

On 5/9/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> Lars Hjemli <hjemli@gmail.com> wrote:
> > On 5/9/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> > >If you are building "porcelain" to sit over Git and offer up a pretty
> > >view of things, I would encourage you to avoid the stock porcelain.
> > >Don't use git-checkout, its stock porcelain.  Instead go right to
> > >the plumbing.  The plumbing doesn't really change behavior as often
> > >(if ever).
> >
> > Thanks, I probably will (also to avoid the shell scripts, since my
> > porcelain is aimed at my co-workers who are stuck on windows)
>
> Are you building a strictly Win32 native GUI?  Or something else?

It's mono/.net, so I can test it on my linux box and push the binary
directly to the poor souls on windows :)

> Can I ask what sort of features you are going after?  (And if
> there's a git repository available, feel free to just point me at
> it and ignore my questions.)

The features I'm focusing on are mostly trivial day-to-day operations
of your average coder: status, diff, commit, push, fetch, merge,
checkout, log. This should be enough to support our (planned) workflow
of one public repo per developer + a shared integration repo with
restricted push access + active use of topic-branches.

We currently use subversion, so real branches + real merges are killer
arguments for a switch to git. But we also use tortoisesvn, and the
"simplicity" of the gui must be met by some tool. Hence me playing
around....

If/when it becomes useful, I'll put it up on http://hjemli.net/git/


> I'm just curious.  We seem to have a lot of user interface projects
> going on at once right now (Eclipse plugin, git-gui, gitk, qgit, tig,
> gitweb, blameview) and everyone's been learning from each other.

Heh, I actually considered calling it yagg (but it doesn't deserve a name yet)


> I think the competition is good, there's no clear right way to do
> things here.  As the primary author of git-gui, I do want to try
> and keep current with what the others are up to.  ;-)

Absolutely, nice work btw.


-- 
larsh

^ permalink raw reply

* Re: [PATCH] deprecate the new loose object header format
From: Nicolas Pitre @ 2007-05-09 20:42 UTC (permalink / raw)
  To: Dana How; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <56b7f5510705091316t78e31032k95815e190239717e@mail.gmail.com>

On Wed, 9 May 2007, Dana How wrote:

> This doesn't just deprecate the format,
> it removes the ability to create it.
> So to me this patch goes too far.

Well, I disagree.  We cannot create it anymore, but we still can read 
it.  Older Git versions nay not even read it.  And since this format was 
off by default anyway, I doubt you've lost anything.

> Also, if we're interested in "simpler", wouldn't
> it be better for loose and in-pack objects to be
> the same?  I thought that was the point of
> !legacy_headers.

It is not simpler because:

 1) backward compatibility requires the legacy format, and

 2) the object SHA1 is always computed with the legacy header included.

So what is simpler is really to get rid of over 100 lines of code that 
didn't provide a real benefit.  The faster we remove the ability to 
write such objects the fewer they'll be in the field.


Nicolas

^ permalink raw reply

* Re: [PATCH] Add a birdview-on-the-source-code section to the user manual
From: Daniel Barkalow @ 2007-05-09 20:45 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: J. Bruce Fields, Petr Baudis, kha, junio, git
In-Reply-To: <Pine.LNX.4.64.0705092208230.4167@racer.site>

On Wed, 9 May 2007, Johannes Schindelin wrote:

> Let me explain it in this way:
> 
> If you parse a number, passed to a program, with strtol(argv[1], NULL, 0) 
> you would expect something like this on an Intel processor:
> 
> Input 0x1234 -> memory 0x34 0x12 0x00 0x00.
> 
> On a big endian machine, you'd expect 0x00 0x00 0x12 0x34.
> 
> That is what endianness means.
> 
> If you tell Git that it should look for commit e83c6516..., it will store 
> the sha1 as 0xe8 0x3c 0x65 0x16 ... in memory, no matter which 
> endianness the processor has.

But it would be really weird to get 0x90 0xf2 0x4a 0x60 ... 0x16 0x65 0x3c 
0xe8 unless you've got a 160-bit little-endian processor. That would be as 
strange as having "Test" stored as 0x74 0x73 0x65 0x54, I think.

> Which was positively confusing for me, since I automatically searched for 
> the sequence 0x90 0xf2 0x4a 0x60 ... (which is the tail of that hash).
> 
> But if all this sounds too confusing, I agree to delete the 
> "(big-endian)".

If it confused you, there should be something there. Maybe "(in order)" or 
something else implying that the underlying type is an octet sequence, 
rather than a 160-bit integer?

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply

* [PATCH v3] Custom compression levels for objects and packs
From: Dana How @ 2007-05-09 20:56 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, danahow


Add config variables pack.compression and core.loosecompression ,
and switch --compression=level to pack-objects.

Loose objects will be compressed using core.loosecompression if set,
else core.compression if set, else Z_BEST_SPEED.
Packed objects will be compressed using --compression=level if seen,
else pack.compression if set, else core.compression if set,
else Z_DEFAULT_COMPRESSION.  This is the "pack compression level".

Loose objects added to a pack undeltified will be recompressed
to the pack compression level if it is unequal to the current
loose compression level by the preceding rules,  or if the loose
object was written while core.legacyheaders = true.  Newly
deltified loose objects are always compressed to the current
pack compression level.

Previously packed objects added to a pack are recompressed
to the current pack compression level exactly when their
deltification status changes,  since the previous pack data
cannot be reused.

In either case,  the --no-reuse-object switch from the first
patch below will always force recompression to the current pack
compression level,  instead of assuming the pack compression level
hasn't changed and pack data can be reused when possible.

This applies on top of the following patches from Nicolas Pitre:
[PATCH] allow for undeltified objects not to be reused
[PATCH] make "repack -f" imply "pack-objects --no-reuse-object"

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 Documentation/config.txt           |   17 +++++++++++++++--
 Documentation/git-pack-objects.txt |   14 +++++++++++++-
 builtin-pack-objects.c             |   33 +++++++++++++++++++++++++++++++--
 cache.h                            |    2 ++
 config.c                           |   18 +++++++++++++++++-
 csum-file.c                        |    4 ++--
 csum-file.h                        |    2 +-
 environment.c                      |    4 +++-
 8 files changed, 84 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ea434af..382a31b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -204,10 +204,16 @@ core.warnAmbiguousRefs::
 	and might match multiple refs in the .git/refs/ tree. True by default.
 
 core.compression::
+	An integer -1..9, indicating a default compression level.
+	-1 is the zlib default. 0 means no compression,
+	and 1..9 are various speed/size tradeoffs, 9 being slowest.
+
+core.loosecompression::
 	An integer -1..9, indicating the compression level for objects that
-	are not in a pack file. -1 is the zlib and git default. 0 means no
+	are not in a pack file. -1 is the zlib default. 0 means no
 	compression, and 1..9 are various speed/size tradeoffs, 9 being
-	slowest.
+	slowest.  If not set,  defaults to core.compression.  If that is
+	not set,  defaults to 0 (best speed).
 
 core.legacyheaders::
 	A boolean which
@@ -550,6 +556,13 @@ pack.depth::
 	The maximum delta depth used by gitlink:git-pack-objects[1] when no
 	maximum depth is given on the command line. Defaults to 50.
 
+pack.compression::
+	An integer -1..9, indicating the compression level for objects
+	in a pack file. -1 is the zlib default. 0 means no
+	compression, and 1..9 are various speed/size tradeoffs, 9 being
+	slowest.  If not set,  defaults to core.compression.  If that is
+	not set,  defaults to -1.
+
 pull.octopus::
 	The default merge strategy to use when pulling multiple branches
 	at once.
diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index ce89214..2531238 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -130,10 +130,22 @@ base-name::
 --no-reuse-object::
 	This flag tells the command not to reuse existing object data at all,
 	including non deltified object, forcing recompression of everything.
-	This implies --no-reuse-delta. Useful only in the obscur case where
+	This implies --no-reuse-delta. Useful only in the obscure case where
 	wholesale enforcement of a different compression level on the
 	packed data is desired.
 
+--compression=[N]::
+	Specifies compression level for newly-compressed data in the
+	generated pack.  If not specified,  pack compression level is
+	determined first by pack.compression,  then by core.compression,
+	and defaults to -1,  the zlib default,  if neither is set.
+	Data copied from loose objects will be recompressed
+	if core.legacyheaders was true when they were created or if
+	the loose compression level (see core.loosecompression and
+	core.compression) is now a different value than the pack
+	compression level.  Add --no-reuse-object if you want to force
+	a uniform compression level on all data no matter the source.
+
 --delta-base-offset::
 	A packed archive can express base object of a delta as
 	either 20-byte object name or as an offset in the
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index d94c79a..5c468aa 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -68,6 +68,8 @@ static int depth = 50;
 static int pack_to_stdout;
 static int num_preferred_base;
 static struct progress progress_state;
+static int pack_compression_level = Z_DEFAULT_COMPRESSION;
+static int pack_compression_seen;
 
 /*
  * The object names in objects array are hashed with this hashtable,
@@ -427,7 +429,10 @@ static unsigned long write_object(struct sha1file *f,
 				 * and we do not need to deltify it.
 				 */
 
-	if (!no_reuse_object && !entry->in_pack && !entry->delta) {
+	/* differing core & pack compression when loose object -> must recompress */
+	if (!entry->in_pack && pack_compression_level != zlib_compression_level)
+		to_reuse = 0;
+	else if (!no_reuse_object && !entry->in_pack && !entry->delta) {
 		unsigned char *map;
 		unsigned long mapsize;
 		map = map_sha1_file(entry->sha1, &mapsize);
@@ -487,7 +492,7 @@ static unsigned long write_object(struct sha1file *f,
 			sha1write(f, entry->delta->sha1, 20);
 			hdrlen += 20;
 		}
-		datalen = sha1write_compressed(f, buf, size);
+		datalen = sha1write_compressed(f, buf, size, pack_compression_level);
 		free(buf);
 	}
 	else {
@@ -1496,6 +1501,16 @@ static int git_pack_config(const char *k, const char *v)
 		depth = git_config_int(k, v);
 		return 0;
 	}
+	if (!strcmp(k, "pack.compression")) {
+		int level = git_config_int(k, v);
+		if (level == -1)
+			level = Z_DEFAULT_COMPRESSION;
+		else if (level < 0 || level > Z_BEST_COMPRESSION)
+			die("bad pack compression level %d", level);
+		pack_compression_level = level;
+		pack_compression_seen = 1;
+		return 0;
+	}
 	return git_default_config(k, v);
 }
 
@@ -1607,6 +1622,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	rp_ac = 2;
 
 	git_config(git_pack_config);
+	if (!pack_compression_seen && core_compression_seen)
+		pack_compression_level = core_compression_level;
 
 	progress = isatty(2);
 	for (i = 1; i < argc; i++) {
@@ -1627,6 +1644,18 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			incremental = 1;
 			continue;
 		}
+		if (!prefixcmp(arg, "--compression=")) {
+			char *end;
+			int level = strtoul(arg+14, &end, 0);
+			if (!arg[14] || *end)
+				usage(pack_usage);
+			if (level == -1)
+				level = Z_DEFAULT_COMPRESSION;
+			else if (level < 0 || level > Z_BEST_COMPRESSION)
+				die("bad pack compression level %d", level);
+			pack_compression_level = level;
+			continue;
+		}
 		if (!prefixcmp(arg, "--window=")) {
 			char *end;
 			window = strtoul(arg+9, &end, 0);
diff --git a/cache.h b/cache.h
index 8e76152..2b3f359 100644
--- a/cache.h
+++ b/cache.h
@@ -283,6 +283,8 @@ extern int warn_ambiguous_refs;
 extern int shared_repository;
 extern const char *apply_default_whitespace;
 extern int zlib_compression_level;
+extern int core_compression_level;
+extern int core_compression_seen;
 extern size_t packed_git_window_size;
 extern size_t packed_git_limit;
 extern size_t delta_base_cache_limit;
diff --git a/config.c b/config.c
index 70d1055..5627ed6 100644
--- a/config.c
+++ b/config.c
@@ -12,6 +12,8 @@
 static FILE *config_file;
 static const char *config_file_name;
 static int config_linenr;
+static int zlib_compression_seen;
+
 static int get_next_char(void)
 {
 	int c;
@@ -304,13 +306,27 @@ int git_default_config(const char *var, const char *value)
 		return 0;
 	}
 
-	if (!strcmp(var, "core.compression")) {
+	if (!strcmp(var, "core.loosecompression")) {
 		int level = git_config_int(var, value);
 		if (level == -1)
 			level = Z_DEFAULT_COMPRESSION;
 		else if (level < 0 || level > Z_BEST_COMPRESSION)
 			die("bad zlib compression level %d", level);
 		zlib_compression_level = level;
+		zlib_compression_seen = 1;
+		return 0;
+	}
+
+	if (!strcmp(var, "core.compression")) {
+		int level = git_config_int(var, value);
+		if (level == -1)
+			level = Z_DEFAULT_COMPRESSION;
+		else if (level < 0 || level > Z_BEST_COMPRESSION)
+			die("bad zlib compression level %d", level);
+		core_compression_level = level;
+		core_compression_seen = 1;
+		if (!zlib_compression_seen)
+			zlib_compression_level = level;
 		return 0;
 	}
 
diff --git a/csum-file.c b/csum-file.c
index 7c806ad..7088f6e 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -119,14 +119,14 @@ struct sha1file *sha1fd(int fd, const char *name)
 	return f;
 }
 
-int sha1write_compressed(struct sha1file *f, void *in, unsigned int size)
+int sha1write_compressed(struct sha1file *f, void *in, unsigned int size, int level)
 {
 	z_stream stream;
 	unsigned long maxsize;
 	void *out;
 
 	memset(&stream, 0, sizeof(stream));
-	deflateInit(&stream, zlib_compression_level);
+	deflateInit(&stream, level);
 	maxsize = deflateBound(&stream, size);
 	out = xmalloc(maxsize);
 
diff --git a/csum-file.h b/csum-file.h
index 7e13391..4e8b83e 100644
--- a/csum-file.h
+++ b/csum-file.h
@@ -16,7 +16,7 @@ extern struct sha1file *sha1fd(int fd, const char *name);
 extern struct sha1file *sha1create(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
 extern int sha1close(struct sha1file *, unsigned char *, int);
 extern int sha1write(struct sha1file *, void *, unsigned int);
-extern int sha1write_compressed(struct sha1file *, void *, unsigned int);
+extern int sha1write_compressed(struct sha1file *, void *, unsigned int, int);
 extern void crc32_begin(struct sha1file *);
 extern uint32_t crc32_end(struct sha1file *);
 
diff --git a/environment.c b/environment.c
index 2231659..b7aeb1a 100644
--- a/environment.c
+++ b/environment.c
@@ -24,7 +24,9 @@ const char *git_commit_encoding;
 const char *git_log_output_encoding;
 int shared_repository = PERM_UMASK;
 const char *apply_default_whitespace;
-int zlib_compression_level = Z_DEFAULT_COMPRESSION;
+int zlib_compression_level = Z_BEST_SPEED;
+int core_compression_level;
+int core_compression_seen;
 size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
 size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
 size_t delta_base_cache_limit = 16 * 1024 * 1024;
-- 
1.5.2.rc0.787.g0014

^ permalink raw reply related

* Re: [PATCH] deprecate the new loose object header format
From: Dana How @ 2007-05-09 21:00 UTC (permalink / raw)
  To: Nicolas Pitre; +Cc: Junio C Hamano, Git Mailing List, danahow
In-Reply-To: <alpine.LFD.0.99.0705091633590.24220@xanadu.home>

On 5/9/07, Nicolas Pitre <nico@cam.org> wrote:
> It is not simpler because:
>  2) the object SHA1 is always computed with the legacy header included.
That convinces me.

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

^ permalink raw reply

* Re: FFmpeg considering GIT
From: Fredrik Kuivinen @ 2007-05-09 21:09 UTC (permalink / raw)
  To: Jan Hudec
  Cc: Marco Costalba, Paul Mackerras, Alex Riesen, Linus Torvalds,
	Karl Hasselstr?m, Junio C Hamano, Carl Worth, Michael Niedermayer,
	Git Mailing List
In-Reply-To: <20070509182844.GA2982@efreet.light.src>

On 5/9/07, Jan Hudec <bulb@ucw.cz> wrote:
> On Wed, May 09, 2007 at 08:38:48 +0200, Marco Costalba wrote:
> > On 5/9/07, Paul Mackerras <paulus@samba.org> wrote:
> > >Marco Costalba writes:
> > >
> > >> Language to use is C++, not C (much more powerful IMHO)
> > >
> > >Sorry, C++ is not an option because I dislike it so much.
> >
> > Well, speaking about GUI applications, the 90% is in the graphic
> > library and only in small part in the language. With Qt we are at 95%
> >
> > Anyhow does exist also python bindings for Qt.
>
> Tried them, beed deeply disapointed. Qt always destroys all child objects
> with the parent, which is OK in C++, but does not play well with
> garbage-collection. And the python bindings (ruby ones seem to be better)
> fail to check reference validity, so you can quite easily segfault the python
> interpreter. Gtk plays much better with dynamic languages.

I have used PyQt for some smaller projects (notably Hgct, a no longer developed
commit tool for git and Mercurial. See
http://repo.or.cz/w/hgct.git?a=tree). For me
PyQt has worked very well. The python interface to Qt is more or less a direct
translation of the C++ interface, so the excellent documentation troll
tech provides
for Qt can be used when developing with PyQt as well.

I have never seen the segfaulting you mention. Maybe my programs have been too
small to trigger that bug...

- Fredrik

^ permalink raw reply

* Re: FFmpeg considering GIT
From: Jan Hudec @ 2007-05-09 21:36 UTC (permalink / raw)
  To: Fredrik Kuivinen
  Cc: Marco Costalba, Paul Mackerras, Alex Riesen, Linus Torvalds,
	Karl Hasselstr?m, Junio C Hamano, Carl Worth, Michael Niedermayer,
	Git Mailing List
In-Reply-To: <4c8ef70705091409g30674cb6p6d3af42eb47ffc08@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

On Wed, May 09, 2007 at 23:09:25 +0200, Fredrik Kuivinen wrote:
> I have used PyQt for some smaller projects (notably Hgct, a no longer 
> developed
> commit tool for git and Mercurial. See
> http://repo.or.cz/w/hgct.git?a=tree). For me
> PyQt has worked very well. The python interface to Qt is more or less a 
> direct
> translation of the C++ interface, so the excellent documentation troll
> tech provides
> for Qt can be used when developing with PyQt as well.
> 
> I have never seen the segfaulting you mention. Maybe my programs have been 
> too
> small to trigger that bug...

It's not about size of the programs. It's about having to be careful not to
refer to widgets inside eg. dialog box from outside and close that dialog
box. That is having to be careful about something, that is normal in C++, but
what you normally expect python to handle for you. And such quirks of the
bindings are completely undocumented.

-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ 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