On 5 September 2017 at 12:02, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Martin Ågren <martin.agren@xxxxxxxxx> writes: > >> On 30 August 2017 at 04:52, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: >>> v3 looks good to me. Thanks! >>> >>> Reviewed-by: Michael Haggerty <mhagger@xxxxxxxxxxxx> >> >> 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. I'm with you up to here. > 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. There is a "Special hack" comment related to this, and I don't feel particularly confident that I could make any meaningful contribution in this area. To be honest, I don't immediately see in which direction your suggestion/idea/thought is going, which tells me I should not be making a mess out of it. :-) Martin
