a bunch of outstanding updates

a bunch of outstanding updates

[Sam Vilain]

Following up to this e-mail are a whole load of outstanding feature
requests of mine.

These changes are relatively mundane:

    * repack: improve documentation on -a option
    * git-remote: document -n
    * git-remote: allow 'git-remote fetch' as a synonym for 'git fetch'
    * git-svn: use git-log rather than rev-list | xargs cat-file
    * git-svn: cache max revision in rev_db databases

This one will impact on the version displayed by "git --version", but
I think this is for the better:

    * GIT-VERSION-GEN: don't convert - delimiter to .'s

These ones are really only very minor updates based on feedback so
far:

    * git-merge-ff: fast-forward only merge
    * git-mergetool: add support for ediff

This one is just the previously posted hook script put into the
templates directory, let me know if you'd rather I reshaped it to go
into contrib/hooks:

    * contrib/hooks: add post-update hook for updating working copy

This one probably needs a bit more consideration and review, could
perhaps sit on pu.

    * git-repack: generational repacking (and example hook script)

[PATCH] repack: improve documentation on -a option

[Sam Vilain]

Some minor enhancements to the git-repack manual page.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 Documentation/git-repack.txt |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index c33a512..be8e5f8 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -14,7 +14,8 @@ DESCRIPTION
 -----------
 
 This script is used to combine all objects that do not currently
-reside in a "pack", into a pack.
+reside in a "pack", into a pack.  It can also be used to re-organise
+existing packs into a single, more efficient pack.
 
 A pack is a collection of objects, individually compressed, with
 delta compression applied, stored in a single file, with an
@@ -28,11 +29,13 @@ OPTIONS
 
 -a::
 	Instead of incrementally packing the unpacked objects,
-	pack everything available into a single pack.
+	pack everything referenced into a single pack.
 	Especially useful when packing a repository that is used
-	for private development and there is no need to worry
-	about people fetching via dumb file transfer protocols
-	from it.  Use with '-d'.
+	for private development and there no need to worry
+	about people fetching via dumb protocols from it.  Use
+	with '-d'.  This will clean up the objects that `git prune`
+	leaves behind, but `git fsck-objects --full` shows as
+	dangling.
 
 -d::
 	After packing, if the newly created packs make some
-- 
1.5.2.1.1131.g3b90

[PATCH] git-svn: use git-log rather than rev-list | xargs cat-file

[Sam Vilain]

From: Sam Vilain <sam@vilain.net>

This saves a bit of time when rebuilding the git-svn index.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 git-svn.perl |   36 ++++++++++++++++++++++--------------
 1 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 3033b50..556cd7d 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -782,12 +782,12 @@ sub read_repo_config {
 
 sub extract_metadata {
 	my $id = shift or return (undef, undef, undef);
-	my ($url, $rev, $uuid) = ($id =~ /^git-svn-id:\s(\S+?)\@(\d+)
+	my ($url, $rev, $uuid) = ($id =~ /^\s*git-svn-id:\s+(.*)\@(\d+)
 							\s([a-f\d\-]+)$/x);
 	if (!defined $rev || !$uuid || !$url) {
 		# some of the original repositories I made had
 		# identifiers like this:
-		($rev, $uuid) = ($id =~/^git-svn-id:\s(\d+)\@([a-f\d\-]+)/);
+		($rev, $uuid) = ($id =~/^\s*git-svn-id:\s(\d+)\@([a-f\d\-]+)/);
 	}
 	return ($url, $rev, $uuid);
 }
@@ -799,10 +799,16 @@ sub cmt_metadata {
 
 sub working_head_info {
 	my ($head, $refs) = @_;
-	my ($fh, $ctx) = command_output_pipe('rev-list', $head);
-	while (my $hash = <$fh>) {
-		chomp($hash);
-		my ($url, $rev, $uuid) = cmt_metadata($hash);
+	my ($fh, $ctx) = command_output_pipe('log', $head);
+	my $hash;
+	while (<$fh>) {
+		if ( m{^commit ($::sha1)$} ) {
+			unshift @$refs, $hash if $hash and $refs;
+			$hash = $1;
+			next;
+		}
+		next unless s{^\s*(git-svn-id:)}{$1};
+		my ($url, $rev, $uuid) = extract_metadata($_);
 		if (defined $url && defined $rev) {
 			if (my $gs = Git::SVN->find_by_url($url)) {
 				my $c = $gs->rev_db_get($rev);
@@ -812,7 +818,6 @@ sub working_head_info {
 				}
 			}
 		}
-		unshift @$refs, $hash if $refs;
 	}
 	command_close_pipe($fh, $ctx);
 	(undef, undef, undef, undef);
@@ -2019,16 +2024,19 @@ sub rebuild {
 		return;
 	}
 	print "Rebuilding $db_path ...\n";
-	my ($rev_list, $ctx) = command_output_pipe("rev-list", $self->refname);
+	my ($log, $ctx) = command_output_pipe("log", $self->refname);
 	my $latest;
 	my $full_url = $self->full_url;
 	remove_username($full_url);
 	my $svn_uuid;
-	while (<$rev_list>) {
-		chomp;
-		my $c = $_;
-		die "Non-SHA1: $c\n" unless $c =~ /^$::sha1$/o;
-		my ($url, $rev, $uuid) = ::cmt_metadata($c);
+	my $c;
+	while (<$log>) {
+		if ( m{^commit ($::sha1)$} ) {
+			$c = $1;
+			next;
+		}
+		next unless s{^\s*(git-svn-id:)}{$1};
+		my ($url, $rev, $uuid) = ::extract_metadata($_);
 		remove_username($url);
 
 		# ignore merges (from set-tree)
@@ -2046,7 +2054,7 @@ sub rebuild {
 		$self->rev_db_set($rev, $c);
 		print "r$rev = $c\n";
 	}
-	command_close_pipe($rev_list, $ctx);
+	command_close_pipe($log, $ctx);
 	print "Done rebuilding $db_path\n";
 }
 
-- 
1.5.2.1.1131.g3b90

[PATCH] git-svn: cache max revision in rev_db databases

[Sam Vilain]

From: Sam Vilain <sam@vilain.net>

Cache the maximum revision for each rev_db URL rather than looking it
up each time.  This saves a lot of time when rebuilding indexes on a
freshly cloned repository.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 git-svn.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 556cd7d..a8b6669 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -801,6 +801,7 @@ sub working_head_info {
 	my ($head, $refs) = @_;
 	my ($fh, $ctx) = command_output_pipe('log', $head);
 	my $hash;
+	my %max;
 	while (<$fh>) {
 		if ( m{^commit ($::sha1)$} ) {
 			unshift @$refs, $hash if $hash and $refs;
@@ -810,11 +811,14 @@ sub working_head_info {
 		next unless s{^\s*(git-svn-id:)}{$1};
 		my ($url, $rev, $uuid) = extract_metadata($_);
 		if (defined $url && defined $rev) {
+			next if $max{$url} and $max{$url} < $rev;
 			if (my $gs = Git::SVN->find_by_url($url)) {
 				my $c = $gs->rev_db_get($rev);
 				if ($c && $c eq $hash) {
 					close $fh; # break the pipe
 					return ($url, $rev, $uuid, $gs);
+				} else {
+					$max{$url} ||= $gs->rev_db_max;
 				}
 			}
 		}
-- 
1.5.2.1.1131.g3b90

[PATCH] GIT-VERSION-GEN: don't convert - delimiter to .'s

[Sam Vilain]

Otherwise, a custom "v1.5.2.42.gb1ff" is considered newer than a
"v1.5.2.1.69.gcafe"

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 GIT-VERSION-GEN |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 06c360b..ac6a062 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -18,7 +18,7 @@ elif test -d .git &&
 	v[0-9]*) : happy ;;
 	esac
 then
-	VN=$(echo "$VN" | sed -e 's/-/./g');
+	:;
 else
 	VN="$DEF_VER"
 fi
-- 
1.5.2.1.1131.g3b90

[PATCH] git-remote: document -n

[Sam Vilain]

From: Sam Vilain <sam@vilain.net>

The 'show' and 'prune' commands accept an option '-n'; document what
it does.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 Documentation/git-remote.txt |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index ab232c2..61a6022 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -49,6 +49,9 @@ branch the `HEAD` at the remote repository actually points at.
 'show'::
 
 Gives some information about the remote <name>.
++
+With `-n` option, the remote heads are not queried first with
+`git ls-remote <name>`; cached information is used instead.
 
 'prune'::
 
@@ -56,6 +59,10 @@ Deletes all stale tracking branches under <name>.
 These stale branches have already been removed from the remote repository
 referenced by <name>, but are still locally available in
 "remotes/<name>".
++
+With `-n` option, the remote heads are not confirmed first with `git
+ls-remote <name>`; cached information is used instead.  Use with
+caution.
 
 'update'::
 
-- 
1.5.2.1.1131.g3b90

[PATCH] git-remote: allow 'git-remote fetch' as a synonym for 'git fetch'

[Sam Vilain]

From: Sam Vilain <sam@vilain.net>

I found myself typing this when doing remote-like things.  Perhaps
other people will find this useful

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 Documentation/git-remote.txt |    4 ++++
 git-remote.perl              |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 61a6022..b462ccd 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -64,6 +64,10 @@ With `-n` option, the remote heads are not confirmed first with `git
 ls-remote <name>`; cached information is used instead.  Use with
 caution.
 
+'fetch'::
+
+Synonym for `git fetch <name>`, and accepts all the same options.
+
 'update'::
 
 Fetch updates for a named set of remotes in the repository as defined by
diff --git a/git-remote.perl b/git-remote.perl
index b59cafd..2c60cae 100755
--- a/git-remote.perl
+++ b/git-remote.perl
@@ -404,11 +404,15 @@ elsif ($ARGV[0] eq 'add') {
 	}
 	add_remote($ARGV[1], $ARGV[2], \%opts);
 }
+elsif ($ARGV[0] eq 'fetch') {
+	exec("git-fetch", @ARGV[1..$#ARGV]);
+}
 else {
 	print STDERR "Usage: git remote\n";
 	print STDERR "       git remote add <name> <url>\n";
 	print STDERR "       git remote show <name>\n";
 	print STDERR "       git remote prune <name>\n";
 	print STDERR "       git remote update [group]\n";
+	print STDERR "       git remote fetch <fetch-options> <repository> <refspec>...\n";
 	exit(1);
 }
-- 
1.5.2.1.1131.g3b90

[PATCH] git-merge-ff: fast-forward only merge

[Sam Vilain]

This is primarily so that there is an easy switch to 'git-pull' to
be sure to fast forward only.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 Documentation/merge-strategies.txt |    5 +++++
 Makefile                           |    2 +-
 git-merge-ff.sh                    |    8 ++++++++
 git-merge.sh                       |    4 ++--
 4 files changed, 16 insertions(+), 3 deletions(-)
 create mode 100644 git-merge-ff.sh

diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 7df0266..00739bc 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -33,3 +33,8 @@ ours::
 	merge is always the current branch head.  It is meant to
 	be used to supersede old development history of side
 	branches.
+
+ff::
+	This is a degenerate merge strategy that always fails, which
+	means that the only time the target branch will change is if
+	there was no merge ("fast-forward" merge only).
diff --git a/Makefile b/Makefile
index 4ea5e45..7fa8fe3 100644
--- a/Makefile
+++ b/Makefile
@@ -210,7 +210,7 @@ SCRIPT_SH = \
 	git-tag.sh git-verify-tag.sh \
 	git-am.sh \
 	git-merge.sh git-merge-stupid.sh git-merge-octopus.sh \
-	git-merge-resolve.sh git-merge-ours.sh \
+	git-merge-resolve.sh git-merge-ours.sh git-merge-ff.sh \
 	git-lost-found.sh git-quiltimport.sh git-submodule.sh \
 	git-filter-branch.sh
 
diff --git a/git-merge-ff.sh b/git-merge-ff.sh
new file mode 100644
index 0000000..b0e0f85
--- /dev/null
+++ b/git-merge-ff.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Sam Vilain
+#
+# A degenerate merge strategy that only allows fast-forwarding.
+#
+
+exit 1;
diff --git a/git-merge.sh b/git-merge.sh
index 981d69d..63aa374 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -16,10 +16,10 @@ test -z "$(git ls-files -u)" ||
 LF='
 '
 
-all_strategies='recur recursive octopus resolve stupid ours subtree'
+all_strategies='recur recursive octopus resolve stupid ours subtree ff'
 default_twohead_strategies='recursive'
 default_octopus_strategies='octopus'
-no_trivial_merge_strategies='ours subtree'
+no_trivial_merge_strategies='ours subtree ff'
 use_strategies=
 
 index_merge=t
-- 
1.5.2.1.1131.g3b90

[PATCH] git-mergetool: add support for ediff

[Sam Vilain]

There was emerge already but I much prefer this mode.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 Documentation/config.txt        |    3 ++-
 Documentation/git-mergetool.txt |    3 ++-
 git-mergetool.sh                |   19 ++++++++++++++-----
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 50503e8..4661e24 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -550,7 +550,8 @@ merge.summary::
 merge.tool::
 	Controls which merge resolution program is used by
 	gitlink:git-mergetool[l].  Valid values are: "kdiff3", "tkdiff",
-	"meld", "xxdiff", "emerge", "vimdiff", "gvimdiff", and "opendiff".
+	"meld", "xxdiff", "emerge", "ediff", "vimdiff", "gvimdiff", and
+	"opendiff".
 
 merge.verbosity::
 	Controls the amount of output shown by the recursive merge
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 6c32c6d..1efe6e4 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -25,7 +25,8 @@ OPTIONS
 -t or --tool=<tool>::
 	Use the merge resolution program specified by <tool>.
 	Valid merge tools are:
-	kdiff3, tkdiff, meld, xxdiff, emerge, vimdiff, gvimdiff, and opendiff
+	kdiff3, tkdiff, meld, xxdiff, emerge, ediff, vimdiff, gvimdiff,
+	and opendiff
 +
 If a merge resolution program is not specified, 'git mergetool'
 will use the configuration variable merge.tool.  If the
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 7b66309..6fda8af 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -258,6 +258,15 @@ merge_file () {
 	    status=$?
 	    save_backup
 	    ;;
+	ediff)
+	    if base_present ; then
+		emacs --eval "(ediff-merge-files-with-ancestor \"$LOCAL\" \"$REMOTE\" \"$BASE\" nil \"$path\")"
+	    else
+		emacs --eval "(ediff-merge-files \"$LOCAL\" \"$REMOTE\" nil \"$path\")"
+	    fi
+	    status=$?
+	    save_backup
+	    ;;
     esac
     if test "$status" -ne 0; then
 	echo "merge of $path failed" 1>&2
@@ -299,7 +308,7 @@ done
 if test -z "$merge_tool"; then
     merge_tool=`git-config merge.tool`
     case "$merge_tool" in
-	kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | "")
+	kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | ediff | vimdiff | gvimdiff | "")
 	    ;; # happy
 	*)
 	    echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
@@ -320,15 +329,15 @@ if test -z "$merge_tool" ; then
         fi
     fi
     if echo "${VISUAL:-$EDITOR}" | grep 'emacs' > /dev/null 2>&1; then
-        merge_tool_candidates="$merge_tool_candidates emerge"
+        merge_tool_candidates="$merge_tool_candidates emerge ediff"
     fi
     if echo "${VISUAL:-$EDITOR}" | grep 'vim' > /dev/null 2>&1; then
         merge_tool_candidates="$merge_tool_candidates vimdiff"
     fi
-    merge_tool_candidates="$merge_tool_candidates opendiff emerge vimdiff"
+    merge_tool_candidates="$merge_tool_candidates opendiff ediff emerge vimdiff"
     echo "merge tool candidates: $merge_tool_candidates"
     for i in $merge_tool_candidates; do
-        if test $i = emerge ; then
+        if test $i = emerge || test $i = ediff ; then
             cmd=emacs
         else
             cmd=$i
@@ -351,7 +360,7 @@ case "$merge_tool" in
 	    exit 1
 	fi
 	;;
-    emerge)
+    emerge|ediff)
 	if ! type "emacs" > /dev/null 2>&1; then
 	    echo "Emacs is not available"
 	    exit 1
-- 
1.5.2.1.1131.g3b90

[PATCH] contrib/hooks: add post-update hook for updating working copy

[Sam Vilain]

Many users want 'git push' to work like 'git pull'; that is, after the
transfer of the new objects, the working copy is updated, too.  This
hook tries to be paranoid and never lose any information, as well as
being able to be safely just chmod +x'ed without destroying anything
it shouldn't.

Also allude to this potential feature on the man page for git-push.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 Documentation/git-push.txt   |    4 ++-
 templates/hooks--post-update |   78 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 665f6dc..9f5fbc7 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -20,7 +20,9 @@ necessary to complete the given refs.
 
 You can make interesting things happen to a repository
 every time you push into it, by setting up 'hooks' there.  See
-documentation for gitlink:git-receive-pack[1].
+documentation for gitlink:git-receive-pack[1].  One commonly
+requested feature, updating the working copy of the target
+repository, must be enabled in this way.
 
 
 OPTIONS
diff --git a/templates/hooks--post-update b/templates/hooks--post-update
index bcba893..b5d490c 100644
--- a/templates/hooks--post-update
+++ b/templates/hooks--post-update
@@ -1,8 +1,78 @@
 #!/bin/sh
 #
-# An example hook script to prepare a packed repository for use over
-# dumb transports.
+# This hook does two things:
 #
-# To enable this hook, make this file executable by "chmod +x post-update".
+#  1. update the "info" files that allow the list of references to be
+#     queries over dumb transports such as http
+#
+#  2. if this repository looks like it is a non-bare repository, and
+#     the checked-out branch is pushed to, then update the working copy.
+#     This makes "push" and "pull" symmetric operations, as in darcs and
+#     bzr.
+
+git-update-server-info
+
+export GIT_DIR=`cd $GIT_DIR; pwd`
+[ `expr "$GIT_DIR" : '.*/\.git'` = 0 ] && exit 0
+
+tree_in_revlog() {
+    ref=$1
+    tree=$2
+    found=$(
+    tail logs/$ref | while read commit rubbish
+    do
+        this_tree=`git-rev-parse commit $commit^{tree}`
+	if [ "$this_tree" = "$tree" ]
+        then
+	    echo $commit
+        fi
+    done
+    )
+    [ -n "$found" ] && true
+}
+
+for ref
+do
+active=`git-symbolic-ref HEAD`
+if [ "$ref" = "$active" ]
+then
+  echo "Pushing to checked out branch - updating working copy" >&2
+  success=
+  if ! (cd ..; git-diff-files) | grep -q .
+  then
+    # save the current index just in case
+    current_tree=`git-write-tree`
+    if tree_in_revlog $ref $current_tree
+    then
+      cd ..
+      if git-diff-index -R --name-status HEAD >&2 &&
+         git-diff-index -z --name-only --diff-filter=A HEAD | xargs -0r rm &&
+         git-reset --hard HEAD
+      then
+         success=1
+      else
+        echo "E:unexpected error during update" >&2
+      fi
+    else
+      echo "E:uncommitted, staged changes found" >&2
+    fi
+  else
+    echo "E:unstaged changes found" >&2
+  fi
 
-exec git-update-server-info
+  if [ -z "$success" ]
+  then
+    (
+    echo "Non-bare repository checkout is not clean - not updating it"
+    echo "However I AM going to update the index.  Any half-staged commit"
+    echo "in that checkout will be thrown away, but on the bright side"
+    echo "this is probably the least confusing thing for us to do and at"
+    echo "least we're not throwing any files somebody has changed away"
+    git-reset --mixed HEAD
+    echo
+    echo "This is the new status of the upstream working copy:"
+    git-status
+    ) >&2
+  fi
+fi
+done
-- 
1.5.2.1.1131.g3b90

[PATCH] git-repack: generational repacking (and example hook script)

[Sam Vilain]

Add an option to git-repack that makes the repack run suitable for
running very often.  The idea is that packs get given a "generation",
and that the number of packs in each generation (except the last one)
is bounded.

The useful invocation of this is git-repack -d -g

The -a option then becomes a degenerate case of generative repacking.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 Documentation/git-repack.txt |    6 +++
 git-repack.sh                |   74 +++++++++++++++++++++++++++++++++++-------
 templates/hooks--post-commit |   14 +++++++-
 3 files changed, 81 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index be8e5f8..d458377 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -42,6 +42,12 @@ OPTIONS
 	existing packs redundant, remove the redundant packs.
 	Also runs gitlink:git-prune-packed[1].
 
+-g::
+	Enable "generational" repacking.  This attempts to keep the
+	number of packs under control when repacking very often.  Most
+	useful when called from the `post-commit` hook (see
+	link:hooks.html[hooks] for more information).
+
 -l::
         Pass the `--local` option to `git pack-objects`, see
         gitlink:git-pack-objects[1].
diff --git a/git-repack.sh b/git-repack.sh
index 8c32724..3d253fa 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -3,19 +3,21 @@
 # Copyright (c) 2005 Linus Torvalds
 #
 
-USAGE='[-a] [-d] [-f] [-l] [-n] [-q] [--max-pack-size=N] [--window=N] [--depth=N]'
+USAGE='[-a] [-d] [-f] [-l] [-n] [-q] [-g] [--max-pack-size=N] [--window=N] [--depth=N]'
 SUBDIRECTORY_OK='Yes'
 . git-sh-setup
 
-no_update_info= all_into_one= remove_redundant=
-local= quiet= no_reuse= extra=
+no_update_info= generations= remove_redundant=
+local= quiet= no_reuse= extra= generation_width=
 while case "$#" in 0) break ;; esac
 do
 	case "$1" in
 	-n)	no_update_info=t ;;
-	-a)	all_into_one=t ;;
+	-a)	generations=0 ;;
 	-d)	remove_redundant=t ;;
 	-q)	quiet=-q ;;
