From: Elijah Newren <newren@gmail.com>
Date: Wed, 25 May 2022 03:51:50 GMT
Subject: Re: [PATCH v4] setup: don't die if realpath(3) fails on getcwd(3)
Message-ID: <CABPp-BHZQXt-=XtTk7Ez463rhbcQQBNGUVkbbWiavZT+Rsjn_A@mail.gmail.com>
In-Reply-To: <xmqqk0aafwsd.fsf@gitster.g>

On Tue, May 24, 2022 at 2:25 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Kevin Locke <kevin@kevinlocke.name> writes:
>
> >       /* Normalize the directory */
> > -     strbuf_realpath(&tmp, tmp_original_cwd, 1);
> > +     if (!strbuf_realpath(&tmp, tmp_original_cwd, 0)) {
> > +             trace2_data_string("setup", the_repository,
> > +                                "realpath-path", tmp_original_cwd);
> > +             trace2_data_string("setup", the_repository,
> > +                                "realpath-failure", strerror(errno));
> > +             free((char*)tmp_original_cwd);
> > +             tmp_original_cwd = NULL;
> > +             return;
> > +     }
> > +
> >       free((char*)tmp_original_cwd);
> >       tmp_original_cwd = NULL;
> >       startup_info->original_cwd = strbuf_detach(&tmp, NULL);
>
> It is somewhat sad that we cannot readily use FREE_AND_NULL() here.
> If it casted away the constness (see the attached at the end), we
> could have saved two lines from the above snippet.
>
> The startup_info->original_cwd member is initialized to NULL, and
> I think it is a safe assumption that it still is so when the control
> reaches here.  Otherwise, the assignment of strbuf_detach() to it
> without first freeing the current value we see in the post context
> is leaking already.
>
> So, overall this looks good to me.  Anybody else who have already
> spent cycles to review this want to add Reviewed-by: to it?

Well, I added my Reviewed-by to v3 and apparently missed a few things
which were fixed up in v4.  So, my review apparently wasn't careful
enough.  But I am happy with this version, so here it is again:

Reviewed-by: Elijah Newren <newren@gmail.com>

(Now someone is free to spot more problems and embarrass me even more...)

>
> Thanks.
>
> diff --git i/git-compat-util.h w/git-compat-util.h
> index 58fd813bd0..56c6c48461 100644
> --- i/git-compat-util.h
> +++ w/git-compat-util.h
> @@ -976,7 +976,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);
>   * FREE_AND_NULL(ptr) is like free(ptr) followed by ptr = NULL. Note
>   * that ptr is used twice, so don't pass e.g. ptr++.
>   */
> -#define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)
> +#define FREE_AND_NULL(p) do { free((void*)p); (p) = NULL; } while (0)
>
>  #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
>  #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))

I also like this change, even if it feels like it should be part of a
separate patch.