{"thread":{"id":"2056","subject":"[RFC] Cleaning up die() error messages","startedAt":"2005-10-10T10:50:08Z","lastAt":"2005-10-12T06:04:36Z","messageCount":13,"participants":["Elfyn McBratney","Junio C Hamano","Daniel Barkalow","Alex Riesen","H. Peter Anvin","Matthias Urlichs"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"9878","messageId":"20051010105008.GB30202@gentoo.org","threadId":"2056","inReplyTo":null,"subject":"[RFC] Cleaning up die() error messages","fromName":"Elfyn McBratney","fromEmail":"beu@gentoo.org","sentAt":"2005-10-10T10:50:08Z","receivedAt":"2005-10-10T10:50:08Z","isPatch":false,"sender":{"key":"beu@gentoo.org","avatar":null},"body":"Hello git list,\n\nI've started working on cleaning up the various die() error messages\nfound throughout git, and had a few thoughts along the way.\n\nCurrently, I've been adding missing program name prefixes, and quoting path\nnames (e.g., \"%s\" -> \"'%s'\"), but before I go any further, I'm wondering\nif this is a) desired, or perhaps b) superfluous? This may be best\ndiscussed with along-side the patch - which'll follow shortly. ;)\n\nAlso, I got to thinking whether it might be an idea to use the following\nidiom in the code:\n\n\t[shell scripts]\n\tprog=\"`basename $0`\"\n\t..\n\tfoo || die \"${prog}: foo failed\"\n\n\t[C sources]\n\tstatic char *prog;\n\t..\n\tstatic inline void set_prog_name (char *argv0)\n\t{\n\t\tprog = strrchr(argv0, '/');\n\t\tif (prog)\n\t\t\tprog++;\n\t\telse\n\t\t\tprog = argv0;\n\t}\n\t..\n\tint main (int argc, char **argv)\n\t{\n\t\tset_prog_name(argv[0]);\n\t\t..\n\t\tif (!do_bar())\n\t\t\tdie(\"%s: do_bar() failed\", prog);\n\t\t..\n\t}\n\nThe idea behind this being that, if any of the git programs get renamed\n(again :) there won't be a need for s/git-foo/git-bar/g just to fix-up\ndie() error messages, and it'll also shave a *bit* off of the size of the\ncompiled binaries. ;)\n\n(Of course, the C parts (`prog' and `set_prog_name()') would go into a\nheader, and not in every single C source file. ;)\n\nSo, any thoughts/comments/flames? :)\n\nBest,\nElfyn\n\n-- \nElfyn McBratney\nGentoo Developer/Perl Team Lead\nbeu/irc.freenode.net http://dev.gentoo.org/~beu/\n+------------O.o--------------------- http://dev.gentoo.org/~beu/pubkey.asc\n\nPGP Key ID: 0x69DF17AD\nPGP Key Fingerprint:\n DBD3 B756 ED58 B1B4 47B9 B3BD 8D41 E597 69DF 17AD\n"},{"id":"9905","messageId":"7vzmph42j2.fsf@assigned-by-dhcp.cox.net","threadId":"2056","inReplyTo":"20051010105008.GB30202@gentoo.org","subject":"Re: [RFC] Cleaning up die() error messages","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-10-10T19:04:17Z","receivedAt":"2005-10-10T19:04:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elfyn McBratney writes:\n\n> (Of course, the C parts (`prog' and `set_prog_name()') would go into a\n> header, and not in every single C source file. ;)\n>\n> So, any thoughts/comments/flames? :)\n\nI do not have objections to either one, except I tend to prefer\nprograms that tells its full path when erroring out, which helps\nme identify \"Oops, my path was screwed up and I am not testing\nthe right one\" case.\n\nOne thing to keep in mind is how badly this C part might\ninteract with the libification effort going on underwater.\nSince current code Smurf is working on is based on 0.99.6 and\nmany small pieces need to be reviewed anyway, I am not so much\nworried about forward porting the changes. But some die()s that\nare in the parts that will be moved to the common library code\nwould also want to use this prog global somehow.\n\nBut that would not be too much of a problem. Worst case, we\nforce the library clients to do set_prog_name(), or initialize\nprog to \"(unnamed)\", or do both.\n"},{"id":"9910","messageId":"Pine.LNX.4.63.0510101603520.23242@iabervon.org","threadId":"2056","inReplyTo":"20051010105008.GB30202@gentoo.org","subject":"Re: [RFC] Cleaning up die() error messages","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2005-10-10T20:14:31Z","receivedAt":"2005-10-10T20:14:31Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 10 Oct 2005, Elfyn McBratney wrote:\n\n> \t[C sources]\n> \tstatic char *prog;\n> \t..\n> \tstatic inline void set_prog_name (char *argv0)\n> \t{\n> \t\tprog = strrchr(argv0, '/');\n> \t\tif (prog)\n> \t\t\tprog++;\n> \t\telse\n> \t\t\tprog = argv0;\n> \t}\n> \t..\n> \tint main (int argc, char **argv)\n> \t{\n> \t\tset_prog_name(argv[0]);\n> \t\t..\n> \t\tif (!do_bar())\n> \t\t\tdie(\"%s: do_bar() failed\", prog);\n> \t\t..\n> \t}\n\nJust put set_prog_name() next to die(), and have die() write \"prog: \" at \nthe beginning if set_prog_name() got called. Assuming it's useful at all \nto show the name of the program, it's not going to be less useful if the \nerror is actually found in a library function called by the program.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"9962","messageId":"81b0412b0510110802lbcdebe0m17bce7ca81ea76d2@mail.gmail.com","threadId":"2056","inReplyTo":"20051010105008.GB30202@gentoo.org","subject":"Re: [RFC] Cleaning up die() error messages","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2005-10-11T15:02:11Z","receivedAt":"2005-10-11T15:02:11Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 10/10/05, Elfyn McBratney wrote:\n> int main (int argc, char **argv)\n> {\n> set_prog_name(argv[0]);\n\nI'd also use readlink on /proc/self/exe by default (if set_prog_name\n_not_ called).\nIt simplifies the code at least on linux, and makes possible very slow\ntransition for other platforms. So you don't have to update each and\nevery .c file containing \"main[[:space:]]*(\" ;)\n"},{"id":"9966","messageId":"434BE41F.4060909@zytor.com","threadId":"2056","inReplyTo":"81b0412b0510110802lbcdebe0m17bce7ca81ea76d2@mail.gmail.com","subject":"Re: [RFC] Cleaning up die() error messages","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-11T16:11:11Z","receivedAt":"2005-10-11T16:11:11Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Alex Riesen wrote:\n> On 10/10/05, Elfyn McBratney wrote:\n> \n>> int main (int argc, char **argv)\n>> {\n>> set_prog_name(argv[0]);\n> \n> \n> I'd also use readlink on /proc/self/exe by default (if set_prog_name\n> _not_ called).\n> It simplifies the code at least on linux, and makes possible very slow\n> transition for other platforms. So you don't have to update each and\n> every .c file containing \"main[[:space:]]*(\" ;)\n\nIt's really better just to put the stuff at the beginning of each main. \n If that's too annoying, librarize main and rename your mains \"git_main\".\n\n\t-hpa\n"},{"id":"9974","messageId":"pan.2005.10.11.19.48.04.675482@smurf.noris.de","threadId":"2056","inReplyTo":"7vzmph42j2.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] Cleaning up die() error messages","fromName":"Matthias Urlichs","fromEmail":"smurf@smurf.noris.de","sentAt":"2005-10-11T19:48:08Z","receivedAt":"2005-10-11T19:48:08Z","isPatch":false,"sender":{"key":"matthias@urlichs.de","avatar":"https://gravatar.com/avatar/2708905af227313eba6f2b2ae0f7d0259b5ac5d71baef58fe5a13c699ce0bbf0?d=mp&s=160"},"body":"Hi, Junio C Hamano wrote:\n\n> One thing to keep in mind is how badly this C part might\n> interact with the libification effort going on underwater.\n\nNot too badly.\n\n> Since current code Smurf is working on is based on 0.99.6 and\n\nI've merged it up once already; will do that again soon.\n\n> many small pieces need to be reviewed anyway, I am not so much\n> worried about forward porting the changes. \n\nI've also mostly succeeded in keeping the individual patches clean so that\neverything still builds (and verifies), so it might be easiest to just\nmerge with it. ;-)\n\nBut we'll cross that bridge when we get to it.\n\n> But some die()s that\n> are in the parts that will be moved to the common library code\n> would also want to use this prog global somehow.\n\nIMHO, common library code should not be allowed to die.\n(Yes, that does imply replacing all the xmalloc() calls.)\n\nMy library effort has a buffer for the (first) error message. The\ncaller can elect to suppress printing it, so that it can be formatted\nappropriately. Python, for instance, will wrap errors in an exception.\n\nThe way I structured it so far, a C program would do \n\n\tgit_env = git_env_new();\n\tdie_if_null(git_env());\n\tgit_env->print_error = 0;\n[...]\n\tgit_whatever(git_env, ...);\n\tif (git_env->error) {\n\t\tfprintf(stderr, \"%s: doing whatever: %s\\n\",\n\t\t\tmy_program_name, git_env->error);\n\t\tgit_env_clear_error(git_env);\n\t\tgoto whatever_bad_so_go_clean_up;\n\t}\n\n-- \nMatthias Urlichs | {M:U} IT Design @ m-u-it.de | smurf@smurf.noris.de\nDisclaimer: The quote was selected randomly. Really. | http://smurf.noris.de\n - -\nA hundred mouths, a hundred tongues, And throats of brass, inspired with\niron lungs.\n\t\t\t\t\t-- Virgil\n"},{"id":"9978","messageId":"434C2590.3040107@zytor.com","threadId":"2056","inReplyTo":"pan.2005.10.11.19.48.04.675482@smurf.noris.de","subject":"Re: [RFC] Cleaning up die() error messages","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-11T20:50:24Z","receivedAt":"2005-10-11T20:50:24Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Matthias Urlichs wrote:\n> \n> IMHO, common library code should not be allowed to die.\n> (Yes, that does imply replacing all the xmalloc() calls.)\n> \n\nThe sane way to do this is probably to call an overridable git_die() \nfunction, which can be specified by the user to use longjmp(), to use \nexceptions, or do something else appropriately.\n\nHowever, a much bigger problem is cleanup.\n\n\t-hpa\n"},{"id":"9989","messageId":"pan.2005.10.12.01.20.17.917829@smurf.noris.de","threadId":"2056","inReplyTo":"434C2590.3040107@zytor.com","subject":"Re: [RFC] Cleaning up die() error messages","fromName":"Matthias Urlichs","fromEmail":"smurf@smurf.noris.de","sentAt":"2005-10-12T01:20:19Z","receivedAt":"2005-10-12T01:20:19Z","isPatch":false,"sender":{"key":"matthias@urlichs.de","avatar":"https://gravatar.com/avatar/2708905af227313eba6f2b2ae0f7d0259b5ac5d71baef58fe5a13c699ce0bbf0?d=mp&s=160"},"body":"H. Peter Anvin wrote:\n\n> The sane way to do this is probably to call an overridable git_die() \n> function, which can be specified by the user to use longjmp(), to use \n> exceptions, or do something else appropriately.\n> \nI thought about doing something like that, but ...\n\n> However, a much bigger problem is cleanup.\n> \n... exactly.\n\n-- \nMatthias Urlichs | {M:U} IT Design @ m-u-it.de | smurf@smurf.noris.de\nDisclaimer: The quote was selected randomly. Really. | http://smurf.noris.de\n - -\nBOFH excuse #282:\n\nHigh altitude condensation from U.S.A.F prototype aircraft has contaminated\nthe primary subnet mask. Turn off your computer for 9 days to avoid\ndamaging it.\n"},{"id":"9993","messageId":"434C8095.4080201@zytor.com","threadId":"2056","inReplyTo":"pan.2005.10.12.01.20.17.917829@smurf.noris.de","subject":"Re: [RFC] Cleaning up die() error messages","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-12T03:18:45Z","receivedAt":"2005-10-12T03:18:45Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Matthias Urlichs wrote:\n> \n> I thought about doing something like that, but ...\n>>However, a much bigger problem is cleanup.\n> \n> ... exactly.\n> \n\nI thought about this, and probably the sanest way is to wrap malloc() \nwith something that creates a linked list of allocations. If we abort, \nwe can unwind the linked list and free all allocations.\n\n\t-hpa\n"},{"id":"9996","messageId":"7vd5mbqs2x.fsf@assigned-by-dhcp.cox.net","threadId":"2056","inReplyTo":"434C8095.4080201@zytor.com","subject":"Re: [RFC] Cleaning up die() error messages","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-10-12T04:25:58Z","receivedAt":"2005-10-12T04:25:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H. Peter Anvin\" writes:\n\n> I thought about this, and probably the sanest way is to wrap malloc() \n> with something that creates a linked list of allocations. If we abort, \n> we can unwind the linked list and free all allocations.\n\nYou can free() the allocated memory with something like that,\nbut it is likely that the aborted function would have already\ncreated some linked structure out of those memory blocks, and\ncleaning up that you would need the real exception handling\nwouldn't you?\n"},{"id":"9997","messageId":"434C93C5.9040505@zytor.com","threadId":"2056","inReplyTo":"7vd5mbqs2x.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] Cleaning up die() error messages","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-12T04:40:37Z","receivedAt":"2005-10-12T04:40:37Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Junio C Hamano wrote:\n> \"H. Peter Anvin\" writes:\n> \n>>I thought about this, and probably the sanest way is to wrap malloc() \n>>with something that creates a linked list of allocations. If we abort, \n>>we can unwind the linked list and free all allocations.\n> \n> You can free() the allocated memory with something like that,\n> but it is likely that the aborted function would have already\n> created some linked structure out of those memory blocks, and\n> cleaning up that you would need the real exception handling\n> wouldn't you?\n> \n\nNot unless any of those linked structures can be reached from outside \nthe git library.\n\n\t-hpa\n"},{"id":"9998","messageId":"7vek6rnx8z.fsf@assigned-by-dhcp.cox.net","threadId":"2056","inReplyTo":"434C93C5.9040505@zytor.com","subject":"Re: [RFC] Cleaning up die() error messages","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-10-12T05:02:36Z","receivedAt":"2005-10-12T05:02:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H. Peter Anvin\" writes:\n\n> Not unless any of those linked structures can be reached from outside \n> the git library.\n\nOK. How about things like open file descriptors and mmap'ed\nregions then? That is:\n\n fd = open();\n xmalloc(); /* may die now */\n close(fd);\n\nI think in the long run we would be better off if we properly\ndealt with the case where (x)malloc returns NULL. Of course,\nthat would make libification a bigger task, so we do not have to\ndo it from day one.\n"},{"id":"10002","messageId":"20051012060436.GA567@kiste.smurf.noris.de","threadId":"2056","inReplyTo":"434C8095.4080201@zytor.com","subject":"Re: [RFC] Cleaning up die() error messages","fromName":"Matthias Urlichs","fromEmail":"smurf@smurf.noris.de","sentAt":"2005-10-12T06:04:36Z","receivedAt":"2005-10-12T06:04:36Z","isPatch":false,"sender":{"key":"matthias@urlichs.de","avatar":"https://gravatar.com/avatar/2708905af227313eba6f2b2ae0f7d0259b5ac5d71baef58fe5a13c699ce0bbf0?d=mp&s=160"},"body":"Hi,\n\nH. Peter Anvin:\n> >I thought about doing something like that, but ...\n> >>However, a much bigger problem is cleanup.\n> >\n> >... exactly.\n> \n> I thought about this, and probably the sanest way is to wrap malloc() \n> with something that creates a linked list of allocations. If we abort, \n> we can unwind the linked list and free all allocations.\n> \nThere already is a malloc library that does this, plus it can call\ncleanup for you -- there's more to cleaning up than freeing memory. :-/\n\nLet's see if I can actually find it again.\n\nOn the other hand, I wonder if the overhead when managing data\nstructures like that really offsets the additional work we'd need to do\notherwise, which is simply checking a few more return values.\n\n-- \nMatthias Urlichs | {M:U} IT Design @ m-u-it.de | smurf@smurf.noris.de\nDisclaimer: The quote was selected randomly. Really. | http://smurf.noris.de\n - -\nBOFH excuse #381:\n\nRobotic tape changer mistook operator's tie for a backup tape.\n"}]}