Git development
 help / color / mirror / Atom feed
* Updates to the latest {diff,merge}tool series
From: David Aguilar @ 2009-04-07  8:21 UTC (permalink / raw)
  To: gitster; +Cc: git, charles, markus.heidelberg

Here they are.

07/14: Updated usage synopsis
09/14: Updated diff.tool config.txt documentation
10/14: The original broke the test suite on Mac OS...
       Testing {diff,merge}tool.<tool>.path is tricky.
11/14: This depends on 07/14, so the rebase requires a resend
14/14: Reworked logic as discussed.

^ permalink raw reply

* [PATCH v2 09/14] difftool: move 'git-difftool' out of contrib
From: David Aguilar @ 2009-04-07  8:21 UTC (permalink / raw)
  To: gitster; +Cc: git, charles, markus.heidelberg, David Aguilar
In-Reply-To: <1239092483-14973-2-git-send-email-davvid@gmail.com>

This prepares 'git-difftool' and its documentation for
mainstream use.

'git-difftool-helper' became 'git-difftool--helper'
since users should not use it directly.

'git-difftool' was added to the list of commands as
an ancillaryinterrogator.

Signed-off-by: David Aguilar <davvid@gmail.com>
---

Includes suggestions from Markus

 .gitignore                                         |    2 ++
 Documentation/config.txt                           |   18 ++++++++++++++++++
 .../difftool => Documentation}/git-difftool.txt    |    0
 Makefile                                           |    2 ++
 command-list.txt                                   |    1 +
 .../git-difftool-helper => git-difftool--helper.sh |    2 +-
 contrib/difftool/git-difftool => git-difftool.perl |    6 +++---
 7 files changed, 27 insertions(+), 4 deletions(-)
 rename {contrib/difftool => Documentation}/git-difftool.txt (100%)
 rename contrib/difftool/git-difftool-helper => git-difftool--helper.sh (98%)
 rename contrib/difftool/git-difftool => git-difftool.perl (92%)

diff --git a/.gitignore b/.gitignore
index 1c57d4c..a36da9d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -35,6 +35,8 @@ git-diff
 git-diff-files
 git-diff-index
 git-diff-tree
+git-difftool
+git-difftool--helper
 git-describe
 git-fast-export
 git-fast-import
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3afd124..94ef1a6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -667,6 +667,24 @@ diff.suppressBlankEmpty::
 	A boolean to inhibit the standard behavior of printing a space
 	before each empty output line. Defaults to false.
 
+diff.tool::
+	Controls which diff tool is used.  `diff.tool` overrides
+	`merge.tool` when used by linkgit:git-difftool[1] and has
+	the same valid values as `merge.tool` minus "tortoisemerge"
+	and plus "kompare".
+
+difftool.<tool>.path::
+	Override the path for the given tool.  This is useful in case
+	your tool is not in the PATH.
+
+difftool.<tool>.cmd::
+	Specify the command to invoke the specified diff tool.
+	The specified command is evaluated in shell with the following
+	variables available:  'LOCAL' is set to the name of the temporary
+	file containing the contents of the diff pre-image and 'REMOTE'
+	is set to the name of the temporary file containing the contents
+	of the diff post-image.
+
 diff.wordRegex::
 	A POSIX Extended Regular Expression used to determine what is a "word"
 	when performing word-by-word difference calculations.  Character
diff --git a/contrib/difftool/git-difftool.txt b/Documentation/git-difftool.txt
similarity index 100%
rename from contrib/difftool/git-difftool.txt
rename to Documentation/git-difftool.txt
diff --git a/Makefile b/Makefile
index 7867eac..a80055f 100644
--- a/Makefile
+++ b/Makefile
@@ -277,6 +277,7 @@ TEST_PROGRAMS =
 
 SCRIPT_SH += git-am.sh
 SCRIPT_SH += git-bisect.sh
+SCRIPT_SH += git-difftool--helper.sh
 SCRIPT_SH += git-filter-branch.sh
 SCRIPT_SH += git-lost-found.sh
 SCRIPT_SH += git-merge-octopus.sh
@@ -296,6 +297,7 @@ SCRIPT_SH += git-submodule.sh
 SCRIPT_SH += git-web--browse.sh
 
 SCRIPT_PERL += git-add--interactive.perl
+SCRIPT_PERL += git-difftool.perl
 SCRIPT_PERL += git-archimport.perl
 SCRIPT_PERL += git-cvsexportcommit.perl
 SCRIPT_PERL += git-cvsimport.perl
diff --git a/command-list.txt b/command-list.txt
index 3583a33..fb03a2e 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -33,6 +33,7 @@ git-diff                                mainporcelain common
 git-diff-files                          plumbinginterrogators
 git-diff-index                          plumbinginterrogators
 git-diff-tree                           plumbinginterrogators
+git-difftool                            ancillaryinterrogators
 git-fast-export				ancillarymanipulators
 git-fast-import				ancillarymanipulators
 git-fetch                               mainporcelain common
diff --git a/contrib/difftool/git-difftool-helper b/git-difftool--helper.sh
similarity index 98%
rename from contrib/difftool/git-difftool-helper
rename to git-difftool--helper.sh
index 4b0daec..fc61416 100755
--- a/contrib/difftool/git-difftool-helper
+++ b/git-difftool--helper.sh
@@ -1,5 +1,5 @@
 #!/bin/sh
-# git-difftool-helper is a GIT_EXTERNAL_DIFF-compatible diff tool launcher.
+# git-difftool--helper is a GIT_EXTERNAL_DIFF-compatible diff tool launcher.
 # This script is typically launched by using the 'git difftool'
 # convenience command.
 #
diff --git a/contrib/difftool/git-difftool b/git-difftool.perl
similarity index 92%
rename from contrib/difftool/git-difftool
rename to git-difftool.perl
index 8c160e5..8857ac8 100755
--- a/contrib/difftool/git-difftool
+++ b/git-difftool.perl
@@ -2,9 +2,9 @@
 # Copyright (c) 2009 David Aguilar
 #
 # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
-# git-difftool-helper script.  This script exports
+# git-difftool--helper script.  This script exports
 # GIT_EXTERNAL_DIFF and GIT_PAGER for use by git, and
-# GIT_DIFFTOOL_NO_PROMPT and GIT_DIFF_TOOL for use by git-difftool-helper.
+# GIT_DIFFTOOL_NO_PROMPT and GIT_DIFF_TOOL for use by git-difftool--helper.
 # Any arguments that are unknown to this script are forwarded to 'git diff'.
 
 use strict;
@@ -27,7 +27,7 @@ sub setup_environment
 {
 	$ENV{PATH} = "$DIR:$ENV{PATH}";
 	$ENV{GIT_PAGER} = '';
-	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool-helper';
+	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
 }
 
 sub exe
-- 
1.6.2.2.414.g81aa9

^ permalink raw reply related

* [PATCH v2 07/14] difftool: add a -y shortcut for --no-prompt
From: David Aguilar @ 2009-04-07  8:21 UTC (permalink / raw)
  To: gitster; +Cc: git, charles, markus.heidelberg, David Aguilar
In-Reply-To: <1239092483-14973-1-git-send-email-davvid@gmail.com>

This is another consistency cleanup to make git-difftool's options
match git-mergetool.

Signed-off-by: David Aguilar <davvid@gmail.com>
---

Includes suggestions from Markus

 contrib/difftool/git-difftool     |    6 +++---
 contrib/difftool/git-difftool.txt |   36 ++++++++++++++----------------------
 2 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/contrib/difftool/git-difftool b/contrib/difftool/git-difftool
index 207dd50..8c160e5 100755
--- a/contrib/difftool/git-difftool
+++ b/contrib/difftool/git-difftool
@@ -18,7 +18,7 @@ my $DIR = abs_path(dirname($0));
 sub usage
 {
 	print << 'USAGE';
-usage: git difftool [--tool=<tool>] [--no-prompt] ["git diff" options]
+usage: git difftool [--tool=<tool>] [-y|--no-prompt] ["git diff" options]
 USAGE
 	exit 1;
 }
@@ -60,11 +60,11 @@ sub generate_command
 			$ENV{GIT_DIFF_TOOL} = substr($arg, 7);
 			next;
 		}
