{"thread":{"id":"5396","subject":"[PATCH] dir: do all size checks before seeking back and fix file closing","startedAt":"2006-08-26T14:17:09Z","lastAt":"2006-08-27T23:55:46Z","messageCount":10,"participants":["Jonas Fonseca","Mitchell Blank Jr","Linus Torvalds","Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"25938","messageId":"20060826141709.GC11601@diku.dk","threadId":"5396","inReplyTo":null,"subject":"[PATCH] dir: do all size checks before seeking back and fix file closing","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2006-08-26T14:17:09Z","receivedAt":"2006-08-26T14:17:09Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"Signed-off-by: Jonas Fonseca \n---\n dir.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex d53d48f..ff8a2fb 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -122,11 +122,11 @@ static int add_excludes_from_file_1(cons\n \tsize = lseek(fd, 0, SEEK_END);\n \tif (size < 0)\n \t\tgoto err;\n-\tlseek(fd, 0, SEEK_SET);\n \tif (size == 0) {\n \t\tclose(fd);\n \t\treturn 0;\n \t}\n+\tlseek(fd, 0, SEEK_SET);\n \tbuf = xmalloc(size+1);\n \tif (read(fd, buf, size) != size)\n \t\tgoto err;\n@@ -146,7 +146,7 @@ static int add_excludes_from_file_1(cons\n \treturn 0;\n \n err:\n-\tif (0 <= fd)\n+\tif (0 >= fd)\n \t\tclose(fd);\n \treturn -1;\n }\n-- \n1.4.2.GIT\n\n-- \nJonas Fonseca\n"},{"id":"25949","messageId":"20060826184330.GA34439@gaz.sfgoth.com","threadId":"5396","inReplyTo":"20060826141709.GC11601@diku.dk","subject":"Re: [PATCH] dir: do all size checks before seeking back and fix file closing","fromName":"Mitchell Blank Jr","fromEmail":"mitch@sfgoth.com","sentAt":"2006-08-26T18:43:30Z","receivedAt":"2006-08-26T18:43:30Z","isPatch":true,"sender":{"key":"mitch@sfgoth.com","avatar":null},"body":"Jonas Fonseca wrote:\n> err:\n> -\tif (0 <= fd)\n> +\tif (0 >= fd)\n> \t\tclose(fd);\n\nCould you explain that piece? You now only close \"fd\" if it's zero (stdin)\nor negative (invalid). The old code (close if its >=0) make more sense.\n\n-Mitch\n"},{"id":"25955","messageId":"20060826204449.GA19104@diku.dk","threadId":"5396","inReplyTo":"20060826184330.GA34439@gaz.sfgoth.com","subject":"Re: [PATCH] dir: do all size checks before seeking back and fix file closing","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2006-08-26T20:44:49Z","receivedAt":"2006-08-26T20:44:49Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"Mitchell Blank Jr wrote Sat, Aug 26, 2006:\n> Jonas Fonseca wrote:\n> > err:\n> > -\tif (0 <= fd)\n> > +\tif (0 >= fd)\n> > \t\tclose(fd);\n> \n> Could you explain that piece? You now only close \"fd\" if it's zero (stdin)\n> or negative (invalid). The old code (close if its >=0) make more sense.\n\nAh, yes, sorry for the noise. I read the old code as (fd <= 0).\n\n-- \nJonas Fonseca\n"},{"id":"25960","messageId":"Pine.LNX.4.64.0608261509290.11811@g5.osdl.org","threadId":"5396","inReplyTo":"20060826141709.GC11601@diku.dk","subject":"Re: [PATCH] dir: do all size checks before seeking back and fix file closing","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-08-26T22:13:00Z","receivedAt":"2006-08-26T22:13:00Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 26 Aug 2006, Jonas Fonseca wrote:\n> \n> diff --git a/dir.c b/dir.c\n> index d53d48f..ff8a2fb 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -122,11 +122,11 @@ static int add_excludes_from_file_1(cons\n> \tsize = lseek(fd, 0, SEEK_END);\n> \tif (size < 0)\n> \t\tgoto err;\n> -\tlseek(fd, 0, SEEK_SET);\n> \tif (size == 0) {\n> \t\tclose(fd);\n> \t\treturn 0;\n> \t}\n> +\tlseek(fd, 0, SEEK_SET);\n\nI really think you'd be better off rewriting that to use \"fstat()\" \ninstead. I don't know why it uses two lseek's, but it's wrong, and looks \nlike some bad habit Junio picked up at some point.\n\n> @@ -146,7 +146,7 @@ static int add_excludes_from_file_1(cons\n> \treturn 0;\n> \n> err:\n> -\tif (0 <= fd)\n> +\tif (0 >= fd)\n> \t\tclose(fd);\n\nThat's wrong. \n\nNow, admittedly it's wrong because another bad habit Junio picked up \n(doing comparisons with constants in the wrong order), so please write it \nas\n\n\tif (fd >= 0)\n\t\tclose(fd);\n\ninstead.\n\nJunio: I realize that you claim that you learnt that syntax from an \nauthorative source, but he was _wrong_. Doing the constant first is more \nlikely to cause bugs, rather than less. Compilers will warn about the\n\n\tif (x = 0)\n\t\t..\n\nkind of bug, and putting the constant first just confuses humans.\n\nIt's more important to _not_ confuse humans than it is to try to avoid an \nuncommon error that compilers can and do warn about anyway.\n\n\t\t\tLinus\n"},{"id":"25961","messageId":"ecqibj$51v$1@sea.gmane.org","threadId":"5396","inReplyTo":"Pine.LNX.4.64.0608261509290.11811@g5.osdl.org","subject":"Re: [PATCH] dir: do all size checks before seeking back and fix file closing","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-26T22:35:34Z","receivedAt":"2006-08-26T22:35:34Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Linus Torvalds wrote:\n\n> Now, admittedly it's wrong because another bad habit Junio picked up \n> (doing comparisons with constants in the wrong order), so please write it \n> as\n> \n>         if (fd >= 0)\n>                 close(fd);\n> \n> instead.\n> \n> Junio: I realize that you claim that you learnt that syntax from an \n> authorative source, but he was _wrong_. Doing the constant first is more \n> likely to cause bugs, rather than less. Compilers will warn about the\n> \n>         if (x = 0)\n>                 ..\n> \n> kind of bug, and putting the constant first just confuses humans.\n\nWell, perhaps except checking if variable is in given range, e.g.\n\n if (0 <= x && x <= 5)\n \n> It's more important to _not_ confuse humans than it is to try to avoid an \n> uncommon error that compilers can and do warn about anyway.\n \n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"25963","messageId":"7v4pvz11o6.fsf@assigned-by-dhcp.cox.net","threadId":"5396","inReplyTo":"Pine.LNX.4.64.0608261509290.11811@g5.osdl.org","subject":"Re: [PATCH] dir: do all size checks before seeking back and fix file closing","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-27T00:07:37Z","receivedAt":"2006-08-27T00:07:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> I really think you'd be better off rewriting that to use \"fstat()\" \n> instead. I don't know why it uses two lseek's, but it's wrong, and looks \n> like some bad habit Junio picked up at some point.\n\nI think the code was written to avoid getting confused by\nunseekable input (pipes) but was done in early morning before\nthe first shot of caffeine.\n\n> Now, admittedly it's wrong because another bad habit Junio picked up \n> (doing comparisons with constants in the wrong order)\n\nI think you misunderstand the rationale used to encourage the\ncomparison used there. It does not have anything to do with\nhaving comparison on the left.\n\nThe comparison order is done in textual order. You list smaller\nthings on the left and then larger things on the right (iow, you\nalmost never use >= or >).\n\n> Junio: I realize that you claim that you learnt that syntax from an \n> authorative source, but he was _wrong_....\n\nThis does not come from any authoritative source, but I picked\nit up because I felt it made a lot of sense.\n\n> ... Doing the constant first is more likely to cause bugs,\n> rather than less.\n\nThat's a funny thing to say, because I was about to send out a\ncomment that touches this exact topic.\n\nI spotted the bug the patch was trying to introduce right away\n_because_ the original comparison was written in textual order.\n\nThe patch changed the comparison operator which first confused\nme for a handful seconds, and then after I swapped everything in\nmy head to read as\n\n\tif (fd <= 0)\n \tclose(fd);\n\nit became blatantly obvious it was a bogus change. In the\nmessage I was about to send out, I would have said \"a fine\nexample that using texual order comparison consistently avoids\nbugs\". So it is really relative to what you are used to.\n\nGet used to it, please ;-).\n"},{"id":"25967","messageId":"7v8xlazzyb.fsf@assigned-by-dhcp.cox.net","threadId":"5396","inReplyTo":"7v4pvz11o6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] dir: do all size checks before seeking back and fix file closing","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-27T02:15:24Z","receivedAt":"2006-08-27T02:15:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Linus Torvalds writes:\n>\n>> Now, admittedly it's wrong because another bad habit Junio picked up \n>> (doing comparisons with constants in the wrong order)\n>\n> I think you misunderstand the rationale used to encourage the\n> comparison used there. It does not have anything to do with\n> having comparison on the left.\n\ns/comparison/constant/; I cannot spell.\n"},{"id":"25968","messageId":"Pine.LNX.4.64.0608261931460.11811@g5.osdl.org","threadId":"5396","inReplyTo":"7v4pvz11o6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] dir: do all size checks before seeking back and fix file closing","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-08-27T02:37:57Z","receivedAt":"2006-08-27T02:37:57Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 26 Aug 2006, Junio C Hamano wrote:\n> \n> > Now, admittedly it's wrong because another bad habit Junio picked up \n> > (doing comparisons with constants in the wrong order)\n> \n> I think you misunderstand the rationale used to encourage the\n> comparison used there. It does not have anything to do with\n> having comparison on the left.\n> \n> The comparison order is done in textual order. You list smaller\n> things on the left and then larger things on the right (iow, you\n> almost never use >= or >).\n\nAhh. A number of people do the \"0 == x\" thing, because they want to be \ncaught if they use \"=\" instead of \"==\" by mistake. I thought it was the \nsame thing.\n\n> This does not come from any authoritative source, but I picked\n> it up because I felt it made a lot of sense.\n\nTo anybody who has _ever_ done any math at all, it makes no sense at all. \n\nYou _always_ put constants on the right-hand side (or, possibly last on \nthe left-hand side, in order to make the right-hand side be \"0\").\n\nSimilarly, if you say it out loud, you'd always say \"if 'x' is larger than \nor equal to zero\", not \"if zero is smaller or less than 'x'\". That's \nbecause \"zero\" obviously never varies, so you'd never talk about \"zero\" \nbeing compared to anything else.\n\nThe only exception would be the mathematical \"0 < x < 10\" kind of thing, \nwhich some languages (not C, of course) allows in that form. I can imagine \nthat people would just do that as \"0 < x && x < 10\" just to keep the C \nform as close to the mathematical form, although I would at least \npersonally do it as\n\n\tif (x > 0 &&\n\t x < 10)\n\nespecially if I ever needed to write it that way on multiple lines due to \nsome of the expressions being more complicated.\n\n\t\tLinus\n"},{"id":"25970","messageId":"7vy7tayj4d.fsf@assigned-by-dhcp.cox.net","threadId":"5396","inReplyTo":"Pine.LNX.4.64.0608261931460.11811@g5.osdl.org","subject":"Re: [PATCH] dir: do all size checks before seeking back and fix file closing","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-27T03:04:18Z","receivedAt":"2006-08-27T03:04:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n>> The comparison order is done in textual order. You list smaller\n>> things on the left and then larger things on the right (iow, you\n>> almost never use >= or >).\n>\n> Ahh. A number of people do the \"0 == x\" thing, because they want to be \n> caught if they use \"=\" instead of \"==\" by mistake. I thought it was the \n> same thing.\n\nThe rest is repeating what you said 15 months ago, so I did not\nquote that part, but interested parties can follow this thread:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/3907/focus=4126\n"},{"id":"26017","messageId":"20060827235546.GA20904@diku.dk","threadId":"5396","inReplyTo":"7v4pvz11o6.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] Use fstat instead of fseek","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2006-08-27T23:55:46Z","receivedAt":"2006-08-27T23:55:46Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"Signed-off-by: Jonas Fonseca \n---\n\n dir.c | 8 +++-----\n 1 files changed, 3 insertions(+), 5 deletions(-)\n\nJunio C Hamano wrote Sat, Aug 26, 2006:\n> Linus Torvalds writes:\n> \n> > I really think you'd be better off rewriting that to use \"fstat()\" \n> > instead. I don't know why it uses two lseek's, but it's wrong, and looks \n> > like some bad habit Junio picked up at some point.\n> \n> I think the code was written to avoid getting confused by\n> unseekable input (pipes) but was done in early morning before\n> the first shot of caffeine.\n\nI take it that you want this change, so here's a little addition to the\n\"use X instead of Y\" series.\n\ndiff --git a/dir.c b/dir.c\nindex d53d48f..5a40d8f 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -112,17 +112,15 @@ static int add_excludes_from_file_1(cons\n \t\t\t\t int baselen,\n \t\t\t\t struct exclude_list *which)\n {\n+\tstruct stat st;\n \tint fd, i;\n \tlong size;\n \tchar *buf, *entry;\n \n \tfd = open(fname, O_RDONLY);\n-\tif (fd < 0)\n+\tif (fd < 0 || fstat(fd, &st) < 0)\n \t\tgoto err;\n-\tsize = lseek(fd, 0, SEEK_END);\n-\tif (size < 0)\n-\t\tgoto err;\n-\tlseek(fd, 0, SEEK_SET);\n+\tsize = st.st_size;\n \tif (size == 0) {\n \t\tclose(fd);\n \t\treturn 0;\n-- \n1.4.2.g2f76-dirty\n\n-- \nJonas Fonseca\n"}]}