+	-g)	generations=3 generation_width=10 ;;
+	-G)	generations=$2; generation_width=5; shift ;;
 	-f)	no_reuse=--no-reuse-object ;;
 	-l)	local=--local ;;
 	--max-pack-size=*) extra="$extra $1" ;;
@@ -40,24 +42,69 @@ PACKTMP="$GIT_OBJECT_DIRECTORY/.tmp-$$-pack"
 rm -f "$PACKTMP"-*
 trap 'rm -f "$PACKTMP"-*' 0 1 2 3 15
 
+generation=
+redundant=
+
 # There will be more repacking strategies to come...
-case ",$all_into_one," in
+case ",$generations," in
 ,,)
 	args='--unpacked --incremental'
 	;;
-,t,)
+,*,)
 	if [ -d "$PACKDIR" ]; then
+		max_gen=0
 		for e in `cd "$PACKDIR" && find . -type f -name '*.pack' \
 			| sed -e 's/^\.\///' -e 's/\.pack$//'`
 		do
 			if [ -e "$PACKDIR/$e.keep" ]; then
 				: keep
 			else
-				args="$args --unpacked=$e.pack"
 				existing="$existing $e"
+				if [ -e "$PACKDIR/$e.gen" ]; then
+					gen=`cat $PACKDIR/$e.gen`
+				else
+					gen=1
+				fi
+				[ "$max_gen" -lt $gen ] && max_gen=$gen
+				eval "gen_${gen}=\"\$gen_${gen} $e\"";
+				eval "c_gen_${gen}=\$((\$c_gen_${gen} + 1))";
 			fi
 		done
+		i=$max_gen
+		packing=
+		while [ $i -gt 0 ]
+		do
+			eval "c_gen=\$c_gen_$i"
+			eval "packs=\$gen_$i"
+			if [ -n "$c_gen" -a $i -gt "$generations" ]
+			then
+				echo "saw $c_gen packs at generation $i"
+				echo "therefore, repacking everything"
+				packing=1
+				[ -z "$generation" ] && generation=$(($i + 1))
+			elif [ -n "$c_gen" -a "$c_gen" -ge "$generation_width" -a "$i" -lt "$generations" ]
+			then
+				echo -n "generation $i has too many packs "
+				echo "($c_gen >= $generation_width)"
+				echo "repacking at this level and below"
+				packing=1
+				[ -z "$generation" ] && generation=$(($i + 1))
+			fi
+			if [ -n "$packing" ]
+			then
+				for x in $packs; do
+					args="$args --unpacked=$x.pack"
+					redundant="$redundant $x"
+				done
+			fi
+			i=$(($i - 1))
+		done
+		if [ -n "$generation" ]; then
+			[ "$generation" -gt "$generations" ] && generation=$generations
+			[ "$generation" -eq 0 ] && generation=1
+		fi
 	fi
+
 	[ -z "$args" ] && args='--unpacked --incremental'
 	;;
 esac
@@ -95,20 +142,23 @@ for name in $names ; do
 		exit 1
 	}
 	rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"
+	[ -n "$generation" ] && echo $generation > "$PACKDIR/pack-$name.gen"
 done
 
 if test "$remove_redundant" = t
 then
-	# We know $existing are all redundant.
-	if [ -n "$existing" ]
+	echo "removing redundant packs"
+	# We know $redundant are all redundant.
+	if [ -n "$redundant" ]
 	then
 		sync
 		( cd "$PACKDIR" &&
-		  for e in $existing
+		  for e in $redundant
 		  do
 			case " $fullbases " in
-			*" $e "*) ;;
-			*)	rm -f "$e.pack" "$e.idx" "$e.keep" ;;
+			*" $e "*) echo "ignoring $e" ;;
+			*)	echo "removing $e.pack etc";
+				rm -f "$e.pack" "$e.idx" "$e.keep" ;;
 			esac
 		  done
 		)
diff --git a/templates/hooks--post-commit b/templates/hooks--post-commit
index 8be6f34..669f1fc 100644
--- a/templates/hooks--post-commit
+++ b/templates/hooks--post-commit
@@ -5,4 +5,16 @@
 #
 # To enable this hook, make this file executable.
 
-: Nothing
+threshold=`git-config gc.threshold`
+threshold=${threshold-250}
+
+gd=`git-rev-parse --git-dir`
+found=$(find $gd/objects/?? -type f | head -$threshold | wc -l)
+
+if [ $found -ge $threshold ]
+then
+    echo "At least $threshold loose objects, running generational repack"
+    git-repack -g -d
+else
+    echo "Found only $found loose objects, less than $threshold"
+fi
-- 
1.5.2.1.1131.g3b90

Re: [PATCH] git-repack: generational repacking (and example hook script)

[Nicolas Pitre]

On Sat, 30 Jun 2007, Sam Vilain wrote:

> Add an option to git-repack that makes the repack run suitable for
> running very often.  The idea is that packs get given a "generation",
> and that the number of packs in each generation (except the last one)
> is bounded.

Please explain again why this should be useful and is worth the 
complexity it brings along.  Last time this was discussed I wasn't 
convinced at all, and I'm still not convinced this time either.


Nicolas

Re: [PATCH] git-repack: generational repacking (and example hook script)

[Sam Vilain]

Nicolas Pitre wrote:
>> Add an option to git-repack that makes the repack run suitable for
>> running very often.  The idea is that packs get given a "generation",
>> and that the number of packs in each generation (except the last one)
>> is bounded.
> 
> Please explain again why this should be useful and is worth the 
> complexity it brings along.  Last time this was discussed I wasn't 
> convinced at all, and I'm still not convinced this time either.

First I think we should establish some common ground.

1. Do you agree that some users would want their git repositories to be
"maintenance free"?

2. Do you agree that having thousands of packs would add measurable
overhead?

Sam.

Re: [PATCH] git-repack: generational repacking (and example hook script)

[Nicolas Pitre]

On Tue, 3 Jul 2007, Sam Vilain wrote:

> Nicolas Pitre wrote:
> >> Add an option to git-repack that makes the repack run suitable for
> >> running very often.  The idea is that packs get given a "generation",
> >> and that the number of packs in each generation (except the last one)
> >> is bounded.
> > 
> > Please explain again why this should be useful and is worth the 
> > complexity it brings along.  Last time this was discussed I wasn't 
> > convinced at all, and I'm still not convinced this time either.
> 
> First I think we should establish some common ground.
> 
> 1. Do you agree that some users would want their git repositories to be
> "maintenance free"?

I'm not so sure.  I think it is best to let GIT users know (or the 
admins on their behalf) how to properly maintain their repository than 
pretending that it needs no maintenance.  GIT is a tool for "developers" 
after all, not for Aunt Tillie.

And even if your developers are completely inept to the point of not 
wanting to run 'git gc' once a week for example, or once a day if 
they're otherwise really really productive, I'm sure you can automate 
some of that maintenance asynchronously from a simple post commit hook 
or something, based on the output of 'git count-objects -v'.

> 2. Do you agree that having thousands of packs would add measurable
> overhead?

Sure it would, but far less as it used to when we last discussed this 
since performances in those cases has been improved significantly.

And if you end up with thousands of packs in the first place I think you 
have a more fundamental problem to fix, something that generational 
repacking would just paper over.


Nicolas

Re: [PATCH] git-repack: generational repacking (and example hook script)

[Shawn O. Pearce]

Nicolas Pitre <nico@cam.org> wrote:
> And even if your developers are completely inept to the point of not 
> wanting to run 'git gc' once a week for example, or once a day if 
> they're otherwise really really productive, I'm sure you can automate 
> some of that maintenance asynchronously from a simple post commit hook 
> or something, based on the output of 'git count-objects -v'.

Yea, you need not just the loose object count but also the number
of packfiles.  git-gui suggests repacking based on loose object
count alone right now, but with us keeping fetched packfiles by
git-index-pack I found a repository on my desktop the other day
that had 30 packfiles in it.  I need to fix that in git-gui and
also add a limit based on the number of small-ish packfiles present.

BTW, I have some users that might as well be Aunt Tillie.  They
merge any branch they can find.  "Oh, look, there's a new branch
called Highly-Experimental!  I'll bet that's good for merging too!"

Asking them to also run git-gc once in a while is like asking them
to actually do their job or something...  *sighs* OK, I have to go
to work and undo that Highly-Experimental merge I found last night.
*sigh*
 
-- 
Shawn.

Re: [PATCH] git-repack: generational repacking (and example hook script)

[Sam Vilain]

Nicolas Pitre wrote:
>> 1. Do you agree that some users would want their git repositories to be
>> "maintenance free"?
>
> I'm not so sure.

Well, no offence, but I think you should withhold from voicing a
fundamental concern as this, because you're not one of its target users.

I'd be more than happy to reshape the patch so that it does not
introduce this "complexity" into the current code path.  Potentially it
could entirely fit into the post-commit hook, which should not upset
anybody as they don't have to turn it on.  I just noticed that the
"repack -a" code path was already doing a lot of what a generational
repack would have to do, so thought I'd re-use the code.

Of course your critical analysis of code is more than welcome.

