Re: [PATCH v20 00/48] Use ref transactions
On Tue, Jul 8, 2014 at 11:48 AM, Junio C Hamano wrote: > Michael Haggerty writes: > >> Patches 01-19 -- ACK mhagger >> Patches 20-42 -- I sent various comments, small to large, concerning >> these patches >> Patch 43 -- Needs more justification if it is to be acceptable >> Patch 44 -- Depends on 43 >> Patches 45-48 -- I didn't quite get to these, but... >> >> Perhaps it would be more appropriate for the rules about reference name >> conflicts to be enforced by the backend, since it is the limitations of >> the current backend that impose the restrictions. Would that make sense? >> >> On the other hand, removing the restrictions isn't simply a matter of >> picking a different backend, because all Git repositories have to be >> able to interact with each other. > > I'd say that "if you have foo/bar you cannot have foo" may have > started as an implementation limitation, but the interoperability > requirement with existing versions of Git and with existing > repositories makes it necessary to enforce it the same way as other > rules such as "you cannot have double-dots in name, e.g. foo..bar" > or "no branches whose name begins with a dash", neither of which > comes from any filesystem issues. That a rule can be loosened with > one new backend does not at all mean it is a good idea to loosen it > "because we can" in the first place. ACK. > >> I think it would be good to try to merge the first part of this patch >> series to lock in some progress while we continue iterating on the >> remainder. I'm satisfied that it is all going in the right direction >> and I am thankful to Ronnie for pushing it forward. But handling >> 48-patch series is very daunting and I would welcome a split. >> >> I'm not sure whether patches 01-19 are necessarily the right split >> between merge-now/iterate-more; it is more or less an accident that I >> stopped after patch 19 on an earlier review. Maybe Ronnie could propose >> a logical subset of the commits as being ready to be merged to next in >> the nearish term? > > Yeah, thanks for going through this, and I agree that we would be > better off merging the earlier part first. > Ok, 01-19 is as good split as any. If you merge just the 01-19 part of this series I will address Michael's concerns and repost patches 20-48 as a separate series. regards ronnie sahlberg -- 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: [PATCH v20 00/48] Use ref transactions
On Tue, Jul 8, 2014 at 9:29 AM, Michael Haggerty wrote: > On 06/20/2014 04:42 PM, Ronnie Sahlberg wrote: >> This patch series can also be found at >> https://github.com/rsahlberg/git/tree/ref-transactions >> >> This patch series is based on current master and expands on the transaction >> API. It converts all ref updates, inside refs.c as well as external, to use >> the >> transaction API for updates. This makes most of the ref updates to become >> atomic when there are failures locking or writing to a ref. >> >> This version completes the work to convert all ref updates to use >> transactions. >> Now that all updates are through transactions I will start working on >> cleaning up the reading of refs and to create an api for managing reflogs but >> all that will go in a different patch series. >> >> Version 20: >> - Whitespace and style changes suggested by Jun. > > I spent most of the day on reviewing this patch series, Thanks! >but now I'm out > of time again. Here is a summary from my point of view: > > Patches 01-19 -- ACK mhagger > Patches 20-42 -- I sent various comments, small to large, concerning > these patches > Patch 43 -- Needs more justification if it is to be acceptable > Patch 44 -- Depends on 43 > Patches 45-48 -- I didn't quite get to these, but... > > Perhaps it would be more appropriate for the rules about reference name > conflicts to be enforced by the backend, since it is the limitations of > the current backend that impose the restrictions. Would that make sense? > > On the other hand, removing the restrictions isn't simply a matter of > picking a different backend, because all Git repositories have to be > able to interact with each other. > > So, I don't yet have a considered opinion on the matter. I think for compatibility I would prefer to keep the same rules for name conflicts as for the current files implementation. But we could have a configuration option to disable these checks, with the caveat that this might mean that some users will no longer be able to access pull all the branches anymore. > > > I think it would be good to try to merge the first part of this patch > series to lock in some progress while we continue iterating on the > remainder. I'm satisfied that it is all going in the right direction > and I am thankful to Ronnie for pushing it forward. But handling > 48-patch series is very daunting and I would welcome a split. Will do. > > I'm not sure whether patches 01-19 are necessarily the right split > between merge-now/iterate-more; it is more or less an accident that I > stopped after patch 19 on an earlier review. Maybe Ronnie could propose > a logical subset of the commits as being ready to be merged to next in > the nearish term? > > Michael > > -- > 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: [PATCH v20 00/48] Use ref transactions
On Tue, Jul 08, 2014 at 11:48:06AM -0700, Junio C Hamano wrote: > I'd say that "if you have foo/bar you cannot have foo" may have > started as an implementation limitation, but the interoperability > requirement with existing versions of Git and with existing > repositories makes it necessary to enforce it the same way as other > rules such as "you cannot have double-dots in name, e.g. foo..bar" > or "no branches whose name begins with a dash", neither of which > comes from any filesystem issues. That a rule can be loosened with > one new backend does not at all mean it is a good idea to loosen it > "because we can" in the first place. To me it makes sense to to have it as an option, for two reasons: 1. If you want to pay the compatibility cost (e.g., because you work with a defined set of users on a small project and can dictate how they set their git options), you get the extra feature. 2. It provides a migration path if we eventually want to move to a default backend that allows it. I admit that I don't care _too_ much, though. My main desire for it is not to store two live branches that overlap, but to store reflogs for deleted branches without conflicting with live branches. And of course all of this is getting rather ahead of ourselves. We do not have _any_ alternate backends yet, nor even yet the infrastructure to make them. -Peff -- 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: [PATCH v20 00/48] Use ref transactions
Michael Haggerty writes: > Patches 01-19 -- ACK mhagger > Patches 20-42 -- I sent various comments, small to large, concerning > these patches > Patch 43 -- Needs more justification if it is to be acceptable > Patch 44 -- Depends on 43 > Patches 45-48 -- I didn't quite get to these, but... > > Perhaps it would be more appropriate for the rules about reference name > conflicts to be enforced by the backend, since it is the limitations of > the current backend that impose the restrictions. Would that make sense? > > On the other hand, removing the restrictions isn't simply a matter of > picking a different backend, because all Git repositories have to be > able to interact with each other. I'd say that "if you have foo/bar you cannot have foo" may have started as an implementation limitation, but the interoperability requirement with existing versions of Git and with existing repositories makes it necessary to enforce it the same way as other rules such as "you cannot have double-dots in name, e.g. foo..bar" or "no branches whose name begins with a dash", neither of which comes from any filesystem issues. That a rule can be loosened with one new backend does not at all mean it is a good idea to loosen it "because we can" in the first place. > I think it would be good to try to merge the first part of this patch > series to lock in some progress while we continue iterating on the > remainder. I'm satisfied that it is all going in the right direction > and I am thankful to Ronnie for pushing it forward. But handling > 48-patch series is very daunting and I would welcome a split. > > I'm not sure whether patches 01-19 are necessarily the right split > between merge-now/iterate-more; it is more or less an accident that I > stopped after patch 19 on an earlier review. Maybe Ronnie could propose > a logical subset of the commits as being ready to be merged to next in > the nearish term? Yeah, thanks for going through this, and I agree that we would be better off merging the earlier part first. -- 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: [PATCH v20 00/48] Use ref transactions
On 06/20/2014 04:42 PM, Ronnie Sahlberg wrote: > This patch series can also be found at > https://github.com/rsahlberg/git/tree/ref-transactions > > This patch series is based on current master and expands on the transaction > API. It converts all ref updates, inside refs.c as well as external, to use > the > transaction API for updates. This makes most of the ref updates to become > atomic when there are failures locking or writing to a ref. > > This version completes the work to convert all ref updates to use > transactions. > Now that all updates are through transactions I will start working on > cleaning up the reading of refs and to create an api for managing reflogs but > all that will go in a different patch series. > > Version 20: > - Whitespace and style changes suggested by Jun. I spent most of the day on reviewing this patch series, but now I'm out of time again. Here is a summary from my point of view: Patches 01-19 -- ACK mhagger Patches 20-42 -- I sent various comments, small to large, concerning these patches Patch 43 -- Needs more justification if it is to be acceptable Patch 44 -- Depends on 43 Patches 45-48 -- I didn't quite get to these, but... Perhaps it would be more appropriate for the rules about reference name conflicts to be enforced by the backend, since it is the limitations of the current backend that impose the restrictions. Would that make sense? On the other hand, removing the restrictions isn't simply a matter of picking a different backend, because all Git repositories have to be able to interact with each other. So, I don't yet have a considered opinion on the matter. I think it would be good to try to merge the first part of this patch series to lock in some progress while we continue iterating on the remainder. I'm satisfied that it is all going in the right direction and I am thankful to Ronnie for pushing it forward. But handling 48-patch series is very daunting and I would welcome a split. I'm not sure whether patches 01-19 are necessarily the right split between merge-now/iterate-more; it is more or less an accident that I stopped after patch 19 on an earlier review. Maybe Ronnie could propose a logical subset of the commits as being ready to be merged to next in the nearish term? Michael -- 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
[PATCH v20 00/48] Use ref transactions
This patch series can also be found at https://github.com/rsahlberg/git/tree/ref-transactions This patch series is based on current master and expands on the transaction API. It converts all ref updates, inside refs.c as well as external, to use the transaction API for updates. This makes most of the ref updates to become atomic when there are failures locking or writing to a ref. This version completes the work to convert all ref updates to use transactions. Now that all updates are through transactions I will start working on cleaning up the reading of refs and to create an api for managing reflogs but all that will go in a different patch series. Version 20: - Whitespace and style changes suggested by Jun. Ronnie Sahlberg (48): refs.c: remove ref_transaction_rollback refs.c: ref_transaction_commit should not free the transaction refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: allow passing NULL to ref_transaction_free refs.c: add a strbuf argument to ref_transaction_commit for error logging lockfile.c: add a new public function unable_to_lock_message lockfile.c: make lock_file return a meaningful errno on failurei refs.c: add an err argument to repack_without_refs refs.c: make sure log_ref_setup returns a meaningful errno refs.c: verify_lock should set errno to something meaningful refs.c: make remove_empty_directories always set errno to something sane refs.c: commit_packed_refs to return a meaningful errno on failure refs.c: make resolve_ref_unsafe set errno to something meaningful on error refs.c: log_ref_write should try to return meaningful errno refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c: make update_ref_write update a strbuf on failure update-ref: use err argument to get error from ref_transaction_commit refs.c: remove the onerr argument to ref_transaction_commit refs.c: change ref_transaction_update() to do error checking and return status refs.c: change ref_transaction_create to do error checking and return status refs.c: update ref_transaction_delete to check for error and return status refs.c: make ref_transaction_begin take an err argument refs.c: add transaction.status and track OPEN/CLOSED/ERROR tag.c: use ref transactions when doing updates replace.c: use the ref transaction functions for updates commit.c: use ref transactions for updates sequencer.c: use ref transactions for all ref updates fast-import.c: change update_branch to use ref transactions branch.c: use ref transaction for all ref updates refs.c: change update_ref to use a transaction receive-pack.c: use a reference transaction for updating the refs fast-import.c: use a ref transaction when dumping tags walker.c: use ref transaction for ref updates refs.c: make lock_ref_sha1 static refs.c: remove the update_ref_lock function refs.c: remove the update_ref_write function refs.c: remove lock_ref_sha1 refs.c: make prune_ref use a transaction to delete the ref refs.c: make delete_ref use a transaction refs.c: add an err argument to delete_ref_loose refs.c: pass the ref log message to _create/delete/update instead of _commit refs.c: pass NULL as *flags to read_ref_full refs.c: move the check for valid refname to lock_ref_sha1_basic refs.c: call lock_ref_sha1_basic directly from commit refs.c: pass a skip list to name_conflict_fn refs.c: propagate any errno==ENOTDIR from _commit back to the callers fetch.c: change s_update_ref to use a ref transaction refs.c: make write_ref_sha1 static branch.c | 30 +-- builtin/commit.c | 24 ++- builtin/fetch.c| 36 ++-- builtin/receive-pack.c | 97 ++ builtin/remote.c | 5 +- builtin/replace.c | 15 +- builtin/tag.c | 15 +- builtin/update-ref.c | 34 ++-- cache.h| 4 +- fast-import.c | 53 -- lockfile.c | 39 ++-- refs.c | 506 - refs.h | 131 + sequencer.c| 24 ++- walker.c | 58 +++--- wrapper.c | 14 +- 16 files changed, 710 insertions(+), 375 deletions(-) -- 2.0.0.420.g181e020.dirty -- 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
