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

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