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

[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 <***@google.com>
Signed-off-by: Jonathan Nieder <***@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();
return 0; /* no refname exists in packed refs */
-
- if (lock_packed_refs(0)) {
- unable_to_lock_message(git_path("packed-refs"), errno, err);
- return -1;
}
+
packed = get_packed_refs(&ref_cache);

/* Remove refnames from the cache */
@@ -3796,10 +3797,12 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
int transaction_commit(struct transaction *transaction,
struct strbuf *err)
{
- int ret = 0, delnum = 0, i;
+ int ret = 0, delnum = 0, i, need_repack = 0;
const char **delnames;
int n = transaction->nr;
+ struct packed_ref_cache *packed_ref_cache;
struct ref_update **updates = transaction->updates;
+ struct ref_dir *packed;

assert(err);

@@ -3821,23 +3824,109 @@ int transaction_commit(struct transaction *transaction,
goto cleanup;
}

- /* Acquire all ref locks while verifying old values */
+ /* Lock packed refs during commit */
+ if (lock_packed_refs(0)) {
+ if (err)
+ unable_to_lock_message(git_path("packed-refs"),
+ errno, err);
+ ret = -1;
+ goto cleanup;
+ }
+
+ /* any loose refs are to be deleted are first copied to packed refs */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
- int flags = update->flags;
+ unsigned char sha1[20];

if (update->update_type != UPDATE_SHA1)
continue;
+ if (!is_null_sha1(update->new_sha1))
+ continue;
+ if (get_packed_ref(update->refname))
+ continue;
+ if (!resolve_ref_unsafe(update->refname, RESOLVE_REF_READING,
+ sha1, NULL))
+ continue;

- if (is_null_sha1(update->new_sha1))
- flags |= REF_DELETING;
+ add_packed_ref(update->refname, sha1);
+ need_repack = 1;
+ }
+ if (need_repack) {
+ packed = get_packed_refs(&ref_cache);;
+ sort_ref_dir(packed);
+ if (commit_packed_refs()){
+ strbuf_addf(err, "unable to overwrite old ref-pack "
+ "file");
+ ret = -1;
+ goto cleanup;
+ }
+ /* lock the packed refs again so no one can change it */
+ if (lock_packed_refs(0)) {
+ if (err)
+ unable_to_lock_message(git_path("packed-refs"),
+ errno, err);
+ ret = -1;
+ goto cleanup;
+ }
+ }
+
+ /*
+ * At this stage any refs that are to be deleted have been moved to the
+ * packed refs file anf the packed refs file is deleted. We can now
+ * safely delete these loose refs.
+ */
+
+ /* Unlink any loose refs scheduled for deletion */
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update->update_type != UPDATE_SHA1)
+ continue;
+ if (!is_null_sha1(update->new_sha1))
+ continue;
+ update->flags |= REF_DELETING;
+ update->lock = lock_ref_sha1_basic(update->refname,
+ (update->have_old ?
+ update->old_sha1 :
+ NULL),
+ NULL,
+ update->flags,
+ &update->type);
+ if (!update->lock) {
+ int df_conflict = (errno == ENOTDIR);
+
+ strbuf_addf(err, "Cannot lock the ref '%s'.",
+ update->refname);
+ ret = df_conflict ?
+ TRANSACTION_NAME_CONFLICT :
+ TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
+ if (delete_ref_loose(update->lock, update->type, err)) {
+ ret = -1;
+ goto cleanup;
+ }
+ try_remove_empty_parents((char *)update->refname);
+ if (!(update->flags & REF_ISPRUNING))
+ delnames[delnum++] = xstrdup(update->lock->ref_name);
+ unlock_ref(update->lock);
+ update->lock = NULL;
+ }
+
+ /* Acquire all ref locks for updates while verifying old values */
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];

+ if (update->update_type != UPDATE_SHA1)
+ continue;
+ if (is_null_sha1(update->new_sha1))
+ continue;
update->lock = lock_ref_sha1_basic(update->refname,
(update->have_old ?
update->old_sha1 :
NULL),
NULL,
- flags,
+ update->flags,
&update->type);
if (!update->lock) {
ret = (errno == ENOTDIR)
@@ -3849,6 +3938,10 @@ int transaction_commit(struct transaction *transaction,
}
}

+ /* delete reflog for all deleted refs */
+ for (i = 0; i < delnum; i++)
+ unlink_or_warn(git_path("logs/%s", delnames[i]));
+
/* Lock all reflog files */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
@@ -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);
+ goto cleanup;
+ }
+ if (!reflog_exists(update->refname))
+ continue;
update->reflog_fd = hold_lock_file_for_append(
update->reflog_lock,
git_path("logs/%s", update->refname),
@@ -3875,7 +3978,7 @@ int transaction_commit(struct transaction *transaction,
}
}

- /* Perform ref updates first so live commits remain referenced */
+ /* Perform ref updates */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];

@@ -3894,23 +3997,6 @@ int transaction_commit(struct transaction *transaction,
}
}

- /* Perform deletes now that updates are safely completed */
- for (i = 0; i < n; i++) {
- struct ref_update *update = updates[i];
-
- if (update->update_type != UPDATE_SHA1)
- continue;
- if (update->lock) {
- if (delete_ref_loose(update->lock, update->type, err)) {
- ret = TRANSACTION_GENERIC_ERROR;
- goto cleanup;
- }
-
- if (!(update->flags & REF_ISPRUNING))
- delnames[delnum++] = update->lock->ref_name;
- }
- }
-
/*
* Update all reflog files
* We have already committed all ref updates and deletes.
@@ -3961,15 +4047,14 @@ int transaction_commit(struct transaction *transaction,
}
}

- if (repack_without_refs(delnames, delnum, err)) {
+ if (repack_without_refs(delnames, delnum, err))
ret = TRANSACTION_GENERIC_ERROR;
- goto cleanup;
- }
- for (i = 0; i < delnum; i++)
- unlink_or_warn(git_path("logs/%s", delnames[i]));
clear_loose_ref_cache(&ref_cache);

cleanup:
+ packed_ref_cache = get_packed_ref_cache(&ref_cache);
+ if (packed_ref_cache->lock)
+ rollback_packed_refs();
transaction->state = TRANSACTION_CLOSED;

for (i = 0; i < n; i++)

[Ronnie Sahlberg]

commit e193c10fc4f9274d1e751cfcdcc4507818e8d498 upstream.

Change lock_ref_sha1_basic to return an error instead of dying when
we fail to lock a file during a transaction.
This function is only called from transaction_commit() and it knows how
to handle these failures.

Change-Id: Ie830b7970412d9299e76a86f08633ce721130aed
Signed-off-by: Ronnie Sahlberg <***@google.com>
Signed-off-by: Jonathan Nieder <***@gmail.com>
---
refs.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index e49fbe9..c088d36 100644
--- a/refs.c
+++ b/refs.c
@@ -2340,6 +2340,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,

lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
if (lock->lock_fd < 0) {
+ last_errno = errno;
if (errno == ENOENT && --attempts_remaining > 0)
/*
* Maybe somebody just deleted one of the
@@ -2347,8 +2348,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
* again:
*/
goto retry;
- else
- unable_to_lock_die(ref_file, errno);
+ else {
+ struct strbuf err = STRBUF_INIT;
+ unable_to_lock_message(ref_file, errno, &err);
+ error("%s", err.buf);
+ strbuf_reset(&err);
+ goto error_return;
+ }
}
return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;

[Ronnie Sahlberg]

Change-Id: Ibfc64ab19257484129ab0ad861b72c02414388df
Signed-off-by: Ronnie Sahlberg <***@google.com>
---
refs.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index e0d5a82..7fb0d6c 100644
--- a/refs.c
+++ b/refs.c
@@ -2393,13 +2393,17 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
return 0;
}

