threads / patch / 37213

patch, 5 partsrefs.c: allow passing raw git_committer_info as email to _update_reflog

Subject: [PATCH 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog

## tl;dr

13 messages between Jul 25, 2014 and Jul 28, 2014. Diffs are folded; open one to read it.

replies: 12people: 3as markdown or json

Ronnie Sahlberg· Jul 25, 2014, 16:58 UTC · lore

[PATCH 0/5] ref-transactions-rename

This patch series adds support for using transactions and atomic renames. It focuses on what needs to be done in order to support fully atomic and rollbackable renames that may or may not involve name conflicts.

By performing the actual delete old/create new via a single operation to the packed refs file this process will also appear as an atomic change for any external observers.

(While this series is "short" it contains a very important change where we start performing ref changes as operations inside a locked packed refs file instead of as discreete operations of loose ref files. This approach will be extended in the next patch series where we will start using it also to make multi-ref creations/updates become fully atomic for external observers.)

This series is built on iand depend on the previous reflog series:
  * rs/ref-transaction-reflog (2014-07-23) 15 commits
   - refs.c: allow deleting refs with a broken sha1
   - refs.c: remove lock_any_ref_for_update
   - refs.c: make unlock_ref/close_ref/commit_ref static
   - refs.c: rename log_ref_setup to create_reflog
   - reflog.c: use a reflog transaction when writing during expire
   - refs.c: allow multiple reflog updates during a single transaction
   - refs.c: only write reflog update if msg is non-NULL
   - refs.c: add a flag to allow reflog updates to truncate the log
   - refs.c: add a transaction function to append a reflog entry
   - lockfile.c: make hold_lock_file_for_append preserve meaningful errno
   - refs.c: add a function to append a reflog entry to a fd
   - refs.c: add a new update_type field to ref_update
   - refs.c: rename the transaction functions
   - refs.c: make ref_transaction_delete a wrapper for ref_transaction_update
   - refs.c: make ref_transaction_create a wrapper to ref_transaction_update
   (this branch uses rs/ref-transaction and rs/ref-transaction-1.)
Ronnie Sahlberg (5):
  refs.c: allow passing raw git_committer_info as email to
    _update_reflog
  refs.c: return error instead of dying when locking fails during
    transaction
  refs.c: use packed refs when deleting refs during a transaction
  refs.c: update rename_ref to use a transaction
  refs.c: rollback the lockfile before we die() in repack_without_refs
 builtin/remote.c  |  13 +-
 refs.c            | 369 +++++++++++++++++++++++++++++-------------------------
 refs.h            |   1 +
 t/t3200-branch.sh |   7 --
 4 files changed, 210 insertions(+), 180 deletions(-)
-- 
2.0.1.508.g763ab16
Ronnie Sahlberg· Jul 25, 2014, 16:58 UTC · re: Ronnie Sahlberg · lore

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_EMAIL_IS_COMMITTER to _update_reflog to tell it that what we pass in as email is already the fully baked committer string we can use as-is.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 20 ++++++++++++--------
 refs.h |  1 +
 2 files changed, 13 insertions(+), 8 deletions(-)
Show changes to 2 files +13 −8

refs.c, refs.h

diff --git a/refs.c b/refs.c
index 2662ef6..6c55032 100644
--- a/refs.c
+++ b/refs.c
@@ -3522,14 +3522,18 @@ int transaction_update_reflog(struct ref_transaction *transaction,
 	hashcpy(update->old_sha1, old_sha1);
 	update->reflog_fd = -1;
 	if (email) {
-		struct strbuf buf = STRBUF_INIT;
-		char sign = (tz < 0) ? '-' : '+';
-		int zone = (tz < 0) ? (-tz) : tz;
-
-		strbuf_addf(&buf, "%s %lu %c%04d", email, timestamp, sign,
-			    zone);
-		update->committer = xstrdup(buf.buf);
-		strbuf_release(&buf);
+		if (flags & REFLOG_EMAIL_IS_COMMITTER)
+			update->committer = xstrdup(email);
+		else {
+			struct strbuf buf = STRBUF_INIT;
+			char sign = (tz < 0) ? '-' : '+';
+			int zone = (tz < 0) ? (-tz) : tz;
+
+			strbuf_addf(&buf, "%s %lu %c%04d", email, timestamp,
+				    sign, zone);
+			update->committer = xstrdup(buf.buf);
+			strbuf_release(&buf);
+		}
 	}
 	if (msg)
 		update->msg = xstrdup(msg);
diff --git a/refs.h b/refs.h
index 0172f48..eb918a0 100644
--- a/refs.h
+++ b/refs.h
@@ -309,6 +309,7 @@ int transaction_delete_sha1(struct ref_transaction *transaction,
  * Flags >= 0x100 are reserved for internal use.
  */
 #define REFLOG_TRUNCATE 0x01
+#define REFLOG_EMAIL_IS_COMMITTER 0x02
 /*
  * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
  * this update will first truncate the reflog before writing the entry.

-- 
2.0.1.508.g763ab16
Jonathan Nieder· Jul 25, 2014, 19:37 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog

Ronnie Sahlberg wrote:
> Add a new flag REFLOG_EMAIL_IS_COMMITTER to _update_reflog to tell it
> that what we pass in as email is already the fully baked committer string
> we can use as-is.

With and without the new flag, the 'email' argument has two different meanings:

 * with the new flag, it should be an ident string, like
   'Jonathan Nieder <jrnieder@gmail.com> 1406251347 -0700'
 * without it, it should be the name-part of an ident string,
   like 'Jonathan Nieder <jrnieder@gmail.com>

In neither case is it an email address. This seems unnecessarily confusing.

Is the caller responsible for checking the argument for validity? Do callers do so? Is this performance-critical or could the transaction_update_reflog function do a sanity-check?

[...]
Show 13 quoted lines
>  /*
>   * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
>   * this update will first truncate the reflog before writing the entry.
>   * If msg is NULL no update will be written to the log.
>   */
>  int transaction_update_reflog(struct ref_transaction *transaction,
>                                const char *refname,
>                                const unsigned char *new_sha1,
>                                const unsigned char *old_sha1,
>                                const char *email,
>                                unsigned long timestamp, int tz,
>                                const char *msg, int flags,
>                                struct strbuf *err);

This is a lot of parameters, some optional, not all documented. Would it make sense to pack some into a struct?

Thanks and hope that helps, Jonathan

Ronnie Sahlberg· Jul 28, 2014, 18:01 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog

On Fri, Jul 25, 2014 at 12:37 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 17 quoted lines
> Ronnie Sahlberg wrote:
>
>> Add a new flag REFLOG_EMAIL_IS_COMMITTER to _update_reflog to tell it
>> that what we pass in as email is already the fully baked committer string
>> we can use as-is.
>
> With and without the new flag, the 'email' argument has two different
> meanings:
>
>  * with the new flag, it should be an ident string, like
>    'Jonathan Nieder <jrnieder@gmail.com> 1406251347 -0700'
>
>  * without it, it should be the name-part of an ident string,
>    like 'Jonathan Nieder <jrnieder@gmail.com>
>
> In neither case is it an email address.  This seems unnecessarily
> confusing.
True.

I have changed it to to be a field named 'id' instead of 'email'. I also added changes to update other places in the code to change the name for this to 'id' too.

It is confusing. I too were caught by this a while back when just based on the name for the callback iterators I assumed what was passed there was an email address while it was not. I didn't fix that then but I fixed it now.

Thanks!
>
> Is the caller responsible for checking the argument for validity?
> Do callers do so?  Is this performance-critical or could the
> transaction_update_reflog function do a sanity-check?
I have added basic sanity checks, such as required strings are non-NULL.
Show 18 quoted lines
>
> [...]
>>  /*
>>   * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
>>   * this update will first truncate the reflog before writing the entry.
>>   * If msg is NULL no update will be written to the log.
>>   */
>>  int transaction_update_reflog(struct ref_transaction *transaction,
>>                                const char *refname,
>>                                const unsigned char *new_sha1,
>>                                const unsigned char *old_sha1,
>>                                const char *email,
>>                                unsigned long timestamp, int tz,
>>                                const char *msg, int flags,
>>                                struct strbuf *err);
>
> This is a lot of parameters, some optional, not all documented.  Would
> it make sense to pack some into a struct?
I changed email,timestamp,tz into a struct
/*
 * Committer data provided to reflog updates.
 * If flags contain REFLOG_COMMITTER_DATA_IS_VALID then
 * 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; };

>
> Thanks and hope that helps,
> Jonathan
Eric Sunshine· Jul 28, 2014, 23:39 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog

On Mon, Jul 28, 2014 at 2:01 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
Show 24 quoted lines
> On Fri, Jul 25, 2014 at 12:37 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Ronnie Sahlberg wrote:
>>>  /*
>>>   * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
>>>   * this update will first truncate the reflog before writing the entry.
>>>   * If msg is NULL no update will be written to the log.
>>>   */
>>>  int transaction_update_reflog(struct ref_transaction *transaction,
>>>                                const char *refname,
>>>                                const unsigned char *new_sha1,
>>>                                const unsigned char *old_sha1,
>>>                                const char *email,
>>>                                unsigned long timestamp, int tz,
>>>                                const char *msg, int flags,
>>>                                struct strbuf *err);
>>
>> This is a lot of parameters, some optional, not all documented.  Would
>> it make sense to pack some into a struct?
>
> I changed email,timestamp,tz into a struct
> /*
>  * Committer data provided to reflog updates.
>  * If flags contain REFLOG_COMMITTER_DATA_IS_VALID then
>  * then the structure contains a prebaked committer string
s/then then/then/
Show 20 quoted lines
>  * 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;
> };
>
>>
>> Thanks and hope that helps,
>> Jonathan
Ronnie Sahlberg· Jul 25, 2014, 16:58 UTC · re: Ronnie Sahlberg · lore

[PATCH 2/5] refs.c: return error instead of dying when locking fails during transaction

Change lock_ref_sha1_basic to return an error instead of dying when we fail to lock a file during a transaction.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to refs.c +1 −1
diff --git a/refs.c b/refs.c
index 6c55032..2ea85a8 100644
--- a/refs.c
+++ b/refs.c
@@ -2214,7 +2214,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			 */
 			goto retry;
 		else
-			unable_to_lock_index_die(ref_file, errno);
+			goto error_return;
 	}
 	if (bad_ref)
 		return lock;

-- 
2.0.1.508.g763ab16
Jonathan Nieder· Jul 25, 2014, 19:40 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 2/5] refs.c: return error instead of dying when locking fails during transaction

Ronnie Sahlberg wrote:
Show 8 quoted lines
> --- a/refs.c
> +++ b/refs.c
> @@ -2214,7 +2214,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  			 */
>  			goto retry;
>  		else
> -			unable_to_lock_index_die(ref_file, errno);
> +			goto error_return;

Should probably save last_errno so error_return can pass that information back.

Can the caller recover from this error? Does it have enough information to produce the same helpful message as unable_to_lock_index_die?

If this could be done without regressing behavior (e.g., by passing back error information as a message instead of through errno) then I think it would make sense.

Jonathan
Ronnie Sahlberg· Jul 28, 2014, 19:01 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/5] refs.c: return error instead of dying when locking fails during transaction

