[PATCHv3 00/13] the refs-transactions-reflog series
This is the whole refs-transactions-reflog series[1], which was in discussion for a bit already. It applies to origin/master. The idea is to have the reflog being part of the transactions, which the refs are already using, so the we're moving towards a database like API in the long run. This makes git easier to maintain as well as opening the possibility to replace the backend with a real database. If you've followed the topic a bit, start reading at patch [PATCH 06/13] refs.c: add a transaction function to append a reflog as the first 5 patches have been discussed a lot separately and can be found in origin/pu already[2]. The first two patches are deduplicating code. The third patch is ripping some code out of log_ref_write and introduces log_ref_write_fd, which does the actual writing. The patches 4+5 are renaming variables for clarity. The patch 6 and 7 are the entree in this series. Patch 6 adds two functions to the refs API: transaction_truncate_reflog and transaction_update_reflog. Both functions do not affect the files directy, but rather become effective once transaction_commit function is called. The transaction_truncate_reflog will wipe the reflog, which can be used for rebuilding the reflog, whereas the transaction_update_reflog function will just append one entry to the reflog. The patch 7 will make use of the functions introduced in patch 6. The command git reflog expire will then use the reflog transactions. Patch 8 is renaming log_ref_setup to create_reflog and should not Patches 9-12 are removing functions from the public refs API, which are unused then. Patch 13 is adding new functionality again, you can finally delete broken refs without having to know the details on git. [1] http://comments.gmane.org/gmane.comp.version-control.git/259712 or http://www.spinics.net/lists/git/msg242502.html [2] patch 1 + 2: origin/sb/ref-transaction-unify-to-update patch 3: origin/sb/log-ref-write-fd patch 4 + 5: origin/sb/ref-transaction-reflog patch 1-5 is unchanged in this series. Ronnie Sahlberg (9): refs.c: make ref_transaction_create a wrapper for ref_transaction_update refs.c: make ref_transaction_delete a wrapper for ref_transaction_update refs.c: add a function to append a reflog entry to a fd refs.c: rename the transaction functions reflog.c: use a reflog transaction when writing during expire refs.c: rename log_ref_setup to create_reflog refs.c: remove unlock_ref/close_ref/commit_ref from the refs api refs.c: remove lock_any_ref_for_update refs.c: allow deleting refs with a broken sha1 Stefan Beller (4): refs.c: rename transaction.updates to transaction.ref_updates refs.c: add a transaction function to truncate or append a reflog entry refs.c: don't expose the internal struct ref_lock in the header file refs.c: use a bit for ref_update have_old branch.c | 13 +- builtin/branch.c | 5 +- builtin/checkout.c | 8 +- builtin/commit.c | 10 +- builtin/fetch.c| 12 +- builtin/receive-pack.c | 13 +- builtin/reflog.c | 84 +-- builtin/replace.c | 10 +- builtin/tag.c | 10 +- builtin/update-ref.c | 26 ++-- cache.h| 7 + fast-import.c | 22 +-- refs.c | 371 +++-- refs.h | 95 ++--- sequencer.c| 12 +- t/t3200-branch.sh | 8 ++ walker.c | 10 +- 17 files changed, 408 insertions(+), 308 deletions(-) -- 2.2.0 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 00/13] the refs-transactions-reflog series
On 12/04/2014 09:29 AM, Stefan Beller wrote: This is the whole refs-transactions-reflog series[1], which was in discussion for a bit already. It applies to origin/master. 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. Of course, reflog expiration *should* be done atomically. But that is the business of the refs module; callers shouldn't have to do all the complicated work of building the transaction themselves. I spent the day working on an alternate proposal, to convert expire_reflogs() to take callback functions that decide *what* to expire, but which itself does the work of acquiring locks, iterating through the reflog entries, rewriting the file, and overwriting the old file with the new one. The goal is to move this function into refs.c and make builtin/reflog.c much simpler--it will only have to implement the callbacks that determine the expiration policy. I'm almost done with the series but won't be able to finish it until tomorrow. For those who are impatient, here is my work in progress [2]. Michael [1] http://thread.gmane.org/gmane.comp.version-control.git/259712/focus=259770 [2] https://github.com/mhagger/git, branch reflog-expire-api. -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 00/13] the refs-transactions-reflog series
Hi, Michael Haggerty wrote: On 12/04/2014 09:29 AM, Stefan Beller wrote: This is the whole refs-transactions-reflog series[1], which was in discussion for a bit already. It applies to origin/master. 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. Would it make sense to propose competing documentation patches (to Documentation/technical/api-ref-transactions.txt, or to refs.h), so we can work out the API that way? I don't think the API questions that we're talking about would end up affecting the details of how the files backend implements them too much. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 00/13] the refs-transactions-reflog series
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. Okay, now returning to the substance of this comment. This is revisiting themes from [3], so my opinions are probably not a surprise. I think that the API changes that you and Stefan are proposing are consistent and could both go in. You suggested refactoring expire_reflogs() to use a callback that decides what to expire. Then it doesn't have to care that the expiration happens by creating a new reflog and copying over the reflog entries that are not being expired. The result is a clean reflog expiration API. The ref-transaction-reflog series allows those low-level steps to be part of a ref transaction. Any ref backend (the current files-based backend or a future other one) would get a chance to reimplement those low-level steps, which are part of what happens during ref updates and reflog deletion. The goal is for all reflog updates to use the transaction API, so that new ref/reflog backends only need to implement the transaction functions. So *both* are making good changes, with different goals. The implementation of the reflog expiration API can use the ref transaction API. Of course, reflog expiration *should* be done atomically. But that is the business of the refs module; callers shouldn't have to do all the complicated work of building the transaction themselves. I don't understand this comment. After the ref-transaction-reflog series, a transaction_update_ref() call still takes care of the corresponding reflog update without the caller having to worry about it. Thanks for looking it over, Jonathan [1] http://thread.gmane.org/gmane.comp.version-control.git/259712/focus=259770 [3] http://thread.gmane.org/gmane.comp.version-control.git/259939/focus=259967 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 00/13] the refs-transactions-reflog series
Michael Haggerty mhag...@alum.mit.edu writes: ... an alternate proposal, to convert expire_reflogs() to take callback functions that decide *what* to expire, but which itself does the work of acquiring locks, iterating through the reflog entries, rewriting the file, and overwriting the old file with the new one. The goal is to move this function into refs.c and make builtin/reflog.c much simpler--it will only have to implement the callbacks that determine the expiration policy. It sounds like a very sensible approach to me. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 00/13] the refs-transactions-reflog series
Stefan Beller sbel...@google.com writes: This is the whole refs-transactions-reflog series[1], which was in discussion for a bit already. It applies to origin/master. The idea is to have the reflog being part of the transactions, which the refs are already using, so the we're moving towards a database like API in the long run. This makes git easier to maintain as well as opening the possibility to replace the backend with a real database. If you've followed the topic a bit, start reading at patch [PATCH 06/13] refs.c: add a transaction function to append a reflog as the first 5 patches have been discussed a lot separately and can be found in origin/pu already[2]. The first two patches are deduplicating code. The third patch is ripping some code out of log_ref_write and introduces log_ref_write_fd, which does the actual writing. The patches 4+5 are renaming variables for clarity. Thanks. It seems that we have a bit of hashing out the approaches that is necessary between Michael's and this one. I'll refrain from picking this up for today (even though I would read it through as time permits). It might turn out to be necessary to drop those five early patches from my tree when this series gets rerolled to take input from the alternative Michael's working on, but that droppage does not have to happen today. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 00/13] the refs-transactions-reflog series
On Thu, Dec 4, 2014 at 10:41 AM, Junio C Hamano gits...@pobox.com wrote: Stefan Beller sbel...@google.com writes: This is the whole refs-transactions-reflog series[1], which was in discussion for a bit already. It applies to origin/master. The idea is to have the reflog being part of the transactions, which the refs are already using, so the we're moving towards a database like API in the long run. This makes git easier to maintain as well as opening the possibility to replace the backend with a real database. If you've followed the topic a bit, start reading at patch [PATCH 06/13] refs.c: add a transaction function to append a reflog as the first 5 patches have been discussed a lot separately and can be found in origin/pu already[2]. The first two patches are deduplicating code. The third patch is ripping some code out of log_ref_write and introduces log_ref_write_fd, which does the actual writing. The patches 4+5 are renaming variables for clarity. Thanks. It seems that we have a bit of hashing out the approaches that is necessary between Michael's and this one. I'll refrain from picking this up for today (even though I would read it through as time permits). It might turn out to be necessary to drop those five early patches from my tree when this series gets rerolled to take input from the alternative Michael's working on, but that droppage does not have to happen today. Ok, let's see how Michaels series evolves and if I can base the transactions series on that afterwards. Michael has picked up 3 out the 5 patches to get his series started, so maybe we can resolve it nicely. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 00/13] the refs-transactions-reflog series
Stefan Beller wrote: Ok, let's see how Michaels series evolves and if I can base the transactions series on that That sounds good to me, too, FWIW. That way we don't have to fuss with conflicts in refs.c. :) Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 00/13] the refs-transactions-reflog series
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
mhag...@alum.mit.edu
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