-/* This should return a meaningful errno on failure */
-static int lock_packed_refs(int flags)
+static int lock_packed_refs(struct strbuf *err)
{
struct packed_ref_cache *packed_ref_cache;

- if (hold_lock_file_for_update(&packlock, git_path("packed-refs"), flags) < 0)
+ if (hold_lock_file_for_update(&packlock, git_path("packed-refs"),
+ 0) < 0) {
+ if (err)
+ unable_to_lock_message(git_path("packed-refs"),
+ errno, err);
return -1;
+ }
/*
* Get the current packed-refs while holding the lock. If the
* packed-refs file has been modified since we last read it,
@@ -2587,11 +2591,14 @@ static void prune_refs(struct ref_to_prune *r)
int pack_refs(unsigned int flags)
{
struct pack_refs_cb_data cbdata;
+ struct strbuf err = STRBUF_INIT;

memset(&cbdata, 0, sizeof(cbdata));
cbdata.flags = flags;

- lock_packed_refs(LOCK_DIE_ON_ERROR);
+ if (lock_packed_refs(&err))
+ die("%s", err.buf);
+
cbdata.packed_refs = get_packed_refs(&ref_cache);

do_for_each_entry_in_dir(get_loose_refs(&ref_cache), 0,
@@ -3757,10 +3764,7 @@ int transaction_commit(struct transaction *transaction,
}

/* Lock packed refs during commit */
- if (lock_packed_refs(0)) {
- if (err)
- unable_to_lock_message(git_path("packed-refs"),
- errno, err);
+ if (lock_packed_refs(err)) {
ret = -1;
goto cleanup;
}
@@ -3815,10 +3819,7 @@ int transaction_commit(struct transaction *transaction,
goto cleanup;
}
/* lock the packed refs again so no one can change it */
- if (lock_packed_refs(0)) {
- if (err)
- unable_to_lock_message(git_path("packed-refs"),
- errno, err);
+ if (lock_packed_refs(err)) {
ret = -1;
goto cleanup;
}

[Ronnie Sahlberg]

commit 167e0c6d205ed785cc17b96e22d9366accfa1665 upstream.

In many places in the code we do not have access to the individual fields
in the committer data. Instead we might only have access to prebaked data
such as what is returned by git_committer_info() containing a string
that consists of email, timestamp, zone etc.

This makes it inconvenient to use transaction_update_reflog since it means
you would have to first parse git_committer_info before you can call
update_reflog.

Add a new flag REFLOG_COMMITTER_INFO_IS_VALID to _update_reflog to tell it
that we pass in a fully prebaked committer info string that can be used as is.

At the same time, also go over and change all references from email
to id where the code actually refers to a committer id and not just an email
address. I.e. where the string is : NAME <EMAIL>

Change-Id: I179cd8cc88570801ce6ef1daa8f738e7d20851fa
Signed-off-by: Ronnie Sahlberg <***@google.com>
Signed-off-by: Jonathan Nieder <***@gmail.com>
---
builtin/reflog.c | 19 +++++++++++++------
refs.c | 21 ++++++++++++---------
refs.h | 25 +++++++++++++++++++++++--
3 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 6bb7454..be88a53 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -292,7 +292,7 @@ static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, unsig
}

static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
- const char *email, unsigned long timestamp, int tz,
+ const char *id, unsigned long timestamp, int tz,
const char *message, void *cb_data)
{
struct expire_reflog_cb *cb = cb_data;
@@ -320,9 +320,14 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
goto prune;

if (cb->t) {
+ struct reflog_committer_info ci;
+
+ memset(&ci, 0, sizeof(ci));
+ ci.id = id;
+ ci.timestamp = timestamp;
+ ci.tz = tz;
if (transaction_update_reflog(cb->t, cb->refname, nsha1, osha1,
- email, timestamp, tz, message, 0,
- &err))
+ &ci, message, 0, &err))
return -1;
hashcpy(cb->last_kept_sha1, nsha1);
}
@@ -356,6 +361,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,
struct expire_reflog_cb cb;
struct commit *tip_commit;
struct commit_list *tips;
+ struct reflog_committer_info ci;
int status = 0;

memset(&cb, 0, sizeof(cb));
@@ -368,9 +374,10 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,
status |= error("%s", err.buf);
goto cleanup;
}
+
+ memset(&ci, 0, sizeof(ci));
if (transaction_update_reflog(cb.t, cb.refname, null_sha1, null_sha1,
- NULL, 0, 0, NULL, REFLOG_TRUNCATE,
- &err)) {
+ &ci, NULL, REFLOG_TRUNCATE, &err)) {
status |= error("%s", err.buf);
goto cleanup;
}
@@ -672,7 +679,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
}

