Git development
 help / color / mirror / Atom feed
* Re: git send-email prompting too much
From: Jay Soffian @ 2009-04-05  2:13 UTC (permalink / raw)
  To: Bruce Stephens; +Cc: Dan McGee, git
In-Reply-To: <80hc148hso.fsf@tiny.isode.net>

On Sat, Apr 4, 2009 at 1:12 PM, Bruce Stephens <bruce.stephens@isode.com> wrote:
> Dan McGee <dpmcgee@gmail.com> writes:
>
>> I'm guessing this is related to commit
>> 6e1825186bd052fc1f77b7c8c9a31fbb9a67d90c but I haven't bisected yet.
>> Having to hit enter 10 times ad the Message-ID prompt seemed a bit odd
>> to me. Has anyone else seen this behavior?
>
> Yes.  Apparently one uses \C-d now.

Wait. Did hitting just enter work in the past? Because I tested this
before I made this commit and at least on my machine, hitting enter
just presented the prompt again. So I retained that behavior.

I looked over the Term::ReadLine code and I don't see how just hitting
return ever worked. But if you tell me it did, I believe that and I
can restore that behavior.

Out of curiosity, what is your OS and version, perl version, and what
does does perl -e 'use Term::ReadLine; print
"$Term::ReadLine::VERSION\n"' return?

Thanks,

j.

^ permalink raw reply

* [PATCH 2/2 resend] git-gui: use `git --html-path` to get the location of installed HTML docs
From: Markus Heidelberg @ 2009-04-05  1:48 UTC (permalink / raw)
  To: Junio C Hamano, Shawn O. Pearce; +Cc: git
In-Reply-To: <200904050333.46453.markus.heidelberg@web.de>

Previously a hardcoded path $GIT_EXEC_PATH/../Documentation/ was used to
search for the documentation, when the user has asked for it via menu
"Help -> Online Documentation".
This didn't work for the default directory structure.

To find the path reliably, use the new git command line option, which
returns the correct path.

If the output of `git --html-path` is empty because git is not found or
the option is not yet supported in the installed git, the documentation
from kernel.org is launched. There is no additional guessing of the
right location of the installed docs.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---

Resend since the other mail didn't reach the list. This problem happens
far too often to me.

 git-gui.sh |   33 +++++++++++++++++++++++++++++----
 1 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index b3aa732..259251b 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -122,6 +122,7 @@ unset oguimsg
 set _appname {Git Gui}
 set _gitdir {}
 set _gitexec {}
+set _githtmldir {}
 set _reponame {}
 set _iscygwin {}
 set _search_path {}
@@ -168,6 +169,28 @@ proc gitexec {args} {
 	return [eval [list file join $_gitexec] $args]
 }
 
+proc githtmldir {args} {
+	global _githtmldir
+	if {$_githtmldir eq {}} {
+		if {[catch {set _githtmldir [git --html-path]}]} {
+			# Git not installed or option not yet supported
+			return {}
+		}
+		if {[is_Cygwin]} {
+			set _githtmldir [exec cygpath \
+				--windows \
+				--absolute \
+				$_githtmldir]
+		} else {
+			set _githtmldir [file normalize $_githtmldir]
+		}
+	}
+	if {$args eq {}} {
+		return $_githtmldir
+	}
+	return [eval [list file join $_githtmldir] $args]
+}
+
 proc reponame {} {
 	return $::_reponame
 }
@@ -2591,11 +2614,13 @@ if {![is_MacOSX]} {
 }
 
 
