Git development
 help / color / mirror / Atom feed
* [PATCH 2/5] sq_quote_argv and add_to_string rework with strbuf's.
From: Pierre Habouzit @ 2007-09-18 20:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <20070918223947.GB4535@artemis.corp>

* sq_quote_buf is made public, and works on a strbuf.
* sq_quote_argv also works on a strbuf.
* make sq_quote_argv take a "maxlen" argument to check the buffer won't grow
  too big.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 connect.c |   21 ++++++--------
 git.c     |   16 +++-------
 quote.c   |   91 ++++++++++++++++---------------------------------------------
 quote.h   |    9 ++----
 rsh.c     |   33 ++++++----------------
 trace.c   |   35 +++++++-----------------
 6 files changed, 60 insertions(+), 145 deletions(-)

diff --git a/connect.c b/connect.c
index 1653a0e..06d279e 100644
--- a/connect.c
+++ b/connect.c
@@ -577,16 +577,13 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
 	if (pid < 0)
 		die("unable to fork");
 	if (!pid) {
-		char command[MAX_CMD_LEN];
-		char *posn = command;
-		int size = MAX_CMD_LEN;
-		int of = 0;
+		struct strbuf cmd;
 
-		of |= add_to_string(&posn, &size, prog, 0);
-		of |= add_to_string(&posn, &size, " ", 0);
-		of |= add_to_string(&posn, &size, path, 1);
-
-		if (of)
+		strbuf_init(&cmd, MAX_CMD_LEN);
+		strbuf_addstr(&cmd, prog);
+		strbuf_addch(&cmd, ' ');
+		sq_quote_buf(&cmd, path);
+		if (cmd.len >= MAX_CMD_LEN)
 			die("command line too long");
 
 		dup2(pipefd[1][0], 0);
@@ -606,10 +603,10 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
 				ssh_basename++;
 
 			if (!port)
-				execlp(ssh, ssh_basename, host, command, NULL);
+				execlp(ssh, ssh_basename, host, cmd.buf, NULL);
 			else
 				execlp(ssh, ssh_basename, "-p", port, host,
-				       command, NULL);
+				       cmd.buf, NULL);
 		}
 		else {
 			unsetenv(ALTERNATE_DB_ENVIRONMENT);
@@ -618,7 +615,7 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
 			unsetenv(GIT_WORK_TREE_ENVIRONMENT);
 			unsetenv(GRAFT_ENVIRONMENT);
 			unsetenv(INDEX_ENVIRONMENT);
-			execlp("sh", "sh", "-c", command, NULL);
+			execlp("sh", "sh", "-c", cmd.buf, NULL);
 		}
 		die("exec failed");
 	}
diff --git a/git.c b/git.c
index 56ae8cc..9eaca1d 100644
--- a/git.c
+++ b/git.c
@@ -187,19 +187,13 @@ static int handle_alias(int *argcp, const char ***argv)
 	if (alias_string) {
 		if (alias_string[0] == '!') {
 			if (*argcp > 1) {
-				int i, sz = PATH_MAX;
-				char *s = xmalloc(sz), *new_alias = s;
+				struct strbuf buf;
 
-				add_to_string(&s, &sz, alias_string, 0);
+				strbuf_init(&buf, PATH_MAX);
+				strbuf_addstr(&buf, alias_string);
+				sq_quote_argv(&buf, (*argv) + 1, *argcp - 1, PATH_MAX);
 				free(alias_string);
-				alias_string = new_alias;
-				for (i = 1; i < *argcp &&
-					!add_to_string(&s, &sz, " ", 0) &&
-					!add_to_string(&s, &sz, (*argv)[i], 1)
-					; i++)
-					; /* do nothing */
-				if (!sz)
-					die("Too many or long arguments");
+				alias_string = buf.buf;
 			}
 			trace_printf("trace: alias to shell cmd: %s => %s\n",
 				     alias_command, alias_string + 1);
diff --git a/quote.c b/quote.c
index d88bf75..4df3262 100644
--- a/quote.c
+++ b/quote.c
@@ -20,29 +20,26 @@ static inline int need_bs_quote(char c)
 	return (c == '\'' || c == '!');
 }
 
-static size_t sq_quote_buf(char *dst, size_t n, const char *src)
+void sq_quote_buf(struct strbuf *dst, const char *src)
 {
-	char c;
-	char *bp = dst;
-	size_t len = 0;
-
-	EMIT('\'');
-	while ((c = *src++)) {
-		if (need_bs_quote(c)) {
-			EMIT('\'');
-			EMIT('\\');
-			EMIT(c);
-			EMIT('\'');
-		} else {
-			EMIT(c);
+	char *to_free = NULL;
+
+	if (dst->buf == src)
+		to_free = strbuf_detach(dst);
+
+	strbuf_addch(dst, '\'');
+	while (*src) {
+		size_t len = strcspn(src, "'\\");
+		strbuf_add(dst, src, len);
+		src += len;
+		while (need_bs_quote(*src)) {
+			strbuf_addstr(dst, "'\\");
+			strbuf_addch(dst, *src++);
+			strbuf_addch(dst, '\'');
 		}
 	}
-	EMIT('\'');
-
-	if ( n )
-		*bp = 0;
-
-	return len;
+	strbuf_addch(dst, '\'');
+	free(to_free);
 }
 
 void sq_quote_print(FILE *stream, const char *src)
@@ -62,11 +59,10 @@ void sq_quote_print(FILE *stream, const char *src)
 	fputc('\'', stream);
 }
 
-char *sq_quote_argv(const char** argv, int count)
+void sq_quote_argv(struct strbuf *dst, const char** argv, int count,
+                   size_t maxlen)
 {
-	char *buf, *to;
 	int i;
-	size_t len = 0;
 
 	/* Count argv if needed. */
 	if (count < 0) {
@@ -74,53 +70,14 @@ char *sq_quote_argv(const char** argv, int count)
 			; /* just counting */
 	}
 
-	/* Special case: no argv. */
-	if (!count)
-		return xcalloc(1,1);
-
-	/* Get destination buffer length. */
-	for (i = 0; i < count; i++)
-		len += sq_quote_buf(NULL, 0, argv[i]) + 1;
-
-	/* Alloc destination buffer. */
-	to = buf = xmalloc(len + 1);
-
 	/* Copy into destination buffer. */
+	strbuf_grow(dst, 32 * count);
 	for (i = 0; i < count; ++i) {
-		*to++ = ' ';
-		to += sq_quote_buf(to, len, argv[i]);
+		strbuf_addch(dst, ' ');
+		sq_quote_buf(dst, argv[i]);
+		if (maxlen && dst->len > maxlen)
+			die("Too many or long arguments");
 	}
-
-	return buf;
-}
-
-/*
- * Append a string to a string buffer, with or without shell quoting.
- * Return true if the buffer overflowed.
- */
-int add_to_string(char **ptrp, int *sizep, const char *str, int quote)
-{
-	char *p = *ptrp;
-	int size = *sizep;
-	int oc;
-	int err = 0;
-
-	if (quote)
-		oc = sq_quote_buf(p, size, str);
-	else {
-		oc = strlen(str);
-		memcpy(p, str, (size <= oc) ? size - 1 : oc);
-	}
-
-	if (size <= oc) {
-		err = 1;
-		oc = size - 1;
-	}
-
-	*ptrp += oc;
-	**ptrp = '\0';
-	*sizep -= oc;
-	return err;
 }
 
 char *sq_dequote(char *arg)
diff --git a/quote.h b/quote.h
index 8a59cc5..78e8d3e 100644
--- a/quote.h
+++ b/quote.h
@@ -29,13 +29,10 @@
  */
 
 extern void sq_quote_print(FILE *stream, const char *src);
-extern char *sq_quote_argv(const char** argv, int count);
 
-/*
- * Append a string to a string buffer, with or without shell quoting.
- * Return true if the buffer overflowed.
- */
-extern int add_to_string(char **ptrp, int *sizep, const char *str, int quote);
+extern void sq_quote_buf(struct strbuf *, const char *src);
+extern void sq_quote_argv(struct strbuf *, const char **argv, int count,
+                          size_t maxlen);
 
 /* This unwraps what sq_quote() produces in place, but returns
  * NULL if the input does not look like what sq_quote would have
diff --git a/rsh.c b/rsh.c
index 5754a23..e4ce255 100644
--- a/rsh.c
+++ b/rsh.c
@@ -7,14 +7,10 @@
 int setup_connection(int *fd_in, int *fd_out, const char *remote_prog,
 		     char *url, int rmt_argc, char **rmt_argv)
 {
+	struct strbuf cmd;
 	char *host;
 	char *path;
 	int sv[2];
-	char command[COMMAND_SIZE];
-	char *posn;
-	int sizen;
-	int of;
-	int i;
 	pid_t pid;
 
 	if (!strcmp(url, "-")) {
@@ -37,24 +33,13 @@ int setup_connection(int *fd_in, int *fd_out, const char *remote_prog,
 		return error("Bad URL: %s", url);
 	}
 	/* $GIT_RSH <host> "env GIT_DIR=<path> <remote_prog> <args...>" */
-	sizen = COMMAND_SIZE;
-	posn = command;
-	of = 0;
-	of |= add_to_string(&posn, &sizen, "env ", 0);
-	of |= add_to_string(&posn, &sizen, GIT_DIR_ENVIRONMENT "=", 0);
-	of |= add_to_string(&posn, &sizen, path, 1);
-	of |= add_to_string(&posn, &sizen, " ", 0);
-	of |= add_to_string(&posn, &sizen, remote_prog, 1);
-
-	for ( i = 0 ; i < rmt_argc ; i++ ) {
-		of |= add_to_string(&posn, &sizen, " ", 0);
-		of |= add_to_string(&posn, &sizen, rmt_argv[i], 1);
-	}
-
-	of |= add_to_string(&posn, &sizen, " -", 0);
-
-	if ( of )
-		return error("Command line too long");
+	strbuf_init(&cmd, COMMAND_SIZE);
+	strbuf_addstr(&cmd, "env " GIT_DIR_ENVIRONMENT "=");
+	sq_quote_buf(&cmd, path);
+	strbuf_addch(&cmd, ' ');
+	sq_quote_buf(&cmd, remote_prog);
+	sq_quote_argv(&cmd, (const char **)rmt_argv, rmt_argc, COMMAND_SIZE - 2);
+	strbuf_addstr(&cmd, " -");
 
 	if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv))
 		return error("Couldn't create socket");