static int count_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
- const char *email, unsigned long timestamp, int tz,
+ const char *id, unsigned long timestamp, int tz,
const char *message, void *cb_data)
{
struct cmd_reflog_expire_cb *cb = cb_data;
diff --git a/refs.c b/refs.c
index ae29d11..e49fbe9 100644
--- a/refs.c
+++ b/refs.c
@@ -3226,7 +3226,7 @@ struct read_ref_at_cb {
};

static int read_ref_at_ent(unsigned char *osha1, unsigned char *nsha1,
- const char *email, unsigned long timestamp, int tz,
+ const char *id, unsigned long timestamp, int tz,
const char *message, void *cb_data)
{
struct read_ref_at_cb *cb = cb_data;
@@ -3273,7 +3273,7 @@ static int read_ref_at_ent(unsigned char *osha1, unsigned char *nsha1,
}

static int read_ref_at_ent_oldest(unsigned char *osha1, unsigned char *nsha1,
- const char *email, unsigned long timestamp,
+ const char *id, unsigned long timestamp,
int tz, const char *message, void *cb_data)
{
struct read_ref_at_cb *cb = cb_data;
@@ -3623,8 +3623,7 @@ int transaction_update_reflog(struct transaction *transaction,
const char *refname,
const unsigned char *new_sha1,
const unsigned char *old_sha1,
- const char *email,
- unsigned long timestamp, int tz,
+ struct reflog_committer_info *ci,
const char *msg, int flags,
struct strbuf *err)
{
@@ -3652,13 +3651,17 @@ int transaction_update_reflog(struct transaction *transaction,
hashcpy(update->new_sha1, new_sha1);
hashcpy(update->old_sha1, old_sha1);
update->reflog_fd = -1;
- if (email) {
+ if (flags & REFLOG_COMMITTER_INFO_IS_VALID) {
+ if (!ci->committer_info)
+ die("BUG: committer_info is NULL in reflog update");
+ update->committer = xstrdup(ci->committer_info);
+ } else if (ci->id) {
struct strbuf buf = STRBUF_INIT;
- char sign = (tz < 0) ? '-' : '+';
- int zone = (tz < 0) ? (-tz) : tz;
+ char sign = (ci->tz < 0) ? '-' : '+';
+ int zone = (ci->tz < 0) ? (-ci->tz) : ci->tz;

- strbuf_addf(&buf, "%s %lu %c%04d", email, timestamp, sign,
- zone);
+ strbuf_addf(&buf, "%s %lu %c%04d", ci->id,
+ ci->timestamp, sign, zone);
update->committer = xstrdup(buf.buf);
strbuf_release(&buf);
}
diff --git a/refs.h b/refs.h
index 2e97f4f..9153e1d 100644
--- a/refs.h
+++ b/refs.h
@@ -318,6 +318,28 @@ int transaction_delete_ref(struct transaction *transaction,
* Flags >= 0x100 are reserved for internal use.
*/
#define REFLOG_TRUNCATE 0x01
+#define REFLOG_COMMITTER_INFO_IS_VALID 0x02
+
+/*
+ * Committer data provided to reflog updates.
+ * If flags contain REFLOG_COMMITTER_DATA_IS_VALID then the structure
+ * contains a prebaked committer string just like git_committer_info()
+ * would return.
+ *
+ * If flags does not contain REFLOG_COMMITTER_DATA_IS_VALID
+ * then the committer info string will be generated using the passed
+ * email, timestamp and tz fields.
+ * This is useful for example from reflog iterators where you are passed
+ * these fields individually and not as a prebaked git_committer_info()
+ * string.
+ */
+struct reflog_committer_info {
+ const char *committer_info;
+
+ const char *id;
+ unsigned long timestamp;
+ int tz;
+};
/*
* Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
* this update will first truncate the reflog before writing the entry.
@@ -327,8 +349,7 @@ int transaction_update_reflog(struct transaction *transaction,
const char *refname,
const unsigned char *new_sha1,
const unsigned char *old_sha1,
- const char *email,
- unsigned long timestamp, int tz,
+ struct reflog_committer_info *ci,
const char *msg, int flags,
struct strbuf *err);

[Ronnie Sahlberg]

Transactions now use packed refs when deleting multiple refs so there is no
need to do it manually from remote.c any more.

Change-Id: I50cf9c4ab0c190b99e2c6c4e2f05abe260b0fe62
Signed-off-by: Ronnie Sahlberg <***@google.com>
---
builtin/remote.c | 80 ++++++++++++++++++++++++++++----------------------------
1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 6806251..42702d7 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,30 +750,27 @@ static int mv(int argc, const char **argv)

static int remove_branches(struct string_list *branches)
{
- struct strbuf err = STRBUF_INIT;
int i, result = 0;
+ struct transaction *transaction;
+ struct strbuf err = STRBUF_INIT;

- 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;
- }
+ transaction = transaction_begin(&err);
+ if (!transaction)
+ die("%s", err.buf);

- if (repack_without_refs(branches, &err))
+ for (i = 0; i < branches->nr; i++)
+ if (transaction_delete_ref(transaction,
+ branches->items[i].string, NULL,
+ 0, 0, "remote-branches", &err)) {
+ result |= error("%s", err.buf);
+ goto cleanup;
+ }
+ if (transaction_commit(transaction, &err))
result |= error("%s", err.buf);
- strbuf_release(&err);
-
- for (i = 0; i < branches->nr; i++) {
- struct string_list_item *item = branches->items + i;
- const char *refname = item->string;
-
- if (delete_ref(refname, NULL, 0))
- result |= error(_("Could not remove branch %s"), refname);
- }

+ cleanup:
+ strbuf_release(&err);
+ transaction_free(transaction);
return result;
}

@@ -1325,42 +1322,38 @@ static int prune_remote(const char *remote, int dry_run)
const char *dangling_msg = dry_run
? _(" %s will become dangling!")
: _(" %s has become dangling!");
+ struct transaction *transaction = NULL;
+ struct strbuf err = STRBUF_INIT;

memset(&states, 0, sizeof(states));
get_remote_ref_states(remote, &states, GET_REF_STATES);

- for (i = 0; i < states.stale.nr; i++) {
- string_list_insert(&delete_refs_list,
- states.stale.items[i].util);
- }
-
if (states.stale.nr) {
printf_ln(_("Pruning %s"), remote);
printf_ln(_("URL: %s"),
states.remote->url_nr
? states.remote->url[0]
: _("(no URL)"));
-
- if (!dry_run) {
- struct strbuf err = STRBUF_INIT;
-
- 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_list,
- &err))
- result |= error("%s", err.buf);
- strbuf_release(&err);
- }
}

+ if (!dry_run) {
+ transaction = transaction_begin(&err);
+ if (!transaction)
+ die("%s", err.buf);
+ }
for (i = 0; i < states.stale.nr; i++) {
const char *refname = states.stale.items[i].util;

- if (!dry_run)
- result |= delete_ref(refname, NULL, 0);
+ string_list_insert(&delete_refs_list, refname);
+
+ if (!dry_run) {
+ if (transaction_delete_ref(transaction,
+ refname, NULL,
+ 0, 0, "remote-branches", &err)) {
+ result |= error("%s", err.buf);
+ goto cleanup;
+ }
+ }

if (dry_run)
printf_ln(_(" * [would prune] %s"),
@@ -1370,6 +1363,13 @@ static int prune_remote(const char *remote, int dry_run)
abbrev_ref(refname, "refs/remotes/"));
}

+ if (!dry_run)
+ if (transaction_commit(transaction, &err))
+ result |= error("%s", err.buf);
+
+ cleanup:
+ strbuf_release(&err);
+ transaction_free(transaction);
warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
string_list_clear(&delete_refs_list, 0);

[Ronnie Sahlberg]

Change-Id: I87a1b93c4b4f4a10ad6a9f8e2bb4d53f89b28c4a
Signed-off-by: Ronnie Sahlberg <***@google.com>
---
builtin/remote.c | 23 ++++++++---------------
refs.c | 42 +++++++++++++++++++++---------------------
refs.h | 2 +-
3 files changed, 30 insertions(+), 37 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index c25420f..6806251 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -751,7 +751,6 @@ static int mv(int argc, const char **argv)
static int remove_branches(struct string_list *branches)
{
struct strbuf err = STRBUF_INIT;
- const char **branch_names;
int i, result = 0;

if (lock_packed_refs(0)) {
@@ -763,13 +762,9 @@ static int remove_branches(struct string_list *branches)
return -1;
}

- branch_names = xmalloc(branches->nr * sizeof(*branch_names));
- for (i = 0; i < branches->nr; i++)
- branch_names[i] = branches->items[i].string;
- if (repack_without_refs(branch_names, branches->nr, &err))
+ if (repack_without_refs(branches, &err))
result |= error("%s", err.buf);
strbuf_release(&err);
- free(branch_names);

for (i = 0; i < branches->nr; i++) {
struct string_list_item *item = branches->items + i;
@@ -1327,7 +1322,6 @@ static int prune_remote(const char *remote, int dry_run)
int result = 0, i;
struct ref_states states;
struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
- const char **delete_refs;
const char *dangling_msg = dry_run
? _(" %s will become dangling!")
: _(" %s has become dangling!");
@@ -1335,6 +1329,11 @@ static int prune_remote(const char *remote, int dry_run)
memset(&states, 0, sizeof(states));
get_remote_ref_states(remote, &states, GET_REF_STATES);

+ for (i = 0; i < states.stale.nr; i++) {
+ string_list_insert(&delete_refs_list,
+ states.stale.items[i].util);
+ }
+
if (states.stale.nr) {
printf_ln(_("Pruning %s"), remote);
printf_ln(_("URL: %s"),
@@ -1342,9 +1341,6 @@ static int prune_remote(const char *remote, int dry_run)
? states.remote->url[0]
: _("(no URL)"));

- delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
- for (i = 0; i < states.stale.nr; i++)
- delete_refs[i] = states.stale.items[i].util;
if (!dry_run) {
struct strbuf err = STRBUF_INIT;

@@ -1353,19 +1349,16 @@ static int prune_remote(const char *remote, int dry_run)
errno, &err);
result |= error("%s", err.buf);
} else
- if (repack_without_refs(delete_refs,
- states.stale.nr, &err))
+ if (repack_without_refs(&delete_refs_list,
+ &err))
result |= error("%s", err.buf);
strbuf_release(&err);
}
- free(delete_refs);
}

for (i = 0; i < states.stale.nr; i++) {
const char *refname = states.stale.items[i].util;

- string_list_insert(&delete_refs_list, refname);
-
if (!dry_run)
result |= delete_ref(refname, NULL, 0);

diff --git a/refs.c b/refs.c
index 662af9b..b9c8f91 100644
--- a/refs.c
+++ b/refs.c
@@ -2663,31 +2663,32 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
/*
* Must be called with packed refs already locked (and sorted)
*/
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *without, struct strbuf *err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int i, ret, removed = 0;
+ int count, ret, removed = 0;

assert(err);

/* Look for a packed ref */
- for (i = 0; i < n; i++)
- if (get_packed_ref(refnames[i]))
- break;
+ count = 0;
+ for_each_string_list_item(ref_to_delete, without)
+ if (get_packed_ref(ref_to_delete->string))
+ count++;

- /* Avoid processing if we have nothing to do */
- if (i == n) {
+ /* No refname exists in packed refs */
+ if (!count) {
rollback_packed_refs();
- return 0; /* no refname exists in packed refs */
+ return 0;
}

packed = get_packed_refs(&ref_cache);

/* Remove refnames from the cache */
- for (i = 0; i < n; i++)
- if (remove_entry(packed, refnames[i]) != -1)
+ for_each_string_list_item(ref_to_delete, without)
+ if (remove_entry(packed, ref_to_delete->string) != -1)
removed = 1;
if (!removed) {
/*
@@ -3797,12 +3798,13 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
int transaction_commit(struct transaction *transaction,
struct strbuf *err)
{
- int ret = 0, delnum = 0, i, need_repack = 0;
- const char **delnames;
+ int ret = 0, i, need_repack = 0;
int n = transaction->nr;
struct packed_ref_cache *packed_ref_cache;
struct ref_update **updates = transaction->updates;
struct ref_dir *packed;
+ struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
+ struct string_list_item *ref_to_delete;

assert(err);

@@ -3814,9 +3816,6 @@ int transaction_commit(struct transaction *transaction,
return 0;
}

- /* Allocate work space */
- delnames = xmalloc(sizeof(*delnames) * n);
-
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
if (ref_update_reject_duplicates(updates, n, err)) {
@@ -3908,7 +3907,8 @@ int transaction_commit(struct transaction *transaction,
}
try_remove_empty_parents((char *)update->refname);
if (!(update->flags & REF_ISPRUNING))
- delnames[delnum++] = xstrdup(update->lock->ref_name);
+ string_list_insert(&refs_to_delete,
+ update->lock->ref_name);
unlock_ref(update->lock);
update->lock = NULL;
}
@@ -3925,7 +3925,7 @@ int transaction_commit(struct transaction *transaction,
(update->have_old ?
update->old_sha1 :
NULL),
- NULL,
+ &refs_to_delete,
update->flags,
&update->type);
if (!update->lock) {
@@ -3939,8 +3939,8 @@ int transaction_commit(struct transaction *transaction,
}

/* delete reflog for all deleted refs */
- for (i = 0; i < delnum; i++)
- unlink_or_warn(git_path("logs/%s", delnames[i]));
+ for_each_string_list_item(ref_to_delete, &refs_to_delete)
+ unlink_or_warn(git_path("logs/%s", ref_to_delete->string));

/* Lock all reflog files */
for (i = 0; i < n; i++) {
@@ -4047,7 +4047,7 @@ int transaction_commit(struct transaction *transaction,
}
}

- if (repack_without_refs(delnames, delnum, err))
+ if (repack_without_refs(&refs_to_delete, err))
ret = TRANSACTION_GENERIC_ERROR;
clear_loose_ref_cache(&ref_cache);

@@ -4060,7 +4060,7 @@ cleanup:
for (i = 0; i < n; i++)
if (updates[i]->lock)
unlock_ref(updates[i]->lock);
- free(delnames);
+ string_list_clear(&refs_to_delete, 0);
return ret;
}

diff --git a/refs.h b/refs.h
index 9153e1d..d174380 100644
--- a/refs.h
+++ b/refs.h
@@ -163,7 +163,7 @@ extern void rollback_packed_refs(void);
*/
int pack_refs(unsigned int flags);

-extern int repack_without_refs(const char **refnames, int n,
+extern int repack_without_refs(struct string_list *without,
struct strbuf *err);

extern int ref_exists(const char *);

[Ronnie Sahlberg]

write_ref_sha1 tries to update the reflog while updating the ref.
Move these reflog changes out into its own function so that we can do the
same thing if we write a sha1 ref differently, for example by writing a ref
to the packed refs file instead.

No functional changes intended. We only move some code out into a separate
function.

Change-Id: I5ef6b32d183455685f8966379767f8c6e1ec49c9
Signed-off-by: Ronnie Sahlberg <***@google.com>
---
refs.c | 60 +++++++++++++++++++++++++++++++++++-------------------------
1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/refs.c b/refs.c
index 43df656..4eec7e4 100644
--- a/refs.c
+++ b/refs.c
@@ -3022,6 +3022,40 @@ int is_branch(const char *refname)
return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
}

+static int write_sha1_update_reflog(struct ref_lock *lock,
+ const unsigned char *sha1, const char *logmsg)
+{
+ if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
+ (strcmp(lock->ref_name, lock->orig_ref_name) &&
+ log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {
+ unlock_ref(lock);
+ return -1;
+ }
+ if (strcmp(lock->orig_ref_name, "HEAD") != 0) {
+ /*
+ * Special hack: If a branch is updated directly and HEAD
+ * points to it (may happen on the remote side of a push
+ * for example) then logically the HEAD reflog should be
+ * updated too.
+ * A generic solution implies reverse symref information,
+ * but finding all symrefs pointing to the given branch
+ * would be rather costly for this rare event (the direct
+ * update of a branch) to be worth it. So let's cheat and
+ * check with HEAD only which should cover 99% of all usage
+ * scenarios (even 100% of the default ones).
+ */
+ unsigned char head_sha1[20];
+ int head_flag;
+ const char *head_ref;
+ head_ref = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
+ head_sha1, &head_flag);
+ if (head_ref && (head_flag & REF_ISSYMREF) &&
+ !strcmp(head_ref, lock->ref_name))
+ log_ref_write("HEAD", lock->old_sha1, sha1, logmsg);
+ }
+ return 0;
+}
+
/*
* Write sha1 into the ref specified by the lock. Make sure that errno
* is sane on error.
@@ -3065,34 +3099,10 @@ static int write_ref_sha1(struct ref_lock *lock,
return -1;
}
clear_loose_ref_cache(&ref_cache);
- if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
- (strcmp(lock->ref_name, lock->orig_ref_name) &&
- log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {
+ if (write_sha1_update_reflog(lock, sha1, logmsg)) {
unlock_ref(lock);
return -1;
}
- if (strcmp(lock->orig_ref_name, "HEAD") != 0) {
- /*
- * Special hack: If a branch is updated directly and HEAD
- * points to it (may happen on the remote side of a push
- * for example) then logically the HEAD reflog should be
- * updated too.
- * A generic solution implies reverse symref information,
- * but finding all symrefs pointing to the given branch
- * would be rather costly for this rare event (the direct
- * update of a branch) to be worth it. So let's cheat and
- * check with HEAD only which should cover 99% of all usage
- * scenarios (even 100% of the default ones).
- */
- unsigned char head_sha1[20];
- int head_flag;
- const char *head_ref;
- head_ref = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
- head_sha1, &head_flag);
- if (head_ref && (head_flag & REF_ISSYMREF) &&
- !strcmp(head_ref, lock->ref_name))
- log_ref_write("HEAD", lock->old_sha1, sha1, logmsg);
- }
if (commit_ref(lock)) {
error("Couldn't set %s", lock->ref_name);
unlock_ref(lock);

[Ronnie Sahlberg]

commit 0295e9cebc41020ee84da275549b164a8770ffba upstream.

Change refs.c to use a single transaction to copy/rename both the refs and
its reflog. Since we are no longer using rename() to move the reflog file
we no longer need to disallow rename_ref for refs with a symlink for its
reflog so we can remove that test from the testsuite.

Change the function to return 1 on failure instead of either -1 or 1.

These changes make the rename_ref operation atomic. This also eliminates the
need to use rename() to shift the reflog around via a temporary filename.
As an extension to this, since we no longer use rename() on the reflog file,
we can now safely perform renames even if the reflog is a symbolic link
and thus can remove the check and fail for that condition.

Change-Id: I59477d410a34298a29cf0cbf32328b9053b158fe
Signed-off-by: Ronnie Sahlberg <***@google.com>
Signed-off-by: Jonathan Nieder <***@gmail.com>
---
refs.c | 192 ++++++++++++++++++++----------------------------------
t/t3200-branch.sh | 7 --
2 files changed, 70 insertions(+), 129 deletions(-)

diff --git a/refs.c b/refs.c
index b9c8f91..f43fef4 100644
--- a/refs.c
+++ b/refs.c
@@ -2752,58 +2752,26 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
return 0;
}

-/*
- * People using contrib's git-new-workdir have .git/logs/refs ->
- * /some/other/path/.git/logs/refs, and that may live on another device.
- *
- * IOW, to avoid cross device rename errors, the temporary renamed log must
- * live into logs/refs.
- */
-#define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"
+struct rename_reflog_cb {
+ struct transaction *transaction;
+ const char *refname;
+ struct strbuf *err;
+};

-static int rename_tmp_log(const char *newrefname)
+static int rename_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+ const char *id, unsigned long timestamp, int tz,
+ const char *message, void *cb_data)
{
- int attempts_remaining = 4;
+ struct rename_reflog_cb *cb = cb_data;
+ struct reflog_committer_info ci;

- retry:
- switch (safe_create_leading_directories(git_path("logs/%s", newrefname))) {
- case SCLD_OK:
- break; /* success */
- case SCLD_VANISHED:
- if (--attempts_remaining > 0)
- goto retry;
- /* fall through */
- default:
- error("unable to create directory for %s", newrefname);
- return -1;
- }
-
- if (rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", newrefname))) {
- if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) {
- /*
- * rename(a, b) when b is an existing
- * directory ought to result in ISDIR, but
- * Solaris 5.8 gives ENOTDIR. Sheesh.
- */
- if (remove_empty_directories(git_path("logs/%s", newrefname))) {
- error("Directory not empty: logs/%s", newrefname);
- return -1;
- }
- goto retry;
- } else if (errno == ENOENT && --attempts_remaining > 0) {
- /*
- * Maybe another process just deleted one of
- * the directories in the path to newrefname.
- * Try again from the beginning.
- */
- goto retry;
- } else {
- error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s",
- newrefname, strerror(errno));
- return -1;
- }
- }
- return 0;
+ memset(&ci, 0, sizeof(ci));
+ ci.id = id;
+ ci.timestamp = timestamp;
+ ci.tz = tz;
+ return transaction_update_reflog(cb->transaction, cb->refname,
+ nsha1, osha1, &ci, message, 0,
+ cb->err);
}

static int rename_ref_available(const char *oldname, const char *newname)
@@ -2823,91 +2791,71 @@ static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,

int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
{
- unsigned char sha1[20], orig_sha1[20];
- int flag = 0, logmoved = 0;
- struct ref_lock *lock;
- struct stat loginfo;
- int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
+ unsigned char sha1[20];
+ int flag = 0, log;
+ struct transaction *transaction = NULL;
+ struct strbuf err = STRBUF_INIT;
const char *symref = NULL;
+ struct rename_reflog_cb cb;
+ struct reflog_committer_info ci;

- if (log && S_ISLNK(loginfo.st_mode))
- return error("reflog for %s is a symlink", oldrefname);
+ memset(&ci, 0, sizeof(ci));
+ ci.committer_info = git_committer_info(0);

symref = resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING,
- orig_sha1, &flag);
- if (flag & REF_ISSYMREF)
- return error("refname %s is a symbolic ref, renaming it is not supported",
- oldrefname);
- if (!symref)
- return error("refname %s not found", oldrefname);
-
- if (!rename_ref_available(oldrefname, newrefname))
+ sha1, &flag);
+ if (flag & REF_ISSYMREF) {
+ error("refname %s is a symbolic ref, renaming it is not "
+ "supported", oldrefname);
return 1;
-
- if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG)))
- return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
- oldrefname, strerror(errno));
-
- if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
- error("unable to delete old %s", oldrefname);
- goto rollback;
}
-
- if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) &&
- delete_ref(newrefname, sha1, REF_NODEREF)) {
- if (errno==EISDIR) {
- if (remove_empty_directories(git_path("%s", newrefname))) {
- error("Directory not empty: %s", newrefname);
- goto rollback;
- }
- } else {
- error("unable to delete existing %s", newrefname);
- goto rollback;
- }
+ if (!symref) {
+ error("refname %s not found", oldrefname);
+ return 1;
}

- if (log && rename_tmp_log(newrefname))
- goto rollback;
-
- logmoved = log;
+ if (!rename_ref_available(oldrefname, newrefname))
+ return 1;

- lock = lock_ref_sha1_basic(newrefname, NULL, NULL, 0, NULL);
- if (!lock) {
- error("unable to lock %s for update", newrefname);
- goto rollback;
- }
- lock->force_write = 1;
- hashcpy(lock->old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
- error("unable to write current sha1 into %s", newrefname);
- goto rollback;
+ log = reflog_exists(oldrefname);
+ transaction = transaction_begin(&err);
+ if (!transaction)
+ goto fail;
+
+ if (strcmp(oldrefname, newrefname)) {
+ if (log && transaction_update_reflog(transaction, newrefname,
+ sha1, sha1, &ci, NULL,
+ REFLOG_TRUNCATE, &err))
+ goto fail;
+ cb.transaction = transaction;
+ cb.refname = newrefname;
+ cb.err = &err;
+ if (log && for_each_reflog_ent(oldrefname, rename_reflog_ent,
+ &cb))
+ goto fail;
+
+ if (transaction_delete_ref(transaction, oldrefname, sha1,
+ REF_NODEREF,
+ 1, NULL, &err))
+ goto fail;
}
-
+ if (transaction_update_ref(transaction, newrefname, sha1,
+ NULL, 0, 0, NULL, &err))
+ goto fail;
+ if (log && transaction_update_reflog(transaction, newrefname, sha1,
+ sha1, &ci, logmsg,
+ REFLOG_COMMITTER_INFO_IS_VALID,
+ &err))
+ goto fail;
+ if (transaction_commit(transaction, &err))
+ goto fail;
+ transaction_free(transaction);
return 0;

- rollback:
- lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, 0, NULL);
- if (!lock) {
- error("unable to lock %s for rollback", oldrefname);
- goto rollbacklog;
- }
-
- lock->force_write = 1;
- flag = log_all_ref_updates;
- log_all_ref_updates = 0;
- if (write_ref_sha1(lock, orig_sha1, NULL))
- error("unable to write current sha1 into %s", oldrefname);
- log_all_ref_updates = flag;
-
- rollbacklog:
- if (logmoved && rename(git_path("logs/%s", newrefname), git_path("logs/%s", oldrefname)))
- error("unable to restore logfile %s from %s: %s",
- oldrefname, newrefname, strerror(errno));
- if (!logmoved && log &&
- rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", oldrefname)))
- error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
- oldrefname, strerror(errno));
-
+ fail:
+ error("rename_ref failed: %s", err.buf);
+ strbuf_release(&err);
+ transaction_free(transaction);
return 1;
}

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 432921b..c6c53e4 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -302,13 +302,6 @@ test_expect_success 'renaming a symref is not allowed' '
test_path_is_missing .git/refs/heads/master3
'

