{"thread":{"id":"12135","subject":"[PATCH] parse_tag: fail, if tagged objected would be NULL","startedAt":"2008-02-16T13:04:40Z","lastAt":"2008-02-17T17:48:26Z","messageCount":3,"participants":["Martin Koegler","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"68901","messageId":"12031670802582-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"12135","inReplyTo":null,"subject":"[PATCH] parse_tag: fail, if tagged objected would be NULL","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-16T13:04:40Z","receivedAt":"2008-02-16T13:04:40Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"The cause of a NULL in tag->tagged can be:\n* a unknown object type is used\n* the tag points to a object with an other type as stated\n in the tag.\n\nBoth situations are most likley be caused by a not welformed\ntag. Catching this error here avoids doing tag->tagged!=NULL\nchecks in the rest of git.\n\nSigned-off-by: Martin Koegler \n---\nIn my option, this change would simplity many things.\n\ntag.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex 38bf913..96fef70 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -82,8 +82,12 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n \t} else {\n \t\terror(\"Unknown type %s\", type);\n \t\titem->tagged = NULL;\n+\t\treturn -1;\n \t}\n \n+\tif (!item->tagged)\n+\t\treturn -1;\n+\n \tif (item->tagged && track_object_refs) {\n \t\tstruct object_refs *refs = alloc_object_refs(1);\n \t\trefs->ref[0] = item->tagged;\n-- \n1.5.4.1.gcb4c5\n"},{"id":"68917","messageId":"7vpruw90jv.fsf@gitster.siamese.dyndns.org","threadId":"12135","inReplyTo":"12031670802582-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"Re: [PATCH] parse_tag: fail, if tagged objected would be NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-16T19:13:56Z","receivedAt":"2008-02-16T19:13:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler writes:\n\n> The cause of a NULL in tag->tagged can be:\n> * a unknown object type is used\n> * the tag points to a object with an other type as stated\n> in the tag.\n>\n> Both situations are most likley be caused by a not welformed\n> tag. Catching this error here avoids doing tag->tagged!=NULL\n> checks in the rest of git.\n\nHonestly, I am torn on this.\n\nThe approach certainly is attractive if you care only about\nworking in a perfectly well connected repository of a known\nvintage.\n\nOn the other hand, however, this robs from callers the clue that\nthe tag itself was Ok but it points at something we do not know\nabout. Maybe the caller was only interested in the tag itself\nbut did not care about the pointee, and erroring out like this\nmay make it impossible for the caller to act on the tag itself.\nMaybe the caller even knew about the breakage of the repository\nand wanted to salvage as much as possible, but because this\nerrors out, it would now consider this tag object itself is bad\nand give up, salvaging one less object. It looks to me that\nthis, along with other \"tighten parse_X_buffer()\" changes you\nsent earlier, closes door to them. That's why I think that\nparse_X_buffer() should be more lenient than fsck and keep\nsaying it.\n"},{"id":"69001","messageId":"20080217174826.GB22515@auto.tuwien.ac.at","threadId":"12135","inReplyTo":"7vpruw90jv.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] parse_tag: fail, if tagged objected would be NULL","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-17T17:48:26Z","receivedAt":"2008-02-17T17:48:26Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sat, Feb 16, 2008 at 11:13:56AM -0800, Junio C Hamano wrote:\n> Martin Koegler writes:\n> \n> > The cause of a NULL in tag->tagged can be:\n> > * a unknown object type is used\n> > * the tag points to a object with an other type as stated\n> > in the tag.\n> >\n> > Both situations are most likley be caused by a not welformed\n> > tag. Catching this error here avoids doing tag->tagged!=NULL\n> > checks in the rest of git.\n> \n> Honestly, I am torn on this.\n> \n> The approach certainly is attractive if you care only about\n> working in a perfectly well connected repository of a known\n> vintage.\n> \n> On the other hand, however, this robs from callers the clue that\n> the tag itself was Ok but it points at something we do not know\n> about. Maybe the caller was only interested in the tag itself\n> but did not care about the pointee, and erroring out like this\n> may make it impossible for the caller to act on the tag itself.\n> Maybe the caller even knew about the breakage of the repository\n> and wanted to salvage as much as possible, but because this\n> errors out, it would now consider this tag object itself is bad\n> and give up, salvaging one less object. \n\nThis opens up the question, why do we set tagged to NULL in the case\nof an unknown object? If we want to get out as much information as\npossible, we could call lookup_unknown_object and store the result.\n\n> It looks to me that\n> this, along with other \"tighten parse_X_buffer()\" changes you\n> sent earlier, closes door to them. That's why I think that\n> parse_X_buffer() should be more lenient than fsck and keep\n> saying it.\n\nOK, so please forget the parse_tag_buffer patch. \n\nIf I understand you correctly, you want to reject my last\nparse_commit_buffer patch (parse_commit: don't ignore NULL from\nloopup_commit, http://www.spinics.net/lists/git/msg58148.html) because\nof the same reasons.\n\nThis means, that Shawns idea (using rev-list for reachability checking\nduring upload, http://www.spinics.net/lists/git/msg57978.html) is not\nimplementable, as certain kinds of errors are not reported to the\ncaller by parse_commit(_buffer).\n\nmfg Martin Kögler\n"}]}