[PATCH 0/5] use packed refs for ref-transactions

[PATCH 0/5] use packed refs for ref-transactions

[Ronnie Sahlberg]

This is a small patch series that continues ontop of the 
ref-transactions-rename series I posted earlier today.

This series expands the use of the packed refs file to make multi-ref
updates much more atomic. Additionally it allows us to continue pushing
external caller to use transactions instead of accessing (packed) refs
directly which allows us to remove a whole bunch of refs functions
from the public api.

After this series there is no real need any more to expose that packed
refs even exist to external callers.


This series is built ontop of the previous series that was posted earlier
today as :
  [PATCH 0/5] ref-transactions-rename


Ronnie Sahlberg (5):
  refs.c: move reflog updates into its own function
  refs.c: write updates to packed refs when a transaction has more than
    one ref
  remote.c: use a transaction for deleting refs
  refs.c: make repack_without_refs static
  refs.c: make the *_packed_refs functions static

 builtin/clone.c       |  16 ++++--
 builtin/remote.c      |  70 +++++++++++++----------
 refs.c                | 153 +++++++++++++++++++++++++++++++++-----------------
 refs.h                |  33 -----------
 t/t5516-fetch-push.sh |   2 +-
 5 files changed, 153 insertions(+), 121 deletions(-)

-- 
2.0.1.518.g4f5a8ad

[PATCH 1/5] refs.c: move reflog updates into its own function

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

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 62 ++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 36 insertions(+), 26 deletions(-)

diff --git a/refs.c b/refs.c
index 619725a..1048017 100644
--- a/refs.c
+++ b/refs.c
@@ -2869,6 +2869,39 @@ static 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)) {
+		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", head_sha1,
+					      RESOLVE_REF_READING, &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;
+}
+
 /*
  * Writes sha1 into the ref specified by the lock. Makes sure that errno
  * is sane on error.
@@ -2912,34 +2945,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", head_sha1,
-					      RESOLVE_REF_READING, &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);
@@ -3649,7 +3658,8 @@ int transaction_commit(struct ref_transaction *transaction,
 			continue;
 		if (get_packed_ref(update->refname))
 			continue;
-		if (!resolve_ref_unsafe(update->refname, sha1, 1, NULL))
+		if (!resolve_ref_unsafe(update->refname, sha1,
+					RESOLVE_REF_READING, NULL))
 			continue;
 
 		add_packed_ref(update->refname, sha1);
-- 
2.0.1.518.g4f5a8ad

[PATCH 2/5] refs.c: write updates to packed refs when a transaction has more than one ref

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

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/clone.c       | 16 +++++++---
 refs.c                | 83 +++++++++++++++++++++++++++++++++++++--------------
 t/t5516-fetch-push.sh |  2 +-
 3 files changed, 74 insertions(+), 27 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f7307e6..7737e12 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -497,17 +497,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 ref_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_sha1(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 1048017..efa4f0d 100644
--- a/refs.c
+++ b/refs.c
@@ -2539,33 +2539,18 @@ int repack_without_refs(const char **refnames, int n, 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 i, ret;
 
 	/* Look for a packed ref */
 	for (i = 0; i < n; i++)
 		if (get_packed_ref(refnames[i]))
 			break;
 
-	/* Avoid processing if we have nothing to do */
-	if (i == n) {
-		rollback_packed_refs();
-		return 0; /* no refname exists in packed refs */
-	}
-
 	packed = get_packed_refs(&ref_cache);
 
 	/* Remove refnames from the cache */
 	for (i = 0; i < n; i++)
-		if (remove_entry(packed, refnames[i]) != -1)
-			removed = 1;
-	if (!removed) {
-		/*
-		 * All packed entries disappeared while we were
-		 * acquiring the lock.
-		 */
-		rollback_packed_refs();
-		return 0;
-	}
+		remove_entry(packed, refnames[i]);
 
 	/* Remove any other accumulated cruft */
 	do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);