On Fri, Jul 25, 2014 at 12:40 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 13 quoted lines
> Ronnie Sahlberg wrote:
>
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2214,7 +2214,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>>                        */
>>                       goto retry;
>>               else
>> -                     unable_to_lock_index_die(ref_file, errno);
>> +                     goto error_return;
>
> Should probably save last_errno so error_return can pass that
> information back.
Done. Thanks.
Show 7 quoted lines
>
> Can the caller recover from this error?  Does it have enough information
> to produce the same helpful message as unable_to_lock_index_die?
>
> If this could be done without regressing behavior (e.g., by passing
> back error information as a message instead of through errno) then I
> think it would make sense.

The callers should all be able to cope with this returning error (and now logging error() since this function is only called from within transaction_commit() and it knows how to deal with these errors.

>
> Jonathan
Ronnie Sahlberg· Jul 25, 2014, 16:58 UTC · re: Ronnie Sahlberg · lore

[PATCH 3/5] refs.c: use packed refs when deleting refs during a transaction

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.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/remote.c |  13 +++--
 refs.c           | 151 +++++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 128 insertions(+), 36 deletions(-)
Show changes to 2 files +128 −36

builtin/remote.c, refs.c

diff --git a/builtin/remote.c b/builtin/remote.c
index be8ebac..9a9cc92 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -756,6 +756,8 @@ static int remove_branches(struct string_list *branches)
 	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
 	for (i = 0; i < branches->nr; i++)
 		branch_names[i] = branches->items[i].string;
+	if (lock_packed_refs(0))
+		result |= unable_to_lock_error(git_path("packed-refs"), errno);
 	result |= repack_without_refs(branch_names, branches->nr, NULL);
 	free(branch_names);
 
@@ -1333,9 +1335,14 @@ static int prune_remote(const char *remote, int dry_run)
 		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)
