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

Re: [PATCH 3/3] remote API: don't buggily FREE_AND_NULL(), free() instead

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 7, 2022, 17:29 UTC
Message-ID
<xmqqee00xtyw.fsf@gitster.g>
In-Reply-To
<patch-3.3-062fb3f454e-20220607T154520Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> Change the buggy "remote_clear()" function to stop pretending to to be
"buggy" -> ""; "to to" -> "to".

Over-using FREE_AND_NULL() is indeed irritating, but as long as we are not reusing the struct whose member are getting freed, there is no "bug" per-se, no?

Show 5 quoted lines
> able to zero out a "struct remote". Setting "url" and "pushurl" to
> NULL results in an invalid state unless the corresponding "url_nr" and
> "pushurl_nr" are also set to zero.
>
> In this case however we do not use the "struct remote", so the
"use" -> "reuse", with comma around "however" on both sides, probably.
> FREE_AND_NULL() pattern added in fd3cb0501e1 (remote: move static
> variables into per-repository struct, 2021-11-17) can be replaced with
> free().
Yay.
> The API was also odd in that remote_state_new() would xmalloc() for us,
> but the user had to free() it themselves, let's instead change the
> behavior to have the destructor free() what we malloc() in the
> constructer.

I am not sure if remote_clear() that does not free the remote goes well with remote_state_clear() that does free remote_state. As long as it is clear whose responsibility to release resources is, we can go either way, but helper functions with similar names having quite different resource maintainance semantics looks like a potential source of confusion to me.

