static variables

static variables

[Stefan Zager]

This is probably a naive question, but: there are quite a lot of static
variables in the git code where it's really unnecessary.  Is that just a
historical artifact, or is there some reason to prefer them?  I'm working
on a patch that will introduce threading, so naturally I'm on the lookout
for static variables.  In general, can I get rid of static variables where
it seems straightforward to do so?

As an example, here's an excerpt from symlnks.c.  In addition to being
static, if I'm reading this right, it appears that the 'removal' variable
is used before it's initialized:

static struct removal_def {
  char path[PATH_MAX];
  int len;
} removal;

static void do_remove_scheduled_dirs(int new_len)
{
  while (removal.len > new_len) {
    removal.path[removal.len] = '\0';
    if (rmdir(removal.path))
      break;
    do {
      removal.len--;
    } while (removal.len > new_len &&
       removal.path[removal.len] != '/');
  }
  removal.len = new_len;
}

void schedule_dir_for_removal(const char *name, int len)
{
  int match_len, last_slash, i, previous_slash;

  match_len = last_slash = i =
    longest_path_match(name, len, removal.path, removal.len,
           &previous_slash);
  /* Find last slash inside 'name' */
  while (i < len) {
    if (name[i] == '/')
      last_slash = i;
    i++;
  }

  /*
   * If we are about to go down the directory tree, we check if
   * we must first go upwards the tree, such that we then can
   * remove possible empty directories as we go upwards.
   */
  if (match_len < last_slash && match_len < removal.len)
    do_remove_scheduled_dirs(match_len);
  /*
   * If we go deeper down the directory tree, we only need to
   * save the new path components as we go down.
   */
  if (match_len < last_slash) {
    memcpy(&removal.path[match_len], &name[match_len],
           last_slash - match_len);
    removal.len = last_slash;
  }
}

void remove_scheduled_dirs(void)
{
  do_remove_scheduled_dirs(0);
}

EOF



Thanks,

Stefan

Re: static variables

[Jonathan Nieder]

Stefan Zager wrote:

> This is probably a naive question, but: there are quite a lot of static
> variables in the git code where it's really unnecessary.  Is that just a
> historical artifact, or is there some reason to prefer them?

Sometimes it's for convenience.  Other times it's to work around C89's
requirement that initializers can't include pointers to automatic
variables, so when using parse_options, old commands tend to use
statics for the variables initialized by options.  (Since then, git
has stopped following that so rigidly, which is probably a good
thing.)

Worse, some functions have static buffers when they need a large
buffer and want to avoid too much allocation churn.  As a general
rule, historically very little of git's code (mostly pack related)
needed to be usable with threads, though of course it would be
excellent to fix more code to be thread-safe.

> As an example, here's an excerpt from symlnks.c.  In addition to being
> static, if I'm reading this right, it appears that the 'removal' variable
> is used before it's initialized:

statics are allocated from the .bss section, where they are zeroed
automatically.

> static struct removal_def {
>   char path[PATH_MAX];
>   int len;
> } removal;

Plumbing this through the call stack instead of using a static sounds
like a good idea.  That would mean allocating the removal_def in
unpack-trees.c::check_updates, I think (see v1.6.3-rc0~147^2~16,
"unlink_entry(): introduce schedule_dir_for_removal()", 2009-02-09 for
context).  Then the loop could be divided into chunks that each use
their own removal_def or something.

Sometimes when git needs parallelism and threads don't work, it uses
fork + exec (aka run_command).  Making the relevant functionality
thread-safe is generally much nicer, though.

Thanks and hope that helps,
Jonathan

Re: static variables

[Jonathan Nieder]

Jonathan Nieder wrote:
> Stefan Zager wrote:

>> This is probably a naive question, but: there are quite a lot of static
>> variables in the git code where it's really unnecessary.  Is that just a
>> historical artifact, or is there some reason to prefer them?
>
> Sometimes it's for convenience.

See for example path.c::git_path and mkpath.

Threaded code uses a specialized thread-safe dialect of the usual C
used in git.  I wish I had better news to offer.