{"thread":{"id":"35516","subject":"static variables","startedAt":"2013-12-11T01:25:16Z","lastAt":"2013-12-11T01:53:14Z","messageCount":3,"participants":["Stefan Zager","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"231880","messageId":"CAHOQ7J-rO-KjHyYk1Gw6Wv+iH_M7DPr76t3G7YN_sUv3YqcJcg@mail.gmail.com","threadId":"35516","inReplyTo":null,"subject":"static variables","fromName":"Stefan Zager","fromEmail":"szager@google.com","sentAt":"2013-12-11T01:25:16Z","receivedAt":"2013-12-11T01:25:16Z","isPatch":false,"sender":{"key":"szager@google.com","avatar":null},"body":"This is probably a naive question, but: there are quite a lot of static\nvariables in the git code where it's really unnecessary.  Is that just a\nhistorical artifact, or is there some reason to prefer them?  I'm working\non a patch that will introduce threading, so naturally I'm on the lookout\nfor static variables.  In general, can I get rid of static variables where\nit seems straightforward to do so?\n\nAs an example, here's an excerpt from symlnks.c.  In addition to being\nstatic, if I'm reading this right, it appears that the 'removal' variable\nis used before it's initialized:\n\nstatic struct removal_def {\n  char path[PATH_MAX];\n  int len;\n} removal;\n\nstatic void do_remove_scheduled_dirs(int new_len)\n{\n  while (removal.len > new_len) {\n    removal.path[removal.len] = '\\0';\n    if (rmdir(removal.path))\n      break;\n    do {\n      removal.len--;\n    } while (removal.len > new_len &&\n       removal.path[removal.len] != '/');\n  }\n  removal.len = new_len;\n}\n\nvoid schedule_dir_for_removal(const char *name, int len)\n{\n  int match_len, last_slash, i, previous_slash;\n\n  match_len = last_slash = i =\n    longest_path_match(name, len, removal.path, removal.len,\n           &previous_slash);\n  /* Find last slash inside 'name' */\n  while (i < len) {\n    if (name[i] == '/')\n      last_slash = i;\n    i++;\n  }\n\n  /*\n   * If we are about to go down the directory tree, we check if\n   * we must first go upwards the tree, such that we then can\n   * remove possible empty directories as we go upwards.\n   */\n  if (match_len < last_slash && match_len < removal.len)\n    do_remove_scheduled_dirs(match_len);\n  /*\n   * If we go deeper down the directory tree, we only need to\n   * save the new path components as we go down.\n   */\n  if (match_len < last_slash) {\n    memcpy(&removal.path[match_len], &name[match_len],\n           last_slash - match_len);\n    removal.len = last_slash;\n  }\n}\n\nvoid remove_scheduled_dirs(void)\n{\n  do_remove_scheduled_dirs(0);\n}\n\nEOF\n\n\n\nThanks,\n\nStefan\n"},{"id":"231882","messageId":"20131211014501.GI2311@google.com","threadId":"35516","inReplyTo":"CAHOQ7J-rO-KjHyYk1Gw6Wv+iH_M7DPr76t3G7YN_sUv3YqcJcg@mail.gmail.com","subject":"Re: static variables","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-11T01:45:01Z","receivedAt":"2013-12-11T01:45:01Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Zager wrote:\n\n> This is probably a naive question, but: there are quite a lot of static\n> variables in the git code where it's really unnecessary.  Is that just a\n> historical artifact, or is there some reason to prefer them?\n\nSometimes it's for convenience.  Other times it's to work around C89's\nrequirement that initializers can't include pointers to automatic\nvariables, so when using parse_options, old commands tend to use\nstatics for the variables initialized by options.  (Since then, git\nhas stopped following that so rigidly, which is probably a good\nthing.)\n\nWorse, some functions have static buffers when they need a large\nbuffer and want to avoid too much allocation churn.  As a general\nrule, historically very little of git's code (mostly pack related)\nneeded to be usable with threads, though of course it would be\nexcellent to fix more code to be thread-safe.\n\n> As an example, here's an excerpt from symlnks.c.  In addition to being\n> static, if I'm reading this right, it appears that the 'removal' variable\n> is used before it's initialized:\n\nstatics are allocated from the .bss section, where they are zeroed\nautomatically.\n\n> static struct removal_def {\n>   char path[PATH_MAX];\n>   int len;\n> } removal;\n\nPlumbing this through the call stack instead of using a static sounds\nlike a good idea.  That would mean allocating the removal_def in\nunpack-trees.c::check_updates, I think (see v1.6.3-rc0~147^2~16,\n\"unlink_entry(): introduce schedule_dir_for_removal()\", 2009-02-09 for\ncontext).  Then the loop could be divided into chunks that each use\ntheir own removal_def or something.\n\nSometimes when git needs parallelism and threads don't work, it uses\nfork + exec (aka run_command).  Making the relevant functionality\nthread-safe is generally much nicer, though.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"231883","messageId":"20131211015314.GJ2311@google.com","threadId":"35516","inReplyTo":"20131211014501.GI2311@google.com","subject":"Re: static variables","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-11T01:53:14Z","receivedAt":"2013-12-11T01:53:14Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Stefan Zager wrote:\n\n>> This is probably a naive question, but: there are quite a lot of static\n>> variables in the git code where it's really unnecessary.  Is that just a\n>> historical artifact, or is there some reason to prefer them?\n>\n> Sometimes it's for convenience.\n\nSee for example path.c::git_path and mkpath.\n\nThreaded code uses a specialized thread-safe dialect of the usual C\nused in git.  I wish I had better news to offer.\n"}]}