git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [BUG] add_again() off-by-one error in custom format

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 12, 2017, 22:49 UTC
Message-ID
<xmqqd1a8n7o8.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<CACi5S_1j46SbP7cQMdUnULmgGD7xBkSUrS2PKbzq8ZydybHE=w@mail.gmail.com>
Michael Giuffrida <michaelpg@chromium.org> writes:
Show 21 quoted lines
> 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;
Previous: Michael GiuffridaNext: René Scharfe
Message 2 of 30 in “[BUG] add_again() off-by-one error in custom format”
  1. Michael GiuffridaJun 12, 2017
  2. Junio C HamanoJun 12, 2017
  3. René ScharfeJun 13, 2017
  4. Junio C HamanoJun 13, 2017
  5. René ScharfeJun 13, 2017
  6. Junio C HamanoJun 13, 2017
  7. René ScharfeJun 14, 2017
  8. Jeff KingJun 15, 2017
  9. René ScharfeJun 15, 2017
  10. Jeff KingJun 15, 2017
  11. René ScharfeJun 18, 2017
  12. Jeff KingJun 18, 2017
  13. René ScharfeJun 18, 2017
  14. Jeff KingJun 18, 2017
  15. René ScharfeJun 22, 2017
  16. Jeff KingJun 22, 2017
  17. René ScharfeJun 18, 2017
  18. Jeff KingJun 18, 2017
  19. Junio C HamanoJun 19, 2017
  20. sha1_name: cache readdir(3) results in find_short_object_filename()René Scharfe, Jun 22, 2017
  21. Jeff KingJun 22, 2017
  22. René ScharfeJun 24, 2017
  23. Jeff KingJun 24, 2017
  24. René ScharfeJun 24, 2017
  25. Jeff KingJun 24, 2017
  26. René ScharfeJun 24, 2017
  27. Jeff KingJun 24, 2017
  28. Junio C HamanoJun 15, 2017
  29. SZEDER GáborJun 13, 2017
  30. René ScharfeJun 14, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.