-		if ($arg eq '--no-prompt') {
+		if ($arg eq '-y' || $arg eq '--no-prompt') {
 			$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
 			next;
 		}
-		if ($arg eq '-h' or $arg eq '--help') {
+		if ($arg eq '-h' || $arg eq '--help') {
 			usage();
 		}
 		push @command, $arg;
diff --git a/contrib/difftool/git-difftool.txt b/contrib/difftool/git-difftool.txt
index 2b7bc03..a00e943 100644
--- a/contrib/difftool/git-difftool.txt
+++ b/contrib/difftool/git-difftool.txt
@@ -3,35 +3,32 @@ git-difftool(1)
 
 NAME
 ----
-git-difftool - compare changes using common merge tools
+git-difftool - Show changes using common diff tools
 
 SYNOPSIS
 --------
-'git difftool' [--tool=<tool>] [--no-prompt] ['git diff' options]
+'git difftool' [--tool=<tool>] [-y|--no-prompt] [<'git diff' options>]
 
 DESCRIPTION
 -----------
 'git-difftool' is a git command that allows you to compare and edit files
-between revisions using common merge tools.  At its most basic level,
-'git-difftool' does what 'git-mergetool' does but its use is for non-merge
-situations such as when preparing commits or comparing changes against
-the index.
-
-'git difftool' is a frontend to 'git diff' and accepts the same
-arguments and options.
-
-See linkgit:git-diff[1] for the full list of supported options.
+between revisions using common diff tools.  'git difftool' is a frontend
+to 'git-diff' and accepts the same options and arguments.
 
 OPTIONS
 -------
+-y::
+--no-prompt::
+	Do not prompt before launching a diff tool.
+
 -t <tool>::
 --tool=<tool>::
-	Use the merge resolution program specified by <tool>.
+	Use the diff tool specified by <tool>.
 	Valid merge tools are:
 	kdiff3, kompare, tkdiff, meld, xxdiff, emerge,
 	vimdiff, gvimdiff, ecmerge, and opendiff
 +
-If a merge resolution program is not specified, 'git-difftool'
+If a diff tool is not specified, 'git-difftool'
 will use the configuration variable `diff.tool`.  If the
 configuration variable `diff.tool` is not set, 'git-difftool'
 will pick a suitable default.
@@ -42,7 +39,7 @@ can configure the absolute path to kdiff3 by setting
 `difftool.kdiff3.path`. Otherwise, 'git-difftool' assumes the
 tool is available in PATH.
 +
-Instead of running one of the known merge tool programs,
+Instead of running one of the known diff tools,
 'git-difftool' can be customized to run an alternative program
 by specifying the command line to invoke in a configuration
 variable `difftool.<tool>.cmd`.
@@ -56,8 +53,7 @@ is set to the name of the temporary file containing the contents
 of the diff post-image.  `$BASE` is provided for compatibility
 with custom merge tool commands and has the same value as `$LOCAL`.
 
---no-prompt::
-	Do not prompt before launching a diff tool.
+See linkgit:git-diff[1] for the full list of supported options.
 
 CONFIG VARIABLES
 ----------------
@@ -65,21 +61,17 @@ CONFIG VARIABLES
 difftool equivalents have not been defined.
 
 diff.tool::
-	The default merge tool to use.
+	The default diff tool to use.
 
 difftool.<tool>.path::
 	Override the path for the given tool.  This is useful in case
 	your tool is not in the PATH.
 
 difftool.<tool>.cmd::
-	Specify the command to invoke the specified merge tool.
+	Specify the command to invoke the specified diff tool.
 +
 See the `--tool=<tool>` option above for more details.
 
-merge.keepBackup::
-	The original, unedited file content can be saved to a file with
-	a `.orig` extension.  Defaults to `true` (i.e. keep the backup files).
-
 SEE ALSO
 --------
 linkgit:git-diff[1]::
-- 
1.6.2.2.414.g81aa9

^ permalink raw reply related

* [PATCH v2 10/14] difftool: add various git-difftool tests
From: David Aguilar @ 2009-04-07  8:21 UTC (permalink / raw)
  To: gitster; +Cc: git, charles, markus.heidelberg, David Aguilar
In-Reply-To: <1239092483-14973-3-git-send-email-davvid@gmail.com>

t7800-difftool.sh tests the various command-line flags,
git-config variables, and environment settings supported by
git-difftool.

Signed-off-by: David Aguilar <davvid@gmail.com>
---

The original broke the test suite on Mac OS.
Testing difftool.<path>.path is hard without internal support
for a fake tool.

 t/t7800-difftool.sh |  139 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 139 insertions(+), 0 deletions(-)
 create mode 100755 t/t7800-difftool.sh

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
new file mode 100755
index 0000000..ceef84b
--- /dev/null
+++ b/t/t7800-difftool.sh
@@ -0,0 +1,139 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 David Aguilar
+#
+
+test_description='git-difftool
+
+Testing basic diff tool invocation
+'
+
+. ./test-lib.sh
+
+remove_config_vars()
+{
+	# Unset all config variables used by git-difftool
+	git config --unset diff.tool
+	git config --unset difftool.test-tool.cmd
+	git config --unset merge.tool
+	git config --unset mergetool.test-tool.cmd
+	return 0
+}
+
+restore_test_defaults()
+{
+	# Restores the test defaults used by several tests
+	remove_config_vars
+	unset GIT_DIFF_TOOL
+	unset GIT_MERGE_TOOL
+	unset GIT_DIFFTOOL_NO_PROMPT
+	git config diff.tool test-tool &&
+	git config difftool.test-tool.cmd 'cat $LOCAL'
+}
+
+# Create a file on master and change it on branch
+test_expect_success 'setup' '
+	echo master >file &&
+	git add file &&
+	git commit -m "added file" &&
+
+	git checkout -b branch master &&
+	echo branch >file &&
+	git commit -a -m "branch changed file" &&
+	git checkout master
+'
+
+# Configure a custom difftool.<tool>.cmd and use it
+test_expect_success 'custom commands' '
+	restore_test_defaults &&
+	git config difftool.test-tool.cmd "cat \$REMOTE" &&
+
+	diff=$(git difftool --no-prompt branch) &&
+	test "$diff" = "master" &&
+
+	restore_test_defaults &&
+	diff=$(git difftool --no-prompt branch) &&
+	test "$diff" = "branch"
+'
+
+# Ensures that git-difftool ignores bogus --tool values
+test_expect_success 'difftool ignores bad --tool values' '
+	diff=$(git difftool --no-prompt --tool=bogus-tool branch)
+	test "$?" = 1 &&
+	test "$diff" = ""
+'
+
+# Specify the diff tool using $GIT_DIFF_TOOL
+test_expect_success 'GIT_DIFF_TOOL variable' '
+	git config --unset diff.tool
+	GIT_DIFF_TOOL=test-tool &&
+	export GIT_DIFF_TOOL &&
+
+	diff=$(git difftool --no-prompt branch) &&
+	test "$diff" = "branch" &&
+
+	restore_test_defaults
+'
+
+# Test the $GIT_*_TOOL variables and ensure
+# that $GIT_DIFF_TOOL always wins unless --tool is specified
+test_expect_success 'GIT_DIFF_TOOL overrides' '
+	git config diff.tool bogus-tool &&
+	git config merge.tool bogus-tool &&
+
+	GIT_MERGE_TOOL=test-tool &&
+	export GIT_MERGE_TOOL &&
+	diff=$(git difftool --no-prompt branch) &&
+	test "$diff" = "branch" &&
+	unset GIT_MERGE_TOOL &&
+
+	GIT_MERGE_TOOL=bogus-tool &&
+	GIT_DIFF_TOOL=test-tool &&
+	export GIT_MERGE_TOOL &&
+	export GIT_DIFF_TOOL &&
+
+	diff=$(git difftool --no-prompt branch) &&
+	test "$diff" = "branch" &&
+
+	GIT_DIFF_TOOL=bogus-tool &&
+	export GIT_DIFF_TOOL &&
+
+	diff=$(git difftool --no-prompt --tool=test-tool branch) &&
+	test "$diff" = "branch" &&
+
+	restore_test_defaults
+'
+
+# Test that we don't have to pass --no-prompt to difftool
+# when $GIT_DIFFTOOL_NO_PROMPT is true
+test_expect_success 'GIT_DIFFTOOL_NO_PROMPT variable' '
+	GIT_DIFFTOOL_NO_PROMPT=true &&
+	export GIT_DIFFTOOL_NO_PROMPT &&
+
+	diff=$(git difftool branch) &&
+	test "$diff" = "branch" &&
+
+	restore_test_defaults
+'
+
+# git-difftool falls back to git-mergetool config variables
+# so test that behavior here
+test_expect_success 'difftool + mergetool config variables' '
+	remove_config_vars
+	git config merge.tool test-tool &&
+	git config mergetool.test-tool.cmd "cat \$LOCAL" &&
+
+	diff=$(git difftool --no-prompt branch) &&
+	test "$diff" = "branch" &&
+
+	# set merge.tool to something bogus, diff.tool to test-tool
+	git config merge.tool bogus-tool &&
+	git config diff.tool test-tool &&
+
+	diff=$(git difftool --no-prompt branch) &&
+	test "$diff" = "branch" &&
+
+	restore_test_defaults
+'
+
+test_done
-- 
1.6.2.2.414.g81aa9

^ permalink raw reply related

* [PATCH v2 11/14] difftool: add support for a difftool.prompt config variable
From: David Aguilar @ 2009-04-07  8:21 UTC (permalink / raw)
  To: gitster; +Cc: git, charles, markus.heidelberg, David Aguilar
In-Reply-To: <1239092483-14973-4-git-send-email-davvid@gmail.com>

difftool now supports difftool.prompt so that users do not have to
pass --no-prompt or hit enter each time a diff tool is launched.
The --prompt flag overrides the configuration variable.

Signed-off-by: David Aguilar <davvid@gmail.com>
---

This one had to be resent since rewriting #9 would
have caused a conflict in here.

 Documentation/config.txt       |    3 ++
 Documentation/git-difftool.txt |   10 +++++-
 git-difftool--helper.sh        |   10 +++++-
 git-difftool.perl              |   15 +++++++--
 t/t7800-difftool.sh            |   64 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 96 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e8ff0be..6e29623 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -684,6 +684,9 @@ difftool.<tool>.cmd::
 	is set to the name of the temporary file containing the contents
 	of the diff post-image.
 
+difftool.prompt::
+	Prompt before each invocation of the diff tool.
+
 diff.wordRegex::
 	A POSIX Extended Regular Expression used to determine what is a "word"
 	when performing word-by-word difference calculations.  Character
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index af68466..15b247b 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -7,7 +7,7 @@ git-difftool - Show changes using common diff tools
 
 SYNOPSIS
 --------
-'git difftool' [--tool=<tool>] [-y|--no-prompt] [<'git diff' options>]
+'git difftool' [--tool=<tool>] [-y|--no-prompt|--prompt] [<'git diff' options>]
 
 DESCRIPTION
 -----------
@@ -21,6 +21,11 @@ OPTIONS
 --no-prompt::
 	Do not prompt before launching a diff tool.
 
+--prompt::
+	Prompt before each invocation of the diff tool.
+	This is the default behaviour; the option is provided to
+	override any configuration settings.
+
 -t <tool>::
 --tool=<tool>::
 	Use the diff tool specified by <tool>.
@@ -72,6 +77,9 @@ difftool.<tool>.cmd::
 +
 See the `--tool=<tool>` option above for more details.
 
+difftool.prompt::
+	Prompt before each invocation of the diff tool.
+
 SEE ALSO
 --------
 linkgit:git-diff[1]::
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index fc61416..f3c27d8 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -5,9 +5,15 @@
 #
 # Copyright (c) 2009 David Aguilar
 
-# Set GIT_DIFFTOOL_NO_PROMPT to bypass the per-file prompt.
+# difftool.prompt controls the default prompt/no-prompt behavior
+# and is overridden with $GIT_DIFFTOOL*_PROMPT.
 should_prompt () {
-	test -z "$GIT_DIFFTOOL_NO_PROMPT"
+	prompt=$(git config --bool difftool.prompt || echo true)
+	if test "$prompt" = true; then
+		test -z "$GIT_DIFFTOOL_NO_PROMPT"
+	else
+		test -n "$GIT_DIFFTOOL_PROMPT"
+	fi
 }
 
 # This function prepares temporary files and launches the appropriate
diff --git a/git-difftool.perl b/git-difftool.perl
index 8857ac8..948ff7f 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -2,9 +2,12 @@
 # Copyright (c) 2009 David Aguilar
 #
 # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
-# git-difftool--helper script.  This script exports
-# GIT_EXTERNAL_DIFF and GIT_PAGER for use by git, and
-# GIT_DIFFTOOL_NO_PROMPT and GIT_DIFF_TOOL for use by git-difftool--helper.
+# git-difftool--helper script.
+#
+# This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
+# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, and GIT_DIFF_TOOL
+# are exported for use by git-difftool--helper.
+#
 # Any arguments that are unknown to this script are forwarded to 'git diff'.
 
 use strict;
@@ -62,6 +65,12 @@ sub generate_command
 		}
 		if ($arg eq '-y' || $arg eq '--no-prompt') {
 			$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
+			delete $ENV{GIT_DIFFTOOL_PROMPT};
+			next;
+		}
+		if ($arg eq '--prompt') {
+			$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
+			delete $ENV{GIT_DIFFTOOL_NO_PROMPT};
 			next;
 		}
 		if ($arg eq '-h' || $arg eq '--help') {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 85946d9..0b263ea 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -15,6 +15,7 @@ remove_config_vars()
 	# Unset all config variables used by git-difftool
 	git config --unset diff.tool
 	git config --unset difftool.test-tool.cmd
+	git config --unset difftool.prompt
 	git config --unset merge.tool
 	git config --unset mergetool.test-tool.cmd
 	return 0
@@ -26,11 +27,18 @@ restore_test_defaults()
 	remove_config_vars
 	unset GIT_DIFF_TOOL
 	unset GIT_MERGE_TOOL
+	unset GIT_DIFFTOOL_PROMPT
 	unset GIT_DIFFTOOL_NO_PROMPT
 	git config diff.tool test-tool &&
 	git config difftool.test-tool.cmd 'cat $LOCAL'
 }
 
+prompt_given()
+{
+	prompt="$1"
+	test "$prompt" = "Hit return to launch 'test-tool': branch"
+}
+
 # Create a file on master and change it on branch
 test_expect_success 'setup' '
 	echo master >file &&
@@ -116,6 +124,62 @@ test_expect_success 'GIT_DIFFTOOL_NO_PROMPT variable' '
 	restore_test_defaults
 '
 
+# git-difftool supports the difftool.prompt variable.
+# Test that GIT_DIFFTOOL_PROMPT can override difftool.prompt = false
+test_expect_success 'GIT_DIFFTOOL_PROMPT variable' '
+	git config difftool.prompt false &&
+	GIT_DIFFTOOL_PROMPT=true &&
+	export GIT_DIFFTOOL_PROMPT &&
+
+	prompt=$(echo | git difftool --prompt branch | tail -1) &&
+	prompt_given "$prompt" &&
+
+	restore_test_defaults
+'
+
+# Test that we don't have to pass --no-prompt when difftool.prompt is false
+test_expect_success 'difftool.prompt config variable is false' '
+	git config difftool.prompt false &&
+
+	diff=$(git difftool branch) &&
+	test "$diff" = "branch" &&
+
+	restore_test_defaults
+'
+
+# Test that the -y flag can override difftool.prompt = true
+test_expect_success 'difftool.prompt can overridden with -y' '
+	git config difftool.prompt true &&
+
+	diff=$(git difftool -y branch) &&
+	test "$diff" = "branch" &&
+
+	restore_test_defaults
+'
+
+# Test that the --prompt flag can override difftool.prompt = false
+test_expect_success 'difftool.prompt can overridden with --prompt' '
+	git config difftool.prompt false &&
+
+	prompt=$(echo | git difftool --prompt branch | tail -1) &&
+	prompt_given "$prompt" &&
+
+	restore_test_defaults
+'
+
+# Test that the last flag passed on the command-line wins
+test_expect_success 'difftool last flag wins' '
+	diff=$(git difftool --prompt --no-prompt branch) &&
+	test "$diff" = "branch" &&
+
+	restore_test_defaults &&
+
+	prompt=$(echo | git difftool --no-prompt --prompt branch | tail -1) &&
+	prompt_given "$prompt" &&
+
+	restore_test_defaults
+'
+
 # git-difftool falls back to git-mergetool config variables
 # so test that behavior here
 test_expect_success 'difftool + mergetool config variables' '
-- 
1.6.2.2.414.g81aa9

^ permalink raw reply related

* [PATCH v3 14/14] difftool/mergetool: refactor commands to use git-mergetool--lib
From: David Aguilar @ 2009-04-07  8:21 UTC (permalink / raw)
  To: gitster; +Cc: git, charles, markus.heidelberg, David Aguilar
In-Reply-To: <1239092483-14973-5-git-send-email-davvid@gmail.com>

This consolidates the common functionality from git-mergetool and
git-difftool--helper into a single git-mergetool--lib scriptlet.

Signed-off-by: David Aguilar <davvid@gmail.com>
---

Includes suggestions from Markus.
We also moved opendiff up to the front so that it's
preferred when on Mac OS.

 .gitignore                           |    1 +
 Documentation/git-mergetool--lib.txt |   56 +++++
 Makefile                             |    1 +
 git-difftool--helper.sh              |  186 +----------------
 git-mergetool--lib.sh                |  387 ++++++++++++++++++++++++++++++++++
 git-mergetool.sh                     |  224 ++------------------
 6 files changed, 467 insertions(+), 388 deletions(-)
 create mode 100644 Documentation/git-mergetool--lib.txt
 create mode 100644 git-mergetool--lib.sh

diff --git a/.gitignore b/.gitignore
index a36da9d..757c7f0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -80,6 +80,7 @@ git-merge-recursive
 git-merge-resolve
 git-merge-subtree
 git-mergetool
+git-mergetool--lib
 git-mktag
 git-mktree
 git-name-rev
diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
new file mode 100644
index 0000000..3d57031
--- /dev/null
+++ b/Documentation/git-mergetool--lib.txt
@@ -0,0 +1,56 @@
+git-mergetool--lib(1)
+=====================
+
+NAME
+----
+git-mergetool--lib - Common git merge tool shell scriptlets
+
+SYNOPSIS
+--------
+'. "$(git --exec-path)/git-mergetool--lib"'
+
+DESCRIPTION
+-----------
+
+This is not a command the end user would want to run.  Ever.
+This documentation is meant for people who are studying the
+Porcelain-ish scripts and/or are writing new ones.
+
+The 'git-mergetool--lib' scriptlet is designed to be sourced (using
+`.`) by other shell scripts to set up functions for working
+with git merge tools.
+
+Before sourcing it, your script should set up a few variables;
+`TOOL_MODE` is used to define the operation mode for various
+functions.  'diff' and 'merge' are valid values.
+
+FUNCTIONS
+---------
+get_merge_tool::
+	returns a merge tool
+
+get_merge_tool_cmd::
+	returns the custom command for a merge tool.
+
+get_merge_tool_path::
+	returns the custom path for a merge tool.
+
+run_merge_tool::
+	launches a merge tool given the tool name and a true/false
+	flag to indicate whether a merge base is present.
+	'$merge_tool', '$merge_tool_path', and for custom commands,
+	'$merge_tool_cmd', must be defined prior to calling
+	run_merge_tool.  Additionally, '$MERGED', '$LOCAL', '$REMOTE',
+	and '$BASE' must be defined for use by the merge tool.
+
+Author
+------
+Written by David Aguilar <davvid@gmail.com>
+
+Documentation
+--------------
+Documentation by David Aguilar and the git-list <git@vger.kernel.org>.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index a80055f..3e56274 100644
--- a/Makefile
+++ b/Makefile
@@ -284,6 +284,7 @@ SCRIPT_SH += git-merge-octopus.sh
 SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
+SCRIPT_SH += git-mergetool--lib.sh
 SCRIPT_SH += git-parse-remote.sh
 SCRIPT_SH += git-pull.sh
 SCRIPT_SH += git-quiltimport.sh
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index f3c27d8..b450036 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -5,6 +5,10 @@
 #
 # Copyright (c) 2009 David Aguilar
 
+# Load common functions from git-mergetool--lib
+TOOL_MODE=diff
+. git-mergetool--lib
+
 # difftool.prompt controls the default prompt/no-prompt behavior
 # and is overridden with $GIT_DIFFTOOL*_PROMPT.
 should_prompt () {
@@ -16,8 +20,7 @@ should_prompt () {
 	fi
 }
 
-# This function prepares temporary files and launches the appropriate
-# merge tool.
+# Sets up shell variables and runs a merge tool
 launch_merge_tool () {
 	# Merged is the filename as it appears in the work tree
 	# Local is the contents of a/filename
@@ -37,187 +40,16 @@ launch_merge_tool () {
 	fi
 
 	# Run the appropriate merge tool command
-	case "$merge_tool" in
-	kdiff3)
-		basename=$(basename "$MERGED")
-		"$merge_tool_path" --auto \
-			--L1 "$basename (A)" \
-			--L2 "$basename (B)" \
-			"$LOCAL" "$REMOTE" \
-			> /dev/null 2>&1
-		;;
-
-	kompare)
-		"$merge_tool_path" "$LOCAL" "$REMOTE"
-		;;
-
-	tkdiff)
-		"$merge_tool_path" "$LOCAL" "$REMOTE"
-		;;
-
-	meld)
-		"$merge_tool_path" "$LOCAL" "$REMOTE"
-		;;
-
-	diffuse)
-		"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
-		;;
-
-	vimdiff)
-		"$merge_tool_path" -d -c "wincmd l" "$LOCAL" "$REMOTE"
-		;;
-
-	gvimdiff)
-		"$merge_tool_path" -d -c "wincmd l" -f "$LOCAL" "$REMOTE"
-		;;
-
-	xxdiff)
-		"$merge_tool_path" \
-			-R 'Accel.Search: "Ctrl+F"' \
-			-R 'Accel.SearchForward: "Ctrl-G"' \
-			"$LOCAL" "$REMOTE"
-		;;
-
-	opendiff)
-		"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
-		;;
-
-	ecmerge)
-		"$merge_tool_path" "$LOCAL" "$REMOTE" \
-			--default --mode=merge2 --to="$MERGED"
-		;;
-
-	emerge)
-		"$merge_tool_path" -f emerge-files-command \
-			"$LOCAL" "$REMOTE" "$(basename "$MERGED")"
-		;;
-
-	*)
-		if test -n "$merge_tool_cmd"; then
-			( eval $merge_tool_cmd )
-		fi
-		;;
-	esac
-}
-
-# Verifies that (difftool|mergetool).<tool>.cmd exists
-valid_custom_tool() {
-	merge_tool_cmd="$(git config difftool.$1.cmd)"
-	test -z "$merge_tool_cmd" &&
-	merge_tool_cmd="$(git config mergetool.$1.cmd)"
-	test -n "$merge_tool_cmd"
-}
-
-# Verifies that the chosen merge tool is properly setup.
-# Built-in merge tools are always valid.
-valid_tool() {
-	case "$1" in
-	kdiff3 | kompare | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
-		;; # happy
-	*)
-		if ! valid_custom_tool "$1"
-		then
-			return 1
-		fi
-		;;
-	esac
-}
-
-# Sets up the merge_tool_path variable.
-# This handles the difftool.<tool>.path configuration.
-# This also falls back to mergetool defaults.
-init_merge_tool_path() {
-	merge_tool_path=$(git config difftool."$1".path)
-	test -z "$merge_tool_path" &&
-	merge_tool_path=$(git config mergetool."$1".path)
-	if test -z "$merge_tool_path"; then
-		case "$1" in
-		vimdiff)
-			merge_tool_path=vim
-			;;
-		gvimdiff)
-			merge_tool_path=gvim
-			;;
-		emerge)
-			merge_tool_path=emacs
-			;;
-		*)
-			merge_tool_path="$1"
-			;;
-		esac
-	fi
+	run_merge_tool "$merge_tool"
 }
 
 # Allow GIT_DIFF_TOOL and GIT_MERGE_TOOL to provide default values
 test -n "$GIT_MERGE_TOOL" && merge_tool="$GIT_MERGE_TOOL"
 test -n "$GIT_DIFF_TOOL" && merge_tool="$GIT_DIFF_TOOL"
 