> And even if your developers are completely inept to the point of not 
> wanting to run 'git gc' once a week for example, 

This kind of characterisation does not help the discussion.

> I'm sure you can automate 
> some of that maintenance asynchronously from a simple post commit hook 
> or something, based on the output of 'git count-objects -v'.

Yet another little command that I didn't know about that could make the
 patch simpler.

Potentially the calculations could be performed in count-objects.  I'll
investigate that.

>> 2. Do you agree that having thousands of packs would add measurable
>> overhead?
> 
> Sure it would, but far less as it used to when we last discussed this 
> since performances in those cases has been improved significantly.

Far less for examining recent history.  It would however make examining
older history, and potentially blame operations slower.  Just how much
slower I don't know, but I'd guess that random access with 1000 small
indices scanned sequentially is slower than with 10 larger indices.

> And if you end up with thousands of packs in the first place I think you 
> have a more fundamental problem to fix, something that generational 
> repacking would just paper over.

Right, but only if you are of the opinion that a repack is something
that is best run off-line from normal work flow.  If you want it to run
in-line, then the fundamental problem would be "a simple operation now
takes much longer because a huge repack is occurring".

So I think this fundamental decision is more of a user preference.

Sam.

Re: [PATCH] git-repack: generational repacking (and example hook script)

[Johannes Schindelin]

Hi,

On Wed, 4 Jul 2007, Sam Vilain wrote:

> Nicolas Pitre wrote:
> >> 1. Do you agree that some users would want their git repositories to be
> >> "maintenance free"?
> >
> > I'm not so sure.
> 
> Well, no offence, but I think you should withhold from voicing a
> fundamental concern as this, because you're not one of its target users.

Let's put it this way. A lot of car drivers would probably agree that it 
is a Good Thing (tm) if their car automatically went to get gas, before it 
ran out of it. Less hassle, right?

Yes, except if your car decides to get gas when you are already late, 
speeding, trying to catch your plane.

Same holds for Git. Is is worth the hassle having to wait for this 
automatic git-gc when your boss is waiting impatiently for you to show 
some results?

Now, you seem to argue that the cost of a single git-gc should be 
decreased. But I maintain that the _usefulness_ of git-gc is decreased 
that way, too.

In all of my projects, the most efficient setup is one big pack. That is 
why I set up some cronjobs on the machines that run 24/7, and that is why 
I run "git-gc --prune" when idling, on almost all my repos.

Ciao,
Dscho

Re: [PATCH] git-repack: generational repacking (and example hook script)

[Sam Vilain]

Johannes Schindelin wrote:
>>>> 1. Do you agree that some users would want their git repositories to be
>>>> "maintenance free"?
>>> I'm not so sure.
>> Well, no offence, but I think you should withhold from voicing a
>> fundamental concern as this, because you're not one of its target users.
> Let's put it this way. A lot of car drivers would probably agree that it 
> is a Good Thing (tm) if their car automatically went to get gas, before it 
> ran out of it. Less hassle, right?
> 
> Yes, except if your car decides to get gas when you are already late, 
> speeding, trying to catch your plane.

Ok, but if you're only packing a few hundred objects it usually won't
matter because it is fast enough that you hardly notice.

And if you don't like it, you turn it off, or don't turn it on.

Sam.

Re: [PATCH] git-repack: generational repacking (and example hook script)

[Alex Riesen]

On 7/4/07, Sam Vilain <sam@vilain.net> wrote:
> Johannes Schindelin wrote:
> >>>> 1. Do you agree that some users would want their git repositories to be
> >>>> "maintenance free"?
> >>> I'm not so sure.
> >> Well, no offence, but I think you should withhold from voicing a
> >> fundamental concern as this, because you're not one of its target users.
> > Let's put it this way. A lot of car drivers would probably agree that it
> > is a Good Thing (tm) if their car automatically went to get gas, before it
> > ran out of it. Less hassle, right?
> >
> > Yes, except if your car decides to get gas when you are already late,
> > speeding, trying to catch your plane.
>
> Ok, but if you're only packing a few hundred objects it usually won't
> matter because it is fast enough that you hardly notice.

Unless you are on Windows, MacOSX, a notebook with P233, or unless
it is your home server in cellar built out of decommissioned desktop
(trusty old P133 with reasonable (for such a thing) 256Mb).

Re: [PATCH] git-repack: generational repacking (and example hook script)

[Nicolas Pitre]

On Wed, 4 Jul 2007, Sam Vilain wrote:

> Johannes Schindelin wrote:
> >>>> 1. Do you agree that some users would want their git repositories to be
> >>>> "maintenance free"?
> >>> I'm not so sure.
> >> Well, no offence, but I think you should withhold from voicing a
> >> fundamental concern as this, because you're not one of its target users.
> > Let's put it this way. A lot of car drivers would probably agree that it 
> > is a Good Thing (tm) if their car automatically went to get gas, before it 
> > ran out of it. Less hassle, right?
> > 
> > Yes, except if your car decides to get gas when you are already late, 
> > speeding, trying to catch your plane.
> 
> Ok, but if you're only packing a few hundred objects it usually won't
> matter because it is fast enough that you hardly notice.

... in which case you might as well keep them loose too.

> And if you don't like it, you turn it off, or don't turn it on.

You seem to forget the maintenance cost of having this in the Git 
distribution.  When something is merged in, it has to be maintained and 
kept working.  Given the complexity of your proposal weighted against 
the relative benefits I remain unconvinced.

Yet you didn't state what exactly is the issue you're trying to solve.  
If it is only to avoid running "git gc" occasionally then this clearly 
isn't a benefit worth the cost.

If, instead, you implement it as a post-commit or post-receive hook 
meant for contrib/hooks/ then I wouldn't have any issue with that.


Nicolas

Re: [PATCH] contrib/hooks: add post-update hook for updating working copy

[Junio C Hamano]

Sam Vilain <sam.vilain@catalyst.net.nz> writes:

> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 665f6dc..9f5fbc7 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -20,7 +20,9 @@ necessary to complete the given refs.
>  
>  You can make interesting things happen to a repository
>  every time you push into it, by setting up 'hooks' there.  See
> -documentation for gitlink:git-receive-pack[1].
> +documentation for gitlink:git-receive-pack[1].  One commonly
> +requested feature, updating the working copy of the target
> +repository, must be enabled in this way.

That is more like "could be", not "must be", and it is not the
manpage's job to pass judgement on if a feature is often requested.

> diff --git a/templates/hooks--post-update b/templates/hooks--post-update
> index bcba893..b5d490c 100644
> --- a/templates/hooks--post-update
> +++ b/templates/hooks--post-update
> @@ -1,8 +1,78 @@
>  #!/bin/sh
>  #
> -# An example hook script to prepare a packed repository for use over
> -# dumb transports.
> +# This hook does two things:
>  #
> -# To enable this hook, make this file executable by "chmod +x post-update".
> +#  1. update the "info" files that allow the list of references to be
> +#     queries over dumb transports such as http
> +#
> +#  2. if this repository looks like it is a non-bare repository, and
> +#     the checked-out branch is pushed to, then update the working copy.
> +#     This makes "push" and "pull" symmetric operations, as in darcs and
> +#     bzr.
> +
> +git-update-server-info
> +
> +export GIT_DIR=`cd $GIT_DIR; pwd`
> +[ `expr "$GIT_DIR" : '.*/\.git'` = 0 ] && exit 0

That's convoluted.  If you use 'expr', probably

	expr "$GIT_DIR" : '.*/\.git' >/dev/null || exit 0

but I would probably do without an extra fork, like this:

	case "$GIT_DIR" in */.git) : happy ;; *) exit 0 ;; esac

Also you can exit early if $GIT_DIR/index does not exist.

> +
> +tree_in_revlog() {

revlog?  Since when are we Hg?

> +    ref=$1
> +    tree=$2
> +    found=$(
> +    tail logs/$ref | while read commit rubbish
> +    do
> +        this_tree=`git-rev-parse commit $commit^{tree}`
> +	if [ "$this_tree" = "$tree" ]
> +        then
> +	    echo $commit
> +        fi
> +    done
> +    )
> +    [ -n "$found" ] && true
> +}

I would imagine that "$some_command && true" would always give
the same result as "$some_command" alone.  I'd just write this
as:

	test -n "$found"

if I were you.

> +
> +for ref
> +do
> +active=`git-symbolic-ref HEAD`

 - You do not want to do this inside "for ref" loop as this is
   constant expression.

 - When the HEAD is detached, this will give you an error message,
   and an empty string.  But you do not care about detached HEAD
   case anyway I would imagine.

Perhaps...

	active=$(git symbolic-ref -q HEAD) || exit 0
	for ref
        do
        	...

> +if [ "$ref" = "$active" ]
> +then
> +  echo "Pushing to checked out branch - updating working copy" >&2
> +  success=
> +  if ! (cd ..; git-diff-files) | grep -q .
> +  then

Trying to see if there is any difference from the index, aka

	git diff-files --quiet

?

> +    # save the current index just in case
> +    current_tree=`git-write-tree`

What happens if the user is in the middle of a merge?
write-tree would fail and you should error out.

> +    if tree_in_revlog $ref $current_tree
> +    then

Why should it behave differently depending on whether the index
matches one of the arbitrary (i.e. taken from "tail" default)
number of commits the user happened to be at in the recent past?
If the check were "does it match with the HEAD", there could be
a valid justification but this check does not make any sense to
me.

> +      cd ..
> +      if git-diff-index -R --name-status HEAD >&2 &&
> +         git-diff-index -z --name-only --diff-filter=A HEAD | xargs -0r rm &&
> +         git-reset --hard HEAD

I do not understand the first two lines at all.  Are you trying
to lose working files for the paths that were added to the index
since HEAD?  "git reset --hard HEAD" should take care of that
already.  To test:

	$ >a-new-file
        $ git add a-new-file
        $ git reset --hard HEAD
        $ ls -l a-new-file
	ls: a-new-file: No such file or directory

But more importantly, why is it justified to throw away such
files to begin with?

> +      then
> +         success=1
> +      else
> +        echo "E:unexpected error during update" >&2
> +      fi
> +    else
> +      echo "E:uncommitted, staged changes found" >&2
> +    fi
> +  else
> +    echo "E:unstaged changes found" >&2
> +  fi