Show 87 quoted lines
> In this case this appears to have been done for consistency with
> repo_clear(), let's instead have repo_clear() handle the NULL-ing of
> its "remote_state", and not attempt to reset the structure in remote.c
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  remote.c     | 14 +++++++-------
>  remote.h     | 10 +++++++++-
>  repository.c |  2 +-
>  3 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 0b243b090d9..c6ce04dacb7 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -147,15 +147,15 @@ static void remote_clear(struct remote *remote)
>  
>  	for (i = 0; i < remote->url_nr; i++)
>  		free((char *)remote->url[i]);
> -	FREE_AND_NULL(remote->url);
> +	free(remote->url);
>  
>  	for (i = 0; i < remote->pushurl_nr; i++)
>  		free((char *)remote->pushurl[i]);
> -	FREE_AND_NULL(remote->pushurl);
> +	free(remote->pushurl);
>  	free((char *)remote->receivepack);
>  	free((char *)remote->uploadpack);
> -	FREE_AND_NULL(remote->http_proxy);
> -	FREE_AND_NULL(remote->http_proxy_authmethod);
> +	free(remote->http_proxy);
> +	free(remote->http_proxy_authmethod);
>  }
>  
>  static void add_merge(struct branch *branch, const char *name)
> @@ -2720,12 +2720,12 @@ void remote_state_clear(struct remote_state *remote_state)
>  
>  	for (i = 0; i < remote_state->remotes_nr; i++)
>  		remote_clear(remote_state->remotes[i]);
> -	FREE_AND_NULL(remote_state->remotes);
> -	remote_state->remotes_alloc = 0;
> -	remote_state->remotes_nr = 0;
> +	free(remote_state->remotes);
>  
>  	hashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);
>  	hashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent);
> +
> +	free(remote_state);
>  }
>  
>  /*
> diff --git a/remote.h b/remote.h
> index dd4402436f1..d91b2b29373 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -54,9 +54,17 @@ struct remote_state {
>  	int initialized;
>  };
>  
> -void remote_state_clear(struct remote_state *remote_state);
> +/**
> + * xmalloc() a "struct remote_state" and initialize it. The resulting
> + * data should be free'd with remote_state_clear().
> + */
>  struct remote_state *remote_state_new(void);
>  
> +/**
> + * free() the structure returned by remote_state_new().
> + */
> +void remote_state_clear(struct remote_state *remote_state);
> +
>  struct remote {
>  	struct hashmap_entry ent;
>  
> diff --git a/repository.c b/repository.c
> index 5d166b692c8..0a6df6937e4 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -292,7 +292,7 @@ void repo_clear(struct repository *repo)
>  
>  	if (repo->remote_state) {
>  		remote_state_clear(repo->remote_state);
> -		FREE_AND_NULL(repo->remote_state);
> +		repo->remote_state = NULL;
>  	}
>  
>  	repo_clear_path_cache(&repo->cached_paths);
Previous: Junio C HamanoNext: Junio C Hamano
Message 50 of 51 in “Fix GCC -fanalyzer warnings & add -fanalyzer DEVOPTS mode”
  1. 00/15 Fix GCC -fanalyzer warnings & add -fanalyzer DEVOPTS modeÆvar Arnfjörð Bjarmason, Jun 3, 2022
  2. 01/15 remote.c: don't dereference NULL in freeing loopÆvar Arnfjörð Bjarmason, Jun 3, 2022
  3. René ScharfeJun 3, 2022
  4. Junio C HamanoJun 3, 2022
  5. Glen ChooJun 3, 2022
  6. Phillip WoodJun 4, 2022
  7. Ævar Arnfjörð BjarmasonJun 4, 2022
  8. 02/15 pull.c: don't feed NULL to strcmp() on get_rebase_fork_point() pathÆvar Arnfjörð Bjarmason, Jun 3, 2022
  9. René ScharfeJun 3, 2022
  10. 03/15 reftable: don't memset() a NULL from failed malloc()Ævar Arnfjörð Bjarmason, Jun 3, 2022
  11. René ScharfeJun 3, 2022
  12. Ævar Arnfjörð BjarmasonJun 4, 2022
  13. René ScharfeJun 4, 2022
  14. Ævar Arnfjörð BjarmasonJun 4, 2022
  15. René ScharfeJun 4, 2022
  16. Junio C HamanoJun 6, 2022
  17. Ævar Arnfjörð BjarmasonJun 6, 2022
  18. Junio C HamanoJun 6, 2022
  19. Ævar Arnfjörð BjarmasonJun 6, 2022
  20. 04/15 diff-lib.c: don't dereference NULL in oneway_diff()Ævar Arnfjörð Bjarmason, Jun 3, 2022
  21. René ScharfeJun 3, 2022
  22. 05/15 refs/packed-backend.c: add a BUG() if iter is NULLÆvar Arnfjörð Bjarmason, Jun 3, 2022
  23. René ScharfeJun 3, 2022
  24. 06/15 ref-filter.c: BUG() out on show_ref() with NULL refnameÆvar Arnfjörð Bjarmason, Jun 3, 2022
  25. René ScharfeJun 4, 2022
  26. 07/15 strbuf.c: placate -fanalyzer in strbuf_grow()Ævar Arnfjörð Bjarmason, Jun 3, 2022
  27. René ScharfeJun 4, 2022
  28. Phillip WoodJun 4, 2022
  29. Ævar Arnfjörð BjarmasonJun 4, 2022
  30. René ScharfeJun 4, 2022
  31. Phillip WoodJun 5, 2022
  32. 08/15 strbuf.c: use st_add3(), not unsigned_add_overflows()Ævar Arnfjörð Bjarmason, Jun 3, 2022
  33. René ScharfeJun 4, 2022
  34. 09/15 add-patch: assert parse_diff() expectations with BUG()Ævar Arnfjörð Bjarmason, Jun 3, 2022
  35. Phillip WoodJun 4, 2022
  36. 10/15 reftable: don't have reader_get_block() confuse -fanalyzerÆvar Arnfjörð Bjarmason, Jun 3, 2022
  37. 11/15 blame.c: clarify the state of "final_commit" for -fanalyzerÆvar Arnfjörð Bjarmason, Jun 3, 2022
  38. 14/15 config.mak.dev: add a DEVOPTS=analyzer mode to use GCC's -fanalyzerÆvar Arnfjörð Bjarmason, Jun 3, 2022
  39. 12/15 pack.h: wrap write_*file*() functionsÆvar Arnfjörð Bjarmason, Jun 3, 2022
  40. 13/15 pack-write API: pass down "verify" not arbitrary flagsÆvar Arnfjörð Bjarmason, Jun 3, 2022
  41. 15/15 config.mak.dev: add and use ASSERT_FOR_FANALYZER() macroÆvar Arnfjörð Bjarmason, Jun 3, 2022
  42. Phillip WoodJun 4, 2022
  43. 0/3 remote API: fix -fanalyzer-spotted freeing issueÆvar Arnfjörð Bjarmason, Jun 7, 2022
  44. 1/3 remote.c: remove braces from one-statement "for"-loopsÆvar Arnfjörð Bjarmason, Jun 7, 2022
  45. 2/3 remote.c: don't dereference NULL in freeing loopÆvar Arnfjörð Bjarmason, Jun 7, 2022
  46. Junio C HamanoJun 7, 2022
  47. 3/3 remote API: don't buggily FREE_AND_NULL(), free() insteadÆvar Arnfjörð Bjarmason, Jun 7, 2022
  48. Glen ChooJun 7, 2022
  49. Junio C HamanoJun 7, 2022
  50. Junio C HamanoJun 7, 2022
  51. Junio C HamanoJun 7, 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.