Git development
 help / color / mirror / Atom feed
* Re: [RFC] git reflog show
From: Shawn Pearce @ 2006-12-24  6:11 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.63.0612231552140.19693@wbgn013.biozentrum.uni-wuerzburg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> I wonder if it would make sense to teach the revision walking machinery 
> about reflogs. A commit could be marked as coming from a reflog entry, and 
> in that case the parents could be determined by the reflog rather than the 
> commit itself.

The revision machinery already knows about reflogs with --reflog,
used by git-pack-objects via git-repack.  But here its really only
useful to seed the list of commits to be walked as part of a pack
generation, to make sure the things referenced by the reflog stay
around after a repacking.  And it implies --all.

Rewriting the commits in memory to appear to have parents based
on their order of appearence in the reflog would nicely generate
a single strand of perls, but it makes it difficult to then access
the same commit's real parents, doesn't it?  So that may make the
revision machinary somewhat limited in some applications.

Besides we want the reflog message entry and not the commit message
when we perform pretty output, etc.  So really we are then talking
about generating synthetic commit objects for the reflog data.

-- 
Shawn.

^ permalink raw reply

* [PATCH 0/7] Various sp/mmap improvemnts
From: Shawn Pearce @ 2006-12-24  5:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Small improvements on top of my sp/mmap branch:

[PATCH 1/7] Rename gitfakemmap to git_mmap. 
[PATCH 2/7] Switch git_mmap to use pread.   

	These two might apply right to master as-is and aren't
	necessarily part of my sp/mmap topic.

[PATCH 3/7] Ensure packed_git.next is initia...
[PATCH 4/7] Default core.packdGitWindowSize ...
[PATCH 5/7] Don't exit successfully on EPIPE...
[PATCH 6/7] Release pack windows before repo...
[PATCH 7/7] Replace mmap with xmmap, better ...

	These apply on top of my sp/mmap topic and some also assume
	the 1/7,2/7 patches above to git_mmap were already applied.


1/7-5/7 were suggested by comments on the mailing list. Two I just
forgot to implement (6/7, 7/7) prior to submitting the original
series.

-- 
Shawn.

^ permalink raw reply

* [PATCH 7/7] Replace mmap with xmmap, better handling MAP_FAILED.
From: Shawn O. Pearce @ 2006-12-24  5:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <487c7d0ea81f2f82f330e277e0aea38a66ca7cfe.1166939109.git.spearce@spearce.org>

In some cases we did not even bother to check the return value of
mmap() and just assume it worked.  This is bad, because if we are
out of virtual address space the kernel returned MAP_FAILED and we
would attempt to dereference that address, segfaulting without any
real error output to the user.

We are replacing all calls to mmap() with xmmap() and moving all
MAP_FAILED checking into that single location.  If a mmap call
fails we try to release enough least-recently-used pack windows
to possibly succeed, then retry the mmap() attempt.  If we cannot
mmap even after releasing pack memory then we die() as none of our
callers have any reasonable recovery strategy for a failed mmap.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 config.c          |    2 +-
 diff.c            |    4 +---
 git-compat-util.h |   13 +++++++++++++
 read-cache.c      |    2 +-
 refs.c            |    2 +-
 sha1_file.c       |   18 +++++-------------
 6 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/config.c b/config.c
index edc42f4..410d5e9 100644
--- a/config.c
+++ b/config.c
@@ -698,7 +698,7 @@ int git_config_set_multivar(const char* key, const char* value,
 		}
 
 		fstat(in_fd, &st);
-		contents = mmap(NULL, st.st_size, PROT_READ,
+		contents = xmmap(NULL, st.st_size, PROT_READ,
 			MAP_PRIVATE, in_fd, 0);
 		close(in_fd);
 
diff --git a/diff.c b/diff.c
index f14288b..244292a 100644
--- a/diff.c
+++ b/diff.c
@@ -1341,10 +1341,8 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		fd = open(s->path, O_RDONLY);
 		if (fd < 0)
 			goto err_empty;
-		s->data = mmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
+		s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
 		close(fd);
-		if (s->data == MAP_FAILED)
-			goto err_empty;
 		s->should_munmap = 1;
 	}
 	else {
diff --git a/git-compat-util.h b/git-compat-util.h
index 4a417be..51e8d7a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -185,6 +185,19 @@ static inline void *xcalloc(size_t nmemb, size_t size)
 	return ret;
 }
 
+static inline void *xmmap(void *start, size_t length,
+	int prot, int flags, int fd, off_t offset)
+{
+	void *ret = mmap(start, length, prot, flags, fd, offset);
+	if (ret == MAP_FAILED) {
+		release_pack_memory(length);
+		ret = mmap(start, length, prot, flags, fd, offset);
+		if (ret == MAP_FAILED)
+			die("Out of memory? mmap failed: %s", strerror(errno));
+	}
+	return ret;
+}
+
 static inline ssize_t xread(int fd, void *buf, size_t len)
 {
 	ssize_t nr;
diff --git a/read-cache.c b/read-cache.c
index b8d83cc..ca3efbb 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -798,7 +798,7 @@ int read_cache_from(const char *path)
 		cache_mmap_size = st.st_size;
 		errno = EINVAL;
 		if (cache_mmap_size >= sizeof(struct cache_header) + 20)
-			cache_mmap = mmap(NULL, cache_mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+			cache_mmap = xmmap(NULL, cache_mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
 	}
 	close(fd);
 	if (cache_mmap == MAP_FAILED)
diff --git a/refs.c b/refs.c
index a101ff3..286ae45 100644
--- a/refs.c
+++ b/refs.c
@@ -1025,7 +1025,7 @@ int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *
 	fstat(logfd, &st);
 	if (!st.st_size)
 		die("Log %s is empty.", logfile);
-	logdata = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, logfd, 0);
+	logdata = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, logfd, 0);
 	close(logfd);
 
 	lastrec = NULL;
diff --git a/sha1_file.c b/sha1_file.c
index fb1032b..84037fe 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -355,10 +355,8 @@ static void read_info_alternates(const char * relative_base, int depth)
 		close(fd);
 		return;
 	}
-	map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	map = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if (map == MAP_FAILED)
-		return;
 
 	link_alt_odb_entries(map, map + st.st_size, '\n', relative_base, depth);
 
@@ -442,10 +440,8 @@ static int check_packed_git_idx(const char *path, unsigned long *idx_size_,
 		return -1;
 	}
 	idx_size = st.st_size;
