[PATCH] for-each-ref's new per-atom formatting was failing if there were multiple fields per line

Andy Parkins· Oct 2, 2007, 11:02 UTC · lore

builtin-for-each-ref.c was searching backwards for ":" in the atom searches performed by parse_atom. In implementing the format handler I had assumed that parse_atom was being handed a single atom pointer. That was not the case. In fact the "atom" pointer is just a pointer within the longer format string, that means that the NUL for the end of the string is not at the end of the current atom, but at the end of the format string.

Finding the ":" separating the atom name from the format was done with strrchr(), which searches from the end of the string. That would be fine if it was searching a single atom string, but in the case of:

 --format="%(atom:format) %(atom:format)"

When the first atom is being parsed a backwards search actually finds the second colon, making the atom name look like:

 "atom:format) %(atom"

Which obviously doesn't match any valid atom name and so exits with "unknown field name".

The fix is to abandon the reverse search (which was only to allow colons in atom names, which was redundant as there are no atom names with colons) and replace it with a forward search. The potential presence of a second ":" also requires a check to confirm that the found ":" is between the start and end pointers, which this patch also adds.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 builtin-for-each-ref.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Show changes to builtin-for-each-ref.c +3 −3

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 2ca4fc6..f06d006 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -109,8 +109,8 @@ static int parse_atom(const char *atom, const char *ep)
 		/* If the atom name has a colon, strip it and everything after
 		 * it off - it specifies the format for this entry, and
 		 * shouldn't be used for checking against the valid_atom table */
-		const char *formatp = strrchr(sp, ':' );
-		if (formatp == NULL )
+		const char *formatp = strchr(sp, ':' );
+		if (formatp == NULL || formatp > ep )
 			formatp = ep;
 		if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
 			break;
@@ -366,7 +366,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
 	 * it's not possible that <something> is not ":<format>" because
 	 * parse_atom() wouldn't have allowed it, so we can assume that no
 	 * ":" means no format is specified, use the default */
-	formatp = strrchr( atomname, ':' );
+	formatp = strchr( atomname, ':' );
 	if (formatp != NULL) {
 		formatp++;
 		date_mode = parse_date_format(formatp);

-- 
1.5.3.2.105.gf47f2-dirty

Junio C Hamano· Oct 2, 2007, 22:12 UTC · re: Andy Parkins · lore

[PATCH] for-each-ref: fix %(numparent) and %(parent)

The string value of %(numparent) was not returned correctly. Also %(parent) misbehaved for the root commits (returned garbage) and merge commits (returned first parent, followed by a space).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I noticed this while playing with Andy's patch to enhance the
   date format string we saw recently on the list.

   Andy does not have anything to do with the breakage; this
   patch is against 'maint' to fix the bug that has always been
   there from the very beginning of this code.

 builtin-for-each-ref.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

Show changes to builtin-for-each-ref.c +6 −4

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 0afa1c5..29f70aa 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -43,7 +43,7 @@ static struct {
 	{ "objectsize", FIELD_ULONG },
 	{ "objectname" },
 	{ "tree" },
-	{ "parent" }, /* NEEDSWORK: how to address 2nd and later parents? */
+	{ "parent" },
 	{ "numparent", FIELD_ULONG },
 	{ "object" },
 	{ "type" },
@@ -262,24 +262,26 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object
 		}
 		if (!strcmp(name, "numparent")) {
 			char *s = xmalloc(40);
+			v->ul = num_parents(commit);
 			sprintf(s, "%lu", v->ul);
 			v->s = s;
-			v->ul = num_parents(commit);
 		}
 		else if (!strcmp(name, "parent")) {
 			int num = num_parents(commit);
 			int i;
 			struct commit_list *parents;
-			char *s = xmalloc(42 * num);
+			char *s = xmalloc(41 * num + 1);
 			v->s = s;
 			for (i = 0, parents = commit->parents;
 			     parents;
-			     parents = parents->next, i = i + 42) {
+			     parents = parents->next, i = i + 41) {
 				struct commit *parent = parents->item;
 				strcpy(s+i, sha1_to_hex(parent->object.sha1));
 				if (parents->next)
 					s[i+40] = ' ';
 			}
+			if (!i)
+				*s = '\0';
 		}
 	}
 }

-- 
1.5.3.3.1144.gf10f2

patchfor-each-ref's new per-atom formatting was failing if there were multiple fields per line