I think this part is a good demonstration why pushing into a
live branch should not attempt to update the working tree.  It
sometimes happens, and it sometimes cannot (which is not your
fault at all), but the indication of what happened (or did not
happen) goes to the person who pushed the changes, not to the
person who gets confusing behaviour if the index/worktree
suddenly goes out of sync with respect to the updated HEAD.

The longer I look at this patch, the more inclined I become to
say that the only part that is worth saving is the next hunk.

> -exec git-update-server-info
> +  if [ -z "$success" ]
> +  then
> +    (
> +    echo "Non-bare repository checkout is not clean - not updating it"
> +    echo "However I AM going to update the index.  Any half-staged commit"
> +    echo "in that checkout will be thrown away, but on the bright side"
> +    echo "this is probably the least confusing thing for us to do and at"
> +    echo "least we're not throwing any files somebody has changed away"
> +    git-reset --mixed HEAD
> +    echo
> +    echo "This is the new status of the upstream working copy:"
> +    git-status
> +    ) >&2
> +  fi
> +fi
> +done
> -- 
> 1.5.2.1.1131.g3b90

Re: [PATCH] contrib/hooks: add post-update hook for updating working copy

[Sam Vilain]

Junio C Hamano wrote:
> >  You can make interesting things happen to a repository
> > >  every time you push into it, by setting up 'hooks' there.  See
> > > -documentation for gitlink:git-receive-pack[1].
> > > +documentation for gitlink:git-receive-pack[1].  One commonly
> > > +requested feature, updating the working copy of the target
> > > +repository, must be enabled in this way.
>
> That is more like "could be", not "must be", and it is not the
> manpage's job to pass judgement on if a feature is often requested.

Ok, I'll just remove that clause.  Or do you think it perhaps belongs in
a NOTES or HINTS section?

>> +    if tree_in_revlog $ref $current_tree
>> +    then
> 
> Why should it behave differently depending on whether the index
> matches one of the arbitrary (i.e. taken from "tail" default)
> number of commits the user happened to be at in the recent past?
> If the check were "does it match with the HEAD", there could be
> a valid justification but this check does not make any sense to
> me.

Ok, well first we check that the index matches the working copy.  But if
there are staged changes which have not been committed, then the written
tree will (probably) not exist anywhere in the reflog for the current
branch, and we can stop.

Basically I'm trying to figure out "does the current index have any
uncommitted changes".  If it matches the tree from the previous (handful
of) ref(s), then the answer is "no".  If we can't find it anywhere then
it's probably got staged changes, and short of trying to move the
changes forward, we should stop.

>> +      if git-diff-index -R --name-status HEAD >&2 &&
>> +         git-diff-index -z --name-only --diff-filter=A HEAD | xargs -0r rm &&
>> +         git-reset --hard HEAD
> 
> I do not understand the first two lines at all.  Are you trying
> to lose working files for the paths that were added to the index
> since HEAD?  "git reset --hard HEAD" should take care of that
> already.

The first one simply displays what is happening to the working copy for
the benefit of the user.

The second is implementing something I for some reason thought git-reset
wasn't doing.

> But more importantly, why is it justified to throw away such
> files to begin with?

Because we've already previously decided that they are safely stowed in
a previous (via time/reflog) revision of the current branch.

>> +        echo "E:unexpected error during update" >&2
>> +      fi
>> +    else
>> +      echo "E:uncommitted, staged changes found" >&2
>> +    fi
>> +  else
>> +    echo "E:unstaged changes found" >&2
>> +  fi
> 
> I think this part is a good demonstration why pushing into a
> live branch should not attempt to update the working tree.  It
> sometimes happens, and it sometimes cannot (which is not your
> fault at all), but the indication of what happened (or did not
> happen) goes to the person who pushed the changes, not to the
> person who gets confusing behaviour if the index/worktree
> suddenly goes out of sync with respect to the updated HEAD.

One counter-argument to this is that you indicate that is the behaviour
that you want when you chmod +x the hook.  It should gracefully step out
of the way when people who currently set that hook active keep doing it.

But this problem exists without this hook, in fact it is far worse.  The
indication of what happened goes nowhere, and the person gets extremely
confusing behaviour when they commit.

Perhaps it would make sense to do this check in the "update" hook as
well, thereby chmod +x refuses to allow a push that touches the
currently checked out branch.  The check would then be run twice if both
hooks are enabled, unless the first one can signal success/verification
to the second somehow.

> The longer I look at this patch, the more inclined I become to
> say that the only part that is worth saving is the next hunk.
> 
>> -exec git-update-server-info
>> +  if [ -z "$success" ]
>> +  then
>> +    (
>> +    echo "Non-bare repository checkout is not clean - not updating it"
>> +    echo "However I AM going to update the index.  Any half-staged commit"
>> +    echo "in that checkout will be thrown away, but on the bright side"
>> +    echo "this is probably the least confusing thing for us to do and at"
>> +    echo "least we're not throwing any files somebody has changed away"
>> +    git-reset --mixed HEAD
>> +    echo
>> +    echo "This is the new status of the upstream working copy:"
>> +    git-status
>> +    ) >&2
>> +  fi
>> +fi
>> +done

I disagree; I think any half-measure is going to leave new users
horribly surprised by what happens, and if you just reset the index then
the staged commit is lost.

Sam.

Re: [PATCH] contrib/hooks: add post-update hook for updating working copy

[Junio C Hamano]

Sam Vilain <sam@vilain.net> writes:

> Basically I'm trying to figure out "does the current index have any
> uncommitted changes".  If it matches the tree from the previous (handful
> of) ref(s), then the answer is "no".  If we can't find it anywhere then
> it's probably got staged changes, and short of trying to move the
> changes forward, we should stop.

The fact that the index does not match the HEAD means that the
user (possibly not the one who is pushing) is in the middle of
doing something.  A tree that happens to match that state exists
in the recent reflog history would only mean that the same state
exists _somewhere_; it does not mean it is easy for the end user
to go back to it at all.

>> But more importantly, why is it justified to throw away such
>> files to begin with?
>
> Because we've already previously decided that they are safely stowed in
> a previous (via time/reflog) revision of the current branch.

The user may have spent hours to come up to that state while
doing something we do not have any way of knowing what, and this
"heuristic" is allowing to lose that.  As you say, we do not
lose the tree from the repository, but we lose track of which
state the user was interested in.  I find that unjustified.

> Perhaps it would make sense to do this check in the "update" hook as
> well, thereby chmod +x refuses to allow a push that touches the
> currently checked out branch.

Having the check in update to prevent it makes sense,
independently.

>> The longer I look at this patch, the more inclined I become to
>> say that the only part that is worth saving is the next hunk.

Actually, I think "the first sentence of the output in the next
hunk" was what I meant.  That is, "we are not updating it
because it is dirty and you cannot get back to the original
state if this was a mistake".  And not updating the index nor
the working tree.

How about doing something simpler, more predicatable and safer,
like this...

 * If HEAD/index/working tree match, then obviously we can do an
   equivalent of "reset --hard".  There is little chance that
   this is a wrong thing to do, and even when the user did not
   want that happen, the user can easily recover with for
   example "git checkout @{1} .".  So I am not opposed to
   updating the index/working tree in this case at all.

 * Otherwise, especially when HEAD and index do not match,
   touching index nor working tree is absolutely a no-no,
   without giving the user to sort the mess out.  So either in
   "update" hook you prevent it from happening.

Later, when we have git-stash, we can do a bit better in a dirty
working tree.  We could make a stash of the state _before_
updating the tip of the current branch, and let the push update
the tip, and do an equivalent of "reset --hard".  Unstashing the
state on top of the updated tip could fail, but at that point,
the user has the choice of making a new branch (or use detached
HEAD) at @{1} (that is, the HEAD before the push updated it) and
then unstash the state on top of it to recreate the state before
the push made a mess.


    

Re: [PATCH] git-mergetool: add support for ediff

[Junio C Hamano]

Sam Vilain <sam.vilain@catalyst.net.nz> writes:

> There was emerge already but I much prefer this mode.

I thought Ted said he'll look into clearning this up, so I won't
apply it yet at this moment to my tree, but have one comment...

> @@ -320,15 +329,15 @@ if test -z "$merge_tool" ; then
>          fi
>      fi
>      if echo "${VISUAL:-$EDITOR}" | grep 'emacs' > /dev/null 2>&1; then
> -        merge_tool_candidates="$merge_tool_candidates emerge"
> +        merge_tool_candidates="$merge_tool_candidates emerge ediff"
>      fi
>      if echo "${VISUAL:-$EDITOR}" | grep 'vim' > /dev/null 2>&1; then
>          merge_tool_candidates="$merge_tool_candidates vimdiff"
>      fi
> -    merge_tool_candidates="$merge_tool_candidates opendiff emerge vimdiff"
> +    merge_tool_candidates="$merge_tool_candidates opendiff ediff emerge vimdiff"
>      echo "merge tool candidates: $merge_tool_candidates"

So by default outside X environment, if your $EDITOR is emacs,
you would use emerge and not ediff, but if your $EDITOR is unset
and have emacs in your $PATH you would use ediff not emerge?

Re: [PATCH] git-mergetool: add support for ediff

[Sam Vilain]

Junio C Hamano wrote:
> I thought Ted said he'll look into clearning this up, so I won't
> apply it yet at this moment to my tree, but have one comment...

Yes, sorry I should have left this one out, it didn't get any changes.

Sam.

Re: [PATCH] git-merge-ff: fast-forward only merge

[Johannes Schindelin]

Hi,

On Sat, 30 Jun 2007, Sam Vilain wrote:

>  Documentation/merge-strategies.txt |    5 +++++
>  Makefile                           |    2 +-
>  git-merge-ff.sh                    |    8 ++++++++
>  git-merge.sh                       |    4 ++--

Still no test script that could tell you if it does what it is supposed to 
be...

Ciao,
Dscho

Re: [PATCH] git-merge-ff: fast-forward only merge

[Matthias Lederhofer]

Sam Vilain <sam.vilain@catalyst.net.nz> wrote:
> This is primarily so that there is an easy switch to 'git-pull' to
> be sure to fast forward only.

Is this still broken or am I just doing something totally wrong?

    % git reset --hard origin/master~15
    HEAD is now at e1341ab... Merge branch 'master' of git://git.kernel.org/pub/scm/gitk/gitk into pm/gitk
    % git merge -s ff origin/master
    Automatic merge failed; fix conflicts and then commit the result.
    [1]    19368 exit 1     git merge -s ff origin/master
    % git merge origin/master   
    Updating e1341ab..7c85173              
    Fast forward
    [..]
     23 files changed, 236 insertions(+), 79 deletions(-)

