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

Message

Ævar Arnfjörð Bjarmason Sept. 21, 2021, 1:12 p.m. UTC

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/[e-mail]

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

Comments

Derrick Stolee Sept. 21, 2021, 3:58 p.m. UTC | #1

On 9/21/2021 9:12 AM, Ævar Arnfjörð Bjarmason wrote:
> 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/[e-mail]
> 
> 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

This version looks good to me.

Thanks,
-Stolee

Taylor Blau Sept. 21, 2021, 8:46 p.m. UTC | #2

On Tue, Sep 21, 2021 at 11:58:34AM -0400, Derrick Stolee wrote:
> On 9/21/2021 9:12 AM, Ævar Arnfjörð Bjarmason wrote:
> > 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/[e-mail]
> >
> > [...]
>
> This version looks good to me.

Likewise. Thanks Ævar for addressing my review.

    Reviewed-by: Taylor Blau <[e-mail]>

Thanks,
Taylor

Junio C Hamano Sept. 22, 2021, 8:23 p.m. UTC | #3

Taylor Blau <[e-mail]> writes:

> On Tue, Sep 21, 2021 at 11:58:34AM -0400, Derrick Stolee wrote:
>> On 9/21/2021 9:12 AM, Ævar Arnfjörð Bjarmason wrote:
>> > 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/[e-mail]
>> >
>> > [...]
>>
>> This version looks good to me.
>
> Likewise. Thanks Ævar for addressing my review.

Yeah, I think this one is good enough for 'next'.

Thanks, all.