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