* [PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l @ 2009-03-19 20:30 Alex Riesen 2009-03-19 21:55 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Alex Riesen @ 2009-03-19 20:30 UTC (permalink / raw) To: git; +Cc: Jakub Narebski, Junio C Hamano Initialize the size with 0. The error message is already printed by sha1_object_info itself. Otherwise the uninitialized size is printed, which does not make any sense at all. Signed-off-by: Alex Riesen <raa.lkml@gmail.com> --- builtin-ls-tree.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c index fca4631..a8cdafb 100644 --- a/builtin-ls-tree.c +++ b/builtin-ls-tree.c @@ -60,7 +60,6 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen, { int retval = 0; const char *type = blob_type; - unsigned long size; if (S_ISGITLINK(mode)) { /* @@ -91,6 +90,7 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen, if (!(ls_options & LS_NAME_ONLY)) { if (ls_options & LS_SHOW_SIZE) { if (!strcmp(type, blob_type)) { + unsigned long size = 0; sha1_object_info(sha1, &size); printf("%06o %s %s %7lu\t", mode, type, abbrev ? find_unique_abbrev(sha1, abbrev) -- 1.6.2.1.250.ga1458 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l 2009-03-19 20:30 [PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l Alex Riesen @ 2009-03-19 21:55 ` Junio C Hamano 2009-03-19 22:00 ` Alex Riesen 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2009-03-19 21:55 UTC (permalink / raw) To: Alex Riesen; +Cc: git, Jakub Narebski Alex Riesen <raa.lkml@gmail.com> writes: > Initialize the size with 0. The error message is already printed > by sha1_object_info itself. Otherwise the uninitialized size is > printed, which does not make any sense at all. > > Signed-off-by: Alex Riesen <raa.lkml@gmail.com> > --- > > builtin-ls-tree.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c > index fca4631..a8cdafb 100644 > --- a/builtin-ls-tree.c > +++ b/builtin-ls-tree.c > @@ -60,7 +60,6 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen, > { > int retval = 0; > const char *type = blob_type; > - unsigned long size; > > if (S_ISGITLINK(mode)) { > /* > @@ -91,6 +90,7 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen, > if (!(ls_options & LS_NAME_ONLY)) { > if (ls_options & LS_SHOW_SIZE) { > if (!strcmp(type, blob_type)) { > + unsigned long size = 0; > sha1_object_info(sha1, &size); > printf("%06o %s %s %7lu\t", mode, type, > abbrev ? find_unique_abbrev(sha1, abbrev) Hmm, shouldn't you be checking the return value from sha1_object_info() and skipping the printf() altogether instead? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l 2009-03-19 21:55 ` Junio C Hamano @ 2009-03-19 22:00 ` Alex Riesen 2009-03-19 22:13 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Alex Riesen @ 2009-03-19 22:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jakub Narebski Junio C Hamano, Thu, Mar 19, 2009 22:55:56 +0100: > Alex Riesen <raa.lkml@gmail.com> writes: > > @@ -91,6 +90,7 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen, > > if (!(ls_options & LS_NAME_ONLY)) { > > if (ls_options & LS_SHOW_SIZE) { > > if (!strcmp(type, blob_type)) { > > + unsigned long size = 0; > > sha1_object_info(sha1, &size); > > printf("%06o %s %s %7lu\t", mode, type, > > abbrev ? find_unique_abbrev(sha1, abbrev) > > Hmm, shouldn't you be checking the return value from sha1_object_info() > and skipping the printf() altogether instead? But then I cannot know the name of the failed tree entry. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l 2009-03-19 22:00 ` Alex Riesen @ 2009-03-19 22:13 ` Junio C Hamano 2009-03-19 22:54 ` Alex Riesen 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2009-03-19 22:13 UTC (permalink / raw) To: Alex Riesen; +Cc: git, Jakub Narebski Alex Riesen <raa.lkml@gmail.com> writes: > Junio C Hamano, Thu, Mar 19, 2009 22:55:56 +0100: >> Alex Riesen <raa.lkml@gmail.com> writes: >> > @@ -91,6 +90,7 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen, >> > if (!(ls_options & LS_NAME_ONLY)) { >> > if (ls_options & LS_SHOW_SIZE) { >> > if (!strcmp(type, blob_type)) { >> > + unsigned long size = 0; >> > sha1_object_info(sha1, &size); >> > printf("%06o %s %s %7lu\t", mode, type, >> > abbrev ? find_unique_abbrev(sha1, abbrev) >> >> Hmm, shouldn't you be checking the return value from sha1_object_info() >> and skipping the printf() altogether instead? > > But then I cannot know the name of the failed tree entry. Why? if (sha1_object_info() == OBJ_BAD) die("object recorded at tree entry %s is bad", pathname); printf ... ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l 2009-03-19 22:13 ` Junio C Hamano @ 2009-03-19 22:54 ` Alex Riesen 2009-03-19 23:08 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Alex Riesen @ 2009-03-19 22:54 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jakub Narebski An error message is already printed by sha1_object_info itself, and the failed entries are additionally marked in the listing. Signed-off-by: Alex Riesen <raa.lkml@gmail.com> --- Junio C Hamano, Thu, Mar 19, 2009 23:13:10 +0100: > Alex Riesen <raa.lkml@gmail.com> writes: > > > Junio C Hamano, Thu, Mar 19, 2009 22:55:56 +0100: > >> Alex Riesen <raa.lkml@gmail.com> writes: > >> > @@ -91,6 +90,7 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen, > >> > if (!(ls_options & LS_NAME_ONLY)) { > >> > if (ls_options & LS_SHOW_SIZE) { > >> > if (!strcmp(type, blob_type)) { > >> > + unsigned long size = 0; > >> > sha1_object_info(sha1, &size); > >> > printf("%06o %s %s %7lu\t", mode, type, > >> > abbrev ? find_unique_abbrev(sha1, abbrev) > >> > >> Hmm, shouldn't you be checking the return value from sha1_object_info() > >> and skipping the printf() altogether instead? > > > > But then I cannot know the name of the failed tree entry. > > Why? > > if (sha1_object_info() == OBJ_BAD) > die("object recorded at tree entry %s is bad", pathname); > printf ... Tried. Makes exactly this code much uglier, and the pathname is printed nicely quoted after the outer if() is closed. And I don't like the idea of dying here: it'll take longer to collect all the needed entry names for later recovery (that's how it came to the change, AFAIR). How about this patch instead? I chose "BAD" for the marker, as any automatic processing trying blindly to convert it into a number will get a 0, which seems safe to me. builtin-ls-tree.c | 22 ++++++++++++---------- 1 files changed, 12 insertions(+), 10 deletions(-) diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c index fca4631..22008df 100644 --- a/builtin-ls-tree.c +++ b/builtin-ls-tree.c @@ -60,7 +60,6 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen, { int retval = 0; const char *type = blob_type; - unsigned long size; if (S_ISGITLINK(mode)) { /* @@ -90,17 +89,20 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen, if (!(ls_options & LS_NAME_ONLY)) { if (ls_options & LS_SHOW_SIZE) { + char size_text[24]; if (!strcmp(type, blob_type)) { - sha1_object_info(sha1, &size); - printf("%06o %s %s %7lu\t", mode, type, - abbrev ? find_unique_abbrev(sha1, abbrev) - : sha1_to_hex(sha1), - size); + unsigned long size; + if (sha1_object_info(sha1, &size) == OBJ_BAD) + strcpy(size_text, "BAD"); + else + snprintf(size_text, sizeof(size_text), + "%lu", size); } else - printf("%06o %s %s %7c\t", mode, type, - abbrev ? find_unique_abbrev(sha1, abbrev) - : sha1_to_hex(sha1), - '-'); + strcpy(size_text, "-"); + printf("%06o %s %s %7s\t", mode, type, + abbrev ? find_unique_abbrev(sha1, abbrev) + : sha1_to_hex(sha1), + size_text); } else printf("%06o %s %s\t", mode, type, abbrev ? find_unique_abbrev(sha1, abbrev) -- 1.6.2.1.237.g7206c6 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l 2009-03-19 22:54 ` Alex Riesen @ 2009-03-19 23:08 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2009-03-19 23:08 UTC (permalink / raw) To: Alex Riesen; +Cc: git, Jakub Narebski Alex Riesen <raa.lkml@gmail.com> writes: > How about this patch instead? I chose "BAD" for the marker, as any > automatic processing trying blindly to convert it into a number will > get a 0, which seems safe to me. Such a broken automatic processing won't mind getting any garbage; the choice among this patch, your original "say 0 when we do not know" patch, or unpatched "size is undefined when an entry is corrupt" git wouldn't make a whit of difference to it. An automatic processing that does validate its input will notice BAD is not a number, and can handle such a corrupt entry more sanely, which is potentially a big plus. I think this round is a big improvement. > builtin-ls-tree.c | 22 ++++++++++++---------- > 1 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c > index fca4631..22008df 100644 > --- a/builtin-ls-tree.c > +++ b/builtin-ls-tree.c > @@ -60,7 +60,6 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen, > { > int retval = 0; > const char *type = blob_type; > - unsigned long size; > > if (S_ISGITLINK(mode)) { > /* > @@ -90,17 +89,20 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen, > > if (!(ls_options & LS_NAME_ONLY)) { > if (ls_options & LS_SHOW_SIZE) { > + char size_text[24]; > if (!strcmp(type, blob_type)) { > - sha1_object_info(sha1, &size); > - printf("%06o %s %s %7lu\t", mode, type, > - abbrev ? find_unique_abbrev(sha1, abbrev) > - : sha1_to_hex(sha1), > - size); > + unsigned long size; > + if (sha1_object_info(sha1, &size) == OBJ_BAD) > + strcpy(size_text, "BAD"); > + else > + snprintf(size_text, sizeof(size_text), > + "%lu", size); > } else > - printf("%06o %s %s %7c\t", mode, type, > - abbrev ? find_unique_abbrev(sha1, abbrev) > - : sha1_to_hex(sha1), > - '-'); > + strcpy(size_text, "-"); > + printf("%06o %s %s %7s\t", mode, type, > + abbrev ? find_unique_abbrev(sha1, abbrev) > + : sha1_to_hex(sha1), > + size_text); > } else > printf("%06o %s %s\t", mode, type, > abbrev ? find_unique_abbrev(sha1, abbrev) > -- > 1.6.2.1.237.g7206c6 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-03-19 23:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-03-19 20:30 [PATCH] Produce a nicer output in case of sha1_object_info failures in ls-tree -l Alex Riesen 2009-03-19 21:55 ` Junio C Hamano 2009-03-19 22:00 ` Alex Riesen 2009-03-19 22:13 ` Junio C Hamano 2009-03-19 22:54 ` Alex Riesen 2009-03-19 23:08 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).