-			result |= repack_without_refs(delete_refs,
-						      states.stale.nr, NULL);
+		if (!dry_run) {
+			if (lock_packed_refs(0))
+				result |= unable_to_lock_error(
+					git_path("packed-refs"), errno);
+			else
+				result |= repack_without_refs(delete_refs,
+							states.stale.nr, NULL);
+		}
 		free(delete_refs);
 	}
 
diff --git a/refs.c b/refs.c
index 2ea85a8..0d800f1 100644
--- a/refs.c
+++ b/refs.c
@@ -1330,7 +1330,7 @@ static struct ref_entry *get_packed_ref(const char *refname)
 
 /*
  * A loose ref file doesn't exist; check for a packed ref.  The
- * options are forwarded from resolve_safe_unsafe().
+ * options are forwarded from resolve_ref_unsafe().
  */
 static const char *handle_missing_loose_ref(const char *refname,
 					    unsigned char *sha1,
@@ -1387,7 +1387,6 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int fla
 		}
 
 		git_snpath(path, sizeof(path), "%s", refname);
-
 		/*
 		 * We might have to loop back here to avoid a race
 		 * condition: first we lstat() the file, then we try
@@ -2532,6 +2531,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;
@@ -2544,19 +2546,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)) {
-		if (err) {
-			unable_to_lock_message(git_path("packed-refs"), errno,
-					       err);
-			return -1;
-		}
-		unable_to_lock_error(git_path("packed-refs"), errno);
-		return error("cannot delete '%s' from packed refs", refnames[i]);
 	}
+
 	packed = get_packed_refs(&ref_cache);
 
 	/* Remove refnames from the cache */
