{"thread":{"id":"5249","subject":"[PATCH 00/28] clean-ups of static functions and returns","startedAt":"2006-08-14T20:17:16Z","lastAt":"2006-08-15T01:54:34Z","messageCount":10,"participants":["David Rientjes","Jakub Narebski","Johannes Schindelin","Junio C Hamano","trajce nedev"],"isPatch":true,"patchVersion":1,"patchTotal":28},"messages":[{"id":"25280","messageId":"Pine.LNX.4.63.0608141314350.19383@chino.corp.google.com","threadId":"5249","inReplyTo":null,"subject":"[PATCH 00/28] clean-ups of static functions and returns","fromName":"David Rientjes","fromEmail":"rientjes@google.com","sentAt":"2006-08-14T20:17:16Z","receivedAt":"2006-08-14T20:17:16Z","isPatch":true,"sender":{"key":"rientjes@google.com","avatar":null},"body":"This patch series cleans up a number of static function returns that are either \nmeaningless or could be more efficiently written.\n\n\t\tDavid\n"},{"id":"25297","messageId":"ebqmlk$npt$1@sea.gmane.org","threadId":"5249","inReplyTo":"Pine.LNX.4.63.0608141314350.19383@chino.corp.google.com","subject":"Re: [PATCH 00/28] clean-ups of static functions and returns","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-14T20:33:38Z","receivedAt":"2006-08-14T20:33:38Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"David Rientjes wrote:\n\n> This patch series cleans up a number of static function returns that are either \n> meaningless or could be more efficiently written.\n\nCould you please make description of patch series email to be parent\n(ancestor) of all patches emails, i.e. for patches either to be \nchain-replied to introduction email, or all be replies to introduction\nemail.\n\nIt makes for easier reading/viewing/applying/ignoring the series.\n\nThanks in advance\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"25310","messageId":"Pine.LNX.4.63.0608141345530.19669@chino.corp.google.com","threadId":"5249","inReplyTo":"ebqmlk$npt$1@sea.gmane.org","subject":"Re: [PATCH 00/28] clean-ups of static functions and returns","fromName":"David Rientjes","fromEmail":"rientjes@google.com","sentAt":"2006-08-14T20:47:55Z","receivedAt":"2006-08-14T20:47:55Z","isPatch":true,"sender":{"key":"rientjes@google.com","avatar":null},"body":"On Mon, 14 Aug 2006, Jakub Narebski wrote:\n> Could you please make description of patch series email to be parent\n> (ancestor) of all patches emails, i.e. for patches either to be \n> chain-replied to introduction email, or all be replies to introduction\n> email.\n> \n> It makes for easier reading/viewing/applying/ignoring the series.\n> \n\nSure, but you might also want to include this request explicitly in \nDocumentation/SubmittingPatches.\n\n\t\tDavid\n"},{"id":"25311","messageId":"Pine.LNX.4.63.0608142257130.28360@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"5249","inReplyTo":"Pine.LNX.4.63.0608141345530.19669@chino.corp.google.com","subject":"Re: [PATCH 00/28] clean-ups of static functions and returns","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-08-14T20:59:18Z","receivedAt":"2006-08-14T20:59:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 14 Aug 2006, David Rientjes wrote:\n\n> On Mon, 14 Aug 2006, Jakub Narebski wrote:\n> > Could you please make description of patch series email to be parent\n> > (ancestor) of all patches emails, i.e. for patches either to be \n> > chain-replied to introduction email, or all be replies to introduction\n> > email.\n> > \n> > It makes for easier reading/viewing/applying/ignoring the series.\n> > \n> \n> Sure, but you might also want to include this request explicitly in \n> Documentation/SubmittingPatches.\n\nWell... there are people (like yours truly), who still hack there mails in \npine, and who do not use git-send-email (I had too many problems with \nPerl, and git-send-email _is_ in Perl)... For me, it would be an undue \nburden.\n\nCiao,\nDscho\n"},{"id":"25312","messageId":"ebqphe$1ga$1@sea.gmane.org","threadId":"5249","inReplyTo":"Pine.LNX.4.63.0608142257130.28360@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 00/28] clean-ups of static functions and returns","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-14T21:22:36Z","receivedAt":"2006-08-14T21:22:36Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Johannes Schindelin wrote:\n\n> On Mon, 14 Aug 2006, David Rientjes wrote:\n> \n>> On Mon, 14 Aug 2006, Jakub Narebski wrote:\n>> > Could you please make description of patch series email to be parent\n>> > (ancestor) of all patches emails, i.e. for patches either to be \n>> > chain-replied to introduction email, or all be replies to introduction\n>> > email.\n>> > \n>> > It makes for easier reading/viewing/applying/ignoring the series.\n>> > \n>> \n>> Sure, but you might also want to include this request explicitly in \n>> Documentation/SubmittingPatches.\n> \n> Well... there are people (like yours truly), who still hack there mails in \n> pine, and who do not use git-send-email (I had too many problems with \n> Perl, and git-send-email _is_ in Perl)... For me, it would be an undue \n> burden.\n\nI also not always use git-send-email (because of the box connection to the\nnet); mu latest series of patches was send using KMail; still it is not\nthat hard to send patches as reply to introductory letter, or\nchain-replied; just reply-all (without quoting) from your \"sent\" folder.\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"25321","messageId":"7vlkpqdikx.fsf@assigned-by-dhcp.cox.net","threadId":"5249","inReplyTo":"Pine.LNX.4.63.0608141314350.19383@chino.corp.google.com","subject":"Re: [PATCH 00/28] clean-ups of static functions and returns","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-14T23:05:18Z","receivedAt":"2006-08-14T23:05:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Rientjes writes:\n\n> This patch series cleans up a number of static function\n> returns that are either meaningless or could be more\n> efficiently written.\n\nInteresting. Did you use some automated tool to spot them?\n\nI see two categories of changes in your series.\n\n * Making stricter error checking in the future harder. There\n are three classes, but the lines between them are fuzzy.\n\n [PATCH 04/28] builtin-diff.c cleanup\n [PATCH 06/28] make cmd_log_walk void\n [PATCH 07/28] builtin-mailinfo.c cleanup\n [PATCH 09/28] makes prune_dir void\n [PATCH 11/28] makes append_ref and show_indepedent void\n [PATCH 12/28] makes generate_tar void\n [PATCH 13/28] builtin-unpack-objects.c cleanup\n [PATCH 14/28] make do_reupdate void\n [PATCH 16/28] daemon.c cleanup\n [PATCH 17/28] makes diff_cache void\n [PATCH 19/28] makes finish_pack void\n [PATCH 20/28] makes fetch_pack void\n [PATCH 23/28] makes peek_remote void\n\n The callers of the first group check their return values, so\n we could make error checking of these functions stricter in\n the future without affecting the rest of the code. The ones\n that currently die() (or usage()) could be made into more\n libified form to return error codes.\n\n So I do not think it is worth doing these.\n \n [PATCH 03/28] makes checkout_all void\n [PATCH 15/28] makes sha1flush void\n [PATCH 21/28] makes fsck_dir void\n [PATCH 25/28] makes pack_objects void\n [PATCH 27/28] makes track_tree_refs void\n [PATCH 28/28] makes upload_pack void\n\n The callers of the second group do not check their return\n values, so making them void for now is fine, but if we wanted\n to make error checking of these functions stricter in the\n future, we would need to change them back and update the\n callers.\n\n [PATCH 02/28] makes pprint_tag void\n\t[PATCH 26/28] makes show_entry void\n\n These, strictly speaking, fall in the second category, but I\n do not think of a reasonable graceful error return case from\n them. show_entry() dies when it cannot continue upon\n encountering a corrupt tree, which I think is reasonable. So\n I think the change to make these void makes sense.\n\n I said the lines between these three categories are fuzzy.\n If we look at the functions more closely, I am reasonably\n sure that we would find some from the first and the second\n categories do not have a reasonable graceful error return\n case from them, in which case making them void would become\n reasonable like you did.\n\n * Style and readability.\n\n [PATCH 01/28] blame.c return cleanup\n [PATCH 05/28] builtin-grep.c cleanup\n [PATCH 08/28] remove conditional return\n [PATCH 10/28] builtin-push.c cleanup\n [PATCH 15/28] makes sha1flush void\n [PATCH 18/28] diff.c cleanup\n [PATCH 22/28] http-push.c cleanup\n [PATCH 24/28] read-cache.c cleanup\n\n Most of them use the well established idiom, \"return\n !!(something)\", and I think they are fine (15/28 do not even\n need !! -- the function already returns 0 or 1).\n\n I personally feel the original is more readable for 08/28.\n\n Of course, this distinction is subjective.\n"},{"id":"25324","messageId":"Pine.LNX.4.63.0608141618470.21705@chino.corp.google.com","threadId":"5249","inReplyTo":"7vlkpqdikx.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 00/28] clean-ups of static functions and returns","fromName":"David Rientjes","fromEmail":"rientjes@google.com","sentAt":"2006-08-14T23:30:51Z","receivedAt":"2006-08-14T23:30:51Z","isPatch":true,"sender":{"key":"rientjes@google.com","avatar":null},"body":"On Mon, 14 Aug 2006, Junio C Hamano wrote:\n\n> Interesting. Did you use some automated tool to spot them?\n> \n\nNo, these changes are from my own personal tree that optimizes everything for \nspeed since I am working with terabytes of data. I only submitted changes that \nI thought would be beneficial for your project as well.\n\n> * Making stricter error checking in the future harder. There\n> are three classes, but the lines between them are fuzzy.\n> \n> [PATCH 04/28] builtin-diff.c cleanup\n> [PATCH 06/28] make cmd_log_walk void\n> [PATCH 07/28] builtin-mailinfo.c cleanup\n> [PATCH 09/28] makes prune_dir void\n> [PATCH 11/28] makes append_ref and show_indepedent void\n> [PATCH 12/28] makes generate_tar void\n> [PATCH 13/28] builtin-unpack-objects.c cleanup\n> [PATCH 14/28] make do_reupdate void\n> [PATCH 16/28] daemon.c cleanup\n> [PATCH 17/28] makes diff_cache void\n> [PATCH 19/28] makes finish_pack void\n> [PATCH 20/28] makes fetch_pack void\n> [PATCH 23/28] makes peek_remote void\n> \n> The callers of the first group check their return values, so\n> we could make error checking of these functions stricter in\n> the future without affecting the rest of the code. The ones\n> that currently die() (or usage()) could be made into more\n> libified form to return error codes.\n> \n> So I do not think it is worth doing these.\n> \n\nI disagree. Having static functions return ints that are the same for _every_ \ncode path and are checked against upon return is never good style. It implies \nthat error checking is already done and the return value is of importance. It \nalso suggests you can program against a specific return value with expected \nresults which are, in fact, true for any return. Additionally, changes in the \nfuture will be easier, in my opinion, because a void -> int change is very \nsimple and existant calls need not be changed (their return values are \ndiscarded) so that the implementer can make the checks where necessary.\n\nI do not understand your point about making error checking of the functions \nstricter without affecting the rest of the code when int returns are discarded \nif not assigned upon return. It would certainly be bad style to declare new \nerror codes with a specific meaning without checking all the affected code and \nwe certainly cannot predict this behavior now.\n\n\t\tDavid\n"},{"id":"25333","messageId":"7vsljyajxj.fsf@assigned-by-dhcp.cox.net","threadId":"5249","inReplyTo":"Pine.LNX.4.63.0608141618470.21705@chino.corp.google.com","subject":"Re: [PATCH 00/28] clean-ups of static functions and returns","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-15T01:04:24Z","receivedAt":"2006-08-15T01:04:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Rientjes writes:\n\n> On Mon, 14 Aug 2006, Junio C Hamano wrote:\n>\n>> Interesting. Did you use some automated tool to spot them?\n>> \n>\n> No, these changes are from my own personal tree that optimizes everything for \n> speed since I am working with terabytes of data. I only submitted changes that \n> I thought would be beneficial for your project as well.\n>\n>> * Making stricter error checking in the future harder. There\n>> are three classes, but the lines between them are fuzzy.\n>> \n>> [PATCH 04/28] builtin-diff.c cleanup\n>>...\n>> [PATCH 23/28] makes peek_remote void\n>> \n>> The callers of the first group check their return values, so\n>> we could make error checking of these functions stricter in\n>> the future without affecting the rest of the code. The ones\n>> that currently die() (or usage()) could be made into more\n>> libified form to return error codes.\n>> \n>> So I do not think it is worth doing these.\n>\n> I disagree. Having static functions return ints that are the\n> same for _every_ code path and are checked against upon return\n> is never good style. It implies that error checking is\n> already done and the return value is of importance.\n\nI would agree with your above statement about the second group,\nbut not the ones listed in the first group, whose callers are\nprepared to receive error returns. It just happens that these\ncallees do not currently detect errors, but some of them\ncertainly could be improved to return errors, instead of just\ncalling die().\n"},{"id":"25334","messageId":"BAY110-F2347BACD15C6BACC34E839B84F0@phx.gbl","threadId":"5249","inReplyTo":"7vsljyajxj.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 00/28] clean-ups of static functions and returns","fromName":"trajce nedev","fromEmail":"trajcenedev@hotmail.com","sentAt":"2006-08-15T01:40:21Z","receivedAt":"2006-08-15T01:40:21Z","isPatch":true,"sender":{"key":"trajcenedev@hotmail.com","avatar":null},"body":"So you're just going to continue passing numbers around in the hope that \nsomeday they'll be used for something?\n\nTrajce Nedev\n\n>From: Junio C Hamano \n>To: David Rientjes \n>CC: git@vger.kernel.org\n>Subject: Re: [PATCH 00/28] clean-ups of static functions and returns\n>Date: Mon, 14 Aug 2006 18:04:24 -0700\n>\n>David Rientjes writes:\n>\n> > On Mon, 14 Aug 2006, Junio C Hamano wrote:\n> >\n> >> Interesting. Did you use some automated tool to spot them?\n> >>\n> >\n> > No, these changes are from my own personal tree that optimizes \n>everything for\n> > speed since I am working with terabytes of data. I only submitted \n>changes that\n> > I thought would be beneficial for your project as well.\n> >\n> >> * Making stricter error checking in the future harder. There\n> >> are three classes, but the lines between them are fuzzy.\n> >>\n> >> [PATCH 04/28] builtin-diff.c cleanup\n> >>...\n> >> [PATCH 23/28] makes peek_remote void\n> >>\n> >> The callers of the first group check their return values, so\n> >> we could make error checking of these functions stricter in\n> >> the future without affecting the rest of the code. The ones\n> >> that currently die() (or usage()) could be made into more\n> >> libified form to return error codes.\n> >>\n> >> So I do not think it is worth doing these.\n> >\n> > I disagree. Having static functions return ints that are the\n> > same for _every_ code path and are checked against upon return\n> > is never good style. It implies that error checking is\n> > already done and the return value is of importance.\n>\n>I would agree with your above statement about the second group,\n>but not the ones listed in the first group, whose callers are\n>prepared to receive error returns. It just happens that these\n>callees do not currently detect errors, but some of them\n>certainly could be improved to return errors, instead of just\n>calling die().\n>\n>-\n>To unsubscribe from this list: send the line \"unsubscribe git\" in\n>the body of a message to majordomo@vger.kernel.org\n>More majordomo info at http://vger.kernel.org/majordomo-info.html\n\n_________________________________________________________________\nExpress yourself instantly with MSN Messenger! Download today - it's FREE! \nhttp://messenger.msn.click-url.com/go/onm00200471ave/direct/01/\n"},{"id":"25335","messageId":"7vlkpqahlx.fsf@assigned-by-dhcp.cox.net","threadId":"5249","inReplyTo":"BAY110-F2347BACD15C6BACC34E839B84F0@phx.gbl","subject":"Re: [PATCH 00/28] clean-ups of static functions and returns","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-15T01:54:34Z","receivedAt":"2006-08-15T01:54:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"trajce nedev\" writes:\n\n> So you're just going to continue passing numbers around in the hope\n> that someday they'll be used for something?\n>\n> Trajce Nedev\n\nIt is more like \"fearing someday we might modify the callees and\nthey may start returning error codes\".\n\nAdmittedly, \"someday\" has a tendency not to come, but making the\ncallees void and changing the callers who are already checking\nthe error codes to ignore what are returned from them somehow\nfeel backwards and pushing that someday further into the future.\n\nEither patches to implement the error checking in the callees,\nor arguments that particular callees the series touch do not\nhave reasonably graceful error return path are welcome (the\nformer advocating against and the latter advocating for the\napplication of the patch). \n"}]}