* [PATCH 1/3] Add strbuf_rtrim (to remove trailing spaces).
From: Pierre Habouzit @ 2007-09-08 11:53 UTC (permalink / raw)
To: git; +Cc: Pierre Habouzit
In-Reply-To: <11892523992761-git-send-email-madcoder@debian.org>
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
strbuf.c | 7 +++++++
strbuf.h | 3 +++
2 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/strbuf.c b/strbuf.c
index acc7fc8..565c343 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -28,6 +28,13 @@ void strbuf_grow(struct strbuf *sb, size_t extra) {
ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
}
+void strbuf_rtrim(struct strbuf *sb)
+{
+ while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
+ sb->len--;
+ sb->buf[sb->len] = '\0';
+}
+
void strbuf_add(struct strbuf *sb, const void *data, size_t len) {
strbuf_grow(sb, len);
memcpy(sb->buf + sb->len, data, len);
diff --git a/strbuf.h b/strbuf.h
index b40dc99..9b708cc 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -68,6 +68,9 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
extern void strbuf_grow(struct strbuf *, size_t);
+/*----- content related -----*/
+extern void strbuf_rtrim(struct strbuf *);
+
/*----- add data in your buffer -----*/
static inline void strbuf_addch(struct strbuf *sb, int c) {
strbuf_grow(sb, 1);
--
1.5.3.1
^ permalink raw reply related
* Use strbufs in commit.c (pretty printing)
From: Pierre Habouzit @ 2007-09-08 11:53 UTC (permalink / raw)
To: git
In-Reply-To: <20070902224213.GB431@artemis.corp>
Here is a 3 patch series to use strbufs for the pretty printing
functions in commit.c.
* The first one adds a needed useful function to strbufs (rtrim).
* The second one changes the semantics of interpolate (as the third
will need that) and fixes the rest of the code accordingly.
* Do the change.
Testsuite passes after step 2 and 3.
^ permalink raw reply
* [PATCH 2/3] Change semantics of interpolate to work like snprintf.
From: Pierre Habouzit @ 2007-09-08 11:53 UTC (permalink / raw)
To: git; +Cc: Pierre Habouzit
In-Reply-To: <1189252399433-git-send-email-madcoder@debian.org>
Also fix many off-by-ones and a useless memset.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
commit.c | 9 ++++-----
interpolate.c | 20 ++++++++------------
2 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/commit.c b/commit.c
index dc5a064..30db9f4 100644
--- a/commit.c
+++ b/commit.c
@@ -922,15 +922,14 @@ static long format_commit_message(const struct commit *commit,
do {
char *buf = *buf_p;
- unsigned long space = *space_p;
+ unsigned long len;
- space = interpolate(buf, space, user_format,
+ len = interpolate(buf, *space_p, user_format,
table, ARRAY_SIZE(table));
- if (!space)
+ if (len < *space_p)
break;
- buf = xrealloc(buf, space);
+ ALLOC_GROW(buf, len + 1, *space_p);
*buf_p = buf;
- *space_p = space;
} while (1);
interp_clear_table(table, ARRAY_SIZE(table));
diff --git a/interpolate.c b/interpolate.c
index 0082677..ff4fb10 100644
--- a/interpolate.c
+++ b/interpolate.c
@@ -44,9 +44,8 @@ void interp_clear_table(struct interp *table, int ninterps)
* { "%%", "%"},
* }
*
- * Returns 0 on a successful substitution pass that fits in result,
- * Returns a number of bytes needed to hold the full substituted
- * string otherwise.
+ * Returns the length of the substituted string (not including the final \0).
+ * Like with snprintf, if the result is >= reslen, then it overflowed.
*/
unsigned long interpolate(char *result, unsigned long reslen,
@@ -61,8 +60,6 @@ unsigned long interpolate(char *result, unsigned long reslen,
int i;
char c;
- memset(result, 0, reslen);
-
while ((c = *src)) {
if (c == '%') {
/* Try to match an interpolation string. */
@@ -78,9 +75,9 @@ unsigned long interpolate(char *result, unsigned long reslen,
value = interps[i].value;
valuelen = strlen(value);
- if (newlen + valuelen + 1 < reslen) {
+ if (newlen + valuelen < reslen) {
/* Substitute. */
- strncpy(dest, value, valuelen);
+ memcpy(dest, value, valuelen);
dest += valuelen;
}
newlen += valuelen;
@@ -89,14 +86,13 @@ unsigned long interpolate(char *result, unsigned long reslen,
}
}
/* Straight copy one non-interpolation character. */
- if (newlen + 1 < reslen)
+ if (newlen < reslen)
*dest++ = *src;
src++;
newlen++;
}
- if (newlen + 1 < reslen)
- return 0;
- else
- return newlen + 2;
+ if (reslen > 0)
+ *dest = '\0';
+ return newlen;
}
--
1.5.3.1
^ permalink raw reply related
* [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.
From: Pierre Habouzit @ 2007-09-08 11:53 UTC (permalink / raw)
To: git; +Cc: Pierre Habouzit
In-Reply-To: <1189252399316-git-send-email-madcoder@debian.org>
Also remove the "len" parameter, as:
(1) it was used as a max boundary, and every caller used ~0u
(2) we check for final NUL no matter what, so it doesn't help for speed.
As a result most of the pp_* function takes 3 arguments less, and we need
a lot less local variables, this makes the code way more readable, and
easier to extend if needed.
This patch also fixes some spacing and cosmetic issues.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
builtin-branch.c | 15 +--
builtin-log.c | 12 +-
builtin-rev-list.c | 13 +-
builtin-show-branch.c | 13 +-
commit.c | 330 ++++++++++++++++++-------------------------------
commit.h | 6 +-
log-tree.c | 56 +++------
7 files changed, 171 insertions(+), 274 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index 5f5c182..68cd5f9 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -268,23 +268,22 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
}
if (verbose) {
- char *subject = NULL;
- unsigned long subject_len = 0;
+ struct strbuf subject;
const char *sub = " **** invalid ref ****";
+ strbuf_init(&subject);
+
commit = lookup_commit(item->sha1);
if (commit && !parse_commit(commit)) {
- pretty_print_commit(CMIT_FMT_ONELINE, commit, ~0,
- &subject, &subject_len, 0,
- NULL, NULL, 0);
- sub = subject;
+ pretty_print_commit(CMIT_FMT_ONELINE, commit,
+ &subject, 0, NULL, NULL, 0);
+ sub = subject.buf;
}
printf("%c %s%-*s%s %s %s\n", c, branch_get_color(color),
maxwidth, item->name,
branch_get_color(COLOR_BRANCH_RESET),
find_unique_abbrev(item->sha1, abbrev), sub);
- if (subject)
- free(subject);
+ strbuf_release(&subject);
} else {
printf("%c %s%s%s\n", c, branch_get_color(color), item->name,
branch_get_color(COLOR_BRANCH_RESET));
diff --git a/builtin-log.c b/builtin-log.c
index fa81c25..21b35a1 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -763,13 +763,13 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
sign = '-';
if (verbose) {
- char *buf = NULL;
- unsigned long buflen = 0;
- pretty_print_commit(CMIT_FMT_ONELINE, commit, ~0,
- &buf, &buflen, 0, NULL, NULL, 0);
+ struct strbuf buf;
+ strbuf_init(&buf);
+ pretty_print_commit(CMIT_FMT_ONELINE, commit,
+ &buf, 0, NULL, NULL, 0);
printf("%c %s %s\n", sign,
- sha1_to_hex(commit->object.sha1), buf);
- free(buf);
+ sha1_to_hex(commit->object.sha1), buf.buf);
+ strbuf_release(&buf);
}
else {
printf("%c %s\n", sign,
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index ac551d5..7967f86 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -80,13 +80,12 @@ static void show_commit(struct commit *commit)
putchar('\n');
if (revs.verbose_header) {
- char *buf = NULL;
- unsigned long buflen = 0;
- pretty_print_commit(revs.commit_format, commit, ~0,
- &buf, &buflen,
- revs.abbrev, NULL, NULL, revs.date_mode);
- printf("%s%c", buf, hdr_termination);
- free(buf);
+ struct strbuf buf;
+ strbuf_init(&buf);
+ pretty_print_commit(revs.commit_format, commit,
+ &buf, revs.abbrev, NULL, NULL, revs.date_mode);
+ printf("%s%c", buf.buf, hdr_termination);
+ strbuf_release(&buf);
}
maybe_flush_or_die(stdout, "stdout");
if (commit->parents) {
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 4fa87f6..020116f 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -259,16 +259,15 @@ static void join_revs(struct commit_list **list_p,
static void show_one_commit(struct commit *commit, int no_name)
{
- char *pretty = NULL;
+ struct strbuf pretty;
const char *pretty_str = "(unavailable)";
- unsigned long pretty_len = 0;
struct commit_name *name = commit->util;
+ strbuf_init(&pretty);
if (commit->object.parsed) {
- pretty_print_commit(CMIT_FMT_ONELINE, commit, ~0,
- &pretty, &pretty_len,
- 0, NULL, NULL, 0);
- pretty_str = pretty;
+ pretty_print_commit(CMIT_FMT_ONELINE, commit,
+ &pretty, 0, NULL, NULL, 0);
+ pretty_str = pretty.buf;
}
if (!prefixcmp(pretty_str, "[PATCH] "))
pretty_str += 8;
@@ -289,7 +288,7 @@ static void show_one_commit(struct commit *commit, int no_name)
find_unique_abbrev(commit->object.sha1, 7));
}
puts(pretty_str);
- free(pretty);
+ strbuf_release(&pretty);
}
static char *ref_name[MAX_REVS + 1];
diff --git a/commit.c b/commit.c
index 30db9f4..dafa821 100644
--- a/commit.c
+++ b/commit.c
@@ -458,11 +458,11 @@ void clear_commit_marks(struct commit *commit, unsigned int mark)
/*
* Generic support for pretty-printing the header
*/
-static int get_one_line(const char *msg, unsigned long len)
+static int get_one_line(const char *msg)
{
int ret = 0;
- while (len--) {
+ for (;;) {
char c = *msg++;
if (!c)
break;
@@ -485,31 +485,24 @@ static int is_rfc2047_special(char ch)
return (non_ascii(ch) || (ch == '=') || (ch == '?') || (ch == '_'));
}
-static int add_rfc2047(char *buf, const char *line, int len,
+static void add_rfc2047(struct strbuf *sb, const char *line, int len,
const char *encoding)
{
- char *bp = buf;
- int i, needquote;
- char q_encoding[128];
- const char *q_encoding_fmt = "=?%s?q?";
+ int i, last;
- for (i = needquote = 0; !needquote && i < len; i++) {
+ for (i = 0; i < len; i++) {
int ch = line[i];
if (non_ascii(ch))
- needquote++;
- if ((i + 1 < len) &&
- (ch == '=' && line[i+1] == '?'))
- needquote++;
+ goto needquote;
+ if ((i + 1 < len) && (ch == '=' && line[i+1] == '?'))
+ goto needquote;
}
- if (!needquote)
- return sprintf(buf, "%.*s", len, line);
-
- i = snprintf(q_encoding, sizeof(q_encoding), q_encoding_fmt, encoding);
- if (sizeof(q_encoding) < i)
- die("Insanely long encoding name %s", encoding);
- memcpy(bp, q_encoding, i);
- bp += i;
- for (i = 0; i < len; i++) {
+ strbuf_add(sb, line, len);
+ return;
+
+needquote:
+ strbuf_addf(sb, "=?%s?q?", encoding);
+ for (i = last = 0; i < len; i++) {
unsigned ch = line[i] & 0xFF;
/*
* We encode ' ' using '=20' even though rfc2047
@@ -518,15 +511,13 @@ static int add_rfc2047(char *buf, const char *line, int len,
* leave the underscore in place.
*/
if (is_rfc2047_special(ch) || ch == ' ') {
- sprintf(bp, "=%02X", ch);
- bp += 3;
+ strbuf_add(sb, line + last, i - last);
+ strbuf_addf(sb, "=%02X", ch);
+ last = i + 1;
}
- else
- *bp++ = ch;
}
- memcpy(bp, "?=", 2);
- bp += 2;
- return bp - buf;
+ strbuf_add(sb, line + last, len - last);
+ strbuf_addstr(sb, "?=");
}
static unsigned long bound_rfc2047(unsigned long len, const char *encoding)
@@ -537,21 +528,21 @@ static unsigned long bound_rfc2047(unsigned long len, const char *encoding)
return len * 3 + elen + 100;
}
-static int add_user_info(const char *what, enum cmit_fmt fmt, char *buf,
+static void add_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,
const char *line, enum date_mode dmode,
const char *encoding)
{
char *date;
int namelen;
unsigned long time;
- int tz, ret;
+ int tz;
const char *filler = " ";
if (fmt == CMIT_FMT_ONELINE)
- return 0;
+ return;
date = strchr(line, '>');
if (!date)
- return 0;
+ return;
namelen = ++date - line;
time = strtoul(date, &date, 10);
tz = strtol(date, NULL, 10);
@@ -560,42 +551,35 @@ static int add_user_info(const char *what, enum cmit_fmt fmt, char *buf,
char *name_tail = strchr(line, '<');
int display_name_length;
if (!name_tail)
- return 0;
+ return;
while (line < name_tail && isspace(name_tail[-1]))
name_tail--;
display_name_length = name_tail - line;
filler = "";
- strcpy(buf, "From: ");
- ret = strlen(buf);
- ret += add_rfc2047(buf + ret, line, display_name_length,
- encoding);
- memcpy(buf + ret, name_tail, namelen - display_name_length);
- ret += namelen - display_name_length;
- buf[ret++] = '\n';
+ strbuf_addstr(sb, "From: ");
+ add_rfc2047(sb, line, display_name_length, encoding);
+ strbuf_add(sb, name_tail, namelen - display_name_length);
+ strbuf_addch(sb, '\n');
}
else {
- ret = sprintf(buf, "%s: %.*s%.*s\n", what,
+ strbuf_addf(sb, "%s: %.*s%.*s\n", what,
(fmt == CMIT_FMT_FULLER) ? 4 : 0,
filler, namelen, line);
}
switch (fmt) {
case CMIT_FMT_MEDIUM:
- ret += sprintf(buf + ret, "Date: %s\n",
- show_date(time, tz, dmode));
+ strbuf_addf(sb, "Date: %s\n", show_date(time, tz, dmode));
break;
case CMIT_FMT_EMAIL:
- ret += sprintf(buf + ret, "Date: %s\n",
- show_date(time, tz, DATE_RFC2822));
+ strbuf_addf(sb, "Date: %s\n", show_date(time, tz, DATE_RFC2822));
break;
case CMIT_FMT_FULLER:
- ret += sprintf(buf + ret, "%sDate: %s\n", what,
- show_date(time, tz, dmode));
+ strbuf_addf(sb, "%sDate: %s\n", what, show_date(time, tz, dmode));
break;
default:
/* notin' */
break;
}
- return ret;
}
static int is_empty_line(const char *line, int *len_p)
@@ -607,16 +591,16 @@ static int is_empty_line(const char *line, int *len_p)
return !len;
}
-static int add_merge_info(enum cmit_fmt fmt, char *buf, const struct commit *commit, int abbrev)
+static void add_merge_info(enum cmit_fmt fmt, struct strbuf *sb,
+ const struct commit *commit, int abbrev)
{
struct commit_list *parent = commit->parents;
- int offset;
if ((fmt == CMIT_FMT_ONELINE) || (fmt == CMIT_FMT_EMAIL) ||
!parent || !parent->next)
- return 0;
+ return;
- offset = sprintf(buf, "Merge:");
+ strbuf_addstr(sb, "Merge:");
while (parent) {
struct commit *p = parent->item;
@@ -629,10 +613,9 @@ static int add_merge_info(enum cmit_fmt fmt, char *buf, const struct commit *com
dots = (abbrev && strlen(hex) != 40) ? "..." : "";
parent = parent->next;
- offset += sprintf(buf + offset, " %s%s", hex, dots);
+ strbuf_addf(sb, " %s%s", hex, dots);
}
- buf[offset++] = '\n';
- return offset;
+ strbuf_addch(sb, '\n');
}
static char *get_header(const struct commit *commit, const char *key)
@@ -787,8 +770,8 @@ static void fill_person(struct interp *table, const char *msg, int len)
interp_set_entry(table, 6, show_date(date, tz, DATE_ISO8601));
}
-static long format_commit_message(const struct commit *commit,
- const char *msg, char **buf_p, unsigned long *space_p)
+static void format_commit_message(const struct commit *commit,
+ const char *msg, struct strbuf *sb)
{
struct interp table[] = {
{ "%H" }, /* commit hash */
@@ -841,6 +824,7 @@ static long format_commit_message(const struct commit *commit,
};
struct commit_list *p;
char parents[1024];
+ unsigned long len;
int i;
enum { HEADER, SUBJECT, BODY } state;
@@ -920,20 +904,15 @@ static long format_commit_message(const struct commit *commit,
if (!table[i].value)
interp_set_entry(table, i, "<unknown>");
- do {
- char *buf = *buf_p;
- unsigned long len;
-
- len = interpolate(buf, *space_p, user_format,
- table, ARRAY_SIZE(table));
- if (len < *space_p)
- break;
- ALLOC_GROW(buf, len + 1, *space_p);
- *buf_p = buf;
- } while (1);
+ len = interpolate(sb->buf + sb->len, strbuf_avail(sb),
+ user_format, table, ARRAY_SIZE(table));
+ if (len > strbuf_avail(sb)) {
+ strbuf_grow(sb, len);
+ interpolate(sb->buf + sb->len, strbuf_avail(sb) + 1,
+ user_format, table, ARRAY_SIZE(table));
+ }
+ strbuf_setlen(sb, sb->len + len);
interp_clear_table(table, ARRAY_SIZE(table));
-
- return strlen(*buf_p);
}
static void pp_header(enum cmit_fmt fmt,
@@ -942,34 +921,24 @@ static void pp_header(enum cmit_fmt fmt,
const char *encoding,
const struct commit *commit,
const char **msg_p,
- unsigned long *len_p,
- unsigned long *ofs_p,
- char **buf_p,
- unsigned long *space_p)
+ struct strbuf *sb)
{
int parents_shown = 0;
for (;;) {
const char *line = *msg_p;
- char *dst;
- int linelen = get_one_line(*msg_p, *len_p);
- unsigned long len;
+ int linelen = get_one_line(*msg_p);
if (!linelen)
return;
*msg_p += linelen;
- *len_p -= linelen;
if (linelen == 1)
/* End of header */
return;
- ALLOC_GROW(*buf_p, linelen + *ofs_p + 20, *space_p);
- dst = *buf_p + *ofs_p;
-
if (fmt == CMIT_FMT_RAW) {
- memcpy(dst, line, linelen);
- *ofs_p += linelen;
+ strbuf_add(sb, line, linelen);
continue;
}
@@ -987,10 +956,8 @@ static void pp_header(enum cmit_fmt fmt,
parent = parent->next, num++)
;
/* with enough slop */
- num = *ofs_p + num * 50 + 20;
- ALLOC_GROW(*buf_p, num, *space_p);
- dst = *buf_p + *ofs_p;
- *ofs_p += add_merge_info(fmt, dst, commit, abbrev);
+ strbuf_grow(sb, num * 50 + 20);
+ add_merge_info(fmt, sb, commit, abbrev);
parents_shown = 1;
}
@@ -1000,129 +967,100 @@ static void pp_header(enum cmit_fmt fmt,
* FULLER shows both authors and dates.
*/
if (!memcmp(line, "author ", 7)) {
- len = linelen;
+ unsigned long len = linelen;
if (fmt == CMIT_FMT_EMAIL)
len = bound_rfc2047(linelen, encoding);
- ALLOC_GROW(*buf_p, *ofs_p + len + 80, *space_p);
- dst = *buf_p + *ofs_p;
- *ofs_p += add_user_info("Author", fmt, dst,
- line + 7, dmode, encoding);
+ strbuf_grow(sb, len + 80);
+ add_user_info("Author", fmt, sb, line + 7, dmode, encoding);
}
if (!memcmp(line, "committer ", 10) &&
(fmt == CMIT_FMT_FULL || fmt == CMIT_FMT_FULLER)) {
- len = linelen;
+ unsigned long len = linelen;
if (fmt == CMIT_FMT_EMAIL)
len = bound_rfc2047(linelen, encoding);
- ALLOC_GROW(*buf_p, *ofs_p + len + 80, *space_p);
- dst = *buf_p + *ofs_p;
- *ofs_p += add_user_info("Commit", fmt, dst,
- line + 10, dmode, encoding);
+ strbuf_grow(sb, len + 80);
+ add_user_info("Commit", fmt, sb, line + 10, dmode, encoding);
}
}
}
static void pp_title_line(enum cmit_fmt fmt,
const char **msg_p,
- unsigned long *len_p,
- unsigned long *ofs_p,
- char **buf_p,
- unsigned long *space_p,
- int indent,
+ struct strbuf *sb,
const char *subject,
const char *after_subject,
const char *encoding,
int plain_non_ascii)
{
- char *title;
- unsigned long title_alloc, title_len;
+ struct strbuf title;
unsigned long len;
- title_len = 0;
- title_alloc = 80;
- title = xmalloc(title_alloc);
+ strbuf_init(&title);
+ strbuf_grow(&title, 80);
+
for (;;) {
const char *line = *msg_p;
- int linelen = get_one_line(line, *len_p);
- *msg_p += linelen;
- *len_p -= linelen;
+ int linelen = get_one_line(line);
+ *msg_p += linelen;
if (!linelen || is_empty_line(line, &linelen))
break;
- if (title_alloc <= title_len + linelen + 2) {
- title_alloc = title_len + linelen + 80;
- title = xrealloc(title, title_alloc);
- }
- len = 0;
- if (title_len) {
+ strbuf_grow(&title, linelen + 2);
+ if (title.len) {
if (fmt == CMIT_FMT_EMAIL) {
- len++;
- title[title_len++] = '\n';
+ strbuf_addch(&title, '\n');
}
- len++;
- title[title_len++] = ' ';
+ strbuf_addch(&title, ' ');
}
- memcpy(title + title_len, line, linelen);
- title_len += linelen;
+ strbuf_add(&title, line, linelen);
}
/* Enough slop for the MIME header and rfc2047 */
- len = bound_rfc2047(title_len, encoding)+ 1000;
+ len = bound_rfc2047(title.len, encoding) + 1000;
if (subject)
len += strlen(subject);
if (after_subject)
len += strlen(after_subject);
if (encoding)
len += strlen(encoding);
- ALLOC_GROW(*buf_p, title_len + *ofs_p + len, *space_p);
+ strbuf_grow(sb, title.len + len);
if (subject) {
- len = strlen(subject);
- memcpy(*buf_p + *ofs_p, subject, len);
- *ofs_p += len;
- *ofs_p += add_rfc2047(*buf_p + *ofs_p,
- title, title_len, encoding);
+ strbuf_addstr(sb, subject);
+ add_rfc2047(sb, title.buf, title.len, encoding);
} else {
- memcpy(*buf_p + *ofs_p, title, title_len);
- *ofs_p += title_len;
+ strbuf_addbuf(sb, &title);
}
- (*buf_p)[(*ofs_p)++] = '\n';
+ strbuf_addch(sb, '\n');
+
if (plain_non_ascii) {
const char *header_fmt =
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=%s\n"
"Content-Transfer-Encoding: 8bit\n";
- *ofs_p += snprintf(*buf_p + *ofs_p,
- *space_p - *ofs_p,
- header_fmt, encoding);
+ strbuf_addf(sb, header_fmt, encoding);
}
if (after_subject) {
- len = strlen(after_subject);
- memcpy(*buf_p + *ofs_p, after_subject, len);
- *ofs_p += len;
+ strbuf_addstr(sb, after_subject);
}
- free(title);
if (fmt == CMIT_FMT_EMAIL) {
- ALLOC_GROW(*buf_p, *ofs_p + 20, *space_p);
- (*buf_p)[(*ofs_p)++] = '\n';
+ strbuf_addch(sb, '\n');
}
+ strbuf_release(&title);
}
static void pp_remainder(enum cmit_fmt fmt,
const char **msg_p,
- unsigned long *len_p,
- unsigned long *ofs_p,
- char **buf_p,
- unsigned long *space_p,
+ struct strbuf *sb,
int indent)
{
int first = 1;
for (;;) {
const char *line = *msg_p;
- int linelen = get_one_line(line, *len_p);
+ int linelen = get_one_line(line);
*msg_p += linelen;
- *len_p -= linelen;
if (!linelen)
break;
@@ -1135,36 +1073,30 @@ static void pp_remainder(enum cmit_fmt fmt,
}
first = 0;
- ALLOC_GROW(*buf_p, *ofs_p + linelen + indent + 20, *space_p);
+ strbuf_grow(sb, linelen + indent + 20);
if (indent) {
- memset(*buf_p + *ofs_p, ' ', indent);
- *ofs_p += indent;
+ memset(sb->buf + sb->len, ' ', indent);
+ strbuf_setlen(sb, sb->len + indent);
}
- memcpy(*buf_p + *ofs_p, line, linelen);
- *ofs_p += linelen;
- (*buf_p)[(*ofs_p)++] = '\n';
+ strbuf_add(sb, line, linelen);
+ strbuf_addch(sb, '\n');
}
}
-unsigned long pretty_print_commit(enum cmit_fmt fmt,
- const struct commit *commit,
- unsigned long len,
- char **buf_p, unsigned long *space_p,
- int abbrev, const char *subject,
- const char *after_subject,
+void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
+ struct strbuf *sb, int abbrev,
+ const char *subject, const char *after_subject,
enum date_mode dmode)
{
- unsigned long offset = 0;
unsigned long beginning_of_body;
int indent = 4;
const char *msg = commit->buffer;
int plain_non_ascii = 0;
char *reencoded;
const char *encoding;
- char *buf;
if (fmt == CMIT_FMT_USERFORMAT)
- return format_commit_message(commit, msg, buf_p, space_p);
+ return format_commit_message(commit, msg, sb);
encoding = (git_log_output_encoding
? git_log_output_encoding
@@ -1174,7 +1106,6 @@ unsigned long pretty_print_commit(enum cmit_fmt fmt,
reencoded = logmsg_reencode(commit, encoding);
if (reencoded) {
msg = reencoded;
- len = strlen(reencoded);
}
if (fmt == CMIT_FMT_ONELINE || fmt == CMIT_FMT_EMAIL)
@@ -1189,14 +1120,13 @@ unsigned long pretty_print_commit(enum cmit_fmt fmt,
if (fmt == CMIT_FMT_EMAIL && !after_subject) {
int i, ch, in_body;
- for (in_body = i = 0; (ch = msg[i]) && i < len; i++) {
+ for (in_body = i = 0; (ch = msg[i]); i++) {
if (!in_body) {
/* author could be non 7-bit ASCII but
* the log may be so; skip over the
* header part first.
*/
- if (ch == '\n' &&
- i + 1 < len && msg[i+1] == '\n')
+ if (ch == '\n' && msg[i+1] == '\n')
in_body = 1;
}
else if (non_ascii(ch)) {
@@ -1206,59 +1136,44 @@ unsigned long pretty_print_commit(enum cmit_fmt fmt,
}
}
- pp_header(fmt, abbrev, dmode, encoding,
- commit, &msg, &len,
- &offset, buf_p, space_p);
+ pp_header(fmt, abbrev, dmode, encoding, commit, &msg, sb);
if (fmt != CMIT_FMT_ONELINE && !subject) {
- ALLOC_GROW(*buf_p, offset + 20, *space_p);
- (*buf_p)[offset++] = '\n';
+ strbuf_addch(sb, '\n');
}
/* Skip excess blank lines at the beginning of body, if any... */
for (;;) {
- int linelen = get_one_line(msg, len);
+ int linelen = get_one_line(msg);
int ll = linelen;
if (!linelen)
break;
if (!is_empty_line(msg, &ll))
break;
msg += linelen;
- len -= linelen;
}
/* These formats treat the title line specially. */
- if (fmt == CMIT_FMT_ONELINE
- || fmt == CMIT_FMT_EMAIL)
- pp_title_line(fmt, &msg, &len, &offset,
- buf_p, space_p, indent,
- subject, after_subject, encoding,
- plain_non_ascii);
-
- beginning_of_body = offset;
- if (fmt != CMIT_FMT_ONELINE)
- pp_remainder(fmt, &msg, &len, &offset,
- buf_p, space_p, indent);
-
- while (offset && isspace((*buf_p)[offset-1]))
- offset--;
+ if (fmt == CMIT_FMT_ONELINE || fmt == CMIT_FMT_EMAIL)
+ pp_title_line(fmt, &msg, sb, subject,
+ after_subject, encoding, plain_non_ascii);
- ALLOC_GROW(*buf_p, offset + 20, *space_p);
- buf = *buf_p;
+ beginning_of_body = sb->len;
+ if (fmt != CMIT_FMT_ONELINE)
+ pp_remainder(fmt, &msg, sb, indent);
+ strbuf_rtrim(sb);
/* Make sure there is an EOLN for the non-oneline case */
if (fmt != CMIT_FMT_ONELINE)
- buf[offset++] = '\n';
+ strbuf_addch(sb, '\n');
/*
* The caller may append additional body text in e-mail
* format. Make sure we did not strip the blank line
* between the header and the body.
*/
- if (fmt == CMIT_FMT_EMAIL && offset <= beginning_of_body)
- buf[offset++] = '\n';
- buf[offset] = '\0';
+ if (fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)
+ strbuf_addch(sb, '\n');
free(reencoded);
- return offset;
}
struct commit *pop_commit(struct commit_list **stack)
@@ -1337,12 +1252,12 @@ void sort_in_topological_order_fn(struct commit_list ** list, int lifo,
next=next->next;
}
/*
- * find the tips
- *
- * tips are nodes not reachable from any other node in the list
- *
- * the tips serve as a starting set for the work queue.
- */
+ * find the tips
+ *
+ * tips are nodes not reachable from any other node in the list
+ *
+ * the tips serve as a starting set for the work queue.
+ */
next=*list;
insert = &work;
while (next) {
@@ -1369,9 +1284,9 @@ void sort_in_topological_order_fn(struct commit_list ** list, int lifo,
if (pn) {
/*
* parents are only enqueued for emission
- * when all their children have been emitted thereby
- * guaranteeing topological order.
- */
+ * when all their children have been emitted thereby
+ * guaranteeing topological order.
+ */
pn->indegree--;
if (!pn->indegree) {
if (!lifo)
@@ -1383,9 +1298,9 @@ void sort_in_topological_order_fn(struct commit_list ** list, int lifo,
parents=parents->next;
}
/*
- * work_item is a commit all of whose children
- * have already been emitted. we can emit it now.
- */
+ * work_item is a commit all of whose children
+ * have already been emitted. we can emit it now.
+ */
*pptr = work_node->list_item;
pptr = &(*pptr)->next;
*pptr = NULL;
@@ -1481,8 +1396,7 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)
}
struct commit_list *get_merge_bases(struct commit *one,
- struct commit *two,
- int cleanup)
+ struct commit *two, int cleanup)
{
struct commit_list *list;
struct commit **rslt;
diff --git a/commit.h b/commit.h
index 467872e..745f538 100644
--- a/commit.h
+++ b/commit.h
@@ -3,6 +3,7 @@
#include "object.h"
#include "tree.h"
+#include "strbuf.h"
#include "decorate.h"
struct commit_list {
@@ -61,7 +62,10 @@ enum cmit_fmt {
};
extern enum cmit_fmt get_commit_format(const char *arg);
-extern unsigned long pretty_print_commit(enum cmit_fmt fmt, const struct commit *, unsigned long len, char **buf_p, unsigned long *space_p, int abbrev, const char *subject, const char *after_subject, enum date_mode dmode);
+extern void pretty_print_commit(enum cmit_fmt fmt, const struct commit*,
+ struct strbuf *,
+ int abbrev, const char *subject,
+ const char *after_subject, enum date_mode);
/** Removes the first commit from a list sorted by date, and adds all
* of its parents.
diff --git a/log-tree.c b/log-tree.c
index a642371..5186939 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -79,25 +79,14 @@ static int detect_any_signoff(char *letter, int size)
return seen_head && seen_name;
}
-static unsigned long append_signoff(char **buf_p, unsigned long *buf_sz_p,
- unsigned long at, const char *signoff)
+static void append_signoff(struct strbuf *sb, const char *signoff)
{
static const char signed_off_by[] = "Signed-off-by: ";
size_t signoff_len = strlen(signoff);
int has_signoff = 0;
char *cp;
- char *buf;
- unsigned long buf_sz;
-
- buf = *buf_p;
- buf_sz = *buf_sz_p;
- if (buf_sz <= at + strlen(signed_off_by) + signoff_len + 3) {
- buf_sz += strlen(signed_off_by) + signoff_len + 3;
- buf = xrealloc(buf, buf_sz);
- *buf_p = buf;
- *buf_sz_p = buf_sz;
- }
- cp = buf;
+
+ cp = sb->buf;
/* First see if we already have the sign-off by the signer */
while ((cp = strstr(cp, signed_off_by))) {
@@ -105,29 +94,25 @@ static unsigned long append_signoff(char **buf_p, unsigned long *buf_sz_p,
has_signoff = 1;
cp += strlen(signed_off_by);
- if (cp + signoff_len >= buf + at)
+ if (cp + signoff_len >= sb->buf + sb->len)
break;
if (strncmp(cp, signoff, signoff_len))
continue;
if (!isspace(cp[signoff_len]))
continue;
/* we already have him */
- return at;
+ return;
}
if (!has_signoff)
- has_signoff = detect_any_signoff(buf, at);
+ has_signoff = detect_any_signoff(sb->buf, sb->len);
if (!has_signoff)
- buf[at++] = '\n';
-
- strcpy(buf + at, signed_off_by);
- at += strlen(signed_off_by);
- strcpy(buf + at, signoff);
- at += signoff_len;
- buf[at++] = '\n';
- buf[at] = 0;
- return at;
+ strbuf_addch(sb, '\n');
+
+ strbuf_addstr(sb, signed_off_by);
+ strbuf_add(sb, signoff, signoff_len);
+ strbuf_addch(sb, '\n');
}
static unsigned int digits_in_number(unsigned int number)
@@ -142,14 +127,12 @@ static unsigned int digits_in_number(unsigned int number)
void show_log(struct rev_info *opt, const char *sep)
{
- char *msgbuf = NULL;
- unsigned long msgbuf_len = 0;
+ struct strbuf msgbuf;
struct log_info *log = opt->loginfo;
struct commit *commit = log->commit, *parent = log->parent;
int abbrev = opt->diffopt.abbrev;
int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
const char *extra;
- int len;
const char *subject = NULL, *extra_headers = opt->extra_headers;
opt->loginfo = NULL;
@@ -288,18 +271,17 @@ void show_log(struct rev_info *opt, const char *sep)
/*
* And then the pretty-printed message itself
*/
- len = pretty_print_commit(opt->commit_format, commit, ~0u,
- &msgbuf, &msgbuf_len, abbrev, subject,
- extra_headers, opt->date_mode);
+ strbuf_init(&msgbuf);
+ pretty_print_commit(opt->commit_format, commit, &msgbuf,
+ abbrev, subject, extra_headers, opt->date_mode);
if (opt->add_signoff)
- len = append_signoff(&msgbuf, &msgbuf_len, len,
- opt->add_signoff);
+ append_signoff(&msgbuf, opt->add_signoff);
if (opt->show_log_size)
- printf("log size %i\n", len);
+ printf("log size %i\n", (int)msgbuf.len);
- printf("%s%s%s", msgbuf, extra, sep);
- free(msgbuf);
+ printf("%s%s%s", msgbuf.buf, extra, sep);
+ strbuf_release(&msgbuf);
}
int log_tree_diff_flush(struct rev_info *opt)
--
1.5.3.1
^ permalink raw reply related
* Re: [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.
From: David Kastrup @ 2007-09-08 11:59 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <11892523992038-git-send-email-madcoder@debian.org>
Pierre Habouzit <madcoder@debian.org> writes:
> Also remove the "len" parameter, as:
> (1) it was used as a max boundary, and every caller used ~0u
> (2) we check for final NUL no matter what, so it doesn't help for speed.
That sounds like a change that makes improvement of callers impossible
when it is found out that it leads to a performance issue. Is it only
the pretty-print that is affected?
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.
From: Pierre Habouzit @ 2007-09-08 12:17 UTC (permalink / raw)
To: David Kastrup; +Cc: git
In-Reply-To: <851wd9xt98.fsf@lola.goethe.zz>
[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]
On Sat, Sep 08, 2007 at 11:59:31AM +0000, David Kastrup wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > Also remove the "len" parameter, as:
> > (1) it was used as a max boundary, and every caller used ~0u
> > (2) we check for final NUL no matter what, so it doesn't help for speed.
>
> That sounds like a change that makes improvement of callers impossible
> when it is found out that it leads to a performance issue. Is it only
> the pretty-print that is affected?
I removed the "len" argument of pretty_print_commit and all the sub
pp_* functions it uses. This argument was supposed to tell which size
the commit message you want to read to format the pretty printing.
It leads to a lot of code that works like this:
if (position < len || *msg) /* test if len is overflowed or if we are
at the end of the string */
break;
This impose us to maintain the len of the message while we make the
"msg" pointer progress, and so on. And it's a limit of what to read in the
commit message.
Seeing where pretty_print_commit is used:
builtin-branch.c: pretty_print_commit(CMIT_FMT_ONELINE, commit,
builtin-log.c: pretty_print_commit(CMIT_FMT_ONELINE, commit,
builtin-rev-list.c: pretty_print_commit(revs.commit_format, commit,
builtin-show-branch.c: pretty_print_commit(CMIT_FMT_ONELINE, commit,
commit.c:void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
log-tree.c: pretty_print_commit(opt->commit_format, commit, &msgbuf,
I assume that the user would not be very pleased if we decide to crop
his commits logs when he uses git-log. And given that git log in the
linux repository on my laptop takes:
2,13s user 0,06s system 99% cpu 2,213 total
when the repo is hot ... I hardly think it can ever be a performance
issue :)
But if people disagree I can refactor a patch with the "len" argument
kept.
--
·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/3] Rework pretty_print_commit to use strbufs instead of custom buffers.
From: Pierre Habouzit @ 2007-09-08 12:28 UTC (permalink / raw)
To: David Kastrup; +Cc: git
In-Reply-To: <851wd9xt98.fsf@lola.goethe.zz>
[-- Attachment #1: Type: text/plain, Size: 2863 bytes --]
On sam, sep 08, 2007 at 11:59:31 +0000, David Kastrup wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > Also remove the "len" parameter, as:
> > (1) it was used as a max boundary, and every caller used ~0u
> > (2) we check for final NUL no matter what, so it doesn't help for speed.
>
> That sounds like a change that makes improvement of callers impossible
> when it is found out that it leads to a performance issue. Is it only
> the pretty-print that is affected?
Speaking of performance, here is a small interesting bench:
$ for i in `seq 1 10`; do time git-log >| /dev/null; done
git-log >| /dev/null 2,12s user 0,08s system 99% cpu 2,205 total
git-log >| /dev/null 2,14s user 0,06s system 99% cpu 2,208 total
git-log >| /dev/null 2,12s user 0,08s system 99% cpu 2,205 total
git-log >| /dev/null 2,10s user 0,10s system 99% cpu 2,209 total
git-log >| /dev/null 2,14s user 0,07s system 99% cpu 2,211 total
git-log >| /dev/null 2,11s user 0,09s system 99% cpu 2,210 total
git-log >| /dev/null 2,15s user 0,05s system 99% cpu 2,213 total
git-log >| /dev/null 2,12s user 0,07s system 99% cpu 2,203 total
git-log >| /dev/null 2,13s user 0,06s system 99% cpu 2,204 total
git-log >| /dev/null 2,17s user 0,09s system 100% cpu 2,254 total
$ for i in `seq 1 10`; do time ~/dev/scm/git/git-log >| /dev/null; done
~/dev/scm/git/git-log >| /dev/null 2,06s user 0,11s system 99% cpu 2,185 total
~/dev/scm/git/git-log >| /dev/null 2,11s user 0,07s system 99% cpu 2,192 total
~/dev/scm/git/git-log >| /dev/null 2,10s user 0,10s system 99% cpu 2,207 total
~/dev/scm/git/git-log >| /dev/null 2,10s user 0,08s system 99% cpu 2,188 total
~/dev/scm/git/git-log >| /dev/null 2,10s user 0,08s system 99% cpu 2,187 total
~/dev/scm/git/git-log >| /dev/null 2,10s user 0,10s system 100% cpu 2,196 total
~/dev/scm/git/git-log >| /dev/null 2,08s user 0,11s system 99% cpu 2,195 total
~/dev/scm/git/git-log >| /dev/null 2,11s user 0,08s system 99% cpu 2,193 total
~/dev/scm/git/git-log >| /dev/null 2,10s user 0,08s system 100% cpu 2,188 total
~/dev/scm/git/git-log >| /dev/null 2,12s user 0,07s system 98% cpu 2,213 total
$ git rev-list --all | wc -l
64271
The underling repository is a not so old linux-2.6 git repository.
git-log is the default unstable git-log, (a pristine 1.5.3.1,
pre-new-strbuf).
~/dev/scm/git/git-log is obviously the new one with the whole strbuf
patch series applied. Strbufs definitely made the code more readable (at
least it's my impression), here is the proof that it did not affected an
inch from performance (we even seem to have a marginal 0.5% gain in
performance ;p).
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* gitweb on MAC OSX 10.3 Panther
From: Brian Mc Enery @ 2007-09-08 14:05 UTC (permalink / raw)
To: git
Hi there,
I have just completed the installation of gitweb (git version 1.5.3.1)
on Mac OSX 10.3 Panther, and I thought that you may be interested in
including a note with your distribution, to enable an easier
experience for others.
Essentially the commands I used were variations on the following.
****** Start installation of gitweb on Mac OSX ******
make prefix="/usr/local" \
PERL_PATH="/usr/local/ActivePerl-5.8/bin/perl" \
all
sudo make prefix=/usr/local install
make PERL_PATH="/usr/local/ActivePerl-5.8/bin/perl" \
GITWEB_PROJECTROOT="/Library/WebServer/Documents/gitweb/rep" \
GITWEB_CSS="/gitweb/res/gitweb.css" \
GITWEB_LOGO="/gitweb/res/git-logo.png" \
GITWEB_FAVICON="/gitweb/res/git-favicon.png" \
GITWEB_SITENAME="Local Git" \
bindir=/usr/local/bin \
gitweb/gitweb.cgi
cp -fv ./gitweb/gitweb.cgi /Library/WebServer/CGI-Executables/gitweb
cp -fv ./gitweb/gitweb.css \
./gitweb/git-{favicon,logo}.png \
/Library/WebServer/Documents/gitweb/res
****** End installation of gitweb on Mac OSX ******
Notes:
Perl_Path was necessary in order to change the shebang line in any perl
or cgi scripts to use my non-standard
perl interpreter, i.e. different from usr/bin/perl.
The gitweb.cgi script and the configuration file gitweb_configure.perl
must be placed in a directory under /Library/WebServer/CGI-Executables,
as this is equivalent to localhost/cgi-bin. In this case I used
/Library/WebServer/CGI-Executables/gitweb.
The gitweb resources *must* be placed in a folder in
/Library/WebServer/Documents, not in
/Library/WebServer/CGI-Executables. In this case I placed the resources
in /Library/WebServer/Documents/gitweb/res, and used
/Library/WebServer/Documents/gitweb/rep, as the project root. The
absolute url for the project root must be specified.
In order to use symbolic links from a live project to the projectroot
it was not necessary to make any changes to the Apache configuration
file, however the files being linked to i.e. the project/.git files
*must*(it took forever to work this out), be in a publicly accessible
area, even if you are accessing using a browser under you own username.
In my case I placed those projects in the Public folder, under my own
username.
Relevant file permissions must of course be tweaked.
If all goes according to plan repositories are accessible from
http://localhost/cgi-bin/gitweb/gitweb.cgi
Thank you,
Brían
Brían Mach Aon Innéirghthe
Corcaigh
Éire
^ permalink raw reply
* how to export git repository?
From: Tamas K Papp @ 2007-09-08 14:55 UTC (permalink / raw)
To: git
Sorry, but I didn't find this in the documentation. How can I export
the contents of a repository (just the files, not the version control
information)?
I am looking for something similar to svn export.
Thanks,
Tamas
^ permalink raw reply
* Re: how to export git repository?
From: Jeff King @ 2007-09-08 15:08 UTC (permalink / raw)
To: Tamas K Papp; +Cc: git
In-Reply-To: <20070908145555.GA24132@pu100877.student.princeton.edu>
On Sat, Sep 08, 2007 at 04:55:55PM +0200, Tamas K Papp wrote:
> Sorry, but I didn't find this in the documentation. How can I export
> the contents of a repository (just the files, not the version control
> information)?
>
> I am looking for something similar to svn export.
Try "git-archive".
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] Add strbuf_rtrim (to remove trailing spaces).
From: René Scharfe @ 2007-09-08 16:18 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <1189252399433-git-send-email-madcoder@debian.org>
Pierre Habouzit schrieb:
> diff --git a/strbuf.c b/strbuf.c
> index acc7fc8..565c343 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -28,6 +28,13 @@ void strbuf_grow(struct strbuf *sb, size_t extra) {
> ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
> }
>
> +void strbuf_rtrim(struct strbuf *sb)
> +{
> + while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
> + sb->len--;
> + sb->buf[sb->len] = '\0';
> +}
Please use tabs instead of spaces to indent, just like you did in your
other patches. :)
René
^ permalink raw reply
* RE: git-svn 1.5.3 does not understand grafts?
From: Joakim Tjernlund @ 2007-09-08 17:43 UTC (permalink / raw)
To: 'Eric Wong'; +Cc: 'git'
In-Reply-To: <20070908050146.GA28855@soma>
> -----Original Message-----
> From: Eric Wong [mailto:normalperson@yhbt.net]
> Sent: den 8 september 2007 07:02
> To: Joakim Tjernlund
> Cc: git
> Subject: Re: git-svn 1.5.3 does not understand grafts?
>
> Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> > On Fri, 2007-09-07 at 18:52 +0200, Joakim Tjernlund wrote:
> > > On Fri, 2007-09-07 at 18:41 +0200, Joakim Tjernlund wrote:
> > > > svnadmin create /usr/local/src/TM/svn-tst/7720-svn/
> > > > svn mkdir
> file:///usr/local/src/TM/svn-tst/7720-svn/trunk -m "Add trunk dir"
> > > > svn mkdir
> file:///usr/local/src/TM/svn-tst/7720-svn/trunk/swp -m "Add swp dir"
> > > >
> > > > In my git repo I do
> > > > git-svn init
> file:///usr/local/src/TM/svn-tst/7720-svn/trunk/swp
> > > > git-svn fetch
> > > > git branch svn remotes/git-svn
> > > > #make remotes/git-svn parent to the initial commit in
> my git tree
> > > > graftid=`git-show-ref -s svn`
> > > > echo da783cce390ce013b19f1d308ea6813269c6a6b5 $graftid
> > .git/info/grafts
> > > > #da783... is the initial commit in my git tree.
> > > > git-svn dcommit
> > > >
> > > > fails with:
> > > > Committing to
> file:///usr/local/src/TM/svn-tst/7720-svn/trunk/swp ...
> > > > Commit da783cce390ce013b19f1d308ea6813269c6a6b5
> > > > has no parent commit, and therefore nothing to diff against.
> > > > You should be working from a repository originally
> created by git-svn
> > >
> > > Using filter-branch helps, but git-svn isn't too happy:
> > >
> > > git-svn init file:///usr/local/src/TM/svn-tst/7720-svn/trunk/swp
> > > git-svn fetch
> > > git branch svn remotes/git-svn
> > > #make remotes/git-svn parent to the initial commit in my git tree
> > > graftid=`git-show-ref -s svn`
> > > echo da783cce390ce013b19f1d308ea6813269c6a6b5 $graftid >
> .git/info/grafts
> > > #da783... is the initial commit in my git tree.
> > > git filter-branch $graftid..HEAD
> > > git-svn dcommit
> > >
> > > Now I get alot of complaints, but it commits to svn.
> > > It takes forever though:
> > > r3 = 55a489bd4f66dd1f641a4676359d7b8911dc7d83 (git-svn)
> > > W: HEAD and refs/remotes/git-svn differ, using rebase:
> > > :100644 100644 f85ae11af7715a224015582724cb2bab87ec914a
>
> I haven't used filter-branch myself, but you probably need to
> remove all
> .rev_db* files in $GIT_DIR after running it (git-svn can recreate them
> unless you use the svmRevProps or noMetadata options.
>
> > [SNIP]
> >
> > Just wanted to add that 1.5.2.2 works with grafts and
> > that I suspect sub read_commit_parents in git-svn, but as I don't
> > do perl I am stuck.
>
> Crap, it looks like I completely forgot about the existence
> of grafts while doing this function.
hmm, I think git-cat-file is to blame.
git-cat-file commit da783cce390ce013b19f1d308ea6813269c6a6b5 does
not list list any parent even though I did
graftid=`git-show-ref -s remotes/git-svn`
echo da783cce390ce013b19f1d308ea6813269c6a6b5 $graftid > .git/info/grafts
Jocke
^ permalink raw reply
* Re: [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.
From: René Scharfe @ 2007-09-08 18:40 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Git Mailing List
In-Reply-To: <11892523992038-git-send-email-madcoder@debian.org>
Pierre Habouzit schrieb:
> Also remove the "len" parameter, as:
> (1) it was used as a max boundary, and every caller used ~0u
> (2) we check for final NUL no matter what, so it doesn't help for speed.
>
> As a result most of the pp_* function takes 3 arguments less, and we need
> a lot less local variables, this makes the code way more readable, and
> easier to extend if needed.
>
> This patch also fixes some spacing and cosmetic issues.
>
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
> builtin-branch.c | 15 +--
> builtin-log.c | 12 +-
> builtin-rev-list.c | 13 +-
> builtin-show-branch.c | 13 +-
> commit.c | 330 ++++++++++++++++++-------------------------------
> commit.h | 6 +-
> log-tree.c | 56 +++------
> 7 files changed, 171 insertions(+), 274 deletions(-)
Nice! I wonder if we should #include strbuf.h from git-compat-util.h
(just like e.g. string.h) instead of from commit.h, in order to have
strbuf available everywhere in git.
Please be aware of the changes to commit.c already in next which your
patch conflicts with: format_commit_message() has been exported and is
used in builtin-archive.c there.
René
^ permalink raw reply
* Re: [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.
From: Pierre Habouzit @ 2007-09-08 18:49 UTC (permalink / raw)
To: René Scharfe; +Cc: Git Mailing List
In-Reply-To: <46E2EC88.6000500@lsrfire.ath.cx>
[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]
On Sat, Sep 08, 2007 at 06:40:08PM +0000, René Scharfe wrote:
> Pierre Habouzit schrieb:
> > Also remove the "len" parameter, as:
> > (1) it was used as a max boundary, and every caller used ~0u
> > (2) we check for final NUL no matter what, so it doesn't help for speed.
> >
> > As a result most of the pp_* function takes 3 arguments less, and we need
> > a lot less local variables, this makes the code way more readable, and
> > easier to extend if needed.
> >
> > This patch also fixes some spacing and cosmetic issues.
> >
> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > ---
> > builtin-branch.c | 15 +--
> > builtin-log.c | 12 +-
> > builtin-rev-list.c | 13 +-
> > builtin-show-branch.c | 13 +-
> > commit.c | 330 ++++++++++++++++++-------------------------------
> > commit.h | 6 +-
> > log-tree.c | 56 +++------
> > 7 files changed, 171 insertions(+), 274 deletions(-)
>
> Nice! I wonder if we should #include strbuf.h from git-compat-util.h
> (just like e.g. string.h) instead of from commit.h, in order to have
> strbuf available everywhere in git.
>
> Please be aware of the changes to commit.c already in next which your
> patch conflicts with: format_commit_message() has been exported and is
> used in builtin-archive.c there.
Okay, then I'll hope than Junio will merge the first series and then
rebase this one on next. If I'm going to propose more of those patches,
should I write them rather on next or master (like I do now) ?
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* [PATCH] diff-delta.c: Rationalize culling of hash buckets
From: David Kastrup @ 2007-09-08 19:41 UTC (permalink / raw)
To: git
In-Reply-To: <f1161c08aeeca60b6c33af34ccea68fd99b9ea4e.1189243702.git.dak@gnu.org>
The previous hash bucket culling resulted in a somewhat unpredictable
number of hash bucket entries in the order of magnitude of HASH_LIMIT.
Replace this with a Bresenham-like algorithm leaving us with exactly
HASH_LIMIT entries by uniform culling.
---
This is assuming that the previous patch series has been applied
(lacking any final comments or Acks on it; but I think I addressed the
issues). The hash bucket code culling has really annoyed me with its
unintuitive results. If the preceding patch series is thrown out (I
can't think why it would), one could rework this to fit the current
master if really needed.
diff-delta.c | 41 +++++++++++++++++++++++++++++++----------
1 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/diff-delta.c b/diff-delta.c
index e7c33aa..98795c6 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -207,19 +207,40 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
* the reference buffer.
*/
for (i = 0; i < hsize; i++) {
- if (hash_count[i] < HASH_LIMIT)
+ int acc;
+
+ if (hash_count[i] <= HASH_LIMIT)
continue;
+
+ entries -= hash_count[i] - HASH_LIMIT;
+ /* We leave exactly HASH_LIMIT entries in the bucket */
+
uentry = uhash[i];
+ acc = 0;
do {
- struct unpacked_index_entry *keep = uentry;
- int skip = hash_count[i] / HASH_LIMIT;
- do {
- --entries;
- uentry = uentry->next;
- } while(--skip && uentry);
- ++entries;
- keep->next = uentry;
- } while(uentry);
+ acc += hash_count[i] - HASH_LIMIT;
+ if (acc > 0) {
+ struct unpacked_index_entry *keep = uentry;
+ do {
+ uentry = uentry->next;
+ acc -= HASH_LIMIT;
+ } while (acc > 0);
+ keep->next = uentry->next;
+ }
+ uentry = uentry->next;
+ } while (uentry);
+
+ /* Assume that this loop is gone through exactly
+ * HASH_LIMIT times and is entered and left with
+ * acc==0. So the first statement in the loop
+ * contributes (hash_count[i]-HASH_LIMIT)*HASH_LIMIT
+ * to the accumulator, and the inner loop consequently
+ * is run (hash_count[i]-HASH_LIMIT) times, removing
+ * one element from the list each time. Since acc
+ * balances out to 0 at the final run, the inner loop
+ * body can't be left with uentry==NULL. So we indeed
+ * encounter uentry==NULL in the outer loop only.
+ */
}
free(hash_count);
--
1.5.3.GIT
^ permalink raw reply related
* Re: git-svn 1.5.3 does not understand grafts?
From: Junio C Hamano @ 2007-09-08 19:53 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: 'Eric Wong', 'git'
In-Reply-To: <045501c7f23f$c359c450$5267a8c0@Jocke>
"Joakim Tjernlund" <joakim.tjernlund@transmode.se> writes:
> hmm, I think git-cat-file is to blame.
> git-cat-file commit da783cce390ce013b19f1d308ea6813269c6a6b5 does
> not list list any parent...
The plumbing cat-file does not deal with grafts and this is
deliberate. Otherwise you would not be able to find the true
set of parents when you'd want to.
So do not blame cat-file, but blame the Porcelain that uses
cat-file to read a commit object, without annotating what it
read with what is in grafts, in this case your command line
experiment ;-).
The log family of commands and rev-list plumbing while
traversing commit ancestry chain do take grafts into account.
One caveat is pretty=raw output format shows true parents
without grafts on "parent " header line, while the "commit "
fake header prepended in the output for each commit shows the
parents that takes into account.
To illustrate, if you forge the history and say the parent of
1ddea77 is 5da1606 (when the true parent is 820eca68) with
grafts mechanism, here is what happens:
$ echo '1ddea77e449ef28d8a7c74521af21121ab01abc0 5da1606d0bf5b970fadfa0ca91618a1e871f6755' >.git/info/grafts
$ git show -s --pretty=raw --parents 1ddea77
commit 1ddea77e449ef28d8a7c74521af21121ab01abc0 5da1606d0bf5b970fadfa0ca91618a1e871f6755
tree e9e61bc801438062978ff47b0963c536ed1e51a9
parent 820eca68c2577d7499d203d7f4f7ae479b577683
author Nick Hengeveld <nickh@reactrix.com> 1127757131 -0700
committer Junio C Hamano <junkio@cox.net> 1127805558 -0700
[PATCH] Return CURL error message when object transfer fails
Return CURL error message when object transfer fails
...
$ git-cat-file commit 1ddea77
tree e9e61bc801438062978ff47b0963c536ed1e51a9
parent 820eca68c2577d7499d203d7f4f7ae479b577683
author Nick Hengeveld <nickh@reactrix.com> 1127757131 -0700
committer Junio C Hamano <junkio@cox.net> 1127805558 -0700
[PATCH] Return CURL error message when object transfer fails
Return CURL error message when object transfer fails
...
^ permalink raw reply
* Re: [PATCH] diff-delta.c: pack the index structure
From: David Kastrup @ 2007-09-08 20:50 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: git
In-Reply-To: <854pi5zh8q.fsf@lola.goethe.zz>
[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]
David Kastrup <dak@gnu.org> writes:
> Another option avoiding the realloc would be not to use linked lists
> at all but just collect all entries sequentually in "packed" form,
> and sort them into order afterwards. That's an O(n lg n) operation
> with a large n. Even if one has a sorting algorithm with good
> memory locality, I doubt that the locality would compensate for the
> lg n factor, even when taking into account that we save ourselves
> the copying. And that is not even taken the possibility of having
> to cull some buckets into account, another O(n) operation (which
> amounts to copying everything once, too).
As an illustration, I have implemented a proof-of-concept. This is a
patch (not formatted for submission) against the current master. It
takes about a 20% performance hit on the whole git-pack-objects run,
so the diffing code itself is likely affected worse. For the
proof-of-concept I did not do the hash bucket collision culling, nor
did I realloc to the possibly reduced size. So a fairer comparison
would likely involve removing the culling code from master (or
implementing the culling in the proof-of-concept, but I am too lazy
for that).
I hope this addresses the not-in-place complaint. It is conceivable
that under tight memory conditions, this approach might actually work
out better. However, the glibc qsort implementation (at least it did
so at one time) reverts to a mergesort for large numbers, and
allocates a temporary buffer of the same size as the original data.
If that is still the case, the memory impact would actually be worse
here. One would need to implement a custom _real_ in place sorter to
get around this. All in all, I don't think this approach is worth the
trouble: the sorting overhead factor of lg n is just too much.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 4158 bytes --]
diff --git a/diff-delta.c b/diff-delta.c
index 0dde2f2..f6e2416 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -115,7 +115,6 @@ static const unsigned int U[256] = {
struct index_entry {
const unsigned char *ptr;
unsigned int val;
- struct index_entry *next;
};
struct delta_index {
@@ -126,12 +125,24 @@ struct delta_index {
struct index_entry *hash[FLEX_ARRAY];
};
+static unsigned int cmp_hmask;
+
+static int cmp_index_entry(const void *va, const void *vb)
+{
+ const struct index_entry *a = va, *b = vb;
+ if ((a->val & cmp_hmask) != (b->val & cmp_hmask))
+ return (a->val & cmp_hmask) < (b->val & cmp_hmask) ? -1 : 1;
+ if (a->val != b->val)
+ return a->val < b->val ? -1 : 1;
+ return a->ptr < b->ptr ? -1 : 1;
+}
+
struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
{
- unsigned int i, hsize, hmask, entries, prev_val, *hash_count;
+ unsigned int i, hsize, hmask, entries, prev_val, ihash;
const unsigned char *data, *buffer = buf;
struct delta_index *index;
- struct index_entry *entry, **hash;
+ struct index_entry *entry, **hash, *entry_base;
void *mem;
unsigned long memsize;
@@ -149,7 +160,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
/* allocate lookup index */
memsize = sizeof(*index) +
- sizeof(*hash) * hsize +
+ sizeof(*hash) * (hsize + 1) +
sizeof(*entry) * entries;
mem = malloc(memsize);
if (!mem)
@@ -157,21 +168,14 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
index = mem;
mem = index + 1;
hash = mem;
- mem = hash + hsize;
- entry = mem;
+ mem = hash + hsize + 1;
+ entry_base = mem;
+ entry = entry_base;
index->memsize = memsize;
index->src_buf = buf;
index->src_size = bufsize;
index->hash_mask = hmask;
- memset(hash, 0, hsize * sizeof(*hash));
-
- /* allocate an array to count hash entries */
- hash_count = calloc(hsize, sizeof(*hash_count));
- if (!hash_count) {
- free(index);
- return NULL;
- }
/* then populate the index */
prev_val = ~0;
@@ -184,29 +188,33 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
if (val == prev_val) {
/* keep the lowest of consecutive identical blocks */
entry[-1].ptr = data + RABIN_WINDOW;
+ --entries;
} else {
prev_val = val;
i = val & hmask;
entry->ptr = data + RABIN_WINDOW;
entry->val = val;
- entry->next = hash[i];
- hash[i] = entry++;
- hash_count[i]++;
+ entry++;
}
}
+ cmp_hmask = hmask;
+ qsort(entry_base, entries, sizeof(struct index_entry), cmp_index_entry);
+ ihash = 0;
+
+ for (i=0; i<entries; i++) {
+ entry = &entry_base[i];
+ if ((entry->val & hmask) >= ihash) {
+ /* cull here */
+ do {
+ hash[ihash++] = entry;
+ } while ((entry->val & hmask) >= ihash);
+ }
+ }
+ while (ihash <= hsize) {
+ hash[ihash++] = &entry_base[entries];
+ }
- /*
- * Determine a limit on the number of entries in the same hash
- * bucket. This guards us against pathological data sets causing
- * really bad hash distribution with most entries in the same hash
- * bucket that would bring us to O(m*n) computing costs (m and n
- * corresponding to reference and target buffer sizes).
- *
- * Make sure none of the hash buckets has more entries than
- * we're willing to test. Otherwise we cull the entry list
- * uniformly to still preserve a good repartition across
- * the reference buffer.
- */
+#if 0
for (i = 0; i < hsize; i++) {
if (hash_count[i] < HASH_LIMIT)
continue;
@@ -221,6 +229,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
} while(entry);
}
free(hash_count);
+#endif
return index;
}
@@ -302,7 +311,7 @@ create_delta(const struct delta_index *index,
val ^= U[data[-RABIN_WINDOW]];
val = ((val << 8) | *data) ^ T[val >> RABIN_SHIFT];
i = val & index->hash_mask;
- for (entry = index->hash[i]; entry; entry = entry->next) {
+ for (entry = index->hash[i]; entry < index->hash[i+1]; entry++) {
const unsigned char *ref = entry->ptr;
const unsigned char *src = data;
unsigned int ref_size = ref_top - ref;
[-- Attachment #3: Type: text/plain, Size: 52 bytes --]
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply related
* Re: [PATCH] diff-delta.c: rename {a,}{entry,hash} to {,u}{entry,hash}
From: Nicolas Pitre @ 2007-09-08 20:52 UTC (permalink / raw)
To: David Kastrup; +Cc: git
In-Reply-To: <f1161c08aeeca60b6c33af34ccea68fd99b9ea4e.1189243702.git.dak@gnu.org>
On Sat, 8 Sep 2007, David Kastrup wrote:
> The variables for the packed entries are now called just entry and
> hash rather than aentry+ahash, and those for the unpacked entries have
> been renamed to uentry and uhash from the original entry and hash.
>
> While this makes the diff to the unchanged code larger, it matches the
> type declarations better.
Since the packed version is used less often, could you rename that one
instead, and use packed_entry and packed_hash forclarity.
Also please fold this with your other patch (not the culling one).
Then you can add my ACK.
Nicolas
^ permalink raw reply
* Re: [PATCH] diff-delta.c: rename {a,}{entry,hash} to {,u}{entry,hash}
From: David Kastrup @ 2007-09-08 21:06 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: git
In-Reply-To: <alpine.LFD.0.9999.0709081648281.21186@xanadu.home>
Nicolas Pitre <nico@cam.org> writes:
> On Sat, 8 Sep 2007, David Kastrup wrote:
>
>> The variables for the packed entries are now called just entry and
>> hash rather than aentry+ahash, and those for the unpacked entries have
>> been renamed to uentry and uhash from the original entry and hash.
>>
>> While this makes the diff to the unchanged code larger, it matches the
>> type declarations better.
>
> Since the packed version is used less often, could you rename that one
> instead, and use packed_entry and packed_hash forclarity.
>
> Also please fold this with your other patch (not the culling one).
Uh, what other patch? If I don't do the renames in the second patch,
then only the first patch remains. Which I'll change to use the
proposed names. Hope that's what you meant.
> Then you can add my ACK.
I'll do that, and rework the culling patch on top of that (unacked for
now).
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* [PATCH] diff-delta.c: pack the index structure
From: David Kastrup @ 2007-09-08 21:17 UTC (permalink / raw)
To: git
In normal use cases, the performance wins are not overly impressive:
we get something like 5-10% due to the slightly better locality of
memory accesses using the packed structure.
However, since the data structure for index entries saves 33% of
memory on 32-bit platforms and 40% on 64-bit platforms, the behavior
when memory gets limited should be nicer.
This is a rather well-contained change. One obvious improvement would
be sorting the elements in one bucket according to their hash, then
using binary probing to find the elements with the right hash value.
As it stands, the output should be strictly the same as previously
unless one uses the option for limiting the amount of used memory, in
which case the created packs might be better.
Signed-off-by: David Kastrup <dak@gnu.org>
Acked-by: Nicolas Pitre <nico@cam.org>
---
diff-delta.c | 74 +++++++++++++++++++++++++++++++++++++++++++++------------
1 files changed, 58 insertions(+), 16 deletions(-)
diff --git a/diff-delta.c b/diff-delta.c
index 0dde2f2..d355e5e 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -115,9 +115,13 @@ static const unsigned int U[256] = {
struct index_entry {
const unsigned char *ptr;
unsigned int val;
- struct index_entry *next;
};
+struct unpacked_index_entry {
+ struct index_entry entry;
+ struct unpacked_index_entry *next;
+};
+
struct delta_index {
unsigned long memsize;
const void *src_buf;
@@ -131,7 +135,8 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
unsigned int i, hsize, hmask, entries, prev_val, *hash_count;
const unsigned char *data, *buffer = buf;
struct delta_index *index;
- struct index_entry *entry, **hash;
+ struct unpacked_index_entry *entry, **hash;
+ struct index_entry *packed_entry, **packed_hash;
void *mem;
unsigned long memsize;
@@ -148,28 +153,21 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
hmask = hsize - 1;
/* allocate lookup index */
- memsize = sizeof(*index) +
- sizeof(*hash) * hsize +
+ memsize = sizeof(*hash) * hsize +
sizeof(*entry) * entries;
mem = malloc(memsize);
if (!mem)
return NULL;
- index = mem;
- mem = index + 1;
hash = mem;
mem = hash + hsize;
entry = mem;
- index->memsize = memsize;
- index->src_buf = buf;
- index->src_size = bufsize;
- index->hash_mask = hmask;
memset(hash, 0, hsize * sizeof(*hash));
/* allocate an array to count hash entries */
hash_count = calloc(hsize, sizeof(*hash_count));
if (!hash_count) {
- free(index);
+ free(hash);
return NULL;
}
@@ -183,12 +181,13 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
val = ((val << 8) | data[i]) ^ T[val >> RABIN_SHIFT];
if (val == prev_val) {
/* keep the lowest of consecutive identical blocks */
- entry[-1].ptr = data + RABIN_WINDOW;
+ entry[-1].entry.ptr = data + RABIN_WINDOW;
+ --entries;
} else {
prev_val = val;
i = val & hmask;
- entry->ptr = data + RABIN_WINDOW;
- entry->val = val;
+ entry->entry.ptr = data + RABIN_WINDOW;
+ entry->entry.val = val;
entry->next = hash[i];
hash[i] = entry++;
hash_count[i]++;
@@ -212,16 +211,59 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
continue;
entry = hash[i];
do {
- struct index_entry *keep = entry;
+ struct unpacked_index_entry *keep = entry;
int skip = hash_count[i] / HASH_LIMIT;
do {
+ --entries;
entry = entry->next;
} while(--skip && entry);
+ ++entries;
keep->next = entry;
} while(entry);
}
free(hash_count);
+ /* Now create the packed index in array form rather than
+ * linked lists */
+
+ memsize = sizeof(*index)
+ + sizeof(*packed_hash) * (hsize+1)
+ + sizeof(*packed_entry) * entries;
+
+ mem = malloc(memsize);
+
+ if (!mem) {
+ free(hash);
+ return NULL;
+ }
+
+ index = mem;
+ index->memsize = memsize;
+ index->src_buf = buf;
+ index->src_size = bufsize;
+ index->hash_mask = hmask;
+
+ mem = index + 1;
+ packed_hash = mem;
+ mem = packed_hash + (hsize+1);
+ packed_entry = mem;
+
+ /* Coalesce all entries belonging to one linked list into
+ * consecutive array entries */
+
+ for (i = 0; i < hsize; i++) {
+ packed_hash[i] = packed_entry;
+ for (entry = hash[i]; entry; entry = entry->next)
+ *packed_entry++ = entry->entry;
+ }
+
+ /* Sentinel value to indicate the length of the last hash
+ * bucket */
+
+ packed_hash[hsize] = packed_entry;
+ assert(packed_entry - (struct index_entry *)mem == entries);
+ free(hash);
+
return index;
}
@@ -302,7 +344,7 @@ create_delta(const struct delta_index *index,
val ^= U[data[-RABIN_WINDOW]];
val = ((val << 8) | *data) ^ T[val >> RABIN_SHIFT];
i = val & index->hash_mask;
- for (entry = index->hash[i]; entry; entry = entry->next) {
+ for (entry = index->hash[i]; entry < index->hash[i+1]; entry++) {
const unsigned char *ref = entry->ptr;
const unsigned char *src = data;
unsigned int ref_size = ref_top - ref;
--
1.5.3.GIT
^ permalink raw reply related
* [PATCH] diff-delta.c: Rationalize culling of hash buckets
From: David Kastrup @ 2007-09-08 21:25 UTC (permalink / raw)
To: git
In-Reply-To: <851wd8ua9z.fsf@lola.goethe.zz>
The previous hash bucket culling resulted in a somewhat unpredictable
number of hash bucket entries in the order of magnitude of HASH_LIMIT.
Replace this with a Bresenham-like algorithm leaving us with exactly
HASH_LIMIT entries by uniform culling.
Signed-off-by: David Kastrup <dak@gnu.org>
---
This is to be applied on top of the patch for packing the index
structure. Which is the reason it is posted as a followup to the
respective article. In contrast to that patch, this patch has only
been tested/reviewed by myself as to yet.
diff-delta.c | 41 +++++++++++++++++++++++++++++++----------
1 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/diff-delta.c b/diff-delta.c
index d355e5e..1a8d6fb 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -207,19 +207,40 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
* the reference buffer.
*/
for (i = 0; i < hsize; i++) {
- if (hash_count[i] < HASH_LIMIT)
+ int acc;
+
+ if (hash_count[i] <= HASH_LIMIT)
continue;
+
+ entries -= hash_count[i] - HASH_LIMIT;
+ /* We leave exactly HASH_LIMIT entries in the bucket */
+
entry = hash[i];
+ acc = 0;
do {
- struct unpacked_index_entry *keep = entry;
- int skip = hash_count[i] / HASH_LIMIT;
- do {
- --entries;
- entry = entry->next;
- } while(--skip && entry);
- ++entries;
- keep->next = entry;
- } while(entry);
+ acc += hash_count[i] - HASH_LIMIT;
+ if (acc > 0) {
+ struct unpacked_index_entry *keep = entry;
+ do {
+ entry = entry->next;
+ acc -= HASH_LIMIT;
+ } while (acc > 0);
+ keep->next = entry->next;
+ }
+ entry = entry->next;
+ } while (entry);
+
+ /* Assume that this loop is gone through exactly
+ * HASH_LIMIT times and is entered and left with
+ * acc==0. So the first statement in the loop
+ * contributes (hash_count[i]-HASH_LIMIT)*HASH_LIMIT
+ * to the accumulator, and the inner loop consequently
+ * is run (hash_count[i]-HASH_LIMIT) times, removing
+ * one element from the list each time. Since acc
+ * balances out to 0 at the final run, the inner loop
+ * body can't be left with entry==NULL. So we indeed
+ * encounter entry==NULL in the outer loop only.
+ */
}
free(hash_count);
--
1.5.3.GIT
^ permalink raw reply related
* Re: [PATCH] HEAD, ORIG_HEAD and FETCH_HEAD are really special.
From: Alex Riesen @ 2007-09-08 22:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: Pierre Habouzit, Johannes Sixt, Keith Packard, Git Mailing List
In-Reply-To: <7vabry43cg.fsf@gitster.siamese.dyndns.org>
Junio C Hamano, Fri, Sep 07, 2007 22:39:43 +0200:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > I'd have added though that maybe update-ref should print a warning for
> > the references that do not match the restriction Junio added. This could
> > be done using the function Junio proposed un update_ref() in refs.c
>
> I would even suggest making it into an error, even if we do not
> error out on the reading side (being liberal when reading but
> more strict when creating, that is).
I agree (and suggest failing even on reading), but see below
> That confused_ref() needs to be tightened further, by the way.
> It is called only when we are considering to tack the user
> string immediately below $GIT_DIR/ so the only valid cases are
> (1) the string begins with "refs/",
If that will be the case git-p4-import.bat (yes, just a script of
mine) will break because it has its namespace directly in $GIT_DIR
(i.e. .git/p4/*) and stores there backup references. It is just a
someones (ok, it is mine) script, but maybe there are others, who
expect that plumbing level git-update-ref just do what its told.
> or (2) the string is all uppercase (or underscore), especially
> without slash.
I'd suggest just check for uppercase+underscore _or_ slash. It is
plumbing after all.
^ permalink raw reply
* Re: [RFC] Convert builin-mailinfo.c to use The Better String Library.
From: Andreas Ericsson @ 2007-09-09 1:36 UTC (permalink / raw)
To: Pierre Habouzit, Andreas Ericsson, Walter Bright, git
In-Reply-To: <20070909003718.GE13385@artemis.corp>
Pierre Habouzit wrote:
> On Sat, Sep 08, 2007 at 11:50:34PM +0000, Andreas Ericsson wrote:
>
>>>> You can tell C compilers to
>>>> check all array accesses, but that is a performance issue.
>>> Runtime checking of arrays in D is a performance issue too, so it is
>>> selectable via a command line switch.
>> Same as in C then.
>
> HAHAHAHAHAHA. Please, who do you try to convince here ? Except in the
> local scope, there is few differences between a foo* and a foo[] in C.
>
"Runtime checking of arrays is a performance issue." It's true whether it's
done manually by the coder or by the compiler. The difference is that in C,
you get to choose where it should be done.
>>> But more importantly,
>>> 2) For dynamically sized arrays, the dimension of the array is carried
>>> with the array, so loops automatically loop the correct number of times.
>>> No runtime check is necessary, and it's easier for the code reviewer to
>>> visually check the code for correctness.
>> But this introduces handy but, strictly speaking, unnecessary overhead
>> as well, meaning, in short; 'D is slower than C, but easier to write
>> code in'.
>
> That's BS. See the strbuf API I've been pushing recently ? It has
> simplified git's code a lot, because each time git had to deal with a
> growing string, it had to deal with at least three variables: the buffer
> pointer, the current occupied length, and its allocated size. That was
> three thing to have variable names for, and to pass to functions.
>
Yup. I applaud your efforts, but it does come with a slight overhead,
except where it replaces faulty code. In practice, it's probably better
to use the api for all the string-handling, as none of it is performance-
critical.
> Now instead, it's just one struct. D gives that gratis. There is no
> performance loss because you _need_ to do the same. How do you deal with
> dynamic arrays if you dont't store their lenght and size somewhere ? Or
> are you the kind of programmer that write:
>
> /* 640kb should be enough for everyone… */
> some_type *array = malloc(640 << 10);
>
No, but it would depend on what I am to do with it.
>
>> So in essence, it's a bit like Python, but a teensy bit faster and a
>> lot easier to shoot yourself in the foot with.
>
>> What was the niche you were going for when you thought up D? It can't
>> have been systems programming, because *any* extra baggage is baggage
>> one would like to get rid of. If it was application programming I fail
>> to see how one more language would help, as there will be portability
>> problems galore and it's still considerably slower to develop in than
>> fe Python, while at the same time being considerably easier to mess up
>> in.
>
> Right now I'm just laughing. There is for sure overheads in some
> places of D, but the example you take, and what you try to attack in D
> is definitely not where you lose any kind of performance. You could have
> attacked the GC instead (which is after all an easy classical target).
>
I was asking what role D was designed to fill. I didn't mean it as an
attack, but re-reading what I wrote earlier I see it came off a bit harsh.
> Just to evaluate the silliness of your arguments:
> * http://www.digitalmars.com/d/comparison.html so that you can tell
> what the D features really are,
You may notice that the feature-list is being provided by the creators
and marketeers of the D language. Walter Bright certainly seems like a
nice enough person, but it's possible it's a tad biased.
> * http://shootout.alioth.debian.org/gp4/benchmark.php?test=all&lang=all
> so that you can know what the D performance really is about. Of
> course those are only micro benchmarks, but well, python is "just"
> 15 times slower than D, and D seems to be 10% slower.
I get it to 7.7xC and 1.2xC, respectively, but whatever. It still means
performance-critical apps will be written in C, while
insert-script-language-of-choice will still be used for prototyping and
not-so performance-critical apps.
> Well then I'm
> okay with D, I'm ready to buy 10% faster CPUs and avoid a lot of
> painful debugging time. In my world, 10% faster hardware is cheaper
> by many orders of magnitude than skilled programmers, but YMMV.
>
I'm curious as to how many fewer bugs D developers write compared to C
programmers. I guess it's hard to do a fair test given the comparatively
shallow pool of D gurus around, but it'd still be interesting to see a
practical test. 20% increase in runtime is certainly acceptable for
never having to see a bug again, but is it acceptable for 10% fewer bugs?
Or 20% fewer?
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply
* [PATCH] git-tag -s must fail if gpg cannot sign the tag.
From: Carlos Rica @ 2007-09-09 1:37 UTC (permalink / raw)
To: git, Junio C Hamano, Shawn O. Pearce, Johannes Schindelin
If the user has misconfigured `user.signingkey` in their .git/config
or just doesn't have any secret keys on their keyring and they ask
for a signed tag with `git tag -s` we better make sure the resulting
tag was actually signed by gpg.
Prior versions of builtin git-tag allowed this failure to slip
by without error as they were not checking the return value of
the finish_command() so they did not notice when gpg exited with
an error exit status. They also did not fail if gpg produced an
empty output or if read_in_full received an error from the read
system call while trying to read the pipe back from gpg.
Finally, we did not actually honor any return value from the do_sign
function as it returns ssize_t but was being stored into an unsigned
long. This caused the compiler to optimize out the die condition,
allowing git-tag to continue along and create the tag object.
However, when gpg gets a wrong username, it exits before any read was done
and then the writing process receives SIGNPIPE and program is terminated.
By ignoring this signal, anyway, the function write_or_die gets EPIPE from
write_in_full and exits returning 0 to the system without a message.
Here we better call to write_in_full directly so the next read can fail
printing a message and return safely to the caller.
With these issues fixed `git-tag -s` will now fail to create the
tag and will report a non-zero exit status to its caller, thereby
allowing automated helper scripts to detect (and recover from)
failure if gpg is not working properly.
Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
This patch was initially sent by Shawn in a previous email:
http://marc.info/?l=git&m=118905254711808&w=2
I made many tests trying to know the reason for the problem
and these changes are the result. I'm not sure how to send this
right, because the patch was almost entirely from Shawn and I
only changed a few things. Please, see if the new test now pass
for everybody. Comments about the change are also welcomed.
I always CC to my mentor, Johannes Sch., because that was
usual when the GSoC project was active, so I continue
doing it that way to make him able to track my contributions.
Tell me if that's no longer necessary or it'is anoying.
builtin-tag.c | 14 ++++++++++----
t/t7004-tag.sh | 7 +++++++
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/builtin-tag.c b/builtin-tag.c
index 348919c..c9be69a 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -200,6 +200,10 @@ static ssize_t do_sign(char *buffer, size_t size, size_t max)
bracket[1] = '\0';
}
+ /* When the username signingkey is bad, program could be terminated
+ * because gpg exits without reading and then write gets SIGNPIPE. */
+ signal(SIGPIPE, SIG_IGN);
+
memset(&gpg, 0, sizeof(gpg));
gpg.argv = args;
gpg.in = -1;
@@ -212,12 +216,13 @@ static ssize_t do_sign(char *buffer, size_t size, size_t max)
if (start_command(&gpg))
return error("could not run gpg.");
- write_or_die(gpg.in, buffer, size);
+ write_in_full(gpg.in, buffer, size);
close(gpg.in);
gpg.close_in = 0;
len = read_in_full(gpg.out, buffer + size, max - size);
- finish_command(&gpg);
+ if (finish_command(&gpg) || !len || len < 0)
+ return error("gpg failed to sign the tag");
if (len == max - size)
return error("could not read the entire signature from gpg.");
@@ -310,9 +315,10 @@ static void create_tag(const unsigned char *object, const char *tag,
size += header_len;
if (sign) {
- size = do_sign(buffer, size, max_size);
- if (size < 0)
+ ssize_t r = do_sign(buffer, size, max_size);
+ if (r < 0)
die("unable to sign the tag");
+ size = r;
}
if (write_sha1_file(buffer, size, tag_type, result) < 0)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 606d4f2..0d07bc3 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -990,6 +990,13 @@ test_expect_success \
git diff expect actual
'
+# try to sign with bad user.signingkey
+git config user.signingkey BobTheMouse
+test_expect_failure \
+ 'git-tag -s fails if gpg is misconfigured' \
+ 'git tag -s -m tail tag-gpg-failure'
+git config --unset user.signingkey
+
# try to verify without gpg:
rm -rf gpghome
--
1.5.0
^ permalink raw reply related
* Re: [RFC] Convert builin-mailinfo.c to use The Better String Library.
From: Pierre Habouzit @ 2007-09-09 0:37 UTC (permalink / raw)
To: Andreas Ericsson; +Cc: Walter Bright, git
In-Reply-To: <46E3354A.7030407@op5.se>
[-- Attachment #1: Type: text/plain, Size: 4510 bytes --]
On Sat, Sep 08, 2007 at 11:50:34PM +0000, Andreas Ericsson wrote:
> Walter Bright wrote:
> > David Kastrup wrote:
> > > Again, C won't keep you from shooting yourself in the foot.
> > Right, it won't. A good systems language should do what it can to
> > prevent the programmer from *inadvertently* shooting himself in the
> > foot, while allowing him to *deliberately* shoot himself in the foot.
>
> No, a good systems language should do exactly what it's told.
> Supporting tools should tell the programmer if he's risking shooting
> himself in the foot.
I beg to differ. I mean, knowing enough of D, I think that what Walter
tries to say is that a good language should provide constructions that
when used prevent the programmer to shoot himself in both foot at the
same time.
D supports most of the C constructions, so when you want to juggle
with razor blades, you're free to do so in D. Though, the language
provides idioms that prevent you to write stupid mistakes when used. And
that is great.
D is not Java, you have pointers, you can deal with memory
explicitely, you can do whatever you can do in C with no or very little
overhead. Or you can use higher level D, at your own discretion.
> > > You can tell C compilers to
> > > check all array accesses, but that is a performance issue.
> > Runtime checking of arrays in D is a performance issue too, so it is
> > selectable via a command line switch.
>
> Same as in C then.
HAHAHAHAHAHA. Please, who do you try to convince here ? Except in the
local scope, there is few differences between a foo* and a foo[] in C.
> > But more importantly,
> > 2) For dynamically sized arrays, the dimension of the array is carried
> > with the array, so loops automatically loop the correct number of times.
> > No runtime check is necessary, and it's easier for the code reviewer to
> > visually check the code for correctness.
>
> But this introduces handy but, strictly speaking, unnecessary overhead
> as well, meaning, in short; 'D is slower than C, but easier to write
> code in'.
That's BS. See the strbuf API I've been pushing recently ? It has
simplified git's code a lot, because each time git had to deal with a
growing string, it had to deal with at least three variables: the buffer
pointer, the current occupied length, and its allocated size. That was
three thing to have variable names for, and to pass to functions.
Now instead, it's just one struct. D gives that gratis. There is no
performance loss because you _need_ to do the same. How do you deal with
dynamic arrays if you dont't store their lenght and size somewhere ? Or
are you the kind of programmer that write:
/* 640kb should be enough for everyone… */
some_type *array = malloc(640 << 10);
> So in essence, it's a bit like Python, but a teensy bit faster and a
> lot easier to shoot yourself in the foot with.
> What was the niche you were going for when you thought up D? It can't
> have been systems programming, because *any* extra baggage is baggage
> one would like to get rid of. If it was application programming I fail
> to see how one more language would help, as there will be portability
> problems galore and it's still considerably slower to develop in than
> fe Python, while at the same time being considerably easier to mess up
> in.
Right now I'm just laughing. There is for sure overheads in some
places of D, but the example you take, and what you try to attack in D
is definitely not where you lose any kind of performance. You could have
attacked the GC instead (which is after all an easy classical target).
Just to evaluate the silliness of your arguments:
* http://www.digitalmars.com/d/comparison.html so that you can tell
what the D features really are,
* http://shootout.alioth.debian.org/gp4/benchmark.php?test=all&lang=all
so that you can know what the D performance really is about. Of
course those are only micro benchmarks, but well, python is "just"
15 times slower than D, and D seems to be 10% slower. Well then I'm
okay with D, I'm ready to buy 10% faster CPUs and avoid a lot of
painful debugging time. In my world, 10% faster hardware is cheaper
by many orders of magnitude than skilled programmers, but YMMV.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox