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@xxxxxxxxxxxxxxxxxxx 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
