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