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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2014, 16:47 UTC
Message-ID: <xmqqr3wx4y5x.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20141120021540.GF6527@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 20 quoted lines

> [...]
>> +++ b/builtin/remote.c
> [...]
>> @@ -1361,8 +1352,9 @@ static int prune_remote(const char *remote, int dry_run)
>>  			       abbrev_ref(refname, "refs/remotes/"));
>>  	}
>>  
>> -	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
>> -	string_list_clear(&delete_refs_list, 0);
>> +	sort_string_list(&delete_refs);
>> +	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs);
>> +	string_list_clear(&delete_refs, 0);
>>  
>>  	free_remote_ref_states(&states);
>>  	return result;
>
> Micronit: it would be clearer (and easier to remember to free the list
> in other code paths if this function gains more 'return' statements)
> with the string_list_clear in the same block as other code that frees
> resources (i.e., if the blank line moved one line up).

Thanks for a careful reading. This kind of attention to detail helps the longer term health of the codebase.

> The function has to be able to write to 'err' on error, so I think the
> comment doesn't have to mention that err must be non-NULL.  Any caller
> that tries to pass NULL will get an assertion error quickly.

That invites a bit of question, though.

An equally plausible alternative definition for set of API functions that take strbuf *err is to pass it only when you care about the explanation of the error (i.e. it is valid for "git cmd --quiet" to pass NULL there) [*1*] (do we already have such a function?). And the comment may help clarifying which is which. I however think we shouldn't have mixtures (formatting into "strbuf *err" may be costly when we know we are asked to fail silently, but an error path is not usually performance sensitive).

[Footnote]

*1* With yet another one, a function may call error() on its own when a NULL is passed to strbuf *err, but let's not go there.