{"thread":{"id":"18418","subject":"[PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l","startedAt":"2009-03-19T20:30:02Z","lastAt":"2009-03-19T23:08:43Z","messageCount":6,"participants":["Alex Riesen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"108580","messageId":"20090319203002.GA31014@blimp.localdomain","threadId":"18418","inReplyTo":null,"subject":"[PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-03-19T20:30:02Z","receivedAt":"2009-03-19T20:30:02Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Initialize the size with 0. The error message is already printed\nby sha1_object_info itself. Otherwise the uninitialized size is\nprinted, which does not make any sense at all.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\n builtin-ls-tree.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-ls-tree.c b/builtin-ls-tree.c\nindex fca4631..a8cdafb 100644\n--- a/builtin-ls-tree.c\n+++ b/builtin-ls-tree.c\n@@ -60,7 +60,6 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n {\n \tint retval = 0;\n \tconst char *type = blob_type;\n-\tunsigned long size;\n \n \tif (S_ISGITLINK(mode)) {\n \t\t/*\n@@ -91,6 +90,7 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n \tif (!(ls_options & LS_NAME_ONLY)) {\n \t\tif (ls_options & LS_SHOW_SIZE) {\n \t\t\tif (!strcmp(type, blob_type)) {\n+\t\t\t\tunsigned long size = 0;\n \t\t\t\tsha1_object_info(sha1, &size);\n \t\t\t\tprintf(\"%06o %s %s %7lu\\t\", mode, type,\n \t\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n-- \n1.6.2.1.250.ga1458\n"},{"id":"108597","messageId":"7v4oxp89eb.fsf@gitster.siamese.dyndns.org","threadId":"18418","inReplyTo":"20090319203002.GA31014@blimp.localdomain","subject":"Re: [PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-19T21:55:56Z","receivedAt":"2009-03-19T21:55:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> Initialize the size with 0. The error message is already printed\n> by sha1_object_info itself. Otherwise the uninitialized size is\n> printed, which does not make any sense at all.\n>\n> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>\n> ---\n>\n>  builtin-ls-tree.c |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c\n> index fca4631..a8cdafb 100644\n> --- a/builtin-ls-tree.c\n> +++ b/builtin-ls-tree.c\n> @@ -60,7 +60,6 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n>  {\n>  \tint retval = 0;\n>  \tconst char *type = blob_type;\n> -\tunsigned long size;\n>  \n>  \tif (S_ISGITLINK(mode)) {\n>  \t\t/*\n> @@ -91,6 +90,7 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n>  \tif (!(ls_options & LS_NAME_ONLY)) {\n>  \t\tif (ls_options & LS_SHOW_SIZE) {\n>  \t\t\tif (!strcmp(type, blob_type)) {\n> +\t\t\t\tunsigned long size = 0;\n>  \t\t\t\tsha1_object_info(sha1, &size);\n>  \t\t\t\tprintf(\"%06o %s %s %7lu\\t\", mode, type,\n>  \t\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n\nHmm, shouldn't you be checking the return value from sha1_object_info()\nand skipping the printf() altogether instead?\n"},{"id":"108600","messageId":"20090319220020.GA8433@blimp.localdomain","threadId":"18418","inReplyTo":"7v4oxp89eb.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-03-19T22:00:21Z","receivedAt":"2009-03-19T22:00:21Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Thu, Mar 19, 2009 22:55:56 +0100:\n> Alex Riesen <raa.lkml@gmail.com> writes:\n> > @@ -91,6 +90,7 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n> >  \tif (!(ls_options & LS_NAME_ONLY)) {\n> >  \t\tif (ls_options & LS_SHOW_SIZE) {\n> >  \t\t\tif (!strcmp(type, blob_type)) {\n> > +\t\t\t\tunsigned long size = 0;\n> >  \t\t\t\tsha1_object_info(sha1, &size);\n> >  \t\t\t\tprintf(\"%06o %s %s %7lu\\t\", mode, type,\n> >  \t\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n> \n> Hmm, shouldn't you be checking the return value from sha1_object_info()\n> and skipping the printf() altogether instead?\n\nBut then I cannot know the name of the failed tree entry.\n"},{"id":"108604","messageId":"7vmybh6u15.fsf@gitster.siamese.dyndns.org","threadId":"18418","inReplyTo":"20090319220020.GA8433@blimp.localdomain","subject":"Re: [PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-19T22:13:10Z","receivedAt":"2009-03-19T22:13:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> Junio C Hamano, Thu, Mar 19, 2009 22:55:56 +0100:\n>> Alex Riesen <raa.lkml@gmail.com> writes:\n>> > @@ -91,6 +90,7 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n>> >  \tif (!(ls_options & LS_NAME_ONLY)) {\n>> >  \t\tif (ls_options & LS_SHOW_SIZE) {\n>> >  \t\t\tif (!strcmp(type, blob_type)) {\n>> > +\t\t\t\tunsigned long size = 0;\n>> >  \t\t\t\tsha1_object_info(sha1, &size);\n>> >  \t\t\t\tprintf(\"%06o %s %s %7lu\\t\", mode, type,\n>> >  \t\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n>> \n>> Hmm, shouldn't you be checking the return value from sha1_object_info()\n>> and skipping the printf() altogether instead?\n>\n> But then I cannot know the name of the failed tree entry.\n\nWhy?\n\n\tif (sha1_object_info() == OBJ_BAD)\n\t\tdie(\"object recorded at tree entry %s is bad\", pathname);\n\tprintf ...\n"},{"id":"108609","messageId":"20090319225429.GC8433@blimp.localdomain","threadId":"18418","inReplyTo":"7vmybh6u15.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-03-19T22:54:29Z","receivedAt":"2009-03-19T22:54:29Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"An error message is already printed by sha1_object_info itself, and\nthe failed entries are additionally marked in the listing.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nJunio C Hamano, Thu, Mar 19, 2009 23:13:10 +0100:\n> Alex Riesen <raa.lkml@gmail.com> writes:\n> \n> > Junio C Hamano, Thu, Mar 19, 2009 22:55:56 +0100:\n> >> Alex Riesen <raa.lkml@gmail.com> writes:\n> >> > @@ -91,6 +90,7 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n> >> >  \tif (!(ls_options & LS_NAME_ONLY)) {\n> >> >  \t\tif (ls_options & LS_SHOW_SIZE) {\n> >> >  \t\t\tif (!strcmp(type, blob_type)) {\n> >> > +\t\t\t\tunsigned long size = 0;\n> >> >  \t\t\t\tsha1_object_info(sha1, &size);\n> >> >  \t\t\t\tprintf(\"%06o %s %s %7lu\\t\", mode, type,\n> >> >  \t\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n> >> \n> >> Hmm, shouldn't you be checking the return value from sha1_object_info()\n> >> and skipping the printf() altogether instead?\n> >\n> > But then I cannot know the name of the failed tree entry.\n> \n> Why?\n> \n> \tif (sha1_object_info() == OBJ_BAD)\n> \t\tdie(\"object recorded at tree entry %s is bad\", pathname);\n> \tprintf ...\n\nTried. Makes exactly this code much uglier, and the pathname is\nprinted nicely quoted after the outer if() is closed. And I don't like\nthe idea of dying here: it'll take longer to collect all the needed\nentry names for later recovery (that's how it came to the change,\nAFAIR).\n\nHow about this patch instead? I chose \"BAD\" for the marker, as any\nautomatic processing trying blindly to convert it into a number will\nget a 0, which seems safe to me.\n\n builtin-ls-tree.c |   22 ++++++++++++----------\n 1 files changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin-ls-tree.c b/builtin-ls-tree.c\nindex fca4631..22008df 100644\n--- a/builtin-ls-tree.c\n+++ b/builtin-ls-tree.c\n@@ -60,7 +60,6 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n {\n \tint retval = 0;\n \tconst char *type = blob_type;\n-\tunsigned long size;\n \n \tif (S_ISGITLINK(mode)) {\n \t\t/*\n@@ -90,17 +89,20 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n \n \tif (!(ls_options & LS_NAME_ONLY)) {\n \t\tif (ls_options & LS_SHOW_SIZE) {\n+\t\t\tchar size_text[24];\n \t\t\tif (!strcmp(type, blob_type)) {\n-\t\t\t\tsha1_object_info(sha1, &size);\n-\t\t\t\tprintf(\"%06o %s %s %7lu\\t\", mode, type,\n-\t\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n-\t\t\t\t              : sha1_to_hex(sha1),\n-\t\t\t\t       size);\n+\t\t\t\tunsigned long size;\n+\t\t\t\tif (sha1_object_info(sha1, &size) == OBJ_BAD)\n+\t\t\t\t\tstrcpy(size_text, \"BAD\");\n+\t\t\t\telse\n+\t\t\t\t\tsnprintf(size_text, sizeof(size_text),\n+\t\t\t\t\t\t \"%lu\", size);\n \t\t\t} else\n-\t\t\t\tprintf(\"%06o %s %s %7c\\t\", mode, type,\n-\t\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n-\t\t\t\t              : sha1_to_hex(sha1),\n-\t\t\t\t       '-');\n+\t\t\t\tstrcpy(size_text, \"-\");\n+\t\t\tprintf(\"%06o %s %s %7s\\t\", mode, type,\n+\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n+\t\t\t\t      : sha1_to_hex(sha1),\n+\t\t\t       size_text);\n \t\t} else\n \t\t\tprintf(\"%06o %s %s\\t\", mode, type,\n \t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n-- \n1.6.2.1.237.g7206c6\n"},{"id":"108612","messageId":"7v4oxp6rgk.fsf@gitster.siamese.dyndns.org","threadId":"18418","inReplyTo":"20090319225429.GC8433@blimp.localdomain","subject":"Re: [PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-19T23:08:43Z","receivedAt":"2009-03-19T23:08:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> How about this patch instead? I chose \"BAD\" for the marker, as any\n> automatic processing trying blindly to convert it into a number will\n> get a 0, which seems safe to me.\n\nSuch a broken automatic processing won't mind getting any garbage; the\nchoice among this patch, your original \"say 0 when we do not know\" patch,\nor unpatched \"size is undefined when an entry is corrupt\" git wouldn't\nmake a whit of difference to it.\n\nAn automatic processing that does validate its input will notice BAD is\nnot a number, and can handle such a corrupt entry more sanely, which is\npotentially a big plus.\n\nI think this round is a big improvement.\n\n>  builtin-ls-tree.c |   22 ++++++++++++----------\n>  1 files changed, 12 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c\n> index fca4631..22008df 100644\n> --- a/builtin-ls-tree.c\n> +++ b/builtin-ls-tree.c\n> @@ -60,7 +60,6 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n>  {\n>  \tint retval = 0;\n>  \tconst char *type = blob_type;\n> -\tunsigned long size;\n>  \n>  \tif (S_ISGITLINK(mode)) {\n>  \t\t/*\n> @@ -90,17 +89,20 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n>  \n>  \tif (!(ls_options & LS_NAME_ONLY)) {\n>  \t\tif (ls_options & LS_SHOW_SIZE) {\n> +\t\t\tchar size_text[24];\n>  \t\t\tif (!strcmp(type, blob_type)) {\n> -\t\t\t\tsha1_object_info(sha1, &size);\n> -\t\t\t\tprintf(\"%06o %s %s %7lu\\t\", mode, type,\n> -\t\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n> -\t\t\t\t              : sha1_to_hex(sha1),\n> -\t\t\t\t       size);\n> +\t\t\t\tunsigned long size;\n> +\t\t\t\tif (sha1_object_info(sha1, &size) == OBJ_BAD)\n> +\t\t\t\t\tstrcpy(size_text, \"BAD\");\n> +\t\t\t\telse\n> +\t\t\t\t\tsnprintf(size_text, sizeof(size_text),\n> +\t\t\t\t\t\t \"%lu\", size);\n>  \t\t\t} else\n> -\t\t\t\tprintf(\"%06o %s %s %7c\\t\", mode, type,\n> -\t\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n> -\t\t\t\t              : sha1_to_hex(sha1),\n> -\t\t\t\t       '-');\n> +\t\t\t\tstrcpy(size_text, \"-\");\n> +\t\t\tprintf(\"%06o %s %s %7s\\t\", mode, type,\n> +\t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n> +\t\t\t\t      : sha1_to_hex(sha1),\n> +\t\t\t       size_text);\n>  \t\t} else\n>  \t\t\tprintf(\"%06o %s %s\\t\", mode, type,\n>  \t\t\t       abbrev ? find_unique_abbrev(sha1, abbrev)\n> -- \n> 1.6.2.1.237.g7206c6\n"}]}