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

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

From
Jeff King <peff@peff.net>
Date
Jun 5, 2017, 08:18 UTC
Message-ID
<20170605081845.tvzidc5nblbnuner@sigill.intra.peff.net>
In-Reply-To
<CAM0VKjkfuyBzZmpJL47xmK24EOh6X5OjrxtxVdsgGupAseT_wQ@mail.gmail.com>
On Wed, May 31, 2017 at 11:34:23AM +0200, SZEDER Gábor wrote:
Show 31 quoted lines
> >> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
> >> +{
> >> +     struct refspec *rs;
> >> +
> >> +     add_fetch_refspec(remote, refspec);
> >> +     rs = parse_fetch_refspec(1, &refspec);
> >> +     REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
> >> +     remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
> >> +
> >> +     /* Not free_refspecs(), as we copied its pointers above */
> >> +     free(rs);
> >> +}
> >
> > What happens here if remote->fetch isn't already initialized? I think
> > we'd end up with a bunch of garbage values. That's what I was trying to
> > protect against in my original suggestion.
> >
> > I'm not sure if that's possible or not. We seem to initialize it in both
> > remote_get() and for_each_remote(), and I don't think there are any
> > other ways to get a remote.
> 
> The only place creating remotes is remote.c:make_remote(), which
> calloc()s the required memory, making all of struct remote's fields
> zero-initialized.  In case of clone the common case is that the user
> doesn't specify any additional fetch refspecs, so remote->fetch will
> still be NULL after full initialization and when
> add_and_parse_fetch_refspec() is called with the default fetch
> refspec, meaning we can't 'if (remote->fetch) { parse ... }'.  OTOH,
> all functions involved can cope with the fetch-refspec-related fields
> being 0/NULL, and at the time remote->fetch_refspec_nr-1 is used for
> array indexing it's not 0 anymore.

Yeah, I agree it is safe now. I'm just worried about some function in remote.c later doing:

   read_config();
   add_and_parse_fetch_refspec(remotes[0], whatever);

which leaves the struct in an inconsistent state (we realloc NULL which allocates from scratch, and all of the other entries in remote->fetch end up uninitialized). Can we at least add an assertion like:

  if (!remote->fetch)
	BUG("cannot add refspec to an unparsed remote");
?
-Peff
Previous: SZEDER GáborNext: SZEDER Gábor
Message 19 of 48 in “[PATCHv3 0/4] clone: respect configured fetch respecs during initial fetch”
  1. SZEDER GáborMay 15, 2017
  2. 2/4 Documentation/clone: document ignored configuration variablesSZEDER Gábor, May 15, 2017
  3. SZEDER GáborMay 26, 2017
  4. 1/4 clone: respect additional configured fetch refspecs during initial fetchSZEDER Gábor, May 15, 2017
  5. Jeff KingMay 15, 2017
  6. SZEDER GáborMay 26, 2017
  7. Jeff KingMay 26, 2017
  8. Junio C HamanoMay 30, 2017
  9. Jeff KingMay 30, 2017
  10. SZEDER GáborMay 30, 2017
  11. 1/2 clone: respect additional configured fetch refspecs during initial fetchSZEDER Gábor, May 30, 2017
  12. 2/2 Documentation/clone: document ignored configuration variablesSZEDER Gábor, May 30, 2017
  13. Ævar Arnfjörð BjarmasonMay 30, 2017
  14. SZEDER GáborMay 31, 2017
  15. Ævar Arnfjörð BjarmasonMay 31, 2017
  16. Jonathan NiederJun 13, 2017
  17. Jeff KingMay 31, 2017
  18. SZEDER GáborMay 31, 2017
  19. Jeff KingJun 5, 2017
  20. SZEDER GáborJun 6, 2017
  21. Jeff KingJun 6, 2017
  22. SZEDER GáborJun 7, 2017
  23. Jonathan NiederJun 14, 2017
  24. Jeff KingJun 14, 2017
  25. SZEDER GáborJun 16, 2017
  26. 0/2 clone: respect additional configured fetch refspecs during initial fetchSZEDER Gábor, Jun 16, 2017
  27. 2/2 Documentation/clone: document ignored configuration variablesSZEDER Gábor, Jun 16, 2017
  28. Ævar Arnfjörð BjarmasonJun 16, 2017
  29. Jonathan NiederJun 16, 2017
  30. Ævar Arnfjörð BjarmasonJun 16, 2017
  31. SZEDER GáborJun 16, 2017
  32. Jonathan NiederJun 16, 2017
  33. Junio C HamanoJun 16, 2017
  34. 1/2 clone: respect additional configured fetch refspecs during initial fetchSZEDER Gábor, Jun 16, 2017
  35. Jonathan NiederJun 16, 2017
  36. Jeff KingJun 17, 2017
  37. Junio C HamanoJun 22, 2017
  38. Junio C HamanoAug 12, 2017
  39. remote: drop free_refspecs() functionJeff King, May 31, 2017
  40. Jonathan NiederJun 14, 2017
  41. 3/4 remote: drop free_refspecs() functionSZEDER Gábor, May 15, 2017
  42. 4/4 clone: use free_refspec() to free refspec listSZEDER Gábor, May 15, 2017
  43. SZEDER GáborMay 15, 2017
  44. Jeff KingMay 15, 2017
  45. Junio C HamanoMay 23, 2017
  46. Jeff KingMay 23, 2017
  47. SZEDER GáborMay 23, 2017
  48. Jeff KingMay 15, 2017

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.