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-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

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

[Jason Sewall]

On 6/28/07, Sam Vilain <sam.vilain@catalyst.net.nz> wrote:
> There was emerge already but I much prefer this mode.
>

I beat ya to it: http://marc.info/?l=git&m=118301192520295&w=2

But it looks like maybe you did a better job (updated docs, for
example). Other than that, it's almost exactly the same.

Ack.

Jason

P.S.

doing this:
>      if echo "${VISUAL:-$EDITOR}" | grep 'emacs' > /dev/null 2>&1; then
>         merge_tool_candidates="$merge_tool_candidates emerge ediff"
>      fi

and then this

>     merge_tool_candidates="$merge_tool_candidates opendiff ediff emerge vimdiff"

makes this

>      echo "merge tool candidates: $merge_tool_candidates"

print out emerge and ediff twice, presumably because we're adding it
in for both "visual" emacs and "regular" (i.e. -nw) emacs. I suck at
shell scripts, so I'm probably missing something but what why do we
have all of that testing for emacs + vim if we just add their tools
anyway right afterwards?

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

[Theodore Tso]

On Thu, Jun 28, 2007 at 06:31:50PM -0700, Jason Sewall wrote:
> >     echo "merge tool candidates: $merge_tool_candidates"

This was a debugging echo that slipped by; I had never intended for it
to be kept.

> print out emerge and ediff twice, presumably because we're adding it
> in for both "visual" emacs and "regular" (i.e. -nw) emacs. I suck at
> shell scripts, so I'm probably missing something but what why do we
> have all of that testing for emacs + vim if we just add their tools
> anyway right afterwards?

Some things get added twice but in a different order because the
search order matters.  But in terms of adding emerge and ediff, yes,
there's no point, since they always get added in the same order.  

I'll have to look at the two and see why people like one over the
other, and then we'll have to pick which one should be the default.
Although as I've said, past a certain point people should just put
their personal preference in .gitconfig.

						- Ted

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

[Theodore Tso]

On Fri, Jun 29, 2007 at 12:03:28AM -0400, Theodore Tso wrote:
> I'll have to look at the two and see why people like one over the
> other, and then we'll have to pick which one should be the default.
> Although as I've said, past a certain point people should just put
> their personal preference in .gitconfig.

After looking at ediff, it is definitely the more polished and
featureful compared to emerge --- except in one critical area, which
is calling as a mergeing tool from a shell script or command line.
Ediff fundamentally assumes that it fired off from inside an emacs
environment, whereas emerge is much friendly as an external merge
program. 

This can be shown in the relatively easy way emerge can be run from
the command-line:

	emacs -f emerge-files-with-ancestor-command "$LOCAL" "$REMOTE" "$BASE" "$path"

... where as with ediff, you have to run it this way:

	emacs --eval "(ediff-merge-files-with-ancestor \"$LOCAL\" \"$REMOTE\" \"$BASE\" nil \"$path\")"

Unfortunately, it's not enough.  Ediff doesn't have an "abort" command
which returns a non-zero exit status, and when you use the "quit"
command, it asks you a series of obnoxious questions:

Quit this Ediff session? (y or n)
File /usr/projects/git/test/testfile.c exists, overwrite? (y or n)
Merge buffer saved in /usr/projects/git/test/testfile.c
<delay for 3 annoying seconds>
Merge buffer saved.  Now kill the buffer? (y or n)

... and then it leaves you in the emacs window, and you have to type
^X^C by hand.

So while ediff is more featureful, its integration is so lacking that
it is incredibly annoying to use.

Which leaves us with the interesting question.  We could just
integrate it, but not make it the default (the above makes ediff just
far too annoying for a user who is not expecting it).  

Alternatively, we could patch around the problem.  The following emacs
lisp code fixes the ediff issues:

(defun ediff-write-merge-buffer ()
  (let ((file ediff-merge-store-file))
    (set-buffer ediff-buffer-C)
    (write-region (point-min) (point-max) file)
    (message "Merge buffer saved in: %s" file)
    (set-buffer-modified-p nil)
    (sit-for 1)))