@@ -74,7 +59,7 @@ int setup_connection(int *fd_in, int *fd_out, const char *remote_prog,
 		close(sv[1]);
 		dup2(sv[0], 0);
 		dup2(sv[0], 1);
-		execlp(ssh, ssh_basename, host, command, NULL);
+		execlp(ssh, ssh_basename, host, cmd.buf, NULL);
 	}
 	close(sv[0]);
 	*fd_in = sv[1];
diff --git a/trace.c b/trace.c
index 7961a27..ed1cdf0 100644
--- a/trace.c
+++ b/trace.c
@@ -113,39 +113,24 @@ void trace_printf(const char *format, ...)
 
 void trace_argv_printf(const char **argv, int count, const char *format, ...)
 {
-	char *argv_str, *format_str, *trace_str;
-	size_t argv_len, format_len, trace_len;
-	va_list rest;
+	struct strbuf trace;
+	va_list ap;
 	int need_close = 0;
 	int fd = get_trace_fd(&need_close);
 
 	if (!fd)
 		return;
 
-	/* Get the argv string. */
-	argv_str = sq_quote_argv(argv, count);
-	argv_len = strlen(argv_str);
-
-	/* Get the formated string. */
-	va_start(rest, format);
-	nfvasprintf(&format_str, format, rest);
-	va_end(rest);
+	strbuf_init(&trace, 0);
 
-	/* Allocate buffer for trace string. */
-	format_len = strlen(format_str);
-	trace_len = argv_len + format_len + 1; /* + 1 for \n */
-	trace_str = xmalloc(trace_len + 1);
+	va_start(ap, format);
+	strbuf_addvf(&trace, format, ap);
+	va_end(ap);
+	sq_quote_argv(&trace, argv, count, 0);
+	strbuf_addch(&trace, '\n');
 
-	/* Copy everything into the trace string. */
-	strncpy(trace_str, format_str, format_len);
-	strncpy(trace_str + format_len, argv_str, argv_len);
-	strcpy(trace_str + trace_len - 1, "\n");
-
-	write_or_whine_pipe(fd, trace_str, trace_len, err_msg);
-
-	free(argv_str);
-	free(format_str);
-	free(trace_str);
+	write_or_whine_pipe(fd, trace.buf, trace.len, err_msg);
+	strbuf_release(&trace);
 
 	if (need_close)
 		close(fd);
-- 
1.5.3.1

^ permalink raw reply related

* [PATCH 1/5] strbuf API additions and enhancements.
From: Pierre Habouzit @ 2007-09-18 17:18 UTC (permalink / raw)
  To: Junio C Hamano, Shawn O. Pearce; +Cc: git
In-Reply-To: <20070918223947.GB4535@artemis.corp>

Add strbuf_remove, change strbuf_insert:
  As both are special cases of strbuf_splice, implement them as such.
  gcc is able to do the math and generate almost optimal code this way.

Add strbuf_addvf (vsprintf-like)

Add strbuf_swap:
  Exchange the values of its arguments.
  Use it in fast-import.c

Also fix spacing issues in strbuf.h

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 commit.c      |    2 +-
 fast-import.c |    4 +---
 strbuf.c      |   38 ++++++++++++++++++++++++++++----------
 strbuf.h      |   19 +++++++++++++------
 4 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/commit.c b/commit.c
index f86fa77..55b08ec 100644
--- a/commit.c
+++ b/commit.c
@@ -656,7 +656,7 @@ static char *replace_encoding_header(char *buf, const char *encoding)
 	strbuf_attach(&tmp, buf, strlen(buf), strlen(buf) + 1);
 	if (is_encoding_utf8(encoding)) {
 		/* we have re-coded to UTF-8; drop the header */
-		strbuf_splice(&tmp, start, len, NULL, 0);
+		strbuf_remove(&tmp, start, len);
 	} else {
 		/* just replaces XXXX in 'encoding XXXX\n' */
 		strbuf_splice(&tmp, start + strlen("encoding "),
diff --git a/fast-import.c b/fast-import.c
index f990658..eddae22 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1111,9 +1111,7 @@ static int store_object(
 		if (last->no_swap) {
 			last->data = *dat;
 		} else {
-			struct strbuf tmp = *dat;
-			*dat = last->data;
-			last->data = tmp;
+			strbuf_swap(&last->data, dat);
 		}
 		last->offset = e->offset;
 	}
diff --git a/strbuf.c b/strbuf.c
index 59383ac..51aa2de 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -50,16 +50,6 @@ void strbuf_rtrim(struct strbuf *sb)
 	sb->buf[sb->len] = '\0';
 }
 
-void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
-{
-	strbuf_grow(sb, len);
-	if (pos > sb->len)
-		die("`pos' is too far after the end of the buffer");
-	memmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);
-	memcpy(sb->buf + pos, data, len);
-	strbuf_setlen(sb, sb->len + len);
-}
-
 void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
 				   const void *data, size_t dlen)
 {
@@ -79,6 +69,16 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
 	strbuf_setlen(sb, sb->len + dlen - len);
 }
 
