Martin Ågren <martin.ag...@gmail.com> writes: > On 30 August 2017 at 04:52, Michael Haggerty <mhag...@alum.mit.edu> wrote: >> v3 looks good to me. Thanks! >> >> Reviewed-by: Michael Haggerty <mhag...@alum.mit.edu> > > Thank _you_ for very helpful feedback on the earlier versions. > > Martin
Yes, the earlier attempt was sort-of barking up a wrong tree. Once we step back and observe other users of affected_refnames and realize that the list is meant to to point at a refname field of an existing instance of another structure by borrowing, the blame shifts from files_transaction_prepare() to the real culprit. Michael's review gave us a very good "let's step back a bit" that made the huge improvement between v1 and v2/v3. I wonder if we should be tightening the use of affected_refnames even further, though. It is may be sufficient to make sure that we do not throw anything that we would need to free as part of destroying affected_refnames into the string list, purely from the "leak prevention" perspective. But stepping back a bit, the reason why the string list exists in the first place is to make sure we do not touch the same ref twice in a single transaction, the set of all possible updates in the single transaction exists somewhere, and each of these updates know what ref it wants to update. And that is recorded in transaction->updates[]->refname no? So it seems to me that logically any and all string that is registered in affected_refnames list must be the refname field of a ref_update structure in the transaction. And from that point of view, doesn't split_head_update() wants a similar fix? It attempts to insert "HEAD", makes sure it hasn't been inserted and then hangs a new update transaction as its util. It is not wrong per-se from purely leak-prevention point of view, as that "HEAD" is a literal string we woudn't even want to free, but from logical/"what each data means" point of view, it still feels wrong.