-test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for u is a symlink' '
- git branch -l u &&
- mv .git/logs/refs/heads/u real-u &&
- ln -s real-u .git/logs/refs/heads/u &&
- test_must_fail git branch -m u v
-'
-
test_expect_success 'test tracking setup via --track' '
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&

[Ronnie Sahlberg]

Change add_packed_ref to return an error instead of calling die().
Update all callers to check the return value of add_packed_ref.

Change-Id: I5d5e4a75f641c4bbdb59199a233b71e86361c25c
Signed-off-by: Ronnie Sahlberg <***@google.com>
---
refs.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index c7d0825..e0d5a82 100644
--- a/refs.c
+++ b/refs.c
@@ -1224,15 +1224,16 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)
return get_packed_ref_dir(get_packed_ref_cache(refs));
}

-static void add_packed_ref(const char *refname, const unsigned char *sha1)
+static int add_packed_ref(const char *refname, const unsigned char *sha1)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(&ref_cache);

if (!packed_ref_cache->lock)
- die("internal error: packed refs not locked");
+ return -1;
add_ref(get_packed_ref_dir(packed_ref_cache),
create_ref_entry(refname, sha1, REF_ISPACKED, 1));
+ return 0;
}

/*
@@ -3795,7 +3796,13 @@ int transaction_commit(struct transaction *transaction,
sha1, NULL))
continue;

- add_packed_ref(update->refname, sha1);
+ if (add_packed_ref(update->refname, sha1)) {
+ if (err)
+ strbuf_addf(err, "Failed to add %s to packed "
+ "refs", update->refname);
+ ret = -1;
+ goto cleanup;
+ }
need_repack = 1;
}
if (need_repack) {
@@ -3909,7 +3916,13 @@ int transaction_commit(struct transaction *transaction,

packed = get_packed_refs(&ref_cache);
remove_entry(packed, update->refname);
- add_packed_ref(update->refname, update->new_sha1);
+ if (add_packed_ref(update->refname, update->new_sha1)) {
+ if (err)
+ strbuf_addf(err, "Failed to add %s to packed "
+ "refs", update->refname);
+ ret = -1;
+ goto cleanup;
+ }
need_repack = 1;

try_remove_empty_parents((char *)update->refname);

[Ronnie Sahlberg]

When we are updating more than one single ref, i.e. not a commit, then
write the updated refs directly to the packed refs file instead of writing
them as loose refs.

Change clone to use a transaction instead of using the packed refs API.
This changes the behavior of clone slightly. Previously clone would always
clone all refs into a packed refs file. With this change clone will only
clone into packed refs iff there are two or more refs being cloned.
If the repository we are cloning from only contains exactly one single ref
then clone will now store this as a loose ref. The benefit here is that
we no longer need to export a bunch of API functions to clone to access
packed refs directly. Clone can now just use a normal transaction and all
the packed refs goodness will happen automatically.

Update the t5516 test to cope with the fact that clone now only uses
packed refs if there are more than one ref being cloned.

We still use loose refs for single ref transactions, such as are used
by 'git commit' and friends. The reason for this is that if you have very
many refs then having to re-write the whole packed refs file for every
common operation like commit would have a performance impact.
That said, with these changes it should now be fairly straightforward to
add support to optionally start using packed refs for ALL updates
which could solve existing issues with name clashes in case insensitive
filesystems.

This change also means that multi-ref updates will now appear as a single
atomic change to any external observers instead of a sequence of discreete
changes.

Change-Id: I70cdcdb4341048386133e5b56b08c0b580b24c1b
Signed-off-by: Ronnie Sahlberg <***@google.com>
---
builtin/clone.c | 16 ++++++---
refs.c | 89 ++++++++++++++++++++++++++++++++++-----------------
t/t5516-fetch-push.sh | 2 +-
3 files changed, 72 insertions(+), 35 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 7f509d0..5052fac 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -498,17 +498,25 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
static void write_remote_refs(const struct ref *local_refs)
{
const struct ref *r;
+ struct transaction *transaction;
+ struct strbuf err = STRBUF_INIT;

- lock_packed_refs(LOCK_DIE_ON_ERROR);
+ transaction = transaction_begin(&err);
+ if (!transaction)
+ die("%s", err.buf);

for (r = local_refs; r; r = r->next) {
if (!r->peer_ref)
continue;
- add_packed_ref(r->peer_ref->name, r->old_sha1);
+ if (transaction_update_ref(transaction, r->peer_ref->name,
+ r->old_sha1, NULL, 0, 0, NULL,
+ &err))
+ die("%s", err.buf);
}

- if (commit_packed_refs())
- die_errno("unable to overwrite old ref-pack file");
+ if (transaction_commit(transaction, &err))
+ die("%s", err.buf);
+ transaction_free(transaction);
}

static void write_followtags(const struct ref *refs, const char *msg)
diff --git a/refs.c b/refs.c
index 4eec7e4..b64d0c7 100644
--- a/refs.c
+++ b/refs.c
@@ -2668,36 +2668,15 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int count, ret, removed = 0;
+ int ret;

assert(err);

- /* Look for a packed ref */
- count = 0;
- for_each_string_list_item(ref_to_delete, without)
- if (get_packed_ref(ref_to_delete->string))
- count++;
-
- /* No refname exists in packed refs */
- if (!count) {
- rollback_packed_refs();
- return 0;
- }
-
packed = get_packed_refs(&ref_cache);