(setq ediff-quit-hook 'kill-emacs
      ediff-quit-merge-hook 'ediff-write-merge-buffer)

But the only clean way of adding that to git-mergetool would be something like this:

	emacs --eval "(progn (defun ediff-write-merge-buffer () (let ((file ediff-merge-store-file)) (set-buffer ediff-buffer-C) (write-region (point-min) (point-max) file) (message \"Merge buffer saved in: %s\" file) (set-buffer-modified-p nil) (sit-for 1))) (setq ediff-quit-hook 'kill-emacs ediff-quit-merge-hook 'ediff-write-merge-buffer) (ediff-merge-files-with-ancestor \"$LOCAL\" \"$REMOTE\" \"$BASE\" nil \"$path\")"

But that seems too ugly to live, and it could break in the future if
ediff ever changes some of its internal variables.


Alternatively, we could file a bug report with the ediff folks, and
request that they add an 'ediff-files-with-ancestor-command and
'ediff-files-command just as emerge does.  The problem with that
approach is that ediff is shipped with emacs, and emacs has a release
cycle measured in **years**.


So my current thinking is that ediff will *not* be the default for
git-mergetool if emacs is present, and that emerge will be used for
now, because of these problems.

Comments?

						- Ted

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

[Junio C Hamano]

Theodore Tso <tytso@mit.edu> writes:

> Unfortunately, it's not enough.  Ediff doesn't have an "abort" command
> which returns a non-zero exit status, and when you use the "quit"
> command, it asks you a series of obnoxious questions:
>
> ...
> Alternatively, we could patch around the problem.  The following emacs
> lisp code fixes the ediff issues:

But that would be changing the behaviour globally, and not
limited to the particular session invoked from git-mergetool,
wouldn't it?  If that is the case it would be a hard sell to
Emacs users, especially the ones that keep their Emacs running
forever and have emacsclient as their EDITOR, I would think.

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

[Theodore Tso]

On Sun, Jul 01, 2007 at 07:32:59PM -0700, Junio C Hamano wrote:
> Theodore Tso <tytso@mit.edu> writes:
> 
> > Unfortunately, it's not enough.  Ediff doesn't have an "abort" command
> > which returns a non-zero exit status, and when you use the "quit"
> > command, it asks you a series of obnoxious questions:
> >
> > ...
> > Alternatively, we could patch around the problem.  The following emacs
> > lisp code fixes the ediff issues:
> 
> But that would be changing the behaviour globally, and not
> limited to the particular session invoked from git-mergetool,
> wouldn't it?  If that is the case it would be a hard sell to
> Emacs users, especially the ones that keep their Emacs running
> forever and have emacsclient as their EDITOR, I would think.

The emacs lisp code I gave there was the minimal necessary so it
could be passed on the command-line; I was trying to keep it small.

Obviously, the patch that would have to get sent to the ediff folks
would have to be much more generalized --- in fact, probably the right
thing to do is to send a full patch that actually implemented
ediff-merge-files-command and ediff-merge-files-with-ancestoers-commands.

As far as people using emacsclient as their editor, it would be simple
enough to have the emacs lisp code test to see if
server-buffer-clients is non-nill; if it is, then we know that this
merge request was trigered by emacsclient, and so (server-done) should
be called instead of (kill-emacs).  Emerge does not do this; arguably
this is a bug in emerge.

The other way we could deal with this problem is to fire up a separate
emacs even if EDITOR is emacsclient, on the theory that
EDITOR=emacsclient meants that the user prefers emacs, but it doesn't
necessarily mean that we have to *use* emacsclient, especially when
emerge currently doesn't DTRT with emacsclient.

One thing that did cross my mind is that we could put code which
patched ediff.el and emerge.el in /usr/share/git/lisp/... and then
passed called emacs with something like this "emacs -l
$sharedir/lisp/ediff-patches.el ...".  But this implies packaging
emacs lisp files with git, and I'm not at ALL sure we want to go
there.  Personally, I still like kdiff3 as my personal favorite
mergetool, and given that emacs starts up pretty fast these days, I've
given up on emacsclient, but I know there are certainly people who use
them.

(Mmmm...., I just pulled down an early emacs 23 snapshot with Xft
support enabled, so I can enjoy the anti-aliased font goodness.  Even
with all of the Gtk and Xft bloat, the emacs 23 snapshot is still
quick snappy to fire up.)

					- Ted

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

[Junio C Hamano]

Theodore Tso <tytso@mit.edu> writes:

> One thing that did cross my mind is that we could put code which
> patched ediff.el and emerge.el in /usr/share/git/lisp/... and then
> passed called emacs with something like this "emacs -l
> $sharedir/lisp/ediff-patches.el ...".  But this implies packaging
> emacs lisp files with git, and I'm not at ALL sure we want to go
> there. ...

I hope not.

> ...  Personally, I still like kdiff3 as my personal favorite
> mergetool, and given that emacs starts up pretty fast these days, I've
> given up on emacsclient, but I know there are certainly people who use
> them.

The reason I personally use emacsclient is not about the
start-up delay, but with the access to existing buffers,
keyboard macros, Gnus buffers, ... IOW the access to the
"session" while editing.  I suspect people with long running
Emacs session use emacsclient for that reason.

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

[Theodore Tso]

On Sun, Jul 01, 2007 at 09:49:10PM -0700, Junio C Hamano wrote:
> The reason I personally use emacsclient is not about the
> start-up delay, but with the access to existing buffers,
> keyboard macros, Gnus buffers, ... IOW the access to the
> "session" while editing.  I suspect people with long running
> Emacs session use emacsclient for that reason.

Sure, but do you need access to existing buffers, keyboard, macros,
etc., if you're simply firing up an emacs to handle a merge conflict?
If the goal is just to run a merge application, then firing up a
separate process makes a lot more sense.

One other thing which I just noticed is that emacs21's emacsclient
does NOT support the -f or -e option.  And a lot of people may still
be using emacs21.  So in any case, at the moment we are in fact using
to fire up a separate process when using emerge or ediff.  I suppose
we could try testing to see if the user is running emacs21 or emacs22
if EDITOR==emacsclient, but there's no easy way of doing this short of
doing something heavyweight such as firing up emacs and asking to eval
some lisp that prints the value of emacs-version to stdout.  And even
then we would have to fix emerge to do the right thing when invoked
via emacsclient.  Yuck...

This still leaves us with the question about whether the following to
fix ediff is acceptable:

   	  emacs --eval "(progn (defun ediff-write-merge-buffer () (let ((file ediff-merge-store-file)) (set-buffer ediff-buffer-C) (write-region (point-min) (point-max) file) (message \"Merge buffer saved in: %s\" file) (set-buffer-modified-p nil) (sit-for 1))) (setq ediff-quit-hook 'kill-emacs ediff-quit-merge-hook 'ediff-write-merge-buffer) (ediff-merge-files-with-ancestor \"$LOCAL\" \"$REMOTE\" \"$BASE\" nil \"$path\"))"

In my mind it's on the hairy edge. Alternatively we just never use
ediff by default, and assume that either expert users can hack their
.emacs.el file to have the right overrides will use ediff, or who are
willing to put up with ediff's user-hostile approach to quitting an
merge session.

						 -  Ted

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

[Junio C Hamano]

Theodore Tso <tytso@mit.edu> writes:

> On Sun, Jul 01, 2007 at 09:49:10PM -0700, Junio C Hamano wrote:
>> The reason I personally use emacsclient is not about the
>> start-up delay, but with the access to existing buffers,
>> keyboard macros, Gnus buffers, ... IOW the access to the
>> "session" while editing.  I suspect people with long running
>> Emacs session use emacsclient for that reason.
>
> Sure, but do you need access to existing buffers, keyboard, macros,
> etc., if you're simply firing up an emacs to handle a merge conflict?
>
> If the goal is just to run a merge application, then firing up a
> separate process makes a lot more sense.

Existing buffers may help somewhat as I am likely to have that
already loaded, but other than that probably not.

> In my mind it's on the hairy edge. Alternatively we just never use
> ediff by default, and assume that either expert users can hack their
> .emacs.el file to have the right overrides will use ediff, or who are
> willing to put up with ediff's user-hostile approach to quitting an
> merge session.

I think that is sane.

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

[Sam Vilain]

Theodore Tso wrote:
> After looking at ediff, it is definitely the more polished and
> featureful compared to emerge --- except in one critical area, which
> is calling as a mergeing tool from a shell script or command line.
  [...]
> 	emacs --eval "(ediff-merge-files-with-ancestor \"$LOCAL\" \"$REMOTE\" \"$BASE\" nil \"$path\")"
> 
> Unfortunately, it's not enough.  Ediff doesn't have an "abort" command
> which returns a non-zero exit status, and when you use the "quit"
> command, it asks you a series of obnoxious questions:
> 
> Quit this Ediff session? (y or n)
> File /usr/projects/git/test/testfile.c exists, overwrite? (y or n)
> Merge buffer saved in /usr/projects/git/test/testfile.c
> <delay for 3 annoying seconds>
> Merge buffer saved.  Now kill the buffer? (y or n)

Yeah, I normally just save the merged buffer and quit.  This skips all that.

But I will add your little snippet to my .emacs :)