@@ -3614,6 +3599,7 @@ int transaction_commit(struct ref_transaction *transaction,
 		       struct strbuf *err)
 {
 	int ret = 0, delnum = 0, i, df_conflict = 0, need_repack = 0;
+	int num_updates = 0;
 	const char **delnames;
 	int n = transaction->nr;
 	struct packed_ref_cache *packed_ref_cache;
@@ -3647,19 +3633,38 @@ int transaction_commit(struct ref_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 non symref refs
+	 * to the packed refs too.
+	 */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 		unsigned char sha1[20];
+		int flag;
 
 		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;
 		if (!resolve_ref_unsafe(update->refname, sha1,
-					RESOLVE_REF_READING, NULL))
+					RESOLVE_REF_READING, &flag))
+			continue;
+		if (flag & REF_ISSYMREF)
 			continue;
 
 		add_packed_ref(update->refname, sha1);
@@ -3683,11 +3688,14 @@ int transaction_commit(struct ref_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
 	 * safely delete these loose refs.
+	 * If we are updating multiple normal refs, then those will also be in
+	 * the packed refs file so we can delete them too.
 	 */
 
 	/* Unlink any loose refs scheduled for deletion */
@@ -3725,7 +3733,10 @@ int transaction_commit(struct ref_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];
 
@@ -3749,6 +3760,30 @@ int transaction_commit(struct ref_transaction *transaction,
 			ret = -1;
 			goto cleanup;
 		}
+		if (num_updates < 2 || update->type & REF_ISSYMREF)
+			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 */
@@ -3797,7 +3832,7 @@ int transaction_commit(struct ref_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)) {
 			ret = write_ref_sha1(update->lock, update->new_sha1,
 					     update->msg);
 			update->lock = NULL; /* freed by write_ref_sha1 */
@@ -3862,6 +3897,10 @@ int transaction_commit(struct ref_transaction *transaction,
 		}
 	}
 
+	if (need_repack) {
+		packed = get_packed_refs(&ref_cache);
+		sort_ref_dir(packed);
+	}
 	if (repack_without_refs(delnames, delnum, err))
 		ret = -1;
 	clear_loose_ref_cache(&ref_cache);
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 67e0ab3..23978de 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 &&
-- 
2.0.1.518.g4f5a8ad

[PATCH 3/5] remote.c: use a transaction for deleting refs

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

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/remote.c | 70 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 31 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 9a9cc92..3e6ef08 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,25 +750,27 @@ static int mv(int argc, const char **argv)
 
 static int remove_branches(struct string_list *branches)
 {
-	const char **branch_names;
 	int i, result = 0;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
+
+	transaction = transaction_begin(&err);
+	if (!transaction)
+		die("%s", err.buf);
 
-	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);
-
-	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);
-	}
+		if (transaction_delete_sha1(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);
 
+ cleanup:
+	strbuf_release(&err);
+	transaction_free(transaction);
 	return result;
 }
 
@@ -1317,10 +1319,11 @@ 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!");
+	struct ref_transaction *transaction = NULL;
+	struct strbuf err = STRBUF_INIT;
 
 	memset(&states, 0, sizeof(states));
 	get_remote_ref_states(remote, &states, GET_REF_STATES);
@@ -1331,28 +1334,26 @@ static int prune_remote(const char *remote, int dry_run)
 		       states.remote->url_nr
 		       ? 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) {
-			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);
 	}
 
+	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;
 
 		string_list_insert(&delete_refs_list, refname);
 
-		if (!dry_run)
-			result |= delete_ref(refname, NULL, 0);
+		if (!dry_run) {
+			if (transaction_delete_sha1(transaction,
+					    refname, NULL,
+					    0, 0, "remote-branches", &err)) {
+				result |= error("%s", err.buf);
+				goto cleanup;
+			}
+		}
 
 		if (dry_run)
 			printf_ln(_(" * [would prune] %s"),
@@ -1362,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);
 
-- 
2.0.1.518.g4f5a8ad

