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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Derrick Stolee" <dstolee@microsoft.com>,
	"Jeff Hostetler" <git@jeffhostetler.com>,
	"Patrick Steinhardt" <ps@pks.im>, "Jeff King" <peff@peff.net>,
	"Taylor Blau" <me@ttaylorr.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc.
Date: Tue, 21 Sep 2021 15:12:58 +0200	[thread overview]
Message-ID: <cover-v4-0.5-00000000000-20210921T131003Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-v3-0.5-00000000000-20210919T084703Z-avarab@gmail.com>

A hopefully final re-roll addressing Taylor's v3 review, except for
the suggestion (that I read as) perhaps retaining the test-only code,
which I've decided not to do per
http://lore.kernel.org/git/87tuieakms.fsf@evledraar.gmail.com

The x(un)setenv() now returns void, and the error messages are less
chatty, I also improved a BUG() message in 4/5 that we end up deleting
in 5/5 anyway, so it doesn't matter for the end-state, just for
understanding the patches.

Æ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                            |  12 +++
 9 files changed, 98 insertions(+), 94 deletions(-)

Range-diff against v3:
1:  4b320edc933 ! 1:  4dd317ab65e wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c
    @@ Commit message
         errno?) the worst we'll do is die with a nonsensical errno value, but
         we'll want to die in either case.

    -    We could make these return "void" (as far as I can tell there's no
    -    other x*() wrappers that needed to make that decision before),
    -    i.e. our "return 0" is only to indicate that we didn't error, which we
    -    would have died on. Let's return "int" instead to be consistent with
    -    the C library function signatures, including for any future code that
    -    expects a pointer to a setenv()-like function.
    +    Let's make these return "void" instead of "int". As far as I can tell
    +    there's no other x*() wrappers that needed to make the decision of
    +    deviating from the signature in the C library, but since their return
    +    value is only used to indicate errors (so we'd die here), we can catch
    +    unreachable code such as
    +
    +        if (xsetenv(...) < 0)
    +            [...];

         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
    @@ git-compat-util.h: void *xmemdupz(const void *data, size_t len);
      char *xstrndup(const char *str, size_t len);
      void *xrealloc(void *ptr, size_t size);
      void *xcalloc(size_t nmemb, size_t size);
    -+int xsetenv(const char *name, const char *value, int overwrite);
    -+int xunsetenv(const char *name);
    ++void xsetenv(const char *name, const char *value, int overwrite);
    ++void xunsetenv(const char *name);
      void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
      const char *mmap_os_err(void);
      void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);
    @@ wrapper.c: void *xcalloc(size_t nmemb, size_t size)
      	return ret;
      }

    -+int xsetenv(const char *name, const char *value, int overwrite)
    ++void xsetenv(const char *name, const char *value, int overwrite)
     +{
     +	if (setenv(name, value, overwrite))
    -+		die_errno("setenv(%s, '%s', %d) failed", name ? name : "(null)",
    -+			  value, overwrite);
    -+	return 0;
    ++		die_errno(_("could not setenv '%s'"), name ? name : "(null)");
     +}
     +
    -+int xunsetenv(const char *name)
    ++void xunsetenv(const char *name)
     +{
     +	if (!unsetenv(name))
    -+		die_errno("unsetenv(%s) failed", name ? name : "(null)");
    -+	return 0;
    ++		die_errno(_("could not unsetenv '%s'"), name ? name : "(null)");
     +}
     +
      /*
2:  ece340af764 = 2:  3dc37521184 environment.c: remove test-specific "ignore_untracked..." variable
3:  d837d905825 ! 3:  b36b23ee173 read-cache & fetch-negotiator: check "enum" values in switch()
    @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,
      		return;
     +	case FETCH_NEGOTIATION_NONE:
     +	case FETCH_NEGOTIATION_UNSET:
    -+		BUG("FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()");
    ++		BUG("FETCH_NEGOTIATION_{NONE,UNSET} used outside of prepare_repo_settings()!");
      	}
      }

    @@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)
     +	case UNTRACKED_CACHE_KEEP:
     +		break;
     +	case UNTRACKED_CACHE_UNSET:
    -+		BUG("UNTRACKED_CACHE_UNSET only in prepare_repo_settings()");
    ++		BUG("UNTRACKED_CACHE_UNSET used outside of prepare_repo_settings()!");
     +	}
      }

4:  28286a61162 ! 4:  c9f143b26f1 repo-settings.c: simplify the setup
    @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,
      		return;
     -	case FETCH_NEGOTIATION_NONE:
     -	case FETCH_NEGOTIATION_UNSET:
    --		BUG("FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()");
    +-		BUG("FETCH_NEGOTIATION_{NONE,UNSET} used outside of prepare_repo_settings()!");
      	}
      }

    @@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)
     +		 */
      		break;
     -	case UNTRACKED_CACHE_UNSET:
    --		BUG("UNTRACKED_CACHE_UNSET only in prepare_repo_settings()");
    +-		BUG("UNTRACKED_CACHE_UNSET used outside of prepare_repo_settings()!");
      	}
      }

5:  3cc033b8864 = 5:  aadd4c42923 repository.h: don't use a mix of int and bitfields
-- 
2.33.0.1098.gf02a64c1a2d