-# If merge tool was not specified then use the diff.tool
-# configuration variable.  If that's invalid then reset merge_tool.
-# Fallback to merge.tool.
-if test -z "$merge_tool"; then
-	merge_tool=$(git config diff.tool)
-	test -z "$merge_tool" &&
-	merge_tool=$(git config merge.tool)
-	if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
-		echo >&2 "git config option diff.tool set to unknown tool: $merge_tool"
-		echo >&2 "Resetting to default..."
-		unset merge_tool
-	fi
-fi
-
-# Try to guess an appropriate merge tool if no tool has been set.
-if test -z "$merge_tool"; then
-	# We have a $DISPLAY so try some common UNIX merge tools
-	if test -n "$DISPLAY"; then
-		# If gnome then prefer meld, otherwise, prefer kdiff3 or kompare
-		if test -n "$GNOME_DESKTOP_SESSION_ID" ; then
-			merge_tool_candidates="meld kdiff3 kompare tkdiff xxdiff gvimdiff diffuse"
-		else
-			merge_tool_candidates="kdiff3 kompare tkdiff xxdiff meld gvimdiff diffuse"
-		fi
-	fi
-	if echo "${VISUAL:-$EDITOR}" | grep 'emacs' > /dev/null 2>&1; then
-		# $EDITOR is emacs so add emerge as a candidate
-		merge_tool_candidates="$merge_tool_candidates emerge opendiff vimdiff"
-	elif echo "${VISUAL:-$EDITOR}" | grep 'vim' > /dev/null 2>&1; then
-		# $EDITOR is vim so add vimdiff as a candidate
-		merge_tool_candidates="$merge_tool_candidates vimdiff opendiff emerge"
-	else
-		merge_tool_candidates="$merge_tool_candidates opendiff emerge vimdiff"
-	fi
-	echo "merge tool candidates: $merge_tool_candidates"
-
-	# Loop over each candidate and stop when a valid merge tool is found.
-	for i in $merge_tool_candidates
-	do
-		init_merge_tool_path $i
-		if type "$merge_tool_path" > /dev/null 2>&1; then
-			merge_tool=$i
-			break
-		fi
-	done
-
-	if test -z "$merge_tool" ; then
-		echo "No known merge resolution program available."
-		exit 1
-	fi
-
-else
-	# A merge tool has been set, so verify that it's valid.
-	if ! valid_tool "$merge_tool"; then
-		echo >&2 "Unknown merge tool $merge_tool"
-		exit 1
-	fi
-
-	init_merge_tool_path "$merge_tool"
-
-	if test -z "$merge_tool_cmd" && ! type "$merge_tool_path" > /dev/null 2>&1; then
-		echo "The merge tool $merge_tool is not available as '$merge_tool_path'"
-		exit 1
-	fi
-fi
-
+merge_tool=$(get_merge_tool "$merge_tool") || exit
+merge_tool_cmd="$(get_merge_tool_cmd "$merge_tool")"
+merge_tool_path="$(get_merge_tool_path "$merge_tool")" || exit
 
 # Launch the merge tool on each path provided by 'git diff'
 while test $# -gt 6
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
new file mode 100644
index 0000000..a2f45ee
--- /dev/null
+++ b/git-mergetool--lib.sh
@@ -0,0 +1,387 @@
+# git-mergetool--lib is a library for common merge tool functions
+diff_mode() {
+	test $TOOL_MODE = "diff"
+}
+
+merge_mode() {
+	test $TOOL_MODE = "merge"
+}
+
+translate_merge_tool_path () {
+	if test -n "$2"; then
+		echo "$2"
+	else
+		case "$1" in
+		vimdiff)
+			path=vim
+			;;
+		gvimdiff)
+			path=gvim
+			;;
+		emerge)
+			path=emacs
+			;;
+		*)
+			path="$1"
+			;;
+		esac
+		echo "$path"
+	fi
+}
+
+check_unchanged () {
+	if merge_mode; then
+		if test "$MERGED" -nt "$BACKUP"; then
+			status=0
+		else
+			while true; do
+				echo "$MERGED seems unchanged."
+				printf "Was the merge successful? [y/n] "
+				read answer < /dev/tty
+				case "$answer" in
+				y*|Y*) status=0; break ;;
+				n*|N*) status=1; break ;;
+				esac
+			done
+		fi
+	else
+		status=0
+	fi
+}
+
+valid_tool () {
+	case "$1" in
+	kdiff3 | tkdiff | xxdiff | meld | opendiff | \
+	emerge | vimdiff | gvimdiff | ecmerge | diffuse)
+		;; # happy
+	tortoisemerge)
+		if ! merge_mode; then
+			return 1
+		fi
+		;;
+	kompare)
+		if ! diff_mode; then
+			return 1
+		fi
+		;;
+	*)
+		if test -z "$(get_merge_tool_cmd "$1")"; then
+			return 1
+		fi
+		;;
+	esac
+}
+
+get_merge_tool_cmd () {
+	diff_mode &&
+	custom_cmd="$(git config difftool.$1.cmd)"
+	test -z "$custom_cmd" &&
+	custom_cmd="$(git config mergetool.$1.cmd)"
+	test -n "$custom_cmd" &&
+	echo "$custom_cmd"
+}
+
+run_merge_tool () {
+	base_present="$2"
+	if diff_mode; then
+		status=0
+		base_present="false"
+	fi
+
+	case "$1" in
+	kdiff3)
+		if merge_mode; then
+			if $base_present; then
+				("$merge_tool_path" --auto \
+					--L1 "$MERGED (Base)" \
+					--L2 "$MERGED (Local)" \
+					--L3 "$MERGED (Remote)" \
+					-o "$MERGED" \
+					"$BASE" "$LOCAL" "$REMOTE" \
+				> /dev/null 2>&1)
+			else
+				("$merge_tool_path" --auto \
+					--L1 "$MERGED (Local)" \
+					--L2 "$MERGED (Remote)" \
+					-o "$MERGED" \
+					"$LOCAL" "$REMOTE" \
+				> /dev/null 2>&1)
+			fi
+			status=$?
+		else
+			("$merge_tool_path" --auto \
+			 --L1 "$MERGED (A)" \
+			 --L2 "$MERGED (B)" "$LOCAL" "$REMOTE" \
+			 > /dev/null 2>&1)
+		fi
+		;;
+	kompare)
+		"$merge_tool_path" "$LOCAL" "$REMOTE"
+		;;
+	tkdiff)
+		if merge_mode; then
+			if $base_present; then
+				"$merge_tool_path" -a "$BASE" \
+					-o "$MERGED" "$LOCAL" "$REMOTE"
+			else
+				"$merge_tool_path" \
+					-o "$MERGED" "$LOCAL" "$REMOTE"
+			fi
+			status=$?
+		else
+			"$merge_tool_path" "$LOCAL" "$REMOTE"
+		fi
+		;;
+	meld)
+		if merge_mode; then
+			touch "$BACKUP"
+			"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+			check_unchanged
+		else
+			"$merge_tool_path" "$LOCAL" "$REMOTE"
+		fi
+		;;
+	diffuse)
+		if merge_mode; then
+			touch "$BACKUP"
+			if $base_present; then
+				"$merge_tool_path" \
+					"$LOCAL" "$MERGED" "$REMOTE" \
+					"$BASE" | cat
+			else
+				"$merge_tool_path" \
+					"$LOCAL" "$MERGED" "$REMOTE" | cat
+			fi
+			check_unchanged
+		else
+			"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
+		fi
+		;;
+	vimdiff)
+		if merge_mode; then
+			touch "$BACKUP"
+			"$merge_tool_path" -d -c "wincmd l" \
+				"$LOCAL" "$MERGED" "$REMOTE"
+			check_unchanged
+		else
+			"$merge_tool_path" -d -c "wincmd l" \
+				"$LOCAL" "$REMOTE"
+		fi
+		;;
+	gvimdiff)
+		if merge_mode; then
+			touch "$BACKUP"
+			"$merge_tool_path" -d -c "wincmd l" -f \
+				"$LOCAL" "$MERGED" "$REMOTE"
+			check_unchanged
+		else
+			"$merge_tool_path" -d -c "wincmd l" -f \
+				"$LOCAL" "$REMOTE"
+		fi
+		;;
+	xxdiff)
+		if merge_mode; then
+			touch "$BACKUP"
+			if $base_present; then
+				"$merge_tool_path" -X --show-merged-pane \
+					-R 'Accel.SaveAsMerged: "Ctrl-S"' \
+					-R 'Accel.Search: "Ctrl+F"' \
+					-R 'Accel.SearchForward: "Ctrl-G"' \
+					--merged-file "$MERGED" \
+					"$LOCAL" "$BASE" "$REMOTE"
+			else
+				"$merge_tool_path" -X $extra \
+					-R 'Accel.SaveAsMerged: "Ctrl-S"' \
+					-R 'Accel.Search: "Ctrl+F"' \
+					-R 'Accel.SearchForward: "Ctrl-G"' \
+					--merged-file "$MERGED" \
+					"$LOCAL" "$REMOTE"
+			fi
+			check_unchanged
+		else
+			"$merge_tool_path" \
+				-R 'Accel.Search: "Ctrl+F"' \
+				-R 'Accel.SearchForward: "Ctrl-G"' \
+				"$LOCAL" "$REMOTE"
+		fi
+		;;
+	opendiff)
+		if merge_mode; then
+			touch "$BACKUP"
+			if $base_present; then
+				"$merge_tool_path" "$LOCAL" "$REMOTE" \
+					-ancestor "$BASE" \
+					-merge "$MERGED" | cat
+			else
+				"$merge_tool_path" "$LOCAL" "$REMOTE" \
+					-merge "$MERGED" | cat
+			fi
+			check_unchanged
+		else
+			"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
+		fi
+		;;
+	ecmerge)
+		if merge_mode; then
+			touch "$BACKUP"
+			if $base_present; then
+				"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
+					--default --mode=merge3 --to="$MERGED"
+			else
+				"$merge_tool_path" "$LOCAL" "$REMOTE" \
+					--default --mode=merge2 --to="$MERGED"
+			fi
+			check_unchanged
+		else
+			"$merge_tool_path" "$LOCAL" "$REMOTE" \
+				--default --mode=merge2 --to="$MERGED"
+		fi
+		;;
+	emerge)
+		if merge_mode; then
+			if $base_present; then
+				"$merge_tool_path" \
+					-f emerge-files-with-ancestor-command \
+					"$LOCAL" "$REMOTE" "$BASE" \
+					"$(basename "$MERGED")"
+			else
+				"$merge_tool_path" \
+					-f emerge-files-command \
+					"$LOCAL" "$REMOTE" \
+					"$(basename "$MERGED")"
+			fi
+			status=$?
+		else
+			"$merge_tool_path" -f emerge-files-command \
+				"$LOCAL" "$REMOTE" "$(basename "$MERGED")"
+		fi
+		;;
+	tortoisemerge)
+		if $base_present; then
+			touch "$BACKUP"
+			"$merge_tool_path" \
+				-base:"$BASE" -mine:"$LOCAL" \
+				-theirs:"$REMOTE" -merged:"$MERGED"
+			check_unchanged
+		else
+			echo "TortoiseMerge cannot be used without a base" 1>&2
+			status=1
+		fi
+		;;
+	*)
+		if test -z "$merge_tool_cmd"; then
+			if merge_mode; then
+				status=1
+			fi
+			break
+		fi
+		if merge_mode; then
+			if test "$merge_tool_trust_exit_code" = "false"; then
+				touch "$BACKUP"
+				( eval $merge_tool_cmd )
+				check_unchanged
+			else
+				( eval $merge_tool_cmd )
+			fi
+			status=$?
+		else
+			( eval $merge_tool_cmd )
+		fi
+		;;
+	esac
+	return $status
+}
+
+guess_merge_tool () {
+	tools="ecmerge"
+	if merge_mode; then
+		tools="$tools tortoisemerge"
+	else
+		kompare=" kompare "
+	fi
+	if test -n "$DISPLAY"; then
+		if test -n "$GNOME_DESKTOP_SESSION_ID" ; then
+			tools="meld opendiff kdiff3""$kompare""tkdiff $tools"
+			tools="$tools xxdiff gvimdiff diffuse"
+		else
+			tools="opendiff kdiff3""$kompare""tkdiff xxdiff $tools"
+			tools="$tools meld gvimdiff diffuse"
+		fi
+	fi
+	if echo "${VISUAL:-$EDITOR}" | grep emacs > /dev/null 2>&1; then
+		# $EDITOR is emacs so add emerge as a candidate
+		tools="$tools emerge vimdiff"
+	elif echo "${VISUAL:-$EDITOR}" | grep vim > /dev/null 2>&1; then
+		# $EDITOR is vim so add vimdiff as a candidate
+		tools="$tools vimdiff emerge"
+	else
+		tools="$tools emerge vimdiff"
+	fi
+	echo >&2 "merge tool candidates: $tools"
+
+	# Loop over each candidate and stop when a valid merge tool is found.
+	for i in $tools
+	do
+		merge_tool_path="$(translate_merge_tool_path "$i")"
+		if type "$merge_tool_path" > /dev/null 2>&1; then
+			merge_tool="$i"
+			break
+		fi
+	done
+
+	if test -z "$merge_tool" ; then
+		echo >&2 "No known merge resolution program available."
+		return 1
+	fi
+	echo "$merge_tool"
+}
+
+get_configured_merge_tool () {
+	# Diff mode first tries diff.tool and falls back to merge.tool.
+	# Merge mode only checks merge.tool
+	if diff_mode; then
+		tool=$(git config diff.tool)
+	fi
+	if test -z "$tool"; then
+		tool=$(git config merge.tool)
+	fi
+	if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
+		echo >&2 "git config option $TOOL_MODE.tool set to unknown tool: $merge_tool"
+		echo >&2 "Resetting to default..."
+		return 1
+	fi
+	echo "$tool"
+}
+
+get_merge_tool_path () {
+	# A merge tool has been set, so verify that it's valid.
+	if ! valid_tool "$merge_tool"; then
+		echo >&2 "Unknown merge tool $merge_tool"
+		exit 1
+	fi
+	if diff_mode; then
+		merge_tool_path=$(git config difftool."$merge_tool".path)
+	fi
+	if test -z "$merge_tool_path"; then
+		merge_tool_path=$(git config mergetool."$merge_tool".path)
+	fi
+	merge_tool_path="$(translate_merge_tool_path "$merge_tool" "$merge_tool_path")"
+	if test -z "$merge_tool_cmd" && ! type "$merge_tool_path" > /dev/null 2>&1; then
+		echo >&2 "The $TOOL_MODE tool $merge_tool is not available as '$merge_tool_path'"
+		exit 1
+	fi
+	echo "$merge_tool_path"
+}
+
+get_merge_tool () {
+	merge_tool="$1"
+	# Check if a merge tool has been configured
+	if test -z "$merge_tool"; then
+		merge_tool=$(get_configured_merge_tool)
+	fi
+	# Try to guess an appropriate merge tool if no tool has been set.
+	if test -z "$merge_tool"; then
+		merge_tool=$(guess_merge_tool) || exit
+	fi
+	echo "$merge_tool"
+}
diff --git a/git-mergetool.sh b/git-mergetool.sh
index cceebb7..efa31a2 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -11,7 +11,9 @@
 USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