/* Remove refnames from the cache */
for_each_string_list_item(ref_to_delete, without)
- if (remove_entry(packed, ref_to_delete->string) != -1)
- removed = 1;
- if (!removed) {
- /*
- * All packed entries disappeared while we were
- * acquiring the lock.
- */
- rollback_packed_refs();
- return 0;
- }
+ remove_entry(packed, ref_to_delete->string);

/* Remove any other accumulated cruft */
do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);
@@ -3759,6 +3738,7 @@ int transaction_commit(struct transaction *transaction,
struct strbuf *err)
{
int ret = 0, i, need_repack = 0;
+ int num_updates = 0;
int n = transaction->nr;
struct packed_ref_cache *packed_ref_cache;
struct ref_update **updates = transaction->updates;
@@ -3792,14 +3772,30 @@ int transaction_commit(struct transaction *transaction,
goto cleanup;
}

- /* any loose refs are to be deleted are first copied to packed refs */
+ /* count how many refs we are updating (not deleting) */
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update->update_type != UPDATE_SHA1)
+ continue;
+ if (is_null_sha1(update->new_sha1))
+ continue;
+
+ num_updates++;
+ }
+
+ /*
+ * Always copy loose refs that are to be deleted to the packed refs.
+ * If we are updating multiple refs then copy all refs to the packed
+ * refs file.
+ */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
unsigned char sha1[20];