Re: [PATCH] git-remote: allow 'git-remote fetch' as a synonym for 'git fetch'

[Junio C Hamano]

Sam Vilain <sam.vilain@catalyst.net.nz> writes:

> From: Sam Vilain <sam@vilain.net>
>
> I found myself typing this when doing remote-like things.  Perhaps
> other people will find this useful

I would like to reject this, for the same reason I did not apply
three patch series "Human friendly git" on April 1st this year
;-).

Re: [PATCH] git-remote: document -n

[Frank Lichtenheld]

On Sat, Jun 30, 2007 at 08:56:16PM +1200, Sam Vilain wrote:
> From: Sam Vilain <sam@vilain.net>
> 
> The 'show' and 'prune' commands accept an option '-n'; document what
> it does.

You might want to add that in the SYNOPSIS, too.

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

Re: [PATCH] GIT-VERSION-GEN: don't convert - delimiter to .'s

[Junio C Hamano]

Sam Vilain <sam.vilain@catalyst.net.nz> writes:

> Otherwise, a custom "v1.5.2.42.gb1ff" is considered newer than a
> "v1.5.2.1.69.gcafe"

Does this solve anything, I wonder?  v1.5.2-this and
v1.5.2.1-that are not really comparable to begin with.

Re: [PATCH] GIT-VERSION-GEN: don't convert - delimiter to .'s

[Jakub Narebski]

Sam Vilain wrote:

> Otherwise, a custom "v1.5.2.42.gb1ff" is considered newer than a
> "v1.5.2.1.69.gcafe"

Wouldn't it be better to do what tig did, namely put the extra part,
i.e. the number of commits since tagged revision and shortened sha1 into
REVISION rather than VERSION for an rpm for example?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] git-svn: cache max revision in rev_db databases

[Junio C Hamano]

Sam Vilain <sam.vilain@catalyst.net.nz> writes:

> Cache the maximum revision for each rev_db URL rather than looking it
> up each time.  This saves a lot of time when rebuilding indexes on a
> freshly cloned repository.
>
> Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>

I think both the previous one from Sam that makes it use git-log
instead of git-rev-list and this one looks sane.  Ack/Nack is
appreciated.

