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@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html