if (update->update_type != UPDATE_SHA1)
continue;
- if (!is_null_sha1(update->new_sha1))
+ if (num_updates < 2 && !is_null_sha1(update->new_sha1))
continue;
if (get_packed_ref(update->refname))
continue;
@@ -3811,7 +3807,7 @@ int transaction_commit(struct transaction *transaction,
need_repack = 1;
}
if (need_repack) {
- packed = get_packed_refs(&ref_cache);;
+ packed = get_packed_refs(&ref_cache);
sort_ref_dir(packed);
if (commit_packed_refs()){
strbuf_addf(err, "unable to overwrite old ref-pack "
@@ -3828,13 +3824,15 @@ int transaction_commit(struct transaction *transaction,
goto cleanup;
}
}
+ need_repack = 0;

/*
* At this stage any refs that are to be deleted have been moved to the
- * packed refs file anf the packed refs file is deleted. We can now
+ * packed refs file and the packed refs file is committed. We can now
* safely delete these loose refs.
+ * If we are updating multiple refs then those will also be in the
+ * packed refs file so we can delete those too.
*/
-
/* Unlink any loose refs scheduled for deletion */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
@@ -3873,7 +3871,10 @@ int transaction_commit(struct transaction *transaction,
update->lock = NULL;
}

- /* Acquire all ref locks for updates while verifying old values */
+ /*
+ * Acquire all ref locks for updates while verifying old values.
+ * If we are multi-updating then update them in packed refs.
+ */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];

@@ -3896,6 +3897,30 @@ int transaction_commit(struct transaction *transaction,
update->refname);
goto cleanup;
}
+ if (num_updates < 2)
+ continue;
+
+ if (delete_ref_loose(update->lock, update->type, err)) {
+ ret = -1;
+ goto cleanup;
+ }
+ if (write_sha1_update_reflog(update->lock, update->new_sha1,
+ update->msg)) {
+ if (err)
+ strbuf_addf(err, "Failed to update log '%s'.",
+ update->refname);
+ ret = -1;
+ goto cleanup;
+ }
+ unlock_ref(update->lock);
+ update->lock = NULL;
+
+ packed = get_packed_refs(&ref_cache);
+ remove_entry(packed, update->refname);
+ add_packed_ref(update->refname, update->new_sha1);
+ need_repack = 1;
+
+ try_remove_empty_parents((char *)update->refname);
}

