{"thread":{"id":"10114","subject":"[PATCH] for-each-ref's new per-atom formatting was failing if there were multiple fields per line","startedAt":"2007-10-02T11:02:42Z","lastAt":"2007-10-02T22:12:55Z","messageCount":2,"participants":["Andy Parkins","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"54552","messageId":"200710021202.42452.andyparkins@gmail.com","threadId":"10114","inReplyTo":null,"subject":"[PATCH] for-each-ref's new per-atom formatting was failing if there were multiple fields per line","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-10-02T11:02:42Z","receivedAt":"2007-10-02T11:02:42Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"builtin-for-each-ref.c was searching backwards for \":\" in the atom searches\nperformed by parse_atom. In implementing the format handler I had\nassumed that parse_atom was being handed a single atom pointer. That\nwas not the case. In fact the \"atom\" pointer is just a pointer within\nthe longer format string, that means that the NUL for the end of the\nstring is not at the end of the current atom, but at the end of the\nformat string.\n\nFinding the \":\" separating the atom name from the format was done with\nstrrchr(), which searches from the end of the string. That would be\nfine if it was searching a single atom string, but in the case of:\n\n --format=\"%(atom:format) %(atom:format)\"\n\nWhen the first atom is being parsed a backwards search actually finds\nthe second colon, making the atom name look like:\n\n \"atom:format) %(atom\"\n\nWhich obviously doesn't match any valid atom name and so exits with\n\"unknown field name\".\n\nThe fix is to abandon the reverse search (which was only to allow colons\nin atom names, which was redundant as there are no atom names with\ncolons) and replace it with a forward search. The potential presence of\na second \":\" also requires a check to confirm that the found \":\" is\nbetween the start and end pointers, which this patch also adds.\n\nSigned-off-by: Andy Parkins \n---\n builtin-for-each-ref.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c\nindex 2ca4fc6..f06d006 100644\n--- a/builtin-for-each-ref.c\n+++ b/builtin-for-each-ref.c\n@@ -109,8 +109,8 @@ static int parse_atom(const char *atom, const char *ep)\n \t\t/* If the atom name has a colon, strip it and everything after\n \t\t * it off - it specifies the format for this entry, and\n \t\t * shouldn't be used for checking against the valid_atom table */\n-\t\tconst char *formatp = strrchr(sp, ':' );\n-\t\tif (formatp == NULL )\n+\t\tconst char *formatp = strchr(sp, ':' );\n+\t\tif (formatp == NULL || formatp > ep )\n \t\t\tformatp = ep;\n \t\tif (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))\n \t\t\tbreak;\n@@ -366,7 +366,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam\n \t * it's not possible that is not \":\" because\n \t * parse_atom() wouldn't have allowed it, so we can assume that no\n \t * \":\" means no format is specified, use the default */\n-\tformatp = strrchr( atomname, ':' );\n+\tformatp = strchr( atomname, ':' );\n \tif (formatp != NULL) {\n \t\tformatp++;\n \t\tdate_mode = parse_date_format(formatp);\n-- \n1.5.3.2.105.gf47f2-dirty\n"},{"id":"54619","messageId":"7v8x6ljhco.fsf@gitster.siamese.dyndns.org","threadId":"10114","inReplyTo":"200710021202.42452.andyparkins@gmail.com","subject":"[PATCH] for-each-ref: fix %(numparent) and %(parent)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-10-02T22:12:55Z","receivedAt":"2007-10-02T22:12:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The string value of %(numparent) was not returned correctly.\nAlso %(parent) misbehaved for the root commits (returned garbage)\nand merge commits (returned first parent, followed by a space).\n\nSigned-off-by: Junio C Hamano \n---\n\n * I noticed this while playing with Andy's patch to enhance the\n date format string we saw recently on the list.\n\n Andy does not have anything to do with the breakage; this\n patch is against 'maint' to fix the bug that has always been\n there from the very beginning of this code.\n\n builtin-for-each-ref.c | 10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c\nindex 0afa1c5..29f70aa 100644\n--- a/builtin-for-each-ref.c\n+++ b/builtin-for-each-ref.c\n@@ -43,7 +43,7 @@ static struct {\n \t{ \"objectsize\", FIELD_ULONG },\n \t{ \"objectname\" },\n \t{ \"tree\" },\n-\t{ \"parent\" }, /* NEEDSWORK: how to address 2nd and later parents? */\n+\t{ \"parent\" },\n \t{ \"numparent\", FIELD_ULONG },\n \t{ \"object\" },\n \t{ \"type\" },\n@@ -262,24 +262,26 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object\n \t\t}\n \t\tif (!strcmp(name, \"numparent\")) {\n \t\t\tchar *s = xmalloc(40);\n+\t\t\tv->ul = num_parents(commit);\n \t\t\tsprintf(s, \"%lu\", v->ul);\n \t\t\tv->s = s;\n-\t\t\tv->ul = num_parents(commit);\n \t\t}\n \t\telse if (!strcmp(name, \"parent\")) {\n \t\t\tint num = num_parents(commit);\n \t\t\tint i;\n \t\t\tstruct commit_list *parents;\n-\t\t\tchar *s = xmalloc(42 * num);\n+\t\t\tchar *s = xmalloc(41 * num + 1);\n \t\t\tv->s = s;\n \t\t\tfor (i = 0, parents = commit->parents;\n \t\t\t parents;\n-\t\t\t parents = parents->next, i = i + 42) {\n+\t\t\t parents = parents->next, i = i + 41) {\n \t\t\t\tstruct commit *parent = parents->item;\n \t\t\t\tstrcpy(s+i, sha1_to_hex(parent->object.sha1));\n \t\t\t\tif (parents->next)\n \t\t\t\t\ts[i+40] = ' ';\n \t\t\t}\n+\t\t\tif (!i)\n+\t\t\t\t*s = '\\0';\n \t\t}\n \t}\n }\n-- \n1.5.3.3.1144.gf10f2\n"}]}