Jonathan Nieder <jrnie...@gmail.com> writes: > [...] >> +++ 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
