Re: [PATCH v2 0/4] Delete ignore_env member in struct repository

From: Stefan Beller <sbeller@google.com>
Date: Feb 27, 2018, 18:09 UTC
Message-ID: <CAGZ79ka5MEk89+1MxR5Fsjg8PSSRQWehxKCNcacA3be9+pQ9+Q@mail.gmail.com>
In-Reply-To: <20180227095846.9238-1-pclouds@gmail.com>

On Tue, Feb 27, 2018 at 1:58 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 24 quoted lines

> v2 fixes the incorrect use of consecutive getenv() and adds a comment
> to clarify the role of old_gitdir
>
> Interdiff:
>
> diff --git a/environment.c b/environment.c
> index 95de419de8..47c6e31559 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -14,6 +14,7 @@
>  #include "fmt-merge-msg.h"
>  #include "commit.h"
>  #include "object-store.h"
> +#include "argv-array.h"
>
>  int trust_executable_bit = 1;
>  int trust_ctime = 1;
> @@ -148,18 +149,34 @@ static char *expand_namespace(const char *raw_namespace)
>         return strbuf_detach(&buf, NULL);
>  }
>
> +/* Wrapper of getenv() that returns a strdup value. This value is kept
> + * in argv to be freed later.
> + */

/* comment style, see also Erics reply */

I do not understand why we need to use an argv_array here? I see that the push calls xstrdup and then puts it into the array. So to me this looks like a clever way that we can easily free them all at once using the predefined argv_array_clear() after calling repo_set_gitdir.

That makes sense, though is confusing at first; I would have expected a set_gitdir_args_clear() function that just frees all its strings, whether they are NULL or not.

But given that we are clever with not pushing onto the argv_array in case the getenv returns NULL, this seems to be less work in the usual case of no env variables set.

Ok, I think I like it.

> +static const char *getenv_safe(struct argv_array *argv, const char *name)

I would have expected that we already have such a (or similar) helper in wrapper.c, but it turns out we always take care of getenv manually, and we do not have a wrapper yet.

So only the comment style remains as a nit.

Thanks, Stefan