* [PATCH 10/12] pretty: support padding placeholders, %< %> and %><
From: Nguyễn Thái Ngọc Duy @ 2013-03-16 2:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1363400683-14813-1-git-send-email-pclouds@gmail.com>
Either %<, %> or %<> standing before a placeholder specifies how many
columns (at least as the placeholder can exceed it) it takes. Each
differs on how spaces are padded:
%< pads on the right (aka left alignment)
%> pads on the left (aka right alignment)
%>< pads both ways equally (aka centered)
The (<N>) follows them, e.g. `%<(100)', to specify the number of
columns the next placeholder takes.
However, if '|' stands before (<N>), e.g. `%>|(100)', then the number
of columns is calculated so that it reaches the Nth column on screen.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/pretty-formats.txt | 8 +++
pretty.c | 117 ++++++++++++++++++++++++++++++++++++++-
2 files changed, 124 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 8734224..87ca2c4 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -162,6 +162,14 @@ The placeholders are:
- '%x00': print a byte from a hex code
- '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of
linkgit:git-shortlog[1].
+- '%<(<N>)': make the next placeholder take at least N columns,
+ padding spaces on the right if necessary
+- '%<|(<N>)': make the next placeholder take at least until Nth
+ columns, padding spaces on the right if necessary
+- '%>(<N>)', '%>|(<N>)': similar to '%<(<N<)', '%<|(<N<)'
+ respectively, but padding spaces on the left
+- '%><(<N>)', '%><|(<N>)': similar to '%<(<N<)', '%<|(<N<)'
+ respectively, but padding both sides (i.e. the text is centered)
NOTE: Some placeholders may depend on other options given to the
revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index c333fd6..233d69c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -760,12 +760,20 @@ struct chunk {
size_t len;
};
+enum flush_type {
+ no_flush,
+ flush_right,
+ flush_left,
+ flush_both
+};
+
struct format_commit_context {
const struct commit *commit;
const struct pretty_print_context *pretty_ctx;
unsigned commit_header_parsed:1;
unsigned commit_message_parsed:1;
unsigned commit_signature_parsed:1;
+ enum flush_type flush_type;
struct {
char *gpg_output;
char good_bad;
@@ -775,6 +783,7 @@ struct format_commit_context {
char *commit_encoding;
size_t width, indent1, indent2;
int auto_color;
+ int padding;
/* These offsets are relative to the start of the commit message. */
struct chunk author;
@@ -1004,6 +1013,52 @@ static int format_reflog_person(struct strbuf *sb,
return format_person_part(sb, part, ident, strlen(ident), dmode);
}
+static size_t parse_padding_placeholder(struct strbuf *sb,
+ const char *placeholder,
+ struct format_commit_context *c)
+{
+ const char *ch = placeholder;
+ enum flush_type flush_type;
+ int to_column = 0;
+
+ switch (*ch++) {
+ case '<':
+ flush_type = flush_right;
+ break;
+ case '>':
+ if (*ch == '<') {
+ flush_type = flush_both;
+ ch++;
+ } else
+ flush_type = flush_left;
+ break;
+ default:
+ return 0;
+ }
+
+ /* the next value means "wide enough to that column" */
+ if (*ch == '|') {
+ to_column = 1;
+ ch++;
+ }
+
+ if (*ch == '(') {
+ const char *start = ch + 1;
+ const char *end = strchr(start, ')');
+ char *next;
+ int width;
+ if (!end || end == start)
+ return 0;
+ width = strtoul(start, &next, 10);
+ if (next == start || width == 0)
+ return 0;
+ c->padding = to_column ? -width : width;
+ c->flush_type = flush_type;
+ return end - placeholder + 1;
+ }
+ return 0;
+}
+
static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
void *context)
@@ -1090,6 +1145,10 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
return end - placeholder + 1;
} else
return 0;
+
+ case '<':
+ case '>':
+ return parse_padding_placeholder(sb, placeholder, c);
}
/* these depend on the commit */
@@ -1247,6 +1306,59 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
return 0; /* unknown placeholder */
}
+static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
+ const char *placeholder,
+ struct format_commit_context *c)
+{
+ struct strbuf local_sb = STRBUF_INIT;
+ int total_consumed = 0, len, padding = c->padding;
+ if (padding < 0) {
+ const char *start = strrchr(sb->buf, '\n');
+ int occupied;
+ if (!start)
+ start = sb->buf;
+ occupied = utf8_strnwidth(start, -1, 1);
+ padding = (-padding) - occupied;
+ }
+ while (1) {
+ int modifier = *placeholder == 'C';
+ int consumed = format_commit_one(&local_sb, placeholder, c);
+ total_consumed += consumed;
+
+ if (!modifier)
+ break;
+
+ placeholder += consumed;
+ if (*placeholder != '%')
+ break;
+ placeholder++;
+ total_consumed++;
+ }
+ len = utf8_strnwidth(local_sb.buf, -1, 1);
+ if (len > padding)
+ strbuf_addstr(sb, local_sb.buf);
+ else {
+ int sb_len = sb->len, offset = 0;
+ if (c->flush_type == flush_left)
+ offset = padding - len;
+ else if (c->flush_type == flush_both)
+ offset = (padding - len) / 2;
+ /*
+ * we calculate padding in columns, now
+ * convert it back to chars
+ */
+ padding = padding - len + local_sb.len;
+ strbuf_grow(sb, padding);
+ strbuf_setlen(sb, sb_len + padding);
+ memset(sb->buf + sb_len, ' ', sb->len - sb_len);
+ memcpy(sb->buf + sb_len + offset, local_sb.buf,
+ local_sb.len);
+ }
+ strbuf_release(&local_sb);
+ c->flush_type = no_flush;
+ return total_consumed;
+}
+
static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
void *context)
@@ -1277,7 +1389,10 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
placeholder++;
orig_len = sb->len;
- consumed = format_commit_one(sb, placeholder, context);
+ if (((struct format_commit_context *)context)->flush_type != no_flush)
+ consumed = format_and_pad_commit(sb, placeholder, context);
+ else
+ consumed = format_commit_one(sb, placeholder, context);
if (magic == NO_MAGIC)
return consumed;
--
1.8.2.83.gc99314b
^ permalink raw reply related
* [PATCH 11/12] pretty: support truncating in %>, %< and %><
From: Nguyễn Thái Ngọc Duy @ 2013-03-16 2:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1363400683-14813-1-git-send-email-pclouds@gmail.com>
%>(N,trunc) truncates the righ part after N columns and replace the
last two letters with "..". ltrunc does the same on the left. mtrunc
cuts the middle out.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/pretty-formats.txt | 6 +++--
pretty.c | 51 +++++++++++++++++++++++++++++++++++++---
utf8.c | 46 ++++++++++++++++++++++++++++++++++++
utf8.h | 2 ++
4 files changed, 100 insertions(+), 5 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 87ca2c4..17f82f4 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -162,8 +162,10 @@ The placeholders are:
- '%x00': print a byte from a hex code
- '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of
linkgit:git-shortlog[1].
-- '%<(<N>)': make the next placeholder take at least N columns,
- padding spaces on the right if necessary
+- '%<(<N>[,trunc|ltrunc|mtrunc])': make the next placeholder take at
+ least N columns, padding spaces on the right if necessary.
+ Optionally truncate at the beginning (ltrunc), the middle (mtrunc)
+ or the end (trunc) if the output is longer than N columns.
- '%<|(<N>)': make the next placeholder take at least until Nth
columns, padding spaces on the right if necessary
- '%>(<N>)', '%>|(<N>)': similar to '%<(<N<)', '%<|(<N<)'
diff --git a/pretty.c b/pretty.c
index 233d69c..29384b5 100644
--- a/pretty.c
+++ b/pretty.c
@@ -767,6 +767,13 @@ enum flush_type {
flush_both
};
+enum trunc_type {
+ trunc_none,
+ trunc_left,
+ trunc_middle,
+ trunc_right
+};
+
struct format_commit_context {
const struct commit *commit;
const struct pretty_print_context *pretty_ctx;
@@ -774,6 +781,7 @@ struct format_commit_context {
unsigned commit_message_parsed:1;
unsigned commit_signature_parsed:1;
enum flush_type flush_type;
+ enum trunc_type truncate;
struct {
char *gpg_output;
char good_bad;
@@ -1044,7 +1052,7 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
if (*ch == '(') {
const char *start = ch + 1;
- const char *end = strchr(start, ')');
+ const char *end = start + strcspn(start, ",)");
char *next;
int width;
if (!end || end == start)
@@ -1054,6 +1062,23 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
return 0;
c->padding = to_column ? -width : width;
c->flush_type = flush_type;
+
+ if (*end == ',') {
+ start = end + 1;
+ end = strchr(start, ')');
+ if (!end || end == start)
+ return 0;
+ if (!prefixcmp(start, "trunc)"))
+ c->truncate = trunc_right;
+ else if (!prefixcmp(start, "ltrunc)"))
+ c->truncate = trunc_left;
+ else if (!prefixcmp(start, "mtrunc)"))
+ c->truncate = trunc_middle;
+ else
+ return 0;
+ } else
+ c->truncate = trunc_none;
+
return end - placeholder + 1;
}
return 0;
@@ -1335,9 +1360,29 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
total_consumed++;
}
len = utf8_strnwidth(local_sb.buf, -1, 1);
- if (len > padding)
+ if (len > padding) {
+ switch (c->truncate) {
+ case trunc_left:
+ strbuf_utf8_replace(&local_sb,
+ 0, len - (padding - 2),
+ "..");
+ break;
+ case trunc_middle:
+ strbuf_utf8_replace(&local_sb,
+ padding / 2 - 1,
+ len - (padding - 2),
+ "..");
+ break;
+ case trunc_right:
+ strbuf_utf8_replace(&local_sb,
+ padding - 2, len - (padding - 2),
+ "..");
+ break;
+ case trunc_none:
+ break;
+ }
strbuf_addstr(sb, local_sb.buf);
- else {
+ } else {
int sb_len = sb->len, offset = 0;
if (c->flush_type == flush_left)
offset = padding - len;
diff --git a/utf8.c b/utf8.c
index 9d98043..766df80 100644
--- a/utf8.c
+++ b/utf8.c
@@ -421,6 +421,52 @@ void strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len,
free(tmp);
}
+void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width,
+ const char *subst)
+{
+ struct strbuf sb_dst = STRBUF_INIT;
+ char *src = sb_src->buf;
+ char *end = src + sb_src->len;
+ char *dst;
+ int w = 0, subst_len = 0;
+
+ if (subst)
+ subst_len = strlen(subst);
+ strbuf_grow(&sb_dst, sb_src->len + subst_len);
+ dst = sb_dst.buf;
+
+ while (src < end) {
+ char *old;
+ size_t n;
+
+ while ((n = display_mode_esc_sequence_len(src))) {
+ memcpy(dst, src, n);
+ src += n;
+ dst += n;
+ }
+
+ old = src;
+ n = utf8_width((const char**)&src, NULL);
+ if (!src) /* broken utf-8, do nothing */
+ return;
+ if (n && w >= pos && w < pos + width) {
+ if (subst) {
+ memcpy(dst, subst, subst_len);
+ dst += subst_len;
+ subst = NULL;
+ }
+ w += n;
+ continue;
+ }
+ memcpy(dst, old, src - old);
+ dst += src - old;
+ w += n;
+ }
+ strbuf_setlen(&sb_dst, dst - sb_dst.buf);
+ strbuf_attach(sb_src, strbuf_detach(&sb_dst, NULL),
+ sb_dst.len, sb_dst.alloc);
+}
+
int is_encoding_utf8(const char *name)
{
if (!name)
diff --git a/utf8.h b/utf8.h
index 99db3e0..faf2f91 100644
--- a/utf8.h
+++ b/utf8.h
@@ -15,6 +15,8 @@ void strbuf_add_wrapped_text(struct strbuf *buf,
const char *text, int indent, int indent2, int width);
void strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len,
int indent, int indent2, int width);
+void strbuf_utf8_replace(struct strbuf *sb, int pos, int width,
+ const char *subst);
#ifndef NO_ICONV
char *reencode_string_iconv(const char *in, size_t insz,
--
1.8.2.83.gc99314b
^ permalink raw reply related
* [PATCH 12/12] pretty: support %>> that steal trailing spaces
From: Nguyễn Thái Ngọc Duy @ 2013-03-16 2:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1363400683-14813-1-git-send-email-pclouds@gmail.com>
This is pretty useful in `%<(100)%s%Cred%>(20)% an' where %s does not
use up all 100 columns and %an needs more than 20 columns. By
replacing %>(20) with %>>(20), %an can steal spaces from %s.
%>> understands escape sequences, so %Cred does not stop it from
stealing spaces in %<(100).
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/pretty-formats.txt | 5 ++++-
pretty.c | 34 ++++++++++++++++++++++++++++++++++
utf8.c | 2 +-
utf8.h | 1 +
4 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 17f82f4..036c14a 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -170,7 +170,10 @@ The placeholders are:
columns, padding spaces on the right if necessary
- '%>(<N>)', '%>|(<N>)': similar to '%<(<N<)', '%<|(<N<)'
respectively, but padding spaces on the left
-- '%><(<N>)', '%><|(<N>)': similar to '%<(<N<)', '%<|(<N<)'
+- '%>>(<N>)', '%>>|(<N>)': similar to '%>(<N<)', '%>|(<N<)'
+ respectively, except that if the next placeholder takes more spaces
+ than given and there are spaces on its left, use those spaces
+- '%><(<N>)', '%><|(<N>)': similar to '% <(<N<)', '%<|(<N<)'
respectively, but padding both sides (i.e. the text is centered)
NOTE: Some placeholders may depend on other options given to the
diff --git a/pretty.c b/pretty.c
index 29384b5..892274d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -764,6 +764,7 @@ enum flush_type {
no_flush,
flush_right,
flush_left,
+ flush_left_and_steal,
flush_both
};
@@ -1037,6 +1038,9 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
if (*ch == '<') {
flush_type = flush_both;
ch++;
+ } else if (*ch == '>') {
+ flush_type = flush_left_and_steal;
+ ch++;
} else
flush_type = flush_left;
break;
@@ -1360,6 +1364,36 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
total_consumed++;
}
len = utf8_strnwidth(local_sb.buf, -1, 1);
+
+ if (c->flush_type == flush_left_and_steal) {
+ const char *ch = sb->buf + sb->len - 1;
+ while (len > padding && ch > sb->buf) {
+ const char *p;
+ if (*ch == ' ') {
+ ch--;
+ padding++;
+ continue;
+ }
+ /* check for trailing ansi sequences */
+ if (*ch != 'm')
+ break;
+ p = ch - 1;
+ while (ch - p < 10 && *p != '\033')
+ p--;
+ if (*p != '\033' ||
+ ch + 1 - p != display_mode_esc_sequence_len(p))
+ break;
+ /*
+ * got a good ansi sequence, put it back to
+ * local_sb as we're cutting sb
+ */
+ strbuf_insert(&local_sb, 0, p, ch + 1 - p);
+ ch = p - 1;
+ }
+ strbuf_setlen(sb, ch + 1 - sb->buf);
+ c->flush_type = flush_left;
+ }
+
if (len > padding) {
switch (c->truncate) {
case trunc_left:
diff --git a/utf8.c b/utf8.c
index 766df80..414ae49 100644
--- a/utf8.c
+++ b/utf8.c
@@ -9,7 +9,7 @@ struct interval {
int last;
};
-static size_t display_mode_esc_sequence_len(const char *s)
+size_t display_mode_esc_sequence_len(const char *s)
{
const char *p = s;
if (*p++ != '\033')
diff --git a/utf8.h b/utf8.h
index faf2f91..e913edb 100644
--- a/utf8.h
+++ b/utf8.h
@@ -3,6 +3,7 @@
typedef unsigned int ucs_char_t; /* assuming 32bit int */
+size_t display_mode_esc_sequence_len(const char *s);
int utf8_width(const char **start, size_t *remainder_p);
int utf8_strnwidth(const char *string, int len, int skip_ansi);
int utf8_strwidth(const char *string);
--
1.8.2.83.gc99314b
^ permalink raw reply related
* [PATCH 05/12] pretty: save commit encoding from logmsg_reencode if the caller needs it
From: Nguyễn Thái Ngọc Duy @ 2013-03-16 2:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1363400683-14813-1-git-send-email-pclouds@gmail.com>
The commit encoding is parsed by logmsg_reencode, there's no need for
the caller to re-parse it again. The reencoded message now have the
new encoding, not the original one. The caller would need to read
commit object again before parsing.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/blame.c | 2 +-
builtin/commit.c | 2 +-
commit.h | 1 +
pretty.c | 16 ++++++++++++----
revision.c | 2 +-
5 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 86100e9..104a948 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1425,7 +1425,7 @@ static void get_commit_info(struct commit *commit,
commit_info_init(ret);
encoding = get_log_output_encoding();
- message = logmsg_reencode(commit, encoding);
+ message = logmsg_reencode(commit, NULL, encoding);
get_ac_line(message, "\nauthor ",
&ret->author, &ret->author_mail,
&ret->author_time, &ret->author_tz);
diff --git a/builtin/commit.c b/builtin/commit.c
index 3348aa1..beead44 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -953,7 +953,7 @@ static const char *read_commit_message(const char *name)
if (!commit)
die(_("could not lookup commit %s"), name);
out_enc = get_commit_output_encoding();
- return logmsg_reencode(commit, out_enc);
+ return logmsg_reencode(commit, NULL, out_enc);
}
static int parse_and_validate_options(int argc, const char *argv[],
diff --git a/commit.h b/commit.h
index 4138bb4..085349a 100644
--- a/commit.h
+++ b/commit.h
@@ -100,6 +100,7 @@ struct userformat_want {
extern int has_non_ascii(const char *text);
struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
extern char *logmsg_reencode(const struct commit *commit,
+ char **commit_encoding,
const char *output_encoding);
extern void logmsg_free(char *msg, const struct commit *commit);
extern void get_commit_format(const char *arg, struct rev_info *);
diff --git a/pretty.c b/pretty.c
index 79784be..ab5d70f 100644
--- a/pretty.c
+++ b/pretty.c
@@ -584,6 +584,7 @@ static char *replace_encoding_header(char *buf, const char *encoding)
}
char *logmsg_reencode(const struct commit *commit,
+ char **commit_encoding,
const char *output_encoding)
{
static const char *utf8 = "UTF-8";
@@ -605,9 +606,15 @@ char *logmsg_reencode(const struct commit *commit,
sha1_to_hex(commit->object.sha1), typename(type));
}
- if (!output_encoding || !*output_encoding)
+ if (!output_encoding || !*output_encoding) {
+ if (commit_encoding)
+ *commit_encoding =
+ get_header(commit, msg, "encoding");
return msg;
+ }
encoding = get_header(commit, msg, "encoding");
+ if (commit_encoding)
+ *commit_encoding = encoding;
use_encoding = encoding ? encoding : utf8;
if (same_encoding(use_encoding, output_encoding)) {
/*
@@ -648,7 +655,8 @@ char *logmsg_reencode(const struct commit *commit,
if (out)
out = replace_encoding_header(out, output_encoding);
- free(encoding);
+ if (!commit_encoding)
+ free(encoding);
/*
* If the re-encoding failed, out might be NULL here; in that
* case we just return the commit message verbatim.
@@ -1313,7 +1321,7 @@ void format_commit_message(const struct commit *commit,
context.commit = commit;
context.pretty_ctx = pretty_ctx;
context.wrap_start = sb->len;
- context.message = logmsg_reencode(commit, output_enc);
+ context.message = logmsg_reencode(commit, NULL, output_enc);
strbuf_expand(sb, format, format_commit_item, &context);
rewrap_message_tail(sb, &context, 0, 0, 0);
@@ -1476,7 +1484,7 @@ void pretty_print_commit(const struct pretty_print_context *pp,
}
encoding = get_log_output_encoding();
- msg = reencoded = logmsg_reencode(commit, encoding);
+ msg = reencoded = logmsg_reencode(commit, NULL, encoding);
if (pp->fmt == CMIT_FMT_ONELINE || pp->fmt == CMIT_FMT_EMAIL)
indent = 0;
diff --git a/revision.c b/revision.c
index ef60205..c6ff560 100644
--- a/revision.c
+++ b/revision.c
@@ -2290,7 +2290,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
* in it.
*/
encoding = get_log_output_encoding();
- message = logmsg_reencode(commit, encoding);
+ message = logmsg_reencode(commit, NULL, encoding);
/* Copy the commit to temporary if we are using "fake" headers */
if (buf.len)
--
1.8.2.83.gc99314b
^ permalink raw reply related
* [PATCH 08/12] pretty: two phase conversion for non utf-8 commits
From: Nguyễn Thái Ngọc Duy @ 2013-03-16 2:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1363400683-14813-1-git-send-email-pclouds@gmail.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 4644 bytes --]
Always assume format_commit_item() takes an utf-8 string for string
handling simplicity (we can handle utf-8 strings, but can't with other
encodings).
If commit message is in non-utf8, or output encoding is not, then the
commit is first converted to utf-8, processed, then output converted
to output encoding. This of course only works with encodings that are
compatible with Unicode.
This also fixes the iso8859-1 test in t6006. It's supposed to create
an iso8859-1 commit, but the commit content in t6006 is in UTF-8.
t6006 is now converted back in UTF-8 (the downside is we can't put
utf-8 strings there anymore).
Signed-off-by: Nguyá»…n Thái Ngá»c Duy <pclouds@gmail.com>
---
pretty.c | 24 ++++++++++++++++++++++--
t/t6006-rev-list-format.sh | 12 ++++++------
2 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/pretty.c b/pretty.c
index 092dd1d..3f4809a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1003,7 +1003,8 @@ static int format_reflog_person(struct strbuf *sb,
return format_person_part(sb, part, ident, strlen(ident), dmode);
}
-static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
+static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
+ const char *placeholder,
void *context)
{
struct format_commit_context *c = context;
@@ -1235,7 +1236,8 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
return 0; /* unknown placeholder */
}
-static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
+static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
+ const char *placeholder,
void *context)
{
int consumed;
@@ -1315,6 +1317,7 @@ void format_commit_message(const struct commit *commit,
{
struct format_commit_context context;
const char *output_enc = pretty_ctx->output_encoding;
+ const char *utf8 = "UTF-8";
memset(&context, 0, sizeof(context));
context.commit = commit;
@@ -1327,6 +1330,23 @@ void format_commit_message(const struct commit *commit,
strbuf_expand(sb, format, format_commit_item, &context);
rewrap_message_tail(sb, &context, 0, 0, 0);
+ if (output_enc) {
+ if (same_encoding(utf8, output_enc))
+ output_enc = NULL;
+ } else {
+ if (context.commit_encoding &&
+ !same_encoding(context.commit_encoding, utf8))
+ output_enc = context.commit_encoding;
+ }
+
+ if (output_enc) {
+ int outsz;
+ char *out = reencode_string(sb->buf, sb->len,
+ output_enc, utf8, &outsz);
+ if (out)
+ strbuf_attach(sb, out, outsz, outsz + 1);
+ }
+
free(context.commit_encoding);
logmsg_free(context.message, commit);
free(context.signature.gpg_output);
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 3fc3b74..0393c9f 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -184,7 +184,7 @@ Test printing of complex bodies
This commit message is much longer than the others,
and it will be encoded in iso8859-1. We should therefore
-include an iso8859 character: ¡bueno!
+include an iso8859 character: ¡bueno!
EOF
test_expect_success 'setup complex body' '
git config i18n.commitencoding iso8859-1 &&
@@ -192,14 +192,14 @@ git config i18n.commitencoding iso8859-1 &&
'
test_format complex-encoding %e <<'EOF'
-commit f58db70b055c5718631e5c61528b28b12090cdea
+commit 1ed88da4a5b5ed8c449114ac131efc62178734c3
iso8859-1
commit 131a310eb913d107dd3c09a65d1651175898735d
commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
EOF
test_format complex-subject %s <<'EOF'
-commit f58db70b055c5718631e5c61528b28b12090cdea
+commit 1ed88da4a5b5ed8c449114ac131efc62178734c3
Test printing of complex bodies
commit 131a310eb913d107dd3c09a65d1651175898735d
changed foo
@@ -208,17 +208,17 @@ added foo
EOF
test_format complex-body %b <<'EOF'
-commit f58db70b055c5718631e5c61528b28b12090cdea
+commit 1ed88da4a5b5ed8c449114ac131efc62178734c3
This commit message is much longer than the others,
and it will be encoded in iso8859-1. We should therefore
-include an iso8859 character: ¡bueno!
+include an iso8859 character: ¡bueno!
commit 131a310eb913d107dd3c09a65d1651175898735d
commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
EOF
test_expect_success '%x00 shows NUL' '
- echo >expect commit f58db70b055c5718631e5c61528b28b12090cdea &&
+ echo >expect commit 1ed88da4a5b5ed8c449114ac131efc62178734c3 &&
echo >>expect fooQbar &&
git rev-list -1 --format=foo%x00bar HEAD >actual.nul &&
nul_to_q <actual.nul >actual &&
--
1.8.2.83.gc99314b
^ permalink raw reply related
* [PATCH 07/12] utf8: keep NULs in reencode_string()
From: Nguyễn Thái Ngọc Duy @ 2013-03-16 2:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1363400683-14813-1-git-send-email-pclouds@gmail.com>
reencode_string() will be used in the next patch for re-encoding
pretty output, which can contain NULs.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/fast-export.c | 3 ++-
builtin/mailinfo.c | 3 ++-
compat/precompose_utf8.c | 2 +-
notes.c | 4 +++-
pretty.c | 3 ++-
sequencer.c | 5 +++--
utf8.c | 10 +++++++---
utf8.h | 10 +++++++---
8 files changed, 27 insertions(+), 13 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 77dffd1..7ba9f3b 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -316,7 +316,8 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)
mark_next_object(&commit->object);
if (!is_encoding_utf8(encoding))
- reencoded = reencode_string(message, "UTF-8", encoding);
+ reencoded = reencode_string(message, strlen(message),
+ "UTF-8", encoding, NULL);
if (!commit->parents)
printf("reset %s\n", (const char*)commit->util);
printf("commit %s\nmark :%"PRIu32"\n%.*s\n%.*s\ndata %u\n%s",
diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index 24a772d..129e7dc 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -486,7 +486,8 @@ static void convert_to_utf8(struct strbuf *line, const char *charset)
if (same_encoding(metainfo_charset, charset))
return;
- out = reencode_string(line->buf, metainfo_charset, charset);
+ out = reencode_string(line->buf, line->len,
+ metainfo_charset, charset, NULL);
if (!out)
die("cannot convert from %s to %s",
charset, metainfo_charset);
diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index 8cf5955..d9203d0 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -78,7 +78,7 @@ void precompose_argv(int argc, const char **argv)
size_t namelen;
oldarg = argv[i];
if (has_non_ascii(oldarg, (size_t)-1, &namelen)) {
- newarg = reencode_string_iconv(oldarg, namelen, ic_precompose);
+ newarg = reencode_string_iconv(oldarg, namelen, ic_precompose, NULL);
if (newarg)
argv[i] = newarg;
}
diff --git a/notes.c b/notes.c
index f63fd57..4ae3b25 100644
--- a/notes.c
+++ b/notes.c
@@ -1222,7 +1222,9 @@ static void format_note(struct notes_tree *t, const unsigned char *object_sha1,
if (output_encoding && *output_encoding &&
!is_encoding_utf8(output_encoding)) {
- char *reencoded = reencode_string(msg, output_encoding, utf8);
+ char *reencoded = reencode_string(msg, strlen(msg),
+ output_encoding, utf8,
+ NULL);
if (reencoded) {
free(msg);
msg = reencoded;
diff --git a/pretty.c b/pretty.c
index e2241e5..092dd1d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -643,7 +643,8 @@ char *logmsg_reencode(const struct commit *commit,
* this point, we are done with msg. If we allocated a fresh
* copy, we can free it.
*/
- out = reencode_string(msg, output_encoding, use_encoding);
+ out = reencode_string(msg, strlen(msg),
+ output_encoding, use_encoding, NULL);
if (out && msg != commit->buffer)
free(msg);
}
diff --git a/sequencer.c b/sequencer.c
index aef5e8a..bf15531 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -61,8 +61,9 @@ static int get_message(struct commit *commit, struct commit_message *out)
out->reencoded_message = NULL;
out->message = commit->buffer;
if (same_encoding(encoding, git_commit_encoding))
- out->reencoded_message = reencode_string(commit->buffer,
- git_commit_encoding, encoding);
+ out->reencoded_message =
+ reencode_string(commit->buffer, strlen(commit->buffer),
+ git_commit_encoding, encoding, NULL);
if (out->reencoded_message)
out->message = out->reencoded_message;
diff --git a/utf8.c b/utf8.c
index 38322a1..9d98043 100644
--- a/utf8.c
+++ b/utf8.c
@@ -468,7 +468,7 @@ int utf8_fprintf(FILE *stream, const char *format, ...)
#else
typedef char * iconv_ibp;
#endif
-char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv)
+char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv, int *outsz_p)
{
size_t outsz, outalloc;
char *out, *outpos;
@@ -502,13 +502,17 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv)
}
else {
*outpos = '\0';
+ if (outsz_p)
+ *outsz_p = outpos - out;
break;
}
}
return out;
}
-char *reencode_string(const char *in, const char *out_encoding, const char *in_encoding)
+char *reencode_string(const char *in, int insz,
+ const char *out_encoding, const char *in_encoding,
+ int *outsz)
{
iconv_t conv;
char *out;
@@ -518,7 +522,7 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e
conv = iconv_open(out_encoding, in_encoding);
if (conv == (iconv_t) -1)
return NULL;
- out = reencode_string_iconv(in, strlen(in), conv);
+ out = reencode_string_iconv(in, insz, conv, outsz);
iconv_close(conv);
return out;
}
diff --git a/utf8.h b/utf8.h
index a556932..99db3e0 100644
--- a/utf8.h
+++ b/utf8.h
@@ -17,10 +17,14 @@ void strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len,
int indent, int indent2, int width);
#ifndef NO_ICONV
-char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv);
-char *reencode_string(const char *in, const char *out_encoding, const char *in_encoding);
+char *reencode_string_iconv(const char *in, size_t insz,
+ iconv_t conv, int *outsz);
+char *reencode_string(const char *in, int insz,
+ const char *out_encoding,
+ const char *in_encoding,
+ int *outsz);
#else
-#define reencode_string(a,b,c) NULL
+#define reencode_string(a,b,c,d) NULL
#endif
#endif
--
1.8.2.83.gc99314b
^ permalink raw reply related
* [PATCH 00/12] Layout control placeholders for pretty format
From: Nguyễn Thái Ngọc Duy @ 2013-03-16 2:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
This series was ejected out of pu some time ago due to gcc warnings. I
did not have time to look at it (and it worked ok for me so it was not
a pressing matter). This re-roll should fix that.
For demonstration, try
git log --pretty='format:%C(auto)%h %<(80,trunc)%s%>>(10,ltrunc)%C(auto)%d%>(15,mtrunc)% an'
Nguyễn Thái Ngọc Duy (12):
pretty-formats.txt: wrap long lines
pretty: share code between format_decoration and show_decorations
utf8.c: move display_mode_esc_sequence_len() for use by other functions
utf8.c: add utf8_strnwidth() with the ability to skip ansi sequences
pretty: save commit encoding from logmsg_reencode if the caller needs it
pretty: get the correct encoding for --pretty:format=%e
utf8: keep NULs in reencode_string()
pretty: two phase conversion for non utf-8 commits
pretty: add %C(auto) for auto-coloring on the next placeholder
pretty: support padding placeholders, %< %> and %><
pretty: support truncating in %>, %< and %><
pretty: support %>> that steal trailing spaces
Documentation/pretty-formats.txt | 34 ++++-
builtin/blame.c | 2 +-
builtin/commit.c | 2 +-
builtin/fast-export.c | 3 +-
builtin/mailinfo.c | 3 +-
commit.h | 1 +
compat/precompose_utf8.c | 2 +-
log-tree.c | 60 +++++----
log-tree.h | 3 +
notes.c | 4 +-
pretty.c | 282 ++++++++++++++++++++++++++++++++++-----
revision.c | 2 +-
sequencer.c | 5 +-
t/t4207-log-decoration-colors.sh | 8 +-
t/t6006-rev-list-format.sh | 12 +-
utf8.c | 104 +++++++++++----
utf8.h | 14 +-
17 files changed, 434 insertions(+), 107 deletions(-)
--
1.8.2.83.gc99314b
^ permalink raw reply
* Re: [PATCH] status: hint the user about -uno if read_directory takes too long
From: Torsten Bögershausen @ 2013-03-16 7:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: Torsten Bögershausen, Duy Nguyen, git, artagnon,
robert.allan.zeh, finnag
In-Reply-To: <7vzjy4tjd5.fsf@alter.siamese.dyndns.org>
On 15.03.13 22:59, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> Thanks, that looks good to me:
>>
>> # It took 2.58 seconds to enumerate untracked files.
>> # Consider the -u option for a possible speed-up?
>>
>> But:
>> If I follow the advice as is given and use "git status -u", the result is the same.
>
> Yeah, that was taken from
>
> http://thread.gmane.org/gmane.comp.version-control.git/215820/focus=218125
>
> to which I said something about "more levels of indirections". This
> episode shows that even a user who was very well aware of the issue
> did not follow a single level of indirection.
>
>> If I think loud, would it be better to say:
>>
>> # It took 2.58 seconds to search for untracked files.
>> # Consider the -uno option for a possible speed-up?
>>
>> or
>>
>> # It took 2.58 seconds to search for untracked files.
>> # Consider the -u option for a possible speed-up?
>> # Please see git help status
>
> The former actively hurts the users, but the latter would be good,
> given that your documentation updates clarifies the trade off.
>
> Or we can be more explicit and say
>
> # It took 2.58 seconds to search for untracked files. 'status -uno'
> # may speed it up, but you have to be careful not to forget to add
> # new files yourself (see 'git help status').
>
Thanks, that looks good for me
^ permalink raw reply
* Re: Tag peeling peculiarities
From: Michael Haggerty @ 2013-03-16 8:48 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git discussion list
In-Reply-To: <20130314134032.GA9222@sigill.intra.peff.net>
On 03/14/2013 02:40 PM, Jeff King wrote:
> Hmph. I coincidentally ran across another problem with 435c833 today.
> Try this:
> [...]
>
> But that's somewhat off-topic for this discussion. I'll look into it
> further and try to make a patch later today or tomorrow.
>
> On Thu, Mar 14, 2013 at 12:28:53PM +0100, Michael Haggerty wrote:
>> Your patch looks about right aside from its lack of comments :-). I was
>> going to implement approximately the same thing in a patch series that I
>> am working on, but if you prefer then go ahead and submit your patch and
>> I will rebase my branch on top of it.
>
> I just meant it as a quick sketch, since you said you were working in
> the area. I'm not sure what you are working on. If we don't consider it
> an urgent bugfix, I'm just as happy for you to incorporate the idea into
> what you are doing.
>
> But if we want to do it as a maint-track fix, I can clean up my patch.
> Junio?
My patch series is nearly done. I will need another day or two to
review and make it submission-ready, but I wanted to give you an idea of
what I'm up to and I could also use your feedback on some points.
The (preliminary) patch series is available on GitHub:
https://github.com/mhagger/git/commits/pack-refs-unification
Highlights:
* Move pack-refs code into refs.c
* Implement fully-peeled packed-refs files, as discussed upthread: peel
references outside of refs/tags, and keep rigorous track of
which references have REF_KNOWS_PEELED.
* Change how references are iterated over internally (without changing
the exported API):
* Provide direct access to the ref_entries
* Don't set current_ref if not iterating over all refs
* Change pack-refs to use the peeled information from ref_entries if it
is available, rather than looking up the references again.
* repack_without_ref() writes peeled refs to the packed-refs file.
Use the old values if available; otherwise look them up. (The old
version of repack_without_refs() wrote a packed-refs file without
any peeled values even if they were present in the old packed-refs
file.) Remove the deleted reference from the in-RAM packed ref cache
in-place instead of discarding the whole cache.
* Add some tests and lots of comments.
Here are my questions for you:
1. There are multiple functions for peeling refs:
peel_ref() (and peel_object(), which was extracted from it);
peel_entry() (derived from pack-refs.c:pack_one_ref()). It would be
nice to combine these into the One True Function. But given the
problem that you mentioned above (which is rooted in parts of the
code that I don't know) I don't know what that version should do.
2. repack_without_ref() now writes peeled refs, peeling them if
necessary. It does this *without* referring to the loose refs
to avoid having to load all of the loose references, which can be
time-consuming. But this means that it might try to lookup SHA1s
that are not the current value of the corresponding reference any
more, and might even have been garbage collected. Is the code that
I use to do this, in peel_entry(), safe to call for nonexistent
SHA1s (I would like it to return 0 if the SHA1 is invalid)?:
o = parse_object(entry->u.value.sha1);
if (o->type == OBJ_TAG) {
o = deref_tag(o, entry->name, 0);
if (o) {
hashcpy(entry->u.value.peeled, o->sha1);
entry->flag |= REF_KNOWS_PEELED;
return 1;
}
}
return 0;
I've tried to test this experimentally and it seems to work, but I
would like to have some theoretical support :-)
3. This same change to repack_with_ref() means that it could become
significantly slower to delete a packed ref if the packed-ref file
is not fully-peeled. On the plus side, once this is done once, the
packed-refs files will be rewritten in fully-peeled form. But if
two versions of Git are being used in a repository, this cost could
be incurred repeatedly. Does anybody object?
4. Should I change the locking policy as discussed in this thread?:
http://article.gmane.org/gmane.comp.version-control.git/212131
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* [PATCH 0/2] peel-ref optimization fixes
From: Jeff King @ 2013-03-16 9:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty
These patches fix the issue with peel-ref noticed recently by Michael
(namely that we fail to correctly peel packed refs outside of
refs/tags). The problem has been there since we added peeling support
to pack-refs, but traditionally "show-ref -d" was the only caller that
actually triggered the issue. Between that and the fact that most people
only put annotated tags into refs/tags, nobody really noticed.
Since my 435c833 (upload-pack: use peel_ref for ref advertisements,
2012-10-04), which is in v1.8.1, upload-pack can trigger the problem,
too, but I haven't actually heard of any reports in the wild.
I split it into two patches; the first one is the minimal fix that makes
git work properly going forward. The second one helps git be more robust
when reading packed-refs files generated by older git (or other
implementations). The second one depends semantically but not textually
on the first one; if you're worried about that, they can be squashed.
[1/2]: pack-refs: write peeled entry for non-tags
[2/2]: pack-refs: add fully-peeled trait
I think Michael may be rewriting some of this code, but if we are going
to go this direction with the fix (and I think we should), this is the
change that should go to maint in the meantime.
-Peff
^ permalink raw reply
* [PATCH 1/2] pack-refs: write peeled entry for non-tags
From: Jeff King @ 2013-03-16 9:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty
In-Reply-To: <20130316090018.GA26708@sigill.intra.peff.net>
When we pack an annotated tag ref, we write not only the
sha1 of the tag object along with the ref, but also the sha1
obtained by peeling the tag. This lets readers of the
pack-refs file know the peeled value without having to
actually load the object, speeding up upload-pack's ref
advertisement.
The writer marks a packed-refs file with peeled refs using
the "peeled" trait at the top of the file. When the reader
sees this trait, it knows that each ref is either followed
by its peeled value, or it is not an annotated tag.
However, there is a mismatch between the assumptions of the
reader and writer. The writer will only peel refs under
refs/tags, but the reader does not know this; it will assume
a ref without a peeled value must not be a tag object. Thus
an annotated tag object placed outside of the refs/tags
hierarchy will not have its peeled value printed by
upload-pack.
The simplest way to fix this is to start writing peel values
for all refs. This matches what the reader expects for both
new and old versions of git.
Signed-off-by: Jeff King <peff@peff.net>
---
pack-refs.c | 16 ++++++++--------
t/t3211-peel-ref.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+), 8 deletions(-)
create mode 100755 t/t3211-peel-ref.sh
diff --git a/pack-refs.c b/pack-refs.c
index f09a054..10832d7 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -27,6 +27,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
int flags, void *cb_data)
{
struct pack_refs_cb_data *cb = cb_data;
+ struct object *o;
int is_tag_ref;
/* Do not pack the symbolic refs */
@@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
return 0;
fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(sha1), path);
- if (is_tag_ref) {
- struct object *o = parse_object(sha1);
- if (o->type == OBJ_TAG) {
- o = deref_tag(o, path, 0);
- if (o)
- fprintf(cb->refs_file, "^%s\n",
- sha1_to_hex(o->sha1));
- }
+
+ o = parse_object(sha1);
+ if (o->type == OBJ_TAG) {
+ o = deref_tag(o, path, 0);
+ if (o)
+ fprintf(cb->refs_file, "^%s\n",
+ sha1_to_hex(o->sha1));
}
if ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {
diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
new file mode 100755
index 0000000..dd5b48b
--- /dev/null
+++ b/t/t3211-peel-ref.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='tests for the peel_ref optimization of packed-refs'
+. ./test-lib.sh
+
+test_expect_success 'create annotated tag in refs/tags' '
+ test_commit base &&
+ git tag -m annotated foo
+'
+
+test_expect_success 'create annotated tag outside of refs/tags' '
+ git update-ref refs/outside/foo refs/tags/foo
+'
+
+# This matches show-ref's output
+print_ref() {
+ echo "`git rev-parse "$1"` $1"
+}
+
+test_expect_success 'set up expected show-ref output' '
+ {
+ print_ref "refs/heads/master" &&
+ print_ref "refs/outside/foo" &&
+ print_ref "refs/outside/foo^{}" &&
+ print_ref "refs/tags/base" &&
+ print_ref "refs/tags/foo" &&
+ print_ref "refs/tags/foo^{}"
+ } >expect
+'
+
+test_expect_success 'refs are peeled outside of refs/tags (loose)' '
+ git show-ref -d >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'refs are peeled outside of refs/tags (packed)' '
+ git pack-refs --all &&
+ git show-ref -d >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
1.8.2.rc2.7.gef06216
^ permalink raw reply related
* [PATCH 2/2] pack-refs: add fully-peeled trait
From: Jeff King @ 2013-03-16 9:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty
In-Reply-To: <20130316090018.GA26708@sigill.intra.peff.net>
Older versions of pack-refs did not write peel lines for
refs outside of refs/tags. This meant that on reading the
pack-refs file, we might set the REF_KNOWS_PEELED flag for
such a ref, even though we do not know anything about its
peeled value.
The previous commit updated the writer to always peel, no
matter what the ref is. That means that packed-refs files
written by newer versions of git are fine to be read by both
old and new versions of git. However, we still have the
problem of reading packed-refs files written by older
versions of git, or by other implementations which have not
yet learned the same trick.
The simplest fix would be to always unset the
REF_KNOWS_PEELED flag for refs outside of refs/tags that do
not have a peel line (if it has a peel line, we know it is
valid, but we cannot assume a missing peel line means
anything). But that loses an important optimization, as
upload-pack should not need to load the object pointed to by
refs/heads/foo to determine that it is not a tag.
Instead, we add a "fully-peeled" trait to the packed-refs
file. If it is set, we know that we can trust a missing peel
line to mean that a ref cannot be peeled. Otherwise, we fall
back to assuming nothing.
Signed-off-by: Jeff King <peff@peff.net>
---
pack-refs.c | 2 +-
refs.c | 16 +++++++++++++++-
t/t3211-peel-ref.sh | 22 ++++++++++++++++++++++
3 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/pack-refs.c b/pack-refs.c
index 10832d7..87ca04d 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -128,7 +128,7 @@ int pack_refs(unsigned int flags)
die_errno("unable to create ref-pack file structure");
/* perhaps other traits later as well */
- fprintf(cbdata.refs_file, "# pack-refs with: peeled \n");
+ fprintf(cbdata.refs_file, "# pack-refs with: peeled fully-peeled \n");
for_each_ref(handle_one_ref, &cbdata);
if (ferror(cbdata.refs_file))
diff --git a/refs.c b/refs.c
index 175b9fc..6a38c41 100644
--- a/refs.c
+++ b/refs.c
@@ -808,6 +808,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
struct ref_entry *last = NULL;
char refline[PATH_MAX];
int flag = REF_ISPACKED;
+ int fully_peeled = 0;
while (fgets(refline, sizeof(refline), f)) {
unsigned char sha1[20];
@@ -818,13 +819,26 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
const char *traits = refline + sizeof(header) - 1;
if (strstr(traits, " peeled "))
flag |= REF_KNOWS_PEELED;
+ if (strstr(traits, " fully-peeled "))
+ fully_peeled = 1;
/* perhaps other traits later as well */
continue;
}
refname = parse_ref_line(refline, sha1);
if (refname) {
- last = create_ref_entry(refname, sha1, flag, 1);
+ /*
+ * Older git did not write peel lines for anything
+ * outside of refs/tags/; if the fully-peeled trait
+ * is not set, we are dealing with such an older
+ * git and cannot assume an omitted peel value
+ * means the ref is not a tag object.
+ */
+ int this_flag = flag;
+ if (!fully_peeled && prefixcmp(refname, "refs/tags/"))
+ this_flag &= ~REF_KNOWS_PEELED;
+
+ last = create_ref_entry(refname, sha1, this_flag, 1);
add_ref(dir, last);
continue;
}
diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
index dd5b48b..a8eb1aa 100755
--- a/t/t3211-peel-ref.sh
+++ b/t/t3211-peel-ref.sh
@@ -39,4 +39,26 @@ test_expect_success 'refs are peeled outside of refs/tags (packed)' '
test_cmp expect actual
'
+test_expect_success 'create old-style pack-refs without fully-peeled' '
+ # Git no longer writes without fully-peeled, so we just write our own
+ # from scratch; we could also munge the existing file to remove the
+ # fully-peeled bits, but that seems even more prone to failure,
+ # especially if the format ever changes again. At least this way we
+ # know we are emulating exactly what an older git would have written.
+ {
+ echo "# pack-refs with: peeled " &&
+ print_ref "refs/heads/master" &&
+ print_ref "refs/outside/foo" &&
+ print_ref "refs/tags/base" &&
+ print_ref "refs/tags/foo" &&
+ echo "^$(git rev-parse "refs/tags/foo^{}")"
+ } >tmp &&
+ mv tmp .git/packed-refs
+'
+
+test_expect_success 'refs are peeled outside of refs/tags (old packed)' '
+ git show-ref -d >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.8.2.rc2.7.gef06216
^ permalink raw reply related
* Re: [PATCH 11/12] pretty: support truncating in %>, %< and %><
From: Paul Campbell @ 2013-03-16 9:04 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <1363400683-14813-12-git-send-email-pclouds@gmail.com>
On Sat, Mar 16, 2013 at 2:24 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> %>(N,trunc) truncates the righ part after N columns and replace the
> last two letters with "..". ltrunc does the same on the left. mtrunc
> cuts the middle out.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
s/righ/right/
> Documentation/pretty-formats.txt | 6 +++--
> pretty.c | 51 +++++++++++++++++++++++++++++++++++++---
> utf8.c | 46 ++++++++++++++++++++++++++++++++++++
> utf8.h | 2 ++
> 4 files changed, 100 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 87ca2c4..17f82f4 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -162,8 +162,10 @@ The placeholders are:
> - '%x00': print a byte from a hex code
> - '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of
> linkgit:git-shortlog[1].
> -- '%<(<N>)': make the next placeholder take at least N columns,
> - padding spaces on the right if necessary
> +- '%<(<N>[,trunc|ltrunc|mtrunc])': make the next placeholder take at
> + least N columns, padding spaces on the right if necessary.
> + Optionally truncate at the beginning (ltrunc), the middle (mtrunc)
> + or the end (trunc) if the output is longer than N columns.
> - '%<|(<N>)': make the next placeholder take at least until Nth
> columns, padding spaces on the right if necessary
> - '%>(<N>)', '%>|(<N>)': similar to '%<(<N<)', '%<|(<N<)'
> diff --git a/pretty.c b/pretty.c
> index 233d69c..29384b5 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -767,6 +767,13 @@ enum flush_type {
> flush_both
> };
>
> +enum trunc_type {
> + trunc_none,
> + trunc_left,
> + trunc_middle,
> + trunc_right
> +};
> +
> struct format_commit_context {
> const struct commit *commit;
> const struct pretty_print_context *pretty_ctx;
> @@ -774,6 +781,7 @@ struct format_commit_context {
> unsigned commit_message_parsed:1;
> unsigned commit_signature_parsed:1;
> enum flush_type flush_type;
> + enum trunc_type truncate;
> struct {
> char *gpg_output;
> char good_bad;
> @@ -1044,7 +1052,7 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
>
> if (*ch == '(') {
> const char *start = ch + 1;
> - const char *end = strchr(start, ')');
> + const char *end = start + strcspn(start, ",)");
> char *next;
> int width;
> if (!end || end == start)
> @@ -1054,6 +1062,23 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
> return 0;
> c->padding = to_column ? -width : width;
> c->flush_type = flush_type;
> +
> + if (*end == ',') {
> + start = end + 1;
> + end = strchr(start, ')');
> + if (!end || end == start)
> + return 0;
> + if (!prefixcmp(start, "trunc)"))
> + c->truncate = trunc_right;
> + else if (!prefixcmp(start, "ltrunc)"))
> + c->truncate = trunc_left;
> + else if (!prefixcmp(start, "mtrunc)"))
> + c->truncate = trunc_middle;
> + else
> + return 0;
> + } else
> + c->truncate = trunc_none;
> +
> return end - placeholder + 1;
> }
> return 0;
> @@ -1335,9 +1360,29 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
> total_consumed++;
> }
> len = utf8_strnwidth(local_sb.buf, -1, 1);
> - if (len > padding)
> + if (len > padding) {
> + switch (c->truncate) {
> + case trunc_left:
> + strbuf_utf8_replace(&local_sb,
> + 0, len - (padding - 2),
> + "..");
> + break;
> + case trunc_middle:
> + strbuf_utf8_replace(&local_sb,
> + padding / 2 - 1,
> + len - (padding - 2),
> + "..");
> + break;
> + case trunc_right:
> + strbuf_utf8_replace(&local_sb,
> + padding - 2, len - (padding - 2),
> + "..");
> + break;
> + case trunc_none:
> + break;
> + }
> strbuf_addstr(sb, local_sb.buf);
> - else {
> + } else {
> int sb_len = sb->len, offset = 0;
> if (c->flush_type == flush_left)
> offset = padding - len;
> diff --git a/utf8.c b/utf8.c
> index 9d98043..766df80 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -421,6 +421,52 @@ void strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len,
> free(tmp);
> }
>
> +void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width,
> + const char *subst)
> +{
> + struct strbuf sb_dst = STRBUF_INIT;
> + char *src = sb_src->buf;
> + char *end = src + sb_src->len;
> + char *dst;
> + int w = 0, subst_len = 0;
> +
> + if (subst)
> + subst_len = strlen(subst);
> + strbuf_grow(&sb_dst, sb_src->len + subst_len);
> + dst = sb_dst.buf;
> +
> + while (src < end) {
> + char *old;
> + size_t n;
> +
> + while ((n = display_mode_esc_sequence_len(src))) {
> + memcpy(dst, src, n);
> + src += n;
> + dst += n;
> + }
> +
> + old = src;
> + n = utf8_width((const char**)&src, NULL);
> + if (!src) /* broken utf-8, do nothing */
> + return;
> + if (n && w >= pos && w < pos + width) {
> + if (subst) {
> + memcpy(dst, subst, subst_len);
> + dst += subst_len;
> + subst = NULL;
> + }
> + w += n;
> + continue;
> + }
> + memcpy(dst, old, src - old);
> + dst += src - old;
> + w += n;
> + }
> + strbuf_setlen(&sb_dst, dst - sb_dst.buf);
> + strbuf_attach(sb_src, strbuf_detach(&sb_dst, NULL),
> + sb_dst.len, sb_dst.alloc);
> +}
> +
> int is_encoding_utf8(const char *name)
> {
> if (!name)
> diff --git a/utf8.h b/utf8.h
> index 99db3e0..faf2f91 100644
> --- a/utf8.h
> +++ b/utf8.h
> @@ -15,6 +15,8 @@ void strbuf_add_wrapped_text(struct strbuf *buf,
> const char *text, int indent, int indent2, int width);
> void strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len,
> int indent, int indent2, int width);
> +void strbuf_utf8_replace(struct strbuf *sb, int pos, int width,
> + const char *subst);
>
> #ifndef NO_ICONV
> char *reencode_string_iconv(const char *in, size_t insz,
> --
> 1.8.2.83.gc99314b
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Paul [W] Campbell
^ permalink raw reply
* Re: Tag peeling peculiarities
From: Jeff King @ 2013-03-16 9:34 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git discussion list
In-Reply-To: <514431EA.2050402@alum.mit.edu>
On Sat, Mar 16, 2013 at 09:48:42AM +0100, Michael Haggerty wrote:
> My patch series is nearly done. I will need another day or two to
> review and make it submission-ready, but I wanted to give you an idea of
> what I'm up to and I could also use your feedback on some points.
I was just sending out my cleaned up patches to do do fully-peeled, too.
I think the duplication is OK, though, as your topic would be for
"master" and mine can go to "maint".
> * Implement fully-peeled packed-refs files, as discussed upthread: peel
> references outside of refs/tags, and keep rigorous track of
> which references have REF_KNOWS_PEELED.
Looks pretty similar to mine. We even added similar tests.
I notice that you do the "add REF_KNOWS_PEELED if we actually got a peel
line" optimization. I didn't bother, because this will never be
triggered by a git-written file (either we do not write the entry, or we
set fully-peeled). I wonder if any other implementation does peel every
ref, though.
> * Change pack-refs to use the peeled information from ref_entries if it
> is available, rather than looking up the references again.
I don't know that it matters from a performance standpoint (we don't
really care how long pack-refs takes, as long as it is within reason,
because it is run as part of a gc). But it would mean that any errors
in the file are propagated from one run of pack-refs to the next. I
don't know if we want to spend the extra time to be more robust.
> * repack_without_ref() writes peeled refs to the packed-refs file.
> Use the old values if available; otherwise look them up.
Whereas here we probably _do_ want the performance optimization, because
we do care about the performance of a ref deletion.
> 1. There are multiple functions for peeling refs:
> peel_ref() (and peel_object(), which was extracted from it);
> peel_entry() (derived from pack-refs.c:pack_one_ref()). It would be
> nice to combine these into the One True Function. But given the
> problem that you mentioned above (which is rooted in parts of the
> code that I don't know) I don't know what that version should do.
I'm not sure I understand the question. Just skimming your patches, it
looks like peel_entry could just call peel_object?
> 2. repack_without_ref() now writes peeled refs, peeling them if
> necessary. It does this *without* referring to the loose refs
> to avoid having to load all of the loose references, which can be
> time-consuming. But this means that it might try to lookup SHA1s
> that are not the current value of the corresponding reference any
> more, and might even have been garbage collected.
Yuck. I really wonder if repack_without_ref should literally just be
writing out the exact same file, minus the lines for the deleted ref.
Is there a reason we need to do any processing at all? I guess the
answer is that you want to avoid re-parsing the current file, and just
write it out from memory. But don't we need to refresh the memory cache
from disk under the lock anyway? That was the pack-refs race that I
fixed recently.
> Is the code that
> I use to do this, in peel_entry(), safe to call for nonexistent
> SHA1s (I would like it to return 0 if the SHA1 is invalid)?:
>
> o = parse_object(entry->u.value.sha1);
> if (o->type == OBJ_TAG) {
> o = deref_tag(o, entry->name, 0);
> if (o) {
> hashcpy(entry->u.value.peeled, o->sha1);
> entry->flag |= REF_KNOWS_PEELED;
> return 1;
> }
> }
> return 0;
I think this approach is safe, as parse_object will silently return NULL
for a missing object. You do need to check for "o != NULL" in your
conditional, though.
> 3. This same change to repack_with_ref() means that it could become
> significantly slower to delete a packed ref if the packed-ref file
> is not fully-peeled. On the plus side, once this is done once, the
> packed-refs files will be rewritten in fully-peeled form. But if
> two versions of Git are being used in a repository, this cost could
> be incurred repeatedly. Does anybody object?
I think it's OK in concept. But I still am wondering if it would be
simpler still to just pass the file through while locked.
> 4. Should I change the locking policy as discussed in this thread?:
>
> http://article.gmane.org/gmane.comp.version-control.git/212131
I think it's overall a sane direction. It does increase lock contention
slightly when two processes are deleting at the same time, but it would
fix the weird corner cases I described (mostly deleted refs reappearing
due to races). And the lock contention is already there in a
fully-packed repo anyway. I.e., right now we read the packed-refs file
and lock it if our to-delete ref is in there; with the proposed change,
we would lock before even reading it. So the increased contention is
only when two deleters race each other, _and_ one of them is not
deleting a packed ref.
-Peff
^ permalink raw reply
* Some small issues with GIT GUI on MacOS.
From: Pascal @ 2013-03-16 9:47 UTC (permalink / raw)
To: git
Hello, I'm new to this list and using GIT on MacOS 10.8:
$ /usr/local/git/bin/git --version
git version 1.7.4.1
I've caught some small issues when using GIT GUI (French version):
- when GIT GUI just launched, the menu "Dépôt" (repository I guess) and Apple are disabled
- when opening preferences window then clic on cancel button, all menus stay disabled
Maybe, they have been already fixed in a recent release.
HTH, Pascal.
http://blady.pagesperso-orange.fr
^ permalink raw reply
* [PATCH 0/3] fix unparsed object access in upload-pack
From: Jeff King @ 2013-03-16 10:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
This series fixes the issue I mentioned recently with upload-pack, where
we might feed unparsed objects to the revision parser. The bug is in
435c833 (the tip of the jk/peel-ref topic), which is in v1.8.1 and up.
The fix should go to maint. The bug breaks shallow clones from
repositories with packed refs, though the effects range from working OK
to sending too many objects to calling die() in the revision traversal
machinery. Given the size of the breakage (I had several bug reports
within a few hours of deploying v1.8.1.5 on github.com), and the fact
that the bug was introduced only in v1.8.1, it may make sense to roll a
v1.8.1.6 in addition to putting it on the 1.8.2 maint track.
[1/3]: upload-pack: drop lookup-before-parse optimization
This is just a cleanup, and is not necessary for the fix.
[2/3]: upload-pack: make sure "want" objects are parsed
This is the fix itself, which can stand alone from the other two
patches, both semantically and textually.
[3/3]: upload-pack: load non-tip "want" objects from disk
While investigating the bug, I found some weirdness around the
stateless-rpc check_non_tip code. As far as I can tell, that code
never actually gets triggered. It's not too surprising that we
wouldn't have noticed, because it is about falling back due to a
race condition. But please sanity check my explanation and patch.
-Peff
^ permalink raw reply
* [PATCH 1/3] upload-pack: drop lookup-before-parse optimization
From: Jeff King @ 2013-03-16 10:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20130316102428.GA29358@sigill.intra.peff.net>
When we receive a "have" line from the client, we want to
load the object pointed to by the sha1. However, we are
careful to do:
o = lookup_object(sha1);
if (!o || !o->parsed)
o = parse_object(sha1);
to avoid loading the object from disk if we have already
seen it. However, since ccdc603 (parse_object: try internal
cache before reading object db), parse_object already does
this optimization internally. We can just call parse_object
directly.
Signed-off-by: Jeff King <peff@peff.net>
---
upload-pack.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 30146a0..6bf81aa 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -325,9 +325,7 @@ static int got_sha1(char *hex, unsigned char *sha1)
if (!has_sha1_file(sha1))
return -1;
- o = lookup_object(sha1);
- if (!(o && o->parsed))
- o = parse_object(sha1);
+ o = parse_object(sha1);
if (!o)
die("oops (%s)", sha1_to_hex(sha1));
if (o->type == OBJ_COMMIT) {
--
1.8.2.rc2.7.gef06216
^ permalink raw reply related
* [PATCH 2/3] upload-pack: make sure "want" objects are parsed
From: Jeff King @ 2013-03-16 10:27 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20130316102428.GA29358@sigill.intra.peff.net>
When upload-pack receives a "want" line from the client, it
adds it to an object array. We call lookup_object to find
the actual object, which will only check for objects already
in memory. This works because we are expecting to find
objects that we already loaded during the ref advertisement.
We use the resulting object structs for a variety of
purposes. Some of them care only about the object flags, but
others care about the type of the object (e.g.,
ok_to_give_up), or even feed them to the revision parser
(when --depth is used), which assumes that objects it
receives are fully parsed.
Once upon a time, this was OK; any object we loaded into
memory would also have been parsed. But since 435c833
(upload-pack: use peel_ref for ref advertisements,
2012-10-04), we try to avoid parsing objects during the ref
advertisement. This means that lookup_object may return an
object with a type of OBJ_NONE. The resulting mess depends
on the exact set of objects, but can include the revision
parser barfing, or the shallow code sending the wrong set of
objects.
This patch teaches upload-pack to parse each "want" object
as we receive it. We do not replace the lookup_object call
with parse_object, as the current code is careful not to let
just any object appear on a "want" line, but rather only one
we have previously advertised (whereas parse_object would
actually load any arbitrary object from disk).
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5500-fetch-pack.sh | 9 +++++++++
upload-pack.c | 2 +-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 354d32c..d574085 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -364,6 +364,15 @@ EOF
test_cmp count7.expected count7.actual
'
+test_expect_success 'clone shallow with packed refs' '
+ git pack-refs --all &&
+ git clone --depth 1 --branch A "file://$(pwd)/." shallow8 &&
+ echo "in-pack: 4" > count8.expected &&
+ GIT_DIR=shallow8/.git git count-objects -v |
+ grep "^in-pack" > count8.actual &&
+ test_cmp count8.expected count8.actual
+'
+
test_expect_success 'setup tests for the --stdin parameter' '
for head in C D E F
do
diff --git a/upload-pack.c b/upload-pack.c
index 6bf81aa..41736ec 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -639,7 +639,7 @@ static void receive_needs(void)
use_include_tag = 1;
o = lookup_object(sha1_buf);
- if (!o)
+ if (!o || !parse_object(o->sha1))
die("git upload-pack: not our ref %s",
sha1_to_hex(sha1_buf));
if (!(o->flags & WANTED)) {
--
1.8.2.rc2.7.gef06216
^ permalink raw reply related
* [PATCH 3/3] upload-pack: load non-tip "want" objects from disk
From: Jeff King @ 2013-03-16 10:28 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20130316102428.GA29358@sigill.intra.peff.net>
It is a long-time security feature that upload-pack will not
serve any "want" lines that do not correspond to the tip of
one of our refs. Traditionally, this was enforced by
checking the objects in the in-memory hash; they should have
been loaded and received the OUR_REF flag during the
advertisement.
The stateless-rpc mode, however, has a race condition here:
one process advertises, and another receives the want lines,
so the refs may have changed in the interim. To address
this, commit 051e400 added a new verification mode; if the
object is not OUR_REF, we set a "has_non_tip" flag, and then
later verify that the requested objects are reachable from
our current tips.
However, we still die immediately when the object is not in
our in-memory hash, and at this point we should only have
loaded our tip objects. So the check_non_tip code path does
not ever actually trigger, as any non-tip objects would
have already caused us to die.
We can fix that by using parse_object instead of
lookup_object, which will load the object from disk if it
has not already been loaded.
We still need to check that parse_object does not return
NULL, though, as it is possible we do not have the object
at all. A more appropriate error message would be "no such
object" rather than "not our ref"; however, we do not want
to leak information about what objects are or are not in
the object database, so we continue to use the same "not
our ref" message that would be produced by an unreachable
object.
Signed-off-by: Jeff King <peff@peff.net>
---
upload-pack.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 41736ec..948cfff 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -638,8 +638,8 @@ static void receive_needs(void)
if (parse_feature_request(features, "include-tag"))
use_include_tag = 1;
- o = lookup_object(sha1_buf);
- if (!o || !parse_object(o->sha1))
+ o = parse_object(sha1_buf);
+ if (!o)
die("git upload-pack: not our ref %s",
sha1_to_hex(sha1_buf));
if (!(o->flags & WANTED)) {
--
1.8.2.rc2.7.gef06216
^ permalink raw reply related
* Re: regression in multi-threaded git-pack-index
From: Jeff King @ 2013-03-16 11:41 UTC (permalink / raw)
To: Stefan Zager; +Cc: git
In-Reply-To: <20130315224240.50AA340839@wince.sfo.corp.google.com>
On Fri, Mar 15, 2013 at 03:42:40PM -0700, Stefan Zager wrote:
> We have uncovered a regression in this commit:
>
> b8a2486f1524947f232f657e9f2ebf44e3e7a243
>
> The symptom is that 'git fetch' dies with:
>
> error: index-pack died of signal 10
> fatal: index-pack failed
>
> I have only been able to reproduce it on a Mac thus far; will try
> ubuntu next. We can make it go away by running:
>
> git config pack.threads 1
I couldn't reproduce the problem on Linux with the instructions you
gave. I did try running it under valgrind and it produced:
==2380== Conditional jump or move depends on uninitialised value(s)
==2380== at 0x441631: resolve_delta (index-pack.c:837)
==2380== by 0x4419D6: find_unresolved_deltas_1 (index-pack.c:898)
==2380== by 0x441A45: find_unresolved_deltas (index-pack.c:914)
==2380== by 0x4427CA: fix_unresolved_deltas (index-pack.c:1232)
==2380== by 0x4421F5: conclude_pack (index-pack.c:1111)
==2380== by 0x443A5C: cmd_index_pack (index-pack.c:1604)
==2380== by 0x4058A2: run_builtin (git.c:281)
==2380== by 0x405A35: handle_internal_command (git.c:443)
==2380== by 0x405C01: main (git.c:532)
but the line in question is:
if (deepest_delta < delta_obj->delta_depth)
And in the debugger, both of those variables appear to have sane values
(nor should either impacted by the patch you bisected to). On top of
that, running with pack.threads=1 produces the same error. So I think it
may be a false positive from valgrind, and unrelated to your issue.
Other than that, it seems to run fine for me.
-Peff
^ permalink raw reply
* Re: [PATCH/RFC] http_init: only initialize SSL for https
From: Jeff King @ 2013-03-16 12:03 UTC (permalink / raw)
To: Daniel Stenberg
Cc: Junio C Hamano, Johannes Schindelin, kusmabite, git, msysgit
In-Reply-To: <alpine.DEB.2.00.1303151719170.32216@tvnag.unkk.fr>
On Fri, Mar 15, 2013 at 05:23:27PM +0100, Daniel Stenberg wrote:
> >Thanks, then we should stick to starting from ALL like everybody
> >else who followed the suggestion in the documentation. Do you have
> >recommendations on the conditional dropping of SSL?
>
> Not really, no.
>
> SSL initing is as has been mentioned alredy only relevant with
> libcurl if an SSL powered protocol is gonna be used, so if checking
> the URL for the protocol is enough to figure this out then sure that
> should work fine.
But are we correct in assuming that curl will barf if it gets a redirect
to an ssl-enabled protocol? My testing seems to say yes:
[in one terminal]
$ nc -lCp 5001 <<\EOF
HTTP/1.1 301
Location: https://github.com/peff/git.git
EOF
[in another, git compiled with Erik's patch]
$ git ls-remote http://localhost:5001
error: SSL: couldn't create a context: error:140A90A1:lib(20):func(169):reason(161) while accessing http://localhost:5001/info/refs?service=git-upload-pack
fatal: HTTP request failed
-Peff
--
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
---
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
^ permalink raw reply
* Re: git svn error "Not a valid object name"
From: Adam Retter @ 2013-03-16 12:07 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Dannes Wessels, Wolfgang Meier, Leif-Jöran Olsson
In-Reply-To: <20130316014548.GA16253@dcvr.yhbt.net>
>> fatal: Not a valid object name
>> ls-tree -z ./webapp/api/: command returned error: 128
>>
>> I have no idea what this means, or how to fix this.
>> We are using Git version 1.8.1.GIT on Amazon EC2 Linux.
>>
>> Any suggestions please?
>
> You might've hit a bug in branch detection, but I'd have to look at the
> repo to be certain and fix it if neded.
> --no-follow-parent should work, but you'd lose branch/tag history.
Hi Eric,
Thanks for the response, ideally we would like to keep our history
which is why we are really using one of these tools in the first
place.
If your able, any idea of when you might be able to take a look at the
bug? Our svn repo is publicly available for all.
--
Adam Retter
eXist Developer
{ United Kingdom }
adam@exist-db.org
irc://irc.freenode.net/existdb
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Duy Nguyen @ 2013-03-16 12:38 UTC (permalink / raw)
To: Stefan Zager; +Cc: Jeff King, git
In-Reply-To: <20130316114118.GA1940@sigill.intra.peff.net>
On Sat, Mar 16, 2013 at 6:41 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Mar 15, 2013 at 03:42:40PM -0700, Stefan Zager wrote:
>
>> We have uncovered a regression in this commit:
>>
>> b8a2486f1524947f232f657e9f2ebf44e3e7a243
What version did you test? We used to have problems with multithreaded
index-pack on cywgin because its pread implementation is not
thread-safe, see c0f8654 (index-pack: Disable threading on cygwin -
2012-06-26). Not sure if we fall into the same path on Mac, or this is
something else..
>>
>> The symptom is that 'git fetch' dies with:
>>
>> error: index-pack died of signal 10
>> fatal: index-pack failed
I guess it won't help much, but what if you enable coredump and get a
stack trace from it?
>>
>> I have only been able to reproduce it on a Mac thus far; will try
>> ubuntu next. We can make it go away by running:
>>
>> git config pack.threads 1
--
Duy
^ permalink raw reply
* Re: SSH version on Git 1.8.1.2 for Windows is outdated.
From: Sebastian Schuberth @ 2013-03-16 13:14 UTC (permalink / raw)
To: Joshua Jensen; +Cc: Konstantin Khomoutov, Kristof Mattei, git
In-Reply-To: <51438058.7080204@workspacewhiz.com>
On 15.03.2013 21:11, Joshua Jensen wrote:
>> Yes, you should grab the msysGit (the Git for Windows build
>> environment) [2], tweak it to include the new OpenSSH binary, ensure it
>> builds and works OK and then send a pull request (or post your patchset
>> to the msysgit mailing list [3].
>>
> Wow, we can do that now?
>
> When I brought up the vastly improved performance from a newer SSH
> executable, I was told the only way to get it in would be to build from
> source [1].
"tweak it to include the new OpenSSH binary" is supposed to include the
step to adjust the release.sh script to grab the updated sources and
build the binary.
However, another option is to take a look at the new mingwGitDevEnv
project [1], which relies on mingw-get to retrieve binary packages. I
hopefully find soon the time to include OpenSSH 6.1p1 incl. HPN-SSH
patches [2].
[1] https://github.com/sschuberth/mingwGitDevEnv
[2] https://github.com/sschuberth/mingwGitDevEnv/pull/5
--
Sebastian Schuberth
^ permalink raw reply
* Re: Tag peeling peculiarities
From: Michael Haggerty @ 2013-03-16 13:38 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git discussion list
In-Reply-To: <20130316093441.GA26260@sigill.intra.peff.net>
On 03/16/2013 10:34 AM, Jeff King wrote:
> On Sat, Mar 16, 2013 at 09:48:42AM +0100, Michael Haggerty wrote:
>
>> My patch series is nearly done. I will need another day or two to
>> review and make it submission-ready, but I wanted to give you an idea of
>> what I'm up to and I could also use your feedback on some points.
>
> I was just sending out my cleaned up patches to do do fully-peeled, too.
> I think the duplication is OK, though, as your topic would be for
> "master" and mine can go to "maint".
Yes, though I'll have to rebase mine on top of yours.
>> * Implement fully-peeled packed-refs files, as discussed upthread: peel
>> references outside of refs/tags, and keep rigorous track of
>> which references have REF_KNOWS_PEELED.
>
> Looks pretty similar to mine. We even added similar tests.
>
> I notice that you do the "add REF_KNOWS_PEELED if we actually got a peel
> line" optimization. I didn't bother, because this will never be
> triggered by a git-written file (either we do not write the entry, or we
> set fully-peeled). I wonder if any other implementation does peel every
> ref, though.
We read the peeled value for refs outside of refs/tags even if
fully-peeled is not set, so it seemed somehow inconsistent not to set
REF_KNOWS_PEELED. But I agree that Git itself should never write such
entries, except of course for the interval between your first patch and
your second patch ;-)
>> * Change pack-refs to use the peeled information from ref_entries if it
>> is available, rather than looking up the references again.
>
> I don't know that it matters from a performance standpoint (we don't
> really care how long pack-refs takes, as long as it is within reason,
> because it is run as part of a gc). But it would mean that any errors
> in the file are propagated from one run of pack-refs to the next. I
> don't know if we want to spend the extra time to be more robust.
I thought about this argument too. Either way is OK with me. BTW would
it make sense to build a consistency check of peeled references into fsck?
>> * repack_without_ref() writes peeled refs to the packed-refs file.
>> Use the old values if available; otherwise look them up.
>
> Whereas here we probably _do_ want the performance optimization, because
> we do care about the performance of a ref deletion.
Agreed.
>> 1. There are multiple functions for peeling refs:
>> peel_ref() (and peel_object(), which was extracted from it);
>> peel_entry() (derived from pack-refs.c:pack_one_ref()). It would be
>> nice to combine these into the One True Function. But given the
>> problem that you mentioned above (which is rooted in parts of the
>> code that I don't know) I don't know what that version should do.
>
> I'm not sure I understand the question. Just skimming your patches, it
> looks like peel_entry could just call peel_object?
I believe that the version in peel_object() is the one that you
optimized in your now-famous 435c833 patches, which your earlier email
said was connected to a null pointer dereference. I'm not at all
familiar with the API being used there, so I don't know whether the two
versions are interchangeable, or whether you need to fix your
optimization, or whether your optimization will need to be reverted
because of the problem you discovered.
>> 2. repack_without_ref() now writes peeled refs, peeling them if
>> necessary. It does this *without* referring to the loose refs
>> to avoid having to load all of the loose references, which can be
>> time-consuming. But this means that it might try to lookup SHA1s
>> that are not the current value of the corresponding reference any
>> more, and might even have been garbage collected.
>
> Yuck. I really wonder if repack_without_ref should literally just be
> writing out the exact same file, minus the lines for the deleted ref.
> Is there a reason we need to do any processing at all? I guess the
> answer is that you want to avoid re-parsing the current file, and just
> write it out from memory. But don't we need to refresh the memory cache
> from disk under the lock anyway? That was the pack-refs race that I
> fixed recently.
It would certainly be thinkable to rewrite the file textually without
parsing it again. But I did it this way for two reasons:
1. It would be better to have one packed-refs file parser and writer
rather than two. (I'm working towards unifying the two writers, and
will continue once I understand their differences.)
2. Given how peeled references were added, it seems dangerous to read,
modify, and write data that might be in a future format that we don't
entirely understand. For example, suppose a new feature is added to
mark Junio's favorite tags:
# pack-refs with: peeled fully-peeled junios-favorites \n
ce432cac30f98b291be609a0fc974042a2156f55 refs/heads/master
83b3dd7151e7eb0e5ac62ee03aca7e6bccaa8d07 refs/tags/evil-dogs
^e1d04f8aad59397cbd55e72bf8a1bd75606f5350
7ed863a85a6ce2c4ac4476848310b8f917ab41f9 refs/tags/lolcats
^990f856a62a24bfd56bac1f5e4581381369e4ede
^^^junios-favorite
b0517166ae2ad92f3b17638cbdee0f04b8170d99 refs/tags/nonsense
^4baff50551545e2b6825973ec37bcaf03edb95fe
This would be backwards-compatible with the current code (granted, one
would lose the favorite information if the file is rewritten with an
older version of the code). But if we delete the lolcats tag textually,
we would cause the favorite annotation to be moved to a different tag
and thereby corrupt the data.
>> Is the code that
>> I use to do this, in peel_entry(), safe to call for nonexistent
>> SHA1s (I would like it to return 0 if the SHA1 is invalid)?:
>>
>> o = parse_object(entry->u.value.sha1);
>> if (o->type == OBJ_TAG) {
>> o = deref_tag(o, entry->name, 0);
>> if (o) {
>> hashcpy(entry->u.value.peeled, o->sha1);
>> entry->flag |= REF_KNOWS_PEELED;
>> return 1;
>> }
>> }
>> return 0;
>
> I think this approach is safe, as parse_object will silently return NULL
> for a missing object. You do need to check for "o != NULL" in your
> conditional, though.
Thanks; will fix.
>> 3. This same change to repack_with_ref() means that it could become
>> significantly slower to delete a packed ref if the packed-ref file
>> is not fully-peeled. On the plus side, once this is done once, the
>> packed-refs files will be rewritten in fully-peeled form. But if
>> two versions of Git are being used in a repository, this cost could
>> be incurred repeatedly. Does anybody object?
>
> I think it's OK in concept. But I still am wondering if it would be
> simpler still to just pass the file through while locked.
See above.
>> 4. Should I change the locking policy as discussed in this thread?:
>>
>> http://article.gmane.org/gmane.comp.version-control.git/212131
>
> I think it's overall a sane direction. It does increase lock contention
> slightly when two processes are deleting at the same time, but it would
> fix the weird corner cases I described (mostly deleted refs reappearing
> due to races). And the lock contention is already there in a
> fully-packed repo anyway. I.e., right now we read the packed-refs file
> and lock it if our to-delete ref is in there; with the proposed change,
> we would lock before even reading it. So the increased contention is
> only when two deleters race each other, _and_ one of them is not
> deleting a packed ref.
OK, I'll work on that too.
Thanks for your feedback!
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ 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