+TOOL_MODE=merge
 . git-sh-setup
+. git-mergetool--lib
 require_work_tree
 
 # Returns true if the mode reflects a symlink
@@ -110,22 +112,6 @@ resolve_deleted_merge () {
 	done
 }
 
-check_unchanged () {
-    if test "$MERGED" -nt "$BACKUP" ; then
-	status=0;
-    else
-	while true; do
-	    echo "$MERGED seems unchanged."
-	    printf "Was the merge successful? [y/n] "
-	    read answer < /dev/tty
-	    case "$answer" in
-		y*|Y*) status=0; break ;;
-		n*|N*) status=1; break ;;
-	    esac
-	done
-    fi
-}
-
 checkout_staged_file () {
     tmpfile=$(expr "$(git checkout-index --temp --stage="$1" "$2")" : '\([^	]*\)	')
 
@@ -188,107 +174,11 @@ merge_file () {
 	read ans
     fi
 
-    case "$merge_tool" in
-	kdiff3)
-	    if base_present ; then
-		("$merge_tool_path" --auto --L1 "$MERGED (Base)" --L2 "$MERGED (Local)" --L3 "$MERGED (Remote)" \
-		    -o "$MERGED" "$BASE" "$LOCAL" "$REMOTE" > /dev/null 2>&1)
-	    else
-		("$merge_tool_path" --auto --L1 "$MERGED (Local)" --L2 "$MERGED (Remote)" \
-		    -o "$MERGED" "$LOCAL" "$REMOTE" > /dev/null 2>&1)
-	    fi
-	    status=$?
-	    ;;
-	tkdiff)
-	    if base_present ; then
-		"$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
-	    else
-		"$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
-	    fi
-	    status=$?
-	    ;;
-	meld)
-	    touch "$BACKUP"
-	    "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
-	    check_unchanged
-	    ;;
-	vimdiff)
-	    touch "$BACKUP"
-	    "$merge_tool_path" -d -c "wincmd l" "$LOCAL" "$MERGED" "$REMOTE"
-	    check_unchanged
-	    ;;
-	gvimdiff)
-	    touch "$BACKUP"
-	    "$merge_tool_path" -d -c "wincmd l" -f "$LOCAL" "$MERGED" "$REMOTE"
-	    check_unchanged
-	    ;;
-	xxdiff)
-	    touch "$BACKUP"
-	    if base_present ; then
-		"$merge_tool_path" -X --show-merged-pane \
-		    -R 'Accel.SaveAsMerged: "Ctrl-S"' \
-		    -R 'Accel.Search: "Ctrl+F"' \
-		    -R 'Accel.SearchForward: "Ctrl-G"' \
-		    --merged-file "$MERGED" "$LOCAL" "$BASE" "$REMOTE"
-	    else
-		"$merge_tool_path" -X --show-merged-pane \
-		    -R 'Accel.SaveAsMerged: "Ctrl-S"' \
-		    -R 'Accel.Search: "Ctrl+F"' \
-		    -R 'Accel.SearchForward: "Ctrl-G"' \
-		    --merged-file "$MERGED" "$LOCAL" "$REMOTE"
-	    fi
-	    check_unchanged
-	    ;;
-	opendiff)
-	    touch "$BACKUP"
-	    if base_present; then
-		"$merge_tool_path" "$LOCAL" "$REMOTE" -ancestor "$BASE" -merge "$MERGED" | cat
-	    else
-		"$merge_tool_path" "$LOCAL" "$REMOTE" -merge "$MERGED" | cat
-	    fi
-	    check_unchanged
-	    ;;
-	ecmerge)
-	    touch "$BACKUP"
-	    if base_present; then
-		"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" --default --mode=merge3 --to="$MERGED"
-	    else
-		"$merge_tool_path" "$LOCAL" "$REMOTE" --default --mode=merge2 --to="$MERGED"
-	    fi
-	    check_unchanged
-	    ;;
-	emerge)
-	    if base_present ; then
-		"$merge_tool_path" -f emerge-files-with-ancestor-command "$LOCAL" "$REMOTE" "$BASE" "$(basename "$MERGED")"
-	    else
-		"$merge_tool_path" -f emerge-files-command "$LOCAL" "$REMOTE" "$(basename "$MERGED")"
-	    fi
-	    status=$?
-	    ;;
-	tortoisemerge)
-	    if base_present ; then
-		touch "$BACKUP"
-		"$merge_tool_path" -base:"$BASE" -mine:"$LOCAL" -theirs:"$REMOTE" -merged:"$MERGED"
-		check_unchanged
-	    else
-		echo "TortoiseMerge cannot be used without a base" 1>&2
-		status=1
-	    fi
-	    ;;
-	*)
-	    if test -n "$merge_tool_cmd"; then
-		if test "$merge_tool_trust_exit_code" = "false"; then
-		    touch "$BACKUP"
-		    ( eval $merge_tool_cmd )
-		    check_unchanged
-		else
-		    ( eval $merge_tool_cmd )
-		    status=$?
-		fi
-	    fi
-	    ;;
-    esac
-    if test "$status" -ne 0; then
+    present=false
+    base_present &&
+    present=true
+
+    if ! run_merge_tool "$merge_tool" "$present"; then
 	echo "merge of $MERGED failed" 1>&2
 	mv -- "$BACKUP" "$MERGED"
 
