Re: [PATCH 1/3] builtin-remote: move duplicated cleanup code its own function

From: Jay Soffian <jaysoffian@gmail.com>
Date: Feb 12, 2009, 01:44 UTC
Message-ID: <76718490902111744p27e83238x34cb7004d8e3e48f@mail.gmail.com>
In-Reply-To: <20090212001836.GB30231@coredump.intra.peff.net>

On Wed, Feb 11, 2009 at 7:18 PM, Jeff King <peff@peff.net> wrote:

Show 14 quoted lines

> On Wed, Feb 11, 2009 at 01:01:21AM -0500, Jay Soffian wrote:
>
>> +static void free_remote_ref_states(struct ref_states *states)
>> +{
>> +     /* NEEDSWORK: free remote */
>> +     string_list_clear(&states->new, 0);
>> +     string_list_clear(&states->stale, 0);
>> +     string_list_clear(&states->tracked, 0);
>> +}
>
> Hmm. I don't know anything about this code, so maybe it is not trivial.
> But anytime you are touching an area that NEEDSWORK, I think it is worth
> looking at whether you can fix that problem (since you have already
> spent a few brain cycles understanding what is going on in general).

I spent about 5 minutes which was enough time for me to realize that the reason the previous author left it as "NEEDSWORK" is because fixing it is more than 5 minutes of work. This is the remote object -- maybe you could offer me some clues that allow me to know which of its fields need to be freed individually:

struct remote {
	const char *name;
	int origin;

	const char **url;
	int url_nr;
	int url_alloc;

	const char **push_refspec;
	struct refspec *push;
	int push_refspec_nr;
	int push_refspec_alloc;

	const char **fetch_refspec;
	struct refspec *fetch;
	int fetch_refspec_nr;
	int fetch_refspec_alloc;

	/*
	 * -1 to never fetch tags
	 * 0 to auto-follow tags on heuristic (default)
	 * 1 to always auto-follow tags
	 * 2 to always fetch tags
	 */
	int fetch_tags;
	int skip_default_update;
	int mirror;

	const char *receivepack;
	const char *uploadpack;

	/*
	 * for curl remotes only
	 */
	char *http_proxy;
};

I *think* const is a clue that the field need not be freed, because the pointer is to storage that is on the stack. But I wasn't sure, esp with the double pointers. And I really wasn't sure about the struct pointers.

Really, I only pretend to know C. :-)

j.