@@ -3669,10 +3664,12 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 int transaction_commit(struct ref_transaction *transaction,
 		       struct strbuf *err)
 {
-	int ret = 0, delnum = 0, i, df_conflict = 0;
+	int ret = 0, delnum = 0, i, df_conflict = 0, 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;
 
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: commit called for transaction that is not open");
@@ -3692,12 +3689,65 @@ int transaction_commit(struct ref_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];
+		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, sha1, 1, NULL))
+			continue;
+
+		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->lock = lock_ref_sha1_basic(update->refname,
 						   (update->have_old ?
 						    update->old_sha1 :
@@ -3714,8 +3764,47 @@ int transaction_commit(struct ref_transaction *transaction,
 			ret = -1;
 			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),
+						   update->flags,
+						   &update->type,
+						   delnames, delnum);
+		if (!update->lock) {
+			if (errno == ENOTDIR)
+				df_conflict = 1;
+			if (err)
+				strbuf_addf(err, "Cannot lock the ref '%s'.",
+					    update->refname);
+			ret = -1;
+			goto cleanup;
+		}
+	}
+
+	/* 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];
@@ -3727,6 +3816,16 @@ int transaction_commit(struct ref_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),
@@ -3742,7 +3841,7 @@ int transaction_commit(struct ref_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];
 
@@ -3763,21 +3862,6 @@ int transaction_commit(struct ref_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 = -1;
-
-			if (!(update->flags & REF_ISPRUNING))
-				delnames[delnum++] = update->lock->ref_name;
-		}
-	}
-
 	/*
 	 * Update all reflog files
 	 * We have already committed all ref updates and deletes.
@@ -3830,11 +3914,12 @@ int transaction_commit(struct ref_transaction *transaction,
 
 	if (repack_without_refs(delnames, delnum, err))
 		ret = -1;
-	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 = ret ? REF_TRANSACTION_ERROR
 		: REF_TRANSACTION_CLOSED;
 

-- 
2.0.1.508.g763ab16
Jonathan Nieder· Jul 25, 2014, 19:58 UTC · re: Ronnie Sahlberg · lore

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

Ronnie Sahlberg wrote:
Show 5 quoted lines
> 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.
[...]
Show 5 quoted lines
> 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
Makes a lot of sense.

This can potentially slow down a single-ref deletion in a repository with a huge amount of refs (e.g., a Gerrit server with lots of refs/changes/* refs), right? But the cost is not more than a factor of 2 worse than if the ref had been packed (since you have to repack_without_refs anyway) so I think this is acceptable.

[...]
> 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.

This also seems reasonable --- there's no need to be able to roll back into an invalid state. :)

[...]
Show 9 quoted lines
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -756,6 +756,8 @@ static int remove_branches(struct string_list *branches)
>  	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
>  	for (i = 0; i < branches->nr; i++)
>  		branch_names[i] = branches->items[i].string;
> +	if (lock_packed_refs(0))
> +		result |= unable_to_lock_error(git_path("packed-refs"), errno);
>  	result |= repack_without_refs(branch_names, branches->nr, NULL);

What should happen if lock_packed_refs fails? Since repack_without_refs is now documented to require a locked packed-refs file, shouldn't the repack_without_refs call be guarded, like it is below?

[...]
Show 14 quoted lines
> @@ -1333,9 +1335,14 @@ static int prune_remote(const char *remote, int dry_run)
>  		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)
> -			result |= repack_without_refs(delete_refs,
> -						      states.stale.nr, NULL);
> +		if (!dry_run) {
> +			if (lock_packed_refs(0))
> +				result |= unable_to_lock_error(
> +					git_path("packed-refs"), errno);
> +			else
> +				result |= repack_without_refs(delete_refs,
> +							states.stale.nr, NULL);
[...]
Show 8 quoted lines
> --- a/refs.c
> +++ b/refs.c
> @@ -1330,7 +1330,7 @@ static struct ref_entry *get_packed_ref(const char *refname)
>  
>  /*
>   * A loose ref file doesn't exist; check for a packed ref.  The
> - * options are forwarded from resolve_safe_unsafe().
> + * options are forwarded from resolve_ref_unsafe().

Unrelated changes snuck in? (A good change, but it presumably belongs as a separate patch.)

[...]
Show 6 quoted lines
> @@ -1387,7 +1387,6 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int fla
>  		}
>  
>  		git_snpath(path, sizeof(path), "%s", refname);
> -
>  		/*
Why?
[...]
Show 8 quoted lines
> @@ -2532,6 +2531,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)

Aren't packed refs always sorted? If it needs mentioning, that seems more like an implementation comment.

The 'packed refs must be locked' kind of documentation belongs in refs.h since this is part of the API. Usually when there are API changes like this we like to change the function name or signature to make it easy to catch callers that were introduced without noticing the change --- would that be easy to do here?

E.g. introducing a new repack_without_refs_locked (name is just a placeholder, I'm bad at names) and keeping repack_without_refs with the current semantics as a wrapper around that.

[...]
Show 6 quoted lines
> @@ -2544,19 +2546,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 */
> +	/* Avoid processing if we have nothing to do */