+void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
+{
+	strbuf_splice(sb, pos, 0, data, len);
+}
+
+void strbuf_remove(struct strbuf *sb, size_t pos, size_t len)
+{
+	strbuf_splice(sb, pos, len, NULL, 0);
+}
+
 void strbuf_add(struct strbuf *sb, const void *data, size_t len)
 {
 	strbuf_grow(sb, len);
@@ -109,6 +109,24 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 	strbuf_setlen(sb, sb->len + len);
 }
 
+void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap)
+{
+	int len;
+
+	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+	if (len < 0) {
+		len = 0;
+	}
+	if (len > strbuf_avail(sb)) {
+		strbuf_grow(sb, len);
+		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+		if (len > strbuf_avail(sb)) {
+			die("this should not happen, your snprintf is broken");
+		}
+	}
+	strbuf_setlen(sb, sb->len + len);
+}
+
 size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 {
 	size_t res;
diff --git a/strbuf.h b/strbuf.h
index b2cbd97..ac3fb7b 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -55,15 +55,20 @@ extern void strbuf_release(struct strbuf *);
 extern void strbuf_reset(struct strbuf *);
 extern char *strbuf_detach(struct strbuf *);
 extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);
+static inline void strbuf_swap(struct strbuf *a, struct strbuf *b) {
+	struct strbuf tmp = *a;
+	*a = *b;
+	*b = tmp;
+}
 
 /*----- strbuf size related -----*/
 static inline size_t strbuf_avail(struct strbuf *sb) {
-    return sb->alloc ? sb->alloc - sb->len - 1 : 0;
+	return sb->alloc ? sb->alloc - sb->len - 1 : 0;
 }
 static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
-    assert (len < sb->alloc);
-    sb->len = len;
-    sb->buf[len] = '\0';
+	assert (len < sb->alloc);
+	sb->len = len;
+	sb->buf[len] = '\0';
 }
 
 extern void strbuf_grow(struct strbuf *, size_t);
@@ -78,12 +83,12 @@ static inline void strbuf_addch(struct strbuf *sb, int c) {
 	sb->buf[sb->len] = '\0';
 }
 
-/* inserts after pos, or appends if pos >= sb->len */
 extern void strbuf_insert(struct strbuf *, size_t pos, const void *, size_t);
+extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);
 
 /* splice pos..pos+len with given data */
 extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,
-						  const void *, size_t);
+                          const void *, size_t);
 
 extern void strbuf_add(struct strbuf *, const void *, size_t);
 static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
@@ -95,6 +100,8 @@ static inline void strbuf_addbuf(struct strbuf *sb, struct strbuf *sb2) {
 
 __attribute__((format(printf,2,3)))
 extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+__attribute__((format(printf,2,0)))
+extern void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list);
 
 extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
 /* XXX: if read fails, any partial read is undone */
-- 
1.5.3.1

^ permalink raw reply related

* let's refactor quoting ...
From: Pierre Habouzit @ 2007-09-18 22:39 UTC (permalink / raw)
  To: Git ML; +Cc: Junio C Hamano, Shawn O. Pearce

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

  Here comes a series dedicated to quote. I just can't resist the simple
pleasure to show:

$ git diff --shortstat HEAD~5.. ^strbuf*; git diff --shortstat HEAD~5.. strbuf*
 19 files changed, 523 insertions(+), 716 deletions(-)
 2 files changed, 41 insertions(+), 16 deletions(-)

  So it's an overall ~200 sloc reductions for a gain of 30 lines in
the strbuf module.


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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

^ permalink raw reply

* Re: [PATCH 3/4] Mention the parameters that git-pull would need to be equivalent to a --track in the git-checkout docs
From: Junio C Hamano @ 2007-09-18 22:38 UTC (permalink / raw)
  To: Federico Mena Quintero; +Cc: git
In-Reply-To: <1190077881.22387.63.camel@cacharro.xalalinux.org>

Federico Mena Quintero <federico@novell.com> writes:

> To be consistent with the git-branch docs.
>
> Signed-off-by: Federico Mena Quintero <federico@gnu.org>
> ---
>  Documentation/git-checkout.txt |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> index 734928b..6f22626 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -50,7 +50,8 @@ OPTIONS
>  --track::
>  	When -b is given and a branch is created off a remote branch,
>  	set up configuration so that git-pull will automatically
> -	retrieve data from the remote branch.  Set the
> +	retrieve data from the remote branch, otherwise you'll have to
> +	use "git pull <url>" explicitly.  Set the
>  	branch.autosetupmerge configuration variable to true if you
>  	want git-checkout and git-branch to always behave as if
>  	'--track' were given.

Hmph.

I'd rather make them consistent by dropping the not-so-correct
"otherwise" phrase from all three copies.  It is not "otherwise
you'll have to", but "instead you can".

^ permalink raw reply

* [PATCH] Prevent using bold text in entire gui for some fonts sometimes
From: Simon Sasburg @ 2007-09-18 22:33 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: git

---

