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

[Stefan Beller <sbeller@google.com>]

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