Sam.

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

[Theodore Tso]

On Tue, Jul 03, 2007 at 09:32:34AM +1200, Sam Vilain wrote:
> > Unfortunately, it's not enough.  Ediff doesn't have an "abort" command
> > which returns a non-zero exit status, and when you use the "quit"
> > command, it asks you a series of obnoxious questions:
> > 
> > Quit this Ediff session? (y or n)
> > File /usr/projects/git/test/testfile.c exists, overwrite? (y or n)
> > Merge buffer saved in /usr/projects/git/test/testfile.c
> > <delay for 3 annoying seconds>
> > Merge buffer saved.  Now kill the buffer? (y or n)
> 
> Yeah, I normally just save the merged buffer and quit.  This skips all that.
> 
> But I will add your little snippet to my .emacs :)

You probably don't want to just add that snippet to your .emacs, since
it changes the ediff 'quit' command to always cause emacs to
immediately exit, and that's probably not the right thing if you are
starting ediff from an emacs session.

The correct fix would involve stealing code from emerge's
emerge-merge-files-command function to parse the arguments from the
command-line --- and in fact, probably the simplest way of fixing
things for folks would be to write replacement emerge-*-command
functions which call ediff after patching the ediff hooks in the
emacs-lisp fragment I sent above.

In fact, maybe that's the right approach.  I don't think we want to
ship emacs lisp files which git-mergetool depends upon, but what if we
instead ship some emacs lisp code in the contrib directory which a
user could slip into their .emacs file which replaces the two
emerge-*-command functions which ones that call ediff instead?

That way we don't have all of this complexity added into git-mergetool.

		 	   	      	   - Ted

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

[Theodore Tso]

OK, so I've hacked together the following emacs-lisp snippet, which I
propose would go in contrib/use-ediff-instead.el.  If placed in your
.emacs.el file, it will cause you to use ediff instead of emerge when
you call "git mergetool".  It does so by replacing the two functions
emerge-files-command and emerge-files-with-ancestor-comand with ones
that patch the necessary ediff hooks, and then calling the ediff
package instead of the emerge package.

With this .el file, no changes are needed to git-mergetool.sh.  Does
this meet your needs?

					- Ted

;; use-ediff-instead.el
;;
;; This emacs lisp snippet should be placed in your .emacs.el file in
;; order to use the ediff package instead of emerge for git-mergetool.
;; Ediff has more whiz-bang features, but unfortunately it doesn't
;; integrate well with shell scripts that try to invoke ediff from an
;; emacs shell invocation.

(defun ediff-write-merge-buffer ()
  (let ((file ediff-merge-store-file))
    (set-buffer ediff-buffer-C)
    (write-region (point-min) (point-max) file)
    (message "Merge buffer saved in: %s" file)
    (set-buffer-modified-p nil)
    (sit-for 1)))

(defun emerge-files-command ()
  (let ((file-a (nth 0 command-line-args-left))
	(file-b (nth 1 command-line-args-left))
	(file-out (nth 2 command-line-args-left)))
    (setq command-line-args-left (nthcdr 3 command-line-args-left))
    (setq ediff-quit-hook 'kill-emacs
	  ediff-quit-merge-hook 'ediff-write-merge-buffer)
    (ediff-merge-files file-a file-b  nil file-out)))

(defun emerge-files-with-ancestor-command ()
  (let (file-a file-b file-anc file-out)
    ;; check for a -a flag, for filemerge compatibility
    (if (string= (car command-line-args-left) "-a")
	;; arguments are "-a ancestor file-a file-b file-out"
	(progn
	  (setq file-a (nth 2 command-line-args-left))
	  (setq file-b (nth 3 command-line-args-left))
	  (setq file-anc (nth 1 command-line-args-left))
	  (setq file-out (nth 4 command-line-args-left))
	  (setq command-line-args-left (nthcdr 5 command-line-args-left)))
        ;; arguments are "file-a file-b ancestor file-out"
        (setq file-a (nth 0 command-line-args-left))
        (setq file-b (nth 1 command-line-args-left))
        (setq file-anc (nth 2 command-line-args-left))
        (setq file-out (nth 3 command-line-args-left))
        (setq command-line-args-left (nthcdr 4 command-line-args-left)))
    (setq ediff-quit-hook 'kill-emacs
	  ediff-quit-merge-hook 'ediff-write-merge-buffer)
    (ediff-merge-files-with-ancestor file-a file-b file-anc nil file-out)))

;; End of use-ediff-instead.el

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

[Sam Vilain]

Theodore Tso wrote:
> OK, so I've hacked together the following emacs-lisp snippet, which I
> propose would go in contrib/use-ediff-instead.el.  If placed in your
> .emacs.el file, it will cause you to use ediff instead of emerge when
> you call "git mergetool".  It does so by replacing the two functions
> emerge-files-command and emerge-files-with-ancestor-comand with ones
> that patch the necessary ediff hooks, and then calling the ediff
> package instead of the emerge package.
> 
> With this .el file, no changes are needed to git-mergetool.sh.  Does
> this meet your needs?
> 
> 					- Ted
> 
> ;; use-ediff-instead.el
 [...]

Thanks for that, it mostly works, however it doesn't seem to notice if I
abort without making the merge complete (on emacs21).  In my smartmerge
script (http://utsl.gen.nz/scripts/smartmerge) I detect this condition
based on the presence of merge markers, possibly dubious but pragmatic.

I still don't really understand why having to save the merged buffer and
exit is such a huge issue.  Already I have to select "-t emerge" to get
emerge.  I would have thought it would be better to just make the other
mode available, and let the user figure it out.

Sam.

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

[Theodore Tso]

On Tue, Jul 03, 2007 at 11:19:49AM +1200, Sam Vilain wrote:
> Thanks for that, it mostly works, however it doesn't seem to notice if I
> abort without making the merge complete (on emacs21).  In my smartmerge
> script (http://utsl.gen.nz/scripts/smartmerge) I detect this condition
> based on the presence of merge markers, possibly dubious but pragmatic.

Hmm, well, here's a way of fixing it.  (See attached, below.)  It adds
a new command 'x', which when you hit it in the ediff control window,
exits with a error status of '1', indicating that the merge has
failed.  This is something which emerge, kdiff3, tkdiff, et. al all
support; but which ediff doesn't.

> I still don't really understand why having to save the merged buffer and
> exit is such a huge issue.  Already I have to select "-t emerge" to get
> emerge.  I would have thought it would be better to just make the other
> mode available, and let the user figure it out.

I'm just exploring alternatives.  Basically, it just seems interesting
that ediff has a lot of nice features, but also has some incredibly
user-hostile features.  The first time I tried using ediff, I indeed
tried saving the buffer and exiting it.  That's when I discovered that
after I changed the focus to the merge window and saved it, when I
tried typing ^X^C, the exit failed with the error message "Attempt to
delete a surrogate minibuffer frame".  That's the sort of thing that
will cause non-elisp programmers to run screaming off into the
distance.

So if you are going to save the merge the buffer and exit, you *have*
to use the 'q' command, and endure the loads of stupid questions
issued by ediff, OR, you can discover that ^X^C in the ediff control
window doesn't actually cause emacs to exit, but it does make the
ediff control window go away.  (Which is another insane bit of ediff's
UI design... why should ^X^C do something completely different in the
ediff control window?!?)

So yeah, we can add ediff as an optional support that people have to
explicitly request, but quite frankly, having played with it, I don't
know why anyone would use it without a huge number of fix ups, which
is why I was trying to make ediff actually be usable for someone who
doesn't mind typing ^X^C twice, for no good reason, after figuring out
that this illogical thing is what you actually need to do to exit
ediff.  (I actually read the help text first, so I got treated to the
really annoying ediff-quit behavior before I figured out the double
^X^C trick.)

						- Ted

;; use-ediff-instead.el
;;
;; This emacs lisp snippet should be placed in your .emacs.el file in
;; order to use the ediff package instead of emerge for git-mergetool.
;; Ediff has more whiz-bang features, but unfortunately it doesn't
;; integrate well with shell scripts that try to invoke ediff from an
;; emacs shell invocation.  This script tries to address these problems.

(defun ediff-write-merge-buffer ()
  (let ((file ediff-merge-store-file))
    (set-buffer ediff-buffer-C)
    (write-region (point-min) (point-max) file)
    (message "Merge buffer saved in: %s" file)
    (set-buffer-modified-p nil)
    (sit-for 1)))

(defun ediff-abort ()
  "Abort the ediff session without a non-zero exit status"
  (interactive)
  (kill-emacs 1))

(defun ediff-setup-abort ()
  (define-key ediff-mode-map "x" 'ediff-abort))

(defun emerge-files-command ()
  (let ((file-a (nth 0 command-line-args-left))
	(file-b (nth 1 command-line-args-left))
	(file-out (nth 2 command-line-args-left)))
    (setq command-line-args-left (nthcdr 3 command-line-args-left))
    (setq ediff-quit-hook 'kill-emacs
	  ediff-quit-merge-hook 'ediff-write-merge-buffer
	  ediff-keymap-setup-hook 'ediff-setup-abort)
    (ediff-merge-files file-a file-b  nil file-out)))

(defun emerge-files-with-ancestor-command ()
  (let (file-a file-b file-anc file-out)
    ;; check for a -a flag, for filemerge compatibility
    (if (string= (car command-line-args-left) "-a")
	;; arguments are "-a ancestor file-a file-b file-out"
	(progn
	  (setq file-a (nth 2 command-line-args-left))
	  (setq file-b (nth 3 command-line-args-left))
	  (setq file-anc (nth 1 command-line-args-left))
	  (setq file-out (nth 4 command-line-args-left))
	  (setq command-line-args-left (nthcdr 5 command-line-args-left)))
        ;; arguments are "file-a file-b ancestor file-out"
        (setq file-a (nth 0 command-line-args-left))
        (setq file-b (nth 1 command-line-args-left))
        (setq file-anc (nth 2 command-line-args-left))
        (setq file-out (nth 3 command-line-args-left))
        (setq command-line-args-left (nthcdr 4 command-line-args-left)))
    (setq ediff-quit-hook 'kill-emacs
	  ediff-quit-merge-hook 'ediff-write-merge-buffer
	  ediff-keymap-setup-hook 'ediff-setup-abort)
    (ediff-merge-files-with-ancestor file-a file-b file-anc nil file-out)))

;; End of use-ediff-instead.el

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

[Sam Vilain]

Theodore Tso wrote:
> I'm just exploring alternatives.  Basically, it just seems interesting
> that ediff has a lot of nice features, but also has some incredibly
> user-hostile features.  The first time I tried using ediff, I indeed
> tried saving the buffer and exiting it.  That's when I discovered that
> after I changed the focus to the merge window and saved it, when I
> tried typing ^X^C, the exit failed with the error message "Attempt to
> delete a surrogate minibuffer frame".  That's the sort of thing that
> will cause non-elisp programmers to run screaming off into the
> distance.

Ouch.  Yes, I've never seen that before and no doubt if I had've I'd
feel the same way.  I just save the merge buffer and quit, and it is
pretty obedient for me.

However I guess it wouldn't be nice to have a merge mode that did not
work out of the box for a large number of users.

Your .el file certainly does the trick for me - I reckon throw it in
contrib/

Sam.

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

[David Kastrup]

[Picking up an old thread]

Theodore Tso <tytso@mit.edu> writes:

> On Tue, Jul 03, 2007 at 11:19:49AM +1200, Sam Vilain wrote:
>
> Hmm, well, here's a way of fixing it.  (See attached, below.)  It
> adds a new command 'x', which when you hit it in the ediff control
> window, exits with a error status of '1', indicating that the merge
> has failed.  This is something which emerge, kdiff3, tkdiff, et. al
> all support; but which ediff doesn't.
>
>> I still don't really understand why having to save the merged buffer and
>> exit is such a huge issue.  Already I have to select "-t emerge" to get
>> emerge.  I would have thought it would be better to just make the other
>> mode available, and let the user figure it out.
>
> I'm just exploring alternatives.  Basically, it just seems
> interesting that ediff has a lot of nice features, but also has some
> incredibly user-hostile features.  The first time I tried using
> ediff, I indeed tried saving the buffer and exiting it.  That's when
> I discovered that after I changed the focus to the merge window and
> saved it, when I tried typing ^X^C, the exit failed with the error
> message "Attempt to delete a surrogate minibuffer frame".  That's
> the sort of thing that will cause non-elisp programmers to run
> screaming off into the distance.

Ted, I think you are somewhat missing the main audience here.  The
main audience are people who actually _use_ Emacs, and those will be
comfortable with the concept "save to have changes persist, don't save
if you don't want changes to persist, exit using C-x # or C-x C-c as
appropriate".  Basically, it would appear that you try figuring out
how to make ediff appeal to non-Emacs users.  But those would not have
emacs/emacsclient in their EDITOR variable in the first place.

I have been bitten by mergetool calling emacs rather than emacsclient,
resulting in a non-working merge (since the default directory was set
differently from what the call expected due to my use of the desktop
package), and mergetool afterwards assuming that the not-even-started
merge was successful.  A royal nuisance, and completely unworkable.

While it may be nice to have some Lisp preparation for people who
don't want to touch or learn Emacs _except_ for using it for merging
in git, I think we should first cater to people actually using Emacs
already.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

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

[Theodore Tso]

On Sat, Jul 28, 2007 at 11:22:43AM +0200, David Kastrup wrote:
> Ted, I think you are somewhat missing the main audience here.  The
> main audience are people who actually _use_ Emacs, and those will be
> comfortable with the concept "save to have changes persist, don't save
> if you don't want changes to persist, exit using C-x # or C-x C-c as
> appropriate".  Basically, it would appear that you try figuring out
> how to make ediff appeal to non-Emacs users.  But those would not have
> emacs/emacsclient in their EDITOR variable in the first place.
> 
> I have been bitten by mergetool calling emacs rather than emacsclient,
> resulting in a non-working merge (since the default directory was set
> differently from what the call expected due to my use of the desktop
> package), and mergetool afterwards assuming that the not-even-started
> merge was successful.  A royal nuisance, and completely unworkable.

Emacsclient is a completely different problem, or at least adds a
whole new dimention, compared to the ediff/emerge issue.  You can't
run either emerge or ediff using the emacsclient in emacs21, since it
lacks support for either the -e or the -f command-line option.  All
you can do in emacs21 when using eamcsclient is to request emacs to
edit a file.  

One of the problems with emacs is that it is so customizable that
people can set up emacs in such a way that different ways of launching
emacs may lead to surprises, thanks to their .emacs21.  This makes
supporting emacs based merging clients to be highly problematic.  Use
of the desktop package is one way in which things can be quite
surprising.  Worse yet, the desktop package is only in emacs22 and up.
(And emacs 22 was *just* released, not all that long ago; many people
may still be using emacs21).  So if we use emacs --no-desktop to
disable the desktop package, it will cause emacs21 to complain about
an unknown option.  Joy.  Which means that to avoid running into
problems with emacs22 users who are using the desktop package,
git-mergetool is going to have to find out in advance whether emacs21
or emacs22 (or an emacs development 23.0.0 snapshot) is in use; on a
debian system you can have 3 or 4 emacs installed simultaneously.  What fun.

In any case, the main issue is that there is an emerging (sorry)
standard about how merge tools are supposed to work, in terms of being
able to support 2-way or 3-way merges, about being able to specify
which file (and which file only, in the best case) should be used as
the output file as the result of the merge, and about how tools can
signal either a successful merge, or a request by the user to abort
the merge becuase things didn't work out for one reason or another.

The problem is that ediff doesn't really fit this model.  For people
who really want to live their life in emacs, and using emacs as their
desktop (not for me, but maybe for some folks), maybe it would be
better for those folks to simply build a git-mergetool.el that ran
100% in emacs, instead of trying to shift back and forth between the
command-line and emacs, would make everyone happier.  Right now
git-mergetool needs to ask questions about the disposition of
symlinks, permission changes, etc.  If it is done as a
git-mergetool.el which is tied into git.el and ediff, it could be a
lot more seamless.

> While it may be nice to have some Lisp preparation for people who
> don't want to touch or learn Emacs _except_ for using it for merging
> in git, I think we should first cater to people actually using Emacs
> already.

Catering to the hard-core Emacs folks is *hard*.  I knew someone who
had PDP-10 assembly language in their .emacs.el file, and one day his
custom emacs extension worked again when he started playing with the
KLH10 PDP-10 emulator, and reused his .emacs.el startup file there....
Of course, at some level folks like that will always need to fend for
themselves.

As I said earlier, I don't have a huge objection to support ediff in
some degraded mode (I think the UI is ghastly bad), if users
explicitly request it, but I would *not* want to make it the default
and spring it on some unsuspecting user.  Quite frankly, right now the
KDE and GNOME tools are way better either emerge or ediff, so they are
only really useful as a default in the terminal-only case.

     	    	      		       - Ted

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

[David Kastrup]

Theodore Tso <tytso@mit.edu> writes:

> On Sat, Jul 28, 2007 at 11:22:43AM +0200, David Kastrup wrote:

>> Ted, I think you are somewhat missing the main audience here.  The
>> main audience are people who actually _use_ Emacs, and those will
>> be comfortable with the concept "save to have changes persist,
>> don't save if you don't want changes to persist, exit using C-x #
>> or C-x C-c as appropriate".  Basically, it would appear that you
>> try figuring out how to make ediff appeal to non-Emacs users.  But
>> those would not have emacs/emacsclient in their EDITOR variable in
>> the first place.
>> 
>> I have been bitten by mergetool calling emacs rather than
>> emacsclient, resulting in a non-working merge (since the default
>> directory was set differently from what the call expected due to my
>> use of the desktop package), and mergetool afterwards assuming that
>> the not-even-started merge was successful.  A royal nuisance, and
>> completely unworkable.
>
> Emacsclient is a completely different problem, or at least adds a
> whole new dimention, compared to the ediff/emerge issue.  You can't
> run either emerge or ediff using the emacsclient in emacs21, since
> it lacks support for either the -e or the -f command-line option.

If the user asks for it, we should try giving it to him.  If
Emacsclient bombs out because of a non-understood option, one can
still fall back to calling a separate Emacs.

> All you can do in emacs21 when using eamcsclient is to request emacs
> to edit a file.

Yes, but emacsclient --version returns a version string, and
emacsclient will exit with an error if it can't get to understand the
command line options or to talk with Emacs.  So there are reasonably
ways to notice when to fallback.

> One of the problems with emacs is that it is so customizable that
> people can set up emacs in such a way that different ways of
> launching emacs may lead to surprises, thanks to their .emacs21.
> This makes supporting emacs based merging clients to be highly
> problematic.  Use of the desktop package is one way in which things
> can be quite surprising.  Worse yet, the desktop package is only in
> emacs22 and up.

The desktop package has already been in Emacs 21, so it is not exactly
a new problem but has been round for more than 7 years.

> (And emacs 22 was *just* released, not all that long ago; many
> people may still be using emacs21).  So if we use emacs --no-desktop
> to disable the desktop package, it will cause emacs21 to complain
> about an unknown option.  Joy.

Correct: the --no-desktop option is new.

> Which means that to avoid running into problems with emacs22 users
> who are using the desktop package, git-mergetool is going to have to
> find out in advance whether emacs21 or emacs22 (or an emacs
> development 23.0.0 snapshot) is in use; on a debian system you can
> have 3 or 4 emacs installed simultaneously.  What fun.

$EDITOR --version

> In any case, the main issue is that there is an emerging (sorry)
> standard about how merge tools are supposed to work, in terms of
> being able to support 2-way or 3-way merges, about being able to
> specify which file (and which file only, in the best case) should be
> used as the output file as the result of the merge, and about how
> tools can signal either a successful merge, or a request by the user
> to abort the merge becuase things didn't work out for one reason or
> another.
>
> The problem is that ediff doesn't really fit this model.

Emacs is an editor.  If we can't make an editor fit into merge
resolution, we have a design problem.  It is a matter of convenience
that the editor is called with some initial files and something like
ediff-whatever, but the end result clearly should be that the user
writes the file if he wants changes to persist, and doesn't if
doesn't.

> For people who really want to live their life in emacs, and using
> emacs as their desktop (not for me, but maybe for some folks), maybe
> it would be better for those folks to simply build a
> git-mergetool.el that ran 100% in emacs, instead of trying to shift
> back and forth between the command-line and emacs, would make
> everyone happier.  Right now git-mergetool needs to ask questions
> about the disposition of symlinks, permission changes, etc.  If it
> is done as a git-mergetool.el which is tied into git.el and ediff,
> it could be a lot more seamless.

But this is no reason not to fix the currently broken behavior.  If
you insist that "emerge" or "ediff" is _not_ to be used as an editor,
but rather as a special-purpose mergetool for the sake of git, then
the only logical conclusion can be to call it with "-q", bypassing any
user initialization.

I believe this would be a mistake at least when $EDITOR points to
Emacs, because this means the user is used to using Emacs/Emacsclient
as _editor_, and anything else would be _confusing_.

>> While it may be nice to have some Lisp preparation for people who
>> don't want to touch or learn Emacs _except_ for using it for
>> merging in git, I think we should first cater to people actually
>> using Emacs already.
>
> Catering to the hard-core Emacs folks is *hard*.

Saving a desktop session is not hard-core.  Using emacsclient is not
hard-core.  Those are standard, basic, use cases.

> I knew someone who had PDP-10 assembly language in their .emacs.el
> file, and one day his custom emacs extension worked again when he
> started playing with the KLH10 PDP-10 emulator, and reused his
> .emacs.el startup file there....

Can we get another strawman a bit closer to the main road, please?

> Of course, at some level folks like that will always need to fend
> for themselves.

Yes.  I am not talking about people breaking things by code of their
own.  I am talking about a _standard_ setup being broken by git.

> As I said earlier, I don't have a huge objection to support ediff in
> some degraded mode (I think the UI is ghastly bad), if users
> explicitly request it, but I would *not* want to make it the default
> and spring it on some unsuspecting user.  Quite frankly, right now
> the KDE and GNOME tools are way better either emerge or ediff, so
> they are only really useful as a default in the terminal-only case.

Again, you fall into the trap of not allowing others to have a life
and Emacs outside of git's preconceptions.  emerge and ediff might be
worse for people who would not use Emacs for anything but merging.
But one advantage of Emacs is that I can look at all sorts of other
files and buffers and information sources _while_ I am merging, and
declare the merge finished when _I_ want it (signaled by exiting
either Emacs or Emacsclient), not when some arbitrary command thinks
it finished.

Emacs is one of the most flexible tools ever.  Disallowing any editing
use not foreseen and sanctioned by git-mergetool is _always_ going to
lead to trouble.  If you do _anything_ like this, you _must_ call
Emacs -q in order to omit _any_ user initializations you did not
foresee.  But this will also kill user-specific major modes which he
might want to use for visualizing files.  It will be less onerous than
having all hell break lose because you can't cater for even standard
initializations or setup, but it will still be a nuisance.

So please don't try crippling Emacs into a git-only tool.  Call it
with the files in question, give it an appropriate initial command to
work with if possible, and leave the rest to the user.  He will save
and finish, or not save and finish, which is the way of an editor to
communicate with its environment.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum