From: Junio C Hamano <hidden> Date: 2016-06-15 22:43:14
Johan Herland [off-list ref] writes:
Thanks to Johannes Schindelin [off-list ref] for
discovering this.
Also add a testcase for this condition.
Signed-off-by: Johan Herland <redacted>
While this certainly is an improvement, I suspect that your
parse_tag() does a little too much. In a format such as "tag"
object that does header + blank + body, it is customary to allow
header fields that your version does not understand (assuming
that such extention will go after the known fields is fine).
Which means that you should not be even saying "Ok, I've checked all
headers I know about---there should be a double LF to terminate it",
as you do not know if headers have ended.
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
On Friday 08 June 2007, Junio C Hamano wrote:
Johan Herland [off-list ref] writes:
quoted
Thanks to Johannes Schindelin [off-list ref] for
discovering this.
Also add a testcase for this condition.
Signed-off-by: Johan Herland <redacted>
While this certainly is an improvement, I suspect that your
parse_tag() does a little too much. In a format such as "tag"
object that does header + blank + body, it is customary to allow
header fields that your version does not understand (assuming
that such extention will go after the known fields is fine).
Which means that you should not be even saying "Ok, I've checked all
headers I know about---there should be a double LF to terminate it",
as you do not know if headers have ended.
Ok, I'm currently working on a patch series for Dscho and others where I
split up the big patch ('[PATCH 1/6] Refactor git tag objects; make "tag"
header optional; introduce new optional "keywords" header') into babysteps.
I can:
1. Provide a new patch series to totally replace the previous 6-part patch
series (plus bugfixes). The new patch series will make smaller steps and
end up (hopefully) in a better place, with less overzealous
checking/parsing, and more "traditional" whitespacing.
OR
2. Provide the babystep-series ending up exactly where we are today (i.e.
after the patch series, plus bug fixes). Then, provide patches on top of
the existing series to get it into shape, both scope-wise (i.e. not trying
to do too much) and whitespace-wise.
Which do you prefer?
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Junio C Hamano <hidden> Date: 2016-06-15 22:43:14
Johan Herland [off-list ref] writes:
I can:
1. Provide a new patch series to totally replace the previous 6-part patch
series (plus bugfixes). The new patch series will make smaller steps and
end up (hopefully) in a better place, with less overzealous
checking/parsing, and more "traditional" whitespacing.
OR
2. Provide the babystep-series ending up exactly where we are today (i.e.
after the patch series, plus bug fixes). Then, provide patches on top of
the existing series to get it into shape, both scope-wise (i.e. not trying
to do too much) and whitespace-wise.
Which do you prefer?
I am not sure if there is any practical difference between the
two ;-). But in either case, it appears that we should first
revert d9fa4a8 from 'next' and start from clean slate. It
really seems that the patch series did upset too many people;
personally I found the first patch still was follow-able, but I
do agree that it should have been much smaller and not mixing
too many things into one).
So, let's do 1.
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
This patch series implements part of the ground work for the 'notes'
feature discussed earlier in the thread "[PATCH 00/15] git-note: A
mechanism for providing free-form after-the-fact annotations on commits".
The following patches refactors the tag object by:
1. Unifying parsing and verification of tag objects (patches 1-9)
2. Do better and more thorough verification of tag objects (patches 10-13)
3. Making the "tagger" header mandatory as far as possible (patch 14)
4. Making the "tag" header optional (patch 15)
5. Introducing a new optional "keywords" header (patch 16)
6. Auxiliary changes supporting the above (patches 17-21)
This patch series replaces the earlier patch series of the same name
(plus the current bugfixes on top of that series). It's also much easier
on the eyes for those with 80 chars wide displays. Also, the selftest
suite should run successfully at any point in this patch series.
Here's the shortlog:
Johan Herland (21):
Remove unnecessary code and comments on non-existing 8kB tag object restriction
Return error messages when parsing fails.
Refactoring to make verify_tag() and parse_tag_buffer() more similar
Refactor verification of "tagger" line to be more similar to verification of "type" and "tagger" lines
Make parse_tag_buffer_internal() handle item == NULL
Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'
Copy the remaining differences from verify_tag() to parse_tag_buffer_internal()
Switch from verify_tag() to parse_and_verify_tag_buffer() for verifying tag objects in git-mktag
Remove unneeded code from mktag.c
Free mktag's buffer before dying
Rewrite error messages; fix up line lengths
Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers
Collect skipping of header field names and calculation of line lengths in one place
Add proper parsing of "tagger" line, but only when thorough_verify is set
Make tag names (i.e. the tag object's "tag" line) optional
Introduce optional "keywords" on tag objects
Update comments on tag objects in mktag.c
git-fsck: Do thorough verification of tag objects
Documentation/git-mktag: Document the changes in tag object structure
git-mktag tests: Expand on mktag selftests according to the new tag object structure
Add fsck_verify_ref_to_tag_object() to verify that refname matches name stored in tag object
Documentation/git-mktag.txt | 41 +++++--
builtin-fsck.c | 41 +++++++
builtin-log.c | 2 +-
mktag.c | 147 +++++-------------------
t/t3800-mktag.sh | 231 ++++++++++++++++++++++++++++++++++---
tag.c | 268 +++++++++++++++++++++++++++++++++++--------
tag.h | 5 +-
7 files changed, 540 insertions(+), 195 deletions(-)
Have fun!
...Johan
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
This patch brings the already similar tag.c:parse_tag_buffer() and
mktag.c:verify_tag() a little bit closer to eachother.
Signed-off-by: Johan Herland <redacted>
---
tag.c | 39 ++++++++++++++++++++++++++++++---------
1 files changed, 30 insertions(+), 9 deletions(-)
@@ -45,28 +51,41 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)item->object.parsed=1;if(size<64)-return-1;-if(memcmp("object ",data,7)||get_sha1_hex((char*)data+7,sha1))-return-1;+returnerror("failed preliminary size check");+/* Verify object line */+if(memcmp(data,"object ",7))+returnerror("char%d: does not start with \"object \"",0);++if(get_sha1_hex((char*)data+7,sha1))+returnerror("char%d: could not get SHA1 hash",7);++/* Verify type line */type_line=(char*)data+48;-if(memcmp("\ntype ",type_line-1,6))-return-1;+if(memcmp(type_line-1,"\ntype ",6))+returnerror("char%d: could not find \"\\ntype \"",47);+/* Verify tag-line */tag_line=strchr(type_line,'\n');-if(!tag_line||memcmp("tag ",++tag_line,4))-return-1;+if(!tag_line)+returnerror("char"PD_FMT": could not find next \"\\n\"",type_line-(char*)data);+tag_line++;+if(memcmp(tag_line,"tag ",4)||tag_line[4]=='\n')+returnerror("char"PD_FMT": no \"tag \" found",tag_line-(char*)data);sig_line=strchr(tag_line,'\n');if(!sig_line)return-1;sig_line++;+/* Get the actual type */typelen=tag_line-type_line-strlen("type \n");-if(typelen>=20)-return-1;+if(typelen>=sizeof(type))+returnerror("char"PD_FMT": type too long",type_line+5-(char*)data);+memcpy(type,type_line+5,typelen);type[typelen]='\0';+taglen=sig_line-tag_line-strlen("tag \n");item->tag=xmalloc(taglen+1);memcpy(item->tag,tag_line+4,taglen);
@@ -92,6 +111,8 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)}return0;++#undef PD_FMT}intparse_tag(structtag*item)
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
A fair amount of refactoring is included in this patch, none of which
should affect the actual behaviour of the code in any way.
Here are the changes done:
- Refactor out all the (char *) casting in parse_tag_buffer(). Create a
wrapper function (parse_tag_buffer()) that casts _once_ and then calls
parse_tag_buffer_internal() which does the real work
- Variable renaming in parse_tag_buffer_internal():
- sig_line -> tagger_line
- typelen -> type_len
- taglen -> tag_len
- Variable renaming in verify_tag():
- buffer -> data
- typelen -> type_len
- Remove unnecessary variable 'object' from verify_tag(). It has always
the same value as 'data', so use 'data' directly instead
- Fix type of length variables (int -> unsigned long)
- Move null-termination of tag buffer out of verify_tag() and into main().
Signed-off-by: Johan Herland <redacted>
---
mktag.c | 41 +++++++++++++++++++----------------------
tag.c | 50 +++++++++++++++++++++++++++-----------------------
2 files changed, 46 insertions(+), 45 deletions(-)
@@ -32,52 +32,48 @@ static int verify_object(unsigned char *sha1, const char *expected_type)returnret;}+staticintverify_tag(char*data,unsignedlongsize)+{#ifdef NO_C99_FORMAT#define PD_FMT "%d"#else#define PD_FMT "%td"#endif-staticintverify_tag(char*buffer,unsignedlongsize)-{-inttypelen;-chartype[20];unsignedcharsha1[20];-constchar*object,*type_line,*tag_line,*tagger_line;+chartype[20];+constchar*type_line,*tag_line,*tagger_line;+unsignedlongtype_len;if(size<64)returnerror("wanna fool me ? you obviously got the size wrong !");-buffer[size]=0;-/* Verify object line */-object=buffer;-if(memcmp(object,"object ",7))+if(memcmp(data,"object ",7))returnerror("char%d: does not start with \"object \"",0);-if(get_sha1_hex(object+7,sha1))+if(get_sha1_hex(data+7,sha1))returnerror("char%d: could not get SHA1 hash",7);/* Verify type line */-type_line=object+48;+type_line=data+48;if(memcmp(type_line-1,"\ntype ",6))returnerror("char%d: could not find \"\\ntype \"",47);/* Verify tag-line */tag_line=strchr(type_line,'\n');if(!tag_line)-returnerror("char"PD_FMT": could not find next \"\\n\"",type_line-buffer);+returnerror("char"PD_FMT": could not find next \"\\n\"",type_line-data);tag_line++;if(memcmp(tag_line,"tag ",4)||tag_line[4]=='\n')-returnerror("char"PD_FMT": no \"tag \" found",tag_line-buffer);+returnerror("char"PD_FMT": no \"tag \" found",tag_line-data);/* Get the actual type */-typelen=tag_line-type_line-strlen("type \n");-if(typelen>=sizeof(type))-returnerror("char"PD_FMT": type too long",type_line+5-buffer);--memcpy(type,type_line+5,typelen);-type[typelen]=0;+type_len=tag_line-type_line-strlen("type \n");+if(type_len>=sizeof(type))+returnerror("char"PD_FMT": type too long",type_line+5-data);+memcpy(type,type_line+5,type_len);+type[type_len]='\0';/* Verify that the object matches */if(verify_object(sha1,type))
@@ -91,23 +87,23 @@ static int verify_tag(char *buffer, unsigned long size)break;if(c>' ')continue;-returnerror("char"PD_FMT": could not verify tag name",tag_line-buffer);+returnerror("char"PD_FMT": could not verify tag name",tag_line-data);}/* Verify the tagger line */tagger_line=tag_line;if(memcmp(tagger_line,"tagger",6)||(tagger_line[6]=='\n'))-returnerror("char"PD_FMT": could not find \"tagger\"",tagger_line-buffer);+returnerror("char"PD_FMT": could not find \"tagger\"",tagger_line-data);/* TODO: check for committer info + blank line? *//* Also, the minimum length is probably + "tagger .", or 63+8=71 *//* The actual stuff afterwards we don't care about.. */return0;-}#undef PD_FMT+}intmain(intargc,char**argv){
@@ -124,6 +120,7 @@ int main(int argc, char **argv)free(buffer);die("could not read from stdin");}+buffer[size]=0;/* Verify it for some basic sanity: it needs to start with"object <sha1>\ntype\ntagger "*/
@@ -41,14 +41,14 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)#define PD_FMT "%td"#endif-inttypelen,taglen;unsignedcharsha1[20];-constchar*type_line,*tag_line,*sig_line;chartype[20];+constchar*type_line,*tag_line,*tagger_line;+unsignedlongtype_len,tag_len;-if(item->object.parsed)-return0;-item->object.parsed=1;+if(item->object.parsed)+return0;+item->object.parsed=1;if(size<64)returnerror("failed preliminary size check");
@@ -57,39 +57,38 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)if(memcmp(data,"object ",7))returnerror("char%d: does not start with \"object \"",0);-if(get_sha1_hex((char*)data+7,sha1))+if(get_sha1_hex(data+7,sha1))returnerror("char%d: could not get SHA1 hash",7);/* Verify type line */-type_line=(char*)data+48;+type_line=data+48;if(memcmp(type_line-1,"\ntype ",6))returnerror("char%d: could not find \"\\ntype \"",47);/* Verify tag-line */tag_line=strchr(type_line,'\n');if(!tag_line)-returnerror("char"PD_FMT": could not find next \"\\n\"",type_line-(char*)data);+returnerror("char"PD_FMT": could not find next \"\\n\"",type_line-data);tag_line++;if(memcmp(tag_line,"tag ",4)||tag_line[4]=='\n')-returnerror("char"PD_FMT": no \"tag \" found",tag_line-(char*)data);+returnerror("char"PD_FMT": no \"tag \" found",tag_line-data);-sig_line=strchr(tag_line,'\n');-if(!sig_line)+tagger_line=strchr(tag_line,'\n');+if(!tagger_line)return-1;-sig_line++;+tagger_line++;/* Get the actual type */-typelen=tag_line-type_line-strlen("type \n");-if(typelen>=sizeof(type))-returnerror("char"PD_FMT": type too long",type_line+5-(char*)data);--memcpy(type,type_line+5,typelen);-type[typelen]='\0';+type_len=tag_line-type_line-strlen("type \n");+if(type_len>=sizeof(type))+returnerror("char"PD_FMT": type too long",type_line+5-data);+memcpy(type,type_line+5,type_len);+type[type_len]='\0';-taglen=sig_line-tag_line-strlen("tag \n");-item->tag=xmalloc(taglen+1);-memcpy(item->tag,tag_line+4,taglen);-item->tag[taglen]='\0';+tag_len=tagger_line-tag_line-strlen("tag \n");+item->tag=xmalloc(tag_len+1);+memcpy(item->tag,tag_line+4,tag_len);+item->tag[tag_len]='\0';if(!strcmp(type,blob_type)){item->tagged=&lookup_blob(sha1)->object;
@@ -115,6 +114,11 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)#undef PD_FMT}+intparse_tag_buffer(structtag*item,void*data,unsignedlongsize)+{+returnparse_tag_buffer_internal(item,(constchar*)data,size);+}+intparse_tag(structtag*item){enumobject_typetype;
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
Also update selftests to reflect that verification of "tagger" now
happens _before_ verification of type name, object sha1 and tag name.
Signed-off-by: Johan Herland <redacted>
---
mktag.c | 16 ++++++++--------
t/t3800-mktag.sh | 3 +++
tag.c | 6 +++---
3 files changed, 14 insertions(+), 11 deletions(-)
@@ -62,12 +62,18 @@ static int verify_tag(char *data, unsigned long size)/* Verify tag-line */tag_line=strchr(type_line,'\n');-if(!tag_line)+if(!tag_line++)returnerror("char"PD_FMT": could not find next \"\\n\"",type_line-data);-tag_line++;if(memcmp(tag_line,"tag ",4)||tag_line[4]=='\n')returnerror("char"PD_FMT": no \"tag \" found",tag_line-data);+/* Verify the tagger line */+tagger_line=strchr(tag_line,'\n');+if(!tagger_line++)+returnerror("char"PD_FMT": could not find next \"\\n\"",tag_line-data);+if(memcmp(tagger_line,"tagger ",7)||(tagger_line[7]=='\n'))+returnerror("char"PD_FMT": could not find \"tagger\"",tagger_line-data);+/* Get the actual type */type_len=tag_line-type_line-strlen("type \n");if(type_len>=sizeof(type))
@@ -90,12 +96,6 @@ static int verify_tag(char *data, unsigned long size)returnerror("char"PD_FMT": could not verify tag name",tag_line-data);}-/* Verify the tagger line */-tagger_line=tag_line;--if(memcmp(tagger_line,"tagger",6)||(tagger_line[6]=='\n'))-returnerror("char"PD_FMT": could not find \"tagger\"",tagger_line-data);-/* TODO: check for committer info + blank line? *//* Also, the minimum length is probably + "tagger .", or 63+8=71 */
@@ -73,10 +73,10 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const uif(memcmp(tag_line,"tag ",4)||tag_line[4]=='\n')returnerror("char"PD_FMT": no \"tag \" found",tag_line-data);+/* Verify the tagger line */tagger_line=strchr(tag_line,'\n');-if(!tagger_line)-return-1;-tagger_line++;+if(!tagger_line++)+returnerror("char"PD_FMT": could not find next \"\\n\"",tag_line-data);/* Get the actual type */type_len=tag_line-type_line-strlen("type \n");
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
This is in preparation for unifying verify_tag() and
parse_tag_buffer_internal().
Signed-off-by: Johan Herland <redacted>
---
tag.c | 54 +++++++++++++++++++++++++++++-------------------------
1 files changed, 29 insertions(+), 25 deletions(-)
@@ -46,9 +46,11 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const uconstchar*type_line,*tag_line,*tagger_line;unsignedlongtype_len,tag_len;-if(item->object.parsed)-return0;-item->object.parsed=1;+if(item){+if(item->object.parsed)+return0;+item->object.parsed=1;+}if(size<64)returnerror("failed preliminary size check");
@@ -85,28 +87,30 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const umemcpy(type,type_line+5,type_len);type[type_len]='\0';-tag_len=tagger_line-tag_line-strlen("tag \n");-item->tag=xmalloc(tag_len+1);-memcpy(item->tag,tag_line+4,tag_len);-item->tag[tag_len]='\0';--if(!strcmp(type,blob_type)){-item->tagged=&lookup_blob(sha1)->object;-}elseif(!strcmp(type,tree_type)){-item->tagged=&lookup_tree(sha1)->object;-}elseif(!strcmp(type,commit_type)){-item->tagged=&lookup_commit(sha1)->object;-}elseif(!strcmp(type,tag_type)){-item->tagged=&lookup_tag(sha1)->object;-}else{-error("Unknown type %s",type);-item->tagged=NULL;-}--if(item->tagged&&track_object_refs){-structobject_refs*refs=alloc_object_refs(1);-refs->ref[0]=item->tagged;-set_object_refs(&item->object,refs);+if(item){+tag_len=tagger_line-tag_line-strlen("tag \n");+item->tag=xmalloc(tag_len+1);+memcpy(item->tag,tag_line+4,tag_len);+item->tag[tag_len]='\0';++if(!strcmp(type,blob_type)){+item->tagged=&lookup_blob(sha1)->object;+}elseif(!strcmp(type,tree_type)){+item->tagged=&lookup_tree(sha1)->object;+}elseif(!strcmp(type,commit_type)){+item->tagged=&lookup_commit(sha1)->object;+}elseif(!strcmp(type,tag_type)){+item->tagged=&lookup_tag(sha1)->object;+}else{+error("Unknown type %s",type);+item->tagged=NULL;+}++if(item->tagged&&track_object_refs){+structobject_refs*refs=alloc_object_refs(1);+refs->ref[0]=item->tagged;+set_object_refs(&item->object,refs);+}}return0;
@@ -81,19 +81,22 @@ static int verify_tag(char *data, unsigned long size)memcpy(type,type_line+5,type_len);type[type_len]='\0';-/* Verify that the object matches */-if(verify_object(sha1,type))-returnerror("char%d: could not verify object %s",7,sha1_to_hex(sha1));--/* Verify the tag-name: we don't allow control characters or spaces in it */-tag_line+=4;-for(;;){-unsignedcharc=*tag_line++;-if(c=='\n')-break;-if(c>' ')-continue;-returnerror("char"PD_FMT": could not verify tag name",tag_line-data);+{+unsignedlongi;++/* Verify that the object matches */+if(verify_object(sha1,type))+returnerror("char%d: could not verify object %s",7,sha1_to_hex(sha1));++/* Verify the tag-name: we don't allow control characters or spaces in it */+for(i=4;;){+unsignedcharc=tag_line[i++];+if(c=='\n')+break;+if(c>' ')+continue;+returnerror("char"PD_FMT": could not verify tag name",tag_line+i-data);+}}/* TODO: check for committer info + blank line? */
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
Rename parse_tag_buffer_internal() to parse_and_verify_tag_buffer() since
it now does tag object verification as well.
Add a new parameter 'thorough_verify' for turning on/off the extra code
to be run when verifying tag objects (as opposed to general parsing).
verify_tag() and parse_and_verify_tag_buffer() are now functionally
equivalent, provided that parse_and_verify_tag_buffer() is called with
item == NULL and thorough_verification != 0.
Signed-off-by: Johan Herland <redacted>
---
tag.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 48 insertions(+), 2 deletions(-)
@@ -79,6 +98,10 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const utagger_line=strchr(tag_line,'\n');if(!tagger_line++)returnerror("char"PD_FMT": could not find next \"\\n\"",tag_line-data);+if(thorough_verify){+if(memcmp(tagger_line,"tagger ",7)||(tagger_line[7]=='\n'))+returnerror("char"PD_FMT": could not find \"tagger\"",tagger_line-data);+}/* Get the actual type */type_len=tag_line-type_line-strlen("type \n");
@@ -87,6 +110,29 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const umemcpy(type,type_line+5,type_len);type[type_len]='\0';+if(thorough_verify){+unsignedlongi;++/* Verify that the object matches */+if(verify_object(sha1,type))+returnerror("char%d: could not verify object %s",7,sha1_to_hex(sha1));++/* Verify the tag-name: we don't allow control characters or spaces in it */+for(i=4;;){+unsignedcharc=tag_line[i++];+if(c=='\n')+break;+if(c>' ')+continue;+returnerror("char"PD_FMT": could not verify tag name",tag_line+i-data);+}++/* Verify the tagger line */+/* TODO: check for committer/tagger info */++/* The actual stuff afterwards we don't care about.. */+}+if(item){tag_len=tagger_line-tag_line-strlen("tag \n");item->tag=xmalloc(tag_len+1);
@@ -120,7 +166,7 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const uintparse_tag_buffer(structtag*item,void*data,unsignedlongsize){-returnparse_tag_buffer_internal(item,(constchar*)data,size);+returnparse_and_verify_tag_buffer(item,(constchar*)data,size,0);}intparse_tag(structtag*item)
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
This involves exposing parse_and_verify_tag_buffer() in the tag API
(tag.h).
Also synchronize selftest with change in error message.
Signed-off-by: Johan Herland <redacted>
---
mktag.c | 5 ++---
t/t3800-mktag.sh | 2 +-
tag.c | 2 +-
tag.h | 2 ++
4 files changed, 6 insertions(+), 5 deletions(-)
@@ -125,9 +125,8 @@ int main(int argc, char **argv)}buffer[size]=0;-/* Verify it for some basic sanity: it needs to start with-"object <sha1>\ntype\ntagger "*/-if(verify_tag(buffer,size)<0)+/* Verify tag object data */+if(parse_and_verify_tag_buffer(0,buffer,size,1))die("invalid tag signature file");if(write_sha1_file(buffer,size,tag_type,result_sha1)<0)
@@ -14,100 +14,6 @@*shortestsingle-character-tagline.*/-/*-*Werefusetotagsomethingwecan'tverify.Justbecause.-*/-staticintverify_object(unsignedchar*sha1,constchar*expected_type)-{-intret=-1;-enumobject_typetype;-unsignedlongsize;-void*buffer=read_sha1_file(sha1,&type,&size);--if(buffer){-if(type==type_from_string(expected_type))-ret=check_sha1_signature(sha1,buffer,size,expected_type);-free(buffer);-}-returnret;-}--staticintverify_tag(char*data,unsignedlongsize)-{-#ifdef NO_C99_FORMAT-#define PD_FMT "%d"-#else-#define PD_FMT "%td"-#endif--unsignedcharsha1[20];-chartype[20];-constchar*type_line,*tag_line,*tagger_line;-unsignedlongtype_len;--if(size<64)-returnerror("wanna fool me ? you obviously got the size wrong !");--/* Verify object line */-if(memcmp(data,"object ",7))-returnerror("char%d: does not start with \"object \"",0);--if(get_sha1_hex(data+7,sha1))-returnerror("char%d: could not get SHA1 hash",7);--/* Verify type line */-type_line=data+48;-if(memcmp(type_line-1,"\ntype ",6))-returnerror("char%d: could not find \"\\ntype \"",47);--/* Verify tag-line */-tag_line=strchr(type_line,'\n');-if(!tag_line++)-returnerror("char"PD_FMT": could not find next \"\\n\"",type_line-data);-if(memcmp(tag_line,"tag ",4)||tag_line[4]=='\n')-returnerror("char"PD_FMT": no \"tag \" found",tag_line-data);--/* Verify the tagger line */-tagger_line=strchr(tag_line,'\n');-if(!tagger_line++)-returnerror("char"PD_FMT": could not find next \"\\n\"",tag_line-data);-if(memcmp(tagger_line,"tagger ",7)||(tagger_line[7]=='\n'))-returnerror("char"PD_FMT": could not find \"tagger\"",tagger_line-data);--/* Get the actual type */-type_len=tag_line-type_line-strlen("type \n");-if(type_len>=sizeof(type))-returnerror("char"PD_FMT": type too long",type_line+5-data);-memcpy(type,type_line+5,type_len);-type[type_len]='\0';--{-unsignedlongi;--/* Verify that the object matches */-if(verify_object(sha1,type))-returnerror("char%d: could not verify object %s",7,sha1_to_hex(sha1));--/* Verify the tag-name: we don't allow control characters or spaces in it */-for(i=4;;){-unsignedcharc=tag_line[i++];-if(c=='\n')-break;-if(c>' ')-continue;-returnerror("char"PD_FMT": could not verify tag name",tag_line+i-data);-}-}--/* TODO: check for committer info + blank line? */-/* Also, the minimum length is probably + "tagger .", or 63+8=71 */--/* The actual stuff afterwards we don't care about.. */-return0;--#undef PD_FMT-}-intmain(intargc,char**argv){unsignedlongsize=4096;
@@ -32,14 +32,17 @@ int main(int argc, char **argv)buffer[size]=0;/* Verify tag object data */-if(parse_and_verify_tag_buffer(0,buffer,size,1))-die("invalid tag signature file");+if(parse_and_verify_tag_buffer(0,buffer,size,1)){+free(buffer);+die("invalid tag data file");+}-if(write_sha1_file(buffer,size,tag_type,result_sha1)<0)+if(write_sha1_file(buffer,size,tag_type,result_sha1)<0){+free(buffer);die("unable to write tag file");+}free(buffer);-printf("%s\n",sha1_to_hex(result_sha1));return0;}
@@ -72,41 +72,50 @@ int parse_and_verify_tag_buffer(struct tag *item,}if(size<64)-returnerror("failed preliminary size check");+returnerror("Tag object failed preliminary size check");/* Verify object line */if(memcmp(data,"object ",7))-returnerror("char%d: does not start with \"object \"",0);+returnerror("Tag object (@ char 0): "+"Does not start with \"object \"");if(get_sha1_hex(data+7,sha1))-returnerror("char%d: could not get SHA1 hash",7);+returnerror("Tag object (@ char 7): Could not get SHA1 hash");/* Verify type line */type_line=data+48;if(memcmp(type_line-1,"\ntype ",6))-returnerror("char%d: could not find \"\\ntype \"",47);+returnerror("Tag object (@ char 47): "+"Could not find \"\\ntype \"");/* Verify tag-line */tag_line=strchr(type_line,'\n');-if(!tag_line)-returnerror("char"PD_FMT": could not find next \"\\n\"",type_line-data);-tag_line++;+if(!tag_line++)+returnerror("Tag object (@ char "PD_FMT"): "+"Could not find \"\\n\" after \"type\"",+type_line-data);if(memcmp(tag_line,"tag ",4)||tag_line[4]=='\n')-returnerror("char"PD_FMT": no \"tag \" found",tag_line-data);+returnerror("Tag object (@ char "PD_FMT"): "+"Could not find \"tag \"",tag_line-data);/* Verify the tagger line */tagger_line=strchr(tag_line,'\n');if(!tagger_line++)-returnerror("char"PD_FMT": could not find next \"\\n\"",tag_line-data);+returnerror("Tag object (@ char "PD_FMT"): "+"Could not find \"\\n\" after \"tag\"",+tag_line-data);if(thorough_verify){if(memcmp(tagger_line,"tagger ",7)||(tagger_line[7]=='\n'))-returnerror("char"PD_FMT": could not find \"tagger\"",tagger_line-data);+returnerror("Tag object (@ char "PD_FMT"): "+"Could not find \"tagger \"",+tagger_line-data);}/* Get the actual type */type_len=tag_line-type_line-strlen("type \n");if(type_len>=sizeof(type))-returnerror("char"PD_FMT": type too long",type_line+5-data);+returnerror("Tag object (@ char "PD_FMT"): "+"Type too long",type_line+5-data);memcpy(type,type_line+5,type_len);type[type_len]='\0';
@@ -115,16 +124,20 @@ int parse_and_verify_tag_buffer(struct tag *item,/* Verify that the object matches */if(verify_object(sha1,type))-returnerror("char%d: could not verify object %s",7,sha1_to_hex(sha1));+returnerror("Tag object (@ char 7): "+"Could not verify tagged object %s",+sha1_to_hex(sha1));-/* Verify the tag-name: we don't allow control characters or spaces in it */+/* Verify tag name: disallow control characters or spaces */for(i=4;;){unsignedcharc=tag_line[i++];if(c=='\n')break;if(c>' ')continue;-returnerror("char"PD_FMT": could not verify tag name",tag_line+i-data);+returnerror("Tag object (@ char "PD_FMT"): "+"Could not verify tag name",+tag_line+i-data);}/* Verify the tagger line */
@@ -148,7 +161,8 @@ int parse_and_verify_tag_buffer(struct tag *item,}elseif(!strcmp(type,tag_type)){item->tagged=&lookup_tag(sha1)->object;}else{-error("Unknown type %s",type);+error("Tag object (@ char "PD_FMT"): "+"Unknown type '%s'",type_line+5-data,type);item->tagged=NULL;}
@@ -75,7 +82,7 @@ int parse_and_verify_tag_buffer(struct tag *item,returnerror("Tag object failed preliminary size check");/* Verify object line */-if(memcmp(data,"object ",7))+if(prefixcmp(data,"object "))returnerror("Tag object (@ char 0): ""Does not start with \"object \"");
@@ -84,7 +91,7 @@ int parse_and_verify_tag_buffer(struct tag *item,/* Verify type line */type_line=data+48;-if(memcmp(type_line-1,"\ntype ",6))+if(prefixcmp(type_line-1,"\ntype "))returnerror("Tag object (@ char 47): ""Could not find \"\\ntype \"");
@@ -94,7 +101,7 @@ int parse_and_verify_tag_buffer(struct tag *item,returnerror("Tag object (@ char "PD_FMT"): ""Could not find \"\\n\" after \"type\"",type_line-data);-if(memcmp(tag_line,"tag ",4)||tag_line[4]=='\n')+if(prefixcmp(tag_line,"tag ")||tag_line[4]=='\n')returnerror("Tag object (@ char "PD_FMT"): ""Could not find \"tag \"",tag_line-data);
@@ -105,7 +112,7 @@ int parse_and_verify_tag_buffer(struct tag *item,"Could not find \"\\n\" after \"tag\"",tag_line-data);if(thorough_verify){-if(memcmp(tagger_line,"tagger ",7)||(tagger_line[7]=='\n'))+if(prefixcmp(tagger_line,"tagger ")||(tagger_line[7]=='\n'))returnerror("Tag object (@ char "PD_FMT"): ""Could not find \"tagger \"",tagger_line-data);
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
For each of the parsed lines we at some point skip past its initial
identifier ("type ", "tag ", etc.). We also at some point calculate the
length of the remaining line. This patch moves these calculations into
one place. This provides _one_ place for all header lines where their
respective pointers start pointing at the header value (instead of the
start of the line), and their lengths are calculated.
Signed-off-by: Johan Herland <redacted>
---
tag.c | 21 ++++++++++++++-------
1 files changed, 14 insertions(+), 7 deletions(-)
@@ -118,12 +118,20 @@ int parse_and_verify_tag_buffer(struct tag *item,tagger_line-data);}+/*+*Advanceheaderfieldpointerspasttheirinitialidentifier.+*Calculatelengthsofheaderfields.+*/+type_line+=strlen("type ");+type_len=tag_line-type_line-1;+tag_line+=strlen("tag ");+tag_len=tagger_line-tag_line-1;+/* Get the actual type */-type_len=tag_line-type_line-strlen("type \n");if(type_len>=sizeof(type))returnerror("Tag object (@ char "PD_FMT"): "-"Type too long",type_line+5-data);-memcpy(type,type_line+5,type_len);+"Type too long",type_line-data);+memcpy(type,type_line,type_len);type[type_len]='\0';if(thorough_verify){
@@ -136,7 +144,7 @@ int parse_and_verify_tag_buffer(struct tag *item,sha1_to_hex(sha1));/* Verify tag name: disallow control characters or spaces */-for(i=4;;){+for(i=0;;){unsignedcharc=tag_line[i++];if(c=='\n')break;
@@ -154,9 +162,8 @@ int parse_and_verify_tag_buffer(struct tag *item,}if(item){-tag_len=tagger_line-tag_line-strlen("tag \n");item->tag=xmalloc(tag_len+1);-memcpy(item->tag,tag_line+4,tag_len);+memcpy(item->tag,tag_line,tag_len);item->tag[tag_len]='\0';if(!strcmp(type,blob_type)){
@@ -169,7 +176,7 @@ int parse_and_verify_tag_buffer(struct tag *item,item->tagged=&lookup_tag(sha1)->object;}else{error("Tag object (@ char "PD_FMT"): "-"Unknown type '%s'",type_line+5-data,type);+"Unknown type '%s'",type_line-data,type);item->tagged=NULL;}
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
Be explicit about the fact that the "tagger" line is now considered a
mandatory part of the tag object. There are however old tags (from before
July 2005) that don't have a "tagger" line. We therefore consider the
"tagger" line _optional_ when parsing tags without thorough_verify set.
In practice this means that verification of a missing "tagger" line will
only fail when adding or fsck-ing tags.
Signed-off-by: Johan Herland <redacted>
---
tag.c | 48 ++++++++++++++++++++++++++++++++++--------------
1 files changed, 34 insertions(+), 14 deletions(-)
@@ -69,8 +69,9 @@ int parse_and_verify_tag_buffer(struct tag *item,unsignedcharsha1[20];chartype[20];-constchar*type_line,*tag_line,*tagger_line;-unsignedlongtype_len,tag_len;+constchar*type_line,*tag_line,*tagger_line;+unsignedlongtype_len,tag_len,tagger_len;+constchar*header_end;if(item){if(item->object.parsed)
@@ -81,7 +82,7 @@ int parse_and_verify_tag_buffer(struct tag *item,if(size<64)returnerror("Tag object failed preliminary size check");-/* Verify object line */+/* Verify mandatory object line */if(prefixcmp(data,"object "))returnerror("Tag object (@ char 0): ""Does not start with \"object \"");
@@ -89,13 +90,13 @@ int parse_and_verify_tag_buffer(struct tag *item,if(get_sha1_hex(data+7,sha1))returnerror("Tag object (@ char 7): Could not get SHA1 hash");-/* Verify type line */+/* Verify mandatory type line */type_line=data+48;if(prefixcmp(type_line-1,"\ntype "))returnerror("Tag object (@ char 47): ""Could not find \"\\ntype \"");-/* Verify tag-line */+/* Verify mandatory tag line */tag_line=strchr(type_line,'\n');if(!tag_line++)returnerror("Tag object (@ char "PD_FMT"): "
@@ -105,27 +106,46 @@ int parse_and_verify_tag_buffer(struct tag *item,returnerror("Tag object (@ char "PD_FMT"): ""Could not find \"tag \"",tag_line-data);-/* Verify the tagger line */+/*+*Verifymandatorytaggerline,butonlywhenwe'rechecking+*thoroughly,i.e.oninsertinganewtag,andonfsck.+*Thereareexistingtagobjectswithoutataggerline(most+*notablythe"v0.99"taginthemaingitrepo),andwedon't+*wanttofailparsingonthese.+*/tagger_line=strchr(tag_line,'\n');if(!tagger_line++)returnerror("Tag object (@ char "PD_FMT"): ""Could not find \"\\n\" after \"tag\"",tag_line-data);-if(thorough_verify){-if(prefixcmp(tagger_line,"tagger ")||(tagger_line[7]=='\n'))+if(prefixcmp(tagger_line,"tagger ")){/* no tagger given */+if(thorough_verify)returnerror("Tag object (@ char "PD_FMT"): ""Could not find \"tagger \"",tagger_line-data);+header_end=tagger_line;+}+else{/* tagger given */+header_end=strchr(tagger_line,'\n');+if(!header_end++)+returnerror("Tag object (@ char "PD_FMT"): "+"Could not find \"\\n\" after \"tagger\"",+tagger_line-data);}/**Advanceheaderfieldpointerspasttheirinitialidentifier.-*Calculatelengthsofheaderfields.+*Calculatelengthsofheaderfields(0forfieldsthatarenotgiven).*/-type_line+=strlen("type ");-type_len=tag_line-type_line-1;-tag_line+=strlen("tag ");-tag_len=tagger_line-tag_line-1;+type_line+=strlen("type ");+type_len=tag_line>type_line?+(tag_line-type_line)-1:0;+tag_line+=strlen("tag ");+tag_len=tagger_line>tag_line?+(tagger_line-tag_line)-1:0;+tagger_line+=strlen("tagger ");+tagger_len=header_end>tagger_line?+(header_end-tagger_line)-1:0;/* Get the actual type */if(type_len>=sizeof(type))
@@ -155,7 +175,7 @@ int parse_and_verify_tag_buffer(struct tag *item,tag_line+i-data);}-/* Verify the tagger line */+/* Verify tagger line *//* TODO: check for committer/tagger info *//* The actual stuff afterwards we don't care about.. */
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
The tag line is now optional. If not given in the tag object data, it
defaults to the empty string ("") in the parsed tag object.
The patch also adds a change to git-show; when asked to display a tag
object with no name (missing "tag" header), we will show the tag's sha1
instead of an empty string.
Finally the patch includes some tweaks to the selftests to make them work
with optional tag names.
Signed-off-by: Johan Herland <redacted>
---
builtin-log.c | 2 +-
t/t3800-mktag.sh | 6 +++---
tag.c | 51 +++++++++++++++++++++++++++++----------------------
tag.h | 2 +-
4 files changed, 34 insertions(+), 27 deletions(-)
@@ -96,15 +96,21 @@ int parse_and_verify_tag_buffer(struct tag *item,returnerror("Tag object (@ char 47): ""Could not find \"\\ntype \"");-/* Verify mandatory tag line */+/* Verify optional tag line */tag_line=strchr(type_line,'\n');if(!tag_line++)returnerror("Tag object (@ char "PD_FMT"): ""Could not find \"\\n\" after \"type\"",type_line-data);-if(prefixcmp(tag_line,"tag ")||tag_line[4]=='\n')-returnerror("Tag object (@ char "PD_FMT"): "-"Could not find \"tag \"",tag_line-data);+if(prefixcmp(tag_line,"tag "))/* no tag name given */+tagger_line=tag_line;+else{/* tag name given */+tagger_line=strchr(tag_line,'\n');+if(!tagger_line++)+returnerror("Tag object (@ char "PD_FMT"): "+"Could not find \"\\n\" after \"tag\"",+tag_line-data);+}/**Verifymandatorytaggerline,butonlywhenwe'rechecking
@@ -113,11 +119,6 @@ int parse_and_verify_tag_buffer(struct tag *item,*notablythe"v0.99"taginthemaingitrepo),andwedon't*wanttofailparsingonthese.*/-tagger_line=strchr(tag_line,'\n');-if(!tagger_line++)-returnerror("Tag object (@ char "PD_FMT"): "-"Could not find \"\\n\" after \"tag\"",-tag_line-data);if(prefixcmp(tagger_line,"tagger ")){/* no tagger given */if(thorough_verify)returnerror("Tag object (@ char "PD_FMT"): "
@@ -164,15 +165,15 @@ int parse_and_verify_tag_buffer(struct tag *item,sha1_to_hex(sha1));/* Verify tag name: disallow control characters or spaces */-for(i=0;;){-unsignedcharc=tag_line[i++];-if(c=='\n')-break;-if(c>' ')-continue;-returnerror("Tag object (@ char "PD_FMT"): "-"Could not verify tag name",-tag_line+i-data);+if(tag_len){/* tag name was given */+for(i=0;i<tag_len;++i){+unsignedcharc=tag_line[i];+if(c>' '&&c!=0x7f)+continue;+returnerror("Tag object (@ char "PD_FMT"): "+"Could not verify tag name",+tag_line+i-data);+}}/* Verify tagger line */
@@ -181,10 +182,16 @@ int parse_and_verify_tag_buffer(struct tag *item,/* The actual stuff afterwards we don't care about.. */}-if(item){-item->tag=xmalloc(tag_len+1);-memcpy(item->tag,tag_line,tag_len);-item->tag[tag_len]='\0';+if(item){/* Store parsed information into item */+if(tag_len){/* optional tag name was given */+item->tag=xmalloc(tag_len+1);+memcpy(item->tag,tag_line,tag_len);+item->tag[tag_len]='\0';+}+else{/* optional tag name not given */+item->tag=xmalloc(1);+item->tag[0]='\0';+}if(!strcmp(type,blob_type)){item->tagged=&lookup_blob(sha1)->object;
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
This patch introduces a new optional header line to the tag object, called
"keywords". The "keywords" line may contain a comma-separated list of
custom keywords associated with the tag object. There are two "special"
keywords, however: "tag" and "note": When the "keywords" header is
missing, its default value is set to "tag" if a "tag" header is
present; else the default "keywords" value is set to "note". The
"keywords" header is meant to be used by porcelains for classifying
different types of tag objects. This classification may then be used to
filter tag objects in the presentation layer (e.g. by implementing
extra filter options to --decorate, etc.)
Signed-off-by: Johan Herland <redacted>
---
tag.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
tag.h | 1 +
2 files changed, 54 insertions(+), 7 deletions(-)
@@ -69,8 +69,8 @@ int parse_and_verify_tag_buffer(struct tag *item,unsignedcharsha1[20];chartype[20];-constchar*type_line,*tag_line,*tagger_line;-unsignedlongtype_len,tag_len,tagger_len;+constchar*type_line,*tag_line,*keywords_line,*tagger_line;+unsignedlongtype_len,tag_len,keywords_len,tagger_len;constchar*header_end;if(item){
@@ -103,15 +103,26 @@ int parse_and_verify_tag_buffer(struct tag *item,"Could not find \"\\n\" after \"type\"",type_line-data);if(prefixcmp(tag_line,"tag "))/* no tag name given */-tagger_line=tag_line;+keywords_line=tag_line;else{/* tag name given */-tagger_line=strchr(tag_line,'\n');-if(!tagger_line++)+keywords_line=strchr(tag_line,'\n');+if(!keywords_line++)returnerror("Tag object (@ char "PD_FMT"): ""Could not find \"\\n\" after \"tag\"",tag_line-data);}+/* Verify optional keywords line */+if(prefixcmp(keywords_line,"keywords "))/* no keywords given */+tagger_line=keywords_line;+else{/* keywords given */+tagger_line=strchr(keywords_line,'\n');+if(!tagger_line++)+returnerror("Tag object (@ char "PD_FMT"): "+"Could not find \"\\n\" after \"keywords\"",+keywords_line-data);+}+/**Verifymandatorytaggerline,butonlywhenwe'rechecking*thoroughly,i.e.oninsertinganewtag,andonfsck.
@@ -142,8 +153,11 @@ int parse_and_verify_tag_buffer(struct tag *item,type_len=tag_line>type_line?(tag_line-type_line)-1:0;tag_line+=strlen("tag ");-tag_len=tagger_line>tag_line?-(tagger_line-tag_line)-1:0;+tag_len=keywords_line>tag_line?+(keywords_line-tag_line)-1:0;+keywords_line+=strlen("keywords ");+keywords_len=tagger_line>keywords_line?+(tagger_line-keywords_line)-1:0;tagger_line+=strlen("tagger ");tagger_len=header_end>tagger_line?(header_end-tagger_line)-1:0;
@@ -176,6 +190,26 @@ int parse_and_verify_tag_buffer(struct tag *item,}}+/*+*Verifykeywords:disallowcontrolcharacters,spaces,+*ortwosubsequentcommas+*/+if(keywords_len){/* keywords line was given */+for(i=0;i<keywords_len;++i){+unsignedcharc=keywords_line[i];+if(c==','&&keywords_line[i+1]==',')+/* consecutive commas */+returnerror("Tag object (@ char "+PD_FMT"): Found empty keyword",+keywords_line+i-data);+if(c>' '&&c!=0x7f)+continue;+returnerror("Tag object (@ char "PD_FMT"): "+"Could not verify keywords",+keywords_line+i-data);+}+}+/* Verify tagger line *//* TODO: check for committer/tagger info */
@@ -193,6 +227,18 @@ int parse_and_verify_tag_buffer(struct tag *item,item->tag[0]='\0';}+if(keywords_len){/* optional keywords string was given */+item->keywords=xmalloc(keywords_len+1);+memcpy(item->keywords,keywords_line,keywords_len);+item->keywords[keywords_len]='\0';+}+else{/* optional keywords string not given. Set default */+/* if tag name is set, use "tag"; else use "note" */+constchar*default_kw=item->tag?"tag":"note";+item->keywords=xmalloc(strlen(default_kw)+1);+memcpy(item->keywords,default_kw,strlen(default_kw)+1);+}+if(!strcmp(type,blob_type)){item->tagged=&lookup_blob(sha1)->object;}elseif(!strcmp(type,tree_type)){
@@ -9,6 +9,7 @@ struct tag {structobjectobject;structobject*tagged;char*tag;/* optional, may be empty ("") */+char*keywords;/* optional, defaults to tag ? "tag" : "note" */char*signature;/* not actually implemented */};
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
Also update minimum tag object length to the new minimum length after refactoring.
Signed-off-by: Johan Herland <redacted>
---
mktag.c | 30 ++++++++++++++++++++++--------
tag.c | 2 +-
2 files changed, 23 insertions(+), 9 deletions(-)
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
Teach git-fsck to do the same kind of verification on tag objects that is
already done by git-mktag.
Signed-off-by: Johan Herland <redacted>
---
builtin-fsck.c | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
@@ -359,11 +359,26 @@ static int fsck_commit(struct commit *commit)staticintfsck_tag(structtag*tag){structobject*tagged=tag->tagged;+enumobject_typetype;+unsignedlongsize;+char*data=(char*)read_sha1_file(tag->object.sha1,&type,&size);if(verbose)fprintf(stderr,"Checking tag %s\n",sha1_to_hex(tag->object.sha1));+if(!data)+returnobjerror(&tag->object,"could not read tag");+if(type!=OBJ_TAG){+free(data);+returnobjerror(&tag->object,"not a tag (internal error)");+}+if(parse_and_verify_tag_buffer(0,data,size,1)){/* thoroughly verify tag object */+free(data);+returnobjerror(&tag->object,"failed thorough tag object verification");+}+free(data);+if(!tagged){returnobjerror(&tag->object,"could not load tagged object");}
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
The new structure of tag objects is documented.
Also some much-needed cleanup is done. E.g. remove the paragraph on the
8kB limit, since this limit was removed ages ago.
Signed-off-by: Johan Herland <redacted>
---
Documentation/git-mktag.txt | 41 ++++++++++++++++++++++++++++++-----------
1 files changed, 30 insertions(+), 11 deletions(-)
@@ -8,38 +8,57 @@ git-mktag - Creates a tag object SYNOPSIS ---------'git-mktag' < signature_file+[verse]+'git-mktag' < tag_data_file+ DESCRIPTION ------------Reads a tag contents on standard input and creates a tag object+Reads tag object data on standard input and creates a tag object that can also be used to sign other objects. The output is the new tag's <object> identifier.-Tag Format++DISCUSSION -----------A tag signature file has a very simple fixed format: three lines of+Tag object data has the following format+[verse] object <sha1> type <typename>- tag <tagname>+ tag <tagname> (optional)+ keywords <keywords> (optional)+ tagger <committer>++followed by a blank line and a free-form message and an optional signature+that git itself doesn't care about, but that may be verified with gpg or+similar.-followed by some 'optional' free-form signature that git itself-doesn't care about, but that can be verified with gpg or similar.+In the above listing, `<sha1>` represents the object pointed to by this tag,+`<typename>` is the type of the object pointed to ("tag", "blob", "tree" or+"commit"), `<tagname>` is the name of this tag object (and must correspond+to the name of the corresponding ref (if any) in `.git/refs/`). `<keywords>`+is a comma-separated list of keywords associated with this tag object, and+`<committer>` holds the "`name <email>`" of the tag creator and timestamp+of when the tag object was created (analogous to "committer" in commit+objects).-The size of the full object is artificially limited to 8kB. (Just-because I'm a lazy bastard, and if you can't fit a signature in that-size, you're doing something wrong)+If "`tag <tagname>`" is omitted, <tagname> defaults to the empty string.+If "`keywords <keywords>`" is omitted, <keywords> defaults to "`tag`" if+a <tagname> was given, "`note`" otherwise. Author ------ Written by Linus Torvalds <torvalds@osdl.org>+ Documentation ---------------Documentation by David Greaves, Junio C Hamano and the git-list <git@vger.kernel.org>.+Documentation by Johan Herland, David Greaves, Junio C Hamano and the+git-list <git@vger.kernel.org>.+ GIT ---
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
Some more tests are added to test the new "keywords" header, and to test
the more thorough verification routine.
Signed-off-by: Johan Herland <redacted>
---
t/t3800-mktag.sh | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 200 insertions(+), 12 deletions(-)
@@ -127,13 +138,15 @@ EOF check_verify_failure'"tag" line label check #2'############################################################-# 8. type line type-name length check+# 8. type line type name length check cat>tag.sig<<EOF object779e9b33986b1c2670fff52c5067603117b3e895typetaggggggggggggggggggggggggggggggg tagmytag-taggera+keywordsfoo+taggerbar@baz.com+ EOF cat>expect.pat<<EOF
From: Johan Herland <hidden> Date: 2016-06-15 22:43:14
On Monday 28 May 2007, Junio C Hamano wrote:
However it would be a good
idea to add logic to fsck to warn upon inconsistencis (perhaps
by mistake) between refname and tag's true name.
The check would say something like:
If an annotated (signed or unsigned) tag has a "tag"
line to give it the official $name, and if it is pointed
at by a ref, the refname must end with "/$name".
Otherwise we warn.
Trivially, the above rule says that having v2.6.22 tag under
refs/tags/v2.6.20 is a mistake we would want to be warned upon.
This patch adds the check described by Junio.
It also includes a bugfix when tag object parsing fails, from
Johannes Schindelin [off-list ref].
Signed-off-by: Johan Herland <redacted>
---
builtin-fsck.c | 26 ++++++++++++++++++++++++++
1 files changed, 26 insertions(+), 0 deletions(-)
@@ -542,6 +542,30 @@ static int fsck_handle_reflog(const char *logname, const unsigned char *sha1, inreturn0;}+staticvoidfsck_verify_ref_to_tag_object(constchar*refname,structobject*obj)+{+/* Verify that refname matches the name stored in obj's "tag" header */+structtag*tagobj=(structtag*)parse_object(obj->sha1);+size_ttagname_len;+size_trefname_len=strlen(refname);++if(!tagobj->tag){+error("%s: Failed to parse tag object %s",refname,sha1_to_hex(obj->sha1));+return;+}+tagname_len=strlen(tagobj->tag);+if(!tagname_len)return;/* No tag name stored in tagobj. Nothing to do. */++if(tagname_len<refname_len&&+!memcmp(tagobj->tag,refname+(refname_len-tagname_len),tagname_len)&&+refname[(refname_len-tagname_len)-1]=='/'){+/* OK: tag name is "$name", and refname ends with "/$name" */+return;+}+else+error("%s: Mismatch between tag ref and tag object's name: '%s'",refname,tagobj->tag);+}+staticintfsck_handle_ref(constchar*refname,constunsignedchar*sha1,intflag,void*cb_data){structobject*obj;
@@ -556,6 +580,8 @@ static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int f/* We'll continue with the rest despite the error.. */return0;}+if(obj->type==OBJ_TAG)/* ref to tag object */+fsck_verify_ref_to_tag_object(refname,obj);default_refs++;obj->used=1;mark_reachable(obj,REACHABLE);
Quite a lot of changes seem to do this object->data. The patch would have
been much more compact if you just had renamed buffer to object instead of
data.
This renaming variables has nothing to do with refactoring. In fact, I
have a hard time to find code changes (which your subject suggests, as you
want to make two functions more similar).
/* TODO: check for committer info + blank line? */
/* Also, the minimum length is probably + "tagger .", or 63+8=71 */
/* The actual stuff afterwards we don't care about.. */
return 0;
-}
#undef PD_FMT
+}
Any particular reason for this?
quoted hunk
@@ -124,6 +120,7 @@ int main(int argc, char **argv) free(buffer); die("could not read from stdin"); }+ buffer[size] = 0;
Ah, so you terminate the buffer here. From the patch, it is relatively
hard to see if this line is always hit _before_ the function is called
that evidently relies on NUL termination. By moving it here, I think it is
much more likely to overlook the fact that the function, which made sure
that its assumption was met, needs this line. Whereas if you left it where
it was, the assumption would always be met.
- if (item->object.parsed)
- return 0;
- item->object.parsed = 1;
+ if (item->object.parsed)
+ return 0;
+ item->object.parsed = 1;
Again, this has nothing to do with refactoring.
quoted hunk
@@ -57,39 +57,38 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size) if (memcmp(data, "object ", 7)) return error("char%d: does not start with \"object \"", 0);- if (get_sha1_hex((char *) data + 7, sha1))+ if (get_sha1_hex(data + 7, sha1))
Is this really necessary? Even if (which I doubt), it has nothing to do
with refactoring.
If you _want_ to _explicitely_ do arithmetic on a char* instead of void*,
why not DRT and change the function signature?
- sig_line++;
+ tagger_line++;
I am really reluctant with renamings like these. IMHO they don't buy you
much, except for possible confusion. It is evident that sig means the
signer (and it is obvious in the case of an unsigned tag, who is meant,
too).
+int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
+{
+ return parse_tag_buffer_internal(item, (const char *) data, size);
+}
This cast (and indeed, this function, if you ask me) is unnecessary.
I reviewed only this patch out of your long series, mostly because I found
the subject line interesting. But IMHO the patch does not what the subject
line suggests.
Unfortunately, it's unlikely that I will have time until Monday night to
continue with this patch series.
Ciao,
Dscho
From: Johan Herland <hidden> Date: 2016-06-15 22:43:15
On Saturday 09 June 2007, Johannes Schindelin wrote:
Hi,
Hi. Thanks for taking the time to look at (some of) my patch. Most of your
questions below can be answered with a single answer:
The main purpose of the patch is (as the subject line says) to bring the
two functions more in line with eachother. At the time I made the patch,
I had made the observation that these function were trying to do much the
same thing, albeit in a slightly different form. This patch is therefore
about applying a series of (mostly non-functional) refactorings to make
their diff as small as possible. This involves "stupid" changes such as
renaming variables, tweaking whitespace, reordering the declaration of
variables, etc. It's all to make the functions similar to the point where
I can diff them, get a small and meaningful result, see the remaining
_real_ differences, and in the end, _merge_ them (see patches 7-9).
If this whole exercise didn't end up with merging the two functions into
one, I would _totally_ agree with you that all this refactoring is more
harmful than beneficial.
On Sat, 9 Jun 2007, Johan Herland wrote:
quoted
if (size < 64)
return error("wanna fool me ? you obviously got the size wrong !");
- buffer[size] = 0;
Are you sure that your buffer is always NUL terminated?
First, (and you'll see this in the commit message) I'm _moving_ (not
removing) the NUL termination out of verify_tag() and into main() (which I
can be sure is the only caller of verify_tag(), since verify_tag is
declared static, and there is no other call in that file). Two reasons for
doing this:
1. Make verify_tag more similar to parse_tag_buffer() (because
parse_tag_buffer() does not NUL terminate)
2. Do the NUL termination as close to the code that actually populated the
buffer with data (the read_pipe() in main())
So now you can ask: Why doesn't parse_tag_buffer() NUL terminate its
input? It _that_ safe? And I ran around checking all the callers of
parse_tag_buffer, and found that all of them use data (most of which
originates from read_sha1_file()) that's already NUL terminated.
In the end, I also put in a comment on the resulting function
(parse_and_verify_tag_buffer()), explicitly saying the given data _must_
be NUL terminated.
Side note: At first I actually thought the manual NUL termination
could cause a buffer overflow (i.e. if the given size was the same as the
allocated size), so I actually have a version of all of this where I
_don't_ assume the buffer is NUL-terminated at all, and put in lots of
bounds checking, replace strchr() with memchr(), etc.
I then took a hard look at read_pipe(), and discovered that if you
use it to fill a 4096-byte buffer with 4096 bytes of data, it actually
_will_ reallocate to 8192 bytes and leave room for the NUL termination
(and much more) (I believe this should have been documented in read_pipe).
Thus the NUL termination was safe all along.
Quite a lot of changes seem to do this object->data. The patch would have
been much more compact if you just had renamed buffer to object instead of
data.
Yes, but it would have made the aforementioned diff to parse_tag_buffer()
larger.
quoted
/* TODO: check for committer info + blank line? */
/* Also, the minimum length is probably + "tagger .", or 63+8=71 */
/* The actual stuff afterwards we don't care about.. */
return 0;
-}
#undef PD_FMT
+}
Any particular reason for this?
Well, PD_FMT is only used inside the function, so I found it easier to
move the #definition of PD_FMT inside the function to indicate the scope
(_perceived_ scope; I know it hasn't any effect on the compiled code).
But since the whole function is going away in a few patches anyway,
I should have probably left it out of this patch entirely.
quoted
@@ -124,6 +120,7 @@ int main(int argc, char **argv) free(buffer); die("could not read from stdin"); }+ buffer[size] = 0;
Ah, so you terminate the buffer here. From the patch, it is relatively
hard to see if this line is always hit _before_ the function is called
that evidently relies on NUL termination. By moving it here, I think it is
much more likely to overlook the fact that the function, which made sure
that its assumption was met, needs this line. Whereas if you left it where
it was, the assumption would always be met.
But if I leave the NUL termination within the function I would have to
backtrack out of the function to all of its potential callers and check
whether it's safe to write to index size. Since the word "size" could
easily mean "allocated size" I would have the initial feeling that this
might be a buffer overflow, i.e. _not_ safe.
In the end, I think the best solution is to make sure NUL termination
happens before calling the function, and then documenting explicitly
that the function assumes NUL terminated input. Which is exactly what
I end up with at the end of the patch series.
quoted
- sig_line++;
+ tagger_line++;
I am really reluctant with renamings like these. IMHO they don't buy you
much, except for possible confusion. It is evident that sig means the
signer (and it is obvious in the case of an unsigned tag, who is meant,
too).
Hmm. The "type" line is found in the variable type_line, the "tag" line is
found in the variable tag_line, and the "tagger" line is found in the
variable ... sig_line? Nope, I don't buy it.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Junio C Hamano <hidden> Date: 2016-06-15 22:43:15
Johan Herland [off-list ref] writes:
This patch brings the already similar tag.c:parse_tag_buffer() and
mktag.c:verify_tag() a little bit closer to eachother.
While I would agree that it makes sense to have the same
definition of what is and is not a 100% well formatted tag
object for producer side and consumer side, I do not necessarily
think it is a good idea to make parse_tag_buffer() chattier on
errors. mktag.c:verify_tag() can afford to be verbose in its
diagnosis, because it is used when the user is _creating_ the
tag, and it is generally a good idea to be strict when we
create.
On the other hand, parse_tag_buffer() is on the side of users
who use existing tag objects that were produced by somebody
else. It is generally a good practice to be more lenient when
you are consuming.
Also, callers of parse_tag_buffer() know the function is silent
on errors (unless there is something seriously wrong with the
repository); they do their own error messages when they get an
error return.
From: Junio C Hamano <hidden> Date: 2016-06-15 22:43:15
Johan Herland [off-list ref] writes:
quoted hunk
This is in preparation for unifying verify_tag() and
parse_tag_buffer_internal().
Signed-off-by: Johan Herland <redacted>
---
tag.c | 54 +++++++++++++++++++++++++++++-------------------------
1 files changed, 29 insertions(+), 25 deletions(-)
@@ -46,9 +46,11 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const uconstchar*type_line,*tag_line,*tagger_line;unsignedlongtype_len,tag_len;-if(item->object.parsed)-return0;-item->object.parsed=1;+if(item){+if(item->object.parsed)+return0;+item->object.parsed=1;+}if(size<64)returnerror("failed preliminary size check");
Passing both item and data does not feel right. If you are
trying to make the factored out function to do the verification
of data, then perhaps the caller should do the "don't handle the
same data twice" optimization using item?
From: Junio C Hamano <hidden> Date: 2016-06-15 22:43:15
Johan Herland [off-list ref] writes:
quoted hunk
Also update selftests to reflect that verification of "tagger" now
happens _before_ verification of type name, object sha1 and tag name.
Signed-off-by: Johan Herland <redacted>
---
mktag.c | 16 ++++++++--------
t/t3800-mktag.sh | 3 +++
tag.c | 6 +++---
3 files changed, 14 insertions(+), 11 deletions(-)
@@ -62,12 +62,18 @@ static int verify_tag(char *data, unsigned long size)/* Verify tag-line */tag_line=strchr(type_line,'\n');-if(!tag_line)+if(!tag_line++)returnerror("char"PD_FMT": could not find next \"\\n\"",type_line-data);-tag_line++;
Code churn "while we are at it" makes reviewing the rest more
cumbersome. A clean-up like this should be a separate patch.
@@ -73,10 +73,10 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const uif(memcmp(tag_line,"tag ",4)||tag_line[4]=='\n')returnerror("char"PD_FMT": no \"tag \" found",tag_line-data);+/* Verify the tagger line */tagger_line=strchr(tag_line,'\n');-if(!tagger_line)-return-1;-tagger_line++;+if(!tagger_line++)+returnerror("char"PD_FMT": could not find next \"\\n\"",tag_line-data);/* Get the actual type */type_len=tag_line-type_line-strlen("type \n");
From: Johan Herland <hidden> Date: 2016-06-15 22:43:15
On Saturday 09 June 2007, Junio C Hamano wrote:
Johan Herland [off-list ref] writes:
quoted
This patch brings the already similar tag.c:parse_tag_buffer() and
mktag.c:verify_tag() a little bit closer to eachother.
While I would agree that it makes sense to have the same
definition of what is and is not a 100% well formatted tag
object for producer side and consumer side, I do not necessarily
think it is a good idea to make parse_tag_buffer() chattier on
errors. mktag.c:verify_tag() can afford to be verbose in its
diagnosis, because it is used when the user is _creating_ the
tag, and it is generally a good idea to be strict when we
create.
On the other hand, parse_tag_buffer() is on the side of users
who use existing tag objects that were produced by somebody
else. It is generally a good practice to be more lenient when
you are consuming.
Also, callers of parse_tag_buffer() know the function is silent
on errors (unless there is something seriously wrong with the
repository); they do their own error messages when they get an
error return.
Ok. I can make the error messages conditional on 'thorough_verify'.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Johan Herland <hidden> Date: 2016-06-15 22:43:15
We don't want to print error message on regular parsing of tag objects.
With this patch error messages are only printed when 'thorough_verify'
is set, i.e. when creating new or fscking tag objects.
Cc: Junio C Hamano <redacted>
Signed-off-by: Johan Herland <redacted>
---
On Saturday 09 June 2007, Johan Herland wrote:
On Saturday 09 June 2007, Junio C Hamano wrote:
quoted
While I would agree that it makes sense to have the same
definition of what is and is not a 100% well formatted tag
object for producer side and consumer side, I do not necessarily
think it is a good idea to make parse_tag_buffer() chattier on
errors. mktag.c:verify_tag() can afford to be verbose in its
diagnosis, because it is used when the user is _creating_ the
tag, and it is generally a good idea to be strict when we
create.
On the other hand, parse_tag_buffer() is on the side of users
who use existing tag objects that were produced by somebody
else. It is generally a good practice to be more lenient when
you are consuming.
Also, callers of parse_tag_buffer() know the function is silent
on errors (unless there is something seriously wrong with the
repository); they do their own error messages when they get an
error return.
Ok. I can make the error messages conditional on 'thorough_verify'.
Is this one ok?
(It goes on top of the patch series as a whole)
...Johan
tag.c | 34 +++++++++++++++++++---------------
1 files changed, 19 insertions(+), 15 deletions(-)
@@ -67,6 +67,8 @@ int parse_and_verify_tag_buffer(struct tag *item,#define PD_FMT "%td"#endif+#define FAIL(...) ( thorough_verify ? error(__VA_ARGS__) : -1 )+unsignedcharsha1[20];chartype[20];constchar*type_line,*tag_line,*keywords_line,*tagger_line;
@@ -80,26 +82,26 @@ int parse_and_verify_tag_buffer(struct tag *item,}if(size<65)-returnerror("Tag object failed preliminary size check");+returnFAIL("Tag object failed preliminary size check");/* Verify mandatory object line */if(prefixcmp(data,"object "))-returnerror("Tag object (@ char 0): "+returnFAIL("Tag object (@ char 0): ""Does not start with \"object \"");if(get_sha1_hex(data+7,sha1))-returnerror("Tag object (@ char 7): Could not get SHA1 hash");+returnFAIL("Tag object (@ char 7): Could not get SHA1 hash");/* Verify mandatory type line */type_line=data+48;if(prefixcmp(type_line-1,"\ntype "))-returnerror("Tag object (@ char 47): "+returnFAIL("Tag object (@ char 47): ""Could not find \"\\ntype \"");/* Verify optional tag line */tag_line=strchr(type_line,'\n');if(!tag_line++)-returnerror("Tag object (@ char "PD_FMT"): "+returnFAIL("Tag object (@ char "PD_FMT"): ""Could not find \"\\n\" after \"type\"",type_line-data);if(prefixcmp(tag_line,"tag "))/* no tag name given */
@@ -107,7 +109,7 @@ int parse_and_verify_tag_buffer(struct tag *item,else{/* tag name given */keywords_line=strchr(tag_line,'\n');if(!keywords_line++)-returnerror("Tag object (@ char "PD_FMT"): "+returnFAIL("Tag object (@ char "PD_FMT"): ""Could not find \"\\n\" after \"tag\"",tag_line-data);}
@@ -118,7 +120,7 @@ int parse_and_verify_tag_buffer(struct tag *item,else{/* keywords given */tagger_line=strchr(keywords_line,'\n');if(!tagger_line++)-returnerror("Tag object (@ char "PD_FMT"): "+returnFAIL("Tag object (@ char "PD_FMT"): ""Could not find \"\\n\" after \"keywords\"",keywords_line-data);}
@@ -132,7 +134,7 @@ int parse_and_verify_tag_buffer(struct tag *item,*/if(prefixcmp(tagger_line,"tagger ")){/* no tagger given */if(thorough_verify)-returnerror("Tag object (@ char "PD_FMT"): "+returnFAIL("Tag object (@ char "PD_FMT"): ""Could not find \"tagger \"",tagger_line-data);header_end=tagger_line;
@@ -140,7 +142,7 @@ int parse_and_verify_tag_buffer(struct tag *item,else{/* tagger given */header_end=strchr(tagger_line,'\n');if(!header_end++)-returnerror("Tag object (@ char "PD_FMT"): "+returnFAIL("Tag object (@ char "PD_FMT"): ""Could not find \"\\n\" after \"tagger\"",tagger_line-data);}
@@ -164,7 +166,7 @@ int parse_and_verify_tag_buffer(struct tag *item,/* Get the actual type */if(type_len>=sizeof(type))-returnerror("Tag object (@ char "PD_FMT"): "+returnFAIL("Tag object (@ char "PD_FMT"): ""Type too long",type_line-data);memcpy(type,type_line,type_len);type[type_len]='\0';
@@ -174,7 +176,7 @@ int parse_and_verify_tag_buffer(struct tag *item,/* Verify that the object matches */if(verify_object(sha1,type))-returnerror("Tag object (@ char 7): "+returnFAIL("Tag object (@ char 7): ""Could not verify tagged object %s",sha1_to_hex(sha1));
@@ -184,7 +186,7 @@ int parse_and_verify_tag_buffer(struct tag *item,unsignedcharc=tag_line[i];if(c>' '&&c!=0x7f)continue;-returnerror("Tag object (@ char "PD_FMT"): "+returnFAIL("Tag object (@ char "PD_FMT"): ""Could not verify tag name",tag_line+i-data);}
@@ -199,12 +201,12 @@ int parse_and_verify_tag_buffer(struct tag *item,unsignedcharc=keywords_line[i];if(c==','&&keywords_line[i+1]==',')/* consecutive commas */-returnerror("Tag object (@ char "+returnFAIL("Tag object (@ char "PD_FMT"): Found empty keyword",keywords_line+i-data);if(c>' '&&c!=0x7f)continue;-returnerror("Tag object (@ char "PD_FMT"): "+returnFAIL("Tag object (@ char "PD_FMT"): ""Could not verify keywords",keywords_line+i-data);}
@@ -248,7 +250,7 @@ int parse_and_verify_tag_buffer(struct tag *item,}elseif(!strcmp(type,tag_type)){item->tagged=&lookup_tag(sha1)->object;}else{-error("Tag object (@ char "PD_FMT"): "+FAIL("Tag object (@ char "PD_FMT"): ""Unknown type '%s'",type_line-data,type);item->tagged=NULL;}
@@ -262,6 +264,8 @@ int parse_and_verify_tag_buffer(struct tag *item,return0;+#undef FAIL+#undef PD_FMT}
From: Alex Riesen <hidden> Date: 2016-06-15 22:43:15
On 6/9/07, Johan Herland [off-list ref] wrote:
+ /* Verify the tag-name: we don't allow control characters or spaces in it */
+ for (i = 4;;) {
+ unsigned char c = tag_line[i++];
+ if (c == '\n')
+ break;
+ if (c > ' ')
+ continue;
+ return error("char" PD_FMT ": could not verify tag name", tag_line + i - data);
+ }
This looks very familiar. Haven't you just made a very useless patch
which had this very same code? How about putting it in its own
function and just call it from these two places? And what problem
do you have with pointers?!
What is this change good for?
How did you justify the type selection for your
loop index variable?
IOW, the patch looks very useless.
I agree. By itself, the patch is useless.
However, if you look at the next patch, you'll see that this exact piece of
code is moved from verify_tag() to parse_and_verify_tag_buffer(), and in
the new context, we can't increment tag_line, since the code that follows
depends on tag_line not being moved.
In other words this patch is here so that the next patch will be easier to
follow. because it's _literally_ moving copying code from verify_tag() and
pasting it in parse_and_verify_tag_buffer().
I'm sorry if this is not clear from the patches.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Alex Riesen <hidden> Date: 2016-06-15 22:43:15
On 6/9/07, Johan Herland [off-list ref] wrote:
+ if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {
+ free(buffer);
+ die("invalid tag data file");
This, and the similar one below are useless. You're destroying the
process, what do you free that buffer for? Either handle the error
case or do not needlessly complicate your change, which really
also absolutely unneeded.
From: Johan Herland <hidden> Date: 2016-06-15 22:43:15
On Saturday 09 June 2007, Alex Riesen wrote:
On 6/9/07, Johan Herland [off-list ref] wrote:
quoted
+ /* Verify the tag-name: we don't allow control characters or spaces in it */
+ for (i = 4;;) {
+ unsigned char c = tag_line[i++];
+ if (c == '\n')
+ break;
+ if (c > ' ')
+ continue;
+ return error("char" PD_FMT ": could not verify tag name", tag_line + i - data);
+ }
This looks very familiar. Haven't you just made a very useless patch
which had this very same code? How about putting it in its own
function and just call it from these two places? And what problem
do you have with pointers?!
I just answered your comment on the previous patch, and that answer should
apply here as well.
I'm probably splitting this up into too small pieces, since I keep getting
comments that fail to see the overall picture of what I'm trying to do,
namely taking two similar pieces of code and slowly unifying them to the
point where I can replace one of them by a call to the other (see the two
next patches).
Hope this helps,
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Alex Riesen <hidden> Date: 2016-06-15 22:43:15
On 6/9/07, Johan Herland [off-list ref] wrote:
+/*
+ * Perform parsing and verification of tag object data.
+ *
+ * The 'item' parameter may be set to NULL if only verification is desired.
+ *
+ * The given data _must_ be null-terminated.
+ */
int parse_and_verify_tag_buffer(struct tag *item,
const char *data, const unsigned long size, int thorough_verify)
This hunk really belongs into commit which introduced the function
parse_and_verify_tag_buffer.
So, you do some useless changes just to remove the
function completely afterwards?
Yes. Basically so that people can follow my process. If you don't want the
intermediary/useless states, just look at my first patch series that was
replaced by this series because it was too bulky and disruptive to follow.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Johan Herland <hidden> Date: 2016-06-15 22:43:15
On Saturday 09 June 2007, Alex Riesen wrote:
On 6/9/07, Johan Herland [off-list ref] wrote:
quoted
+ if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {
+ free(buffer);
+ die("invalid tag data file");
This, and the similar one below are useless. You're destroying the
process, what do you free that buffer for? Either handle the error
case or do not needlessly complicate your change, which really
also absolutely unneeded.
Well, I was taught to treat my memory with care.
Right now it doesn't make any difference in practice (except that
Valgrind might be a bit happier with it), but in the future -- with
the libifaction effort and whatnot -- you never know what might happen
to this piece of code, and I'd like to stay on the safe side.
Feel free to drop this patch from the series if I'm the only one thinking
like this.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Johan Herland <hidden> Date: 2016-06-15 22:43:15
On Saturday 09 June 2007, Alex Riesen wrote:
On 6/9/07, Johan Herland [off-list ref] wrote:
quoted
+/*
+ * Perform parsing and verification of tag object data.
+ *
+ * The 'item' parameter may be set to NULL if only verification is desired.
+ *
+ * The given data _must_ be null-terminated.
+ */
int parse_and_verify_tag_buffer(struct tag *item,
const char *data, const unsigned long size, int thorough_verify)
This hunk really belongs into commit which introduced the function
parse_and_verify_tag_buffer.
Yes. I'm sorry it slipped out of that patch and into this one.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Alex Riesen <hidden> Date: 2016-06-15 22:43:15
On 6/9/07, Johan Herland [off-list ref] wrote:
This patch introduces a new optional header line to the tag object, called
"keywords". The "keywords" line may contain a comma-separated list of
custom keywords associated with the tag object.
What is the character set for the keywords?
+ for (i = 0; i < keywords_len; ++i) {
+ unsigned char c = keywords_line[i];
+ if (c == ',' && keywords_line[i + 1] == ',')
+ /* consecutive commas */
+ return error("Tag object (@ char "
+ PD_FMT "): Found empty keyword",
+ keywords_line + i - data);
+ if (c > ' ' && c != 0x7f)
+ continue;
And what is so special about 0x7f?
+ if (keywords_len) { /* optional keywords string was given */
+ item->keywords = xmalloc(keywords_len + 1);
Who frees the keywords and what's wrong with strndup?
From: Alex Riesen <hidden> Date: 2016-06-15 22:43:15
On 6/9/07, Johan Herland [off-list ref] wrote:
On Saturday 09 June 2007, Alex Riesen wrote:
quoted
On 6/9/07, Johan Herland [off-list ref] wrote:
quoted
+ if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {
+ free(buffer);
+ die("invalid tag data file");
This, and the similar one below are useless. You're destroying the
process, what do you free that buffer for? Either handle the error
case or do not needlessly complicate your change, which really
also absolutely unneeded.
Well, I was taught to treat my memory with care.
How do you treat your performance?
Besides, was that systems with common address space
where you were taught? Like DOS or MacOS, perhaps?
Right now it doesn't make any difference in practice (except that
Valgrind might be a bit happier with it), but in the future -- with
the libifaction effort and whatnot -- you never know what might happen
to this piece of code, and I'd like to stay on the safe side.
So that people have to check your free as well (they will have to,
they come looking for die-calls). You just made more work for them.
From: Johan Herland <hidden> Date: 2016-06-15 22:43:15
On Saturday 09 June 2007, Alex Riesen wrote:
On 6/9/07, Johan Herland [off-list ref] wrote:
quoted
This patch introduces a new optional header line to the tag object, called
"keywords". The "keywords" line may contain a comma-separated list of
custom keywords associated with the tag object.
What is the character set for the keywords?
Hmm. Same as for the "tag" line, and the rest of the tag object, I guess.
I agree this should probably be specified. What are the rules for other
git objects?
quoted
+ for (i = 0; i < keywords_len; ++i) {
+ unsigned char c = keywords_line[i];
+ if (c == ',' && keywords_line[i + 1] == ',')
+ /* consecutive commas */
+ return error("Tag object (@ char "
+ PD_FMT "): Found empty keyword",
+ keywords_line + i - data);
+ if (c > ' ' && c != 0x7f)
+ continue;
And what is so special about 0x7f?
Isn't DEL a control char?
quoted
+ if (keywords_len) { /* optional keywords string was given */
+ item->keywords = xmalloc(keywords_len + 1);
Who frees the keywords and what's wrong with strndup?
The code that should free the tag name should also free the keywords.
No code appear to do this at the moment (regardless of whether you use my patch
series or not), which is a shame. We should make sure all objects are properly
deallocated. This is currently a general problem in git, isn't it?
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Johan Herland <hidden> Date: 2016-06-15 22:43:15
On Sunday 10 June 2007, Alex Riesen wrote:
On 6/9/07, Johan Herland [off-list ref] wrote:
quoted
On Saturday 09 June 2007, Alex Riesen wrote:
quoted
On 6/9/07, Johan Herland [off-list ref] wrote:
quoted
+ if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {
+ free(buffer);
+ die("invalid tag data file");
This, and the similar one below are useless. You're destroying the
process, what do you free that buffer for? Either handle the error
case or do not needlessly complicate your change, which really
also absolutely unneeded.
Well, I was taught to treat my memory with care.
How do you treat your performance?
Hopefully with care, as well. However, I tend to look at performance _after_
correctness.
Besides, was that systems with common address space
where you were taught? Like DOS or MacOS, perhaps?
Nope. Never programmed on either. I thought care with memory was generally
considered a good principle. If I'm wrong, please point me at the relevant
documentation.
quoted
Right now it doesn't make any difference in practice (except that
Valgrind might be a bit happier with it), but in the future -- with
the libifaction effort and whatnot -- you never know what might happen
to this piece of code, and I'd like to stay on the safe side.
So that people have to check your free as well (they will have to,
they come looking for die-calls). You just made more work for them.
Ok. Drop it. This isn't particularily important to me. I just try to
follow good principles when I can.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Johan Herland <hidden> Date: 2016-06-15 22:43:15
Using xstrndup() yields more compact and readable code than using
xmalloc(), memcpy() and manual NUL termination.
Thanks to Alex Riesen [off-list ref] for suggesting this.
Also fixes a buglet where item->keywords would always be set to "tag",
even if item->tag was empty.
Signed-off-by: Johan Herland <redacted>
---
On Saturday 09 June 2007, Alex Riesen wrote:
@@ -219,26 +219,19 @@ int parse_and_verify_tag_buffer(struct tag *item,}if(item){/* Store parsed information into item */-if(tag_len){/* optional tag name was given */-item->tag=xmalloc(tag_len+1);-memcpy(item->tag,tag_line,tag_len);-item->tag[tag_len]='\0';-}-else{/* optional tag name not given */-item->tag=xmalloc(1);-item->tag[0]='\0';-}+if(tag_len)/* optional tag name was given */+item->tag=xstrndup(tag_line,tag_len);+else/* optional tag name not given */+item->tag=xstrndup("",0);-if(keywords_len){/* optional keywords string was given */-item->keywords=xmalloc(keywords_len+1);-memcpy(item->keywords,keywords_line,keywords_len);-item->keywords[keywords_len]='\0';-}+if(keywords_len)/* optional keywords string was given */+item->keywords=xstrndup(keywords_line,keywords_len);else{/* optional keywords string not given. Set default *//* if tag name is set, use "tag"; else use "note" */-constchar*default_kw=item->tag?"tag":"note";-item->keywords=xmalloc(strlen(default_kw)+1);-memcpy(item->keywords,default_kw,strlen(default_kw)+1);+if(*(item->tag))+item->keywords=xstrndup("tag",3);+else+item->keywords=xstrndup("note",4);}if(!strcmp(type,blob_type)){
From: Johan Herland <hidden> Date: 2016-06-15 22:43:15
This effectively disables the check when creating and fscking tag objects,
which is desirable because we don't want the fact that the object is
already parsed to stop us from verifying thoroughly.
Signed-off-by: Johan Herland <redacted>
---
On Saturday 09 June 2007, Junio C Hamano wrote:
@@ -46,9 +46,11 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const uconstchar*type_line,*tag_line,*tagger_line;unsignedlongtype_len,tag_len;-if(item->object.parsed)-return0;-item->object.parsed=1;+if(item){+if(item->object.parsed)+return0;+item->object.parsed=1;+}
Passing both item and data does not feel right. If you are
trying to make the factored out function to do the verification
of data, then perhaps the caller should do the "don't handle the
same data twice" optimization using item?
You mean, like this?
...Johan
tag.c | 10 ++++------
1 files changed, 4 insertions(+), 6 deletions(-)
@@ -75,12 +75,6 @@ int parse_and_verify_tag_buffer(struct tag *item,unsignedlongtype_len,tag_len,keywords_len,tagger_len;constchar*header_end;-if(item){-if(item->object.parsed)-return0;-item->object.parsed=1;-}-if(size<65)returnFAIL("Tag object failed preliminary size check");
@@ -264,6 +258,10 @@ int parse_and_verify_tag_buffer(struct tag *item,intparse_tag_buffer(structtag*item,void*data,unsignedlongsize){+if(item->object.parsed)+return0;+item->object.parsed=1;+returnparse_and_verify_tag_buffer(item,(constchar*)data,size,0);}
@@ -80,26 +82,26 @@ int parse_and_verify_tag_buffer(struct tag *item, } if (size < 65)- return error("Tag object failed preliminary size check");+ return FAIL("Tag object failed preliminary size check");
This is ugly.
If you _have_ to output the error message in one case, and not in the
other, I'd rather do
enum tag_error { TAG_SIZE_CHECK, TAG_BLA_BLUB, ... };
const char **tag_error_strings = { "tag: size error", ... };
Of course, you'd lose the ability to output some numbers. But those
numbers that you output are even uglier than the code. Guess how surprised
_I_ was, when I hit the error message which made me go mad.
Having said that, I still do not agree in this unifying.
Your rationale seems to be: use the same checking for the tag creation as
for the tag validation.
But this is _wrong_. We _do_ have tags that do not conform to the strict
standards of git-tag, and even if we did _not_, it would _still_ be wrong
to be that strict when _reading_ tags.
To drive that point home: strict checking when creating tags is good.
Strict checking when reading tags is bad.
I strongly encourage keeping both validations separate.
You'd also avoid having that many lines which are well over the encouraged
80 character limit.
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:43:15
Hi,
On Sat, 9 Jun 2007, Junio C Hamano wrote:
Johan Herland [off-list ref] writes:
quoted
+ if (!tag_line++)
return error("char" PD_FMT ": could not find next \"\\n\"", type_line - data);
- tag_line++;
BTW if you _are_ verbosing the output, you might just as well make it
useful.
The common format is "filename:line[:column]:message", not "char[n]",
which is a misnomer to begin with, since you are talking about an offset,
not a char (remember, characters are those things that are displayed in
place for a given number, so I fully expected char32 to be a space).
Ciao,
Dscho
What is this change good for?
How did you justify the type selection for your
loop index variable?
IOW, the patch looks very useless.
I agree. By itself, the patch is useless.
Then it shouldn't be there.
It seems that you do not place the cuts between patches at the
_conceptual_ layer. Therefore, they seem intrusive and often the meaning
evades me.
So, if I understood the purpose of this patch series correctly, namely to
use the same verification routines both for creation as for validation of
tags, you could have
- moved one function into the library (the stricter one), saying
"move this_function() into libgit.a to make it usable from
git-bla" in the commit body,
- used that from the other program, removing the now-unused
function,
- and then changed the behaviour to be more chatty or some such.
As it is, you have a mix of conceptually different changes in almost every
patch, and some changes that conceptually belong into the same patch, are
not.
Be that as may, I think it is not a good change to reuse the same function
like you did, exactly because one version _should_ be more forgiving than
the other.
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:43:15
Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
I'm probably splitting this up into too small pieces, since I keep
getting comments that fail to see the overall picture of what I'm trying
to do, [...]
Maybe I said that your patch was too large. But then, I said something
much more important: hard-to-review.
These small patches, split in a manner making it even more difficult to
understand what you want to accomplish, do not help.
Yes, you should make small patches. Even small patch series. But in such a
fashion that a reviewer can see that it is a good patch[*1*]. Just lean
back, look at your patches, and ask yourself how you would have reacted if
you had reviewed them.
Ciao,
Dscho
*1* A good patch follows the immortal words of Saint Exupery: A designer
knows he has achieved perfection not when there is nothing left to add,
but when there is nothing left to take away.
relate with your commit message?
You might understand that people _could_ get the impression that the
patches were not very carefully crafted. Or even worse, the impression,
that it was tried to slip some changes by, with a totally unrelated
"official" purpose. (Again, reminds me of politics: it's like
slipping an intrusive privacy law into an agriculture related law at the
11th hour, just before Christmas, when people do not really pay
attention.)
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:43:15
Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
Also update selftests to reflect new error messages.
Had you split the patch conceptually, the changes to the tests would have
been included where appropriate.
I consider a patch _breaking_ a test case, with a follow up patch to the
test case, a _bug_.
And I consider a change in error messages not good, _unless_ the changes
have a real value in practice. Which I maintain they do not, in this
patch-series' case.
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:43:15
Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
- if (memcmp(data, "object ", 7))
+ if (prefixcmp(data, "object "))
FWIW I think that _these_ changes are actually somewhat worth it. And they
should have come _instead_ of moving the order of the memcmp() around
(you know which patch that was).
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:43:15
Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
+ /*
+ * Advance header field pointers past their initial identifier.
+ * Calculate lengths of header fields.
+ */
+ type_line += strlen("type ");
+ type_len = tag_line - type_line - 1;
+ tag_line += strlen("tag ");
+ tag_len = tagger_line - tag_line - 1;
+
/* Get the actual type */
- type_len = tag_line - type_line - strlen("type \n");
if (type_len >= sizeof(type))
return error("Tag object (@ char " PD_FMT "): "
- "Type too long", type_line + 5 - data);
- memcpy(type, type_line + 5, type_len);
+ "Type too long", type_line - data);
+ memcpy(type, type_line, type_len);
This change does not clarify anything. It is exactly as confusing as
before.
- for (i = 4;;) {
+ for (i = 0;;) {
I know you introduced this in another patch. This is an ugly construct. If
you want people to review your patches, you might as well put in the
effort to make the reviewing more pleasant.
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:43:15
Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
Be explicit about the fact that the "tagger" line is now considered a
mandatory part of the tag object. There are however old tags (from
before July 2005) that don't have a "tagger" line. We therefore consider
the "tagger" line _optional_ when parsing tags without thorough_verify
set.
No. The "before July 2005" part is _not_ the reason that we consider this
line optional.
The fact that it is bad to fail on a fetch, just because you happen to
have an invalid tag in your repository, is a good reason not to.
The fact that it is bad to fail on a git branch, just because you happen
to have an invalid tag in your repository, is a good reason not to.
The fact that it is bad to fail on an fsck, just because you happen to
have an invalid tag in your repository, is a good reason not to.
And yes, if I remember correctly, your original patch did exactly that.
The paradigm to follow is: fail gracefully. I could have an invalid
_commit_ in my repository, and would still want _every_ Git operation to
succeed, _as long_ as it does not touch that bad object.
And I damned well want git-fsck to not crash, just because some
assumptions are made.
Since this is a fundamental critique on your patch series, I will do the
detailed review on _this_ patch in another mail.
Ciao,
Dscho
This is ugly. Really ugly. Besides, it breaks the minimal patch paradigm.
quoted hunk
@@ -81,7 +82,7 @@ int parse_and_verify_tag_buffer(struct tag *item, if (size < 64) return error("Tag object failed preliminary size check");- /* Verify object line */+ /* Verify mandatory object line */ if (prefixcmp(data, "object ")) return error("Tag object (@ char 0): " "Does not start with \"object \"");
Hmph. I think everybody reading C code understands that this is mandatory.
I even think that the comment is useless. It is sort of a
code-in-human-language duplicated code.
- /* Verify the tagger line */
+ /*
+ * Verify mandatory tagger line, but only when we're checking
+ * thoroughly, i.e. on inserting a new tag, and on fsck.
+ * There are existing tag objects without a tagger line (most
+ * notably the "v0.99" tag in the main git repo), and we don't
+ * want to fail parsing on these.
+ */
I maintain that even with thorough checking, it is _wrong_ to error on a
missing tagger. Since we have to deal with tagger-less tags _anyway_, and
since it is not like you could really do something about it (the tag is
immutable), you should go with a warning.
- * Calculate lengths of header fields.
+ * Calculate lengths of header fields (0 for fields that are not given).
Does that really make sense? You effectively treat a missing field as if
it were empty. IMHO that is wrong. Besides, this
@@ -81,19 +81,22 @@ static int verify_tag(char *data, unsigned long size)memcpy(type,type_line+5,type_len);type[type_len]='\0';-/* Verify that the object matches */-if(verify_object(sha1,type))-returnerror("char%d: could not verify object %s",7,sha1_to_hex(sha1));--/* Verify the tag-name: we don't allow control characters or spaces in it */-tag_line+=4;-for(;;){-unsignedcharc=*tag_line++;-if(c=='\n')-break;-if(c>' ')-continue;-returnerror("char"PD_FMT": could not verify tag name",tag_line-data);+{+unsignedlongi;
Do you realize that half of your diff consists of reindenting, just
because you introduced this ugly construct, instead of being a good boy
and put the declarations where they belong -- at the beginning of the
function (or if it exists, block)?
+ /* Verify the tag-name: we don't allow control characters or spaces in it */
+ for (i = 4;;) {
Yes, you can write this construct. That does not change the fact that it
gives me eye cancer.
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:43:15
Hi,
On Sat, 9 Jun 2007, Johan Herland wrote:
The tag line is now optional. If not given in the tag object data, it
defaults to the empty string ("") in the parsed tag object.
The patch also adds a change to git-show; when asked to display a tag
object with no name (missing "tag" header), we will show the tag's sha1
instead of an empty string.
Finally the patch includes some tweaks to the selftests to make them
work with optional tag names.
If you don't actually _test_ missing tag names, you might just as well
leave the tests alone.
- t->tag,
+ *(t->tag) ? t->tag : name,
This is misleading. What you wanted to say is t->tag[0] == '\0', or
*(t->tag) == '\0'.
As you wrote it, you have to think a couple of times why it is okay to
dereference t->tag, to check if you say t->tag.
Besides, it breaks if you _do_ have an empty tag. In that case, I _want_
to see that it is actually empty, and _not_ the SHA1 substituted for it.
Ciao,
Dscho