{"thread":{"id":"22490","subject":"Fix signal handler","startedAt":"2010-02-02T16:14:23Z","lastAt":"2010-02-24T11:08:32Z","messageCount":36,"participants":["Markus Elfring","Jeff King","Thomas Rast","Shawn O. Pearce","Andreas Ericsson","Bill Lear","Daniel Barkalow","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"133364","messageId":"4B684F5F.7020409@web.de","threadId":"22490","inReplyTo":null,"subject":"Fix signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-02T16:14:23Z","receivedAt":"2010-02-02T16:14:23Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"Hello,\n\nThe function \"early_output\" that is set as a signal handler by the\nfunction \"setup_early_output\" contains a simple looking instruction.\nhttp://git.kernel.org/?p=git/git.git;a=blob;f=builtin-log.c;h=8d16832f7e9483f7903009459a72efc39e267c98;hb=HEAD#l173\n\nA global variable gets a function pointer assigned.\nhttp://git.kernel.org/?p=git/git.git;a=blob;f=revision.h;h=a14deefc252bd641fba5e16f7859b4a985a72578;hb=HEAD#l138\n\nI find that this approach does not fit to standard rules because the\ndata type \"sig_atomic_t\" is the only type that can be safely used for\nglobal write access in signal handlers.\nhttps://www.securecoding.cert.org/confluence/display/seccode/SIG31-C.+Do+not+access+or+modify+shared+objects+in+signal+handlers\n\nWould you like to change any details in the design of your software\nbecause of this issue to avoid undefined behaviour?\n\nRegards,\nMarkus\n"},{"id":"133390","messageId":"20100202205849.GA14385@sigill.intra.peff.net","threadId":"22490","inReplyTo":"4B684F5F.7020409@web.de","subject":"Re: Fix signal handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-02T20:58:49Z","receivedAt":"2010-02-02T20:58:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 02, 2010 at 05:14:23PM +0100, Markus Elfring wrote:\n\n> The function \"early_output\" that is set as a signal handler by the\n> function \"setup_early_output\" contains a simple looking instruction.\n> [...]\n> A global variable gets a function pointer assigned.\n> [...]\n> I find that this approach does not fit to standard rules because the\n> data type \"sig_atomic_t\" is the only type that can be safely used for\n> global write access in signal handlers.\n\nNo, it's not a sig_atomic_t, but it is assignment of a single function\npointer that is properly declared as volatile. Is this actually a\nproblem on any known system?\n\nIf you want to nit-pick, there are much worse cases. For example, in\ndiff.c, we do quite a bit of work in remove_tempfile_on_signal. It\nassumes that char* assignment is atomic, but nothing is even marked as\nvolatile. But again, is this actually a problem on any system?\n\nYou will find that most git developers care about real problems that can\nbe demonstrated on real systems. Standards can be a useful guide, but\nthey can be too loose (e.g., we run on some non-POSIX systems) as well\nas too restrictive. What matters is what actually runs in practice.\n\nIf you can demonstrate a practical problem and provide a patch, then I\nam sure people would be happy to read it.\n\n-Peff\n"},{"id":"133397","messageId":"4B689CC5.3000400@web.de","threadId":"22490","inReplyTo":"20100202205849.GA14385@sigill.intra.peff.net","subject":"Re: Fix signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-02T21:44:37Z","receivedAt":"2010-02-02T21:44:37Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"\n>\n> No, it's not a sig_atomic_t, but it is assignment of a single function\n> pointer that is properly declared as volatile. Is this actually a\n> problem on any known system?\n>   \n\nIs it guaranteed to work on all supported software environments that an\naddress can be atomically set?\n\n\n> If you want to nit-pick, there are much worse cases. For example, in\n> diff.c, we do quite a bit of work in remove_tempfile_on_signal.\n>   \n\nThanks that you point out another open issue.\n\n\n> It assumes that char* assignment is atomic, but nothing is even marked as\n> volatile. But again, is this actually a problem on any system?\n>   \n\nWould you like to provide software implementations that work by design?\n\nRegards,\nMarkus\n"},{"id":"133409","messageId":"20100202223208.GB18781@sigill.intra.peff.net","threadId":"22490","inReplyTo":"4B689CC5.3000400@web.de","subject":"Re: Fix signal handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-02T22:32:08Z","receivedAt":"2010-02-02T22:32:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 02, 2010 at 10:44:37PM +0100, Markus Elfring wrote:\n\n> > No, it's not a sig_atomic_t, but it is assignment of a single function\n> > pointer that is properly declared as volatile. Is this actually a\n> > problem on any known system?\n> \n> Is it guaranteed to work on all supported software environments that an\n> address can be atomically set?\n\nI think you are missing my point. We are not coding to a set of\nstandards that provide guarantees. We are coding to a practical set of\nreal-world implementations that people try to run git on and produce bug\nreports for. I do not think anyone on this list could even enumerate a\ncomplete a list of the \"supported software environments\" of git.\n\nWe try to be conservative about portability issues. Some things are\nobviously wrong. But other things may violate the letter of some\nstandards, and yet work in practice on all of the platforms people are\ninterested in running git on.\n\nI don't think anyone here is much interested in whether there is any\nsort of guarantee on a particular construct working. What we do care\nabout is whether there is an actual problem on some platform that enough\npeople care about to justify rewriting the code to handle it.\n\nSo to answer your question, I honestly don't know. The code may well be\nbroken on common platforms and it is simply a race condition that has\nnever come up. But I do know that it has not been a common source of bug\nreports, which makes me not want to spend time investigating it when\nnobody has demonstrated its incorrectness beyond mentioning a standards\ndocument.  Especially when that time could be better spent fixing other\nbugs.\n\n-Peff\n"},{"id":"133461","messageId":"4B694DEE.5030207@web.de","threadId":"22490","inReplyTo":"20100202223208.GB18781@sigill.intra.peff.net","subject":"Re: Fix signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-03T10:20:30Z","receivedAt":"2010-02-03T10:20:30Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"\n> I don't think anyone here is much interested in whether there is any\n> sort of guarantee on a particular construct working.\n\nThat is a pity. - I would expect that professional software development\nwill build on working specifications instead of potentially undefined\nbehaviour.\n\n\n> So to answer your question, I honestly don't know. The code may well\n> be broken on common platforms and it is simply a race condition that\n> has never come up. But I do know that it has not been a common source\n> of bug reports, which makes me not want to spend time investigating\n> it when nobody has demonstrated its incorrectness beyond mentioning\n> a standards document.\n>   \n\nThanks for your clarification.\n\nI find that programming errors in this area might be hard to identify\nfrom the outside because resulting race conditions and deadlocks fall\ninto the symptom category of heisenbugs, don't they?\nHow many software developers do deal with the nasty design details for\nsignal handler implementations correctly?\n\nRegards,\nMarkus\n"},{"id":"133463","messageId":"20100203102915.GA25486@coredump.intra.peff.net","threadId":"22490","inReplyTo":"4B694DEE.5030207@web.de","subject":"Re: Fix signal handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-03T10:29:16Z","receivedAt":"2010-02-03T10:29:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 03, 2010 at 11:20:30AM +0100, Markus Elfring wrote:\n\n> > I don't think anyone here is much interested in whether there is any\n> > sort of guarantee on a particular construct working.\n> \n> That is a pity. - I would expect that professional software development\n> will build on working specifications instead of potentially undefined\n> behaviour.\n\nI think it is simply impractical. It's not that we're ignoring a\nspecification, it's that there _isn't_ a concrete specification for the\nset of systems we're interested in.\n\n> > So to answer your question, I honestly don't know. The code may well\n> > be broken on common platforms and it is simply a race condition that\n> > has never come up. But I do know that it has not been a common source\n> > of bug reports, which makes me not want to spend time investigating\n> > it when nobody has demonstrated its incorrectness beyond mentioning\n> > a standards document.\n> [...]\n> I find that programming errors in this area might be hard to identify\n> from the outside because resulting race conditions and deadlocks fall\n> into the symptom category of heisenbugs, don't they?\n\nYes, they can be hard to identify from the outside. But if you are\ninterested in addressing the situation, I am suggesting that the first\nstep would be to demonstrate that there in fact _is_ a race condition,\nand it is not simply some theoretical problem.\n\n-Peff\n"},{"id":"133466","messageId":"4B696447.10803@web.de","threadId":"22490","inReplyTo":"20100203102915.GA25486@coredump.intra.peff.net","subject":"Re: Fix signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-03T11:55:51Z","receivedAt":"2010-02-03T11:55:51Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"\n>\n> I think it is simply impractical.\n\nI have got the opposite opinion.\n\n\n> It's not that we're ignoring a specification, it's that there _isn't_\n> a concrete specification for the set of systems we're interested in.\n>   \n\nI have got doubts on your view. Known specifications are available for\nPOSIX and corresponding programming languages like C and C++. I know\nthat they have got open issues on their own because a few important\nwordings are not as precise and clear as you might prefer.\n\nFor which software environments do you miss programming standards?\n\nHow many efforts would you like to spend on conditional compilation for\n\"special\" platforms?\n\n\n>\n> But if you are interested in addressing the situation, I am suggesting\n> that the first step would be to demonstrate that there in fact _is_ a\n> race condition, and it is not simply some theoretical problem.\n>   \n\nI try to point out this open issue once more.\n\nCan you imagine any unwanted results if the desired address can not be\natomically set in a way that fulfils requirements for signal handler\nimplementations?\nCan it happen that the assigned function pointer will become a dangling\npointer because of word-tearing?\n\n\nAnother \"dangerous\" use case:\nDo you know if any function is called during the execution in a signal\nhandling context that is not async-signal-safe like \"fprintf()\"?\n\nRegards,\nMarkus\n"},{"id":"133468","messageId":"201002031412.53195.trast@student.ethz.ch","threadId":"22490","inReplyTo":"4B696447.10803@web.de","subject":"Re: Fix signal handler","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-02-03T13:12:52Z","receivedAt":"2010-02-03T13:12:52Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"On Wednesday 03 February 2010 12:55:51 Markus Elfring wrote:\n> [Jeff King wrote:]\n> >\n> > I think it is simply impractical.\n> \n> I have got the opposite opinion.\n\nYou've been wasting two regular contributors' (Avery and Peff) time on\nissues they point out are impractical to fix, while demonstrating that\nyou have enough standards and C knowledge to do the fix yourself.\n\nSo why don't you post patches (either fixes or testcases exhibiting\nthe issue) instead of more mails containing the same points?  This is,\nafter all, an open source project.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"133471","messageId":"20100203151709.GA28477@coredump.intra.peff.net","threadId":"22490","inReplyTo":"4B696447.10803@web.de","subject":"Re: Fix signal handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-03T15:17:09Z","receivedAt":"2010-02-03T15:17:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 03, 2010 at 12:55:51PM +0100, Markus Elfring wrote:\n\n> > It's not that we're ignoring a specification, it's that there _isn't_\n> > a concrete specification for the set of systems we're interested in.\n> \n> I have got doubts on your view. Known specifications are available for\n> POSIX and corresponding programming languages like C and C++. I know\n> that they have got open issues on their own because a few important\n> wordings are not as precise and clear as you might prefer.\n> \n> For which software environments do you miss programming standards?\n\nOff the top of my head, I have seen mention of git running on Linux,\n{Free,Net,Open}BSD, Solaris 7-10, OpenSolaris, AIX 5.x and 6.x, HP-UX,\nWindows, SCO OpenServer, and SCO UnixWare. Not all of those are POSIX,\nand I am not sure even if they were that we could (or would want to)\nstick to a strict subset of POSIX. If all of those systems allow less\nstrict behavior, then what is the problem in taking advantage of it if\nit makes development or maintenance of the code easier?\n\nDitto for C. We mostly stick to C89, but there are many parts of the C89\nstandard where behavior may be implementation defined or even undefined,\nbut in practice work just fine. I am not interested in spending a lot of\neffort working around those issues just to meet the letter of the\nstandard if there is no practical system on which it matters.\n\n> How many efforts would you like to spend on conditional compilation for\n> \"special\" platforms?\n\nIf I haven't made that clear, _I_ don't want to spend any effort. If\n_you_ are concerned about it, feel free to make a patch. If your patch\nis not too intrusive, and especially if you can demonstrate that it is a\nproblem on a real-world system, then I think your patch would be\nconsidered for inclusion upstream.\n\nIf you are not willing to spend any effort on these problems, and\ninstead are trying to direct _my_ priorities, then I have no interest in\nlistening to what you have to say (and I suspect that goes for the rest\nof the regular git developers, too).\n\n-Peff\n"},{"id":"133475","messageId":"4B699A45.7000905@web.de","threadId":"22490","inReplyTo":"201002031412.53195.trast@student.ethz.ch","subject":"Re: Fix signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-03T15:46:13Z","receivedAt":"2010-02-03T15:46:13Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"\n>\n> So why don't you post patches (either fixes or testcases exhibiting\n> the issue) instead of more mails containing the same points?\n>   \n\nI try to get a feeling about acceptance for update suggestions before\nthey can be expressed in the target programming language.\nThe \"correct\" wording in the source code might become more work than you\nmight agree with.\n\nRegards,\nMarkus\n"},{"id":"133477","messageId":"20100203155252.GA14799@spearce.org","threadId":"22490","inReplyTo":"4B699A45.7000905@web.de","subject":"Re: Fix signal handler","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-03T15:52:52Z","receivedAt":"2010-02-03T15:52:52Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Markus Elfring <Markus.Elfring@web.de> wrote:\n> >\n> > So why don't you post patches (either fixes or testcases exhibiting\n> > the issue) instead of more mails containing the same points?\n> >   \n> \n> I try to get a feeling about acceptance for update suggestions before\n> they can be expressed in the target programming language.\n\nWe've been burned too many times by people who drive-by and demand\nwe fix X, without showing proof that X is a problem, or offering\na patch to resolve whatever X they claim is an issue.  Each such\ntime burns existing well-known contributor time.\n\nWe've also been burned too many times by well-known contributors\nposting \"Hey, we should do Y\" and then never actually writing the\ncode themselves.  I know, I've done it.\n\nPie-in-the-sky discussions serve no purpose, and just waste\neveryone's time.\n\n\nIf its worthwhile, write the damn code, and share it.  If its not\nworth your time to write the code in order to propose the idea,\nits not worth our time to listen.\n\nI've never kill-filed _ANYONE_ on this mailing list before.  You are\n>.< this close to making me go figure out how to setup a kill file\non my domain just so I can stop receving all emails from you.\n\n\n-- \nShawn.\n"},{"id":"133478","messageId":"4B699C08.50400@op5.se","threadId":"22490","inReplyTo":"4B699A45.7000905@web.de","subject":"Re: Fix signal handler","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2010-02-03T15:53:44Z","receivedAt":"2010-02-03T15:53:44Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"On 02/03/2010 04:46 PM, Markus Elfring wrote:\n> \n>>\n>> So why don't you post patches (either fixes or testcases exhibiting\n>> the issue) instead of more mails containing the same points?\n>>\n> \n> I try to get a feeling about acceptance for update suggestions before\n> they can be expressed in the target programming language.\n\nThe general feeling on this list is that patches are listened to, no\nmatter how foul they are, and you will get a (hopefully) polite\nrejection if it is considered useless because it addresses a problem\nthat doesn't exist.\n\nSuggestions that others do a lot of work is generally not listened\nto.\n\n> The \"correct\" wording in the source code might become more work than you\n> might agree with.\n> \n\nSince you're the only one really interested in this, you'll be the\none doing the work. If you do that work well and submit your patch(es)\nby the git patch submission standards, screening them for useless\ncode churn is but a moments work.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"133480","messageId":"4B699E7C.1030007@web.de","threadId":"22490","inReplyTo":"20100203151709.GA28477@coredump.intra.peff.net","subject":"Re: Fix signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-03T16:04:12Z","receivedAt":"2010-02-03T16:04:12Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"\n>\n> If your patch is not too intrusive, and especially if you can demonstrate\n> that it is a problem on a real-world system, then I think your patch would\n> be considered for inclusion upstream.\n>   \n\nI have got the feeling that my corresponding update suggestion (in\nsource code form) would become intrusive to some degree. If I do not get\nan indication that issues from word-tearing in signal handlers is a\nmentionable problem here, I assume that your acceptance is low for\npotential fixes from every software developer (including me).\n\nRegards,\nMarkus\n"},{"id":"133481","messageId":"4B69A34B.7010309@web.de","threadId":"22490","inReplyTo":"4B699C08.50400@op5.se","subject":"Re: Fix signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-03T16:24:43Z","receivedAt":"2010-02-03T16:24:43Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"\n>\n> The general feeling on this list is that patches are listened to, no\n> matter how foul they are, and you will get a (hopefully) polite\n> rejection if it is considered useless because it addresses a problem\n> that doesn't exist.\n>   \n\nI hope that a healthy balance will be found between correct software\ndesign, development and quick \"hacking\". There might also be more\nefforts if too many patches will be rejected just because the suggested\nand planned changes were not discussed before.\n\nWould you like to get an acknowledgement for signal handler problems\nfrom people in other discussion groups like \"comp.programming.threads\"?\n\nRegards,\nMarkus\n"},{"id":"133482","messageId":"19305.41912.44282.604758@blake.zopyra.com","threadId":"22490","inReplyTo":"4B699E7C.1030007@web.de","subject":"Re: Fix signal handler","fromName":"Bill Lear","fromEmail":"rael@zopyra.com","sentAt":"2010-02-03T16:26:32Z","receivedAt":"2010-02-03T16:26:32Z","isPatch":false,"sender":{"key":"rael@zopyra.com","avatar":"https://gravatar.com/avatar/c4f2d2790ca3828d3b4e7dfebabf61d2fe94fd82fa49cdac2a5295dd2d46a874?d=mp&s=160"},"body":"On Wednesday, February 3, 2010 at 17:04:12 (+0100) Markus Elfring writes:\n>> If your patch is not too intrusive, and especially if you can demonstrate\n>> that it is a problem on a real-world system, then I think your patch would\n>> be considered for inclusion upstream.\n>>   \n>I have got the feeling that my corresponding update suggestion (in\n>source code form) would become intrusive to some degree. If I do not get\n>an indication that issues from word-tearing in signal handlers is a\n>mentionable problem here, I assume that your acceptance is low for\n>potential fixes from every software developer (including me).\n\nDo the words \"insufferable !@#$!%%$\" mean anything to you?  I mean,\nseriously, you are WELCOME TO SUBMIT A PATCH FOR THIS.  Do it!  Go\nahead, write it!  We would love to see it.  Fix things, make things\nbetter!  Don't continue to whine and waste people's time.  ALL PATCHES\nARE INTRUSIVE.  JUST SUBMIT A PATCH!\n\n\nBill\n"},{"id":"133591","messageId":"4B6A75EC.6030509@op5.se","threadId":"22490","inReplyTo":"4B69A34B.7010309@web.de","subject":"Re: Fix signal handler","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2010-02-04T07:23:24Z","receivedAt":"2010-02-04T07:23:24Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"On 02/03/2010 05:24 PM, Markus Elfring wrote:\n> \n>>\n>> The general feeling on this list is that patches are listened to, no\n>> matter how foul they are, and you will get a (hopefully) polite\n>> rejection if it is considered useless because it addresses a problem\n>> that doesn't exist.\n>>\n> \n> I hope that a healthy balance will be found between correct software\n> design, development and quick \"hacking\".\n\nWhat's considered a \"healthy balance\" varies from person to person\nthough.\n\n> There might also be more\n> efforts if too many patches will be rejected just because the suggested\n> and planned changes were not discussed before.\n> \n\nPerhaps, but those wasted efforts would have been yours, not ours. Sorry,\nbut the sad fact is that unless you're willing to \"fix\" this (which you\nshould show by submitting patches), it's entirely uninteresting to even\ndiscuss it. We're all busy folks here, and we do not have unlimited time\non our hands to masturbate mentally when most of us realize that it's\nnot a practical approach to blindly follow standards.\n\n> Would you like to get an acknowledgement for signal handler problems\n> from people in other discussion groups like \"comp.programming.threads\"?\n> \n\nNo. I have no interest in theoretical best practices for a program that\nhas no known issues with its use of multiple threads.\n\nI do have interest in bug-reports about my favourite scm and patches to\nmake it better. So far you have only shown a willingness to discuss\npossible problems instead of real-world ones. I've mentally marked you\ndown as \"the threads theory troll\". To remedy that sad title, you should\ndeliver some patches pointing out and fixing problems in the sources.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"134060","messageId":"4B71A2EE.8070708@web.de","threadId":"22490","inReplyTo":"20100202205849.GA14385@sigill.intra.peff.net","subject":"Re: Fix signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-09T18:01:18Z","receivedAt":"2010-02-09T18:01:18Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"\n>\n> If you can demonstrate a practical problem and provide a patch, then I\n> am sure people would be happy to read it.\n>   \n\nI need a few further clarifications on this issue to choose a potential fix.\n\nI have noticed that the variable \"show_early_output\" gets a value\nassigned only at a few places in the source code. I wonder that the set\npointer is only used by the function \"limit_list\" to call the function\n\"log_show_early\" on demand.\nhttp://git.kernel.org/?p=git/git.git;a=blob;f=revision.c;h=3ba6d991f6e9789949c314c2981dfc6b208a6f66;hb=HEAD#l683\n\nI find that a simple flag would be sufficient. I see no need to handle\ndifferent function pointers here. Do any objections exist to achieve the\nsame effect with the data type \"sig_atomic_t\"?\n\nRegards,\nMarkus\n"},{"id":"134095","messageId":"alpine.LNX.2.00.1002091812290.14365@iabervon.org","threadId":"22490","inReplyTo":"4B71A2EE.8070708@web.de","subject":"Re: Fix signal handler","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2010-02-09T23:49:05Z","receivedAt":"2010-02-09T23:49:05Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 9 Feb 2010, Markus Elfring wrote:\n\n> \n> >\n> > If you can demonstrate a practical problem and provide a patch, then I\n> > am sure people would be happy to read it.\n> >   \n> \n> I need a few further clarifications on this issue to choose a potential fix.\n> \n> I have noticed that the variable \"show_early_output\" gets a value\n> assigned only at a few places in the source code. I wonder that the set\n> pointer is only used by the function \"limit_list\" to call the function\n> \"log_show_early\" on demand.\n> http://git.kernel.org/?p=git/git.git;a=blob;f=revision.c;h=3ba6d991f6e9789949c314c2981dfc6b208a6f66;hb=HEAD#l683\n> \n> I find that a simple flag would be sufficient. I see no need to handle\n> different function pointers here. Do any objections exist to achieve the\n> same effect with the data type \"sig_atomic_t\"?\n\nIn that particular instance, there's actually a comment that says it uses \nan int (which is almost certainly what sig_atomic_t is, but sig_atomic_t \nmight not be defined on some actual platforms). Making the code match the \ncomment, at least, would be good.\n\nIn particular, function pointers are more likely than other pointers to be \nnot a machine word. I'm pretty sure that an IA64 machine could potentially \nhave a race with a small window here.\n\nAs to whether to use int (as the comment says) or sig_atomic_t, I don't \nreally have any idea which would have fewer problems.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"134153","messageId":"4B72E81B.3020900@web.de","threadId":"22490","inReplyTo":"4B71A2EE.8070708@web.de","subject":"Re: [PATCH] Fix signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-10T17:08:43Z","receivedAt":"2010-02-10T17:08:43Z","isPatch":true,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"Hello,\n\nHow do Git software developers think about the appended update suggestion?\nWould you like to integrate such adjustments into your source code\nrepository?\n\nRegards,\nMarkus\n\n\n>From c37d8dafef11168d8302d40c8d1453943a058d95 Mon Sep 17 00:00:00 2001\nFrom: Markus Elfring <Markus.Elfring@web.de>\nDate: Wed, 10 Feb 2010 17:05:45 +0100\nSubject: [PATCH] Fix a signal handler\n\nA global flag can only be set by a signal handler in a portable way if it has got the data type \"sig_atomic_t\". The previously used assignment of a function pointer in the function \"early_output\" was moved to another variable in the function \"setup_early_output\".\nThe involved software design details were also mentioned on the mailing list.\n---\n builtin-log.c |   12 +++---------\n revision.c    |   14 ++++++--------\n revision.h    |    3 ++-\n 3 files changed, 11 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 8d16832..358c98b 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -123,7 +123,7 @@ static void show_early_header(struct rev_info *rev, const char *stage, int nr)\n \n static struct itimerval early_output_timer;\n \n-static void log_show_early(struct rev_info *revs, struct commit_list *list)\n+extern void log_show_early(struct rev_info *revs, struct commit_list *list)\n {\n \tint i = revs->early_output;\n \tint show_header = 1;\n@@ -170,20 +170,14 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)\n \n static void early_output(int signal)\n {\n-\tshow_early_output = log_show_early;\n+\tshow_early_output = 1;\n }\n \n static void setup_early_output(struct rev_info *rev)\n {\n \tstruct sigaction sa;\n \n-\t/*\n-\t * Set up the signal handler, minimally intrusively:\n-\t * we only set a single volatile integer word (not\n-\t * using sigatomic_t - trying to avoid unnecessary\n-\t * system dependencies and headers), and using\n-\t * SA_RESTART.\n-\t */\n+\tearly_output_function = &log_show_early;\n \tmemset(&sa, 0, sizeof(sa));\n \tsa.sa_handler = early_output;\n \tsigemptyset(&sa.sa_mask);\ndiff --git a/revision.c b/revision.c\nindex 3ba6d99..62402fb 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -13,7 +13,8 @@\n #include \"decorate.h\"\n #include \"log-tree.h\"\n \n-volatile show_early_output_fn_t show_early_output;\n+sig_atomic_t show_early_output = 0;\n+show_early_output_fn_t early_output_function = NULL;\n \n char *path_name(const struct name_path *path, const char *name)\n {\n@@ -654,7 +655,6 @@ static int limit_list(struct rev_info *revs)\n \t\tstruct commit_list *entry = list;\n \t\tstruct commit *commit = list->item;\n \t\tstruct object *obj = &commit->object;\n-\t\tshow_early_output_fn_t show;\n \n \t\tlist = list->next;\n \t\tfree(entry);\n@@ -680,12 +680,10 @@ static int limit_list(struct rev_info *revs)\n \t\tdate = commit->date;\n \t\tp = &commit_list_insert(commit, p)->next;\n \n-\t\tshow = show_early_output;\n-\t\tif (!show)\n-\t\t\tcontinue;\n-\n-\t\tshow(revs, newlist);\n-\t\tshow_early_output = NULL;\n+\t\tif (show_early_output) {\n+\t\t\t(*early_output_function)(revs, newlist);\n+\t\t\tshow_early_output = 0;\n+\t\t}\n \t}\n \tif (revs->cherry_pick)\n \t\tcherry_pick_list(newlist, revs);\ndiff --git a/revision.h b/revision.h\nindex a14deef..93a8ffc 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -135,7 +135,8 @@ struct rev_info {\n \n /* revision.c */\n typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *);\n-extern volatile show_early_output_fn_t show_early_output;\n+extern show_early_output_fn_t early_output_function;\n+extern sig_atomic_t show_early_output;\n \n extern void init_revisions(struct rev_info *revs, const char *prefix);\n extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def);\n-- \n1.6.6.1\n\n"},{"id":"134155","messageId":"20100210171406.GE2747@spearce.org","threadId":"22490","inReplyTo":"4B72E81B.3020900@web.de","subject":"Re: [PATCH] Fix signal handler","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-10T17:14:06Z","receivedAt":"2010-02-10T17:14:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Markus Elfring <Markus.Elfring@web.de> wrote:\n> How do Git software developers think about the appended update suggestion?\n> Would you like to integrate such adjustments into your source code\n> repository?\n\nFinally, a concrete patch we can comment on!\n \n> Subject: [PATCH] Fix a signal handler\n> \n> A global flag can only be set by a signal handler in a portable way if it has got the data type \"sig_atomic_t\". The previously used assignment of a function pointer in the function \"early_output\" was moved to another variable in the function \"setup_early_output\".\n> The involved software design details were also mentioned on the mailing list.\n\nPlease line wrap your commit messages at ~70 characters per line.\nThis improves readability when reading the messages with tools like\n`git log` and `gitk` where the lines aren't reflowed.\n\nPlease read Documentation/SubmittingPatches and add a Signed-off-by\nline if you agree to the Developer's Certificate of Origin.\n\n\n> +\tearly_output_function = &log_show_early;\n...\n> -volatile show_early_output_fn_t show_early_output;\n> +sig_atomic_t show_early_output = 0;\n> +show_early_output_fn_t early_output_function = NULL;\n...\n> +\t\tif (show_early_output) {\n> +\t\t\t(*early_output_function)(revs, newlist);\n> +\t\t\tshow_early_output = 0;\n> +\t\t}\n\nThe function pointer isn't necessary.  AFAIK its only called in\nthis one call site.  So you can make a direct reference to the\nlog_show_early function.\n\n-- \nShawn.\n"},{"id":"134158","messageId":"20100210173348.GA5091@coredump.intra.peff.net","threadId":"22490","inReplyTo":"4B72E81B.3020900@web.de","subject":"Re: [PATCH] Fix signal handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-10T17:33:48Z","receivedAt":"2010-02-10T17:33:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 10, 2010 at 06:08:43PM +0100, Markus Elfring wrote:\n\n> A global flag can only be set by a signal handler in a portable way if\n> it has got the data type \"sig_atomic_t\". The previously used\n> assignment of a function pointer in the function \"early_output\" was\n> moved to another variable in the function \"setup_early_output\".\n>\n> The involved software design details were also mentioned on the\n> mailing list.\n\nKeep in mind commit messages will be read much later through \"git log\"\nand the like.  Mentioning the mailing list is usually not very helpful\nthere. It is usually a good idea instead to summarize what was said on\nthe list for later readers of the commit (though in this case, I think\nyour first paragraph really says everything that needs to be said).\n\n> --- a/builtin-log.c\n> +++ b/builtin-log.c\n> @@ -123,7 +123,7 @@ static void show_early_header(struct rev_info *rev, const char *stage, int nr)\n>  \n>  static struct itimerval early_output_timer;\n>  \n> -static void log_show_early(struct rev_info *revs, struct commit_list *list)\n> +extern void log_show_early(struct rev_info *revs, struct commit_list *list)\n\nWhy does this need to become extern? It looks like we are still just\nassigning the function pointer from within this file.\n\n> -volatile show_early_output_fn_t show_early_output;\n> +sig_atomic_t show_early_output = 0;\n> +show_early_output_fn_t early_output_function = NULL;\n\nGood. I was worried from the above s/static/extern/ that you were going\nto make log_show_early the only possible early output function, but the\nway you did it is definitely the right way.\n\nOverall, this change looks sane to me. You still haven't provided any\nevidence that this is a problem in practice, but these changes are not\nparticularly cumbersome, so it is probably better to be on the safe\nside.\n\n-Peff\n"},{"id":"134160","messageId":"20100210173527.GB5091@coredump.intra.peff.net","threadId":"22490","inReplyTo":"20100210171406.GE2747@spearce.org","subject":"Re: [PATCH] Fix signal handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-10T17:35:27Z","receivedAt":"2010-02-10T17:35:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 10, 2010 at 09:14:06AM -0800, Shawn O. Pearce wrote:\n\n> > +\tearly_output_function = &log_show_early;\n> ...\n> > -volatile show_early_output_fn_t show_early_output;\n> > +sig_atomic_t show_early_output = 0;\n> > +show_early_output_fn_t early_output_function = NULL;\n> ...\n> > +\t\tif (show_early_output) {\n> > +\t\t\t(*early_output_function)(revs, newlist);\n> > +\t\t\tshow_early_output = 0;\n> > +\t\t}\n> \n> The function pointer isn't necessary.  AFAIK its only called in\n> this one call site.  So you can make a direct reference to the\n> log_show_early function.\n\nI disagree. The original intent was to decrease coupling between the\nlibrary-like revision walker and the actual log command. We aren't using\nthat flexibility now, but I don't see any reason to decrease it\n(especially since it is so easy to keep it).\n\n-Peff\n"},{"id":"134427","messageId":"4B76A985.9070809@web.de","threadId":"22490","inReplyTo":"20100210173348.GA5091@coredump.intra.peff.net","subject":"Re: [PATCH] Fix signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-13T13:30:45Z","receivedAt":"2010-02-13T13:30:45Z","isPatch":true,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"\n>\n> Why does this need to become extern?\n\nHow do you think about to stress the detail that the function\n\"log_show_early\" is called by the function \"limit_list\" from an other\ntranslation unit.\n\n\n\n>\n> Overall, this change looks sane to me.\n\nHow are the chances to get the update suggestion into the public Git\nrepository?\n\n\n> You still haven't provided any evidence that this is a problem in practice,\n> but these changes are not particularly cumbersome, so it is probably better\n> to be on the safe side.\n>   \n\nIt is a matter of safety if all implementation details of the source\ncode conform to well-known programming standards.\n\nRegards,\nMarkus\n"},{"id":"134521","messageId":"20100214064745.GC20630@coredump.intra.peff.net","threadId":"22490","inReplyTo":"4B76A985.9070809@web.de","subject":"Re: [PATCH] Fix signal handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-14T06:47:45Z","receivedAt":"2010-02-14T06:47:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 13, 2010 at 02:30:45PM +0100, Markus Elfring wrote:\n\n> > Why does this need to become extern?\n> \n> How do you think about to stress the detail that the function\n> \"log_show_early\" is called by the function \"limit_list\" from an other\n> translation unit.\n\nStress to whom? The keyword \"extern\" tells something to the linker, but\nthe linker doesn't care (and in fact making it extern introduces cruft\ninto the global namespace). If you want to tell the user, a comment\nwould be appropriate, but I don't think it is necessary. It is not hard\nto see that the only use is assigning to an extern function pointer.\n\n> > Overall, this change looks sane to me.\n> \n> How are the chances to get the update suggestion into the public Git\n> repository?\n\nYou would have a better chance if you followed the directions in\nSubmittingPatches, including sending it to the maintainer, including\nyour patch inline, and wrapping your commit message.\n\n-Peff\n"},{"id":"134525","messageId":"7vsk94qfuy.fsf@alter.siamese.dyndns.org","threadId":"22490","inReplyTo":"20100214064745.GC20630@coredump.intra.peff.net","subject":"Re: [PATCH] Fix signal handler","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-14T10:19:33Z","receivedAt":"2010-02-14T10:19:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> You would have a better chance if you followed the directions in\n> SubmittingPatches, including sending it to the maintainer, including\n> your patch inline, and wrapping your commit message.\n\nPlease do not encourage a patch sent to the maintainer while it is still\nunder active discussion and refinement, aka \"not quite ready yet\".\n\nOther than that, your comments all looked very sensible.\n\nThanks.\n"},{"id":"134979","messageId":"4B7D6B7A.1090004@web.de","threadId":"22490","inReplyTo":"7vsk94qfuy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-18T16:31:54Z","receivedAt":"2010-02-18T16:31:54Z","isPatch":true,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"\n> Other than that, your comments all looked very sensible.\n>   \n\nDo you expect any more tweaks and fine-tuning for my update suggestion?\n\nRegards,\nMarkus\n"},{"id":"134996","messageId":"7veikib96d.fsf@alter.siamese.dyndns.org","threadId":"22490","inReplyTo":"4B7D6B7A.1090004@web.de","subject":"Re: [PATCH] Fix signal handler","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-18T20:06:34Z","receivedAt":"2010-02-18T20:06:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Markus Elfring <Markus.Elfring@web.de> writes:\n\n>> Other than that, your comments all looked very sensible.\n>\n> Do you expect any more tweaks and fine-tuning for my update suggestion?\n\nAre you asking me if _I_ expect?  How would I be able to read your mind to\nsee if you will decide to send a polished update?\n\nOf are you asking me if I'd apply your patch if you send a polished update,\nand asking me to decide it before seeing the patch?\n\nSorry, but I am confused.\n"},{"id":"135071","messageId":"4B7E708B.60006@web.de","threadId":"22490","inReplyTo":"7veikib96d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-19T11:05:47Z","receivedAt":"2010-02-19T11:05:47Z","isPatch":true,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"\n> How would I be able to read your mind to see if you will decide to send a polished update?\n>   \n\nYou indicated that my patch and the discussion about it was \"not quite\nready yet\" so far. I try to collect further suggestions for the next\nrefinement.\n\nRegards,\nMarkus\n"},{"id":"135303","messageId":"4B82744B.4060805@web.de","threadId":"22490","inReplyTo":"7veikib96d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix a signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-22T12:10:51Z","receivedAt":"2010-02-22T12:10:51Z","isPatch":true,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> Of are you asking me if I'd apply your patch if you send a polished update,\n> and asking me to decide it before seeing the patch?\n\nWould you like to pick this source code adjustment up?\n\nRegards,\nMarkus\n\n---\nFrom e138904a08ceaf469fa2f4d0ec87b5891be14760 Mon Sep 17 00:00:00 2001\nFrom: Markus Elfring <Markus.Elfring@web.de>\nDate: Mon, 22 Feb 2010 11:53:35 +0100\nSubject: [PATCH] Fix a signal handler\n\nA global flag can only be set by a signal handler in a portable way\nif it has got the data type \"sig_atomic_t\". The previously used assignment\nof a function pointer in the function \"early_output\" was moved to another\nvariable in the function \"setup_early_output\".\n\nThe involved software design details were also mentioned on the mailing list.\n\nSigned-off-by: Markus Elfring <Markus.Elfring@web.de>\n\n---\n builtin-log.c |   10 ++--------\n revision.c    |   14 ++++++--------\n revision.h    |    3 ++-\n 3 files changed, 10 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex e0d5caa..beccf7f 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -170,20 +170,14 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)\n \n static void early_output(int signal)\n {\n-\tshow_early_output = log_show_early;\n+\tshow_early_output = 1;\n }\n \n static void setup_early_output(struct rev_info *rev)\n {\n \tstruct sigaction sa;\n \n-\t/*\n-\t * Set up the signal handler, minimally intrusively:\n-\t * we only set a single volatile integer word (not\n-\t * using sigatomic_t - trying to avoid unnecessary\n-\t * system dependencies and headers), and using\n-\t * SA_RESTART.\n-\t */\n+\tearly_output_function = &log_show_early;\n \tmemset(&sa, 0, sizeof(sa));\n \tsa.sa_handler = early_output;\n \tsigemptyset(&sa.sa_mask);\ndiff --git a/revision.c b/revision.c\nindex 3ba6d99..62402fb 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -13,7 +13,8 @@\n #include \"decorate.h\"\n #include \"log-tree.h\"\n \n-volatile show_early_output_fn_t show_early_output;\n+sig_atomic_t show_early_output = 0;\n+show_early_output_fn_t early_output_function = NULL;\n \n char *path_name(const struct name_path *path, const char *name)\n {\n@@ -654,7 +655,6 @@ static int limit_list(struct rev_info *revs)\n \t\tstruct commit_list *entry = list;\n \t\tstruct commit *commit = list->item;\n \t\tstruct object *obj = &commit->object;\n-\t\tshow_early_output_fn_t show;\n \n \t\tlist = list->next;\n \t\tfree(entry);\n@@ -680,12 +680,10 @@ static int limit_list(struct rev_info *revs)\n \t\tdate = commit->date;\n \t\tp = &commit_list_insert(commit, p)->next;\n \n-\t\tshow = show_early_output;\n-\t\tif (!show)\n-\t\t\tcontinue;\n-\n-\t\tshow(revs, newlist);\n-\t\tshow_early_output = NULL;\n+\t\tif (show_early_output) {\n+\t\t\t(*early_output_function)(revs, newlist);\n+\t\t\tshow_early_output = 0;\n+\t\t}\n \t}\n \tif (revs->cherry_pick)\n \t\tcherry_pick_list(newlist, revs);\ndiff --git a/revision.h b/revision.h\nindex a14deef..93a8ffc 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -135,7 +135,8 @@ struct rev_info {\n \n /* revision.c */\n typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *);\n-extern volatile show_early_output_fn_t show_early_output;\n+extern show_early_output_fn_t early_output_function;\n+extern sig_atomic_t show_early_output;\n \n extern void init_revisions(struct rev_info *revs, const char *prefix);\n extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def);\n-- \n1.7.0\n"},{"id":"135334","messageId":"7v1vgdgm02.fsf@alter.siamese.dyndns.org","threadId":"22490","inReplyTo":"4B82744B.4060805@web.de","subject":"Re: [PATCH] Fix a signal handler","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-22T18:31:57Z","receivedAt":"2010-02-22T18:31:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Markus Elfring <Markus.Elfring@web.de> writes:\n\n>> Of are you asking me if I'd apply your patch if you send a polished update,\n>> and asking me to decide it before seeing the patch?\n>\n> Would you like to pick this source code adjustment up?\n>\n> Regards,\n> Markus\n>\n> ---\n>>From e138904a08ceaf469fa2f4d0ec87b5891be14760 Mon Sep 17 00:00:00 2001\n\nIt would be preferred to remove everything above this line when you send\npatches.\n\n> From: Markus Elfring <Markus.Elfring@web.de>\n> Date: Mon, 22 Feb 2010 11:53:35 +0100\n> Subject: [PATCH] Fix a signal handler\n\nPlease be a bit more careful when coming up with what Subject: says; it\nwill be the only piece of information to tell the reader of output from\n\"git shortlog\" what the commit made from this patch is about.  E.g.\n\n    Subject: [PATCH] log --early-output: signal handler pedantic fix\n\n> A global flag can only be set by a signal handler in a portable way\n> if it has got the data type \"sig_atomic_t\". The previously used assignment\n> of a function pointer in the function \"early_output\" was moved to another\n> variable in the function \"setup_early_output\".\n\nThe first sentence gives the basis of your argument (so that people can\ndecide to agree or disagree with you).  Then you describe why you think\nthe code you are changing is wrong --- oops, that part is missing --- and\nwhat you did based on the above two observations --- oops, that part is\nmissing, too --- and then any additional info.\n\nI'd phrase the above like this:\n\n    The behavior is undefined if the signal handler refers to any object\n    other than errno with static storage duration other than by assigning\n    a value to a static storage duration variable of type \"volatile\n    sig_atomic_t\", but the existing code updates a variable that holds a\n    pointer to a function (i.e. not a sigatomic_t variable).\n\n    Change it to only set a flag, and adjust the callsite that calls the\n    early-output function to check it.\n\nand that would be sufficiently clear without saying anything else.\n\n> diff --git a/builtin-log.c b/builtin-log.c\n> index e0d5caa..beccf7f 100644\n> --- a/builtin-log.c\n> +++ b/builtin-log.c\n> @@ -170,20 +170,14 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)\n>  \n>  static void early_output(int signal)\n>  {\n> -\tshow_early_output = log_show_early;\n> +\tshow_early_output = 1;\n>  }\n>  \n>  static void setup_early_output(struct rev_info *rev)\n>  {\n>  \tstruct sigaction sa;\n>  \n> -\t/*\n> -\t * Set up the signal handler, minimally intrusively:\n> -\t * we only set a single volatile integer word (not\n> -\t * using sigatomic_t - trying to avoid unnecessary\n> -\t * system dependencies and headers), and using\n> -\t * SA_RESTART.\n> -\t */\n> +\tearly_output_function = &log_show_early;\n\nYour proposed log message also needs to make a good counter-argument why\nthe above \"we purposely avoid using sigatomic_t --- it is not worth the\nhassle of having to deal with systems that lack this type in practice\" is\nworried too much, and it now is sensible to assume that everybody has\nsigatomic_t these days to allow us do \"the right thing\".  It can be just\nas simple as 'Output from \"git grep sigatomic_t\" indicates that we are\nalready using it.' but you need to say something, as this comment you are\nremoving makes it clear that it was not a bug by mistake or ignorance, but\ninstead was a deliberate choice.\n\n> diff --git a/revision.c b/revision.c\n> index 3ba6d99..62402fb 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -13,7 +13,8 @@\n>  #include \"decorate.h\"\n>  #include \"log-tree.h\"\n>  \n> -volatile show_early_output_fn_t show_early_output;\n> +sig_atomic_t show_early_output = 0;\n> +show_early_output_fn_t early_output_function = NULL;\n\nAccording to POSIX, \"s-e-o\" has to be \"volatile sig_atomic_t\".  Also we do\nnot explicitly initialize bss variables to zero or NULL.\n\nThanks.\n"},{"id":"135405","messageId":"4B839811.6040109@web.de","threadId":"22490","inReplyTo":"7v1vgdgm02.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix a signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-23T08:55:45Z","receivedAt":"2010-02-23T08:55:45Z","isPatch":true,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":">     Subject: [PATCH] log --early-output: signal handler pedantic fix\n\nI would prefer a correct and portable approach here instead of a \"pedantic\" one.\n  ;-)\n\n\n> I'd phrase the above like this:\n\nIt might be that the suggested commit message was too terse.\n\n\n>     The behavior is undefined if the signal handler refers to any object\n>     other than errno with static storage duration other than by assigning\n>     a value to a static storage duration variable\n\nI would not repeat the specification of undefined behaviour if a reference to a\nstandard like POSIX will be sufficient.\n\n\n> and that would be sufficiently clear without saying anything else.\n\nIt seems that we have got different opinions about the clarity of signal handling.\n\n\n> Your proposed log message also needs to make a good counter-argument why\n> the above \"we purposely avoid using sigatomic_t --- it is not worth the\n> hassle of having to deal with systems that lack this type in practice\" is\n> worried too much, and it now is sensible to assume that everybody has\n> sigatomic_t these days to allow us do \"the right thing\".\n\nThis data type is actually not used (because an underscore is missing in the\nname).   ;-)\n\n\n> It can be just as simple as 'Output from \"git grep sigatomic_t\" indicates\n> that we are already using it.' but you need to say something, as this\n> comment you are removing makes it clear that it was not a bug by mistake\n> or ignorance, but instead was a deliberate choice.\n\nShould I really add to the log message that there is another user for it like\nthe source file \"progress.c\"?\n\n\n> According to POSIX, \"s-e-o\" has to be \"volatile sig_atomic_t\".\n\nHow do you think about informations from a discussion on a topic like 'Is\n\"volatile sig_atomic_t\" redundant'?\nhttp://groups.google.de/group/comp.lang.c/browse_frm/thread/da3118a2d2c0737c/718dc093b83e03f8?#718dc093b83e03f8\n\n\n> Also we do not explicitly initialize bss variables to zero or NULL.\n\nIf we would like to insist on the implementation of a strictly conforming\nprogram, the source code should be restructured even more.\nhttps://www.securecoding.cert.org/confluence/display/seccode/SIG31-C.+Do+not+access+or+modify+shared+objects+in+signal+handlers\n\nThe variable \"show_early_output\" should be moved to the source file\n\"builtin-log.c\" where it will become \"static\". Other means would be needed to\ntransfer corresponding state changes to the function \"path_name\".\n\nRegards,\nMarkus\n"},{"id":"135406","messageId":"4B839B85.4040608@web.de","threadId":"22490","inReplyTo":"4B839811.6040109@web.de","subject":"Re: [PATCH] Fix a signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-23T09:10:29Z","receivedAt":"2010-02-23T09:10:29Z","isPatch":true,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> Other means would be needed to transfer corresponding state changes to the function \"path_name\".\n\nI'm sorry for a potential confusion. - The function \"limit_list\" is the intended\ncall site so far.\n\nRegards,\nMarkus\n"},{"id":"135473","messageId":"7vmxyzfwt7.fsf@alter.siamese.dyndns.org","threadId":"22490","inReplyTo":"4B839811.6040109@web.de","subject":"Re: [PATCH] Fix a signal handler","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-23T21:48:20Z","receivedAt":"2010-02-23T21:48:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Markus Elfring <Markus.Elfring@web.de> writes:\n\n>> According to POSIX, \"s-e-o\" has to be \"volatile sig_atomic_t\".\n>\n> How do you think about informations from a discussion on a topic like 'Is\n> \"volatile sig_atomic_t\" redundant'?\n> http://groups.google.de/group/comp.lang.c/browse_frm/thread/da3118a2d2c0737c/718dc093b83e03f8?#718dc093b83e03f8\n\nHonestly I don't care; you are the one who are interested in being\npedantic, and you are welcome wasting your time on that endeavor.  Don't\nask me to waste my time by joining your mental masturbation.\n\n>> Also we do not explicitly initialize bss variables to zero or NULL.\n>\n> If we would like to insist on the implementation of a strictly conforming\n> program,...\n\nWe don't.\n\nThe thing is, we do not like to insist any such thing.  We are practical\nbunch who are interested in getting git work well on real platforms used\nby real people.  Portability across platforms people care about is one of\nthe goals and standard conformance for us is merely a tool to achieve it.\n\nName one platform you tried to port git to and had trouble with because\nthe platform did not initialize variables in bss segment to zero, or\nperhaps on that platfor NULL had a bitpattern different from all zero, and\nafter you initialized them explicitly to zero or NULL, you managed to make\neverything work perfectly.\n\nName one platform you actually got a segfault in the early-output codepath\non it, because a function pointer on that platform is not of an atomic\ntype, and the assignment from show_early_output to show done in\nlimit_list() picked up a pointer half-written by the signal handler, and\nwe ended up calling a garbage address, and you managed to make everything\nwork perfectly with your fix.\n\nJust name one.\n\nStandard conformance by itself is never a goal for us, unless it helps to\nsolve real world problems.  And until you understand that, you wouldn't\nunderstand why this patch deserves to be labelled with \"pedantic fix\".\n"},{"id":"135550","messageId":"4B8501A0.3060805@web.de","threadId":"22490","inReplyTo":"7vmxyzfwt7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix a signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-24T10:38:24Z","receivedAt":"2010-02-24T10:38:24Z","isPatch":true,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> Name one platform you tried to port git to and had trouble with because\n> the platform did not initialize variables in bss segment to zero, or\n> perhaps on that platfor NULL had a bitpattern different from all zero, and\n> after you initialized them explicitly to zero or NULL, you managed to make\n> everything work perfectly.\n> \n> Name one platform you actually got a segfault in the early-output codepath\n> on it, because a function pointer on that platform is not of an atomic\n> type, and the assignment from show_early_output to show done in\n> limit_list() picked up a pointer half-written by the signal handler, and\n> we ended up calling a garbage address, and you managed to make everything\n> work perfectly with your fix.\n\nThanks for your feedback.\n\nWhich is the name for this specific software environment where the \"unexpected\"\nbehaviour was observed?\n\nDoes the mentioned improvement justify the integration of my intermediate update\nsuggestion that works without a \"static\" flag so far into your source code\nrepository?\n\nRegards,\nMarkus\n"},{"id":"135552","messageId":"4B8504BE.90503@op5.se","threadId":"22490","inReplyTo":"4B8501A0.3060805@web.de","subject":"Re: [PATCH] Fix a signal handler","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2010-02-24T10:51:42Z","receivedAt":"2010-02-24T10:51:42Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"On 02/24/2010 11:38 AM, Markus Elfring wrote:\n>> Name one platform you tried to port git to and had trouble with because\n>> the platform did not initialize variables in bss segment to zero, or\n>> perhaps on that platfor NULL had a bitpattern different from all zero, and\n>> after you initialized them explicitly to zero or NULL, you managed to make\n>> everything work perfectly.\n>>\n>> Name one platform you actually got a segfault in the early-output codepath\n>> on it, because a function pointer on that platform is not of an atomic\n>> type, and the assignment from show_early_output to show done in\n>> limit_list() picked up a pointer half-written by the signal handler, and\n>> we ended up calling a garbage address, and you managed to make everything\n>> work perfectly with your fix.\n> \n> Thanks for your feedback.\n> \n> Which is the name for this specific software environment where the \"unexpected\"\n> behaviour was observed?\n> \n\nThere isn't one. He was asking you to provide a bugreport for a system where\nthis behaviour was observed to prove that your fix isn't pedantic. Returning\nthe question does not help.\n\n> Does the mentioned improvement justify the integration of my intermediate update\n> suggestion that works without a \"static\" flag so far into your source code\n> repository?\n> \n\nWait and find out. I consider it useless codechurn since it's not fixing any\nreal-world problem, but it's not intrusive enough for me to care much either\nway, so as long as it doesn't break anything I don't care either way. That's\nthe big problem, really. You're asking a lot of people to spend a lot of time\non something that we, over and over, have told you we're not interested in\nunless you can prove you're solving a real problem.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"135553","messageId":"4B8508B0.8070706@web.de","threadId":"22490","inReplyTo":"7vmxyzfwt7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix a signal handler","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2010-02-24T11:08:32Z","receivedAt":"2010-02-24T11:08:32Z","isPatch":true,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> Just name one.\n\nI do not know a concrete software platform so far where the misused assignment\nof the function pointer in the signal handler implementation results in\nunexpected effects that are directly noticeable like segmentation faults.\n\nRegards,\nMarkus\n"}]}