/* delete reflog for all deleted refs */
@@ -3944,7 +3969,7 @@ int transaction_commit(struct transaction *transaction,

if (update->update_type != UPDATE_SHA1)
continue;
- if (!is_null_sha1(update->new_sha1)) {
+ if (update->lock && !is_null_sha1(update->new_sha1)) {
if (write_ref_sha1(update->lock, update->new_sha1,
update->msg)) {
update->lock = NULL; /* freed by write_ref_sha1 */
@@ -4007,6 +4032,10 @@ int transaction_commit(struct transaction *transaction,
}
}

+ if (need_repack) {
+ packed = get_packed_refs(&ref_cache);
+ sort_ref_dir(packed);
+ }
if (repack_without_refs(&refs_to_delete, err))
ret = TRANSACTION_GENERIC_ERROR;
clear_loose_ref_cache(&ref_cache);
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 7c8a769..03375a9 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -592,7 +592,7 @@ test_expect_success 'push updates up-to-date local refs' '

test_expect_success 'push preserves up-to-date packed refs' '

- mk_test testrepo heads/master &&
+ mk_test testrepo heads/master heads/foo heads/bar &&
mk_child testrepo child &&
(
cd child &&

[Ronnie Sahlberg]

Change-Id: Ibf02549e5485ad07da66fe4b1c84f9e2b76b2aca
Signed-off-by: Ronnie Sahlberg <***@google.com>
---
refs.c | 2 +-
refs.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index b64d0c7..fddd59c 100644
--- a/refs.c
+++ b/refs.c
@@ -2663,7 +2663,7 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
/*
* Must be called with packed refs already locked (and sorted)
*/
-int repack_without_refs(struct string_list *without, struct strbuf *err)
+static int repack_without_refs(struct string_list *without, struct strbuf *err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
diff --git a/refs.h b/refs.h
index d174380..db435bf 100644
--- a/refs.h
+++ b/refs.h
@@ -163,9 +163,6 @@ extern void rollback_packed_refs(void);
*/
int pack_refs(unsigned int flags);

-extern int repack_without_refs(struct string_list *without,
- struct strbuf *err);
-
extern int ref_exists(const char *);

extern int is_branch(const char *refname);

[Ronnie Sahlberg]

Change-Id: I1e65ac429c14f01073d95c6440f820dda1c6091b
Signed-off-by: Ronnie Sahlberg <***@google.com>
---
builtin/pack-refs.c | 8 +++++++-
refs.c | 7 +++----
refs.h | 3 ++-
3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index b20b1ec..299768e 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -10,6 +10,7 @@ static char const * const pack_refs_usage[] = {
int cmd_pack_refs(int argc, const char **argv, const char *prefix)
{
unsigned int flags = PACK_REFS_PRUNE;
+ struct strbuf err = STRBUF_INIT;
struct option opts[] = {
OPT_BIT(0, "all", &flags, N_("pack everything"), PACK_REFS_ALL),
OPT_BIT(0, "prune", &flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE),
@@ -17,5 +18,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
};
if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
usage_with_options(pack_refs_usage, opts);
- return pack_refs(flags);
+ if (pack_refs(flags, &err)) {
+ error("%s", err.buf);
+ strbuf_release(&err);
+ return -1;
+ }
+ return 0;
}
diff --git a/refs.c b/refs.c
index 7fb0d6c..a5e1eff 100644
--- a/refs.c
+++ b/refs.c
@@ -2588,16 +2588,15 @@ static void prune_refs(struct ref_to_prune *r)
}
}

-int pack_refs(unsigned int flags)
+int pack_refs(unsigned int flags, struct strbuf *err)
{
struct pack_refs_cb_data cbdata;
- struct strbuf err = STRBUF_INIT;

memset(&cbdata, 0, sizeof(cbdata));
cbdata.flags = flags;

- if (lock_packed_refs(&err))
- die("%s", err.buf);
+ if (lock_packed_refs(err))
+ return -1;

cbdata.packed_refs = get_packed_refs(&ref_cache);

diff --git a/refs.h b/refs.h
index f3e08f5..be16c08 100644
--- a/refs.h
+++ b/refs.h
@@ -130,8 +130,9 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st
/*
* Write a packed-refs file for the current repository.
* flags: Combination of the above PACK_REFS_* flags.
+ * Returns 0 on success and fills in err on failure.
*/
-int pack_refs(unsigned int flags);
+int pack_refs(unsigned int flags, struct strbuf *err);

extern int ref_exists(const char *);

[Ronnie Sahlberg]

Get rid of the action_on_err enum and replace the action argument to
update_ref with a strbuf *err for error reporting.

Update all callers to the new api including two callers in transport*.c
which used the literal 0 instead of an enum.

Change-Id: I8ee1540380393380b5a06db380fb8491bfe1ecdd
Signed-off-by: Ronnie Sahlberg <***@google.com>
---
builtin/checkout.c | 7 +++++--
builtin/clone.c | 20 ++++++++++++--------
builtin/merge.c | 20 +++++++++++++-------
builtin/notes.c | 24 ++++++++++++++----------
builtin/reset.c | 12 ++++++++----
builtin/update-ref.c | 7 +++++--
notes-cache.c | 2 +-
notes-utils.c | 5 +++--
refs.c | 14 +++-----------
refs.h | 10 ++--------
transport-helper.c | 7 ++++++-
transport.c | 9 ++++++---
12 files changed, 78 insertions(+), 59 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8550b6d..60a68f7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -584,6 +584,8 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
{
struct strbuf msg = STRBUF_INIT;
const char *old_desc, *reflog_msg;
+ struct strbuf err = STRBUF_INIT;
+
if (opts->new_branch) {
if (opts->new_orphan_branch) {
if (opts->new_branch_log && !log_all_ref_updates) {
@@ -621,8 +623,9 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
if (!strcmp(new->name, "HEAD") && !new->path && !opts->force_detach) {
/* Nothing to do. */
} else if (opts->force_detach || !new->path) { /* No longer on any branch. */
- update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL,
- REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL,
+ REF_NODEREF, &err))
+ die("%s", err.buf);
if (!opts->quiet) {
if (old->path && advice_detached_head)
detach_advice(new->name);
diff --git a/builtin/clone.c b/builtin/clone.c
index 5052fac..e0a671d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -522,6 +522,7 @@ static void write_remote_refs(const struct ref *local_refs)
static void write_followtags(const struct ref *refs, const char *msg)
{
const struct ref *ref;
+ struct strbuf err = STRBUF_INIT;
for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
@@ -529,8 +530,9 @@ static void write_followtags(const struct ref *refs, const char *msg)
continue;
if (!has_sha1_file(ref->old_sha1))
continue;
- update_ref(msg, ref->name, ref->old_sha1,
- NULL, 0, UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref(msg, ref->name, ref->old_sha1,
+ NULL, 0, &err))
+ die("%s", err.buf);
}
}

@@ -593,28 +595,30 @@ static void update_remote_refs(const struct ref *refs,
static void update_head(const struct ref *our, const struct ref *remote,
const char *msg)
{
+ struct strbuf err = STRBUF_INIT;
const char *head;
if (our && skip_prefix(our->name, "refs/heads/", &head)) {
/* Local default branch link */
create_symref("HEAD", our->name, NULL);
if (!option_bare) {
- update_ref(msg, "HEAD", our->old_sha1, NULL, 0,
- UPDATE_REFS_DIE_ON_ERR);
+ update_ref(msg, "HEAD", our->old_sha1, NULL, 0, &err);
install_branch_config(0, head, option_origin, our->name);
}
} else if (our) {
struct commit *c = lookup_commit_reference(our->old_sha1);
/* --branch specifies a non-branch (i.e. tags), detach HEAD */
- update_ref(msg, "HEAD", c->object.sha1,
- NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref(msg, "HEAD", c->object.sha1,
+ NULL, REF_NODEREF, &err))
+ die("%s", err.buf);
} else if (remote) {
/*
* We know remote HEAD points to a non-branch, or
* HEAD points to a branch but we don't know which one.
* Detach HEAD in all these cases.
*/
- update_ref(msg, "HEAD", remote->old_sha1,
- NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref(msg, "HEAD", remote->old_sha1,
+ NULL, REF_NODEREF, &err))
+ die("%s", err.buf);
}
}

diff --git a/builtin/merge.c b/builtin/merge.c
index bebbe5b..a787b6a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -396,9 +396,11 @@ static void finish(struct commit *head_commit,
printf(_("No merge message -- not updating HEAD\n"));
else {
const char *argv_gc_auto[] = { "gc", "--auto", NULL };
- update_ref(reflog_message.buf, "HEAD",
- new_head, head, 0,
- UPDATE_REFS_DIE_ON_ERR);
+ struct strbuf err = STRBUF_INIT;
+ if (update_ref(reflog_message.buf, "HEAD",
+ new_head, head, 0,
+ &err))
+ die("%s", err.buf);
/*
* We ignore errors in 'gc --auto', since the
* user should see them.
@@ -1086,6 +1088,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
unsigned char head_sha1[20];
struct commit *head_commit;
struct strbuf buf = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
const char *head_arg;
int flag, i, ret = 0, head_subsumed;
int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
@@ -1214,8 +1217,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (!remote_head)
die(_("%s - not something we can merge"), argv[0]);
read_empty(remote_head->object.sha1, 0);
- update_ref("initial pull", "HEAD", remote_head->object.sha1,
- NULL, 0, UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref("initial pull", "HEAD", remote_head->object.sha1,
+ NULL, 0, &err))
+ die("%s", err.buf);
goto done;
} else {
struct strbuf merge_names = STRBUF_INIT;
@@ -1328,8 +1332,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
free(list);
}

- update_ref("updating ORIG_HEAD", "ORIG_HEAD", head_commit->object.sha1,
- NULL, 0, UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref("updating ORIG_HEAD", "ORIG_HEAD",
+ head_commit->object.sha1,
+ NULL, 0, &err))
+ die("%s", err.buf);

if (remoteheads && !common)
; /* No common ancestors found. We need a real merge. */
diff --git a/builtin/notes.c b/builtin/notes.c
index 68b6cd8..b9fec39 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -674,6 +674,7 @@ static int merge_abort(struct notes_merge_options *o)
static int merge_commit(struct notes_merge_options *o)
{
struct strbuf msg = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
unsigned char sha1[20], parent_sha1[20];
struct notes_tree *t;
struct commit *partial;
@@ -714,10 +715,10 @@ static int merge_commit(struct notes_merge_options *o)
format_commit_message(partial, "%s", &msg, &pretty_ctx);
strbuf_trim(&msg);
strbuf_insert(&msg, 0, "notes: ", 7);
- update_ref(msg.buf, o->local_ref, sha1,
- is_null_sha1(parent_sha1) ? NULL : parent_sha1,
- 0, UPDATE_REFS_DIE_ON_ERR);
-
+ if (update_ref(msg.buf, o->local_ref, sha1,
+ is_null_sha1(parent_sha1) ? NULL : parent_sha1,
+ 0, &err))
+ die("%s", err.buf);
free_notes(t);
strbuf_release(&msg);
ret = merge_abort(o);
@@ -728,6 +729,7 @@ static int merge_commit(struct notes_merge_options *o)
static int merge(int argc, const char **argv, const char *prefix)
{
struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
unsigned char result_sha1[20];
struct notes_tree *t;
struct notes_merge_options o;
@@ -808,14 +810,16 @@ static int merge(int argc, const char **argv, const char *prefix)

result = notes_merge(&o, t, result_sha1);

- if (result >= 0) /* Merge resulted (trivially) in result_sha1 */
+ if (result >= 0) {/* Merge resulted (trivially) in result_sha1 */
/* Update default notes ref with new commit */
- update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,
- 0, UPDATE_REFS_DIE_ON_ERR);
- else { /* Merge has unresolved conflicts */
+ if (update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,
+ 0, &err))
+ die("%s", err.buf);
+ } else { /* Merge has unresolved conflicts */
/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */
- update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL,
- 0, UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL,
+ 0, &err))
+ die("%s", err.buf);
/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */
if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL))
die("Failed to store link to current notes ref (%s)",
diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..8ebf4ca 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -245,6 +245,7 @@ static int reset_refs(const char *rev, const unsigned char *sha1)
{
int update_ref_status;
struct strbuf msg = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
unsigned char *orig = NULL, sha1_orig[20],
*old_orig = NULL, sha1_old_orig[20];

@@ -253,13 +254,16 @@ static int reset_refs(const char *rev, const unsigned char *sha1)
if (!get_sha1("HEAD", sha1_orig)) {
orig = sha1_orig;
set_reflog_message(&msg, "updating ORIG_HEAD", NULL);
- update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0,
- UPDATE_REFS_MSG_ON_ERR);
+ if (update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, &err))
+ error("%s", err.buf);
+ strbuf_release(&err);
} else if (old_orig)
delete_ref("ORIG_HEAD", old_orig, 0);
set_reflog_message(&msg, "updating HEAD", rev);
- update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0,
- UPDATE_REFS_MSG_ON_ERR);
+ update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0, &err);
+ if (update_ref_status)
+ error("%s", err.buf);
+ strbuf_release(&err);
strbuf_release(&msg);
return update_ref_status;
}
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index af08dd9..f650647 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -358,6 +358,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
const char *refname, *oldval;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the update")),
OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
@@ -421,6 +422,8 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
if (delete)
return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
else
- return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
- flags, UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
+ flags, &err))
+ die("%s", err.buf);
+ return 0;
}
diff --git a/notes-cache.c b/notes-cache.c
index c4e9bb7..386e6d6 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -60,7 +60,7 @@ int notes_cache_write(struct notes_cache *c)
commit_sha1, NULL, NULL) < 0)
return -1;
if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL,
- 0, UPDATE_REFS_QUIET_ON_ERR) < 0)
+ 0, NULL) < 0)
return -1;

