git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v4] setup: don't die if realpath(3) fails on getcwd(3)

From
Elijah Newren <newren@gmail.com>
Date
May 25, 2022, 03:51 UTC
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:
Show 31 quoted lines
>
> 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...)
Show 16 quoted lines
>
> 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.

Previous: Junio C HamanoNext: Junio C Hamano
Message 21 of 22 in “setup: don't die if realpath(3) fails on getcwd(3)”
  1. setup: don't die if realpath(3) fails on getcwd(3)Kevin Locke, May 19, 2022
  2. Junio C HamanoMay 20, 2022
  3. Elijah NewrenMay 21, 2022
  4. Kevin LockeMay 21, 2022
  5. Derrick StoleeMay 23, 2022
  6. setup: don't die if realpath(3) fails on getcwd(3)Kevin Locke, May 21, 2022
  7. Derrick StoleeMay 23, 2022
  8. Kevin LockeMay 24, 2022
  9. Elijah NewrenMay 24, 2022
  10. Derrick StoleeMay 24, 2022
  11. Elijah NewrenMay 25, 2022
  12. Ævar Arnfjörð BjarmasonMay 27, 2022
  13. Elijah NewrenMay 28, 2022
  14. setup: don't die if realpath(3) fails on getcwd(3)Kevin Locke, May 24, 2022
  15. Elijah NewrenMay 24, 2022
  16. Derrick StoleeMay 24, 2022
  17. Kevin LockeMay 24, 2022
  18. setup: don't die if realpath(3) fails on getcwd(3)Kevin Locke, May 24, 2022
  19. Derrick StoleeMay 24, 2022
  20. Junio C HamanoMay 24, 2022
  21. Elijah NewrenMay 25, 2022
  22. Junio C HamanoMay 25, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.