[v3,0/5] repo-settings.c: refactor for clarity, get rid of hacks etc.

Message

Ævar Arnfjörð Bjarmason Sept. 19, 2021, 8:47 a.m. UTC

This simplification of repo-settings.c addresses Junio's feedback,
changes:

 * Get rid of thename=NULL checkfrom the main codepath of
   xsetenv()/xunsetenv() (but don't feed NULL to %s).
 * Expanded commit message of 2/5 to discuss the safety/desire of
   overriding GIT_CONFIG_COUNT.
 * Marked unreachable code in 3/5 (later removed in 4/5) with a BUG().
 * I kept the UNTRACKED_CACHE_KEEP value instead of
   UNTRACKED_CACHE_UNSET for indicating the default.
 * Don't add a "return" to a void function.

For v2 see: https://lore.kernel.org/git/[e-mail]/

Ævar Arnfjörð Bjarmason (5):
  wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c
  environment.c: remove test-specific "ignore_untracked..." variable
  read-cache & fetch-negotiator: check "enum" values in switch()
  repo-settings.c: simplify the setup
  repository.h: don't use a mix of int and bitfields

 cache.h                              |   7 --
 environment.c                        |  10 +--
 fetch-negotiator.c                   |   1 -
 git-compat-util.h                    |   2 +
 read-cache.c                         |  19 +++--
 repo-settings.c                      | 115 +++++++++++++--------------
 repository.h                         |  20 ++---
 t/helper/test-dump-untracked-cache.c |   6 +-
 wrapper.c                            |  15 ++++
 9 files changed, 101 insertions(+), 94 deletions(-)

Range-diff against v2:
1:  49706b26642 ! 1:  4b320edc933 wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c
    @@ Commit message
         the C library function signatures, including for any future code that
         expects a pointer to a setenv()-like function.
     
    +    I think it would be OK skip the NULL check of the "name" here for the
    +    calls to die_errno(). Almost all of our setenv() callers are taking a
    +    constant string hardcoded in the source as the first argument, and for
    +    the rest we can probably assume they've done the NULL check
    +    themselves. Even if they didn't, modern C libraries are forgiving
    +    about it (e.g. glibc formatting it as "(null)"), on those that aren't,
    +    well, we were about to die anyway. But let's include the check anyway
    +    for good measure.
    +
         1. https://pubs.opengroup.org/onlinepubs/009604499/functions/setenv.html
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
    @@ wrapper.c: void *xcalloc(size_t nmemb, size_t size)
      
     +int xsetenv(const char *name, const char *value, int overwrite)
     +{
    -+	if (!name)
    -+		die("xsetenv() got a NULL name, setenv() would return EINVAL");
     +	if (setenv(name, value, overwrite))
    -+		die_errno("setenv(%s, '%s', %d) failed", name, value, overwrite);
    ++		die_errno("setenv(%s, '%s', %d) failed", name ? name : "(null)",
    ++			  value, overwrite);
     +	return 0;
     +}
     +
     +int xunsetenv(const char *name)
     +{
    -+	if (!name)
    -+		die("xunsetenv() got a NULL name, xunsetenv() would return EINVAL");
     +	if (!unsetenv(name))
    -+		die_errno("unsetenv(%s) failed", name);
    ++		die_errno("unsetenv(%s) failed", name ? name : "(null)");
     +	return 0;
     +}
     +
2:  57290842f0f ! 2:  ece340af764 environment.c: remove test-specific "ignore_untracked..." variable
    @@ Commit message
         t7063-status-untracked-cache.sh, but let's fail earlier anyway if that
         were to happen.
     
    +    This breaks any parent process that's potentially using the
    +    GIT_CONFIG_* and GIT_CONFIG_PARAMETERS mechanism to pass one-shot
    +    config setting down to a git subprocess, but in this case we don't
    +    care about the general case of such potential parents. This process
    +    neither spawns other "git" processes, nor is it interested in other
    +    configuration. We might want to pick up other test modes here, but
    +    those will be passed via GIT_TEST_* environment variables.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## cache.h ##
3:  9f1bb0496ed ! 3:  d837d905825 read-cache & fetch-negotiator: check "enum" values in switch()
    @@ Commit message
         case in fetch_negotiator_init() in favor of checking for
         "FETCH_NEGOTIATION_UNSET" and "FETCH_NEGOTIATION_NONE".
     
    +    As will be discussed in a subsequent we'll only ever have either of
    +    these set to FETCH_NEGOTIATION_NONE, FETCH_NEGOTIATION_UNSET and
    +    UNTRACKED_CACHE_UNSET within the prepare_repo_settings() function
    +    itself. In preparation for fixing that code let's add a BUG() here to
    +    mark this as unreachable code.
    +
         See ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)
         for when the "unset" and "keep" handling for core.untrackedCache was
         consolidated, and aaf633c2ad1 (repo-settings: create
    @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,
      
      	case FETCH_NEGOTIATION_DEFAULT:
     -	default:
    -+	case FETCH_NEGOTIATION_UNSET:
    -+	case FETCH_NEGOTIATION_NONE:
      		default_negotiator_init(negotiator);
      		return;
    ++	case FETCH_NEGOTIATION_NONE:
    ++	case FETCH_NEGOTIATION_UNSET:
    ++		BUG("FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()");
      	}
    + }
     
      ## read-cache.c ##
     @@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)
    @@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)
     +	case UNTRACKED_CACHE_WRITE:
      		add_untracked_cache(istate);
     +		break;
    -+	case UNTRACKED_CACHE_UNSET:
     +	case UNTRACKED_CACHE_KEEP:
     +		break;
    ++	case UNTRACKED_CACHE_UNSET:
    ++		BUG("UNTRACKED_CACHE_UNSET only in prepare_repo_settings()");
     +	}
    -+	return;
      }
      
      static void tweak_split_index(struct index_state *istate)
