Re: [PATCHv3 00/13] the refs-transactions-reflog series

> On Thu, Dec 04, 2014 at 10:14:04AM -0800, Jonathan Nieder wrote:
>> Michael Haggerty wrote:
>>
>>> I am still unhappy with the approach of this series, for the reasons
>>> that I explained earlier [1]. In short, I think that the abstraction
>>> level is wrong. In my opinion, consumers of the refs API should barely
>>> even have to *know* about reflogs, let alone implement reflog expiration
>>> themselves.
>>
> Ok, what is a consumer for you? In my understanding the builtin/reflog.c is
> a consumer of the refs API, so there we want to see clean code just telling the
> refs backend to do its thing.
> 
> I think Jonathan made a good point by saying our patch series have 
> different goals.
> 
> So I really like the code, which leads to 
> 
>     reflog_expiry_prepare(refname, sha1, cb.policy_cb);
>     for_each_reflog_ent(refname, expire_reflog_ent, &cb);
>     reflog_expiry_cleanup(cb.policy_cb);
> 
> but look at the surrounding code:
>     
> 	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
> 		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
> 			...
> 	}
> 
> 
> 	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
> 		if (close_lock_file(&reflog_lock)) {
> 			...
> 		}
> 	}
> 
> That part should also go into the refs.c backend, so in the expire_reflog
> function you can just write:
> 
> 	transaction_begin(...)  // This does all the hold_lock_file_for_update magic 
> 				// lines 457-464 in mhagger/reflog-expire-api
> 
>  	reflog_expiry_prepare(refname, sha1, cb.policy_cb);
> 	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
> 	reflog_expiry_cleanup(cb.policy_cb);
> 
> 	transaction_commit(...) // This does all the close_lock_file/rename/write_in_full
> 				// lines 470-488 in mhagger/reflog-expire-api