This implies a small speed regression but I don't think anyone would mind.

[...]
Show 7 quoted lines
> @@ -3692,12 +3689,65 @@ int transaction_commit(struct ref_transaction *transaction,
>  		goto cleanup;
>  	}
>  
> -	/* Acquire all ref locks while verifying old values */
> +	/* Lock packed refs during commit */
> +	if (lock_packed_refs(0)) {
I stopped reviewing here but assume the rest is okay. :)

Thanks, Jonathan

Ronnie Sahlberg· Jul 25, 2014, 16:58 UTC · re: Ronnie Sahlberg · lore

[PATCH 4/5] refs.c: update rename_ref to use a transaction

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.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c            | 192 ++++++++++++++++++------------------------------------
 t/t3200-branch.sh |   7 --
 2 files changed, 65 insertions(+), 134 deletions(-)
Show changes to 2 files +65 −134

refs.c, t/t3200-branch.sh

diff --git a/refs.c b/refs.c
index 0d800f1..a5053bf 100644
--- a/refs.c
+++ b/refs.c
@@ -2616,82 +2616,43 @@ 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 ref_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 *email, unsigned long timestamp, int tz,
+			     const char *message, void *cb_data)
 {
-	int attempts_remaining = 4;
-
- 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;
-	}
+	struct rename_reflog_cb *cb = cb_data;
 
