Re: [PATCH 03/15] refs.c: use packed refs when deleting refs during a transaction

2014-10-22 Thread Junio C Hamano
Ronnie Sahlberg sahlb...@google.com writes:

 commit fb5fa1d338ce113b0fea3bb955b50bbb3e827805 upstream.

Huh?

 Make the deletion of refs during a transaction more atomic.
 Start by first copying all loose refs we will be deleting to the packed
 refs file and then commit the packed refs file. Then re-lock the packed refs
 file to stop anyone else from modifying these refs and keep it locked until
 we are finished.
 Since all refs we are about to delete are now safely held in the packed refs
 file we can proceed to immediately unlink any corresponding loose refs
 and still be fully rollback-able.

 The exception is for refs that can not be resolved. Those refs are never
 added to the packed refs and will just be un-rollback-ably deleted during
 commit.

 By deleting all the loose refs at the start of the transaction we make make
 it possible to both delete one ref and then re-create a different ref in
 the same transaction even if the two refs would normally conflict.

 Example: rename m-m/m

 In that example we want to delete the file 'm' so that we make room so
 that we can create a directory with the same name in order to lock and
 write to the ref m/m and its lock-file m/m.lock.

 If there is a failure during the commit phase we can rollback without losing
 any refs since we have so far only deleted loose refs that that are
 guaranteed to also have a corresponding entry in the packed refs file.
 Once we have finished all updates for refs and their reflogs we can repack
 the packed refs file and remove the to-be-deleted refs from the packed refs,
 at which point all the deleted refs will disappear in one atomic rename
 operation.

 This also means that for an outside observer, deletion of multiple refs
 in a single transaction will look atomic instead of one ref being deleted
 at a time.

 In order to do all this we need to change the semantics for the
 repack_without_refs function so that we can lock the packed refs file,
 do other stuff, and later be able to call repack_without_refs with the
 lock already taken.
 This means we need some additional changes in remote.c to reflect the
 changes to the repack_without_refs semantics.

 Change-Id: I1e4f58050acaabc6bcfa3409fbc0c2b866bb59aa
 Signed-off-by: Ronnie Sahlberg sahlb...@google.com
 Signed-off-by: Jonathan Nieder jrnie...@gmail.com


 @@ -3821,23 +3824,109 @@ int transaction_commit(struct transaction 
 *transaction,
 ...
 + if (need_repack) {
 + packed = get_packed_refs(ref_cache);;
 + sort_ref_dir(packed);
 + if (commit_packed_refs()){

SP between ){.

 + strbuf_addf(err, unable to overwrite old ref-pack 
 + file);

This is bending backwards sacrificing readability; 80-col limit is
not that hard a limit.  Split at the comma instead, perhaps?

strbuf_addf(err,
unable to overwrite old ref-pack file);

 ...
 + if (!update-lock) {
 + int df_conflict = (errno == ENOTDIR);
 +
 + strbuf_addf(err, Cannot lock the ref '%s'.,
 + update-refname);
 + ret = df_conflict ?
 +   TRANSACTION_NAME_CONFLICT : 

A trailing SP here...

 @@ -3860,6 +3953,16 @@ int transaction_commit(struct transaction *transaction,
   update-reflog_lock = update-orig_update-reflog_lock;
   continue;
   }
 + if (log_all_ref_updates  !reflog_exists(update-refname) 
 + create_reflog(update-refname)) {
 + ret = -1;
 + if (err)
 + strbuf_addf(err, Failed to setup reflog for 
 + %s, update-refname);

Split after comma, perhaps?
--
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 03/15] refs.c: use packed refs when deleting refs during a transaction

2014-10-21 Thread Ronnie Sahlberg
commit fb5fa1d338ce113b0fea3bb955b50bbb3e827805 upstream.

Make the deletion of refs during a transaction more atomic.
Start by first copying all loose refs we will be deleting to the packed
refs file and then commit the packed refs file. Then re-lock the packed refs
file to stop anyone else from modifying these refs and keep it locked until
we are finished.
Since all refs we are about to delete are now safely held in the packed refs
file we can proceed to immediately unlink any corresponding loose refs
and still be fully rollback-able.

The exception is for refs that can not be resolved. Those refs are never
added to the packed refs and will just be un-rollback-ably deleted during
commit.

By deleting all the loose refs at the start of the transaction we make make
it possible to both delete one ref and then re-create a different ref in
the same transaction even if the two refs would normally conflict.

Example: rename m-m/m

In that example we want to delete the file 'm' so that we make room so
that we can create a directory with the same name in order to lock and
write to the ref m/m and its lock-file m/m.lock.

If there is a failure during the commit phase we can rollback without losing
any refs since we have so far only deleted loose refs that that are
guaranteed to also have a corresponding entry in the packed refs file.
Once we have finished all updates for refs and their reflogs we can repack
the packed refs file and remove the to-be-deleted refs from the packed refs,
at which point all the deleted refs will disappear in one atomic rename
operation.

This also means that for an outside observer, deletion of multiple refs
in a single transaction will look atomic instead of one ref being deleted
at a time.

In order to do all this we need to change the semantics for the
repack_without_refs function so that we can lock the packed refs file,
do other stuff, and later be able to call repack_without_refs with the
lock already taken.
This means we need some additional changes in remote.c to reflect the
changes to the repack_without_refs semantics.

Change-Id: I1e4f58050acaabc6bcfa3409fbc0c2b866bb59aa
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
 builtin/remote.c |  20 ++-
 refs.c   | 155 ++-
 2 files changed, 138 insertions(+), 37 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..c25420f 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1,4 +1,5 @@
 #include builtin.h
+#include lockfile.h
 #include parse-options.h
 #include transport.h
 #include remote.h
@@ -753,6 +754,15 @@ static int remove_branches(struct string_list *branches)
const char **branch_names;
int i, result = 0;
 
+   if (lock_packed_refs(0)) {
+   struct strbuf err = STRBUF_INIT;
+
+   unable_to_lock_message(git_path(packed-refs), errno, err);
+   error(%s, err.buf);
+   strbuf_release(err);
+   return -1;
+   }
+
branch_names = xmalloc(branches-nr * sizeof(*branch_names));
for (i = 0; i  branches-nr; i++)
branch_names[i] = branches-items[i].string;
@@ -1337,9 +1347,15 @@ static int prune_remote(const char *remote, int dry_run)
delete_refs[i] = states.stale.items[i].util;
if (!dry_run) {
struct strbuf err = STRBUF_INIT;
-   if (repack_without_refs(delete_refs, states.stale.nr,
-   err))
+
+   if (lock_packed_refs(0)) {
+   unable_to_lock_message(git_path(packed-refs),
+  errno, err);
result |= error(%s, err.buf);
+   } else
+   if (repack_without_refs(delete_refs,
+   states.stale.nr, err))
+   result |= error(%s, err.buf);
strbuf_release(err);
}
free(delete_refs);
diff --git a/refs.c b/refs.c
index c088d36..662af9b 100644
--- a/refs.c
+++ b/refs.c
@@ -2660,6 +2660,9 @@ static int curate_packed_ref_fn(struct ref_entry *entry, 
void *cb_data)
return 0;
 }
 
+/*
+ * Must be called with packed refs already locked (and sorted)
+ */
 int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 {
struct ref_dir *packed;
@@ -2674,14 +2677,12 @@ int repack_without_refs(const char **refnames, int n, 
struct strbuf *err)
if (get_packed_ref(refnames[i]))
break;
 
-   /* Avoid locking if we have nothing to do */
-   if (i == n)
+   /* Avoid processing if we have nothing to do */
+   if (i == n) {
+   rollback_packed_refs();