4:  b28ad2b2607 ! 4:  28286a61162 repo-settings.c: simplify the setup
    @@ Commit message
     
            In [4] the "unset" and "keep" handling for core.untrackedCache was
            consolidated. But it while we understand the "keep" value, we don't
    -       handle it differently than the case of any other unknown value. So
    -       we can remove UNTRACKED_CACHE_KEEP. It's not handled any
    -       differently than UNTRACKED_CACHE_UNSET once we get past the config
    -       parsing step.
    +       handle it differently than the case of any other unknown value.
    +
    +       So let's retain UNTRACKED_CACHE_KEEP and remove the
    +       UNTRACKED_CACHE_UNSET setting (which was always implicitly
    +       UNTRACKED_CACHE_KEEP before). We don't need to inform any code
    +       after prepare_repo_settings() that the setting was "unset", as far
    +       as anyone else is concerned it's core.untrackedCache=keep. if
    +       "core.untrackedcache" isn't present in the config.
     
          * FETCH_NEGOTIATION_UNSET & FETCH_NEGOTIATION_NONE:
     
    @@ Commit message
     
      ## fetch-negotiator.c ##
     @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,
    - 		return;
    - 
      	case FETCH_NEGOTIATION_DEFAULT:
    --	case FETCH_NEGOTIATION_UNSET:
    --	case FETCH_NEGOTIATION_NONE:
      		default_negotiator_init(negotiator);
      		return;
    +-	case FETCH_NEGOTIATION_NONE:
    +-	case FETCH_NEGOTIATION_UNSET:
    +-		BUG("FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()");
      	}
    + }
     
      ## read-cache.c ##
     @@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)
      		add_untracked_cache(istate);
      		break;
    - 	case UNTRACKED_CACHE_UNSET:
    --	case UNTRACKED_CACHE_KEEP:
    -+		/* This includes core.untrackedCache=keep */
    + 	case UNTRACKED_CACHE_KEEP:
    ++		/*
    ++		 * Either an explicit "core.untrackedCache=keep", the
    ++		 * default if "core.untrackedCache" isn't configured,
    ++		 * or a fallback on an unknown "core.untrackedCache"
    ++		 * value.
    ++		 */
      		break;
    +-	case UNTRACKED_CACHE_UNSET:
    +-		BUG("UNTRACKED_CACHE_UNSET only in prepare_repo_settings()");
      	}
    - 	return;
    + }
    + 
     
      ## repo-settings.c ##
     @@
    @@ repo-settings.c
      	/* Defaults */
     -	memset(&r->settings, -1, sizeof(r->settings));
     +	r->settings.index_version = -1;
    -+	r->settings.core_untracked_cache = UNTRACKED_CACHE_UNSET;
    ++	r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;
     +	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
     +
     +	/* Booleans config or default, cascades to other settings */
    @@ repo-settings.c
     +		/*
     +		 * If it's set to "keep", or some other non-boolean
     +		 * value then "v < 0". Then we do nothing and keep it
    -+		 * at UNTRACKED_CACHE_UNSET.
    ++		 * at the default of UNTRACKED_CACHE_KEEP.
     +		 */
     +		if (v >= 0)
     +			r->settings.core_untracked_cache = v ?
    @@ repository.h: struct submodule_cache;
     -	UNTRACKED_CACHE_REMOVE = 0,
     -	UNTRACKED_CACHE_KEEP = 1,
     -	UNTRACKED_CACHE_WRITE = 2
    -+	UNTRACKED_CACHE_UNSET,
    ++	UNTRACKED_CACHE_KEEP,
     +	UNTRACKED_CACHE_REMOVE,
     +	UNTRACKED_CACHE_WRITE,
      };
5:  0b5f213a639 = 5:  3cc033b8864 repository.h: don't use a mix of int and bitfields