return 0;
diff --git a/notes-utils.c b/notes-utils.c
index b64dc1b..bcfe61e 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -34,6 +34,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
{
struct strbuf buf = STRBUF_INIT;
unsigned char commit_sha1[20];
+ struct strbuf err = STRBUF_INIT;

if (!t)
t = &default_notes_tree;
@@ -49,8 +50,8 @@ void commit_notes(struct notes_tree *t, const char *msg)

create_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);
strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */
- update_ref(buf.buf, t->ref, commit_sha1, NULL, 0,
- UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, &err))
+ die("%s", err.buf);

strbuf_release(&buf);
}
diff --git a/refs.c b/refs.c
index 1261a78..c7d0825 100644
--- a/refs.c
+++ b/refs.c
@@ -3675,7 +3675,7 @@ int transaction_delete_ref(struct transaction *transaction,

int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
- int flags, enum action_on_err onerr)
+ int flags, struct strbuf *e)
{
struct transaction *t;
struct strbuf err = STRBUF_INIT;
@@ -3688,16 +3688,8 @@ int update_ref(const char *action, const char *refname,
const char *str = "update_ref failed for ref '%s': %s";

transaction_free(t);
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR:
- error(str, refname, err.buf);
- break;
- case UPDATE_REFS_DIE_ON_ERR:
- die(str, refname, err.buf);
- break;
- case UPDATE_REFS_QUIET_ON_ERR:
- break;
- }
+ if (e)
+ strbuf_addf(e, str, refname, err.buf);
strbuf_release(&err);
return 1;
}
diff --git a/refs.h b/refs.h
index 44ae7fe..f3e08f5 100644
--- a/refs.h
+++ b/refs.h
@@ -212,12 +212,6 @@ extern int rename_ref(const char *oldref, const char *newref, const char *logmsg
*/
extern int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1);

-enum action_on_err {
- UPDATE_REFS_MSG_ON_ERR,
- UPDATE_REFS_DIE_ON_ERR,
- UPDATE_REFS_QUIET_ON_ERR
-};
-
/*
* Begin a reference transaction. The reference transaction must
* be freed by calling transaction_free().
@@ -340,8 +334,8 @@ void transaction_free(struct transaction *transaction);

/** Lock a ref and then write its file */
int update_ref(const char *action, const char *refname,
- const unsigned char *sha1, const unsigned char *oldval,
- int flags, enum action_on_err onerr);
+ const unsigned char *sha1, const unsigned char *oldval,
+ int flags, struct strbuf *err);

extern int parse_hide_refs_config(const char *var, const char *value, const char *);
extern int ref_is_hidden(const char *);
diff --git a/transport-helper.c b/transport-helper.c
index 9b29620..653cf74 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -735,6 +735,7 @@ static int push_update_refs_status(struct helper_data *data,
{
struct strbuf buf = STRBUF_INIT;
struct ref *ref = remote_refs;
+ struct strbuf err = STRBUF_INIT;
int ret = 0;

for (;;) {
@@ -758,7 +759,11 @@ static int push_update_refs_status(struct helper_data *data,
private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
if (!private)
continue;
- update_ref("update by helper", private, ref->new_sha1, NULL, 0, 0);
+ if (update_ref("update by helper", private, ref->new_sha1,
+ NULL, 0, &err))
+ error("%s", err.buf);
+ strbuf_release(&err);
+
free(private);
}
strbuf_release(&buf);
diff --git a/transport.c b/transport.c
index b56620e..51570d6 100644
--- a/transport.c
+++ b/transport.c
@@ -597,6 +597,7 @@ int transport_refs_pushed(struct ref *ref)
void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int verbose)
{
struct refspec rs;
+ struct strbuf err = STRBUF_INIT;

if (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)
return;
@@ -609,9 +610,11 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
if (ref->deletion) {
delete_ref(rs.dst, NULL, 0);
- } else
- update_ref("update by push", rs.dst,
- ref->new_sha1, NULL, 0, 0);
+ } else if (update_ref("update by push", rs.dst,
+ ref->new_sha1, NULL, 0, &err))
+ error("%s", err.buf);
+
+ strbuf_release(&err);
free(rs.dst);
}
}

[Ronnie Sahlberg]

We no longer need to expose the lock/add/commit/rollback functions
for packed refs anymore so make them static and remove them from the
public api.

Change-Id: I1059f1690129f0232cb27872ef494024ef7f299e
Signed-off-by: Ronnie Sahlberg <***@google.com>
---
refs.c | 8 ++++----
refs.h | 30 ------------------------------
2 files changed, 4 insertions(+), 34 deletions(-)

diff --git a/refs.c b/refs.c
index fddd59c..1261a78 100644
--- a/refs.c
+++ b/refs.c
@@ -1224,7 +1224,7 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)
return get_packed_ref_dir(get_packed_ref_cache(refs));
}

-void add_packed_ref(const char *refname, const unsigned char *sha1)
+static void add_packed_ref(const char *refname, const unsigned char *sha1)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(&ref_cache);
@@ -2393,7 +2393,7 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
}

/* This should return a meaningful errno on failure */
-int lock_packed_refs(int flags)
+static int lock_packed_refs(int flags)
{
struct packed_ref_cache *packed_ref_cache;

@@ -2416,7 +2416,7 @@ int lock_packed_refs(int flags)
* Commit the packed refs changes.
* On error we must make sure that errno contains a meaningful value.
*/
-int commit_packed_refs(void)
+static int commit_packed_refs(void)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(&ref_cache);
@@ -2445,7 +2445,7 @@ int commit_packed_refs(void)
return error;
}

-void rollback_packed_refs(void)
+static void rollback_packed_refs(void)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(&ref_cache);
diff --git a/refs.h b/refs.h
index db435bf..44ae7fe 100644
--- a/refs.h
+++ b/refs.h
@@ -120,36 +120,6 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn
extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct string_list *refnames);

/*
- * Lock the packed-refs file for writing. Flags is passed to
- * hold_lock_file_for_update(). Return 0 on success.
- * Errno is set to something meaningful on error.
- */
-extern int lock_packed_refs(int flags);
-
-/*
- * Add a reference to the in-memory packed reference cache. This may
- * only be called while the packed-refs file is locked (see
- * lock_packed_refs()). To actually write the packed-refs file, call
- * commit_packed_refs().
- */
-extern void add_packed_ref(const char *refname, const unsigned char *sha1);
-
-/*
- * Write the current version of the packed refs cache from memory to
- * disk. The packed-refs file must already be locked for writing (see
- * lock_packed_refs()). Return zero on success.
- * Sets errno to something meaningful on error.
- */
-extern int commit_packed_refs(void);
-
-/*
- * Rollback the lockfile for the packed-refs file, and discard the
- * in-memory packed reference cache. (The packed-refs file will be
- * read anew if it is needed again after this function is called.)
- */
-extern void rollback_packed_refs(void);
-
-/*
* Flags for controlling behaviour of pack_refs()
* PACK_REFS_PRUNE: Prune loose refs after packing
* PACK_REFS_ALL: Pack _all_ refs, not just tags and already packed refs

[Ronnie Sahlberg]

commit 3989c2a763c3b355785d609b3144c7935dffb273 upstream.

Change-Id: I63a22da521ecc8eb60d7a8aaa5af666d2827a599
Signed-off-by: Ronnie Sahlberg <***@google.com>
Signed-off-by: Jonathan Nieder <***@gmail.com>
---
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index f43fef4..43df656 100644
--- a/refs.c
+++ b/refs.c
@@ -2702,8 +2702,10 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
/* Remove any other accumulated cruft */
do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);
for_each_string_list_item(ref_to_delete, &refs_to_delete) {
- if (remove_entry(packed, ref_to_delete->string) == -1)
+ if (remove_entry(packed, ref_to_delete->string) == -1) {
+ rollback_packed_refs();
die("internal error");
+ }
}

/* Write what remains */

[Junio C Hamano]

Post by Ronnie Sahlberg
commit fb5fa1d338ce113b0fea3bb955b50bbb3e827805 upstream.

Huh?

Post by Ronnie Sahlberg
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
@@ -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 "){".

Post by Ronnie Sahlberg
+ 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");

Post by Ronnie Sahlberg
...
+ if (!update->lock) {
+ int df_conflict = (errno == ENOTDIR);
+
+ strbuf_addf(err, "Cannot lock the ref '%s'.",
+ update->refname);
+ ret = df_conflict ?

A trailing SP here...

Post by Ronnie Sahlberg
@@ -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?