Git development
 help / color / mirror / Atom feed
* [PATCH 4/9] Teach git var about GIT_EDITOR
From: Jonathan Nieder @ 2009-11-12  0:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ben Walton, Johannes Sixt, David Roundy, GIT List
In-Reply-To: <20091111235100.GA1140@progeny.tock>

Expose the command used by launch_editor() for scripts to use.
This should allow one to avoid searching for a proper editor
separately in each command.

git_editor(void) uses the logic to decide which editor to use
that used to live in launch_editor().  The function returns NULL
if there is no suitable editor; the caller is expected to issue
an error message when appropriate.

launch_editor() uses git_editor() and gives the error message the
same way as before when EDITOR is not set.

"git var GIT_EDITOR" gives the editor name, or an error message
when there is no appropriate one.

"git var -l" gives GIT_EDITOR=name only if there is an
appropriate editor.

Originally-submitted-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Changes from the version in pu:
 * unsquashed with the previous patch;
 * replaces Hannes’s sign-off with something more descriptive (see
<http://thread.gmane.org/gmane.comp.version-control.git/131471/focus=131851>);
 * nicer commit message based on Junio’s summary.

 Documentation/git-var.txt |    8 ++++++++
 cache.h                   |    1 +
 editor.c                  |   14 ++++++++++++--
 var.c                     |   16 +++++++++++++++-
 4 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index e2f4c09..89e4b4f 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -36,6 +36,14 @@ GIT_AUTHOR_IDENT::
 GIT_COMMITTER_IDENT::
     The person who put a piece of code into git.
 
+GIT_EDITOR::
+    Text editor for use by git commands.  The value is meant to be
+    interpreted by the shell when it is used.  Examples: `~/bin/vi`,
+    `$SOME_ENVIRONMENT_VARIABLE`, `"C:\Program Files\Vim\gvim.exe"
+    --nofork`.  The order of preference is the `$GIT_EDITOR`
+    environment variable, then `core.editor` configuration, then
+    `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
+
 Diagnostics
 -----------
 You don't exist. Go away!::
diff --git a/cache.h b/cache.h
index 96840c7..311cfe1 100644
--- a/cache.h
+++ b/cache.h
@@ -750,6 +750,7 @@ extern const char *git_author_info(int);
 extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
+extern const char *git_editor(void);
 
 struct checkout {
 	const char *base_dir;
diff --git a/editor.c b/editor.c
index 3f13751..70618f1 100644
--- a/editor.c
+++ b/editor.c
@@ -2,7 +2,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+const char *git_editor(void)
 {
 	const char *editor = getenv("GIT_EDITOR");
 	const char *terminal = getenv("TERM");
@@ -16,11 +16,21 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		editor = getenv("EDITOR");
 
 	if (!editor && terminal_is_dumb)
-		return error("terminal is dumb, but EDITOR unset");
+		return NULL;
 
 	if (!editor)
 		editor = "vi";
 
+	return editor;
+}
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	const char *editor = git_editor();
+
+	if (!editor)
+		return error("Terminal is dumb, but EDITOR unset");
+
 	if (strcmp(editor, ":")) {
 		size_t len = strlen(editor);
 		int i = 0;
diff --git a/var.c b/var.c
index dacbaab..b502487 100644
--- a/var.c
+++ b/var.c
@@ -8,6 +8,16 @@
 
 static const char var_usage[] = "git var [-l | <variable>]";
 
+static const char *editor(int flag)
+{
+	const char *pgm = git_editor();
+
+	if (!pgm && flag & IDENT_ERROR_ON_NO_NAME)
+		die("Terminal is dumb, but EDITOR unset");
+
+	return pgm;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -15,14 +25,18 @@ struct git_var {
 static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
+	{ "GIT_EDITOR", editor },
 	{ "", NULL },
 };
 
 static void list_vars(void)
 {
 	struct git_var *ptr;
+	const char *val;
+
 	for (ptr = git_vars; ptr->read; ptr++)
-		printf("%s=%s\n", ptr->name, ptr->read(0));
+		if ((val = ptr->read(0)))
+			printf("%s=%s\n", ptr->name, val);
 }
 
 static const char *read_var(const char *var)
-- 
1.6.5.2

^ permalink raw reply related

* [PATCH 5/9] Teach git var about GIT_PAGER
From: Jonathan Nieder @ 2009-11-12  0:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ben Walton, Johannes Sixt, David Roundy, GIT List
In-Reply-To: <20091111235100.GA1140@progeny.tock>

Expose the command found by setup_pager() for scripts to use.
Scripts can use this to avoid repeating the logic to look for a
proper pager in each command.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
The rest of the series is unchanged from pu.

 Documentation/git-var.txt |    6 ++++++
 cache.h                   |    1 +
 pager.c                   |   18 +++++++++++++++---
 var.c                     |   10 ++++++++++
 4 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 89e4b4f..ef6aa81 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -44,6 +44,12 @@ GIT_EDITOR::
     environment variable, then `core.editor` configuration, then
     `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
 
+GIT_PAGER::
+    Text viewer for use by git commands (e.g., 'less').  The value
+    is meant to be interpreted by the shell.  The order of preference
+    is the `$GIT_PAGER` environment variable, then `core.pager`
+    configuration, then `$PAGER`, and then finally 'less'.
+
 Diagnostics
 -----------
 You don't exist. Go away!::
diff --git a/cache.h b/cache.h
index 311cfe1..5aaa4ba 100644
--- a/cache.h
+++ b/cache.h
@@ -751,6 +751,7 @@ extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
 extern const char *git_editor(void);
+extern const char *git_pager(void);
 
 struct checkout {
 	const char *base_dir;
diff --git a/pager.c b/pager.c
index 86facec..0b63d99 100644
--- a/pager.c
+++ b/pager.c
@@ -44,12 +44,14 @@ static void wait_for_pager_signal(int signo)
 	raise(signo);
 }
 
-void setup_pager(void)
+const char *git_pager(void)
 {
-	const char *pager = getenv("GIT_PAGER");
+	const char *pager;
 
 	if (!isatty(1))
-		return;
+		return NULL;
+
+	pager = getenv("GIT_PAGER");
 	if (!pager) {
 		if (!pager_program)
 			git_config(git_default_config, NULL);
@@ -60,6 +62,16 @@ void setup_pager(void)
 	if (!pager)
 		pager = "less";
 	else if (!*pager || !strcmp(pager, "cat"))
+		pager = NULL;
+
+	return pager;
+}
+
+void setup_pager(void)
+{
+	const char *pager = git_pager();
+
+	if (!pager)
 		return;
 
 	spawned_pager = 1; /* means we are emitting to terminal */
diff --git a/var.c b/var.c
index b502487..d9892f8 100644
--- a/var.c
+++ b/var.c
@@ -18,6 +18,15 @@ static const char *editor(int flag)
 	return pgm;
 }
 
+static const char *pager(int flag)
+{
+	const char *pgm = git_pager();
+
+	if (!pgm)
+		pgm = "cat";
+	return pgm;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -26,6 +35,7 @@ static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
 	{ "GIT_EDITOR", editor },
+	{ "GIT_PAGER", pager },
 	{ "", NULL },
 };
 
-- 
1.6.5.2

^ permalink raw reply related

* [PATCH 6/9] add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR"
From: Jonathan Nieder @ 2009-11-12  0:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ben Walton, Johannes Sixt, David Roundy, GIT List
In-Reply-To: <20091111235100.GA1140@progeny.tock>

Use the new "git var GIT_EDITOR" feature to decide what editor to
use, instead of duplicating its logic elsewhere.  This should make
the behavior of commands in edge cases (e.g., editor names with
spaces) a little more consistent.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Unchanged from pu.

 Documentation/config.txt         |    4 +---
 Documentation/git-commit.txt     |    2 +-
 Documentation/git-send-email.txt |    4 ++--
 contrib/fast-import/git-p4       |    5 +----
 git-add--interactive.perl        |    3 +--
 git-send-email.perl              |    3 ++-
 git-sh-setup.sh                  |   19 ++++++-------------
 git-svn.perl                     |    5 ++---
 8 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d1e2120..5181b77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -387,9 +387,7 @@ core.editor::
 	Commands such as `commit` and `tag` that lets you edit
 	messages by launching an editor uses the value of this
 	variable when it is set, and the environment variable
-	`GIT_EDITOR` is not set.  The order of preference is
-	`GIT_EDITOR` environment, `core.editor`, `VISUAL` and
-	`EDITOR` environment variables and then finally `vi`.
+	`GIT_EDITOR` is not set.  See linkgit:git-var[1].
 
 core.pager::
 	The command that git will use to paginate output.  Can
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0578a40..3ea80c8 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -323,7 +323,7 @@ ENVIRONMENT AND CONFIGURATION VARIABLES
 The editor used to edit the commit log message will be chosen from the
 GIT_EDITOR environment variable, the core.editor configuration variable, the
 VISUAL environment variable, or the EDITOR environment variable (in that
-order).
+order).  See linkgit:git-var[1] for details.
 
 HOOKS
 -----
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 767cf4d..c85d7f4 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -60,8 +60,8 @@ The --bcc option must be repeated for each user you want on the bcc list.
 The --cc option must be repeated for each user you want on the cc list.
 
 --compose::
-	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an
-	introductory message for the patch series.
+	Invoke a text editor (see GIT_EDITOR in linkgit:git-var[1])
+	to edit an introductory message for the patch series.
 +
 When '--compose' is used, git send-email will use the From, Subject, and
 In-Reply-To headers specified in the message. If the body of the message
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index e710219..48059d0 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -729,13 +729,10 @@ class P4Submit(Command):
             tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
             tmpFile.close()
             mtime = os.stat(fileName).st_mtime
-            defaultEditor = "vi"
-            if platform.system() == "Windows":
-                defaultEditor = "notepad"
             if os.environ.has_key("P4EDITOR"):
                 editor = os.environ.get("P4EDITOR")
             else:
-                editor = os.environ.get("EDITOR", defaultEditor);
+                editor = read_pipe("git var GIT_EDITOR")
             system(editor + " " + fileName)
 
             response = "y"
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 69aeaf0..0c74e5c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -987,8 +987,7 @@ sub edit_hunk_manually {
 EOF
 	close $fh;
 
-	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
-		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	chomp(my $editor = run_cmd_pipe(qw(git var GIT_EDITOR)));
 	system('sh', '-c', $editor.' "$@"', $editor, $hunkfile);
 
 	if ($? != 0) {
diff --git a/git-send-email.perl b/git-send-email.perl
index a0279de..4f5da4e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -162,7 +162,8 @@ my $compose_filename;
 
 # Handle interactive edition of files.
 my $multiedit;
-my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+my $editor = Git::command_oneline('var', 'GIT_EDITOR');
+
 sub do_edit {
 	if (defined($multiedit) && !$multiedit) {
 		map {
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c41c2f7..99cceeb 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -99,19 +99,12 @@ set_reflog_action() {
 }
 
 git_editor() {
-	: "${GIT_EDITOR:=$(git config core.editor)}"
-	: "${GIT_EDITOR:=${VISUAL:-${EDITOR}}}"
-	case "$GIT_EDITOR,$TERM" in
-	,dumb)
-		echo >&2 "No editor specified in GIT_EDITOR, core.editor, VISUAL,"
-		echo >&2 "or EDITOR. Tried to fall back to vi but terminal is dumb."
-		echo >&2 "Please set one of these variables to an appropriate"
-		echo >&2 "editor or run $0 with options that will not cause an"
-		echo >&2 "editor to be invoked (e.g., -m or -F for git-commit)."
-		exit 1
-		;;
-	esac
-	eval "${GIT_EDITOR:=vi}" '"$@"'
+	if test -z "${GIT_EDITOR:+set}"
+	then
+		GIT_EDITOR="$(git var GIT_EDITOR)" || return $?
+	fi
+
+	eval "$GIT_EDITOR" '"$@"'
 }
 
 is_bare_repository () {
diff --git a/git-svn.perl b/git-svn.perl
index 6a3b501..42c9a72 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1321,9 +1321,8 @@ sub get_commit_entry {
 	close $log_fh or croak $!;
 
 	if ($_edit || ($type eq 'tree')) {
-		my $editor = $ENV{VISUAL} || $ENV{EDITOR} || 'vi';
-		# TODO: strip out spaces, comments, like git-commit.sh
-		system($editor, $commit_editmsg);
+		chomp(my $editor = command_oneline(qw(var GIT_EDITOR)));
+		system('sh', '-c', $editor.' "$@"', $editor, $commit_editmsg);
 	}
 	rename $commit_editmsg, $commit_msg or croak $!;
 	{
-- 
1.6.5.2

^ permalink raw reply related

* [PATCH 7/9] am -i, git-svn: use "git var GIT_PAGER"
From: Jonathan Nieder @ 2009-11-12  0:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ben Walton, Johannes Sixt, David Roundy, GIT List
In-Reply-To: <20091111235100.GA1140@progeny.tock>

Use the new "git var GIT_PAGER" command to ask what pager to use.

Without this change, the core.pager configuration is ignored by
these commands.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Unchanged.

 git-am.sh    |    5 ++++-
 git-svn.perl |    6 ++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index c132f50..2649487 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -649,7 +649,10 @@ do
 		[eE]*) git_editor "$dotest/final-commit"
 		       action=again ;;
 		[vV]*) action=again
-		       LESS=-S ${PAGER:-less} "$dotest/patch" ;;
+		       : ${GIT_PAGER=$(git var GIT_PAGER)}
+		       : ${LESS=-FRSX}
+		       export LESS
+		       $GIT_PAGER "$dotest/patch" ;;
 		*)     action=again ;;
 		esac
 	    done
diff --git a/git-svn.perl b/git-svn.perl
index 42c9a72..c4ca548 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -5171,10 +5171,8 @@ sub git_svn_log_cmd {
 
 # adapted from pager.c
 sub config_pager {
-	$pager ||= $ENV{GIT_PAGER} || $ENV{PAGER};
-	if (!defined $pager) {
-		$pager = 'less';
-	} elsif (length $pager == 0 || $pager eq 'cat') {
+	chomp(my $pager = command_oneline(qw(var GIT_PAGER)));
+	if ($pager eq 'cat') {
 		$pager = undef;
 	}
 	$ENV{GIT_PAGER_IN_USE} = defined($pager);
-- 
1.6.5.2

^ permalink raw reply related

* [PATCH 8/9] Provide a build time default-editor setting
From: Jonathan Nieder @ 2009-11-12  0:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ben Walton, Johannes Sixt, David Roundy, GIT List
In-Reply-To: <20091111235100.GA1140@progeny.tock>

Provide a DEFAULT_EDITOR knob to allow setting the fallback
editor to use instead of vi (when VISUAL, EDITOR, and GIT_EDITOR
are unset).  The value can be set at build time according to a
system’s policy.  For example, on Debian systems, the default
editor should be the 'editor' command.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Unchanged.

 Makefile          |   17 +++++++++++++++++
 editor.c          |    6 +++++-
 t/t7005-editor.sh |   37 +++++++++++++++++++++++++------------
 3 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index 268aede..625866c 100644
--- a/Makefile
+++ b/Makefile
@@ -200,6 +200,14 @@ all::
 # memory allocators with the nedmalloc allocator written by Niall Douglas.
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
+#
+# Define DEFAULT_EDITOR to a sensible editor command (defaults to "vi") if you
+# want to use something different.  The value will be interpreted by the shell
+# if necessary when it is used.  Examples:
+#
+#   DEFAULT_EDITOR='~/bin/vi',
+#   DEFAULT_EDITOR='$GIT_FALLBACK_EDITOR',
+#   DEFAULT_EDITOR='"C:\Program Files\Vim\gvim.exe" --nofork'
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1363,6 +1371,15 @@ BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
 	$(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
+# Quote for C
+
+ifdef DEFAULT_EDITOR
+DEFAULT_EDITOR_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_EDITOR)))"
+DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
+
+BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
+endif
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
diff --git a/editor.c b/editor.c
index 70618f1..615f575 100644
--- a/editor.c
+++ b/editor.c
@@ -2,6 +2,10 @@
 #include "strbuf.h"
 #include "run-command.h"
 
+#ifndef DEFAULT_EDITOR
+#define DEFAULT_EDITOR "vi"
+#endif
+
 const char *git_editor(void)
 {
 	const char *editor = getenv("GIT_EDITOR");
@@ -19,7 +23,7 @@ const char *git_editor(void)
 		return NULL;
 
 	if (!editor)
-		editor = "vi";
+		editor = DEFAULT_EDITOR;
 
 	return editor;
 }
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index a95fe19..5257f4d 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -4,7 +4,21 @@ test_description='GIT_EDITOR, core.editor, and stuff'
 
 . ./test-lib.sh
 
-for i in GIT_EDITOR core_editor EDITOR VISUAL vi
+unset EDITOR VISUAL GIT_EDITOR
+
+test_expect_success 'determine default editor' '
+
+	vi=$(TERM=vt100 git var GIT_EDITOR) &&
+	test -n "$vi"
+
+'
+
+if ! expr "$vi" : '^[a-z]*$' >/dev/null
+then
+	vi=
+fi
+
+for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
 do
 	cat >e-$i.sh <<-EOF
 	#!$SHELL_PATH
@@ -12,19 +26,18 @@ do
 	EOF
 	chmod +x e-$i.sh
 done
-unset vi
-mv e-vi.sh vi
-unset EDITOR VISUAL GIT_EDITOR
+
+if ! test -z "$vi"
+then
+	mv e-$vi.sh $vi
+fi
 
 test_expect_success setup '
 
-	msg="Hand edited" &&
+	msg="Hand-edited" &&
+	test_commit "$msg" &&
 	echo "$msg" >expect &&
-	git add vi &&
-	test_tick &&
-	git commit -m "$msg" &&
-	git show -s --pretty=oneline |
-	sed -e "s/^[0-9a-f]* //" >actual &&
+	git show -s --format=%s > actual &&
 	diff actual expect
 
 '
@@ -54,7 +67,7 @@ test_expect_success 'dumb should prefer EDITOR to VISUAL' '
 
 TERM=vt100
 export TERM
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	unset EDITOR VISUAL GIT_EDITOR
@@ -78,7 +91,7 @@ done
 
 unset EDITOR VISUAL GIT_EDITOR
 git config --unset-all core.editor
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	case "$i" in
-- 
1.6.5.2

^ permalink raw reply related

* [PATCH 9/9] Provide a build time default-pager setting
From: Jonathan Nieder @ 2009-11-12  0:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ben Walton, Johannes Sixt, David Roundy, GIT List
In-Reply-To: <20091111235100.GA1140@progeny.tock>

Provide a DEFAULT_PAGER knob so packagers can set the fallback
pager to something appropriate during the build.

Examples:

On (old) solaris systems, /usr/bin/less (typically the first less
found) doesn't understand the default arguments (FXRS), which
forces users to alter their environment (PATH, GIT_PAGER, LESS,
etc) or have a local or global gitconfig before paging works as
expected.

On Debian systems, by policy packages must fall back to the
'pager' command, so that changing the target of the
/usr/bin/pager symlink changes the default pager for all packages
at once.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
As in pu.

 Makefile |   11 +++++++++++
 pager.c  |    6 +++++-
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 625866c..18fc50a 100644
--- a/Makefile
+++ b/Makefile
@@ -201,6 +201,10 @@ all::
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
 #
+# Define DEFAULT_PAGER to a sensible pager command (defaults to "less") if
+# you want to use something different.  The value will be interpreted by the
+# shell at runtime when it is used.
+#
 # Define DEFAULT_EDITOR to a sensible editor command (defaults to "vi") if you
 # want to use something different.  The value will be interpreted by the shell
 # if necessary when it is used.  Examples:
@@ -1380,6 +1384,13 @@ DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
 BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
 endif
 
+ifdef DEFAULT_PAGER
+DEFAULT_PAGER_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_PAGER)))"
+DEFAULT_PAGER_CQ_SQ = $(subst ','\'',$(DEFAULT_PAGER_CQ))
+
+BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)'
+endif
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
diff --git a/pager.c b/pager.c
index 0b63d99..92c03f6 100644
--- a/pager.c
+++ b/pager.c
@@ -2,6 +2,10 @@
 #include "run-command.h"
 #include "sigchain.h"
 
+#ifndef DEFAULT_PAGER
+#define DEFAULT_PAGER "less"
+#endif
+
 /*
  * This is split up from the rest of git so that we can do
  * something different on Windows.
@@ -60,7 +64,7 @@ const char *git_pager(void)
 	if (!pager)
 		pager = getenv("PAGER");
 	if (!pager)
-		pager = "less";
+		pager = DEFAULT_PAGER;
 	else if (!*pager || !strcmp(pager, "cat"))
 		pager = NULL;
 
-- 
1.6.5.2

^ permalink raw reply related

* Re: [PATCHv2] Update gitworkflows man page to include release workflow
From: Raman Gupta @ 2009-11-12  0:32 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Thomas Rast
In-Reply-To: <7vzl6soniu.fsf@alter.siamese.dyndns.org>

Thanks for your review. Comments inline.

Junio C Hamano wrote:
> rocketraman@fastmail.fm writes:
> 
>> From: Raman Gupta <raman@rocketraman.com>
>>
>> The gitworkflows man page currently provides an overview of the workflows
>> used by git.git itself to serve as inspiration for people to use when
>> designing their own workflows. The current man page does a reasonable
>> job at describing the development process, but it does not contain any
>> guidance as to the workflow used for releases. Now add a basic
>> introduction to the branch management required for a release, so that a
>> reader may understand how the maint, master, next, and topic branches are
>> affected.
>> ---
> 
> Is this meant to show how git.git does its release to serve as an
> inspiration to others?  The document does not seem to describe how I make
> releases.

Here is the existing intro to gitworkflows:

===================
This document attempts to write down and motivate some of the workflow
elements used for git.git itself. Many ideas apply in general, though
the full workflow is rarely required for smaller projects with fewer
people involved.

We formulate a set of rules for quick reference, while the prose tries
to motivate each of them. Do not always take them literally; you
should value good reasons for your actions higher than manpages such
as this one.
===================

It is in this spirit that I am attempting to add to this document in
relation to the release process. If after you read through this email
you don't agree this patch has any value in gitworkflows, let me know
so that I can stop wasting my time and yours.

>> diff --git a/Documentation/gitworkflows.txt b/Documentation/gitworkflows.txt
>> index 2b021e3..69b789a 100644
>> --- a/Documentation/gitworkflows.txt
>> +++ b/Documentation/gitworkflows.txt
>> @@ -348,6 +348,103 @@ in patches to figure out the merge base.  See linkgit:git-am[1] for
>>  other options.
>>  
>>  
>> +RELEASE WORKFLOW
>> +----------------
>> +
>> +The maintainer may use the following release workflow:
> 
> Please set the tone straight.  If this is to suggest various possible
> workflows in general vague terms, "may use" would be good.  If this is to
> precisely describe what I do, then there won't be "you could do this, or
> you could do that."  Your "may use" suggests the former, but the commit
> log message claims the latter.  Which document are you writing?

Ok. The current document is inconsistent. In places it uses "the
maintainer" and in other places it uses "you". In any case, it seems
that the "maintainer" here is not "Junio Hamano" -- rather, it is the
reader.

Let me create a separate (and first) cleanup patch to fix the existing
inconsistencies in this man page. I would prefer to use the pronoun
"you" consistently as also suggested by Thomas Rast.

In addition, I will update the commit log message to be consistent.

>
> Assuming that you are writing what I do...
>
>> +He first tags the tip of 'master' with a release tag, then he updates
>> +the 'maint' branch to the current tip of 'master' for managing future
>> +maintenance fixes on the current release, and lastly he optionally
>> +rebuilds 'next' from the tip of 'master'.
> 
> Not in that order.

Ok, I'll work on the order.

> 	- doubly make sure that there is nothing left in 'maint' that
> 	  is not in 'master';
> 	- review 'master' more thoroughly than usual;

As per the intro to the man page, I think you will agree these items
are not required. They follow under the category of the user thinking
about what they are doing -- the man page is not meant to provide the
user with a "monkey-see, monkey-do" series of steps.

> 	- review RelNotes symlink, Documentation/RelNotes-X.Y.Z.txt,
>           the stalenotes section in Documentation/git.git, and
>           GIT-VERSION-GEN for the last time;
>         - tag it;
>         - review it again for the last time;
> 	- test on buildfarm;
> 	- cut tarball;
>         - cut RPM on FC11 i386 and FC11 x86_64;

I will update the commit log message to show that my intent was to
talk about how the git.git integration and topic branches are affected
by the release rather than providing a complete project release
process. General items like release notes, version files, reviews,
tests, and cutting distribution tarballs are not specific to git.git
nor git. For git.git, these items better belong in MaintNotes and the
release checklist.txt (as they do) rather than this user distributed
man page.

>         - push the tag and master branch alone to the public server---this
>           triggers an autobuilder for documentation pages, updates man and
>           html branches and documentation tarballs;

I think it makes sense to include some verbiage around the push
aspect, as it is part of the distributed nature of git. I'll add this.
The autobuilder is not git specific so should be excluded via the
logic above -- except perhaps the hook part, which I could mention in
general terms when discussing the push e.g.

"You may decide to use a hook script on the public repository to
initiate release-related items such as building documentation."

> When making a maintenance release, everything is the same except that
> 'maint' is used instead of 'master'.

Good point -- I will add a section about maintenance releases.

> Then, after all the release task on 'master' (or 'maint') is done,
> propagate that upwards (i.e. merge 'master' to 'next' and 'pu').

Will add this.

> Merging 'master' to 'maint' is done totally as a separate step, often a
> few days later, "Now the big release is done, let's start maintenance
> track for that relase".
> 
> And then after that, 'next' may be rebuilt.

Ok, I'll change the order/wording accordingly.

>> +Release Tagging
>> +~~~~~~~~~~~~~~~
>> +
>> +The new feature release is tagged on 'master' with a tag matching
>> +vX.Y.Z, where X.Y.Z is the new feature release version.
>> +
>> +.Release tagging
>> +[caption="Recipe: "]
>> +=====================================
>> +`git tag -s -m "GIT X.Y.Z" vX.Y.Z master`
>> +=====================================
> 
> There is no incorrect information here, but I do not think there is
> anything particularly worth saying here, either.  It is in "git tag"
> manpage and anybody can run "git cat-file tag v1.6.3" to learn what is in
> there.

The intention of this is not to illustrate the "git tag" syntax but
rather so that the user understands the tag description and naming
conventions used by git.git -- this prompts them mentally to consider
defining conventions for their project. Therefore, I'd like to keep this.

>> +Maintenance branch update
>> +~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This section largely overlaps with Documentation/howto/maintain-git.txt; I
> am starting to doubt if we even need a new section in the workflows
> document.  Perhaps we could have a release management section in the
> Documentation/howto/maintain-git.txt, though.

Based on my comments and changes above, do you still have this doubt?
 Remember that the audience of a man page is not the git team -- it is
users. A new user to git who is considering developing branch
processes and conventions around their git project will have access to
a man page such as gitworkflows but will not have (easy) access to
maintain-git.txt.

This is in fact why I decided to make this patch submission -- I found
gitworkflows to be very helpful in creating a layout for my git
projects, but found that gitworkflows was missing any guidance as to
what to do with my integration and topic branches when I needed to cut
a release. It was a while before I found maintain-git.txt, and most
users will never find it.

>> +[caption="Recipe: "]
>> +=====================================
>> +* `git checkout maint`
>> +* `git merge master`
>> +=====================================
>> +
>> +This updates 'maint' from 'master', while preserving the 'maint'
>> +reflog.
>> +
>> +An alternative approach to updating the 'maint' branch is to run
>> +
>> +  $ git branch -f maint master
> 
> As I already said, I never do this "alternative", and I do not want
> anybody who will take over git.git maintenance to do so.  There is no
> reason to encourage nor even mention "branch -f" here.  As 'maint' is
> supposed to be a strict subset, pulling 'master' to 'maint' should fast
> forward and otherwise you (the maintainer) would notice that there was a
> mistake made.  If you use "branch -f", you will never notice.

I know you do not do this alternative, however I added it as per our
previous discussion. I quote from
http://article.gmane.org/gmane.comp.version-control.git/115183:

>> I'll add some discussion about the branch -f bit -- I hope you agree
>> that in this document that is distributed with git, some
>> beginner-level explanation of the difference between the branch -f and
>> the merge approach is warranted?
> 
> Surely and thanks.

I will try to make the reason why "branch -f" is not a good option
more clear. I'd like to keep this because many newbies coming from VCs
like subversion will default to taking the "branch -f" approach
because that is conceptually closer to the way subversion works (I
know I did).

Thanks for your comments.

Cheers,
Raman

^ permalink raw reply

* [PATCH] Update 'git remote' usage and man page to match.
From: Tim Henigan @ 2009-11-12  1:56 UTC (permalink / raw)
  To: git

This commit:

1) Removes documentation of '--verbose' from the synopsis portion
of the usage string since it is a general option.

2) Removes the 'remote' option from 'git remote update' in the
man page.  This option had already been removed from the usage
string in the code, but the man page was not updated to match.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

 Documentation/git-remote.txt |    4 ++--
 builtin-remote.c             |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 82a3d29..32ff95b 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -9,14 +9,14 @@ git-remote - manage set of tracked repositories
 SYNOPSIS
 --------
 [verse]
-'git remote' [-v | --verbose]
+'git remote'
 'git remote add' [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>
 'git remote rename' <old> <new>
 'git remote rm' <name>
 'git remote set-head' <name> [-a | -d | <branch>]
 'git remote show' [-n] <name>
 'git remote prune' [-n | --dry-run] <name>
-'git remote update' [-p | --prune] [group | remote]...
+'git remote update' [-p | --prune] [group]...

 DESCRIPTION
 -----------
diff --git a/builtin-remote.c b/builtin-remote.c
index 0777dd7..3756e91 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -8,14 +8,14 @@
 #include "refs.h"

 static const char * const builtin_remote_usage[] = {
-	"git remote [-v | --verbose]",
+	"git remote",
 	"git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>",
 	"git remote rename <old> <new>",
 	"git remote rm <name>",
 	"git remote set-head <name> [-a | -d | <branch>]",
 	"git remote show [-n] <name>",
 	"git remote prune [-n | --dry-run] <name>",
-	"git remote [-v | --verbose] update [-p | --prune] [group]",
+	"git remote update [-p | --prune] [group]",
 	NULL
 };

-- 
1.6.5.2.180.gc5b3e.dirty

^ permalink raw reply related

* [PATCH todo] dodoc: detect doctype-xsl version before making docs
From: Jonathan Nieder @ 2009-11-12  2:31 UTC (permalink / raw)
  To: gitster; +Cc: git

The manpages in the man branch of git.git appear to have acquired
the dreaded escaped .ft disease when the docbook-xsl toolchain
was upgraded to v1.74.3.
---
It is hard work avoiding escaped nroff directives in output.  I am a
bit confused at what happened here, since docbook-xsl v1.73.2 should
have required the same settings.

These changes are untested, unfortunately.  I hope they can be of some
help nevertheless.

 dodoc.sh |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/dodoc.sh b/dodoc.sh
index 5cbc269..bf49710 100755
--- a/dodoc.sh
+++ b/dodoc.sh
@@ -77,6 +77,19 @@ asciidoc' 8'.*)
 	export ASCIIDOC8 ;;
 esac
 
+db_version_file=/usr/share/xml/docbook/stylesheet/nwalsh/VERSION
+case "$(grep "<fm:Version>" "$db_version_file" 2>/dev/null)" in
+*1.72*)
+	DOCBOOK_XSL_172=YesPlease
+	export DOCBOOK_XSL_172 ;;
+*1.69.[1-9]* | *1.70* | *1.71.0*)
+	DOCBOOK_SUPPRESS_SP=YesPlease
+	export DOCBOOK_SUPPRESS_SP ;;
+*1.7[3-9]* | *1.6[0-8]*)
+	ASCIIDOC_NO_ROFF=YesPlease
+	export ASCIIDOC_NO_ROFF ;;
+esac
+
 make >./:html.log 2>&1 \
 	-C Documentation -j 2 \
 	WEBDOC_DEST="$DOCREPO/doc-html-inst" install-webdoc || exit
-- 
1.6.5.2

^ permalink raw reply related

* Re: t5541-http-push hanging
From: Shawn O. Pearce @ 2009-11-12  4:26 UTC (permalink / raw)
  To: Brian Gernhardt; +Cc: Git List
In-Reply-To: <B17AB159-E217-4E1F-BEA3-97E5892C13F4@gernhardtsoftware.com>

Brian Gernhardt <brian@gernhardtsoftware.com> wrote:
> My build script for git has been hanging at t5541.2 and I haven't had the tuits to discover why.  Here's what I've gotten so far, in case anyone can figure it out faster:
...
> Initialized empty Git repository in /Users/brian/dev/git/t/trash directory.t5541-http-push/test_repo_clone/.git/
> error: RPC failed; result=22, HTTP code = 500
> ^CFATAL: Unexpected exit with code 130
...
> 
> [Wed Nov 11 06:19:39 2009] [error] [client 127.0.0.1] git-http-backend(59490) malloc: *** error for object 0x100200340: incorrect checksum for freed object - object was probably modified after being freed.
> [Wed Nov 11 06:19:39 2009] [error] [client 127.0.0.1] *** set a breakpoint in malloc_error_break to debug
> [Wed Nov 11 06:19:40 2009] [error] [client 127.0.0.1] Premature end of script headers: git-http-backend

Probably caused by a missing cast:


diff --git a/http-backend.c b/http-backend.c
index f8ea9d7..957e4ef 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -136,7 +136,7 @@ static void hdr_str(const char *name, const char *value)
 
 static void hdr_int(const char *name, size_t value)
 {
-	format_write(1, "%s: %" PRIuMAX "\r\n", name, value);
+	format_write(1, "%s: %" PRIuMAX "\r\n", name, (uintmax_t)value);
 }
 
 static void hdr_date(const char *name, unsigned long when)

-- 
Shawn.

^ permalink raw reply related

* [PATCH] http-backend: Fix bad treatment of uintmax_t in Content-Length
From: Shawn O. Pearce @ 2009-11-12  4:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tarmigan, git
In-Reply-To: <905315640911100910r5116779eh24796fa5788f4aef@mail.gmail.com>

Our Content-Length needs to report an off_t, which could be larger
precision than size_t on this system (e.g. 32 bit binary built with
64 bit large file support).

We also shouldn't be passing a size_t parameter to printf when
we've used PRIuMAX as the format specifier.

Fix both issues by using uintmax_t for the hdr_int() routine,
allowing strbuf's size_t to automatically upcast, and off_t to
always fit.

Also fixed the copy loop we use inside of send_local_file(), we never
actually updated the size variable so we might as well not use it.

Reported-by: Tarmigan <tarmigan+git@gmail.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

  Tarmigan <tarmigan+git@gmail.com> wrote:
  > unhappy.  Curl returns 18 (CURLE_PARTIAL_FILE), the test takes a long
  > time to fail, and the "out" file looks OK (compared to a linux machine
  > where the test passes) expect for "Content-Length: 37847251812411".
  > 
  > Digging into it a bit more with gdb, the call to hdr_int() in
  > http-backend.c looks OK, but then something goes wrong in
  > format_write().  Hmmm it looks like my setup does not like PRIuMAX
  > with size_t, which puts some garbage in the upper bytes of

  Yup, only the right fix is to keep using PRIuMAX... patch below.
  
 http-backend.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index f8ea9d7..7f48406 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -134,7 +134,7 @@ static void hdr_str(const char *name, const char *value)
 	format_write(1, "%s: %s\r\n", name, value);
 }
 
-static void hdr_int(const char *name, size_t value)
+static void hdr_int(const char *name, uintmax_t value)
 {
 	format_write(1, "%s: %" PRIuMAX "\r\n", name, value);
 }
@@ -216,7 +216,6 @@ static void send_local_file(const char *the_type, const char *name)
 	char *buf = xmalloc(buf_alloc);
 	int fd;
 	struct stat sb;
-	size_t size;
 
 	fd = open(p, O_RDONLY);
 	if (fd < 0)
@@ -224,14 +223,12 @@ static void send_local_file(const char *the_type, const char *name)
 	if (fstat(fd, &sb) < 0)
 		die_errno("Cannot stat '%s'", p);
 
-	size = xsize_t(sb.st_size);
-
-	hdr_int(content_length, size);
+	hdr_int(content_length, sb.st_size);
 	hdr_str(content_type, the_type);
 	hdr_date(last_modified, sb.st_mtime);
 	end_headers();
 
-	while (size) {
+	for (;;) {
 		ssize_t n = xread(fd, buf, buf_alloc);
 		if (n < 0)
 			die_errno("Cannot read '%s'", p);
-- 
1.6.5.2.351.g09432

^ permalink raw reply related

* Re: t5541-http-push hanging
From: Shawn O. Pearce @ 2009-11-12  4:49 UTC (permalink / raw)
  To: Brian Gernhardt; +Cc: Git List
In-Reply-To: <B17AB159-E217-4E1F-BEA3-97E5892C13F4@gernhardtsoftware.com>

Brian Gernhardt <brian@gernhardtsoftware.com> wrote:
> My build script for git has been hanging at t5541.2 and I haven't had the tuits to discover why.  Here's what I've gotten so far, in case anyone can figure it out faster:
...
> 	git clone $HTTPD_URL/smart/test_repo.git test_repo_clone
...
> [Wed Nov 11 06:19:39 2009] [error] [client 127.0.0.1] git-http-backend(59490) malloc: *** error for object 0x100200340: incorrect checksum for freed object - object was probably modified after being freed.
> [Wed Nov 11 06:19:39 2009] [error] [client 127.0.0.1] *** set a breakpoint in malloc_error_break to debug

I can't reproduce this on 10.5.8.  Can you try running it in the
debugger and putting a breakpoint where they suggest?  Maybe it
will help give us some context information:

  REQUEST_METHOD=GET \
  PATH_TRANSLATED=`pwd`/.git/info/refs \
  QUERY_STRING=service=git-upload-pack \
  gdb ./git-http-backend

  b malloc_error_break
  run

I suspect that's where our failure is, the code path for the actual
common negotiation and data transfer is shorter, and less likely
to need to do a memory allocation... and therefore a free.

-- 
Shawn.

^ permalink raw reply

* Re: t5541-http-push hanging
From: Brian Gernhardt @ 2009-11-12  5:18 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Git List
In-Reply-To: <20091112044906.GQ11919@spearce.org>


On Nov 11, 2009, at 11:49 PM, Shawn O. Pearce wrote:

> Brian Gernhardt <brian@gernhardtsoftware.com> wrote:
>> My build script for git has been hanging at t5541.2 and I haven't had the tuits to discover why.  Here's what I've gotten so far, in case anyone can figure it out faster:
> ...
>> 	git clone $HTTPD_URL/smart/test_repo.git test_repo_clone
> ...
>> [Wed Nov 11 06:19:39 2009] [error] [client 127.0.0.1] git-http-backend(59490) malloc: *** error for object 0x100200340: incorrect checksum for freed object - object was probably modified after being freed.
>> [Wed Nov 11 06:19:39 2009] [error] [client 127.0.0.1] *** set a breakpoint in malloc_error_break to debug
> 
> I can't reproduce this on 10.5.8.  Can you try running it in the
> debugger and putting a breakpoint where they suggest?  Maybe it
> will help give us some context information:
> 
>  REQUEST_METHOD=GET \
>  PATH_TRANSLATED=`pwd`/.git/info/refs \
>  QUERY_STRING=service=git-upload-pack \
>  gdb ./git-http-backend
> 
>  b malloc_error_break
>  run
> 
> I suspect that's where our failure is, the code path for the actual
> common negotiation and data transfer is shorter, and less likely
> to need to do a memory allocation... and therefore a free.

Interesting.  This command runs to completion.  In the test environment it gives:

Expires: Fri, 01 Jan 1980 00:00:00 GMT
Pragma: no-cache
Cache-Control: no-cache, max-age=0, must-revalidate
Content-Type: application/x-git-upload-pack-advertisement

001e# service=git-upload-pack
0000009b0c973ae9bd51902a28466f3850b543fa66a6aaf4 HEADmulti_ack thin-pack side-band side-band-64k ofs-delta shallow no-progress include-tag multi_ack_detailed
003f0c973ae9bd51902a28466f3850b543fa66a6aaf4 refs/heads/master
0000

(which looks right to me)

access.log has:
127.0.0.1 - - [12/Nov/2009:04:54:38 +0000] "GET /smart/test_repo.git/info/refs?service=git-upload-pack HTTP/1.1" 200 256
127.0.0.1 - - [12/Nov/2009:04:54:38 +0000] "POST /smart/test_repo.git/git-upload-pack HTTP/1.1" 500 538

After a bit of trial and error, I got this:

test_repo.git (BARE:master)$ REQUEST_METHOD=POST PATH_TRANSLATED=`pwd`/git-upload-pack gdb ../../../../../git-http-backend GNU gdb 6.3.50-20050815 (Apple version gdb-1344) (Fri Jul  3 01:19:56 UTC 2009)
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "x86_64-apple-darwin"...Reading symbols for shared libraries ..
warning: Could not find object file "/sw/src/fink.build/libiconv-1.12-3/libiconv-1.12/lib/.libs/iconv.o" - no debug information available for "./iconv.c".


warning: Could not find object file "/sw/src/fink.build/libiconv-1.12-3/libiconv-1.12/lib/.libs/localcharset.o" - no debug information available for "./../libcharset/lib/localcharset.c".


warning: Could not find object file "/sw/src/fink.build/libiconv-1.12-3/libiconv-1.12/lib/.libs/relocatable.o" - no debug information available for "./relocatable.c".

.. done

(gdb) b malloc_error_break 
Breakpoint 1 at 0x4189374bb86951
(gdb) run
Starting program: /Users/brian/dev/git/git-http-backend 
Reading symbols for shared libraries +++. done
git-http-backend(2358) malloc: *** error for object 0x100200390: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug

Breakpoint 1, 0x00007fff84c19951 in malloc_error_break ()
(gdb) bt
#0  0x00007fff84c19951 in malloc_error_break ()
#1  0x00007fff84c1aaa8 in szone_error ()
#2  0x00007fff84b466e3 in tiny_free_list_remove_ptr ()
#3  0x00007fff84b44df7 in szone_realloc ()
#4  0x00007fff84b44b3b in malloc_zone_realloc ()
#5  0x00007fff84b50bd3 in realloc ()
#6  0x000000010004985f in xrealloc (ptr=0x100200370, size=66) at wrapper.c:59
#7  0x0000000100041372 in strbuf_grow (sb=0x7fff5fbfed70, extra=<value temporarily unavailable, due to optimizations>) at strbuf.c:61
#8  0x000000010001e194 in strbuf_addch [inlined] () at /Users/brian/dev/git/strbuf.h:95
#9  0x000000010001e194 in add_path (out=0x7fff5fbfed70, path=0x100200320 "/usr/local/libexec/git-core") at exec_cmd.c:95
#10 0x000000010001e229 in setup_path () at exec_cmd.c:104
#11 0x000000010000226d in main (argc=<value temporarily unavailable, due to optimizations>, argv=<value temporarily unavailable, due to optimizations>) at http-backend.c:641
(gdb) 

I'm lost as to why it's dying doing setup_path.  My guess is that some of the code before here is doing something bad to memory.

~~ Brian

^ permalink raw reply

* Re: get git not to care about permissions
From: Jay Soffian @ 2009-11-12  5:44 UTC (permalink / raw)
  To: sconeman; +Cc: git
In-Reply-To: <26268938.post@talk.nabble.com>

On Wed, Nov 11, 2009 at 4:17 PM, sconeman <schoen@bu.edu> wrote:
> does.  Git doesn't like this and won't even create a bare repository.  Is
> there any way I can get git to ignore permissions and just do what it needs
> to do?

And the output from "git init --bare" is?

j.

^ permalink raw reply

* Re: git-svn problem with v1.6.5
From: Pascal Obry @ 2009-11-12  6:39 UTC (permalink / raw)
  To: Eric Wong; +Cc: Avery Pennarun, adambrewster, git list
In-Reply-To: <20091111224454.GA16178@dcvr.yhbt.net>

Eric,

> Did you try an early version of Adam's patch before it made it into
> git.git by any chance?

No.

> Or, did you by any chance start a fresh import with a v1.6.5 and then
> rsync $GIT_DIR to one created with 1.6.4 and not use --delete with
> rsync?

No. The import has been done with 1.6.4. A filter-branch has been 
applied to fix author names. But AFAIK (will check) it has been done 
with 1.6.4 too.

Pascal.

-- 

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|    http://www.obry.net  -  http://v2p.fr.eu.org
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver keys.gnupg.net --recv-key F949BD3B

^ permalink raw reply

* Re: git-svn problem with v1.6.5
From: Eric Wong @ 2009-11-12  7:11 UTC (permalink / raw)
  To: Pascal Obry; +Cc: Avery Pennarun, adambrewster, git list
In-Reply-To: <4AFBADA9.3060401@obry.net>

Pascal Obry <pascal@obry.net> wrote:
> Eric,
>
>> Did you try an early version of Adam's patch before it made it into
>> git.git by any chance?
>
> No.
>
>> Or, did you by any chance start a fresh import with a v1.6.5 and then
>> rsync $GIT_DIR to one created with 1.6.4 and not use --delete with
>> rsync?
>
> No. The import has been done with 1.6.4. A filter-branch has been  
> applied to fix author names. But AFAIK (will check) it has been done  
> with 1.6.4 too.

Oh...  Any chance that lost commit you couldn't find was clobbered
by filter-branch?  That would explain a lot...

-- 
Eric Wong

^ permalink raw reply

* Re: [PATCH] Re: Clarify documentation on the "ours" merge strategy.
From: Junio C Hamano @ 2009-11-12  7:55 UTC (permalink / raw)
  To: Thomas Rast
  Cc: Nicolas Sebrecht, Baz, Peter Krefting, Git Mailing List,
	Johannes Schindelin, Björn Steinbrink
In-Reply-To: <200911120037.11901.trast@student.ethz.ch>

Thomas Rast <trast@student.ethz.ch> writes:

> Then again, I'm not sure if resolve vs. recursive makes a difference
> in a rebase.  Octopus is weird for a two-head merge, I'm not sure why

 ...

> +	If there is no `-s` option 'git-merge-recursive' is used
> +	instead.  This implies --merge.
> ++
> +Due to the peculiarities of 'git-rebase' (see \--merge above) the only
> +built-in strategy that is actually useful is 'subtree'.

"recursive is just a slower and sometimes buggier alternative to resolve
but can handle renames" may mean "people do not have much reason to choose
resolve over recursive".  But that is quite different from "resolve is not
useful here _due to_ the peculiarities of rebase".  Wouldn't anybody who
thinks "resolve vs. recursive would not make a difference in a rebase"
also think "resolve vs. recursive would not make a difference anywhere"?

58634db (rebase: Allow merge strategies to be used when rebasing,
2006-06-21) added "-m" and "-s" to rebase to solve the problem of rebasing
against an upstream that has moved files.  What the commit actually did
was to use recursive (by default) while giving longer rope to the users by
choosing other strategies with "-s", without making any judgement as to
why other strategies may possibly be useful.

Perhaps there is some different issue at the root of this one.  Why would
anybody be tempted to say "-s ours" while running a rebase?  What did the
user want to see it do (instead of being a no-op because "ours" by
definition ignores the tree the change is replayed from)?

It is easy to dismiss it as a user misconception and it also is tempting
to think that it would be helped with updated description of "ours" to
dispel that misconception, but there may be some user wish that is totally
different from "ours merge" strategy but still can be validly labelled
using a word "ours" by somebody who does not know the way the word "ours"
is used in the git land, and satisfying that unknown user wish might be
the real solution to this issue.

^ permalink raw reply

* Re: [PATCHv2] Update gitworkflows man page to include release workflow
From: Junio C Hamano @ 2009-11-12  8:03 UTC (permalink / raw)
  To: Raman Gupta; +Cc: git, Thomas Rast
In-Reply-To: <4AFB57A3.2020002@fastmail.fm>

Raman Gupta <rocketraman@fastmail.fm> writes:

> Junio C Hamano wrote:
>> 
>> Is this meant to show how git.git does its release to serve as an
>> inspiration to others?  The document does not seem to describe how I make
>> releases.
>
> Here is the existing intro to gitworkflows:
>
> ===================
> This document attempts to write down and motivate some of the workflow
> elements used for git.git itself. Many ideas apply in general, though
> the full workflow is rarely required for smaller projects with fewer
> people involved.
>
> We formulate a set of rules for quick reference, while the prose tries
> to motivate each of them. Do not always take them literally; you
> should value good reasons for your actions higher than manpages such
> as this one.
> ===================
>
> It is in this spirit that I am attempting to add to this document in
> relation to the release process.

Thanks for reminding me of this.

Let me address the second paragraph from the quoted part first.  The
existing document is structured as a quick reference.  Rules and recipes,
with supporting prose to explain them (I do not think "motivate" is a good
way to phrase it, though).

I am not very fond of documents with that style, but that does not mean
nobody should work on nor we should ship such a document.  It just means I
would not be a very good judge for one, and major parts of my response 
unfortunately came from my bias against such a document.

What the first paragraph says is also important.  We will talk about the
workflow elements we actually use here.  Every description of a workflow
element should be read as if we said "this is what we use and it has
worked well for us; we recommend you to imitate it." after it.  Limiting
the recommendation to what we practice ourselves and what worked well for
us keeps the document honest.

In that light, after I re-read your patch and my comments, I think I
should rescind large part of my comments on your "Release Tagging" section
and "Maintenance branch update" section.  They are mostly good as-is, and
they are in line with the goal of the document stated at the beginning.
They describe the workflow elements actually used in git.git in a
quick-reference format.

>>> +RELEASE WORKFLOW
>>> +----------------
>>> +
>>> +The maintainer may use the following release workflow:
>> 
>> Please set the tone straight.  If this is to suggest various possible
>> workflows in general vague terms, "may use" would be good. ...
>
> Ok. The current document is inconsistent. In places it uses "the
> maintainer" and in other places it uses "you". In any case, it seems
> that the "maintainer" here is not "Junio Hamano" -- rather, it is the
> reader.

I wasn't talking about the difference between these two (Junio vs you).
That is not the issue.  I was contrasting between

 - You _may_ choose to do A for release management as the maintainer, or
   do B or do C as alternatives.

and

 - In managing git.git, its maintainer _does_ A when making a release.

As we are writing down what we practice, aka "workflow elements used for
git.git itself", in my comment I was hoping the latter was what you were
writing.  Of course we could say

 - In managing git.git, its maintainer _does_ A when making a release.
   It is conceivable we _could_ also do B or C instead, but we do not
   recommend them.

but I think we are better off without such an addition.

An anti-recommendation against B and C like that does not carry the same
weight as our positive recommendation for A, which is backed by our actual
experience.

"In the past, we tried B but it did not work well for us for such and such
reasons, so we don't recommend you to use B" would be justifiable, and
that is what I mean by "keeping the document honest by limiting the
description to what we use".

But if this document is meant to be a quick reference, such
anti-recommendation should be kept to minimum for the sake of
"quick"-ness.

Also I agree with you that ...

>> 	- review RelNotes symlink, Documentation/RelNotes-X.Y.Z.txt,
>>           the stalenotes section in Documentation/git.git, and
>>           GIT-VERSION-GEN for the last time;
>>         - tag it;
>>         - review it again for the last time;
>> 	- test on buildfarm;
>> 	- cut tarball;
>>         - cut RPM on FC11 i386 and FC11 x86_64;

these (except "tag it") can safely omitted to keep the document focused on
"revision control" aspect of the system.

> The autobuilder is not git specific so should be excluded via the logic
> above -- except perhaps the hook part, which I could mention in general
> terms when discussing the push e.g.
>
> "You may decide to use a hook script on the public repository to
> initiate release-related items such as building documentation."

Again, if we are going to recommend it, we should say "We use post-update
hook in _this_ way", not "You may do ...".

A major difference is that we describe what has worked for us and exactly
how, as opposed to giving suggestions that we ourselves haven't even used
but suspect it might work in a hand-wavy way.

>>> +Maintenance branch update
>>> +~~~~~~~~~~~~~~~~~~~~~~~~~
>> ...
>>> +An alternative approach to updating the 'maint' branch is to run
>>> +
>>> +  $ git branch -f maint master
>> 
>> As I already said, I never do this "alternative", and I do not want
>> ...
>> mistake made.  If you use "branch -f", you will never notice.
>
> I know you do not do this alternative, however ...

Given the stated goal of the document of showing what we actually do and
what we know have worked well for us, I do not think we would want it
mentioned as a valid alternative.

> previous discussion. I quote from
> http://article.gmane.org/gmane.comp.version-control.git/115183:
>
>>> I'll add some discussion about the branch -f bit -- I hope you agree
>>> that in this document that is distributed with git, some
>>> beginner-level explanation of the difference between the branch -f and
>>> the merge approach is warranted?

Explanation of the difference would have sounded like:

    We do not use "branch -f maint master" here; it is different from
    merging master into maint in such and such way and using "branch -f"
    for this purpose only has downsides.

But is it really worth mentioning random approaches that novices might
think of as alternatives and then refuting every one of them?  Should we
also say "don't do 'checkout maint && reset --hard master'"?  Should we
say "'push . master:maint' would work equally well but don't add --force
to that command line"?

The document is an overview of recommended workflows.  If we have one best
current practice for a task, just describing it without covering other
inferiour (or equivalent but not superiour) alternatives to clutter the
text would be better for such a "quick reference" document, I think.

^ permalink raw reply

* Re: [PATCHv3/RFC 4/5] gitweb: Create links leading to 'blame_incremental' using JavaScript
From: Junio C Hamano @ 2009-11-12  8:05 UTC (permalink / raw)
  To: Jakub Narebski
  Cc: Petr Baudis, git, Fredrik Kuivinen, Giuseppe Bilotta,
	Luben Tuikov, Martin Koegler
In-Reply-To: <200911061905.52285.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> But even if incremental blame turns out to be slower than incremental
> blame it still has the advantage of being _incremental_.  You have at 
> least some result soon.

It wasn't it was slow that bothered me, but early implementations of
incremental blame I tried didn't _appear_ as incremental.  That was the
dissapointing part.

At the protocol and implementation level it certainly was feeding data
incrementally to the browser, but the end user experience on the screen
was "click....wait...wait...wait...voila the whole blame appears", not
"click...trickle...trickle...trickle...ah everything is filled".  The
latter obviously is what an incremental one should be aiming for.

No I haven't tried your latest code.  Probably I should.

^ permalink raw reply

* Re: git-svn problem with v1.6.5
From: Pascal Obry @ 2009-11-12  8:11 UTC (permalink / raw)
  To: Eric Wong; +Cc: Avery Pennarun, adambrewster, git list
In-Reply-To: <20091112071121.GA569@dcvr.yhbt.net>

Eric,

> Oh...  Any chance that lost commit you couldn't find was clobbered
> by filter-branch?  That would explain a lot...

I still have the AWS repo as it was from initial import. And I can see
the commit
on this one:

$ git log --all --parents | grep d2cf08bb67e4b7da33a250127aab784f1f2f58d3
commit bd95b2b254286e6f04015b7f58266d66abc134a7
d2cf08bb67e4b7da33a250127aab784f1f2f58d3
commit d2cf08bb67e4b7da33a250127aab784f1f2f58d3
406eaf7e51229f60048ea80cdb6904e33ef59a81

So that's definitely a problem with the filter-branch. Thanks a lot
for your precious help!

Will review the procedure for filtering branches :(

Pascal.

-- 

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

^ permalink raw reply

* Re: [PATCHv2] Update gitworkflows man page to include release workflow
From: Thomas Rast @ 2009-11-12  8:10 UTC (permalink / raw)
  To: Raman Gupta; +Cc: Junio C Hamano, git
In-Reply-To: <4AFB57A3.2020002@fastmail.fm>

Raman Gupta wrote:
> Junio C Hamano wrote:
> > Please set the tone straight.  If this is to suggest various possible
> > workflows in general vague terms, "may use" would be good.  If this is to
> > precisely describe what I do, then there won't be "you could do this, or
> > you could do that."  Your "may use" suggests the former, but the commit
> > log message claims the latter.  Which document are you writing?
> 
> Ok. The current document is inconsistent. In places it uses "the
> maintainer" and in other places it uses "you". In any case, it seems
> that the "maintainer" here is not "Junio Hamano" -- rather, it is the
> reader.
> 
> Let me create a separate (and first) cleanup patch to fix the existing
> inconsistencies in this man page. I would prefer to use the pronoun
> "you" consistently as also suggested by Thomas Rast.

Well, I'm not sure if this is also in reply to my comment

} The current gitworkflows is mostly formulated in the imperative, [...]
} or by directly describing the tools in the third person, [...]

but note that I do not consider the current form to be inconsistent
(though you may of course convince me otherwise).  It addresses the
presumed user with "you", which is not always the maintainer.  For
example, when talking about patch submission we have

  If the maintainer tells you that your patch no longer applies to the
  current upstream, you will have to rebase your topic (you cannot use a
  merge because you cannot format-patch merges):

since the presumed user of a patch-submission workflow is a
contributor, not the maintainer.  Indeed much of the text talks
*about* the workflow used by our esteemed maintainer, but is addressed
to a contributor who wants to understand how it works so he can
participate.

IOW, I'm neither a native speaker nor a professional writer, so you
may of course convince me that there is something to fix.  I am,
however, fairly sure that s/maintainer/you/ and then fixing the
grammar is *not* a good thing.

[BTW, it would have been nice to get a Cc to begin with, since the
entire manpage blames to me.  I noticed the thread anyway, but other
times I do not have the time to scan the entire list.]

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply

* Re: [PATCHv2] Update gitworkflows man page to include release workflow
From: Thomas Rast @ 2009-11-12  8:27 UTC (permalink / raw)
  To: rocketraman; +Cc: git
In-Reply-To: <1257869339-15999-2-git-send-email-rocketraman@fastmail.fm>

Here's my promised review of the whole patch.  Much of the text fits
my understanding of what's actually going on, but Junio will have the
final word on what he actually does (or what a sensible simplification
might be).

rocketraman@fastmail.fm wrote:
> +The current maintenance branch is optionally copied to another branch
> +named with the older release version number (e.g. maint-X.Y.(Z-1)
> +where X.Y.Z is the previous release). This allows for further
> +maintenance releases on the older codebase.

The use of Z-1 confused me; I guess by "previous release" you mean
"the release we just tagged in the last step".  Otherwise the maint
version number would come out wrong.

> +.Update maint to new release
> +[caption="Recipe: "]
> +=====================================
> +* `git checkout maint`
> +* `git merge master`
> +=====================================
> +
> +This updates 'maint' from 'master', while preserving the 'maint'
> +reflog.

I agree with what Junio said in the other mail: it's important at this
point that this was a fast-forward.  (If it's not, master could be
missing some fixes made on maint.)

> +An alternative approach to updating the 'maint' branch is to run
> +
> +  $ git branch -f maint master

In my book the alternative approach is

  git branch -m maint maint-X.Y.(Z-1)
  git branch maint master

I'd rather not teach users to play with loaded guns, much less in a
"good examples of workflows" manpage.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply

* Re: [PATCH] gitk: Add ability to define an alternate temporary directory
From: Jeff King @ 2009-11-12  8:35 UTC (permalink / raw)
  To: David Aguilar; +Cc: Junio C Hamano, Sam Vilain, paulus, git
In-Reply-To: <20091111164451.GA45475@gmail.com>

On Wed, Nov 11, 2009 at 08:44:53AM -0800, David Aguilar wrote:

> gitk writes file@commit and file@commit^ to temporary files
> and diffs them using an external diff tool.
> 
> Shall I reroll with s/GITK_TMPDIR/TMPDIR/ ?

gitk seems to use a very predictable temp filename (".gitk-tmp.$PID").
Have you checked that you are not introducing any security holes by
creating that predictable filename in a publicly writable tmpdir?

It looks like it always tries to "mkdir" the file. Does tcl's mkdir
function barf on an existing directory? If so, then I think we may be
safe from the naive:

  tmp=.gitk-tmp.`pidof_other_users_gitk`
  mkdir $tmp
  ln -s /file/for/other/user/to/destroy /tmp/1

attack. And I think we are not susceptible to races because we fail if
the mkdir fails (instead of doing something stupid like stat followed
by mkdir).

But it has been a long time since I thought about /tmp security, so I
may be forgetting something.

-Peff

^ permalink raw reply

* Re: git-svn problem with v1.6.5
From: Pascal Obry @ 2009-11-12  9:03 UTC (permalink / raw)
  To: Eric Wong; +Cc: Avery Pennarun, adambrewster, git list
In-Reply-To: <a2633edd0911120011u34d69100p1565ab5bf96a7709@mail.gmail.com>

Eric,

> Will review the procedure for filtering branches :(

A quick review does not show something obviously wrong on my side.

The commit on the rewritten repo has a different sha-1 which is
expected. So to me it seems that after a filter-branch on a git-svn
repo it is mandatory to recreate all .rev_map*? Is that right? Is so,
this is the missing piece in the procedure on my side.

Pascal.

-- 

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

^ permalink raw reply

* Re: [PATCHv3/RFC 4/5] gitweb: Create links leading to 'blame_incremental' using JavaScript
From: Jakub Narebski @ 2009-11-12  9:22 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Petr Baudis, git, Fredrik Kuivinen, Giuseppe Bilotta,
	Luben Tuikov, Martin Koegler
In-Reply-To: <7vtyx0f9rd.fsf@alter.siamese.dyndns.org>

On Thu, 12 Nov 2009, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > But even if incremental blame turns out to be slower than incremental
> > blame it still has the advantage of being _incremental_.  You have at 
> > least some result soon.
> 
> It wasn't it was slow that bothered me, but early implementations of
> incremental blame I tried didn't _appear_ as incremental.  That was the
> dissapointing part.
> 
> At the protocol and implementation level it certainly was feeding data
> incrementally to the browser, but the end user experience on the screen
> was "click....wait...wait...wait...voila the whole blame appears", not
> "click...trickle...trickle...trickle...ah everything is filled".  The
> latter obviously is what an incremental one should be aiming for.
> 
> No I haven't tried your latest code.  Probably I should.

The problem with earliest versions of incremental blame _in some browsers_
was that 'onreadystatechange' event was not fired as soon as new part of
blame data was available (truth to be said the definition of this event
is a bit underspecified).  That is why newer versions of interactive blame
use timer (alarm) to check every 1 second if there is something new.

Perhaps interactive blame should use 'onprogress' event instead, which is
well defined... but is in _draft_ of standard (XHR 2.0), and not yet in
established standard.

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