When I first saw git-gui on windows, I noticed it wasn't using ugly
bold fonts for it's entire gui like it was in linux. I came up with
the following patch to fix this there.

And it worked, trange thing was, even git-gui without this patch
applied was using normal fonts now. The patch didn't seem to make any
difference for me anymore. So I chalked this up to weirdness of my
system.

Yesterday though a saw that a friends git-gui was showing the same
problem with the entire gui being in bold text. And this patch fixed
it.

So its a bit weird that this patch seemed to have 'permanent' effects
for me, even after it was reverted.... but on the other hand it's
really trivial.

 git-gui/git-gui.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index f789e91..28d7c21 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -1648,7 +1648,7 @@ proc apply_config {} {
 		set font [lindex $option 1]
 		if {[catch {
 			foreach {cn cv} $repo_config(gui.$name) {
-				font configure $font $cn $cv
+				font configure $font $cn $cv -weight normal
 			}
 			} err]} {
 			error_popup "Invalid font specified in gui.$name:\n\n$err"
-- 
1.5.3.1.21.g997f2-dirty

^ permalink raw reply related

* Re: Latest builtin-commit series
From: Pierre Habouzit @ 2007-09-18 22:31 UTC (permalink / raw)
  To: Kristian Høgsberg; +Cc: Git Mailing List, Junio C Hamano
In-Reply-To: <1190129009.23692.24.camel@hinata.boston.redhat.com>

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

On Tue, Sep 18, 2007 at 03:23:29PM +0000, Kristian Høgsberg wrote:
>       * rebase to Pierres strbuf changes.  Note, there is still some
>         strbuf tweaking required, to let stripspace work on a strbuf.

  Yeah I wondered if that would be a gain to migrate stripspace as a
first class citizen strbuf function. Note though, that if you always
want to stripspace in place, changing it is an overkill. I mean, there
isn't a lot of gain, as making it work on a strbuf is just a matter of:

    strbuf_setlen(&buf, stripspaces(buf.buf, buf.len));

If you want to do sth like:

    stripspaces(&buf, some_other_string);

And see the stripped version of "some_other_string" appended to the
strbuf "buf", then yes, it's not trivial to use the current stripspaces
as is.


General Note:
~~~~~~~~~~~~

  As a general rule, and I say this to the list, not only to you,
strbufs should not be used everywhere. It may _look_ like I'm peeing
over all the code putting strbufs anywhere I can, it's not true.
Strbuf's can help for two things:
  * the obvious: dealing with variable length strings, it's the least
    they can do.
  * be used as reused, variable length buffer, instead of loops that do:
      for (;;) {
         char *foo = xmalloc(...);
         [...]
         free(foo);
      }
    here, you can have a strbuf outside of the loop, and just reset it
    at the begining of the loop. You'll then work on a buffer that will
    stop beeing reallocated at some point. This make allocation patterns
    better.

  Strbuf's are not especially convenient when it comes to parsing.
Unlike the bstring's that have been discussed here recently, I don't
mean strbuf's to supersede all the standard C string API, because when
it comes to parsing, as soon as your buffers are properly NUL
terminated, C functions are _not_ usafe, no matter what people say.
strchr/memchr, strc?spn, strn?cmp/memcmp, ... are very efficient, and
there is little point to hide them behind stupid strbuf's APIs. Hence,
when it comes to parsing, you just fallback to a string, whose length is
known[0].

  That's the reason why I won't probably be the one converting
builtin-mailinfo.c to strbuf's because there is very little point to do
so in the current state of the art.


Cheers,


  [0] that allows to fallback to memchr[1] instead of strchr, which is
      slightly faster I'm told.

  [1] Note that you must be sure you don't have embedded NULs or memchr
      and strchr won't have the same result then ;)
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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

^ permalink raw reply

* Re: Problem with merge when renaming
From: Junio C Hamano @ 2007-09-18 22:28 UTC (permalink / raw)
  To: David Euresti; +Cc: git
In-Reply-To: <95b3d0af0709181334y1e21507ey485860e4d45aa26f@mail.gmail.com>

"David Euresti" <evelio@gmail.com> writes:

> I think I found a problem when you move a file into a directory of the
> same name.  Here's what I did.

Two questions.

 (1) git --version?

 (2) if you do "git merge -s resolve" instead of just "git
     merge", do you see a difference?

^ permalink raw reply

* Re: Latest builtin-commit series
From: Junio C Hamano @ 2007-09-18 22:25 UTC (permalink / raw)
  To: Kristian Høgsberg; +Cc: Git Mailing List
In-Reply-To: <1190129009.23692.24.camel@hinata.boston.redhat.com>

Kristian Høgsberg <krh@redhat.com> writes:

> Better late than never:
>
>       * rebase to Pierres strbuf changes.  Note, there is still some
>         strbuf tweaking required, to let stripspace work on a strbuf.
>         Also, I changed the semantics of stripspace to always add a
>         newline if the last line doesn't have one.  I believe the
>         current odd semantics (always remove the last newline) comes
>         from not being able to easily add a newline, but now that it's a
>         strbuf, that's easy.

I do not find the "remove trailing newline" so odd.  Didn't it
come because you sometimes needed to _not_ add it?

>       * Set the test suite default editor to '/bin/true' instead of ':'.
>         Since we're not exec'ing the editor from shell anymore, ':'
>         won't work.  Maybe we should special case ':' in launch_editor
>         or perhaps make launch_editor use system(3).  Not sure.

We've had a few threads on the list about what to do with:

	GIT_EDITOR='emacs -nw'

I think David's 08874658b450600e72bb7cb0d0747c1ec4b0bfe1
(git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR
accept commands) is a sensible to deal with this issue, and
prefer to see the same semantics kept in the C version.

^ permalink raw reply

* Re: Latest builtin-commit series
From: Johannes Schindelin @ 2007-09-18 22:24 UTC (permalink / raw)
  To: Alex Riesen; +Cc: Kristian Høgsberg, Git Mailing List, Junio C Hamano
In-Reply-To: <20070918213903.GA14488@steel.home>

Hi,

On Tue, 18 Sep 2007, Alex Riesen wrote:

> Kristian H?gsberg, Tue, Sep 18, 2007 17:23:29 +0200:
> >       * Set the test suite default editor to '/bin/true' instead of ':'.
> >         Since we're not exec'ing the editor from shell anymore, ':'
> >         won't work.  Maybe we should special case ':' in launch_editor
> >         or perhaps make launch_editor use system(3).  Not sure.
> 
> Special case "" (empty string)? MinGW may have problems with
> /bin/true, any future exotic ports notwithstanding (OS/2, anyone?).

No problem.  At least in msysGit.

Ciao,
Dscho

^ permalink raw reply

* [EGIT PATCH] Change to simplified icon.
From: Ben Konrath @ 2007-09-18 22:24 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: git

Hi Robin,

Here's a patch that changes the icon to something that is a little more
aesthetically pleasing than the one I iniitially submitted. Feel free to
use the one you like best.

Cheers, Ben

Signed-off-by: Ben Konrath <bkonrath@redhat.com>
---
 org.spearce.egit/egit.png |  Bin 305 -> 226 bytes
 1 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/org.spearce.egit/egit.png b/org.spearce.egit/egit.png
index 6e231c2c78068d313500afd0192962de8c8028d5..6782d4ab9b757aaa2c3d5edad42215ea02456836 100644
GIT binary patch
delta 128
zc$@)%0Du3n0^$LXK?o5MJ0{fucRrCxENxgxL_t(Y$L*B44FDhv17pNqjD{GE$xJ~P
zx*(py?zrZRNB~JYnPi6}fdmqmWq0m?BZUm`dpCgoqL=^;XxQg39(gUo$^d^orn8sp
i65v`i2_%r<6Ttw60WCp3d$>IS0000<MNUMnLSTZ2z%jJ|

delta 208
zc$@*y05AXI0kHy*K?n>MDJ}#O78sF9EPtm-L_t(Y$L&;G4!|G?6SI5mXvU-MWZege
zOB9Ki`S{Q;t(bz85&$=Hc{g|sC%C#_QY79WU<g4YYd?XW_(#bqO3c~g4CgB`xQyg0
zFV@9K?UxZyrk{c7I9H}(nSaW(v2QkAiA`2oS2kIRM*uZ?QR7&d2dySw^A{aA-95;z
zAIc?o10DFh3XY082`G|iI97Z`lTEg&;HzcId6nL2#m}H^LGS`$*mx<?D@u<50000<
KMNUMnLSTZEmRRfn

-- 
1.5.2.4

^ permalink raw reply

* [EGIT PATCH] Remove src dir and entry in build.properties.
From: Ben Konrath @ 2007-09-18 22:20 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: git

Hi Robin,

This is just a small clean up patch for the branding plugin.

CHeers, Ben

Signed-off-by: Ben Konrath <bkonrath@redhat.com>
---
 org.spearce.egit/build.properties |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
 delete mode 100644 org.spearce.egit/src/.gitignore

diff --git a/org.spearce.egit/build.properties b/org.spearce.egit/build.properties
index 2a21c05..22d4e45 100644
--- a/org.spearce.egit/build.properties
+++ b/org.spearce.egit/build.properties
@@ -1,5 +1,3 @@
-source.. = src/
-output.. = bin/
 bin.includes = META-INF/,\
                egit.png,\
                about.ini
diff --git a/org.spearce.egit/src/.gitignore b/org.spearce.egit/src/.gitignore
deleted file mode 100644
index e69de29..0000000
-- 
1.5.2.4

^ permalink raw reply related

* Re: diffcore-rename performance mode
From: Linus Torvalds @ 2007-09-18 22:12 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20070918082321.GA9883@coredump.intra.peff.net>



On Tue, 18 Sep 2007, Jeff King wrote:
> 
> With this patch:
> 
> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index 6bde439..531a844 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -362,10 +362,7 @@ void diffcore_rename(struct diff_options *options)
>  			m->score = estimate_similarity(one, two,
>  						       minimum_score);
>  			m->name_score = basename_same(one, two);
> -			diff_free_filespec_data(one);
>  		}
> -		/* We do not need the text anymore */
> -		diff_free_filespec_data(two);
>  		dst_cnt++;
>  	}
>  	/* cost matrix sorted by most to least similar pair */
> 
> My 20-minute diff becomes a 2-minute diff.

I think this is a reasonable patch, but only now *after* we limit the 
rename matrix.

The whole reason for freeing the filespec data was to avoid exploding the 
memory usage, but now that we aggressively limit the number of renames in 
flight anyway, I think we can probably remove the code that frees the file 
data in between all the rename similarity analysis.

			Linus

^ permalink raw reply

* [PATCH] Extend t6020 with another test for d/f interactions
From: Alex Riesen @ 2007-09-18 22:12 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano, David Euresti
In-Reply-To: <95b3d0af0709181334y1e21507ey485860e4d45aa26f@mail.gmail.com>

A test for a merge when one of the trees has a file moved into a
directory with the same name (not just file replaced by a directory).

The problem noticed and reported by David Euresti

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>

---
 t/t6020-merge-df.sh |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

David Euresti, Tue, Sep 18, 2007 22:34:53 +0200:
> If I try to merge in the changes from the other branch or if the other
> branch tries to merge in these changes I get this error:
> 
> dir/foo/foo.bin: unmerged (257cc5642cb1a054f08cc83f2d943e56fd3ebe99)
> fatal: git-write-tree: error building trees
> Merge with strategy recursive failed.

I extended the d/f merge test with the case. Have no idea how to fix
it yet though.

diff --git a/t/t6020-merge-df.sh b/t/t6020-merge-df.sh
index a19d49d..c081d3f 100755
--- a/t/t6020-merge-df.sh
+++ b/t/t6020-merge-df.sh
@@ -10,6 +10,7 @@ test_expect_success 'prepare repository' \
 'echo "Hello" > init &&
 git add init &&
 git commit -m "Initial commit" &&
+git branch init &&
 git branch B &&
 mkdir dir &&
 echo "foo" > dir/foo &&
@@ -22,4 +23,20 @@ git commit -m "File: dir"'
 
 test_expect_code 1 'Merge with d/f conflicts' 'git merge "merge msg" B master'
 
+test_expect_success 'file moved into a dir of same name' '
+    git reset --hard &&
+    git checkout -b d1 init &&
+    mkdir dir &&
+    mv init dir/init &&
+    mv dir init &&
+    git add . &&
+    git commit -a -m"init moved into init/init" &&
+    git checkout -b d2 init &&
+    echo file >file &&
+    git add file &&
+    git commit -m"unrelated change" &&
+    git checkout d1 &&
+    git merge d2
+'
+
 test_done
-- 
1.5.3.1.173.g9a67

^ permalink raw reply related

* Re: Latest builtin-commit series
From: Alex Riesen @ 2007-09-18 21:39 UTC (permalink / raw)
  To: Kristian Høgsberg; +Cc: Git Mailing List, Junio C Hamano
In-Reply-To: <1190129009.23692.24.camel@hinata.boston.redhat.com>

Kristian Høgsberg, Tue, Sep 18, 2007 17:23:29 +0200:
>       * Set the test suite default editor to '/bin/true' instead of ':'.
>         Since we're not exec'ing the editor from shell anymore, ':'
>         won't work.  Maybe we should special case ':' in launch_editor
>         or perhaps make launch_editor use system(3).  Not sure.

Special case "" (empty string)? MinGW may have problems with
/bin/true, any future exotic ports notwithstanding (OS/2, anyone?).

^ permalink raw reply

* Re: [EGIT PATCH] Add feature and plugin.
From: Ben Konrath @ 2007-09-18 21:20 UTC (permalink / raw)
  To: Andreas Ericsson; +Cc: git
In-Reply-To: <46EF81BD.7010609@op5.se>

On Tue, 2007-09-18 at 09:43 +0200, Andreas Ericsson wrote:
> Ben Konrath wrote:
> > Hi,
> > 
> > I made a feature and associated branding plugin for Egit. Including
> > these two plugins allows us to build Egit for Fedora but it also makes
> > it easy to create an update site for Egit.
> 
> 
> 
> > These two plugins also add an
> > entry for Egit in Help -> About Eclipse -> Feature Details. 
> > 
> 
> When you start writing "also" in your commit messages, it's a pretty good
> sign that you should have made many smaller commits rathern than one large.

Ok thanks. I think that I could have split this up into two commits.
I'll take that into consideration for future work.

Cheers, Ben

^ permalink raw reply

* Re: [PATCH] Mention that 'push .. master' is in explicit form master:refs/heads/master
From: Junio C Hamano @ 2007-09-18 20:54 UTC (permalink / raw)
  To: Jari Aalto; +Cc: git
In-Reply-To: <wsuomgyu.fsf@blue.sea.net>

Jari Aalto <jari.aalto@cante.net> writes:

> Signed-off-by: Jari Aalto <jari.aalto AT cante.net>
> ---
>  Documentation/git-push.txt |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 7b8e075..71ac450 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -105,7 +105,9 @@ git push origin master::
>  	Find a ref that matches `master` in the source repository
>  	(most likely, it would find `refs/heads/master`), and update
>  	the same ref (e.g. `refs/heads/master`) in `origin` repository
> -	with it.
> +	with it. The following would be exactly same command:
> +
> +	git push origin master:refs/heads/master

They _might_ be exactly the same.

The reason people often explicitly write

	$ git push $URL refs/heads/master:refs/heads/master

in their insns for newbies is because this form would not be
affected by the random factors at $URL repository (or your
repository) and will consistently get the same result.

	$ git push $URL foo

may push branch head 'foo' or tag 'foo' depending on which one
you have locally.  Having both is not encouraged, but spelling
the insn out explicitly as refs/heads/foo makes it clear the
command is talking about the branch even when there is a tag
with the same name.

^ permalink raw reply

* Re: [PATCH] instaweb: added support Ruby's WEBrick server
From: Eric Wong @ 2007-09-18 20:40 UTC (permalink / raw)
  To: mike dalessio; +Cc: git
In-Reply-To: <20070918121634.E8EFF814635@cyrano>

mike dalessio <mike@csa.net> wrote:
> running the webrick server with git requires Ruby and Ruby's YAML and
> Webrick libraries (both of which come standard with Ruby). nice for
> single-user standalone invocations.
> 
> the --httpd=webrick option generates a ruby script on the fly to read
> httpd.conf options and invoke the web server via library call. this
> script is placed in the .git/gitweb directory. it also generates a
> shell script in a feeble attempt to invoke ruby in a portable manner,
> which assumes that 'ruby' is in the user's $PATH.
> 
> Signed-off-by: Mike Dalessio <mike@csa.net>

Acked-by: Eric Wong <normalperson@yhbt.net>

> ---
>  Documentation/git-instaweb.txt |    3 +-
>  git-instaweb.sh                |   44 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-instaweb.txt b/Documentation/git-instaweb.txt
> index cec60ee..914fc4c 100644
> --- a/Documentation/git-instaweb.txt
> +++ b/Documentation/git-instaweb.txt
> @@ -27,7 +27,8 @@ OPTIONS
>  	The HTTP daemon command-line that will be executed.
>  	Command-line options may be specified here, and the
>  	configuration file will be added at the end of the command-line.
> -	Currently, lighttpd and apache2 are the only supported servers.
> +	Currently, lighttpd, apache2 and webrick are the only supported
> +	servers.
>  	(Default: lighttpd)
>  
>  -m|--module-path::
> diff --git a/git-instaweb.sh b/git-instaweb.sh
> index b79c6b6..803a754 100755
> --- a/git-instaweb.sh
> +++ b/git-instaweb.sh
> @@ -37,7 +37,9 @@ start_httpd () {
>  	else
>  		# many httpds are installed in /usr/sbin or /usr/local/sbin
>  		# these days and those are not in most users $PATHs
> -		for i in /usr/local/sbin /usr/sbin
> +		# in addition, we may have generated a server script
> +		# in $fqgitdir/gitweb.
> +		for i in /usr/local/sbin /usr/sbin $fqgitdir/gitweb
>  		do
>  			if test -x "$i/$httpd_only"
>  			then
> @@ -137,6 +139,43 @@ GIT_DIR="$fqgitdir"
>  export GIT_EXEC_PATH GIT_DIR
>  
>  
> +webrick_conf () {
> +	# generate a standalone server script in $fqgitdir/gitweb.
> +	cat > "$fqgitdir/gitweb/$httpd.rb" <<EOF
> +require 'webrick'
> +require 'yaml'
> +options = YAML::load_file(ARGV[0])
> +options[:StartCallback] = proc do
> +  File.open(options[:PidFile],"w") do |f|
> +    f.puts Process.pid
> +  end
> +end
> +options[:ServerType] = WEBrick::Daemon
> +server = WEBrick::HTTPServer.new(options)
> +['INT', 'TERM'].each do |signal|
> +  trap(signal) {server.shutdown}
> +end
> +server.start
> +EOF
> +	# generate a shell script to invoke the above ruby script,
> +	# which assumes _ruby_ is in the user's $PATH. that's _one_
> +	# portable way to run ruby, which could be installed anywhere,
> +	# really.
> +	cat > "$fqgitdir/gitweb/$httpd" <<EOF
> +#! /bin/sh
> +ruby $fqgitdir/gitweb/$httpd.rb \$*
> +EOF
> +	chmod +x "$fqgitdir/gitweb/$httpd"
> +
> +	cat > "$conf" <<EOF
> +:Port: $port
> +:DocumentRoot: "$fqgitdir/gitweb"
> +:DirectoryIndex: ["gitweb.cgi"]
> +:PidFile: "$fqgitdir/pid"
> +EOF
> +	test "$local" = true && echo ':BindAddress: "127.0.0.1"' >> "$conf"
> +}
> +
>  lighttpd_conf () {
>  	cat > "$conf" <<EOF
>  server.document-root = "$fqgitdir/gitweb"
> @@ -237,6 +276,9 @@ case "$httpd" in
>  *apache2*)
>  	apache2_conf
>  	;;
> +webrick)
> +	webrick_conf
> +	;;
>  *)
>  	echo "Unknown httpd specified: $httpd"
>  	exit 1
> -- 
> 1.5.2.5
> 

-- 
Eric Wong

^ permalink raw reply

* Problem with merge when renaming
From: David Euresti @ 2007-09-18 20:34 UTC (permalink / raw)
  To: git

Hi,

I think I found a problem when you move a file into a directory of the
same name.  Here's what I did.

In a repository I created a directory with a file in it.  Commit it.
Then I move the file into a tmp name, make a directory with that name,
and move it into the directory.
git-mv dir/foo dir/foo.bin
mkdir dir/foo
git-mv dir/foo.bin dir/foo/foo.bin

Then in another branch I make a completely unrelated change.  I create
a file in another directory.

If I try to merge in the changes from the other branch or if the other
branch tries to merge in these changes I get this error:

dir/foo/foo.bin: unmerged (257cc5642cb1a054f08cc83f2d943e56fd3ebe99)
fatal: git-write-tree: error building trees
Merge with strategy recursive failed.

This is the script I've been using
#!/bin/bash
#

# Make sure it's all clean
rm -rf git-repo

mkdir git-repo
pushd git-repo
git-init
mkdir dir
git-add dir
pushd dir
echo foo > foo
git-add foo
popd
git-commit -a -m "one"
popd

# Developer A moves file into dir of same name
pushd git-repo
git-checkout -b branch-a
git-mv dir/foo dir/foo.bin
mkdir dir/foo
git-mv dir/foo.bin dir/foo/foo.bin
git-commit -a -m "File renamed"
git-checkout master
popd

# Developer B makes completely unrelated changes.
pushd git-repo
git-checkout -b branch-b
echo baz > foo.txt
echo bar > bar.txt
git-add bar.txt
git-add foo.txt
git-commit -a -m "unrelated changes"
git-checkout master
popd

# Developer A wants to merge changes from B
pushd git-repo
#git-merge branch-a
echo
#git-merge branch-b
#popd

^ permalink raw reply

* Re: testsuite problems
From: Miklos Vajna @ 2007-09-18 19:35 UTC (permalink / raw)
  To: René Scharfe; +Cc: git, Junio C Hamano
In-Reply-To: <46F0054F.8060503@lsrfire.ath.cx>

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

On Tue, Sep 18, 2007 at 07:05:19PM +0200, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> Well, looks OK to me at least.  The Debian package (and thus Ubuntu's
> too) has a symlink related fix, from Christian Spieler, no less:
> 
>   http://packages.debian.org/changelogs/pool/main/u/unzip/unzip_5.52-9/changelog
>   http://ftp.de.debian.org/debian/pool/main/u/unzip/unzip_5.52-9.diff.gz
> 
> Perhaps this is missing from your version?

that was the problem. using vanilla unzip works fine, or after applying
the two security fixes a small patch is needed.

for the archives, here is the small patch that fixed the problem:

http://git.frugalware.org/gitweb/gitweb.cgi?p=frugalware-current.git;a=commit;h=c27277b2354b9ed4d6b0fbdb988643ad203f73c8

so the problem is not git-related

sorry for the noise and thanks for your help :)

- VMiklos

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

^ permalink raw reply

* Re: [EGIT PATCH] Add feature and plugin.
From: Robin Rosenberg @ 2007-09-18 19:06 UTC (permalink / raw)
  To: Andreas Ericsson; +Cc: Ben Konrath, git
In-Reply-To: <46EF81BD.7010609@op5.se>

tisdag 18 september 2007 skrev Andreas Ericsson:
> Ben Konrath wrote:
> > Hi,
> > 
> > I made a feature and associated branding plugin for Egit. Including
> > these two plugins allows us to build Egit for Fedora but it also makes
> > it easy to create an update site for Egit.
> 
> 
> 
> > These two plugins also add an
> > entry for Egit in Help -> About Eclipse -> Feature Details. 
> > 
> 
> When you start writing "also" in your commit messages, it's a pretty good
> sign that you should have made many smaller commits rathern than one large.

Seems it is a hint about what can be done later. That's generally something to
put in cover messages. 

The ability to create an update site encouraged me to create one too. It makes
updating all my eclipses much easier. Thanks Ben.

I've merged and pushed the earlier version before receiving this version whose
content comes as a follow up commit.

-- robin

^ permalink raw reply

* Re: [rfc] git submodules howto
From: Michael Smith @ 2007-09-18 18:12 UTC (permalink / raw)
  To: Miklos Vajna; +Cc: git
In-Reply-To: <20070918161112.GR19019@genesis.frugalware.org>

On Tue, 18 Sep 2007, Miklos Vajna wrote:

> Michael, i think the wiki version is better as my example does not
> contain any extra to the wiki version. is it ok if i would send a patch
> to include your work in the official docs?

Thanks, that would be great.

> i'm not sure how should i mention you, maybe in the commit message?

Maybe signed-off-by is appropriate? Otherwise, don't worry about it.

Mike

^ permalink raw reply

* Re: State of Perforce importing.
From: Reece Dunn @ 2007-09-18 17:53 UTC (permalink / raw)
  To: Sam Vilain, Git
In-Reply-To: <20070918154918.GA19106@old.davidb.org>

On 18/09/2007, David Brown <git@davidb.org> wrote:
> On Tue, Sep 18, 2007 at 07:27:13PM +1200, Sam Vilain wrote:
>
> >I'm pretty close to giving a newer one a spin, that actually imports
> >from the raw perforce back-end files without needing the perforce
> >server.  I am hoping that this should give a very clean import and will
> >be very fast and efficient, sending files that share ancestry to gfi in
> >sequence so that the on-the-fly delta system works.
>
> Unfortunately, this isn't something I'm going to be able to use.  The
> Perforce server will remain live, and resides on a machine I don't have
> access to.

I use git-p4 in the same way. The best approach would be to have both
tools and to use whichever one best matches your needs.

> >It could possibly be adapted to use the p4 client (though I'd expect
> >that to be relatively slow per-revision), and possibly be extended to be
> >bidirectional as all of the upstream change number information is
> >recorded, a la git-svn.
>
> I was able to get 'git-p4' to work a lot better by using @all, but it still
> has some problems, at least bad interactions with P4.

I have also seen this.

>    - It doesn't use any client spec.  Our P4 server space is a complete
>      mismash and has to be fixed up to get a sane directory layout.  For
>      example, some revisions have hundred-MB tar files sitting in the root
>      directory and I don't want that in the repo.  I also need to exclude
>      directories, and in some cases completely rearrange the directory
>      layout.

The directory exclusion you could do the other way, if git-p4
supported multiple directory paths.

The main issues with using client workspaces is that they require you
to use `p4 sync`, whereas git-p4 uses `p4 print` and that they may
change as the repository changes, but Perforce does not track these
changes.

That said, something like what workspaces are doing (allowing you to
specify multiple paths and where they are to go) would be useful.

>    - Our P4 server is set to be case insensitive.  'git-p4' ignores paths
>      that come back from the server that are specified using a different
>      case.  Unfortunately, this means that a handful of files just get
>      randomly dropped from each revision.

It is worse than this. If you have:

    p4 integrate foo Foo
    p4 delete foo

then git-p4 will completely remove the file foo from the repository! I
reported this a while back, but did not get a reply.

This is something I want to fix, as doing an '@all', rebase or sync is
broken in this case when dealing with renamed files as above when
importing a repository on a case insensitive system.

The alternative is to do the importing from Perforce on a Linux
machine and then clone/pull/rebase from it on a Windows machine.

>      I tried importing a client path instead of a depot path, but the names
>      that come back from 'p4 files' are depot based so none ever match.  I
>      end up with a nice revision history of entirely empty trees.

Ideally, git-p4 should bail out here with an error about the path
needing to be specified as a depot path.

> I'm probably going to end up writing an importer that uses an actual client
> workspace to let Perforce do the client mapping.

As eluded to above, I want to extend git-p4 to support a
workspace-like importer map file to use instead of a specific depot
path (with a single depot path being supported as well for backward
compatibility and simplicity if the repository you are importing from
has a simple layout).

There is no need to create yet another Perforce importing tool, git-p4
works well in most cases. If we focus on improving git-p4, extending
it to support the functionality mentioned here, fix the issues that
there are with it, then that will be more beneficial to the community
as they will not have to learn another tool with a different set of
bugs and issues.

> I'm also going to have to
> put some work into some code to clean up the log messages, since most of
> our changes have as a first line "New Features:", which makes for a rather
> uninformative shortlog.

I would not do that. It is a good idea to keep the original log
messages, even if it does make for an uninformative shortlog. Look at
some of the CVS/SVN imported logs!

- Reece

^ permalink raw reply

* Re: testsuite problems
From: René Scharfe @ 2007-09-18 17:05 UTC (permalink / raw)
  To: Miklos Vajna; +Cc: git, Junio C Hamano
In-Reply-To: <20070918155214.GQ19019@genesis.frugalware.org>

Miklos Vajna schrieb:
> $ unzip -v
> UnZip 5.52 of 28 February 2005, by Info-ZIP.  Maintained by C. Spieler.
> Send
> bug reports using http://www.info-zip.org/zip-bug.html; see README for
> details.
> 
> Latest sources and executables are at
> ftp://ftp.info-zip.org/pub/infozip/ ;
> see ftp://ftp.info-zip.org/pub/infozip/UnZip.html for other sites.
> 
> Compiled with gcc 4.1.2 for Unix (Linux ELF) on May  9 2007.
> 
> UnZip special compilation options:
>         ASM_CRC
>         COPYRIGHT_CLEAN (PKZIP 0.9x unreducing method not supported)
>         SET_DIR_ATTRIB
>         TIMESTAMP
>         USE_EF_UT_TIME
>         USE_UNSHRINK (PKZIP/Zip 1.x unshrinking method supported)
>         USE_DEFLATE64 (PKZIP 4.x Deflate64(tm) supported)
>         VMS_TEXT_CONV
>         [decryption, version 2.9 of 05 May 2000]
> 
> if i'm not wrong then these options should be ok. :S

Well, looks OK to me at least.  The Debian package (and thus Ubuntu's
too) has a symlink related fix, from Christian Spieler, no less:

  http://packages.debian.org/changelogs/pool/main/u/unzip/unzip_5.52-9/changelog
  http://ftp.de.debian.org/debian/pool/main/u/unzip/unzip_5.52-9.diff.gz

Perhaps this is missing from your version?

Let's check if the ZIP file on our machines match.  In order for
this to succeed we need to first take out any randomness.  Please
apply the patch at the end of the mail, which sets GIT_AUTHOR_DATE
to a fixed value instead of using the current time, run the test
and then this:

  $ md5sum t/trash/d.zip	# run just after t5000-tar-tree.sh
  35f6183b7816960cadfc6cac74530640  t/trash/d.zip

The test passes just fine here, so if the checksums match then your
unzip is most likely to blame.  In this case you can work around
your problem by commenting out the "ln -s" in t5000-tar-tree.sh.

René


--- snip! ---
archive: make test repeatable

Set GIT_AUTHOR_DATE to a fixed value, just like it's already done
for GIT_COMMITTER_DATE.  This makes the test repeatable, and
resulting archive files can by compared on mailing lists simply by
comparing their checksums.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 42e28ab..f341451 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -50,7 +50,7 @@ test_expect_success \
      treeid=`git write-tree` &&
      echo $treeid >treeid &&
      git update-ref HEAD $(TZ=GMT GIT_COMMITTER_DATE="2005-05-27 22:00:00" \
-     git commit-tree $treeid </dev/null)'
+     GIT_AUTHOR_DATE="2007-09-18 19:00:00" git commit-tree $treeid </dev/null)
 
 test_expect_success \
     'git archive' \