[PATCH 4/5] refs.c: make repack_without_refs static

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <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 efa4f0d..5696d18 100644
--- a/refs.c
+++ b/refs.c
@@ -2534,7 +2534,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(const char **refnames, int n, struct strbuf *err)
+static int repack_without_refs(const char **refnames, int n, 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 eb918a0..7183e8e 100644
--- a/refs.h
+++ b/refs.h
@@ -155,9 +155,6 @@ extern void rollback_packed_refs(void);
  */
 int pack_refs(unsigned int flags);
 
-extern int repack_without_refs(const char **refnames, int n,
-			       struct strbuf *err);
-
 extern int ref_exists(const char *);
 
 /*
-- 
2.0.1.518.g4f5a8ad

[PATCH 5/5] refs.c: make the *_packed_refs functions static

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

Signed-off-by: Ronnie Sahlberg <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 5696d18..d2ee8a2 100644
--- a/refs.c
+++ b/refs.c
@@ -1135,7 +1135,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);
@@ -2268,7 +2268,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;
 
@@ -2291,7 +2291,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);
@@ -2316,7 +2316,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 7183e8e..ec52e13 100644
--- a/refs.h
+++ b/refs.h
@@ -112,36 +112,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
-- 
2.0.1.518.g4f5a8ad

Re: [PATCH 2/5] refs.c: write updates to packed refs when a transaction has more than one ref

[Junio C Hamano]

Ronnie Sahlberg <sahlberg@google.com> writes:

> +	/*
> +	 * Always copy loose refs that are to be deleted to the packed refs.
> +	 * If we are updating multiple refs then copy all non symref refs
> +	 * to the packed refs too.
> +	 */
>  	for (i = 0; i < n; i++) {
>  		struct ref_update *update = updates[i];
>  		unsigned char sha1[20];
> +		int flag;
>  
>  		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;
>  		if (!resolve_ref_unsafe(update->refname, sha1,
> -					RESOLVE_REF_READING, NULL))
> +					RESOLVE_REF_READING, &flag))
> +			continue;
> +		if (flag & REF_ISSYMREF)
>  			continue;

This skipping of symref didn't use to be here.

Is this an enhancement needed to implement the new "feature"
(i.e. use packed refs to represent multi-update as an atomic
operation)?  It looks to me more like an unrelated "oops, forgot
that we cannot use packed refs to store symrefs" fix-up, unless no
caller passed symref in updates[] in the original code, and now we
have callers that do.

Puzzled...

Re: [PATCH 2/5] refs.c: write updates to packed refs when a transaction has more than one ref

[Junio C Hamano]

Ronnie Sahlberg <sahlberg@google.com> writes:

> +		packed = get_packed_refs(&ref_cache);;

s/;;/;/; ;-)

Sorry, I couldn't resist the urge to type many semicolons ;-)

Re: [PATCH 2/5] refs.c: write updates to packed refs when a transaction has more than one ref

[Ronnie Sahlberg]

On Tue, Jul 29, 2014 at 2:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> +     /*
>> +      * Always copy loose refs that are to be deleted to the packed refs.
>> +      * If we are updating multiple refs then copy all non symref refs
>> +      * to the packed refs too.
>> +      */
>>       for (i = 0; i < n; i++) {
>>               struct ref_update *update = updates[i];
>>               unsigned char sha1[20];
>> +             int flag;
>>
>>               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;
>>               if (!resolve_ref_unsafe(update->refname, sha1,
>> -                                     RESOLVE_REF_READING, NULL))
>> +                                     RESOLVE_REF_READING, &flag))
>> +                     continue;
>> +             if (flag & REF_ISSYMREF)
>>                       continue;
>
> This skipping of symref didn't use to be here.
>
> Is this an enhancement needed to implement the new "feature"
> (i.e. use packed refs to represent multi-update as an atomic
> operation)?  It looks to me more like an unrelated "oops, forgot
> that we cannot use packed refs to store symrefs" fix-up, unless no
> caller passed symref in updates[] in the original code, and now we
> have callers that do.
>
> Puzzled...

It was mainly as an enhancement.
Prior to this patch we were only using the packed-refs trick for the
delete+rename operation part of a rename_ref() call.
And for that case we already explicitly test for and disallow symbolic
refs already in rename_ref().

I added this just for extra safety for "now that we possibly delete
multiple refs in one transaction, should I special case/disallow the
packed refs trick for symrefs?"
Looking at it again I don't think we need this special casing and it
only makes the code more complex and confusing.
I will review the use of this a little and run some tests and if all
looks sane I will remove this ISSYMREF special casing.

Thank!
ronnie sahlberg

Re: [PATCH 2/5] refs.c: write updates to packed refs when a transaction has more than one ref

[Ronnie Sahlberg]

On Tue, Jul 29, 2014 at 2:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> +             packed = get_packed_refs(&ref_cache);;
>
> s/;;/;/; ;-)
>
> Sorry, I couldn't resist the urge to type many semicolons ;-)

Fixed, thanks!