-	idx_map = mmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	idx_map = xmmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if (idx_map == MAP_FAILED)
-		return -1;
 
 	index = idx_map;
 	*idx_map_ = idx_map;
@@ -630,7 +626,7 @@ unsigned char* use_pack(struct packed_git *p,
 			while (packed_git_limit < pack_mapped
 				&& unuse_one_window(p))
 				; /* nothing */
-			win->base = mmap(NULL, win->len,
+			win->base = xmmap(NULL, win->len,
 				PROT_READ, MAP_PRIVATE,
 				p->pack_fd, win->offset);
 			if (win->base == MAP_FAILED)
@@ -828,10 +824,8 @@ void *map_sha1_file(const unsigned char *sha1, unsigned long *size)
 		 */
 		sha1_file_open_flag = 0;
 	}
-	map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	map = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if (map == MAP_FAILED)
-		return NULL;
 	*size = st.st_size;
 	return map;
 }
@@ -1987,10 +1981,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, con
 
 	buf = "";
 	if (size)
-		buf = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+		buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
-	if (buf == MAP_FAILED)
-		return -1;
 
 	if (!type)
 		type = blob_type;
-- 
1.4.4.3.g2e63

^ permalink raw reply related

* [PATCH 6/7] Release pack windows before reporting out of memory.
From: Shawn O. Pearce @ 2006-12-24  5:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <487c7d0ea81f2f82f330e277e0aea38a66ca7cfe.1166939109.git.spearce@spearce.org>

If we are about to fail because this process has run out of memory we
should first try to automatically control our appetite for address
space by releasing enough least-recently-used pack windows to gain
back enough memory such that we might actually be able to meet the
current allocation request.

This should help users who have fairly large repositories but are
working on systems with relatively small virtual address space.
Many times we see reports on the mailing list of these users running
out of memory during various Git operations.  Dynamically decreasing
the amount of pack memory used when the demand for heap memory is
increasing is an intelligent solution to this problem.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 git-compat-util.h |   40 ++++++++++++++++++++++++++++++++--------
 sha1_file.c       |    7 +++++++
 2 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index e056339..4a417be 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -120,11 +120,17 @@ extern char *gitstrcasestr(const char *haystack, const char *needle);
 extern size_t gitstrlcpy(char *, const char *, size_t);
 #endif
 
+extern void release_pack_memory(size_t);
+
 static inline char* xstrdup(const char *str)
 {
 	char *ret = strdup(str);
-	if (!ret)
-		die("Out of memory, strdup failed");
+	if (!ret) {
+		release_pack_memory(strlen(str) + 1);
+		ret = strdup(str);
+		if (!ret)
+			die("Out of memory, strdup failed");
+	}
 	return ret;
 }
 
@@ -133,8 +139,14 @@ static inline void *xmalloc(size_t size)
 	void *ret = malloc(size);
 	if (!ret && !size)
 		ret = malloc(1);
-	if (!ret)
-		die("Out of memory, malloc failed");
+	if (!ret) {
+		release_pack_memory(size);
+		ret = malloc(size);
+		if (!ret && !size)
+			ret = malloc(1);
+		if (!ret)
+			die("Out of memory, malloc failed");
+	}
 #ifdef XMALLOC_POISON
 	memset(ret, 0xA5, size);
 #endif
@@ -146,8 +158,14 @@ static inline void *xrealloc(void *ptr, size_t size)
 	void *ret = realloc(ptr, size);
 	if (!ret && !size)
 		ret = realloc(ptr, 1);
-	if (!ret)
-		die("Out of memory, realloc failed");
+	if (!ret) {
+		release_pack_memory(size);
+		ret = realloc(ptr, size);
+		if (!ret && !size)
+			ret = realloc(ptr, 1);
+		if (!ret)
+			die("Out of memory, realloc failed");
+	}
 	return ret;
 }
 
@@ -156,8 +174,14 @@ static inline void *xcalloc(size_t nmemb, size_t size)
 	void *ret = calloc(nmemb, size);
 	if (!ret && (!nmemb || !size))
 		ret = calloc(1, 1);
-	if (!ret)
-		die("Out of memory, calloc failed");
+	if (!ret) {
+		release_pack_memory(nmemb * size);
+		ret = calloc(nmemb, size);
+		if (!ret && (!nmemb || !size))
+			ret = calloc(1, 1);
+		if (!ret)
+			die("Out of memory, calloc failed");
+	}
 	return ret;
 }
 
diff --git a/sha1_file.c b/sha1_file.c
index 8de8ce0..fb1032b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -522,6 +522,13 @@ static int unuse_one_window(struct packed_git *current)
 	return 0;
 }
 
+void release_pack_memory(size_t need)
+{
+	size_t cur = pack_mapped;
+	while (need >= (cur - pack_mapped) && unuse_one_window(NULL))
+		; /* nothing */
+}
+
 void unuse_pack(struct pack_window **w_cursor)
 {
 	struct pack_window *w = *w_cursor;
-- 
1.4.4.3.g2e63

^ permalink raw reply related

* [PATCH 5/7] Don't exit successfully on EPIPE in read_or_die.
From: Shawn O. Pearce @ 2006-12-24  5:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <487c7d0ea81f2f82f330e277e0aea38a66ca7cfe.1166939109.git.spearce@spearce.org>

Junio pointed out to me that read() won't return EPIPE like write()
does, so there is no reason for us to exit successfully if we get
an EPIPE error while in read_or_die.

Instead we should just die if we get any error from read() while
in read_or_die as there is no reasonable recovery.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 write_or_die.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/write_or_die.c b/write_or_die.c
index a56d992..8cf6486 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -9,11 +9,8 @@ void read_or_die(int fd, void *buf, size_t count)
 		loaded = xread(fd, p, count);
 		if (loaded == 0)
 			die("unexpected end of file");
-		else if (loaded < 0) {
-			if (errno == EPIPE)
-				exit(0);
+		else if (loaded < 0)
 			die("read error (%s)", strerror(errno));
-		}
 		count -= loaded;
 		p += loaded;
 	}
-- 
1.4.4.3.g2e63

^ permalink raw reply related

* [PATCH 4/7] Default core.packdGitWindowSize to 1 MiB if NO_MMAP.
From: Shawn O. Pearce @ 2006-12-24  5:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <487c7d0ea81f2f82f330e277e0aea38a66ca7cfe.1166939109.git.spearce@spearce.org>

If the compiler has asked us to disable use of mmap() on their
platform then we are forced to use git_mmap and its emulation via
pread.  In this case large (e.g. 32 MiB) windows for pack access
are simply too big as a command will wind up reading a lot more
data than it will ever need, significantly reducing response time.

To prevent a high latency when NO_MMAP has been selected we now
use a default of 1 MiB for core.packedGitWindowSize.  Credit goes
to Linus and Junio for recommending this more reasonable setting.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 environment.c     |    2 +-
 git-compat-util.h |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/environment.c b/environment.c
index 289fc84..e89aab4 100644
--- a/environment.c
+++ b/environment.c
@@ -22,7 +22,7 @@ char git_commit_encoding[MAX_ENCODING_LENGTH] = "utf-8";
 int shared_repository = PERM_UMASK;
 const char *apply_default_whitespace;
 int zlib_compression_level = Z_DEFAULT_COMPRESSION;
-size_t packed_git_window_size = 32 * 1024 * 1024;
+size_t packed_git_window_size = DEFAULT_packed_git_window_size;
 size_t packed_git_limit = 256 * 1024 * 1024;
 int pager_in_use;
 int pager_use_color = 1;
diff --git a/git-compat-util.h b/git-compat-util.h
index 5d9eb26..e056339 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -87,6 +87,7 @@ extern void set_warn_routine(void (*routine)(const char *warn, va_list params));
 #define MAP_FAILED ((void*)-1)
 #endif
 
+#define DEFAULT_packed_git_window_size (1 * 1024 * 1024)
 #define mmap git_mmap
 #define munmap git_munmap
 extern void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
@@ -95,6 +96,7 @@ extern int git_munmap(void *start, size_t length);
 #else /* NO_MMAP */
 
 #include <sys/mman.h>
+#define DEFAULT_packed_git_window_size (32 * 1024 * 1024)
 
 #endif /* NO_MMAP */
 
-- 
1.4.4.3.g2e63

^ permalink raw reply related

* [PATCH 3/7] Ensure packed_git.next is initialized to NULL.
From: Shawn O. Pearce @ 2006-12-24  5:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <487c7d0ea81f2f82f330e277e0aea38a66ca7cfe.1166939109.git.spearce@spearce.org>

Junio noticed while reviewing this patch series that I removed the
initialization of packed_git.next = NULL in 88078baa.  That removal
was not intended so I'm restoring the initialization where necessary.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 sha1_file.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 1a87f95..8de8ce0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -676,6 +676,7 @@ struct packed_git *add_packed_git(char *path, int path_len, int local)
 	p->index_size = idx_size;
 	p->pack_size = st.st_size;
 	p->index_base = idx_map;
+	p->next = NULL;
 	p->windows = NULL;
 	p->pack_fd = -1;
 	p->pack_local = local;