^ permalink raw reply related

* Re: [rfc] git submodules howto
From: Miklos Vajna @ 2007-09-18 16:11 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: git, Michael Smith
In-Reply-To: <20070918155529.GD18476@fieldses.org>

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

[ adding Michael to CC. ]

On Tue, Sep 18, 2007 at 11:55:29AM -0400, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > hm, i did not know about the wiki page Michael created yesterday. so i
> > don't know what's the rule in case: if something is already in the wiki
> > then should or should not it be added to the 'official docs'?
> 
> It should.  We also need submodules documentation for the "official"
> documentation.

okay.

> If you want to base that work off of that wiki page instead of your
> original email, that's fine.  Just make sure you get Michael's
> permission first.

Michael, i think the wiki version is better as my example does not
contain any extra to the wiki version. is it ok if i would send a patch
to include your work in the official docs?

i'm not sure how should i mention you, maybe in the commit message?

- VMiklos

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

^ permalink raw reply

* Re: [rfc] git submodules howto
From: J. Bruce Fields @ 2007-09-18 15:55 UTC (permalink / raw)
  To: Miklos Vajna; +Cc: git
In-Reply-To: <20070918154734.GP19019@genesis.frugalware.org>

On Tue, Sep 18, 2007 at 05:47:34PM +0200, Miklos Vajna wrote:
> On Tue, Sep 18, 2007 at 09:29:40AM -0400, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > 2) does this worth adding to the documentation? maybe to a .txt under
> > > Documentation/howto? or to git-submodule.txt?
> > 
> > Could you add it as a new chapter to user-manual.txt (probably just
> > after the "git concepts" chapter), and then add links to that chapter
> > from git-submodule(1) and gitmodules(5)?
> 
> hm, i did not know about the wiki page Michael created yesterday. so i
> don't know what's the rule in case: if something is already in the wiki
> then should or should not it be added to the 'official docs'?

It should.  We also need submodules documentation for the "official"
documentation.

If you want to base that work off of that wiki page instead of your
original email, that's fine.  Just make sure you get Michael's
permission first.

--b.

^ 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