Michael Giuffrida <michae...@chromium.org> writes:
> For the abbreviated commit hash placeholder ('h'), pretty.c uses
> add_again() to cache the result of the expansion, and then uses that
> result in future expansions. This causes problems when the expansion
> includes whitespace:
>
> $ git log -n 1 --pretty='format:newline:%+h%nno_newline:%h'
> newline:
> a0b1c2d
> no_newline:
> a0b1c2
>
> The second expansion of the hash added an unwanted newline and removed
> the final character. It seemingly used the cached expansion *starting
> from the inserted newline*.
>
> The expected output is:
>
> $ git log -n 1 --pretty='format:newline:%+h%nno_newline:%h'
> newline:
> a0b1c2d
> no_newline:a0b1c2d
Nicely explained. The add_again() mechanism caches an earlier
result by remembering the offset and the length in the strbuf the
formatted string is being accumulated to, but because %+x (and
probably %-x) magic placeholders futz with the result of
format_commit_one() in place, the cache goes out of sync, of course.
I think the call to format_commit_one() needs to be taught to pass
'magic' through, and then add_again() mechanism needs to be told not
to cache when magic is in effect, or something like that.
Perhaps something along this line...?
pretty.c | 64 ++++++++++++++++++++++++++++++++++++++--------------------------
1 file changed, 38 insertions(+), 26 deletions(-)
diff --git a/pretty.c b/pretty.c
index 09701bd2ff..be22b12986 100644
--- a/pretty.c
+++ b/pretty.c
@@ -789,20 +789,22 @@ struct format_commit_context {
size_t wrap_start;
};
-static int add_again(struct strbuf *sb, struct chunk *chunk)
+static int add_again(struct strbuf *sb, struct chunk *chunk, int no_cache)
{
if (chunk->len) {
strbuf_adddup(sb, chunk->off, chunk->len);
return 1;
}
- /*
- * We haven't seen this chunk before. Our caller is surely
- * going to add it the hard way now. Remember the most likely
- * start of the to-be-added chunk: the current end of the
- * struct strbuf.
- */
- chunk->off = sb->len;
+ if (!no_cache) {
+ /*
+ * We haven't seen this chunk before. Our caller is surely
+ * going to add it the hard way now. Remember the most likely
+ * start of the to-be-added chunk: the current end of the
+ * struct strbuf.
+ */
+ chunk->off = sb->len;
+ }
return 0;
}
@@ -1066,15 +1068,24 @@ static size_t parse_padding_placeholder(struct strbuf
*sb,
return 0;
}
+enum format_commit_item_magic {
+ NO_MAGIC,
+ ADD_LF_BEFORE_NON_EMPTY,
+ DEL_LF_BEFORE_EMPTY,
+ ADD_SP_BEFORE_NON_EMPTY
+};
+
static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
- void *context)
+ void *context,
+ enum format_commit_item_magic magic)
{
struct format_commit_context *c = context;
const struct commit *commit = c->commit;
const char *msg = c->message;
struct commit_list *p;
int ch;
+ int no_cache;
/* these are independent of the commit */
switch (placeholder[0]) {
@@ -1139,6 +1150,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
if (!commit->object.parsed)
parse_object(&commit->object.oid);
+ no_cache = magic != NO_MAGIC;
+
switch (placeholder[0]) {
case 'H': /* commit hash */
strbuf_addstr(sb, diff_get_color(c->auto_color, DIFF_COMMIT));
@@ -1147,24 +1160,26 @@ static size_t format_commit_one(struct strbuf *sb, /*
in UTF-8 */
return 1;
case 'h': /* abbreviated commit hash */
strbuf_addstr(sb, diff_get_color(c->auto_color, DIFF_COMMIT));
- if (add_again(sb, &c->abbrev_commit_hash)) {
+ if (add_again(sb, &c->abbrev_commit_hash, no_cache)) {
strbuf_addstr(sb, diff_get_color(c->auto_color,
DIFF_RESET));
return 1;
}
strbuf_add_unique_abbrev(sb, commit->object.oid.hash,
c->pretty_ctx->abbrev);
strbuf_addstr(sb, diff_get_color(c->auto_color, DIFF_RESET));
- c->abbrev_commit_hash.len = sb->len - c->abbrev_commit_hash.off;
+ if (!no_cache)
+ c->abbrev_commit_hash.len = sb->len -
c->abbrev_commit_hash.off;
return 1;
case 'T': /* tree hash */
strbuf_addstr(sb, oid_to_hex(&commit->tree->object.oid));
return 1;
case 't': /* abbreviated tree hash */
- if (add_again(sb, &c->abbrev_tree_hash))
+ if (add_again(sb, &c->abbrev_tree_hash, no_cache))
return 1;
strbuf_add_unique_abbrev(sb, commit->tree->object.oid.hash,
c->pretty_ctx->abbrev);
- c->abbrev_tree_hash.len = sb->len - c->abbrev_tree_hash.off;
+ if (!no_cache)
+ c->abbrev_tree_hash.len = sb->len -
c->abbrev_tree_hash.off;
return 1;
case 'P': /* parent hashes */
for (p = commit->parents; p; p = p->next) {
@@ -1174,7 +1189,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
}
return 1;
case 'p': /* abbreviated parent hashes */
- if (add_again(sb, &c->abbrev_parent_hashes))
+ if (add_again(sb, &c->abbrev_parent_hashes, no_cache))
return 1;
for (p = commit->parents; p; p = p->next) {
if (p != commit->parents)
@@ -1182,8 +1197,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
strbuf_add_unique_abbrev(sb, p->item->object.oid.hash,
c->pretty_ctx->abbrev);
}
- c->abbrev_parent_hashes.len = sb->len -
- c->abbrev_parent_hashes.off;
+ if (!no_cache)
+ c->abbrev_parent_hashes.len =
+ sb->len - c->abbrev_parent_hashes.off;
return 1;
case 'm': /* left/right/bottom */
strbuf_addstr(sb, get_revision_mark(NULL, commit));
@@ -1314,7 +1330,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
- struct format_commit_context *c)
+ struct format_commit_context *c,
+ enum format_commit_item_magic magic)
{
struct strbuf local_sb = STRBUF_INIT;
int total_consumed = 0, len, padding = c->padding;
@@ -1329,7 +1346,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /*
in UTF-8 */
}
while (1) {
int modifier = *placeholder == 'C';
- int consumed = format_commit_one(&local_sb, placeholder, c);
+ int consumed = format_commit_one(&local_sb, placeholder, c,
magic);
total_consumed += consumed;
if (!modifier)
@@ -1420,12 +1437,7 @@ static size_t format_commit_item(struct strbuf *sb, /*
in UTF-8 */
{
int consumed;
size_t orig_len;
- enum {
- NO_MAGIC,
- ADD_LF_BEFORE_NON_EMPTY,
- DEL_LF_BEFORE_EMPTY,
- ADD_SP_BEFORE_NON_EMPTY
- } magic = NO_MAGIC;
+ enum format_commit_item_magic magic = NO_MAGIC;
switch (placeholder[0]) {
case '-':
@@ -1445,9 +1457,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in
UTF-8 */
orig_len = sb->len;
if (((struct format_commit_context *)context)->flush_type != no_flush)
- consumed = format_and_pad_commit(sb, placeholder, context);
+ consumed = format_and_pad_commit(sb, placeholder, context,
magic);
else
- consumed = format_commit_one(sb, placeholder, context);
+ consumed = format_commit_one(sb, placeholder, context, magic);
if (magic == NO_MAGIC)
return consumed;