-	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;
+	return transaction_update_reflog(cb->transaction, cb->refname,
+					 nsha1, osha1, email, timestamp, tz,
+					 message, 0, cb->err);
 }
 
-static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
-			  const char *logmsg);
-
 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 ref_transaction *transaction = NULL;
+	struct strbuf err = STRBUF_INIT;
 	const char *symref = NULL;
+	struct rename_reflog_cb cb;
 
-	if (log && S_ISLNK(loginfo.st_mode))
-		return error("reflog for %s is a symlink", oldrefname);
-
-	symref = resolve_ref_unsafe(oldrefname, orig_sha1,
+	symref = resolve_ref_unsafe(oldrefname, sha1,
 				    RESOLVE_REF_READING, &flag);
-	if (flag & REF_ISSYMREF)
-		return error("refname %s is a symbolic ref, renaming it is not supported",
+	if (flag & REF_ISSYMREF) {
+		error("refname %s is a symbolic ref, renaming it is not supported",
 			oldrefname);
-	if (!symref)
-		return error("refname %s not found", oldrefname);
+		return 1;
+	}
+	if (!symref) {
+		error("refname %s not found", oldrefname);
+		return 1;
+	}
 
 	if (!is_refname_available(newrefname, get_packed_refs(&ref_cache),
 				  &oldrefname, 1))
@@ -2701,70 +2662,47 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 				  &oldrefname, 1))
 		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, sha1, RESOLVE_REF_READING, 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 (log && rename_tmp_log(newrefname))
-		goto rollback;
-
-	logmoved = log;
-
-	lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0);
-	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,
+						     git_committer_info(0),
+						     0, 0, 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_sha1(transaction, oldrefname, sha1,
+					    REF_NODEREF,
+					    1, NULL, &err))
+			goto fail;
+	}
+	if (transaction_update_sha1(transaction, newrefname, sha1,
+				    NULL, 0, 0, NULL, &err))
+		goto fail;
+	if (log && transaction_update_reflog(transaction, newrefname, sha1,
+					     sha1, git_committer_info(0),
+					     0, 0, logmsg,
+					     REFLOG_EMAIL_IS_COMMITTER, &err))
+		goto fail;
+	if (transaction_commit(transaction, &err))
+		goto fail;
+	transaction_free(transaction);
 	return 0;
 
- rollback:
-	lock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL, NULL, 0);
-	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 ac31b71..64f5bf2 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -293,13 +293,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/* &&

-- 
2.0.1.508.g763ab16
Jonathan Nieder· Jul 25, 2014, 20:00 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 4/5] refs.c: update rename_ref to use a transaction

Ronnie Sahlberg wrote:
> Change refs.c to use a single transaction to copy/rename both the refs and
> its reflog.
Yay!
>             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.

It's generally better to update the testsuite with the new expected behavior instead. :)

Ronnie Sahlberg· Jul 25, 2014, 16:58 UTC · re: Ronnie Sahlberg · lore

[PATCH 5/5] refs.c: rollback the lockfile before we die() in repack_without_refs

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to refs.c +3 −1
diff --git a/refs.c b/refs.c
index a5053bf..619725a 100644
--- a/refs.c
+++ b/refs.c
@@ -2570,8 +2570,10 @@ int repack_without_refs(const char **refnames, int n, 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 */

-- 
2.0.1.508.g763ab16

← back to recent threads