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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 12/04/2014 07:32 PM, Stefan Beller wrote:
> 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

I'm pleasantly surprised that you guys have already looked at my work in
progress. I wish I had had more time earlier today to explain what is
still to be done:

The whole point of all of the refactoring is to move expire_reflog()
(and supporting functions like expire_reflog_ent()) to refs.c. The
"surrounding code" that you mention above would be moved there and would
*not* need to be done by callers.

expire_reflog() will gain three callback function pointers as
parameters. The caller (in this case reflog.c) will pass pointers to
reflog_expiry_prepare(), reflog_expiry_cleanup(), and
should_expire_reflog_ent() into expire_reflog().

There is no need to wrap the code in a transaction, because the
"surrounding code" that you mentioned implements all the "transaction"
that is needed! There is no need to complicated the *ref_transaction*
interface to allow arbitrary reflog updates when all we need is this one
very special case, plus of course the reflog appends that (I claim)
should happen implicitly whenever a reference is updated [1].

>> 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.

Feel free to do what you want, but I really don't think we will ever
need transactions to handle generic reflog updates.

Meanwhile I'll try to get my series finished, including API docs as
Jonathan requested. I hope the code will be more convincing than my
prose :-)

Michael

[1] Of course, only for references that have reflogs enabled.

-- 
Michael Haggerty
mhagger@alum.mit.edu