@@ -347,44 +237,6 @@ do
     shift
 done
 
-valid_custom_tool()
-{
-    merge_tool_cmd="$(git config mergetool.$1.cmd)"
-    test -n "$merge_tool_cmd"
-}
-
-valid_tool() {
-	case "$1" in
-		kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge | tortoisemerge)
-			;; # happy
-		*)
-			if ! valid_custom_tool "$1"; then
-				return 1
-			fi
-			;;
-	esac
-}
-
-init_merge_tool_path() {
-	merge_tool_path=$(git config mergetool.$1.path)
-	if test -z "$merge_tool_path" ; then
-		case "$1" in
-			vimdiff)
-				merge_tool_path=vim
-				;;
-			gvimdiff)
-				merge_tool_path=gvim
-				;;
-			emerge)
-				merge_tool_path=emacs
-				;;
-			*)
-				merge_tool_path=$1
-				;;
-		esac
-	fi
-}
-
 prompt_after_failed_merge() {
     while true; do
 	printf "Continue merging other unresolved paths (y/n) ? "
@@ -402,62 +254,12 @@ prompt_after_failed_merge() {
     done
 }
 
-if test -z "$merge_tool"; then
-    merge_tool=$(git config merge.tool)
-    if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
-	    echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
-	    echo >&2 "Resetting to default..."
-	    unset merge_tool
-    fi
-fi
-
-if test -z "$merge_tool" ; then
-    if test -n "$DISPLAY"; then
-        if test -n "$GNOME_DESKTOP_SESSION_ID" ; then
-            merge_tool_candidates="meld kdiff3 tkdiff xxdiff tortoisemerge gvimdiff diffuse"
-        else
-            merge_tool_candidates="kdiff3 tkdiff xxdiff meld tortoisemerge gvimdiff diffuse"
-        fi
-    fi
-    if echo "${VISUAL:-$EDITOR}" | grep 'emacs' > /dev/null 2>&1; then
-        merge_tool_candidates="$merge_tool_candidates emerge opendiff vimdiff"
-    elif echo "${VISUAL:-$EDITOR}" | grep 'vim' > /dev/null 2>&1; then
-        merge_tool_candidates="$merge_tool_candidates vimdiff opendiff emerge"
-    else
-        merge_tool_candidates="$merge_tool_candidates opendiff emerge vimdiff"
-    fi
-    echo "merge tool candidates: $merge_tool_candidates"
-    for i in $merge_tool_candidates; do
-        init_merge_tool_path $i
-        if type "$merge_tool_path" > /dev/null 2>&1; then
-            merge_tool=$i
-            break
-        fi
-    done
-    if test -z "$merge_tool" ; then
-	echo "No known merge resolution program available."
-	exit 1
-    fi
-else
-    if ! valid_tool "$merge_tool"; then
-        echo >&2 "Unknown merge_tool $merge_tool"
-        exit 1
-    fi
-
-    init_merge_tool_path "$merge_tool"
-
-    merge_keep_backup="$(git config --bool merge.keepBackup || echo true)"
-    merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
-
-    if test -z "$merge_tool_cmd" && ! type "$merge_tool_path" > /dev/null 2>&1; then
-        echo "The merge tool $merge_tool is not available as '$merge_tool_path'"
-        exit 1
-    fi
-
-    if ! test -z "$merge_tool_cmd"; then
-        merge_tool_trust_exit_code="$(git config --bool mergetool.$merge_tool.trustExitCode || echo false)"
-    fi
-fi
+merge_tool=$(get_merge_tool "$merge_tool") || exit
+merge_tool_cmd="$(get_merge_tool_cmd "$merge_tool")"
+merge_tool_path="$(get_merge_tool_path "$merge_tool")" || exit
+merge_keep_backup="$(git config --bool merge.keepBackup || echo true)"
+merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
+merge_tool_trust_exit_code="$(git config --bool mergetool."$merge_tool".trustExitCode || echo false)"
 
 last_status=0
 rollup_status=0
-- 
1.6.2.2.414.g81aa9

^ permalink raw reply related

* Re: non-ascii filenames issue
From: demerphq @ 2009-04-07  8:26 UTC (permalink / raw)
  To: Peter Krefting; +Cc: John Tapsell, Git
In-Reply-To: <alpine.DEB.2.00.0904060823400.21376@ds9.cixit.se>

2009/4/6 Peter Krefting <peter@softwolves.pp.se>:
> John Tapsell:
>
>> Unfortunately not, because for some absolutely crazy reason, there is no
>> way at all to tell what encoding the string is in.  It never occured to
>> anyone that it might actually be useful to be able to read the filename in
>> an unambiguous way.
>
> It comes from the Unix tradition, unfortunately, that file names are just a
> stream of bytes, instead of a stream of characters mapped to a byte
> sequence. The "stream of bytes" think worked back when everyone used ASCII,
> but as soon as other character encodings were used (i.e back in the 1970s or
> so), that assumption broke.

Those interested in this subject may find the following document on
the creation of utf8 interesting.

http://www.cl.cam.ac.uk/~mgk25/ucs/utf-8-history.txt

cheers,
Yves

-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

^ permalink raw reply

* Re: [RFC/PATCH 0/2] New 'stage' command
From: Octavio Alvarez @ 2009-04-07  8:39 UTC (permalink / raw)
  To: Junio C Hamano, David Kågedal
  Cc: Felipe Contreras, markus heidelberg, Sverre Rabbelier,
	David Aguilar, git
In-Reply-To: <7v8wmd46p9.fsf@gitster.siamese.dyndns.org>

On Mon, 06 Apr 2009 18:02:10 -0700, Junio C Hamano <gitster@pobox.com> wrote:

> David Kågedal <davidk@lysator.liu.se> writes:
>
>>>    - when you want to work with both the index and the work tree at the
>>>      same time, you say STAGEANDWORKTREE (the same disambiguation  
>>> caveat
>>>      applies).
>>
>> No, where did this come from?
>
> "git apply STAGEANDWORKTREE this.patch".  I do not want "for diff you can
> use these metavariables to name two things compared, but you can do so
> only for diff".

That example is broken. git apply doesn't even take an arbitrary treeish.

>>> Think.  What does "git log STAGE" mean?  Can you explain why it does  
>>> not
>>> make any sense?

gitk actually does this. Even more, gitk shows them in this order:
STAGE^ would be HEAD.
WORKTREE^ would be STAGE.

Makes sense.

(Not that I think git log should do the same.)

The difference between git diff and git reset is that git diff should take
a range of trees, not a range of commits as parameters. OTOH, git reset
doesn't know or care about trees, it needs commits.

git checkout WORKTREE:file makes sense, even though it is useless, but
that's why it git checout STAGE:file makes sense: it should accept any tree
instead of a commit.

git apply doesn't even take commits. It *could* take trees if it
automagically created a branch on the commit, though. Either that,
or git-apply shouldn't exist at all.

It's similar with git reset. You wouldn't use STAGE or WORKTREE here because
a commit is actually necessary, but according option names like: --stage
--worktree --both --none are better than --hard, --soft and --mixed.

So, if the man page for git-reset says "commit-id" and the man page for
git diff says "tree-id..tree-id", I don't see any kind of confusion.
git checkout could too. git reset and git log are to say "commit-id",
but support clearer options.

WORKTREE, STAGE are trees as commits are also trees, but not all trees are
commits.

^ permalink raw reply

* Re: [PATCH] git-pull.sh: better warning message for "git pull" on  detached head.
From: Mike Ralphson @ 2009-04-07  9:24 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: gitster, git
In-Reply-To: <1239052487-6936-1-git-send-email-Matthieu.Moy@imag.fr>

2009/4/6 Matthieu Moy <Matthieu.Moy@imag.fr>:
> Otherwise, git complains about not finding a branch to pull from in
> 'branch..merge', which is hardly understandable.

> +               echo "You asked me to pull without telling me which branch you want"
> +               echo "to merge with, and you are on a detached HEAD, so I cannot"
> +               echo "know from 'branch.<branchname>.merge' in your configuration"
> +               echo "file.  Please name which branch you want to merge on the command"

'Know from' in this sense is a little confusing. Maybe something like:

s/so I cannot know from 'branch.<branchname>.merge' in your
configuration file/ \
so I cannot use any 'branch.<branchname>.merge' from your configuration file

?

> +               echo "your configuration file does not tell me either.  Please"
> +               echo "name which branch you want to merge on the command line and"
> +               echo "try again (e.g. 'git pull <repository> <refspec>')."

I know you haven't changed this part, but as it's included in the
patch, what we're asking the user to do is to 'specify' the branch to
merge, not to 'name' it.

Mike

^ permalink raw reply

* Re: [PATCH 2/2] git-checkout.txt: clarify that <branch> applies when  no path is given.
From: Mike Ralphson @ 2009-04-07  9:41 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: gitster, git
In-Reply-To: <1239050722-1227-2-git-send-email-Matthieu.Moy@imag.fr>

2009/4/6 Matthieu Moy <Matthieu.Moy@imag.fr>:
> Otherwise, the sentence "Defaults to HEAD." can be mis-read to mean
> that "git checkout -- hello.c" checks-out from HEAD.

> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> index 0b5485b..58bf508 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -126,9 +126,14 @@ the conflicted merge in the specified paths.
>  <new_branch>::
>        Name for the new branch.
>
> +<tree-ish>::
> +       Tree to checkout from (when path are given). If not specified,
> +       the index will be used.
> +
>  <branch>::
> -       Branch to checkout; may be any object ID that resolves to a
> -       commit.  Defaults to HEAD.
> +       Branch to checkout (when no path are given); may be any object
> +       ID that resolves to a commit.  Defaults to HEAD.
> +

s/path are/paths are/g

Sorry if I come across as picking on your grammar, I think your
documentation improvements are very worthwhile.

Mike

^ permalink raw reply

* Re: Performance issue: initial git clone causes massive repack
From: Jakub Narebski @ 2009-04-07  9:45 UTC (permalink / raw)
  To: Björn Steinbrink
  Cc: Nicolas Pitre, Sverre Rabbelier, david, Junio C Hamano,
	Nicolas Sebrecht, Robin H. Johnson, Git Mailing List
In-Reply-To: <20090407081019.GK20356@atjola.homenet>

Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> On 2009.04.05 23:24:27 -0400, Nicolas Pitre wrote:
> > On Sun, 5 Apr 2009, Sverre Rabbelier wrote:
> > > 
> > > I agree here, we should either say "look, we don't really support big
> > > repositories because [explanation here], unless you [workarounds
> > > here]" OR we should work to improve the support we do have. Of course,
> > > the latter option does not magically create developer time to work on
> > > that, but if we do go that way we should at least tell people that we
> > > are aware of the problems and that it's on the global TODO list (not
> > > necessarily on anyone's personal TODO list though).
> > 
> > For the record... I at least am aware of the problem and it is indeed on 
> > my personal git todo list.  Not that I have a clear solution yet (I've 
> > been pondering on some git packing issues for almost 4 years now).
> > 
> > Still, in this particular case, the problem appears to be unclear to me, 
> > like "this shouldn't be so bad".
> 
> It's not primarily pack-objects, I think. It's the rev-list that's run
> by upload-pack.  Running "git rev-list --objects --all" on that repo
> eats about 2G RSS, easily killing the system's cache on a small box,
> leading to swapping and a painful time reading the packfile contents
> afterwards to send them to the client.

Than I think that "packfile caching" GSoC project (which is IIRC
"object enumeration caching", or at least includes it) should help
here.  You would, from what I understand, run "git rev-list -objects
--all --not <tops of cache>" + sequential read of object enumeration
cache...

-- 
Jakub Narebski
Poland
ShadeHawk on #git

^ permalink raw reply

* Re: [RFC/PATCH 0/2] New 'stage' command
From: Felipe Contreras @ 2009-04-07 10:01 UTC (permalink / raw)
  To: Björn Steinbrink
  Cc: Junio C Hamano, David Kågedal, git, David Aguilar,
	Sverre Rabbelier, markus.heidelberg
In-Reply-To: <20090406192514.GI20356@atjola.homenet>

On Mon, Apr 6, 2009 at 10:25 PM, Björn Steinbrink <B.Steinbrink@gmx.de> wrote:
> On 2009.04.06 22:13:26 +0300, Felipe Contreras wrote:
>> On Mon, Apr 6, 2009 at 9:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > David Kågedal <davidk@lysator.liu.se> writes:
>> >
>> >> What do you mean? This was a suggestion for how git diff should
>> >> work. I fail to see how you would need a WORKTREEANDTHEINDEX there.
>> >
>> > You are talking only about "git diff".  I am talking about the whole git
>> > suite, because you have to worry about how such a proposal would affect
>> > other parts of the UI.
>>
>> How do currently do you something like this:
>> git diff HEAD^..STAGE
>
> git diff --cached HEAD^
>
> The "hard" (and pretty weird) one would be "git diff STAGE..HEAD^",
> which is:
>
> git diff -R --cached HEAD^

Sorry, that's what I meant.

So it's possible, but completely unintuitive, and different from other
use cases.

-- 
Felipe Contreras

^ permalink raw reply

* Re: Performance issue: initial git clone causes massive repack
From: Martin Langhoff @ 2009-04-07 10:11 UTC (permalink / raw)
  To: Nicolas Sebrecht; +Cc: david, Robin H. Johnson, Git Mailing List
In-Reply-To: <20090405225954.GA18730@vidovic>

On Mon, Apr 6, 2009 at 12:59 AM, Nicolas Sebrecht
<nicolas.s-dev@laposte.net> wrote:
> What about the rsync solution given in this thread?

Also, HTTP is excellent for initial clones, possibly better than rsync
in some cases.

The Gentoo team has good reasons to do things their way, and it's IMHO
a wart in git that initial clones of large repos. But we do have valid
workarounds (as above) so they can use them .

cheers,



martin
-- 
 martin.langhoff@gmail.com
 martin@laptop.org -- School Server Architect
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff

^ permalink raw reply

* [PATCH 1/2] git-checkout.txt: clarify that <branch> applies when no path is given.
From: Matthieu Moy @ 2009-04-07 10:20 UTC (permalink / raw)
  To: gitster, git; +Cc: Matthieu Moy
In-Reply-To: <e2b179460904070241y493ef876xc628d35b606e4032@mail.gmail.com>

Otherwise, the sentence "Defaults to HEAD." can be mis-read to mean
that "git checkout -- hello.c" checks-out from HEAD.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
> s/path are/paths are/g

Right.

I'm not 100% sure about "no paths are given" Vs "no path is given",
but my understanding of english grammar is that since there could have
been several paths, the absence of path is still plural (French would
be singular here).

> Sorry if I come across as picking on your grammar, I think your
> documentation improvements are very worthwhile.

No harm ;-).

 Documentation/git-checkout.txt |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 0b5485b..7eafcdf 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -126,9 +126,14 @@ the conflicted merge in the specified paths.
 <new_branch>::
 	Name for the new branch.
 
