pack: rename git_packfile_stream_free

[pks-t]

The function git_packfile_stream_free frees all state of the packfile
stream without freeing the structure itself. Thus, the function is
misnamed, as we usually call such a function a "clear" function. Rename
it to make clear that in fact it does not free the structure.

[ethomson]

Not sure I agree here, what about git_buf_free, git_zstream_free... clear suggests that it doesn't necessarily free underlying memory (eg, git_buf_clear).

I don't really mind that these are _free if they don't free the structure itself. But I think I did give it a raised eyebrow when I saw it. But I don't think _clear is correct, so maybe we should come up with something else. And if we're going to do it then we need to be consistent.

[pks-t]

Huh, you're correct about those. I don't mind to find a better term than clear, but I do want those two cases of freeing or not freeing to be distinct. You just cannot see whether the implementation will try to free the pointer itself or not if both types are being called free, and this becomes confusing really fast. I've tripped about this in reviewing another PR, where I first thought the change was incorrect as a stack-allocated pointer was being passed to a free function. And free to me means just that: free the structure and everything else that is associated with it, just like free(3).

I don't really have good alternatives. Some things that came to my mind:

• release
• purge
• wipe

I think I like wipe the best. It shows that it is being cleaned up somehow, but with wiping you don't remove the structure itself. Opinions?

I also definitly agree about being consistent. I didn't think of those other occurrences.

[ethomson]

Hm. I know where you're coming from but this has never bothered me in that way, I don't think. Curious about @carlosmn's opinions.

Note also that git_buf_free is public API which complicates things.

If we do make a change, what about dispose?

[pks-t]

Dispose would be fine with me, sure.

And yeah, the git_buf thing is rather unfortunate. I'm definitly not in favor of changing the API, so I'd treat this as an exception to the rule.

[ethomson]

OK. I'm leaning towards thinking that this makes sense. I think that I prefer dispose.

I'm also okay with adding git_buf_dispose as a public API and adding a deprecation notice to git_buf_free and zapping it eventually. Thoughts?

[pks-t]

As I'm the one pushing towards that direction, I'm definitly in favor of your proposal.

I don't know about removing the git_buf_free function, though. it's really no burden to maintain at all, so if we decide to zap it for code hygiene we should be conservative and give at least two or three releases of time to do so. Probably together with a deprecation warning annotation the compiler, such that downstream notices we have deprecated it in favor of git_buf_dispose.

[pks-t]

I've made your proposed change with regards to git_buf_free. Created a deprecation macro, implemented git_buf_dispose, deprecated git_buf_free and then afterwards made a tree-wide conversion of git_buf_free -> git_buf_dispose.

[ethomson]

I think this is a copy/paste error.

[pks-t]

cough Embarrassing.

[ethomson]

Same.

[ethomson]

Ugh, sorry, I wish I had gotten around to this sooner before there were conflicts. If you could fix those up quickly I'll merge this. 👍

I'm really happy that we're deprecating this nicely and not breaking anything. 😀

[pks-t]

Ugh, sorry, I wish I had gotten around to this sooner before there were conflicts. If you could fix those up quickly I'll merge this.

No worries. Fixing conflicts is trivial to do for this PR.

I'm really happy that we're deprecating this nicely and not breaking anything.

Yup. I think the deprecation-macro is an important step going forward for v1.0, as well. While we cannot really change APIs anymore, we should still notify users that something better is available. I've rebased the PR on latest master.