On Tue, May 24, 2022 at 2:25 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Kevin Locke <kevin@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
(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.