+<tree-ish>::
+	Tree to checkout from (when paths are given). If not specified,
+	the index will be used.
+
 <branch>::
-	Branch to checkout; may be any object ID that resolves to a
-	commit.  Defaults to HEAD.
+	Branch to checkout (when no paths are given); may be any object
+	ID that resolves to a commit.  Defaults to HEAD.
+
 +
 When this parameter names a non-branch (but still a valid commit object),
 your HEAD becomes 'detached'.
-- 
1.6.2.2.449.g92961.dirty

^ permalink raw reply related

* [PATCH v2] git-pull.sh: better warning message for "git pull" on detached head.
From: Matthieu Moy @ 2009-04-07  9:56 UTC (permalink / raw)
  To: gitster, git; +Cc: Matthieu Moy
In-Reply-To: <e2b179460904070224o3057c6efk6930bd1249adb0fe@mail.gmail.com>

Otherwise, git complains about not finding a branch to pull from in
'branch..merge', which is hardly understandable. While we're there,
reword the sentences slightly.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
> 'Know from' in this sense is a little confusing. Maybe something like:
> 
> s/so I cannot know from 'branch.<branchname>.merge' in your
> configuration file/ \
> so I cannot use any 'branch.<branchname>.merge' from your configuration file

Makes sense, yes.

> I know you haven't changed this part, but as it's included in the
> patch, what we're asking the user to do is to 'specify' the branch to
> merge, not to 'name' it.

Applied too.

