On Thu, Dec 29 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> Change an UNLEAK() added in 0c4542738e6 (clone: free or UNLEAK further >> pointers when finished, 2021-03-14) to use a "to_free" pattern >> instead. In this case the "repo" can be either this absolute_pathdup() >> value, or in the "else if" branch seen in the context the the >> "argv[0]" argument to "main()". >> >> We can only free() the value in the former case, hence the "to_free" >> pattern. > > Many other patches in the series, especially the ones that touch the > library-ish parts that can be called unbounded number of times, do > make sense, but this patch helps nobody. Not even the leak checker, > who should already be happy with the existing UNLEAK() marking. The > only thing it does is to free() a piece of memory obtained from a > one-time allocation that will be discarded upon program exit anyway. The changes in this series that deal with UNLEAK() take it as a given that it's a good thing to replace these trivial cases with a free(). But the rationale is in ac95f5d36ad (built-ins: use free() not UNLEAK() if trivial, rm dead code, 2022-11-08) in the earlier topic, this is the follow-up. We only have 15 UNLEAK() invocations in-tree left, with this series (which made no special effort to get the rest, but just the remaining very low hanging fruit) it's 13. If our built-ins were using this pattern consistently I think it would be worth keeping & using UNLEAK() like this, but they aren't. Even the "display_repo" variable seen in the context of this patch is free()'d, see 46da295a770 (clone/fetch: anonymize URLs in the reflog, 2020-06-04), along with the rest in this function and others (with the exception of the remaining UNLEAK(...)), e.g. "remote_name" etc. So I think keeping it just makes the code more confusing, one wonders why this particular variable is being UNLEAK()'d, instead of just free()'d like the rest. As the comment on the SUPPRESS_ANNOTATED_LEAKS macro notes we only get the benefit from it if you compile with SUPPRESS_ANNOTATED_LEAKS, so e.g. if you use SANITIZE=leak it'll kick in, but not on a normal build and e.g.: valgrind --leak-check=full ./git commit-graph verify Which will with UNLEAK() report the leak fixed in 04/20 here (I'm just using it in this example since easier to reproduce), but with free() we won't report it. I agree it has a marginal benefit, e.g. it won't contribute to the linux-leaks job, as it uses SUPPRESS_ANNOTATED_LEAKS, but the above is why I think it's worth changing the remaining low-hanging UNLEAK() fruit to free(). We do have cases where it's much tricker to replace those UNLEAK() with free(). I think it's much better if we narrow its use to those cases at this point.