> ---
>  git-svn.perl |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 556cd7d..a8b6669 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -801,6 +801,7 @@ sub working_head_info {
>  	my ($head, $refs) = @_;
>  	my ($fh, $ctx) = command_output_pipe('log', $head);
>  	my $hash;
> +	my %max;
>  	while (<$fh>) {
>  		if ( m{^commit ($::sha1)$} ) {
>  			unshift @$refs, $hash if $hash and $refs;
> @@ -810,11 +811,14 @@ sub working_head_info {
>  		next unless s{^\s*(git-svn-id:)}{$1};
>  		my ($url, $rev, $uuid) = extract_metadata($_);
>  		if (defined $url && defined $rev) {
> +			next if $max{$url} and $max{$url} < $rev;
>  			if (my $gs = Git::SVN->find_by_url($url)) {
>  				my $c = $gs->rev_db_get($rev);
>  				if ($c && $c eq $hash) {
>  					close $fh; # break the pipe
>  					return ($url, $rev, $uuid, $gs);
> +				} else {
> +					$max{$url} ||= $gs->rev_db_max;
>  				}
>  			}
>  		}
> -- 
> 1.5.2.1.1131.g3b90

Re: [PATCH] git-svn: cache max revision in rev_db databases

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Sam Vilain <sam.vilain@catalyst.net.nz> writes:
> 
> > Cache the maximum revision for each rev_db URL rather than looking it
> > up each time.  This saves a lot of time when rebuilding indexes on a
> > freshly cloned repository.
> >
> > Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
> 
> I think both the previous one from Sam that makes it use git-log
> instead of git-rev-list and this one looks sane.  Ack/Nack is
> appreciated.

Now that 80583c0ef61cc966c7eee79cf3623a83197e19b8 is in, both patches
are:

Acked-by: Eric Wong <normalperson@yhbt.net>

> > ---
> >  git-svn.perl |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/git-svn.perl b/git-svn.perl
> > index 556cd7d..a8b6669 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -801,6 +801,7 @@ sub working_head_info {
> >  	my ($head, $refs) = @_;
> >  	my ($fh, $ctx) = command_output_pipe('log', $head);
> >  	my $hash;
> > +	my %max;
> >  	while (<$fh>) {
> >  		if ( m{^commit ($::sha1)$} ) {
> >  			unshift @$refs, $hash if $hash and $refs;
> > @@ -810,11 +811,14 @@ sub working_head_info {
> >  		next unless s{^\s*(git-svn-id:)}{$1};
> >  		my ($url, $rev, $uuid) = extract_metadata($_);
> >  		if (defined $url && defined $rev) {
> > +			next if $max{$url} and $max{$url} < $rev;
> >  			if (my $gs = Git::SVN->find_by_url($url)) {
> >  				my $c = $gs->rev_db_get($rev);
> >  				if ($c && $c eq $hash) {
> >  					close $fh; # break the pipe
> >  					return ($url, $rev, $uuid, $gs);
> > +				} else {
> > +					$max{$url} ||= $gs->rev_db_max;
> >  				}
> >  			}
> >  		}
> > -- 
> > 1.5.2.1.1131.g3b90
> 

-- 
Eric Wong

Re: [PATCH] git-svn: cache max revision in rev_db databases

[Junio C Hamano]

Thanks.

Re: [PATCH] repack: improve documentation on -a option

[Frank Lichtenheld]

On Sat, Jun 30, 2007 at 08:56:12PM +1200, Sam Vilain wrote:
>  -a::
>  	Instead of incrementally packing the unpacked objects,
> -	pack everything available into a single pack.
> +	pack everything referenced into a single pack.
>  	Especially useful when packing a repository that is used
> -	for private development and there is no need to worry
> -	about people fetching via dumb file transfer protocols
> -	from it.  Use with '-d'.
> +	for private development and there no need to worry

Got "is" lost here intentionally? The change doesn't make sense
to me.

> +	about people fetching via dumb protocols from it.  Use
> +	with '-d'.  This will clean up the objects that `git prune`
> +	leaves behind, but `git fsck-objects --full` shows as
> +	dangling.

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

Re: [PATCH] repack: improve documentation on -a option

[Junio C Hamano]

Frank Lichtenheld <frank@lichtenheld.de> writes:

> On Sat, Jun 30, 2007 at 08:56:12PM +1200, Sam Vilain wrote:
>>  -a::
>>  	Instead of incrementally packing the unpacked objects,
>> -	pack everything available into a single pack.
>> +	pack everything referenced into a single pack.
>>  	Especially useful when packing a repository that is used
>> -	for private development and there is no need to worry
>> -	about people fetching via dumb file transfer protocols
>> -	from it.  Use with '-d'.
>> +	for private development and there no need to worry
>
> Got "is" lost here intentionally? The change doesn't make sense
> to me.
>
>> +	about people fetching via dumb protocols from it.  Use
>> +	with '-d'.  This will clean up the objects that `git prune`
>> +	leaves behind, but `git fsck-objects --full` shows as
>> +	dangling.

Also 'fsck-objects' is somewhat outdated.  Will fix them up.

Re: a bunch of outstanding updates

[Frank Lichtenheld]

On Sat, Jun 30, 2007 at 08:56:11PM +1200, Sam Vilain wrote:
> 
> Following up to this e-mail are a whole load of outstanding feature
> requests of mine.

FWIW, I would prefer you'd use --no-chain-reply-to for totally unrelated
changes. (But really I would prefer not to have -chain-reply-to as
default anyway...)

just my 2¢

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

[PATCH] git-svn: use git-log rather than rev-list | xargs cat-file

[Sam Vilain]

This saves a bit of time when rebuilding the git-svn index.

Signed-off-by: Sam Vilain <sam@vilain.net>
---
 git-svn.perl |   30 +++++++++++++++++++-----------
 1 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index e350061..610563c 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -802,10 +802,15 @@ sub cmt_metadata {
 
 sub working_head_info {
 	my ($head, $refs) = @_;
-	my ($fh, $ctx) = command_output_pipe('rev-list', $head);
-	while (my $hash = <$fh>) {
-		chomp($hash);
-		my ($url, $rev, $uuid) = cmt_metadata($hash);
+	my ($fh, $ctx) = command_output_pipe('log', $head);
+	my $hash;
+	while (<$fh>) {
+		if ( m{^commit ($::sha1)$} ) {
+			$hash = $1;
+			next;
+		}
+		next unless s{^\s+(git-svn-id:)}{$1};
+		my ($url, $rev, $uuid) = extract_metadata($_);
 		if (defined $url && defined $rev) {
 			if (my $gs = Git::SVN->find_by_url($url)) {
 				my $c = $gs->rev_db_get($rev);
@@ -1964,16 +1969,19 @@ sub rebuild {
 		return;
 	}
 	print "Rebuilding $db_path ...\n";
-	my ($rev_list, $ctx) = command_output_pipe("rev-list", $self->refname);
+	my ($log, $ctx) = command_output_pipe("log", $self->refname);
 	my $latest;
 	my $full_url = $self->full_url;
 	remove_username($full_url);
 	my $svn_uuid;
-	while (<$rev_list>) {
-		chomp;
-		my $c = $_;
-		die "Non-SHA1: $c\n" unless $c =~ /^$::sha1$/o;
-		my ($url, $rev, $uuid) = ::cmt_metadata($c);
+	my $c;
+	while (<$log>) {
+		if ( m{^commit ($::sha1)$} ) {
+			$c = $1;
+			next;
+		}
+		next unless s{^\s*(git-svn-id:)}{$1};
+		my ($url, $rev, $uuid) = ::extract_metadata($_);
 		remove_username($url);
 
 		# ignore merges (from set-tree)
@@ -1991,7 +1999,7 @@ sub rebuild {
 		$self->rev_db_set($rev, $c);
 		print "r$rev = $c\n";
 	}
-	command_close_pipe($rev_list, $ctx);
+	command_close_pipe($log, $ctx);
 	print "Done rebuilding $db_path\n";
 }
 
-- 
1.5.0.4.210.gf8a7c-dirty

Re: [PATCH] git-svn: use git-log rather than rev-list | xargs cat-file

[Eric Wong]

Sam Vilain <sam@vilain.net> wrote:
> This saves a bit of time when rebuilding the git-svn index.

Does git-log still have the 16k buffer limit?  If so then we can't use
it because commit messages over 16k will be truncated and the git-svn-id
line will not show up.  Also, if that limit is removed I'd prefer to
just add --pretty=raw to rev-list because git-log is stil porcelain and
more likely to change.

> Signed-off-by: Sam Vilain <sam@vilain.net>
> ---
>  git-svn.perl |   30 +++++++++++++++++++-----------
>  1 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index e350061..610563c 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -802,10 +802,15 @@ sub cmt_metadata {
>  
>  sub working_head_info {
>  	my ($head, $refs) = @_;
> -	my ($fh, $ctx) = command_output_pipe('rev-list', $head);
> -	while (my $hash = <$fh>) {
> -		chomp($hash);
> -		my ($url, $rev, $uuid) = cmt_metadata($hash);
> +	my ($fh, $ctx) = command_output_pipe('log', $head);
> +	my $hash;
> +	while (<$fh>) {
> +		if ( m{^commit ($::sha1)$} ) {
> +			$hash = $1;
> +			next;
> +		}
> +		next unless s{^\s+(git-svn-id:)}{$1};
> +		my ($url, $rev, $uuid) = extract_metadata($_);
>  		if (defined $url && defined $rev) {
>  			if (my $gs = Git::SVN->find_by_url($url)) {
>  				my $c = $gs->rev_db_get($rev);
> @@ -1964,16 +1969,19 @@ sub rebuild {
>  		return;
>  	}
>  	print "Rebuilding $db_path ...\n";
> -	my ($rev_list, $ctx) = command_output_pipe("rev-list", $self->refname);
> +	my ($log, $ctx) = command_output_pipe("log", $self->refname);
>  	my $latest;
>  	my $full_url = $self->full_url;
>  	remove_username($full_url);
>  	my $svn_uuid;
> -	while (<$rev_list>) {
> -		chomp;
> -		my $c = $_;
> -		die "Non-SHA1: $c\n" unless $c =~ /^$::sha1$/o;
> -		my ($url, $rev, $uuid) = ::cmt_metadata($c);
> +	my $c;
> +	while (<$log>) {
> +		if ( m{^commit ($::sha1)$} ) {
> +			$c = $1;
> +			next;
> +		}
> +		next unless s{^\s*(git-svn-id:)}{$1};
> +		my ($url, $rev, $uuid) = ::extract_metadata($_);
>  		remove_username($url);
>  
>  		# ignore merges (from set-tree)
> @@ -1991,7 +1999,7 @@ sub rebuild {
>  		$self->rev_db_set($rev, $c);
>  		print "r$rev = $c\n";
>  	}
> -	command_close_pipe($rev_list, $ctx);
> +	command_close_pipe($log, $ctx);
>  	print "Done rebuilding $db_path\n";
>  }
>  
> -- 
> 1.5.0.4.210.gf8a7c-dirty
> 

-- 
Eric Wong

Re: [PATCH] git-svn: use git-log rather than rev-list | xargs cat-file

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Sam Vilain <sam@vilain.net> wrote:
>> This saves a bit of time when rebuilding the git-svn index.
>
> Does git-log still have the 16k buffer limit?  If so then we can't use
> it because commit messages over 16k will be truncated and the git-svn-id
> line will not show up.  Also, if that limit is removed I'd prefer to
> just add --pretty=raw to rev-list because git-log is stil porcelain and
> more likely to change.

How about this?  It passes the test suite, but other than that
hasn't seen much test yet.  I tried to be careful, but sanity
checking by extra sets of eyeballs would be needed.

It changes length from int unsigned long in several places, and
you would need to look out for a boolean test like this:

	if (current_length < limit_length - slop)
        	... do something ...

as it now should be written like this:

	if (current_length + slop < limit_length)
        	... do something ...

-- >8 --
Subject: Lift 16kB limit of log message output

Traditionally we had 16kB limit when formatting log messages for
output, because it was easier to arrange for the caller to have
a reasonably big buffer and pass it down without ever worrying
about reallocating.

This changes the calling convention of pretty_print_commit() to
lift this limit.  Instead of the buffer and remaining length, it
now takes a pointer to the pointer that points at the allocated
buffer, and another pointer to the location that stores the
allocated length, and reallocates the buffer as necessary.

To support the user format, the error return of interpolate()
needed to be changed.  It used to return a bool telling "Ok the
result fits", or "Sorry, I had to truncate it".  Now it returns
0 on success, and returns the size of the buffer it wants in
order to fit the whole result.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 builtin-branch.c      |   17 +++++++++-----
 builtin-log.c         |    6 +++-
 builtin-rev-list.c    |    8 ++++--
 builtin-show-branch.c |   23 +++++++++++---------
 commit.c              |   55 ++++++++++++++++++++++++++++++++++++++----------
 commit.h              |    2 +-
 interpolate.c         |   46 ++++++++++++++++++----------------------
 interpolate.h         |    6 ++--
 log-tree.c            |   35 ++++++++++++++++++++----------
 9 files changed, 124 insertions(+), 74 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index da48051..d7c321a 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -242,7 +242,6 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	char c;
 	int color;
 	struct commit *commit;
-	char subject[256];
 
 	switch (item->kind) {
 	case REF_LOCAL_BRANCH:
@@ -263,17 +262,23 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	}
 
 	if (verbose) {
+		char *subject = NULL;
+		unsigned long subject_len = 0;
+		const char *sub = " **** invalid ref ****";
+
 		commit = lookup_commit(item->sha1);
-		if (commit && !parse_commit(commit))
+		if (commit && !parse_commit(commit)) {
 			pretty_print_commit(CMIT_FMT_ONELINE, commit, ~0,
-					    subject, sizeof(subject), 0,
+					    &subject, &subject_len, 0,
 					    NULL, NULL, 0);
-		else
-			strcpy(subject, " **** invalid ref ****");
+			sub = subject;
+		}
 		printf("%c %s%-*s%s %s %s\n", c, branch_get_color(color),
 		       maxwidth, item->name,
 		       branch_get_color(COLOR_BRANCH_RESET),
-		       find_unique_abbrev(item->sha1, abbrev), subject);
+		       find_unique_abbrev(item->sha1, abbrev), sub);
+		if (subject)
+			free(subject);
 	} else {
 		printf("%c %s%s%s\n", c, branch_get_color(color), item->name,
 		       branch_get_color(COLOR_BRANCH_RESET));
diff --git a/builtin-log.c b/builtin-log.c
index 0aede76..b9035ab 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -742,11 +742,13 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 			sign = '-';
 
 		if (verbose) {
-			static char buf[16384];
+			char *buf = NULL;
+			unsigned long buflen = 0;
 			pretty_print_commit(CMIT_FMT_ONELINE, commit, ~0,
-			                    buf, sizeof(buf), 0, NULL, NULL, 0);
+			                    &buf, &buflen, 0, NULL, NULL, 0);
 			printf("%c %s %s\n", sign,
 			       sha1_to_hex(commit->object.sha1), buf);
+			free(buf);
 		}
 		else {
 			printf("%c %s\n", sign,
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index ebf53f5..813aadf 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -92,11 +92,13 @@ static void show_commit(struct commit *commit)
 		putchar('\n');
 
 	if (revs.verbose_header) {
-		static char pretty_header[16384];
+		char *buf = NULL;
+		unsigned long buflen = 0;
 		pretty_print_commit(revs.commit_format, commit, ~0,
-				    pretty_header, sizeof(pretty_header),
+				    &buf, &buflen,
 				    revs.abbrev, NULL, NULL, revs.date_mode);
-		printf("%s%c", pretty_header, hdr_termination);
+		printf("%s%c", buf, hdr_termination);
+		free(buf);
 	}
 	fflush(stdout);
 	if (commit->parents) {
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index c892f1f..4fa87f6 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -259,17 +259,19 @@ static void join_revs(struct commit_list **list_p,
 
 static void show_one_commit(struct commit *commit, int no_name)
 {
-	char pretty[256], *cp;
+	char *pretty = NULL;
+	const char *pretty_str = "(unavailable)";
+	unsigned long pretty_len = 0;
 	struct commit_name *name = commit->util;
-	if (commit->object.parsed)
+
+	if (commit->object.parsed) {
 		pretty_print_commit(CMIT_FMT_ONELINE, commit, ~0,
-				    pretty, sizeof(pretty), 0, NULL, NULL, 0);
-	else
-		strcpy(pretty, "(unavailable)");
-	if (!prefixcmp(pretty, "[PATCH] "))
-		cp = pretty + 8;
-	else
-		cp = pretty;
+				    &pretty, &pretty_len,
+				    0, NULL, NULL, 0);
+		pretty_str = pretty;
+	}
+	if (!prefixcmp(pretty_str, "[PATCH] "))
+		pretty_str += 8;
 
 	if (!no_name) {
 		if (name && name->head_name) {
@@ -286,7 +288,8 @@ static void show_one_commit(struct commit *commit, int no_name)
 			printf("[%s] ",
 			       find_unique_abbrev(commit->object.sha1, 7));
 	}
-	puts(cp);
+	puts(pretty_str);
+	free(pretty);
 }
 
 static char *ref_name[MAX_REVS + 1];
diff --git a/commit.c b/commit.c
index 4ca4d44..d43a68e 100644
--- a/commit.c
+++ b/commit.c
@@ -776,7 +776,7 @@ static void fill_person(struct interp *table, const char *msg, int len)
 }
 
 static long format_commit_message(const struct commit *commit,
-		const char *msg, char *buf, unsigned long space)
+		const char *msg, char **buf_p, unsigned long *space_p)
 {
 	struct interp table[] = {
 		{ "%H" },	/* commit hash */
@@ -905,16 +905,27 @@ static long format_commit_message(const struct commit *commit,
 		if (!table[i].value)
 			interp_set_entry(table, i, "<unknown>");
 
-	interpolate(buf, space, user_format, table, ARRAY_SIZE(table));
+	do {
+		char *buf = *buf_p;
+		unsigned long space = *space_p;
+
+		space = interpolate(buf, space, user_format,
+				    table, ARRAY_SIZE(table));
+		if (!space)
+			break;
+		buf = xrealloc(buf, space);
+		*buf_p = buf;
+		*space_p = space;
+	} while (1);
 	interp_clear_table(table, ARRAY_SIZE(table));
 
-	return strlen(buf);
+	return strlen(*buf_p);
 }
 
 unsigned long pretty_print_commit(enum cmit_fmt fmt,
 				  const struct commit *commit,
 				  unsigned long len,
-				  char *buf, unsigned long space,
+				  char **buf_p, unsigned long *space_p,
 				  int abbrev, const char *subject,
 				  const char *after_subject,
 				  enum date_mode dmode)
@@ -927,9 +938,11 @@ unsigned long pretty_print_commit(enum cmit_fmt fmt,
 	int plain_non_ascii = 0;
 	char *reencoded;
 	const char *encoding;
+	char *buf;
+	unsigned long space, slop;
 
 	if (fmt == CMIT_FMT_USERFORMAT)
-		return format_commit_message(commit, msg, buf, space);
+		return format_commit_message(commit, msg, buf_p, space_p);
 
 	encoding = (git_log_output_encoding
 		    ? git_log_output_encoding
@@ -969,6 +982,26 @@ unsigned long pretty_print_commit(enum cmit_fmt fmt,
 		}
 	}
 
+	space = *space_p;
+	buf = *buf_p;
+
+	/*
+	 * We do not want to repeatedly realloc below, so
+	 * preallocate with enough slop to hold MIME headers,
+	 * "Subject: " prefix, etc.
+	 */
+	slop = 1000;
+	if (subject)
+		slop += strlen(subject);
+	if (after_subject)
+		slop += strlen(after_subject);
+	if (space < strlen(msg) + slop) {
+		space = strlen(msg) + slop;
+		buf = xrealloc(buf, space);
+		*space_p = space;
+		*buf_p = buf;
+	}
+
 	for (;;) {
 		const char *line = msg;
 		int linelen = get_one_line(msg, len);
@@ -976,14 +1009,12 @@ unsigned long pretty_print_commit(enum cmit_fmt fmt,
 		if (!linelen)
 			break;
 
-		/*
-		 * We want some slop for indentation and a possible
-		 * final "...". Thus the "+ 20".
-		 */
+		/* 20 would cover indent and leave us some slop */
 		if (offset + linelen + 20 > space) {
-			memcpy(buf + offset, "    ...\n", 8);
-			offset += 8;
-			break;
+			space = offset + linelen + 20;
+			buf = xrealloc(buf, space);
+			*buf_p = buf;
+			*space_p = space;
 		}
 
 		msg += linelen;
diff --git a/commit.h b/commit.h
index a313b53..467872e 100644
--- a/commit.h
+++ b/commit.h
@@ -61,7 +61,7 @@ enum cmit_fmt {
 };
 
 extern enum cmit_fmt get_commit_format(const char *arg);
-extern unsigned long pretty_print_commit(enum cmit_fmt fmt, const struct commit *, unsigned long len, char *buf, unsigned long space, int abbrev, const char *subject, const char *after_subject, enum date_mode dmode);
+extern unsigned long pretty_print_commit(enum cmit_fmt fmt, const struct commit *, unsigned long len, char **buf_p, unsigned long *space_p, int abbrev, const char *subject, const char *after_subject, enum date_mode dmode);
 
 /** Removes the first commit from a list sorted by date, and adds all
  * of its parents.
diff --git a/interpolate.c b/interpolate.c
index fb30694..0082677 100644
--- a/interpolate.c
+++ b/interpolate.c
@@ -44,33 +44,33 @@ void interp_clear_table(struct interp *table, int ninterps)
  *        { "%%", "%"},
  *    }
  *
- * Returns 1 on a successful substitution pass that fits in result,
- * Returns 0 on a failed or overflowing substitution pass.
+ * Returns 0 on a successful substitution pass that fits in result,
+ * Returns a number of bytes needed to hold the full substituted
+ * string otherwise.
  */
 
-int interpolate(char *result, int reslen,
+unsigned long interpolate(char *result, unsigned long reslen,
 		const char *orig,
 		const struct interp *interps, int ninterps)
 {
 	const char *src = orig;
 	char *dest = result;
-	int newlen = 0;
+	unsigned long newlen = 0;
 	const char *name, *value;
-	int namelen, valuelen;
+	unsigned long namelen, valuelen;
 	int i;
 	char c;
 
         memset(result, 0, reslen);
 
-	while ((c = *src) && newlen < reslen - 1) {
+	while ((c = *src)) {
 		if (c == '%') {
 			/* Try to match an interpolation string. */
 			for (i = 0; i < ninterps; i++) {
 				name = interps[i].name;
 				namelen = strlen(name);
-				if (strncmp(src, name, namelen) == 0) {
+				if (strncmp(src, name, namelen) == 0)
 					break;
-				}
 			}
 
 			/* Check for valid interpolation. */
@@ -78,29 +78,25 @@ int interpolate(char *result, int reslen,
 				value = interps[i].value;
 				valuelen = strlen(value);
 
-				if (newlen + valuelen < reslen - 1) {
+				if (newlen + valuelen + 1 < reslen) {
 					/* Substitute. */
 					strncpy(dest, value, valuelen);
-					newlen += valuelen;
 					dest += valuelen;
-					src += namelen;
-				} else {
-					/* Something's not fitting. */
-					return 0;
 				}
-
-			} else {
-				/* Skip bogus interpolation. */
-				*dest++ = *src++;
-				newlen++;
+				newlen += valuelen;
+				src += namelen;
+				continue;
 			}
-
-		} else {
-			/* Straight copy one non-interpolation character. */
-			*dest++ = *src++;
-			newlen++;
 		}
+		/* Straight copy one non-interpolation character. */
+		if (newlen + 1 < reslen)
+			*dest++ = *src;
+		src++;
+		newlen++;
 	}
 
-	return newlen < reslen - 1;
+	if (newlen + 1 < reslen)
+		return 0;
+	else
+		return newlen + 2;
 }
diff --git a/interpolate.h b/interpolate.h
index 16a26b9..77407e6 100644
--- a/interpolate.h
+++ b/interpolate.h
@@ -19,8 +19,8 @@ struct interp {
 extern void interp_set_entry(struct interp *table, int slot, const char *value);
 extern void interp_clear_table(struct interp *table, int ninterps);
 
-extern int interpolate(char *result, int reslen,
-		       const char *orig,
-		       const struct interp *interps, int ninterps);
+extern unsigned long interpolate(char *result, unsigned long reslen,
+				 const char *orig,
+				 const struct interp *interps, int ninterps);
 
 #endif /* INTERPOLATE_H */
diff --git a/log-tree.c b/log-tree.c
index 4bef909..0cf21bc 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -79,16 +79,25 @@ static int detect_any_signoff(char *letter, int size)
 	return seen_head && seen_name;
 }
 
-static int append_signoff(char *buf, int buf_sz, int at, const char *signoff)
+static unsigned long append_signoff(char **buf_p, unsigned long *buf_sz_p,
+				    unsigned long at, const char *signoff)
 {
 	static const char signed_off_by[] = "Signed-off-by: ";
-	int signoff_len = strlen(signoff);
+	size_t signoff_len = strlen(signoff);
 	int has_signoff = 0;
-	char *cp = buf;
-
-	/* Do we have enough space to add it? */
-	if (buf_sz - at <= strlen(signed_off_by) + signoff_len + 3)
-		return at;
+	char *cp;
+	char *buf;
+	unsigned long buf_sz;
+
+	buf = *buf_p;
+	buf_sz = *buf_sz_p;
+	if (buf_sz <= at + strlen(signed_off_by) + signoff_len + 3) {
+		buf_sz += strlen(signed_off_by) + signoff_len + 3;
+		buf = xrealloc(buf, buf_sz);
+		*buf_p = buf;
+		*buf_sz_p = buf_sz;
+	}
+	cp = buf;
 
 	/* First see if we already have the sign-off by the signer */
 	while ((cp = strstr(cp, signed_off_by))) {
@@ -133,7 +142,8 @@ static unsigned int digits_in_number(unsigned int number)
 
 void show_log(struct rev_info *opt, const char *sep)
 {
-	static char this_header[16384];
+	char *msgbuf = NULL;
+	unsigned long msgbuf_len = 0;
 	struct log_info *log = opt->loginfo;
 	struct commit *commit = log->commit, *parent = log->parent;
 	int abbrev = opt->diffopt.abbrev;
@@ -278,14 +288,15 @@ void show_log(struct rev_info *opt, const char *sep)
 	/*
 	 * And then the pretty-printed message itself
 	 */
-	len = pretty_print_commit(opt->commit_format, commit, ~0u, this_header,
-				  sizeof(this_header), abbrev, subject,
+	len = pretty_print_commit(opt->commit_format, commit, ~0u,
+				  &msgbuf, &msgbuf_len, abbrev, subject,
 				  extra_headers, opt->date_mode);
 
 	if (opt->add_signoff)
-		len = append_signoff(this_header, sizeof(this_header), len,
+		len = append_signoff(&msgbuf, &msgbuf_len, len,
 				     opt->add_signoff);
-	printf("%s%s%s", this_header, extra, sep);
+	printf("%s%s%s", msgbuf, extra, sep);
+	free(msgbuf);
 }
 
 int log_tree_diff_flush(struct rev_info *opt)

Re: [PATCH] git-svn: use git-log rather than rev-list | xargs cat-file

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <normalperson@yhbt.net> writes:
> 
> > Sam Vilain <sam@vilain.net> wrote:
> >> This saves a bit of time when rebuilding the git-svn index.
> >
> > Does git-log still have the 16k buffer limit?  If so then we can't use
> > it because commit messages over 16k will be truncated and the git-svn-id
> > line will not show up.  Also, if that limit is removed I'd prefer to
> > just add --pretty=raw to rev-list because git-log is stil porcelain and
> > more likely to change.
> 
> How about this?  It passes the test suite, but other than that
> hasn't seen much test yet.  I tried to be careful, but sanity
> checking by extra sets of eyeballs would be needed.

The patch looks and runs alright to me, but then again I haven't looked
at the C portions of git in a while :x

I expected the malloc/free overhead to be much greater, but it's hardly
noticeable (nor measureable with /usr/bin/time or bash built-in time).
There are just a handful more pagefaults measured with /usr/bin/time,
but the runtime performance is neck-and-neck with/without the patch.

Maybe glibc (2.3.6 on x86 Debian Etch) and Linux (2.6.18) are just doing
a very good job with memory allocation... I wonder how well it runs on
other platforms.

-- 
Eric Wong