I also added s/details on the refspec/details/ in the pointer to
git-pull(1) since first patch.

 git-pull.sh |   43 ++++++++++++++++++++++++++-----------------
 1 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index 8a26763..6d5f558 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -90,23 +90,32 @@ error_on_no_merge_candidates () {
 
 	curr_branch=${curr_branch#refs/heads/}
 
-	echo "You asked me to pull without telling me which branch you"
-	echo "want to merge with, and 'branch.${curr_branch}.merge' in"
-	echo "your configuration file does not tell me either.  Please"
-	echo "name which branch you want to merge on the command line and"
-	echo "try again (e.g. 'git pull <repository> <refspec>')."
-	echo "See git-pull(1) for details on the refspec."
-	echo
-	echo "If you often merge with the same branch, you may want to"
-	echo "configure the following variables in your configuration"
-	echo "file:"
-	echo
-	echo "    branch.${curr_branch}.remote = <nickname>"
-	echo "    branch.${curr_branch}.merge = <remote-ref>"
-	echo "    remote.<nickname>.url = <url>"
-	echo "    remote.<nickname>.fetch = <refspec>"
-	echo
-	echo "See git-config(1) for details."
+	if [ -z "$curr_branch" ]; then
+		echo "You asked me to pull without telling me which branch you want"
+		echo "to merge with, and you are on a detached HEAD, so I cannot"
+		echo "use any 'branch.<branchname>.merge' in your configuration"
+		echo "file.  Please specify which branch you want to merge on the command"
+		echo "line and try again (e.g. 'git pull <repository> <refspec>')."
+		echo "See git-pull(1) for details."
+	else
+		echo "You asked me to pull without telling me which branch you"
+		echo "want to merge with, and 'branch.${curr_branch}.merge' in"
+		echo "your configuration file does not tell me either.	Please"
+		echo "specify which branch you want to merge on the command line and"
+		echo "try again (e.g. 'git pull <repository> <refspec>')."
+		echo "See git-pull(1) for details."
+		echo
+		echo "If you often merge with the same branch, you may want to"
+		echo "configure the following variables in your configuration"
+		echo "file:"
+		echo
+		echo "    branch.${curr_branch}.remote = <nickname>"
+		echo "    branch.${curr_branch}.merge = <remote-ref>"
+		echo "    remote.<nickname>.url = <url>"
+		echo "    remote.<nickname>.fetch = <refspec>"
+		echo
+		echo "See git-config(1) for details."
+	fi
 	exit 1
 }
 
-- 
1.6.2.2.449.g92961.dirty

^ permalink raw reply related

* Re: [PATCH v2] git-pull.sh: better warning message for "git pull" on detached head.
From: Finn Arne Gangstad @ 2009-04-07 10:41 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: gitster, git
In-Reply-To: <1239098181-10360-1-git-send-email-Matthieu.Moy@imag.fr>

On Tue, Apr 07, 2009 at 11:56:21AM +0200, Matthieu Moy wrote:
> Otherwise, git complains about not finding a branch to pull from in
> 'branch..merge', which is hardly understandable. While we're there,
> reword the sentences slightly.
> [...]
> +	if [ -z "$curr_branch" ]; then
> +		echo "You asked me to pull without telling me which branch you want"
> +		echo "to merge with, and you are on a detached HEAD, so I cannot"
> [...]

In this case why can't we just do

echo "You are currently not on any branch."

or

echo "git pull cannot be run without arguments unless you are on a branch."

And possibly also something like this:

echo "Usage: git pull <repository> <refspec>."
echo " See git-pull(1) for details."
echo

If you need all the verbosity this error otherwise gives, "detached
HEAD" is probably going to be confusing?

- Finn Arne

^ permalink raw reply

* Re: [PATCH] mailmap: resurrect lower-casing of email addresses
From: Johannes Schindelin @ 2009-04-07 11:16 UTC (permalink / raw)
  To: A Large Angry SCM; +Cc: Junio C Hamano, git
In-Reply-To: <49DAB5BB.1040100@gmail.com>

Hi,

On Mon, 6 Apr 2009, A Large Angry SCM wrote:

> Johannes Schindelin wrote:
> 
> > On Fri, 3 Apr 2009, A Large Angry SCM wrote:
> > 
> > > Sorry, this is not a flame war (and as Peff already sent a response 
> > > that superior to my own) so I'll let Junio decide.
> > 
> > Thanks for keeping a cool head where I failed.  My sincere apologies.
> > 
> > > However, to keep the peace (and as a thank you for all the hard work 
> > > to date, I'll say that I'm scheduled to be be Germany and Munich the 
> > > first 10 days in October and I'll buy the first $100 dollars in 
> > > drinks at any meet that participate in (as a thank you to all the 
> > > hard work for git that has been performed) that may happen that I 
> > > participate in).
> > 
> > I'll take you up on that!
> 
> s/Germany and Munich/Berlin and Munich/

Cool!  Berlin is only 2 hours from my home, so let's meet there!

We could even use the opportunity for a little informal German 
GitTogether... "Alles wird Git!"?

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH v2] git-pull.sh: better warning message for "git pull" on detached head.
From: Matthieu Moy @ 2009-04-07 11:36 UTC (permalink / raw)
  To: Finn Arne Gangstad; +Cc: gitster, git
In-Reply-To: <20090407104145.GA22658@pvv.org>

Finn Arne Gangstad <finnag@pvv.org> writes:

> On Tue, Apr 07, 2009 at 11:56:21AM +0200, Matthieu Moy wrote:
>> Otherwise, git complains about not finding a branch to pull from in
>> 'branch..merge', which is hardly understandable. While we're there,
>> reword the sentences slightly.
>> [...]
>> +	if [ -z "$curr_branch" ]; then
>> +		echo "You asked me to pull without telling me which branch you want"
>> +		echo "to merge with, and you are on a detached HEAD, so I cannot"
>> [...]
>
> In this case why can't we just do
>
> echo "You are currently not on any branch."

Well, I would understand this as "you can't pull when you're not on a
branch", which would be incorrect.

> echo "git pull cannot be run without arguments unless you are on a branch."

This is better (less missleading).

> And possibly also something like this:
>
> echo "Usage: git pull <repository> <refspec>."
> echo " See git-pull(1) for details."

This usage string would be incorrect, repository and refspec are not
always mandatory. To be correct, it should be
Usage: git pull [<repository> <refspec>]
and then you're back to the problem of having to explain why they are
optionnal.

> If you need all the verbosity this error otherwise gives, "detached
> HEAD" is probably going to be confusing?

Grepping the source, "not on a branch" seems more widely used than
"detached head" in the UI (including in another place of git-pull.sh),
so, yes, something like this would be better:

	if [ -z "$curr_branch" ]; then
		echo "You are not currently on a branch, so I cannot use any"
		echo "'branch.<branchname>.merge' in your configuration file."
		echo "Please specify which branch you want to merge on the command"
		echo "line and try again (e.g. 'git pull <repository> <refspec>')."
		echo "See git-pull(1) for details."
	else

-- 
Matthieu

^ permalink raw reply

* Re: [PATCH] perl: add new module Git::Config for cached 'git config' access
From: Jakub Narebski @ 2009-04-07 12:01 UTC (permalink / raw)
  To: Sam Vilain; +Cc: Frank Lichtenheld, git, Petr Baudis
In-Reply-To: <1239058276.31863.19.camel@maia.lan>

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

> On Mon, 2009-04-06 at 11:29 +0200, Frank Lichtenheld wrote:
> > On Mon, Apr 06, 2009 at 11:46:15AM +1200, Sam Vilain wrote:

> > > +	my ($fh, $c) = $git->command_output_pipe(
> > > +		'config', ( $which ? ("--$which") : () ),
> > > +		'--list',
> > > +	       );
> >
> > Any reason why you don't use --null here? The output of --list
> > without --null is not reliably parsable, since people can put
> > newlines in values.
> 
> No particularly good reason :-)
> 
> Subject: [PATCH] perl: make Git::Config use --null
> 
> Use the form of 'git-config' designed for parsing by modules like
> this for safety with values containing embedded line feeds.
> 
> Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>