@@ -707,6 +708,7 @@ struct packed_git *parse_pack_index_file(const unsigned char *sha1, char *idx_pa
 	p->index_size = idx_size;
 	p->pack_size = 0;
 	p->index_base = idx_map;
+	p->next = NULL;
 	p->windows = NULL;
 	p->pack_fd = -1;
 	hashcpy(p->sha1, sha1);
-- 
1.4.4.3.g2e63

^ permalink raw reply related

* [PATCH 2/7] Switch git_mmap to use pread.
From: Shawn O. Pearce @ 2006-12-24  5:45 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <487c7d0ea81f2f82f330e277e0aea38a66ca7cfe.1166939109.git.spearce@spearce.org>

Now that Git depends on pread in index-pack its safe to say we can
also depend on it within the git_mmap emulation we activate when
NO_MMAP is set.  On most systems pread should be slightly faster
than an lseek/read/lseek sequence as its one system call vs. three
system calls.

We also now honor EAGAIN and EINTR error codes from pread and
restart the prior read.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 compat/mmap.c |   17 ++++-------------
 1 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/compat/mmap.c b/compat/mmap.c
index bb34c7e..98056f0 100644
--- a/compat/mmap.c
+++ b/compat/mmap.c
@@ -2,17 +2,11 @@
 
 void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset)
 {
-	int n = 0;
-	off_t current_offset = lseek(fd, 0, SEEK_CUR);
+	size_t n = 0;
 
 	if (start != NULL || !(flags & MAP_PRIVATE))
 		die("Invalid usage of mmap when built with NO_MMAP");
 
-	if (lseek(fd, offset, SEEK_SET) < 0) {
-		errno = EINVAL;
-		return MAP_FAILED;
-	}
-
 	start = xmalloc(length);
 	if (start == NULL) {
 		errno = ENOMEM;
@@ -20,7 +14,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
 	}
 
 	while (n < length) {
-		int count = read(fd, start+n, length-n);
+		ssize_t count = pread(fd, start + n, length - n, offset + n);
 
 		if (count == 0) {
 			memset(start+n, 0, length-n);
@@ -28,6 +22,8 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
 		}
 
 		if (count < 0) {
+			if (errno == EAGAIN || errno == EINTR)
+				continue;
 			free(start);
 			errno = EACCES;
 			return MAP_FAILED;
@@ -36,11 +32,6 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
 		n += count;
 	}
 
-	if (current_offset != lseek(fd, current_offset, SEEK_SET)) {
-		errno = EINVAL;
-		return MAP_FAILED;
-	}
-
 	return start;
 }
 
-- 
1.4.4.3.g2e63

^ permalink raw reply related

* [PATCH 1/7] Rename gitfakemmap to git_mmap.
From: Shawn O. Pearce @ 2006-12-24  5:45 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

This minor cleanup was suggested by Johannes Schindelin.

The mmap is still fake in the sense that we don't support PROT_WRITE
or MAP_SHARED with external modification at all, but that hasn't
stopped us from using mmap() thoughout the Git code.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 compat/mmap.c     |    6 +++---
 git-compat-util.h |    8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/compat/mmap.c b/compat/mmap.c
index 0fd46e7..bb34c7e 100644
--- a/compat/mmap.c
+++ b/compat/mmap.c
@@ -1,12 +1,12 @@
 #include "../git-compat-util.h"
 
-void *gitfakemmap(void *start, size_t length, int prot , int flags, int fd, off_t offset)
+void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset)
 {
 	int n = 0;
 	off_t current_offset = lseek(fd, 0, SEEK_CUR);
 
 	if (start != NULL || !(flags & MAP_PRIVATE))
-		die("Invalid usage of gitfakemmap.");
+		die("Invalid usage of mmap when built with NO_MMAP");
 
 	if (lseek(fd, offset, SEEK_SET) < 0) {
 		errno = EINVAL;
@@ -44,7 +44,7 @@ void *gitfakemmap(void *start, size_t length, int prot , int flags, int fd, off_
 	return start;
 }
 
-int gitfakemunmap(void *start, size_t length)
+int git_munmap(void *start, size_t length)
 {
 	free(start);
 	return 0;
diff --git a/git-compat-util.h b/git-compat-util.h
index f79365b..5d9eb26 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -87,10 +87,10 @@ extern void set_warn_routine(void (*routine)(const char *warn, va_list params));
 #define MAP_FAILED ((void*)-1)
 #endif
 
-#define mmap gitfakemmap
-#define munmap gitfakemunmap
-extern void *gitfakemmap(void *start, size_t length, int prot , int flags, int fd, off_t offset);
-extern int gitfakemunmap(void *start, size_t length);
+#define mmap git_mmap
+#define munmap git_munmap
+extern void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
+extern int git_munmap(void *start, size_t length);
 
 #else /* NO_MMAP */
 
-- 
1.4.4.3.g2e63

^ permalink raw reply related

* Re: [PATCH 11/17] Fully activate the sliding window pack access.
From: Shawn Pearce @ 2006-12-24  2:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Linus Torvalds, git
In-Reply-To: <7vodpuqtuf.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:
> Linus Torvalds <torvalds@osdl.org> writes:
> > Hmm. You seem to default the window size to 32MB.
> >
> > Maybe I'm reading that code wrong, but I think that's a bit sad.
> >
> > So I'd argue that if you fall back to read() (or pread) instead of mmap, 
> > the 32MB thing is way too big.
> >
> > So maybe you should make the default depend on NO_MMAP (although it would 
> > seem that the default Makefile makes Cygwin actually default to using mmap 
> > these days, so maybe it's not a big deal).
> 
> I agree that 32MB is too big for emulated mmap().

Yes, I agree too.  I'll submit some additional patches on top of
the existing 17 patch series (which I see is already in 'pu').

> We might want
> to further enhance the new use_pack() API so that the caller can
> say how much it expects to consume, to help pread() based
> emulation avoid reading unnecessary data.

I'm not sure that is worthwhile right now.

The only two callers who know how many bytes they expect is
pack-objects (during delta/whole reuse) and verify-pack (during the
SHA1 check of the packfile itself).  Both are maintenance commands
where preventing a read overshoot in the case of NO_MMAP is probably
not worthwhile, especially as both need to read every byte anyway.

-- 
Shawn.

^ permalink raw reply

* Re: [PATCH 11/17] Fully activate the sliding window pack access.
From: Shawn Pearce @ 2006-12-24  2:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Linus Torvalds, git
In-Reply-To: <7vodpuqtuf.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:
> Also the patch makes the maximum mapped from (1<<26) bytes to
> 256MB which is four-fold; I think tweaking such settings should
> be done as a separate step.

Sure, I don't have any objection to that, but I'm also not going
to write a patch to drop it back down to 128 MiB.

The old code was somewhat flawed. It did not always honor the upper
limit of PACK_MAX.  For example it always overshoots that limit
anytime we have a packfile larger than PACK_MAX.

The only way you will hit the new 256 MiB limit is if you actually
have a packfile (or a set of packfiles) that large.  If that is the
case then you were probably overshooting PACK_MAX before.  With this
series you will never overshoot core.packedGitLimit (256 MiB), no
matter how big your packfiles are.

So the new 256 MiB limit, although a four-fold increase, is actually
smaller virtual memory footprint for those users who may have been
hitting PACK_MAX before.

-- 
Shawn.

^ permalink raw reply

* Re: [PATCH 11/17] Fully activate the sliding window pack access.
From: Johannes Schindelin @ 2006-12-24  1:23 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Junio C Hamano, git, Shawn O. Pearce
In-Reply-To: <Pine.LNX.4.64.0612231207400.3671@woody.osdl.org>

Hi,

On Sat, 23 Dec 2006, Linus Torvalds wrote:

> On Sat, 23 Dec 2006, Junio C Hamano wrote:
> > 
> > I have been thinking that we should perhaps change Cygwin
> > default to NO_MMAP.  Safer but slower would be better than not
> > working correctly for people on FAT32.
> 
> The thing is, with a smaller pack access window, it might not even be 
> slower. I don't know just _how_ many hoops cygwin jumps through for mmap, 

AFAIK Windows API _does_ have an equivalent to mmap(), which just is not 
as easy to use.

[/me asks my friend Google]

Yes. The function is called CreateFile(), and it takes a _filename_, not a 
file descriptor. Which does not matter much, since Windows is braindead 
enough not to be able to have files deleted which are still opened. Mind 
you, renaming is no problem...

So, Cygwin has to keep record of the file name (and AFAICT it necessarily 
fails if mmap() is called with a file descriptor of a file which has been 
renamed _externally_ after opening) of the file descriptor.

Cygwin also has to reopen after a fork(), since Windows does not have 
fork(), and it has to be simulated by a CreateProcess() and reestablishing 
existing resources. That is the reason a fork() is really, really 
expensive on Windows.

> and maybe mmap under cygwin is actually perfectly fine, but at the same 
> time I do suspect that UNIX mmap semantics are a lot harder to emulate 
> than just a regular "pread()", so it's quite possible that by avoiding 
> mmap you could avoid a lot of complex cygwin code.

But should we really cater for a braindead API?

At the time, I _hated_ the fact that we had to change HEAD into a regular 
file, just because people feared that Windows could not handle symbolic 
links (and mind you, Cygwin handles them quite gracefully, using .lnk 
files).

Besides, IIRC you said yourself that mmap() is better than pread() when 
seeking a lot. And pack file accesses _are_ the perfect example for such a 
use case, no?

> And yes, making it all work on FAT32 would obviously be a good thing.

If really needed, we could make a hack for Windows, which just adds a new 
config option automagically set by init-db (on Windows only), and which is 
checked in gitfakemmap() and gitfakeunmmap(). (BTW I don't like the naming 
anymore; a good cleanup would be to name them git_mmap() and 
git_unmmap()).

Having said that, if somebody is really dedicated to working on Windows, 
she could overcome the mentioned problems. There's got to be a way to 
avoid the error on FAT32 without using NO_MMAP. Don't look at me, I hate 
Windows, I will not fix it.

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH 11/17] Fully activate the sliding window pack access.
From: Johannes Schindelin @ 2006-12-24  0:58 UTC (permalink / raw)
  To: Eric Blake; +Cc: git
In-Reply-To: <458D84BA.5040301@byu.net>

Hi,

On Sat, 23 Dec 2006, Eric Blake wrote:

> Indeed, on cygwin, using the 1.4.4.3 Makefile setting where NO_MMAP is 
> commented out, I have not seemed to have any mmap problems.

Strictly speaking, it is not commented out. It is not set. And it has been 
pointed out in another mail as well as in another thread that the problems 
without NO_MMAP arise on FAT32 when committing.

Which proves again that "works for me" often is not good enough.

Ciao,
Dscho

^ permalink raw reply

* Re: warn non utf-8 commit log messages.
From: Johannes Schindelin @ 2006-12-23 23:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vfyb6qth6.fsf_-_@assigned-by-dhcp.cox.net>

Hi,

On Sat, 23 Dec 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> But I had enough of UTF-8 for a day.
> >
> > Okay, so I lied (this are both patches revised and combined):
> 
> I am thinking of putting this in 'next', with the following
> changes on top of your combined patch.
> 
> git-commit-tree warns if the commit message does not minimally
> conform to the UTF-8 encoding when i18n.commitencoding is either
> unset, or set to "utf-8".  It does not die as in your version.

Yeah, this is nicer.

> -			if (w < width || space < 0) {
> +			if (w < width || !space) {

This is a real bug fix. Thank you. I changed quite a bit between offset 
and char*, and eventually forgot this part.

Ciao,
Dscho

^ permalink raw reply

* Re: confusion over the new branch and merge config
From: Jakub Narebski @ 2006-12-23 22:48 UTC (permalink / raw)
  To: git
In-Reply-To: <Pine.LNX.4.63.0612231655420.19693@wbgn013.biozentrum.uni-wuerzburg.de>

Johannes Schindelin wrote:

> On Sat, 23 Dec 2006, Junio C Hamano wrote:

>> Having said that, I think we _could_ do this.
>> 
>> If you (or other people) use branch.*.merge, with its value set
>> to remote name _and_ local name, and actually verify that either
>> form works without confusion, please report back and I'll apply.
> 
> I do not claim to understand your patch (I have no idea if || or && is 
> stronger in shell), but here is another proposition: if the config 
> variable starts with "refs/remotes/", assume it is local.

Why? You can track another repository tracking branches, using it as a kind
of proxy repository, even if it is not bare...
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

^ permalink raw reply

* Re: [PATCH] gitweb: Paginate commit/author/committer search output
From: Jakub Narebski @ 2006-12-23 22:43 UTC (permalink / raw)
  To: Robert Fitzsimons; +Cc: git
In-Reply-To: <20061223145712.GE11474@localhost>

Robert Fitzsimons wrote:
> Paginate commit/author/committer search output to only show 100 commits
> at a time, added appropriate nav links.
> 
> Signed-off-by: Robert Fitzsimons <robfitz@273k.net>
> --- 
> 
>> Although with search you have additional complication with marking match,
>> and "log" view like rather than "shortlog" like view... so I'm not sure
>> if it would truly help. On the other hand you can use --skip option you
>> have introduced...
> 
> I used the slower non--skip workflow for the moment, so at least there
> is no need to upgrade the core git commands.

First, git has tradition of introducing options (first) meant for gitweb,
and immediately making use of them. Examples: --git-dir=<path> option to
git wrapper because in mod_perl doesn't pass environmental variables to
subprocesses so setting $ENV{'GIT_DIR'} in gitweb wouldn't work;
--full-history option to git-rev-list for "history" view, because using
path limit instead of piping to git-diff-tree and using path limit of
git-diff-tree changed returned revisions, git-for-each-ref introduced
for better gitweb performance in "summary" view... So you wouldn't do
something unusual. And it is fairly easy to compile and install additional,
newest version of git.

Second, without --skip you have ugly tradeoff if you want to paginate
(search result, but not only that): either get pages*page-size revisions
and call parse_commit which in turn usually calls git-rev-list page-size
times; or get full info pages*page-size and skip (pages - 1)*page-size
bits of output.

And finally, --skip with your abandoned for now parsing revisions not
one by one, but by a bunch using one git command call would help
performance not only of non-pickaxe search, but also history view,
and log and shortlog views.

[...]
> +sub git_search_grep_body {

I'm not sure if it wouldn't be better to try to reuse git_log machinery,
just adding marking match, and removing everything but the immediate
context of match, to format_log_line_html... Just a thought...

-- 
Jakub Narebski
Poland

^ permalink raw reply

* Re: [PATCH 11/17] Fully activate the sliding window pack access.
From: Linus Torvalds @ 2006-12-23 20:10 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Shawn O. Pearce
In-Reply-To: <7vodpuqtuf.fsf@assigned-by-dhcp.cox.net>



On Sat, 23 Dec 2006, Junio C Hamano wrote:
> 
> I have been thinking that we should perhaps change Cygwin
> default to NO_MMAP.  Safer but slower would be better than not
> working correctly for people on FAT32.

The thing is, with a smaller pack access window, it might not even be 
slower. I don't know just _how_ many hoops cygwin jumps through for mmap, 
and maybe mmap under cygwin is actually perfectly fine, but at the same 
time I do suspect that UNIX mmap semantics are a lot harder to emulate 
than just a regular "pread()", so it's quite possible that by avoiding 
mmap you could avoid a lot of complex cygwin code.

And yes, making it all work on FAT32 would obviously be a good thing.

Only testing (or somebody who knows cygwin well) can tell.

			Linus

^ permalink raw reply

* warn non utf-8 commit log messages.
From: Junio C Hamano @ 2006-12-23 19:53 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git
In-Reply-To: <Pine.LNX.4.63.0612230048350.19693@wbgn013.biozentrum.uni-wuerzburg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> But I had enough of UTF-8 for a day.
>
> Okay, so I lied (this are both patches revised and combined):

I am thinking of putting this in 'next', with the following
changes on top of your combined patch.

git-commit-tree warns if the commit message does not minimally
conform to the UTF-8 encoding when i18n.commitencoding is either
unset, or set to "utf-8".  It does not die as in your version.

 builtin-commit-tree.c |   13 +++++++------
 utf8.c                |    2 +-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin-commit-tree.c b/builtin-commit-tree.c
index f274721..f641787 100644
--- a/builtin-commit-tree.c
+++ b/builtin-commit-tree.c
@@ -78,6 +78,11 @@ static int new_parent(int idx)
 	return 1;
 }
 
+static const char commit_utf8_warn[] =
+"Warning: commit message does not conform to UTF-8.\n"
+"You may want to amend it after fixing the message, or set the config\n"
+"variable i18n.commitencoding to the encoding your project uses.\n";
+
 int cmd_commit_tree(int argc, const char **argv, const char *prefix)
 {
 	int i;
@@ -133,12 +138,8 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
 
 	/* And check the encoding */
 	buffer[size] = '\0';
-	if (!strcmp(git_commit_encoding, "utf-8") && !is_utf8(buffer)) {
-		fprintf(stderr, "Commit message does not conform to UTF-8.\n"
-			"Please fix the message,"
-			" or set the config variable i18n.commitencoding.\n");
-		return 1;
-	}
+	if (!strcmp(git_commit_encoding, "utf-8") && !is_utf8(buffer))
+		fprintf(stderr, commit_utf8_warn);
 
 	if (!write_sha1_file(buffer, size, commit_type, commit_sha1)) {
 		printf("%s\n", sha1_to_hex(commit_sha1));
diff --git a/utf8.c b/utf8.c
index aed60ad..8fa6257 100644
--- a/utf8.c
+++ b/utf8.c
@@ -244,7 +244,7 @@ void print_wrapped_text(const char *text, int indent, int indent2, int width)
 	for (;;) {
 		char c = *text;
 		if (!c || isspace(c)) {
-			if (w < width || space < 0) {
+			if (w < width || !space) {
 				const char *start = bol;
 				if (space)
 					start = space;

^ permalink raw reply related

* Re: [PATCH 11/17] Fully activate the sliding window pack access.
From: Eric Blake @ 2006-12-23 19:34 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Shawn O. Pearce, Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0612231038410.3671@woody.osdl.org>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Linus Torvalds on 12/23/2006 11:44 AM:
> 
> In particular, in any setup that doesn't like mmap() at all (eg Cygwin), 
...
> So maybe you should make the default depend on NO_MMAP (although it would 
> seem that the default Makefile makes Cygwin actually default to using mmap 
> these days, so maybe it's not a big deal).

Indeed, on cygwin, using the 1.4.4.3 Makefile setting where NO_MMAP is
commented out, I have not seemed to have any mmap problems.

- --
Life is short - so eat dessert first!

Eric Blake             ebb9@byu.net
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFjYS684KuGfSFAYARAu4hAKDFfGVrgH0dnxkPHiUdrkAxz8waDQCgwGTo
jbUOvu8uI372BYxK2zHftjs=
=5Rwv
-----END PGP SIGNATURE-----

^ permalink raw reply

* Re: [PATCH 11/17] Fully activate the sliding window pack access.
From: Junio C Hamano @ 2006-12-23 19:45 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: git, Shawn O. Pearce
In-Reply-To: <Pine.LNX.4.64.0612231038410.3671@woody.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

> On Sat, 23 Dec 2006, Shawn O. Pearce wrote:
>>
>> This finally turns on the sliding window behavior for packfile data
>> access by mapping limited size windows and chaining them under the
>> packed_git->windows list.
>
> Hmm. You seem to default the window size to 32MB.
>
> Maybe I'm reading that code wrong, but I think that's a bit sad.
>
> In particular, in any setup that doesn't like mmap() at all (eg Cygwin), 
> and uses the "malloc+read" emulation, 32MB is actually likely much too 
> big. It's probably better to use something like a 1MB slice instead, since 
> otherwise you'll often be reading much too much.
>
> So I'd argue that if you fall back to read() (or pread) instead of mmap, 
> the 32MB thing is way too big.
>
> So maybe you should make the default depend on NO_MMAP (although it would 
> seem that the default Makefile makes Cygwin actually default to using mmap 
> these days, so maybe it's not a big deal).

I have been thinking that we should perhaps change Cygwin
default to NO_MMAP.  Safer but slower would be better than not
working correctly for people on FAT32.

I agree that 32MB is too big for emulated mmap().  We might want
to further enhance the new use_pack() API so that the caller can
say how much it expects to consume, to help pread() based
emulation avoid reading unnecessary data.

Also the patch makes the maximum mapped from (1<<26) bytes to
256MB which is four-fold; I think tweaking such settings should
be done as a separate step.

^ permalink raw reply

* Re: [PATCH 11/17] Fully activate the sliding window pack access.
From: Linus Torvalds @ 2006-12-23 18:44 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20061223073428.GL9837@spearce.org>



On Sat, 23 Dec 2006, Shawn O. Pearce wrote:
>
> This finally turns on the sliding window behavior for packfile data
> access by mapping limited size windows and chaining them under the
> packed_git->windows list.

Hmm. You seem to default the window size to 32MB.

Maybe I'm reading that code wrong, but I think that's a bit sad.

In particular, in any setup that doesn't like mmap() at all (eg Cygwin), 
and uses the "malloc+read" emulation, 32MB is actually likely much too 
big. It's probably better to use something like a 1MB slice instead, since 
otherwise you'll often be reading much too much.

So I'd argue that if you fall back to read() (or pread) instead of mmap, 
the 32MB thing is way too big.

So maybe you should make the default depend on NO_MMAP (although it would 
seem that the default Makefile makes Cygwin actually default to using mmap 
these days, so maybe it's not a big deal).

		Linus

^ permalink raw reply

* Re: What's cooking in git.git (topics)
From: Andy Parkins @ 2006-12-23 16:38 UTC (permalink / raw)
  To: git
In-Reply-To: <200612230112.24212.Josef.Weidendorfer@gmx.de>

On Saturday 2006, December 23 00:12, Josef Weidendorfer wrote:

> Andy: Did you check whether your disentangled commits each actually did
> compile on their own? If yes, how did you do it?

I didn't; they were easily separable and independent fortunately.  I don't 
have a good way of doing what you ask other than making the commits then 
testing each of those individually; then again there have been plenty of 
ocassions when a git guru tells me some magic that suddenly does exactly what 
I want.

In the case of testing my split commits I suppose it would need some way of 
pushing the working directory into a kind of alternate index temporarily.


Andy
-- 
Dr Andrew Parkins, M Eng (Hons), AMIEE
andyparkins@gmail.com

^ permalink raw reply

* [PATCH] Makefile: add quick-install-doc for installing pre-built manpages
From: Eric Wong @ 2006-12-23 16:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7virg3t54l.fsf@assigned-by-dhcp.cox.net>

This adds and uses the install-doc-quick.sh file to
Documentation/, which is usable for people who track either the
'html' or 'man' heads in Junio's repository (prefixed with
'origin/' if cloned locally).  You may override this by
specifying DOC_REF in the make environment or in config.mak.

GZ may also be set in the environment (or config.mak) if you
wish to gzip the documentation after installing it.

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 Documentation/Makefile             |    4 ++++
 Documentation/install-doc-quick.sh |   31 +++++++++++++++++++++++++++++++
 Makefile                           |    2 ++
 3 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index d68bc4a..93c7024 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -32,6 +32,7 @@ man7dir=$(mandir)/man7
 # DESTDIR=
 
 INSTALL?=install
+DOC_REF = origin/man
 
 -include ../config.mak.autogen
 
@@ -112,3 +113,6 @@ $(patsubst %.txt,%.html,$(wildcard howto/*.txt)): %.html : %.txt
 
 install-webdoc : html
 	sh ./install-webdoc.sh $(WEBDOC_DEST)
+
+quick-install:
+	sh ./install-doc-quick.sh $(DOC_REF) $(mandir)
diff --git a/Documentation/install-doc-quick.sh b/Documentation/install-doc-quick.sh
new file mode 100755
index 0000000..a640549
--- /dev/null
+++ b/Documentation/install-doc-quick.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+# This requires a branch named in $head
+# (usually 'man' or 'html', provided by the git.git repository)
+set -e
+head="$1"
+mandir="$2"
+SUBDIRECTORY_OK=t
+USAGE='<refname> <target directory>'
+. git-sh-setup
+export GIT_DIR
+
+test -z "$mandir" && usage
+if ! git-rev-parse --verify "$head^0" >/dev/null; then
+	echo >&2 "head: $head does not exist in the current repository"
+	usage
+fi
+
+GIT_INDEX_FILE=`pwd`/.quick-doc.index
+export GIT_INDEX_FILE
+rm -f "$GIT_INDEX_FILE"
+git-read-tree $head
+git-checkout-index -a -f --prefix="$mandir"/
+
+if test -n "$GZ"; then
+	cd "$mandir"
+	for i in `git-ls-tree -r --name-only $head`
+	do
+		gzip < $i > $i.gz && rm $i
+	done
+fi
+rm -f "$GIT_INDEX_FILE"
diff --git a/Makefile b/Makefile
index 4362297..ebc1a17 100644
--- a/Makefile
+++ b/Makefile
@@ -824,6 +824,8 @@ install: all
 install-doc:
 	$(MAKE) -C Documentation install
 
+quick-install-doc:
+	$(MAKE) -C Documentation quick-install
 
 
 
-- 
1.4.4.3.gc902c

^ permalink raw reply related

* Re: confusion over the new branch and merge config
From: Johannes Schindelin @ 2006-12-23 15:58 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Nicolas Pitre, git
In-Reply-To: <7vbqlvrldk.fsf@assigned-by-dhcp.cox.net>

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

Hi,

On Sat, 23 Dec 2006, Junio C Hamano wrote:

> Junio C Hamano <junkio@cox.net> writes:
> 
> > Jeff King <peff@peff.net> writes:
> >
> >> BTW, is there some explanation why branch.*.merge specifies a _remote_
> >> head? The following would make much more sense to me:
> >>
> >> [branch "master"]
> >> remote = origin
> >> merge = refs/remotes/origin/master
> >
> > Only *if* you store it in that tracking branch.  The name the
> > other party gives _do_ matter to you anyway, because you have to
> > _know_ it to fetch.  What it does NOT matter is if you use a
> > tracking branch, or if you do, which local tracking branch you
> > use to track it.
> 
> Having said that, I think we _could_ do this.
> 
> If you (or other people) use branch.*.merge, with its value set
> to remote name _and_ local name, and actually verify that either
> form works without confusion, please report back and I'll apply.

I do not claim to understand your patch (I have no idea if || or && is 
stronger in shell), but here is another proposition: if the config 
variablÃe starts with "refsremotes/", assume it is local.

Ciao,
Dscho

^ permalink raw reply

* [PATCH] gitweb: Paginate commit/author/committer search output
From: Robert Fitzsimons @ 2006-12-23 14:57 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Robert Fitzsimons, git
In-Reply-To: <200612231400.18774.jnareb@gmail.com>

Paginate commit/author/committer search output to only show 100 commits
at a time, added appropriate nav links.

Signed-off-by: Robert Fitzsimons <robfitz@273k.net>
---


> Although with search you have additional complication with marking match,
> and "log" view like rather than "shortlog" like view... so I'm not sure
> if it would truly help. On the other hand you can use --skip option you
> have introduced...

I used the slower non--skip workflow for the moment, so at least there
is no need to upgrade the core git commands.

Robert


 gitweb/gitweb.perl |  148 ++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 103 insertions(+), 45 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cc6bd0c..e4378b9 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2837,6 +2837,58 @@ sub git_heads_body {
 	print "</table>\n";
 }
 
+sub git_search_grep_body {
+	my ($greplist, $from, $to, $extra) = @_;
+	$from = 0 unless defined $from;
+	$to = $#{$greplist} if (!defined $to || $#{$greplist} < $to);
+
+	print "<table class=\"grep\" cellspacing=\"0\">\n";
+	my $alternate = 1;
+	for (my $i = $from; $i <= $to; $i++) {
+		my $commit = $greplist->[$i];
+		my %co = parse_commit($commit);
+		if (!%co) {
+			next;
+		}
+		if ($alternate) {
+			print "<tr class=\"dark\">\n";
+		} else {
+			print "<tr class=\"light\">\n";
+		}
+		$alternate ^= 1;
+		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
+		      "<td><i>" . esc_html(chop_str($co{'author_name'}, 15, 5)) . "</i></td>\n" .
+		      "<td>" .
+		      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}), -class => "list subject"},
+			       esc_html(chop_str($co{'title'}, 50)) . "<br/>");
+		my $comment = $co{'comment'};
+		foreach my $line (@$comment) {
+			if ($line =~ m/^(.*)($searchtext)(.*)$/i) {
+				my $lead = esc_html($1) || "";
+				$lead = chop_str($lead, 30, 10);
+				my $match = esc_html($2) || "";
+				my $trail = esc_html($3) || "";
+				$trail = chop_str($trail, 30, 10);
+				my $text = "$lead<span class=\"match\">$match</span>$trail";
+				print chop_str($text, 80, 5) . "<br/>\n";
+			}
+		}
+		print "</td>\n" .
+		      "<td class=\"link\">" .
+		      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'})}, "commit") .
+		      " | " .
+		      $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'}, hash_base=>$co{'id'})}, "tree");
+		print "</td>\n" .
+		      "</tr>\n";
+	}
+	if (defined $extra) {
+		print "<tr>\n" .
+		      "<td colspan=\"3\">$extra</td>\n" .
+		      "</tr>\n";
+	}
+	print "</table>\n";
+}
+
 ## ======================================================================
 ## ======================================================================
 ## actions
@@ -4154,6 +4206,9 @@ sub git_search {
 	if (!%co) {
 		die_error(undef, "Unknown commit object");
 	}
+	if (!defined $page) {
+		$page = 0;
+	}
 
 	$searchtype ||= 'commit';
 	if ($searchtype eq 'pickaxe') {
@@ -4166,11 +4221,7 @@ sub git_search {
 	}
 
 	git_header_html();
-	git_print_page_nav('','', $hash,$co{'tree'},$hash);
-	git_print_header_div('commit', esc_html($co{'title'}), $hash);
 
-	print "<table cellspacing=\"0\">\n";
-	my $alternate = 1;
 	if ($searchtype eq 'commit' or $searchtype eq 'author' or $searchtype eq 'committer') {
 		my $greptype;
 		if ($searchtype eq 'commit') {
@@ -4180,52 +4231,58 @@ sub git_search {
 		} elsif ($searchtype eq 'committer') {
 			$greptype = "--committer=";
 		}
-		$/ = "\0";
 		open my $fd, "-|", git_cmd(), "rev-list",
-			"--header", "--parents", ($greptype . $searchtext),
-			 $hash, "--"
+			("--max-count=" . (100 * ($page+1))),
+			($greptype . $searchtext),
+			$hash, "--"
 			or next;
-		while (my $commit_text = <$fd>) {
-			my @commit_lines = split "\n", $commit_text;
-			my %co = parse_commit(undef, \@commit_lines);
-			if (!%co) {
-				next;
-			}
-			if ($alternate) {
-				print "<tr class=\"dark\">\n";
-			} else {
-				print "<tr class=\"light\">\n";
-			}
-			$alternate ^= 1;
-			print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
-			      "<td><i>" . esc_html(chop_str($co{'author_name'}, 15, 5)) . "</i></td>\n" .
-			      "<td>" .
-			      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}), -class => "list subject"},
-			               esc_html(chop_str($co{'title'}, 50)) . "<br/>");
-			my $comment = $co{'comment'};
-			foreach my $line (@$comment) {
-				if ($line =~ m/^(.*)($searchtext)(.*)$/i) {
-					my $lead = esc_html($1) || "";
-					$lead = chop_str($lead, 30, 10);
-					my $match = esc_html($2) || "";
-					my $trail = esc_html($3) || "";
-					$trail = chop_str($trail, 30, 10);
-					my $text = "$lead<span class=\"match\">$match</span>$trail";
-					print chop_str($text, 80, 5) . "<br/>\n";
-				}
-			}
-			print "</td>\n" .
-			      "<td class=\"link\">" .
-			      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'})}, "commit") .
-			      " | " .
-			      $cgi->a({-href => href(action=>"tree", hash=>$co{'tree'}, hash_base=>$co{'id'})}, "tree");
-			print "</td>\n" .
-			      "</tr>\n";
-		}
+		my @revlist = map { chomp; $_ } <$fd>;
 		close $fd;
+
+		my $paging_nav = '';
+		if ($page > 0) {
+			$paging_nav .=
+				$cgi->a({-href => href(action=>"search", hash=>$hash,
+						       searchtext=>$searchtext, searchtype=>$searchtype)},
+					"first");
+			$paging_nav .= " &sdot; " .
+				$cgi->a({-href => href(action=>"search", hash=>$hash,
+						       searchtext=>$searchtext, searchtype=>$searchtype,
+						       page=>$page-1),
+					 -accesskey => "p", -title => "Alt-p"}, "prev");
+		} else {
+			$paging_nav .= "first";
+			$paging_nav .= " &sdot; prev";
+		}
+		if ($#revlist >= (100 * ($page+1)-1)) {
+			$paging_nav .= " &sdot; " .
+				$cgi->a({-href => href(action=>"search", hash=>$hash,
+						       searchtext=>$searchtext, searchtype=>$searchtype,
+						       page=>$page+1),
+					 -accesskey => "n", -title => "Alt-n"}, "next");
+		} else {
+			$paging_nav .= " &sdot; next";
+		}
+		my $next_link = '';
+		if ($#revlist >= (100 * ($page+1)-1)) {
+			$next_link =
+				$cgi->a({-href => href(action=>"search", hash=>$hash,
+						       searchtext=>$searchtext, searchtype=>$searchtype,
+						       page=>$page+1),
+					 -accesskey => "n", -title => "Alt-n"}, "next");
+		}
+
+		git_print_page_nav('','', $hash,$co{'tree'},$hash, $paging_nav);
+		git_print_header_div('commit', esc_html($co{'title'}), $hash);
+		git_search_grep_body(\@revlist, ($page * 100), $#revlist, $next_link);
 	}
 
 	if ($searchtype eq 'pickaxe') {
+		git_print_page_nav('','', $hash,$co{'tree'},$hash);
+		git_print_header_div('commit', esc_html($co{'title'}), $hash);
+
+		print "<table cellspacing=\"0\">\n";
+		my $alternate = 1;
 		$/ = "\n";
 		my $git_command = git_cmd_str();
 		open my $fd, "-|", "$git_command rev-list $hash | " .
@@ -4280,8 +4337,9 @@ sub git_search {
 			}
 		}
 		close $fd;
+
+		print "</table>\n";
 	}
-	print "</table>\n";
 	git_footer_html();
 }
 
-- 
1.4.4.3.gae7ae3

^ permalink raw reply related


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