-set doc_path [file dirname [gitexec]]
-set doc_path [file join $doc_path Documentation index.html]
+set doc_path [githtmldir]
+if {$doc_path ne {}} {
+	set doc_path [file join $doc_path index.html]
 
-if {[is_Cygwin]} {
-	set doc_path [exec cygpath --mixed $doc_path]
+	if {[is_Cygwin]} {
+		set doc_path [exec cygpath --mixed $doc_path]
+	}
 }
 
 if {[file isfile $doc_path]} {
-- 
1.6.2.2.405.ge9fc3

^ permalink raw reply related

* [PATCH 1/2] add --html-path to get the location of installed HTML docs
From: Markus Heidelberg @ 2009-04-05  1:35 UTC (permalink / raw)
  To: Junio C Hamano, Shawn O. Pearce; +Cc: git
In-Reply-To: <200904050333.46453.markus.heidelberg@web.de>

This can be used in GUIs to open installed HTML documentation in the
browser.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
 Documentation/git.txt                  |    6 +++++-
 Makefile                               |    1 +
 contrib/completion/git-completion.bash |    1 +
 exec_cmd.c                             |    6 ++++++
 git.c                                  |    5 ++++-
 perl/Git.pm                            |   12 +++++++++++-
 6 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7513c57..2ce5e6b 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -9,7 +9,7 @@ git - the stupid content tracker
 SYNOPSIS
 --------
 [verse]
-'git' [--version] [--exec-path[=GIT_EXEC_PATH]]
+'git' [--version] [--exec-path[=GIT_EXEC_PATH]] [--html-path]
     [-p|--paginate|--no-pager]
     [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE]
     [--help] COMMAND [ARGS]
@@ -178,6 +178,10 @@ help ...`.
 	environment variable. If no path is given, 'git' will print
 	the current setting and then exit.
 
+--html-path::
+	Print the path to wherever your git HTML documentation is installed
+	and exit.
+
 -p::
 --paginate::
 	Pipe all output into 'less' (or if set, $PAGER).
diff --git a/Makefile b/Makefile
index 7867eac..28ccad9 100644
--- a/Makefile
+++ b/Makefile
@@ -1308,6 +1308,7 @@ git.o git.spec \
 exec_cmd.o: exec_cmd.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) \
 		'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
+		'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 		'-DBINDIR="$(bindir_relative_SQ)"' \
 		'-DPREFIX="$(prefix_SQ)"' \
 		$<
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e72ce24..fdc5a24 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1870,6 +1870,7 @@ _git ()
 			--bare
 			--version
 			--exec-path
+			--html-path
 			--work-tree=
 			--help
 			"
diff --git a/exec_cmd.c b/exec_cmd.c
index 408e4e5..25e0521 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -84,6 +84,12 @@ const char *git_exec_path(void)
 	return system_path(GIT_EXEC_PATH);
 }
 
+/* Returns the location to look for git HTML documentation. */
+const char *git_html_path(void)
+{
+	return system_path(GIT_HTML_PATH);
+}
+
 static void add_path(struct strbuf *out, const char *path)
 {
 	if (path && *path) {
diff --git a/git.c b/git.c
index c2b181e..ff934bd 100644
--- a/git.c
+++ b/git.c
@@ -5,7 +5,7 @@
 #include "run-command.h"
 
 const char git_usage_string[] =
-	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
+	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [--html-path] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
 
 const char git_more_info_string[] =
 	"See 'git help COMMAND' for more information on a specific command.";
@@ -75,6 +75,9 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 				puts(git_exec_path());
 				exit(0);
 			}
+		} else if (!strcmp(cmd, "--html-path")) {
+			puts(git_html_path());
+			exit(0);
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
 			use_pager = 1;
 		} else if (!strcmp(cmd, "--no-pager")) {
diff --git a/perl/Git.pm b/perl/Git.pm
index 7d7f2b1..291ff5b 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -56,7 +56,7 @@ require Exporter;
 @EXPORT_OK = qw(command command_oneline command_noisy
                 command_output_pipe command_input_pipe command_close_pipe
                 command_bidi_pipe command_close_bidi_pipe
-                version exec_path hash_object git_cmd_try
+                version exec_path html_path hash_object git_cmd_try
                 remote_refs
                 temp_acquire temp_release temp_reset temp_path);
 
@@ -492,6 +492,16 @@ C<git --exec-path>). Useful mostly only internally.
 sub exec_path { command_oneline('--exec-path') }
 
 
+=item html_path ()
+
+Return path to the Git html documentation (the same as
+C<git --html-path>). Useful mostly only internally.
+
+=cut
+
+sub html_path { command_oneline('--html-path') }
+
+
 =item repo_path ()
 
 Return path to the git repository. Must be called on a repository instance.
-- 
1.6.2.2.428.gea44ab

^ permalink raw reply related

* [PATCH 0/2] Reliably open the installed HTML docs from git-gui
From: Markus Heidelberg @ 2009-04-05  1:33 UTC (permalink / raw)
  To: Junio C Hamano, Shawn O. Pearce; +Cc: git

The path to the installed HTML documentation has to be queried, so that git-gui
is able to open it for viewing. This doesn't really work currently, because a
hardcoded path is used.

Markus Heidelberg (2):
  add --html-path to get the location of installed HTML docs
  git-gui: use `git --html-path` to get the location of installed HTML docs

 Documentation/git.txt                  |    6 +++++-
 Makefile                               |    1 +
 contrib/completion/git-completion.bash |    1 +
 exec_cmd.c                             |    6 ++++++
 git.c                                  |    5 ++++-
 perl/Git.pm                            |   12 +++++++++++-
 git-gui/git-gui.sh                     |   33 +++++++++++++++++++++++++++++----
 7 files changed, 57 insertions(+), 7 deletions(-)

^ permalink raw reply

* Re: Segfault on merge with 1.6.2.1
From: Clemens Buchacher @ 2009-04-05  0:46 UTC (permalink / raw)
  To: git; +Cc: Miklos Vajna, Michael Johnson, Johannes Schindelin
In-Reply-To: <20090401180627.GA14716@localhost>

Hi,

The segmentation fault is caused by a null pointer dereference which happens
during recursive merge with a submodule conflict between two merge bases. This
is fixed by the following patches.

However, there are other problems with merging submodules. For example, git
diff aborts with "fatal: read error 'sub'" for conflicting submodules. I have
also added a test for this.

Dscho has already started working on related issues. I have therefore skipped
t7404, which is already used in Dscho's work.

Clemens

 merge-recursive.c          |   16 ++-------
 t/t7405-submodule-merge.sh |   74 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 12 deletions(-)

[PATCH 1/3] add tests for merging with submodules
[PATCH 2/3] update cache for conflicting submodule entries
[PATCH 3/3] simplify output of conflicting merge

^ permalink raw reply

* [PATCH 2/3] update cache for conflicting submodule entries
From: Clemens Buchacher @ 2009-04-05  0:46 UTC (permalink / raw)
  To: git; +Cc: Miklos Vajna, Michael Johnson, Johannes Schindelin,
	Clemens Buchacher
In-Reply-To: <1238892420-721-2-git-send-email-drizzd@aon.at>

When merging merge bases during a recursive merge we do not want to
leave any unmerged entries. Otherwise we cannot create a temporary
tree for the recursive merge to work with.

We failed to do so in case of a submodule conflict between merge
bases, causing a NULL pointer dereference in the next step of the
recursive merge.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 merge-recursive.c          |    5 +++--
 t/t7405-submodule-merge.sh |    2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 3e1bc3e..f1b120b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1118,10 +1118,11 @@ static int process_entry(struct merge_options *o,
 		clean_merge = mfi.clean;
 		if (mfi.clean)
 			update_file(o, 1, mfi.sha, mfi.mode, path);
-		else if (S_ISGITLINK(mfi.mode))
+		else if (S_ISGITLINK(mfi.mode)) {
 			output(o, 1, "CONFLICT (submodule): Merge conflict in %s "
 			       "- needs %s", path, sha1_to_hex(b.sha1));
-		else {
+			update_file(o, 0, mfi.sha, mfi.mode, path);
+		} else {
 			output(o, 1, "CONFLICT (%s): Merge conflict in %s",
 					reason, path);
 
diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 9778ad4..aa6c44c 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -63,7 +63,7 @@ test_expect_failure 'merging with modify/modify conflict' '
 
 '
 
-test_expect_failure 'merging with a modify/modify conflict between merge bases' '
+test_expect_success 'merging with a modify/modify conflict between merge bases' '
 
 	git reset --hard HEAD &&
 	git checkout -b test2 c &&
-- 
1.6.2.1

^ permalink raw reply related

* [PATCH 3/3] simplify output of conflicting merge
From: Clemens Buchacher @ 2009-04-05  0:47 UTC (permalink / raw)
  To: git; +Cc: Miklos Vajna, Michael Johnson, Johannes Schindelin,
	Clemens Buchacher
In-Reply-To: <1238892420-721-3-git-send-email-drizzd@aon.at>

This simplifies the code without changing the semantics and removes
the unhelpful "needs $sha1" part of the conflicting submodule message.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 merge-recursive.c |   17 ++++-------------
 1 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index f1b120b..d6f0582 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1116,22 +1116,13 @@ static int process_entry(struct merge_options *o,
 				 o->branch1, o->branch2);
 
 		clean_merge = mfi.clean;
-		if (mfi.clean)
-			update_file(o, 1, mfi.sha, mfi.mode, path);
-		else if (S_ISGITLINK(mfi.mode)) {
-			output(o, 1, "CONFLICT (submodule): Merge conflict in %s "
-			       "- needs %s", path, sha1_to_hex(b.sha1));
-			update_file(o, 0, mfi.sha, mfi.mode, path);
-		} else {
+		if (!mfi.clean) {
+			if (S_ISGITLINK(mfi.mode))
+				reason = "submodule";
 			output(o, 1, "CONFLICT (%s): Merge conflict in %s",
 					reason, path);
-
-			if (o->call_depth)
-				update_file(o, 0, mfi.sha, mfi.mode, path);
-			else
-				update_file_flags(o, mfi.sha, mfi.mode, path,
-					      0 /* update_cache */, 1 /* update_working_directory */);
 		}
+		update_file(o, mfi.clean, mfi.sha, mfi.mode, path);
 	} else if (!o_sha && !a_sha && !b_sha) {
 		/*
 		 * this entry was deleted altogether. a_mode == 0 means
-- 
1.6.2.1

^ permalink raw reply related

* [PATCH 1/3] add tests for merging with submodules
From: Clemens Buchacher @ 2009-04-05  0:46 UTC (permalink / raw)
  To: git; +Cc: Miklos Vajna, Michael Johnson, Johannes Schindelin,
	Clemens Buchacher
In-Reply-To: <1238892420-721-1-git-send-email-drizzd@aon.at>


Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 t/t7405-submodule-merge.sh |   74 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 74 insertions(+), 0 deletions(-)
 create mode 100755 t/t7405-submodule-merge.sh

diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
new file mode 100755
index 0000000..9778ad4
--- /dev/null
+++ b/t/t7405-submodule-merge.sh
@@ -0,0 +1,74 @@
+#!/bin/sh
+
+test_description='merging with submodules'
+
+. ./test-lib.sh
+
+#
+# history
+#
+#        a --- c
+#      /   \ /
+# root      X
+#      \   / \
+#        b --- d
+#
+
+test_expect_success setup '
+
+	mkdir sub &&
+	(cd sub &&
+	 git init &&
+	 echo original > file &&
+	 git add file &&
+	 test_tick &&
+	 git commit -m sub-root) &&
+	git add sub &&
+	test_tick &&
+	git commit -m root &&
+
+	git checkout -b a master &&
+	(cd sub &&
+	 echo A > file &&
+	 git add file &&
+	 test_tick &&
+	 git commit -m sub-a) &&
+	git add sub &&
+	test_tick &&
+	git commit -m a &&
+
+	git checkout -b b master &&
+	(cd sub &&
+	 echo B > file &&
+	 git add file &&
+	 test_tick &&
+	 git commit -m sub-b) &&
+	git add sub &&
+	test_tick &&
+	git commit -m b
+
+	git checkout -b c a &&
+	git merge -s ours b &&
+
+	git checkout -b d b &&
+	git merge -s ours a
+'
+
+test_expect_failure 'merging with modify/modify conflict' '
+
+	git checkout -b test1 a &&
+	test_must_fail git merge b &&
+	test -f .git/MERGE_MSG &&
+	git diff
+
+'
+
+test_expect_failure 'merging with a modify/modify conflict between merge bases' '
+
+	git reset --hard HEAD &&
+	git checkout -b test2 c &&
+	git merge d
+
+'
+
+test_done
-- 
1.6.2.1

^ permalink raw reply related

* Re: Performance issue: initial git clone causes massive repack
From: Robin H. Johnson @ 2009-04-05  0:37 UTC (permalink / raw)
  To: Git Mailing List
In-Reply-To: <20090405000536.GA12927@vidovic>

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

On Sun, Apr 05, 2009 at 02:05:36AM +0200, Nicolas Sebrecht wrote:
> > Our full repository conversion is large, even after tuning the
> > repacking, the packed repository is between 1.4 and 1.6GiB. As of Feburary
> > 4th, 2009, it contained 4886949 objects. It is not suitable for
> > splitting into submodules either unfortunately - we have a lot of
> > directory moves that would cause submodule bloat.
> Actually, I'm not sure that a full portage tree repository would be the
> best thing to do. It would not be suitable in the long term and working
> on the repository/history would be a big mess. Why provide a such repo ?
> Or at least, why provide a such readable repo ?
> 
> IMHO, you should provide a repository per upstream package on the main
> server.
That causes incredibly bloat unfortunately.

I'll summarize why here for the git mailing list. Most our developers
have the entire tree checked out, and in informal surveys, would like to
continue to do so. There are ~13500 packages right now (I'm excluding
eclasses/, profiles/, scripts/), and growing by 15-25 new packages/week.
(~45% of packages also have a files/ directory).

For each package, the .git directory, assuming in a single pack,
consumes at least 36 inodes.  Tail-packing is limited to Reiserfs3 and
JFS, and isn't widely used other than that, so assuming 4KiB inodes,
that's an overhead of at least 144KiB per package. Multiple by the
number of packages, and we get an overhead of 2GiB, before we've added
ANY content.

Without tail packing, the Gentoo tree is presently around 520MiB (you
can fit it into ~190MiB with tail packing). This means that
repo-per-package would have an overhead in the range of 400%.

Additionally, there's a lot of commonality between ebuilds and packages,
and having repo-per-package means that the compression algorithms can't
make use of it - dictionary algorithms are effective at compression for
a reason.

Overhead is the reason that we refused to migrate to SVN as well.
- CVS, per each directory of data, has a constant overhead of 4 inodes
  (CVS/ CVS/Root CVS/Repository CVS/Entries)
- SVN, for each data directory, has another complete copy of the data,
  plus a minimum of 10 other inodes.
- Git costs a minimum 36 inodes per repository. In a fully packed repo,
  the number of inodes tends to stay below 50 in all cases.

> PS: what about cc'ing gentoo-scm list ?
It's not an open-posting list, so anybody here on the git list simply
replying would not get their post on there. The issue has been raised
there, and this mainly meant to find a resolution to that problem.

-- 
Robin Hugh Johnson
Gentoo Linux Developer & Infra Guy
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

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

^ permalink raw reply

* Re: [PATCH 2/2] pack-objects: report actual number of threads to be used
From: Nicolas Pitre @ 2009-04-05  0:11 UTC (permalink / raw)
  To: Jeff King; +Cc: Dan McGee, git
In-Reply-To: <20090404232505.GA26906@coredump.intra.peff.net>

On Sat, 4 Apr 2009, Jeff King wrote:

> On Sat, Apr 04, 2009 at 01:20:18PM -0500, Dan McGee wrote:
> 
> > > That makes sense to me, though I wonder if it may confuse and frustrate
> > > users who are expecting their awesome quad-core machine to be using 4
> > > threads when it only uses 2. Is it worth printing both values, or some
> > > indicator that we could have been using more?
> > 
> > I thought of this, but decided it wasn't really worth it. The default
> > window size of 10 makes it a very rare case that you will use fewer
> > than 4 threads. With the default, each thread needs a minimum of 20
> > objects, so even a 100-object repository would spawn the 4 threads.
> 
> Good point. Though by that logic, isn't your patch also not worth it
> (i.e., it is unlikely not to fill the threads, so the output will be the
> same with or without it)?
> 
> I still think yours is an improvement, though, however slight.

I don't think this is worth it at all.

This display is there mainly to confirm expected number of available 
threads.  The number of actually active threads is an implementation 
detail.  Sure if the number of objects is too low, or if the window size 
is too large, then the number of active threads will be lower.  But in 
practice it is also possible that with some patological object set you 
end up with 2 threads out of 4 completing very quickly and the other 2 
threads still busy with big objects and total remaining work set too 
small to split it further amongst idle threads, meaning that you'll end 
up with only 2 busy CPUs even though the display said 4 threads 
initially even with this patch.

In other words I don't think this patch is a good idea as we don't 
update the display with remaining active threads along the way.


Nicolas

^ permalink raw reply

* Re: [PATCH 3/4] Makefile: allow building without perl
From: Robin H. Johnson @ 2009-04-05  0:06 UTC (permalink / raw)
  To: Git Mailing List
In-Reply-To: <20090404235636.GA27354@coredump.intra.peff.net>

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

On Sat, Apr 04, 2009 at 07:56:36PM -0400, Jeff King wrote:
> Ah, I see. You are not asking "was PERL_PATH blank" but rather "does
> PERL_PATH exist". And I think that is not the right thing for the
> Makefile, as it is unlike any other part of the git Makefile, which
> generally does what it is told with the minimum of magic. That sort of
> magic generally goes into configure.ac.
At a glance, PERL_PATH in configure.ac isn't actually used at the
moment, the definition in the Makefile overrides it.

> So NO_PERL_MAKEMAKER is _already_ exported, and I don't think there is
> any reason to export NO_PERL in the environment (see patch 4/4, which
> exports it via GIT-BUILD-OPTIONS).
Ok, drop the export then, but do keep at least the if(is empty
PERL_PATH) then { set NO_PERL }, even if you won't keep the existence
check.

-- 
Robin Hugh Johnson
Gentoo Linux Developer & Infra Guy
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

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

^ permalink raw reply

* Re: Performance issue: initial git clone causes massive repack
From: Nicolas Sebrecht @ 2009-04-05  0:05 UTC (permalink / raw)
  To: Robin H. Johnson; +Cc: Git Mailing List
In-Reply-To: <20090404220743.GA869@curie-int>

On Sat, Apr 04, 2009 at 03:07:43PM -0700, Robin H. Johnson wrote:

> Our full repository conversion is large, even after tuning the
> repacking, the packed repository is between 1.4 and 1.6GiB. As of Feburary
> 4th, 2009, it contained 4886949 objects. It is not suitable for
> splitting into submodules either unfortunately - we have a lot of
> directory moves that would cause submodule bloat.

Actually, I'm not sure that a full portage tree repository would be the
best thing to do. It would not be suitable in the long term and working
on the repository/history would be a big mess. Why provide a such repo ?
Or at least, why provide a such readable repo ?

IMHO, you should provide a repository per upstream package on the main
server.


PS: what about cc'ing gentoo-scm list ?

-- 
Nicolas Sebrecht

^ permalink raw reply

* Re: [PATCH 3/4] Makefile: allow building without perl
From: Jeff King @ 2009-04-04 23:56 UTC (permalink / raw)
  To: Robin H. Johnson; +Cc: Git Mailing List
In-Reply-To: <20090404T234556Z@curie.orbis-terrarum.net>

On Sat, Apr 04, 2009 at 04:51:48PM -0700, Robin H. Johnson wrote:

> On Sat, Apr 04, 2009 at 07:39:36PM -0400, Jeff King wrote:
> > > +ifeq ($(wildcard $(PERL_PATH)),)
> > I'm not sure what you're trying to accomplish with the wildcard, unless
> > it is "PERL_PATH = /usr/*/perl" or similar, but that seems a bit crazy
> > to me. It should probably behave the same as TCLTK_PATH, though (so if
> > there is a good use case, TCLTK_PATH should be enhanced).
> No. The ifeq ($(wildcard $(PERL_PATH)),) is entirely correct.
> It's one of the few ways to detect the existence of a file from within
> Make, without any exec calls. If you give it a non-expandable path,
> /usr/bin/perl in this case, it checks only that path, and either returns
> it or an empty string. This enables us to check that /usr/bin/perl
> exists, and take suitable action if it does not.

Ah, I see. You are not asking "was PERL_PATH blank" but rather "does
PERL_PATH exist". And I think that is not the right thing for the
Makefile, as it is unlike any other part of the git Makefile, which
generally does what it is told with the minimum of magic. That sort of
magic generally goes into configure.ac.

> > I don't think there is a point in setting NO_PERL_MAKEMAKER if NO_PERL
> > is set, and I believe the export is pointless, as I described in an
> > earlier email.
> From further down the Makefile:
> ifdef NO_PERL_MAKEMAKER
>     export NO_PERL_MAKEMAKER
> endif

So NO_PERL_MAKEMAKER is _already_ exported, and I don't think there is
any reason to export NO_PERL in the environment (see patch 4/4, which
exports it via GIT-BUILD-OPTIONS).

-Peff

^ permalink raw reply

* Re: [PATCH 3/4] Makefile: allow building without perl
From: Robin H. Johnson @ 2009-04-04 23:51 UTC (permalink / raw)
  To: Git Mailing List
In-Reply-To: <20090404233936.GB26906@coredump.intra.peff.net>

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

On Sat, Apr 04, 2009 at 07:39:36PM -0400, Jeff King wrote:
> > +ifeq ($(wildcard $(PERL_PATH)),)
> I'm not sure what you're trying to accomplish with the wildcard, unless
> it is "PERL_PATH = /usr/*/perl" or similar, but that seems a bit crazy
> to me. It should probably behave the same as TCLTK_PATH, though (so if
> there is a good use case, TCLTK_PATH should be enhanced).
No. The ifeq ($(wildcard $(PERL_PATH)),) is entirely correct.
It's one of the few ways to detect the existence of a file from within
Make, without any exec calls. If you give it a non-expandable path,
/usr/bin/perl in this case, it checks only that path, and either returns
it or an empty string. This enables us to check that /usr/bin/perl
exists, and take suitable action if it does not.

It's not suitable for use with the TCLTK_PATH, as that isn't an absolute
path, so the wildcard trick doesn't work.

> I don't think there is a point in setting NO_PERL_MAKEMAKER if NO_PERL
> is set, and I believe the export is pointless, as I described in an
> earlier email.
From further down the Makefile:
ifdef NO_PERL_MAKEMAKER
    export NO_PERL_MAKEMAKER
endif

-- 
Robin Hugh Johnson
Gentoo Linux Developer & Infra Guy
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

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

^ permalink raw reply

* Re: [PATCH 4/4] tests: skip perl tests if NO_PERL is defined
From: Jeff King @ 2009-04-04 23:42 UTC (permalink / raw)
  To: Robin H. Johnson; +Cc: Git Mailing List
In-Reply-To: <20090404T225904Z@curie.orbis-terrarum.net>

On Sat, Apr 04, 2009 at 04:30:10PM -0700, Robin H. Johnson wrote:

> Testing on a box without Perl, I get the following failures.
> You can reproduce by doing mv /usr/bin/perl{,.tmp}, 
> gmake NO_PERL=1 clean all test
> 
> This is the only one that doesn't appear to use perl.
> *** t6030-bisect-porcelain.sh ***

Probably this:

  $ grep -B1 -A7 PERL git-bisect.sh
  sq() {
          @@PERL@@ -e '
                  for (@ARGV) {
                          s/'\''/'\'\\\\\'\''/g;
                          print " '\''$_'\''";
                  }
                  print "\n";
          ' "$@"
  }

-Peff

^ permalink raw reply

* Re: [PATCH 3/4] Makefile: allow building without perl
From: Jeff King @ 2009-04-04 23:39 UTC (permalink / raw)
  To: Robin H. Johnson; +Cc: Git Mailing List
In-Reply-To: <20090404T224109Z@curie.orbis-terrarum.net>

On Sat, Apr 04, 2009 at 03:47:29PM -0700, Robin H. Johnson wrote:

> On Fri, Apr 03, 2009 at 03:32:20PM -0400, Jeff King wrote:
> > For systems with a missing or broken perl, it is nicer to
> > explicitly say "we don't want perl" because:
> Part of the patch got missed. In the case of missing perl, we can detect it,
> originally we had a compare of PERL_PATH for emptiness.

Thanks. I was hesitant on this at first because I think the behavior
should be the same as with TCLTK_PATH, but I didn't realize that
TCLTK_PATH already automagically sets NO_TCLTK in the same way.

> +ifeq ($(wildcard $(PERL_PATH)),)
> +PERL_PATH =
> +NO_PERL = NoThanks
> +NO_PERL_MAKEMAKER = NoThanks
> +export NO_PERL NO_PERL_MAKEMAKER
> +endif

The TCLTK code is just:

  ifeq ($(TCLTK_PATH),)
  NO_TCLTK=NoThanks
  endif

I'm not sure what you're trying to accomplish with the wildcard, unless
it is "PERL_PATH = /usr/*/perl" or similar, but that seems a bit crazy
to me. It should probably behave the same as TCLTK_PATH, though (so if
there is a good use case, TCLTK_PATH should be enhanced).

I don't think there is a point in setting NO_PERL_MAKEMAKER if NO_PERL
is set, and I believe the export is pointless, as I described in an
earlier email.

-Peff

^ permalink raw reply

* Re: [PATCH 4/4] tests: skip perl tests if NO_PERL is defined
From: Robin H. Johnson @ 2009-04-04 23:30 UTC (permalink / raw)
  To: Git Mailing List
In-Reply-To: <20090403193359.GD5547@coredump.intra.peff.net>

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

On Fri, Apr 03, 2009 at 03:33:59PM -0400, Jeff King wrote:
> These scripts all test git programs that are written in
> perl, and thus obviously won't work if NO_PERL is defined.
> We pass NO_PERL to the scripts from the building Makefile
> via the GIT-BUILD-OPTIONS file.
Something is missing in the porting of the patch (possibly from an earlier
version where I did check for perl usage in the testcases and just avoided
those tests).

Testing on a box without Perl, I get the following failures.
You can reproduce by doing mv /usr/bin/perl{,.tmp}, 
gmake NO_PERL=1 clean all test

This is the only one that doesn't appear to use perl.
*** t6030-bisect-porcelain.sh ***
* FAIL 2: bisect starts with only one bad
* FAIL 3: bisect does not start with only one good
* FAIL 4: bisect start with one bad and good
* FAIL 5: bisect fails if given any junk instead of revs
* FAIL 7: bisect reset: back in another branch
* FAIL 8: bisect reset when not bisecting
* FAIL 9: bisect reset removes packed refs
* FAIL 10: bisect start: back in good branch
* FAIL 11: bisect start: no ".git/BISECT_START" if junk rev
* FAIL 12: bisect start: no ".git/BISECT_START" if mistaken rev
* FAIL 14: bisect skip: successfull result
* FAIL 15: bisect skip: cannot tell between 3 commits
* FAIL 16: bisect skip: cannot tell between 2 commits
* FAIL 17: bisect skip: with commit both bad and skipped
* FAIL 18: "git bisect run" simple case
* FAIL 19: "git bisect run" with more complex "git bisect start"
* FAIL 20: bisect skip: add line and then a new test
* FAIL 21: bisect skip and bisect replay
* FAIL 22: bisect run & skip: cannot tell between 2
* FAIL 23: bisect run & skip: find first bad
* FAIL 24: bisect skip only one range
* FAIL 25: bisect skip many ranges
* FAIL 26: bisect starting with a detached HEAD
* FAIL 27: bisect errors out if bad and good are mistaken
* FAIL 28: bisect does not create a "bisect" branch
* FAIL 30: good merge base when good and bad are siblings
* FAIL 31: skipped merge base when good and bad are siblings
* FAIL 32: bad merge base when good and bad are siblings
* FAIL 34: good merge bases when good and bad are siblings
* FAIL 35: optimized merge base checks

All the rest of these are all due to perl in test infrastructure:
*** t0020-crlf.sh ***
* FAIL 1: setup
* FAIL 7: switch off autocrlf, safecrlf, reset HEAD
* FAIL 10: checkout with autocrlf=true
* FAIL 11: checkout with autocrlf=input
* FAIL 12: apply patch (autocrlf=input)
* FAIL 13: apply patch --cached (autocrlf=input)
* FAIL 14: apply patch --index (autocrlf=input)
* FAIL 15: apply patch (autocrlf=true)
* FAIL 16: apply patch --cached (autocrlf=true)
* FAIL 17: apply patch --index (autocrlf=true)
* FAIL 18: .gitattributes says two is binary
* FAIL 20: .gitattributes says two and three are text
* FAIL 21: in-tree .gitattributes (1)
* FAIL 22: in-tree .gitattributes (2)
* FAIL 23: in-tree .gitattributes (3)
* FAIL 24: in-tree .gitattributes (4)

*** t1300-repo-config.sh ***
* FAIL 71: --null --list
* FAIL 72: --null --get-regexp

*** t3300-funny-names.sh ***
* FAIL 3: git ls-files -z with-funny
* FAIL 7: git diff-index -z with-funny
* FAIL 8: git diff-tree -z with-funny

*** t4012-diff-binary.sh ***
* FAIL 9: diff --no-index with binary creation

*** t4014-format-patch.sh ***
* FAIL 16: no threading
* FAIL 17: thread
* FAIL 18: thread in-reply-to
* FAIL 19: thread cover-letter
* FAIL 20: thread cover-letter in-reply-to
* FAIL 21: thread explicit shallow
* FAIL 22: thread deep
* FAIL 23: thread deep in-reply-to
* FAIL 24: thread deep cover-letter
* FAIL 25: thread deep cover-letter in-reply-to
* FAIL 26: thread via config
* FAIL 27: thread deep via config
* FAIL 28: thread config + override
* FAIL 29: thread config + --no-thread

*** t4020-diff-external.sh ***
* FAIL 12: force diff with "diff"
* FAIL 15: diff --cached

*** t4029-diff-trailing-space.sh ***
* FAIL 1: diff honors config option, diff.suppressBlankEmpty

*** t4030-diff-textconv.sh ***
* FAIL 5: diff produces text
* FAIL 7: log produces text
* FAIL 9: status -v produces text
* FAIL 10: diffstat does not run textconv
* FAIL 11: textconv does not act on symlinks

*** t4031-diff-rewrite-binary.sh ***
* FAIL 6: rewrite diff respects textconv

*** t4103-apply-binary.sh ***
* FAIL 3: check binary diff -- should fail.
* FAIL 4: check binary diff (copy) -- should fail.
* FAIL 5: check incomplete binary diff with replacement -- should fail.
* FAIL 6: check incomplete binary diff with replacement (copy) -- should fail.
* FAIL 9: apply binary diff -- should fail.
* FAIL 10: apply binary diff -- should fail.
* FAIL 11: apply binary diff (copy) -- should fail.
* FAIL 12: apply binary diff (copy) -- should fail.

*** t4116-apply-reverse.sh ***
* FAIL 1: setup
* FAIL 2: apply in forward
* FAIL 3: apply in reverse
* FAIL 4: setup separate repository lacking postimage
* FAIL 5: apply in forward without postimage
* FAIL 6: apply in reverse without postimage
* FAIL 7: reversing a whitespace introduction

*** t4200-rerere.sh ***
* FAIL 15: clear removed the directory
	test ! -d .git/rr-cache/b5a3939d25d0a649294d5d94f2d338ef8da51812
mkdir: cannot create directory `.git/rr-cache/b5a3939d25d0a649294d5d94f2d338ef8da51812': File exists

*** t5300-pack-object.sh ***
* FAIL 1: setup
* FAIL 2: pack without delta
* FAIL 3: unpack without delta
* FAIL 5: pack with REF_DELTA
* FAIL 6: unpack with REF_DELTA
* FAIL 8: pack with OFS_DELTA
* FAIL 9: unpack with OFS_DELTA
* FAIL 11: compare delta flavors
* FAIL 12: use packed objects
* FAIL 13: use packed deltified (REF_DELTA) objects
* FAIL 14: use packed deltified (OFS_DELTA) objects
* FAIL 15: survive missing objects/pack directory
* FAIL 16: verify pack
* FAIL 17: verify pack -v
* FAIL 18: verify-pack catches mismatched .idx and .pack files
* FAIL 19: verify-pack catches a corrupted pack signature
* FAIL 20: verify-pack catches a corrupted pack version
* FAIL 21: verify-pack catches a corrupted type/size of the 1st packed object data
* FAIL 22: verify-pack catches a corrupted sum of the index file itself
* FAIL 23: build pack index for an existing pack
* FAIL 24: fake a SHA1 hash collision
* FAIL 25: make sure index-pack detects the SHA1 collision
* FAIL 26: honor pack.packSizeLimit
* FAIL 29: tolerate absurdly small packsizelimit

*** t5303-pack-corruption-resilience.sh ***
* FAIL 5: create corruption in data of first object
* FAIL 7: ... and loose copy of second object allows for partial recovery
* FAIL 11: create corruption in data of first delta

*** t6002-rev-list-bisect.sh ***
* FAIL 34: --bisect l5 ^root
* FAIL 35: --bisect l5 ^root ^c3
* FAIL 36: --bisect l5 ^root ^c3 ^b4
* FAIL 37: --bisect l3 ^root ^c3 ^b4
* FAIL 38: --bisect l5 ^b3 ^a3 ^b4 ^a4
* FAIL 39: --bisect l4 ^a2 ^a3 ^b ^a4
* FAIL 40: --bisect l3 ^a2 ^a3 ^b ^a4
* FAIL 41: --bisect a4 ^a2 ^a3 ^b4
* FAIL 42: --bisect a4 ^a2 ^a3 ^b4 ^c2
* FAIL 43: --bisect a4 ^a2 ^a3 ^b4 ^c2 ^c3
* FAIL 44: --bisect a4 ^a2 ^a3 ^b4
* FAIL 45: --bisect c3 ^a2 ^a3 ^b4 ^c2

*** t6003-rev-list-topo-order.sh ***
* FAIL 1: rev-list has correct number of entries
* FAIL 2: simple topo order
* FAIL 3: two diamonds topo order (g6)
* FAIL 4: multiple heads
* FAIL 5: multiple heads, prune at a1
* FAIL 6: multiple heads, prune at l1
* FAIL 7: cross-epoch, head at l5, prune at l1
* FAIL 8: duplicated head arguments
* FAIL 9: prune near topo
* FAIL 10: head has no parent
* FAIL 11: two nodes - one head, one base
* FAIL 12: three nodes one head, one internal, one base
* FAIL 13: linear prune l2 ^root
* FAIL 14: linear prune l2 ^l0
* FAIL 15: linear prune l2 ^l1
* FAIL 16: linear prune l5 ^a4
* FAIL 17: linear prune l5 ^l3
* FAIL 18: linear prune l5 ^l4
* FAIL 19: max-count 10 - topo order
* FAIL 20: max-count 10 - non topo order
* FAIL 21: --max-age=c3, no --topo-order
* FAIL 22: one specified head reachable from another a4, c3, --topo-order
* FAIL 23: one specified head reachable from another c3, a4, --topo-order
* FAIL 24: one specified head reachable from another a4, c3, no --topo-order
* FAIL 25: one specified head reachable from another c3, a4, no --topo-order
* FAIL 26: graph with c3 and a4 parents of head
* FAIL 27: graph with a4 and c3 parents of head
* FAIL 30: simple topo order (l5r1)
* FAIL 31: simple topo order (r1l5)
* FAIL 32: don't print things unreachable from one branch
* FAIL 33: --topo-order a4 l3

*** t6011-rev-list-with-bad-commit.sh ***
* FAIL 3: corrupt second commit object
* FAIL 4: rev-list should fail
* FAIL 5: git repack _MUST_ fail

*** t6013-rev-list-reverse-parents.sh ***
* FAIL 2: --reverse --parents --full-history combines correctly
* FAIL 3: --boundary does too

annotate-tests.sh
(lots of the t8* tests).

-- 
Robin Hugh Johnson
Gentoo Linux Developer & Infra Guy
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

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

^ permalink raw reply

* Re: [PATCH 2/2] pack-objects: report actual number of threads to be used
From: Jeff King @ 2009-04-04 23:25 UTC (permalink / raw)
  To: Dan McGee; +Cc: git
In-Reply-To: <449c10960904041120j38086192s25070912b0371c09@mail.gmail.com>

On Sat, Apr 04, 2009 at 01:20:18PM -0500, Dan McGee wrote:

> > That makes sense to me, though I wonder if it may confuse and frustrate
> > users who are expecting their awesome quad-core machine to be using 4
> > threads when it only uses 2. Is it worth printing both values, or some
> > indicator that we could have been using more?
> 
> I thought of this, but decided it wasn't really worth it. The default
> window size of 10 makes it a very rare case that you will use fewer
> than 4 threads. With the default, each thread needs a minimum of 20
> objects, so even a 100-object repository would spawn the 4 threads.

Good point. Though by that logic, isn't your patch also not worth it
(i.e., it is unlikely not to fill the threads, so the output will be the
same with or without it)?

I still think yours is an improvement, though, however slight.

-Peff

^ permalink raw reply

* Re: [PATCH 3/4] Makefile: allow building without perl
From: Robin H. Johnson @ 2009-04-04 22:47 UTC (permalink / raw)
  To: Git Mailing List
In-Reply-To: <20090403193220.GC5547@coredump.intra.peff.net>

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

On Fri, Apr 03, 2009 at 03:32:20PM -0400, Jeff King wrote:
> For systems with a missing or broken perl, it is nicer to
> explicitly say "we don't want perl" because:
Part of the patch got missed. In the case of missing perl, we can detect it,
originally we had a compare of PERL_PATH for emptiness.

diff Makefile.orig Makefile
--- Makefile.orig
+++ Makefile
@@ -354,12 +354,6 @@ BUILT_INS += git-whatchanged$X
 # what 'all' will build and 'install' will install, in gitexecdir
 ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)
 
-# what 'all' will build but not install in gitexecdir
-OTHER_PROGRAMS = git$X
-ifndef NO_PERL
-OTHER_PROGRAMS += gitweb/gitweb.cgi
-endif
-
 # Set paths to tools early so that they can be used for version tests.
 ifndef SHELL_PATH
        SHELL_PATH = /bin/sh
@@ -367,6 +361,18 @@ endif
 ifndef PERL_PATH
        PERL_PATH = /usr/bin/perl
 endif
+ifeq ($(wildcard $(PERL_PATH)),)
+PERL_PATH =
+NO_PERL = NoThanks
+NO_PERL_MAKEMAKER = NoThanks
+export NO_PERL NO_PERL_MAKEMAKER
+endif
+
+# what 'all' will build but not install in gitexecdir
+OTHER_PROGRAMS = git$X
+ifndef NO_PERL
+OTHER_PROGRAMS += gitweb/gitweb.cgi
+endif

-- 
Robin Hugh Johnson
Gentoo Linux Developer & Infra Guy
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

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

^ permalink raw reply

* Performance issue: initial git clone causes massive repack
From: Robin H. Johnson @ 2009-04-04 22:07 UTC (permalink / raw)
  To: Git Mailing List

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

Hi,

This is a first in my series of mails over the next few days, on issues
that we've run into planning a potential migration for Gentoo's
repository into Git.

Our full repository conversion is large, even after tuning the
repacking, the packed repository is between 1.4 and 1.6GiB. As of Feburary
4th, 2009, it contained 4886949 objects. It is not suitable for
splitting into submodules either unfortunately - we have a lot of
directory moves that would cause submodule bloat.

During an initial clone, I see that git-upload-pack invokes
pack-objects, despite the ENTIRE repository already being packed - no
loose objects whatsoever. git-upload-pack then seems to buffer in
memory.

In a small repository, this wouldn't be a problem, as the entire
repository can fit in memory very easily. However, with our large
repository, git-upload-pack and git-pack-objects grows in memory to well
more than the size of the packed repository, and are usually killed by
the OOM.

During 'remote: Counting objects: 4886949, done.', git-upload-pack peaks at
2474216KB VSZ and 1143048KB RSS. 
Shortly thereafter, we get 'remote: Compressing objects:   0%
(1328/1994284)', git-pack-objects with ~2.8GB VSZ and ~1.8GB RSS. Here,
the CPU burn also starts. On our test server machine (w/ git 1.6.0.6),
it takes about 200 minutes walltime to finish the pack, IFF the OOM
doesn't kick in.

Given that the repo is entirely packed already, I see no point in doing
this.

For the initial clone, can the git-upload-pack algorithm please send
existing packs, and only generate a pack containing the non-packed
items?

-- 
Robin Hugh Johnson
Gentoo Linux Developer & Infra Guy
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

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

^ permalink raw reply

* [PATCH 4/4] replace_object: use the new generic "sha1_pos" function to lookup sha1
From: Christian Couder @ 2009-04-04 20:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Schindelin

instead of the specific one that was simpler but less efficient.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 replace_object.c |   24 +++++++++---------------
 1 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/replace_object.c b/replace_object.c
index 1227214..eb59604 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "sha1-lookup.h"
 #include "refs.h"
 
 static struct replace_object {
@@ -7,23 +8,16 @@ static struct replace_object {
 
 static int replace_object_alloc, replace_object_nr;
 
+static const unsigned char *replace_sha1_access(size_t index, void *table)
+{
+	struct replace_object **replace = table;
+	return replace[index]->sha1[0];
+}
+
 static int replace_object_pos(const unsigned char *sha1)
 {
-	int lo, hi;
-	lo = 0;
-	hi = replace_object_nr;
-	while (lo < hi) {
-		int mi = (lo + hi) / 2;
-		struct replace_object *rep = replace_object[mi];
-		int cmp = hashcmp(sha1, rep->sha1[0]);
-		if (!cmp)
-			return mi;
-		if (cmp < 0)
-			hi = mi;
-		else
-			lo = mi + 1;
-	}
-	return -lo - 1;
+	return sha1_pos(sha1, replace_object, replace_object_nr,
+			replace_sha1_access);
 }
 
 static int register_replace_object(struct replace_object *replace,
-- 
1.6.2.2.404.ge96f3.dirty

^ permalink raw reply related

* [PATCH 3/4] bisect: use the new generic "sha1_pos" function to lookup sha1
From: Christian Couder @ 2009-04-04 20:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Schindelin

instead of the specific one that was simpler but less efficient.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 bisect.c |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/bisect.c b/bisect.c
index affba41..336d611 100644
--- a/bisect.c
+++ b/bisect.c
@@ -5,6 +5,7 @@
 #include "refs.h"
 #include "list-objects.h"
 #include "quote.h"
+#include "sha1-lookup.h"
 #include "bisect.h"
 
 
@@ -461,22 +462,16 @@ static void prepare_skipped(void)
 	qsort(skipped_sha1, skipped_sha1_nr, sizeof(*skipped_sha1), skipcmp);
 }
 
+static const unsigned char *skipped_sha1_access(size_t index, void *table)
+{
+	unsigned char (*skipped)[20] = table;
+	return skipped[index];
+}
+
 static int lookup_skipped(unsigned char *sha1)
 {
-	int lo, hi;
-	lo = 0;
-	hi = skipped_sha1_nr;
-	while (lo < hi) {
-		int mi = (lo + hi) / 2;
-		int cmp = hashcmp(sha1, skipped_sha1[mi]);
-		if (!cmp)
-			return mi;
-		if (cmp < 0)
-			hi = mi;
-		else
-			lo = mi + 1;
-	}
-	return -lo - 1;
+	return sha1_pos(sha1, skipped_sha1, skipped_sha1_nr,
+			skipped_sha1_access);
 }
 
 struct commit_list *filter_skipped(struct commit_list *list,
-- 
1.6.2.2.404.ge96f3.dirty

^ permalink raw reply related

* [PATCH 2/4] patch-ids: use the new generic "sha1_pos" function to lookup sha1
From: Christian Couder @ 2009-04-04 20:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Schindelin

instead of the specific one from which the new one has been copied.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 patch-ids.c |   93 +++-------------------------------------------------------
 1 files changed, 5 insertions(+), 88 deletions(-)

diff --git a/patch-ids.c b/patch-ids.c
index 3be5d31..5717257 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "diff.h"
 #include "commit.h"
+#include "sha1-lookup.h"
 #include "patch-ids.h"
 
 static int commit_patch_id(struct commit *commit, struct diff_options *options,
@@ -15,99 +16,15 @@ static int commit_patch_id(struct commit *commit, struct diff_options *options,
 	return diff_flush_patch_id(options, sha1);
 }
 
-static uint32_t take2(const unsigned char *id)
+static const unsigned char *patch_id_access(size_t index, void *table)
 {
-	return ((id[0] << 8) | id[1]);
+	struct patch_id **id_table = table;
+	return id_table[index]->patch_id;
 }
 
-/*
- * Conventional binary search loop looks like this:
- *
- *      do {
- *              int mi = (lo + hi) / 2;
- *              int cmp = "entry pointed at by mi" minus "target";
- *              if (!cmp)
- *                      return (mi is the wanted one)
- *              if (cmp > 0)
- *                      hi = mi; "mi is larger than target"
- *              else
- *                      lo = mi+1; "mi is smaller than target"
- *      } while (lo < hi);
- *
- * The invariants are:
- *
- * - When entering the loop, lo points at a slot that is never
- *   above the target (it could be at the target), hi points at a
- *   slot that is guaranteed to be above the target (it can never
- *   be at the target).
- *
- * - We find a point 'mi' between lo and hi (mi could be the same
- *   as lo, but never can be the same as hi), and check if it hits
- *   the target.  There are three cases:
- *
- *    - if it is a hit, we are happy.
- *
- *    - if it is strictly higher than the target, we update hi with
- *      it.
- *
- *    - if it is strictly lower than the target, we update lo to be
- *      one slot after it, because we allow lo to be at the target.
- *
- * When choosing 'mi', we do not have to take the "middle" but
- * anywhere in between lo and hi, as long as lo <= mi < hi is
- * satisfied.  When we somehow know that the distance between the
- * target and lo is much shorter than the target and hi, we could
- * pick mi that is much closer to lo than the midway.
- */
 static int patch_pos(struct patch_id **table, int nr, const unsigned char *id)
 {
-	int hi = nr;
-	int lo = 0;
-	int mi = 0;
-
-	if (!nr)
-		return -1;
-
-	if (nr != 1) {
-		unsigned lov, hiv, miv, ofs;
-
-		for (ofs = 0; ofs < 18; ofs += 2) {
-			lov = take2(table[0]->patch_id + ofs);
-			hiv = take2(table[nr-1]->patch_id + ofs);
-			miv = take2(id + ofs);
-			if (miv < lov)
-				return -1;
-			if (hiv < miv)
-				return -1 - nr;
-			if (lov != hiv) {
-				/*
-				 * At this point miv could be equal
-				 * to hiv (but id could still be higher);
-				 * the invariant of (mi < hi) should be
-				 * kept.
-				 */
-				mi = (nr-1) * (miv - lov) / (hiv - lov);
-				if (lo <= mi && mi < hi)
-					break;
-				die("oops");
-			}
-		}
-		if (18 <= ofs)
-			die("cannot happen -- lo and hi are identical");
-	}
-
-	do {
-		int cmp;
-		cmp = hashcmp(table[mi]->patch_id, id);
-		if (!cmp)
-			return mi;
-		if (cmp > 0)
-			hi = mi;
-		else
-			lo = mi + 1;
-		mi = (hi + lo) / 2;
-	} while (lo < hi);
-	return -lo-1;
+	return sha1_pos(id, table, nr, patch_id_access);
 }
 
 #define BUCKET_SIZE 190 /* 190 * 21 = 3990, with slop close enough to 4K */
-- 
1.6.2.2.404.ge96f3.dirty

^ permalink raw reply related

* [PATCH 1/4] sha1-lookup: add new "sha1_pos" function to efficiently lookup sha1
From: Christian Couder @ 2009-04-04 20:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Schindelin

This function has been copied from the "patch_pos" function in
"patch-ids.c" but an additional parameter has been added.

The new parameter is a function pointer, that is used to access the
sha1 of an element in the table.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 sha1-lookup.c |  101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 sha1-lookup.h |    7 ++++
 2 files changed, 108 insertions(+), 0 deletions(-)

diff --git a/sha1-lookup.c b/sha1-lookup.c
index da35747..055dd87 100644
--- a/sha1-lookup.c
+++ b/sha1-lookup.c
@@ -1,6 +1,107 @@
 #include "cache.h"
 #include "sha1-lookup.h"
 
+static uint32_t take2(const unsigned char *sha1)
+{
+	return ((sha1[0] << 8) | sha1[1]);
+}
+
+/*
+ * Conventional binary search loop looks like this:
+ *
+ *      do {
+ *              int mi = (lo + hi) / 2;
+ *              int cmp = "entry pointed at by mi" minus "target";
+ *              if (!cmp)
+ *                      return (mi is the wanted one)
+ *              if (cmp > 0)
+ *                      hi = mi; "mi is larger than target"
+ *              else
+ *                      lo = mi+1; "mi is smaller than target"
+ *      } while (lo < hi);
+ *
+ * The invariants are:
+ *
+ * - When entering the loop, lo points at a slot that is never
+ *   above the target (it could be at the target), hi points at a
+ *   slot that is guaranteed to be above the target (it can never
+ *   be at the target).
+ *
+ * - We find a point 'mi' between lo and hi (mi could be the same
+ *   as lo, but never can be the same as hi), and check if it hits
+ *   the target.  There are three cases:
+ *
+ *    - if it is a hit, we are happy.
+ *
+ *    - if it is strictly higher than the target, we update hi with
+ *      it.
+ *
+ *    - if it is strictly lower than the target, we update lo to be
+ *      one slot after it, because we allow lo to be at the target.
+ *
+ * When choosing 'mi', we do not have to take the "middle" but
+ * anywhere in between lo and hi, as long as lo <= mi < hi is
+ * satisfied.  When we somehow know that the distance between the
+ * target and lo is much shorter than the target and hi, we could
+ * pick mi that is much closer to lo than the midway.
+ */
+/*
+ * The table should contain "nr" elements.
+ * The sha1 of element i (between 0 and nr - 1) should be returned
+ * by "fn(i, table)".
+ */
+int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
+	     sha1_access_fn fn)
+{
+	size_t hi = nr;
+	size_t lo = 0;
+	size_t mi = 0;
+
+	if (!nr)
+		return -1;
+
+	if (nr != 1) {
+		size_t lov, hiv, miv, ofs;
+
+		for (ofs = 0; ofs < 18; ofs += 2) {
+			lov = take2(fn(0, table) + ofs);
+			hiv = take2(fn(nr - 1, table) + ofs);
+			miv = take2(sha1 + ofs);
+			if (miv < lov)
+				return -1;
+			if (hiv < miv)
+				return -1 - nr;
+			if (lov != hiv) {
+				/*
+				 * At this point miv could be equal
+				 * to hiv (but sha1 could still be higher);
+				 * the invariant of (mi < hi) should be
+				 * kept.
+				 */
+				mi = (nr - 1) * (miv - lov) / (hiv - lov);
+				if (lo <= mi && mi < hi)
+					break;
+				die("oops");
+			}
+		}
+		if (18 <= ofs)
+			die("cannot happen -- lo and hi are identical");
+	}
+
+	do {
+		int cmp;
+		cmp = hashcmp(fn(mi, table), sha1);
+		if (!cmp)
+			return mi;
+		if (cmp > 0)
+			hi = mi;
+		else
+			lo = mi + 1;
+		mi = (hi + lo) / 2;
+	} while (lo < hi);
+	return -lo-1;
+}
+
 /*
  * Conventional binary search loop looks like this:
  *
diff --git a/sha1-lookup.h b/sha1-lookup.h
index 3249a81..20af285 100644
--- a/sha1-lookup.h
+++ b/sha1-lookup.h
@@ -1,6 +1,13 @@
 #ifndef SHA1_LOOKUP_H
 #define SHA1_LOOKUP_H
 
+typedef const unsigned char *sha1_access_fn(size_t index, void *table);
+
+extern int sha1_pos(const unsigned char *sha1,
+		    void *table,
+		    size_t nr,
+		    sha1_access_fn fn);
+
 extern int sha1_entry_pos(const void *table,
 			  size_t elem_size,
 			  size_t key_offset,
-- 
1.6.2.2.404.ge96f3.dirty

^ permalink raw reply related

* [PATCH 0/4] start refactoring binary search function
From: Christian Couder @ 2009-04-04 20:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Schindelin

There are many binary search functions in the code base and I have been
asked to refactor them in these message:

http://thread.gmane.org/gmane.comp.version-control.git/105363/focus=105436
http://thread.gmane.org/gmane.comp.version-control.git/114735/focus=115391

so here is a start

The following patch applies on top of pu where they can be squashed into other
patches:

  sha1-lookup: add new "sha1_pos" function to efficiently lookup sha1
  patch-ids: use the new generic "sha1_pos" function to lookup sha1
  bisect: use the new generic "sha1_pos" function to lookup sha1
  replace_object: use the new generic "sha1_pos" function to lookup
    sha1

 bisect.c         |   23 +++++-------
 patch-ids.c      |   93 +++-----------------------------------------------
 replace_object.c |   24 +++++--------
 sha1-lookup.c    |  101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 sha1-lookup.h    |    7 ++++
 5 files changed, 131 insertions(+), 117 deletions(-)

^ 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