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

[Stefan Beller <sbeller@google.com>]

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


> 
> So *both* are making good changes, with different goals.

If you want to I can rebase the reflog series on top of yours to show
they can work together quite nicely.

Thanks for your draft series and feedback,
Stefan