Re: [PATCH] refs.c: use a stringlist for repack_without_refs

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 19, 2014, 18:00 UTC
Message-ID: <xmqq4mtvt6jj.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1416359308-14831-1-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 10 quoted lines

> This patch doesn't intend any functional changes. It is just
> a refactoring, which replaces a char** array by a stringlist
> in the function repack_without_refs.
> This is easier to read and maintain as it delivers the same
> functionality with less lines of code less pointers.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
>
> -

Have three of them, not just one, here. (no need to resend to fix only this). Or...

Show 9 quoted lines

>
> This patch was heavily inspired by a part of the ref-transactions-rename
> series[1], but people tend to dislike large series and this part is
> relatively easy to take out and unrelated, so I'll send it as a single
> patch.
>
> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
>
> ---

... next time, write the comments here, where Git already gives you three dashes.

Also mention what you updated and why relative to your earlier round here, if not covered in the log message already.

For example, renaming of delete_refs_list (in v1) to delete_refs (this version) is a sensible change because readers know it is a list from its type being string_list already, but that change is new relative to the codebase, so it could go to the log message ("Having array delete_refs[] and string_list delete_refs_list is redundant; drop the array and give the string_list variable the shorter name", or something like that) if you wanted to.

Show 17 quoted lines

> @@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)
>  {
>  	int result = 0, i;
>  	struct ref_states states;
> -	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
> -	const char **delete_refs;
> +	struct string_list delete_refs = STRING_LIST_INIT_NODUP;
> +	struct string_list_item *ref;
>  	const char *dangling_msg = dry_run
>  		? _(" %s will become dangling!")
>  		: _(" %s has become dangling!");
> @@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)
>  	memset(&states, 0, sizeof(states));
>  	get_remote_ref_states(remote, &states, GET_REF_STATES);
>  
> +	for_each_string_list_item(ref, &delete_refs)
> +		string_list_append(&delete_refs, ref->string);

What are you trying to do here?

Initialise delete_refs to an empty string list, and then iterate over its elements and append them into the same string list???

It looks like a "currently noop, waiting for somebody to throw an item to the list before this code, at which time it turns into an infinite memory eater".

Curious...