> diff --git a/perl/Git/Config.pm b/perl/Git/Config.pm
> index a0a6a41..a35d9f3 100644
> --- a/perl/Git/Config.pm
> +++ b/perl/Git/Config.pm
> @@ -179,12 +179,14 @@ sub read {
>  
>  	my ($fh, $c) = $git->command_output_pipe(
>  		'config', ( $which ? ("--$which") : () ),
> -		'--list',
> +		 '--null', '--list',
>  	       );
>  	my $read_state = {};
>  
> +	local($/)="\0";
>  	while (<$fh>) {
> -		my ($item, $value) = m{(.*?)=(.*)};
> +		my ($item, $value) = m{(.*?)\n((?s:.*))\0}
> +			or die "failed to parse it; \$_='$_'";
>  		my $sl = \( $read_state->{$item} );
>  		if (!defined $$sl) {
>  			$$sl = $value;

Errr... wouldn't it be better to simply use 

+		my ($item, $value) = split("\n", $_, 2)

here? Have you tested Git::Config with a "null" value, i.e. something
like

    [section]
        noval

in the config file (which evaluates to 'true' with '--bool' option)?
Because from what I remember from the discussion on the 
"git config --null --list" format the lack of "\n" is used to
distinguish between noval (which is equivalent to 'true'), and empty
value (which is equivalent to 'false')

    [boolean
        noval        # equivalent to 'true'
        empty1 =     # equivalent to 'false'
        empty2 = ""  # equivalent to 'false'

> diff --git a/t/t9700/config.t b/t/t9700/config.t
> index 395a5c9..f0f7d2d 100644
> --- a/t/t9700/config.t
> +++ b/t/t9700/config.t
> @@ -16,6 +16,7 @@ in_empty_repo sub {
>  	$git->command_oneline("config", "foo.intval", "12g");
>  	$git->command_oneline("config", "foo.false.val", "false");
>  	$git->command_oneline("config", "foo.true.val", "yes");
> +	$git->command_oneline("config", "multiline.val", "hello\nmultiline.val=world");
>  
>  	my $conf = Git::Config->new();
>  	ok($conf, "constructed a new Git::Config");

-- 
Jakub Narebski
Poland
ShadeHawk on #git

^ permalink raw reply

* [RFC/PATCH] git-submodule: add support for --rebase.
From: Peter Hutterer @ 2009-04-07 11:14 UTC (permalink / raw)
  To: git

'git submodule update --rebase' rebases onto the current branch rather than
detaching the HEAD.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---

I use git-submodule to keep track of repositories that I only infrequently
commit to. I keep them to have a set that is known to work.

git submodule update is annoying, as it (in my case needlessly) detaches the
HEAD, making the workflow more complicated when I do have to commit (checkout
master, rebase onto master, then commit).

This patch adds a "--rebase" flag to git submodule update that calls
git-rebase instead of git-checkout.

Any comments? Is there a better solution for this problem?

Cheers,
  Peter

 Documentation/git-submodule.txt |   11 +++++++++--
 git-submodule.sh                |   21 +++++++++++++++++----
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 3b8df44..117ad3d 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git submodule' [--quiet] add [-b branch] [--] <repository> <path>
 'git submodule' [--quiet] status [--cached] [--] [<path>...]
 'git submodule' [--quiet] init [--] [<path>...]
-'git submodule' [--quiet] update [--init] [-N|--no-fetch] [--] [<path>...]
+'git submodule' [--quiet] update [--init] [-N|--no-fetch] [--rebase] [--] [<path>...]
 'git submodule' [--quiet] summary [--summary-limit <n>] [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach <command>
 'git submodule' [--quiet] sync [--] [<path>...]
@@ -113,7 +113,8 @@ init::
 update::
 	Update the registered submodules, i.e. clone missing submodules and
 	checkout the commit specified in the index of the containing repository.
-	This will make the submodules HEAD be detached.
+	This will make the submodules HEAD be detached unless '--rebase' is
+	specified.
 +
 If the submodule is not yet initialized, and you just want to use the
 setting as stored in .gitmodules, you can automatically initialize the
@@ -177,6 +178,12 @@ OPTIONS
 	This option is only valid for the update command.
 	Don't fetch new objects from the remote site.
 
+--rebase::
+	This option is only valid for the update command.
+	Forward-port local commits to the index of the containing repository.
+	If a a merge failure prevents this process, you will have to resolve
+	these failures with linkgit:git-rebase[1].
+
 <path>...::
 	Paths to submodule(s). When specified this will restrict the command
 	to only operate on the submodules found at the specified paths.
diff --git a/git-submodule.sh b/git-submodule.sh
index 7c2e060..6180bf4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -17,6 +17,7 @@ branch=
 quiet=
 cached=
 nofetch=
+rebase=
 
 #
 # print stuff on stdout unless -q was specified
@@ -314,6 +315,10 @@ cmd_update()
 			shift
 			nofetch=1
 			;;
+		-r|--rebase)
+			shift
+			rebase=1
+			;;
 		--)
 			shift
 			break
@@ -367,11 +372,19 @@ cmd_update()
 				die "Unable to fetch in submodule path '$path'"
 			fi
 
-			(unset GIT_DIR; cd "$path" &&
-				  git-checkout $force -q "$sha1") ||
-			die "Unable to checkout '$sha1' in submodule path '$path'"
+			if test -z "$rebase"
+			then
+				(unset GIT_DIR; cd "$path" &&
+					  git-checkout $force -q "$sha1") ||
+				die "Unable to checkout '$sha1' in submodule path '$path'"
+				say "Submodule path '$path': checked out '$sha1'"
+			else
+				(unset GIT_DIR; cd "$path" &&
+					git-rebase "$sha1") ||
+					die "Unable to rebase onto '$sha1' in submodule path '$path'"
+				say "Submodule path '$path': rebased onto '$sha1'"
+			fi
 
-			say "Submodule path '$path': checked out '$sha1'"
 		fi
 	done
 }
-- 
1.6.2.2.447.ge953ab

^ permalink raw reply related

* Re: [RFC/PATCH] git-submodule: add support for --rebase.
From: Johannes Schindelin @ 2009-04-07 12:38 UTC (permalink / raw)
  To: Peter Hutterer; +Cc: git
In-Reply-To: <20090407111445.GA11344@dingo>

Hi,

Disclaimer: if you are offended by constructive criticism, or likely to
answer with insults to the comments I offer, please stop reading this mail
now (and please do not answer my mail, either). :-)

Still with me?  Good.  Nice to meet you.

Just for the record: responding to a patch is my strongest way of saying
that I appreciate your work.

On Tue, 7 Apr 2009, Peter Hutterer wrote:

> 'git submodule update --rebase' rebases onto the current branch rather 
> than detaching the HEAD.

I know what you want to do, but this text is wrong: it should rather read 
something like this:

	'git submodule update --rebase' rebases your local branch on 
	top of what would have been checked out to a detached HEAD 
	otherwise.

Maybe some of these native English speakers on this list can come up with 
something even better ;-)

> I use git-submodule to keep track of repositories that I only 
> infrequently commit to. I keep them to have a set that is known to work.
> 
> git submodule update is annoying, as it (in my case needlessly) detaches the
> HEAD, making the workflow more complicated when I do have to commit (checkout
> master, rebase onto master, then commit).
> 
> This patch adds a "--rebase" flag to git submodule update that calls
> git-rebase instead of git-checkout.

Maybe a non-first person version of this comment could go into the commit 
message, too?  I found this highly informative.

> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 3b8df44..117ad3d 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -177,6 +178,12 @@ OPTIONS
>  	This option is only valid for the update command.
>  	Don't fetch new objects from the remote site.
>  
> +--rebase::
> +	This option is only valid for the update command.

This is unnecessary, it was mentioned in the synopsis.

> +	Forward-port local commits to the index of the containing repository.

This is a bit misleading/unclear.  I'd rather have it read like this:

	Instead of detaching the HEAD to the revision committed in the 
	superproject, rebase the current branch onto that revision.

> +	If a a merge failure prevents this process, you will have to resolve
> +	these failures with linkgit:git-rebase[1].
> +
>  <path>...::
>  	Paths to submodule(s). When specified this will restrict the command
>  	to only operate on the submodules found at the specified paths.
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 7c2e060..6180bf4 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh

You might want to error out when --rebase was passed with another command 
than "update".

> @@ -367,11 +372,19 @@ cmd_update()
>  				die "Unable to fetch in submodule path '$path'"
>  			fi
>  
> -			(unset GIT_DIR; cd "$path" &&
> -				  git-checkout $force -q "$sha1") ||
> -			die "Unable to checkout '$sha1' in submodule path '$path'"
> +			if test -z "$rebase"
> +			then
> +				(unset GIT_DIR; cd "$path" &&
> +					  git-checkout $force -q "$sha1") ||
> +				die "Unable to checkout '$sha1' in submodule path '$path'"
> +				say "Submodule path '$path': checked out '$sha1'"
> +			else
> +				(unset GIT_DIR; cd "$path" &&
> +					git-rebase "$sha1") ||
> +					die "Unable to rebase onto '$sha1' in submodule path '$path'"
> +				say "Submodule path '$path': rebased onto '$sha1'"
> +			fi

I'd actually put the "(unset GIT_DIR; cd "$path" &&" in front of the "if" 
due to the DRY principle (Don't Repeat Yourself).

In the same spirit, I'd set a variable "action" to "checkout" or "rebase 
onto" and munge the error/info message to use $action.

Maybe you want to add a single primitive test case to make sure this 
feature will not get broken in the future?

Other than that: nice!  very nice!

Ciao,
Dscho

P.S.: the next patch is obvious, too: add support to specify desire to 
rebase the submodule in .gitmodules and .git/config.

^ permalink raw reply

* Re: [RFC/PATCH 0/2] New 'stage' command
From: Johannes Schindelin @ 2009-04-07 12:45 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: Björn Steinbrink, Junio C Hamano, David Kågedal, git,
	David Aguilar, Sverre Rabbelier, markus.heidelberg
In-Reply-To: <94a0d4530904070301k62687e2i772637068494ab74@mail.gmail.com>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1398 bytes --]

Hi,

On Tue, 7 Apr 2009, Felipe Contreras wrote:

> On Mon, Apr 6, 2009 at 10:25 PM, Björn Steinbrink <B.Steinbrink@gmx.de> wrote:
> > On 2009.04.06 22:13:26 +0300, Felipe Contreras wrote:
> >> On Mon, Apr 6, 2009 at 9:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> > David Kågedal <davidk@lysator.liu.se> writes:
> >> >
> >> >> What do you mean? This was a suggestion for how git diff should
> >> >> work. I fail to see how you would need a WORKTREEANDTHEINDEX there.
> >> >
> >> > You are talking only about "git diff".  I am talking about the whole git
> >> > suite, because you have to worry about how such a proposal would affect
> >> > other parts of the UI.
> >>
> >> How do currently do you something like this:
> >> git diff HEAD^..STAGE
> >
> > git diff --cached HEAD^
> >
> > The "hard" (and pretty weird) one would be "git diff STAGE..HEAD^",
> > which is:
> >
> > git diff -R --cached HEAD^
> 
> Sorry, that's what I meant.
> 
> So it's possible, but completely unintuitive, and different from other
> use cases.

At least it is consistent.

You are always able to say "-R" if you want to have the reverse diff, this 
is commonly known from GNU diff and other diff implementations.

Also, "git diff --cached" is _only_ a shortcut for "git diff --cached 
HEAD".  This is yet another proof that you should not teach the shortcuts 
first, it _harms_ understanding.

Ciao,
Dscho

^ permalink raw reply

* [PATCH v2] git-checkout.txt: clarify that <branch> applies when no path is given.
From: Matthieu Moy @ 2009-04-07 12:43 UTC (permalink / raw)
  To: gitster, git; +Cc: Matthieu Moy
In-Reply-To: <1239099643-11959-1-git-send-email-Matthieu.Moy@imag.fr>

Otherwise, the sentence "Defaults to HEAD." can be mis-read to mean
that "git checkout -- hello.c" checks-out from HEAD.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Sorry, patch v1 was introducing a spurious newline, which broke
asciidoc syntax. This fixes it, but Junio, if you already applied v1,
just remove the empty line after <branch>:: and before +.

 Documentation/git-checkout.txt |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 0b5485b..223ea9c 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -126,9 +126,13 @@ the conflicted merge in the specified paths.
 <new_branch>::
 	Name for the new branch.
 
+<tree-ish>::
+	Tree to checkout from (when paths are given). If not specified,
+	the index will be used.
+
 <branch>::
-	Branch to checkout; may be any object ID that resolves to a
-	commit.  Defaults to HEAD.
+	Branch to checkout (when no paths are given); may be any object
+	ID that resolves to a commit.  Defaults to HEAD.
 +
 When this parameter names a non-branch (but still a valid commit object),
 your HEAD becomes 'detached'.
-- 
1.6.2.2.449.g92961.dirty

^ permalink raw reply related

* Re: Performance issue: initial git clone causes massive repack
From: Nicolas Pitre @ 2009-04-07 13:13 UTC (permalink / raw)
  To: Jakub Narebski
  Cc: Björn Steinbrink, Sverre Rabbelier, david, Junio C Hamano,
	Nicolas Sebrecht, Robin H. Johnson, Git Mailing List
In-Reply-To: <m3tz5023rq.fsf@localhost.localdomain>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2102 bytes --]

On Tue, 7 Apr 2009, Jakub Narebski wrote:

> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> > On 2009.04.05 23:24:27 -0400, Nicolas Pitre wrote:
> > > On Sun, 5 Apr 2009, Sverre Rabbelier wrote:
> > > > 
> > > > I agree here, we should either say "look, we don't really support big
> > > > repositories because [explanation here], unless you [workarounds
> > > > here]" OR we should work to improve the support we do have. Of course,
> > > > the latter option does not magically create developer time to work on
> > > > that, but if we do go that way we should at least tell people that we
> > > > are aware of the problems and that it's on the global TODO list (not
> > > > necessarily on anyone's personal TODO list though).
> > > 
> > > For the record... I at least am aware of the problem and it is indeed on 
> > > my personal git todo list.  Not that I have a clear solution yet (I've 
> > > been pondering on some git packing issues for almost 4 years now).
> > > 
> > > Still, in this particular case, the problem appears to be unclear to me, 
> > > like "this shouldn't be so bad".
> > 
> > It's not primarily pack-objects, I think. It's the rev-list that's run
> > by upload-pack.  Running "git rev-list --objects --all" on that repo
> > eats about 2G RSS, easily killing the system's cache on a small box,
> > leading to swapping and a painful time reading the packfile contents
> > afterwards to send them to the client.
> 
> Than I think that "packfile caching" GSoC project (which is IIRC
> "object enumeration caching", or at least includes it) should help
> here.

NO!

Please people stop being so creative with all sort of ways to simply 
avoid the real issue and focussing on a real fix.  Git has not become 
what it is today by the accumulation of workarounds and ignorance of 
fundamental issues.

Having git-rev-list consume about 2G RSS for the enumeration of 4M 
objects is simply inacceptable, period.  This is the equivalent of 500 
bytes per object pinned in memory on average, just for listing object, 
which is completely silly. We ought to do better than that.


Nicolas

^ permalink raw reply

* Re: Performance issue: initial git clone causes massive repack
From: Jakub Narebski @ 2009-04-07 13:37 UTC (permalink / raw)
  To: Nicolas Pitre
  Cc: Björn Steinbrink, Sverre Rabbelier, david, Junio C Hamano,
	Nicolas Sebrecht, Robin H. Johnson, Git Mailing List
In-Reply-To: <alpine.LFD.2.00.0904070903020.6741@xanadu.home>

On Tue, 7 Apr 2009, Nicolas Pitre wrote:
> On Tue, 7 Apr 2009, Jakub Narebski wrote:
>> Björn Steinbrink <B.Steinbrink@gmx.de> writes:

[...]
>>> It's not primarily pack-objects, I think. It's the rev-list that's run
>>> by upload-pack.  Running "git rev-list --objects --all" on that repo
>>> eats about 2G RSS, easily killing the system's cache on a small box,
>>> leading to swapping and a painful time reading the packfile contents
>>> afterwards to send them to the client.
>> 
>> Than I think that "packfile caching" GSoC project (which is IIRC
>> "object enumeration caching", or at least includes it) should help
>> here.
> 
> NO!
> 
> Please people stop being so creative with all sort of ways to simply 
> avoid the real issue and focussing on a real fix.  Git has not become 
> what it is today by the accumulation of workarounds and ignorance of 
> fundamental issues.
> 
> Having git-rev-list consume about 2G RSS for the enumeration of 4M 
> objects is simply inacceptable, period.  This is the equivalent of 500 
> bytes per object pinned in memory on average, just for listing object, 
> which is completely silly. We ought to do better than that.

I have thought that the large amount of memory consumed by git-rev-list
was caused by not-so-sequential access to very large packfile (1.5GB+ if
I remember correctly), which I thought causes the whole packfile to be
mmapped and not only window, plus large amount of objects in 300MB+ mem
range or something; those both would account for around 2GB.

Besides even if git-rev-list wouldn't take so much memory, object
enumeration caching would still help with CPU load... admittedly less.

-- 
Jakub Narebski
Poland

^ permalink raw reply


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