Re: [RFC PATCH v2 4/7] parse: create new library for parsing strings and env values

From: Glen Choo <chooglen@google.com>
Date: Aug 10, 2023, 23:21 UTC
Message-ID: <kl6ledka8owj.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20230810163654.275023-4-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

Show 8 quoted lines

> While string and environment value parsing is mainly consumed by
> config.c, there are other files that only need parsing functionality and
> not config functionality. By separating out string and environment value
> parsing from config, those files can instead be dependent on parse,
> which has a much smaller dependency chain than config.
>
> Move general string and env parsing functions from config.[ch] to
> parse.[ch].

An unstated purpose of this patch is that parse.[ch] becomes part of git-std-lib, but not config.[ch], right?

I think it's reasonable to have the string value parsing logic in git-std-lib, e.g. this parsing snippet from diff.c seems like a good thing to put into a library that wants to accept user input:

  static int parse_color_moved(const char *arg)
  {
    switch (git_parse_maybe_bool(arg)) {
    case 0:
      return COLOR_MOVED_NO;
    case 1:
      return COLOR_MOVED_DEFAULT;
    default:
      break;
    }

    if (!strcmp(arg, "no"))
      return COLOR_MOVED_NO;
    else if (!strcmp(arg, "plain"))
      return COLOR_MOVED_PLAIN;
    else if (!strcmp(arg, "blocks"))
      return COLOR_MOVED_BLOCKS;
    /* ... */
  }

But, I don't see a why a non-Git caller would want environment value parsing in git-std-lib. I wouldn't think that libraries should be reading Git-formatted environment variables. If I had to guess, you arranged it this way because you want to keep xmalloc in git-std-lib, which has a dependency on env var parsing here:

  static int memory_limit_check(size_t size, int gentle)
  {
    static size_t limit = 0;
    if (!limit) {
      limit = git_env_ulong("GIT_ALLOC_LIMIT", 0);
      if (!limit)
        limit = SIZE_MAX;
    }
    if (size > limit) {
      if (gentle) {
        error("attempting to allocate %"PRIuMAX" over limit %"PRIuMAX,
              (uintmax_t)size, (uintmax_t)limit);
        return -1;
      } else
        die("attempting to allocate %"PRIuMAX" over limit %"PRIuMAX,
            (uintmax_t)size, (uintmax_t)limit);
    }
    return 0;
  }

If we libified this as-is, wouldn't our caller start paying attention to the GIT_ALLOC_LIMIT environment variable? That seems like an undesirable side effect.

I see later in the series that you have "stubs", which are presumably entrypoints for the caller to specify their own implementations of Git-specific things. If so, then an alternative would be to provide a "stub" to get the memory limit, something like:

  /* wrapper.h aka the things to stub */
  size_t git_get_memory_limit(void);

  /* stub-wrapper-or-something.c aka Git's implementation of the stub */

  #include "wrapper.h"
  size_t git_get_memory_limit(void)
  {
      return git_env_ulong("GIT_ALLOC_LIMIT", 0);
  }

  /* wrapper.c aka the thing in git-stb-lib */
  static int memory_limit_check(size_t size, int gentle)
  {
    static size_t limit = 0;
    if (!limit) {
      limit = git_get_memory_limit();
      if (!limit)
        limit = SIZE_MAX;
    }
    if (size > limit) {
      if (gentle) {
        error("attempting to allocate %"PRIuMAX" over limit %"PRIuMAX,
              (uintmax_t)size, (uintmax_t)limit);
        return -1;
      } else
        die("attempting to allocate %"PRIuMAX" over limit %"PRIuMAX,
            (uintmax_t)size, (uintmax_t)limit);
    }
    return 0;
  }