threads / patch / 29152

patch, 6 partst5519: push two branches to alternate repo

Subject: [PATCH 1/6] t5519: push two branches to alternate repo

## tl;dr

7 messages between Dec 13, 2011 and Dec 13, 2011. Diffs are folded; open one to read it.

replies: 6people: 1as markdown or json

mhagger@alum.mit.edu· Dec 13, 2011, 20:06 UTC · lore

[PATCH 0/6] Handle extra_refs separately from ref_caches

From: Michael Haggerty <mhagger@alum.mit.edu>

Extra refs don't have much to do with real references, and in fact they have to be handled differently. For example, they do not support flags, they might not have unique names (indeed, the names are rather meaningless), and they are only ever iterated over, never looked up. So seemingly innocent things that one might want to do with real references, like check for conflicting duplicates, must not be done for extra refs.

This patch series creates a new linked-list data structure for the extra refs, separates iteration over the extra refs into a new function, and changes a test to actually create multiple extra refs with the same name.

This patch series applies on top of master. If this approach is selected, then the ref-api-D series will have to be rebased on top of it and touched up to avoid the problems that it has with duplicate extra refs.

By the way, I have been carrying around the CC list of this email for quite a while. If you are tired of being spammed with my patch series, send me a private email and I will be happy to remove you from future mailings.

Michael Haggerty (6):
  t5519: push two branches to alternate repo
  add_extra_ref(): remove flag argument
  Extract a function do_for_each_extra_ref()
  Store extra_refs in a separate data structure
  Omit extra_refs except when iterating using for_each_ref()
  do_for_each_extra_ref(): simplify signature
 builtin/clone.c            |    4 ++--
 builtin/receive-pack.c     |    2 +-
 refs.c                     |   44 ++++++++++++++++++++++++++++++++++----------
 refs.h                     |    2 +-
 t/t5519-push-alternates.sh |   10 +++++++++-
 5 files changed, 47 insertions(+), 15 deletions(-)
-- 
1.7.8
mhagger@alum.mit.edu· Dec 13, 2011, 20:06 UTC · re: mhagger@alum.mit.edu · lore
From: Michael Haggerty <mhagger@alum.mit.edu>

Since each branch in the alternate repo results in an "extra_ref" named ".have", pushing two of them results in two extra_refs with the same name. This change to the test therefore makes sure that we can handle extra_refs names that are not unique.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

I'm not sure how well this change fits into the other things that the test wants to do, but it triggers the failure mode in ref-api-D v2 that was predicted by Junio.

 t/t5519-push-alternates.sh |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)
Show changes to t/t5519-push-alternates.sh +9 −1
diff --git a/t/t5519-push-alternates.sh b/t/t5519-push-alternates.sh
index c00c9b0..315f65d 100755
--- a/t/t5519-push-alternates.sh
+++ b/t/t5519-push-alternates.sh
@@ -17,7 +17,15 @@ test_expect_success setup '
 		>file &&
 		git add . &&
 		git commit -m initial &&
-		git push ../alice-pub master
+		git checkout -b foo &&
+		>file1 &&
+		git add . &&
+		git commit -m file1 &&
+		git checkout master &&
+		>file2 &&
+		git add . &&
+		git commit -m file2 &&
+		git push ../alice-pub master foo
 	) &&
 
 	# Project Bob is a fork of project Alice

-- 
1.7.8
mhagger@alum.mit.edu· Dec 13, 2011, 20:06 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 2/6] add_extra_ref(): remove flag argument

From: Michael Haggerty <mhagger@alum.mit.edu>

The argument was always set to 0 (and other values do not make sense) so remove the argument.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/clone.c        |    4 ++--
 builtin/receive-pack.c |    2 +-
 refs.c                 |    4 ++--
 refs.h                 |    2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)
Show changes to 4 files +6 −6

builtin/clone.c, builtin/receive-pack.c, refs.c, refs.h

diff --git a/builtin/clone.c b/builtin/clone.c
index efe8b6c..5035767 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -252,7 +252,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 	transport = transport_get(remote, ref_git);
 	for (extra = transport_get_remote_refs(transport); extra;
 	     extra = extra->next)
-		add_extra_ref(extra->name, extra->old_sha1, 0);
+		add_extra_ref(extra->name, extra->old_sha1);
 
 	transport_disconnect(transport);
 	free(ref_git);
@@ -441,7 +441,7 @@ static void write_remote_refs(const struct ref *local_refs)
 	for (r = local_refs; r; r = r->next) {
 		if (!r->peer_ref)
 			continue;
-		add_extra_ref(r->peer_ref->name, r->old_sha1, 0);
+		add_extra_ref(r->peer_ref->name, r->old_sha1);
 	}
 
 	pack_refs(PACK_REFS_ALL);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b6d957c..e3b46ce 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -872,7 +872,7 @@ static int delete_only(struct command *commands)
 
 static void add_one_alternate_sha1(const unsigned char sha1[20], void *unused)
 {
-	add_extra_ref(".have", sha1, 0);
+	add_extra_ref(".have", sha1);
 }
 
 static void collect_one_alternate_ref(const struct ref *ref, void *data)
diff --git a/refs.c b/refs.c
index f5cb297..6115487 100644
--- a/refs.c
+++ b/refs.c
@@ -248,9 +248,9 @@ static void read_packed_refs(FILE *f, struct ref_array *array)
 	sort_ref_array(array);
 }
 
-void add_extra_ref(const char *name, const unsigned char *sha1, int flag)
+void add_extra_ref(const char *name, const unsigned char *sha1)
 {
-	add_ref(name, sha1, flag, 0, &extra_refs, NULL);
+	add_ref(name, sha1, 0, 0, &extra_refs, NULL);
 }
 
 void clear_extra_refs(void)
diff --git a/refs.h b/refs.h
index 3fd5536..39bb289 100644
--- a/refs.h
+++ b/refs.h
@@ -56,7 +56,7 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn
  * called. Only extra refs added before for_each_ref() is called will
  * be listed on a given call of for_each_ref().
  */
-extern void add_extra_ref(const char *refname, const unsigned char *sha1, int flags);
+extern void add_extra_ref(const char *refname, const unsigned char *sha1);
 extern void clear_extra_refs(void);
 extern int ref_exists(const char *);
 

-- 
1.7.8
mhagger@alum.mit.edu· Dec 13, 2011, 20:06 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 3/6] Extract a function do_for_each_extra_ref()

From: Michael Haggerty <mhagger@alum.mit.edu>
We want to hold the extra refs at arms length.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)
Show changes to refs.c +16 −5
diff --git a/refs.c b/refs.c
index 6115487..268bda9 100644
--- a/refs.c
+++ b/refs.c
@@ -693,17 +693,28 @@ fallback:
 	return -1;
 }
 
+static int do_for_each_extra_ref(const char *base, each_ref_fn fn,
+				 int trim, int flags, void *cb_data)
+{
+	int i;
+	for (i = 0; i < extra_refs.nr; i++) {
+		int retval = do_one_ref(base, fn, trim, flags, cb_data, extra_refs.refs[i]);
+		if (retval)
+			return retval;
+	}
+	return 0;
+}
+
 static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,
 			   int trim, int flags, void *cb_data)
 {
-	int retval = 0, i, p = 0, l = 0;
+	int retval = 0, p = 0, l = 0;
 	struct ref_array *packed = get_packed_refs(submodule);
 	struct ref_array *loose = get_loose_refs(submodule);
 
-	struct ref_array *extra = &extra_refs;
-
-	for (i = 0; i < extra->nr; i++)
-		retval = do_one_ref(base, fn, trim, flags, cb_data, extra->refs[i]);
+	retval = do_for_each_extra_ref(base, fn, trim, flags, cb_data);
+	if (retval)
+		goto end_each;
 
 	while (p < packed->nr && l < loose->nr) {
 		struct ref_entry *entry;

-- 
1.7.8
mhagger@alum.mit.edu· Dec 13, 2011, 20:06 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 4/6] Store extra_refs in a separate data structure

From: Michael Haggerty <mhagger@alum.mit.edu>

Extra references really don't have much to do with real references, and we want to change how real references are handled. So hold extra references in a separate data structure.

Since extra references cannot be broken, don't store flags with them and don't pass the flags argument (a different "flags"!) to do_for_each_extra_ref().

Finally, current_ref is not useful while iterating through the extra_refs. In fact, peel_ref() doesn't even work for extra refs because they cannot be peeled themselves, and moreover they might coincidentally have the same name as a "real" reference. So explicitly set current_ref to NULL while iterating over extra_refs to avoid confusion.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

Strictly speaking, this patch changes how an extra reference with an invalid SHA1 would be treated. In the old code, it would be skipped over (assuming DO_FOR_EACH_INCLUDE_BROKEN is not set). In the new code the SHA1 is not checked. However, from my understanding of how extra_refs are used, they should never have invalid SHA1s and so the old test is superfluous.

 refs.c |   38 ++++++++++++++++++++++++++++----------
 1 files changed, 28 insertions(+), 10 deletions(-)
Show changes to refs.c +28 −10
diff --git a/refs.c b/refs.c
index 268bda9..843c530 100644
--- a/refs.c
+++ b/refs.c
@@ -146,7 +146,11 @@ static struct ref_cache {
 
 static struct ref_entry *current_ref;
 
-static struct ref_array extra_refs;
+static struct extra_ref {
+	struct extra_ref *next;
+	unsigned char sha1[20];
+	char name[FLEX_ARRAY];
+} *extra_refs;
 
 static void free_ref_array(struct ref_array *array)
 {
@@ -250,12 +254,21 @@ static void read_packed_refs(FILE *f, struct ref_array *array)
 
 void add_extra_ref(const char *name, const unsigned char *sha1)
 {
-	add_ref(name, sha1, 0, 0, &extra_refs, NULL);
+	int len = strlen(name) + 1;
+	struct extra_ref *entry = xmalloc(sizeof(struct extra_ref) + len);
+	hashcpy(entry->sha1, sha1);
+	memcpy(entry->name, name, len);
+	entry->next = extra_refs;
+	extra_refs = entry;
 }
 
 void clear_extra_refs(void)
 {
-	free_ref_array(&extra_refs);
+	while (extra_refs) {
+		struct extra_ref *entry = extra_refs;
+		extra_refs = entry->next;
+		free(entry);
+	}
 }
 
 static struct ref_array *get_packed_refs(const char *submodule)
@@ -694,13 +707,18 @@ fallback:
 }
 
 static int do_for_each_extra_ref(const char *base, each_ref_fn fn,
-				 int trim, int flags, void *cb_data)
+				 int trim, void *cb_data)
 {
-	int i;
-	for (i = 0; i < extra_refs.nr; i++) {
-		int retval = do_one_ref(base, fn, trim, flags, cb_data, extra_refs.refs[i]);
-		if (retval)
-			return retval;
+	struct extra_ref *extra;
+
+	current_ref = NULL;
+	for (extra = extra_refs; extra; extra = extra->next) {
+		if (!prefixcmp(extra->name, base)) {
+			int retval = fn(extra->name + trim, extra->sha1,
+					0, cb_data);
+			if (retval)
+				return retval;
+		}
 	}
 	return 0;
 }
@@ -712,7 +730,7 @@ static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn
 	struct ref_array *packed = get_packed_refs(submodule);
 	struct ref_array *loose = get_loose_refs(submodule);
 
-	retval = do_for_each_extra_ref(base, fn, trim, flags, cb_data);
+	retval = do_for_each_extra_ref(base, fn, trim, cb_data);
 	if (retval)
 		goto end_each;
 

-- 
1.7.8
mhagger@alum.mit.edu· Dec 13, 2011, 20:06 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 5/6] Omit extra_refs except when iterating using for_each_ref()

From: Michael Haggerty <mhagger@alum.mit.edu>

According to Junio, the only reference iteration function that needs to include the extra refs is for_each_ref(). So call do_for_each_extra_ref() directly from there instead of from do_for_each_ref().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)
Show changes to refs.c +3 −4
diff --git a/refs.c b/refs.c
index 843c530..f52d8b5 100644
--- a/refs.c
+++ b/refs.c
@@ -730,10 +730,6 @@ static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn
 	struct ref_array *packed = get_packed_refs(submodule);
 	struct ref_array *loose = get_loose_refs(submodule);
 
-	retval = do_for_each_extra_ref(base, fn, trim, cb_data);
-	if (retval)
-		goto end_each;
-
 	while (p < packed->nr && l < loose->nr) {
 		struct ref_entry *entry;
 		int cmp = strcmp(packed->refs[p]->name, loose->refs[l]->name);
@@ -798,6 +794,9 @@ int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
 
 int for_each_ref(each_ref_fn fn, void *cb_data)
 {
+	int retval = do_for_each_extra_ref("", fn, 0, cb_data);
+	if (retval)
+		return retval;
 	return do_for_each_ref(NULL, "", fn, 0, 0, cb_data);
 }
 

-- 
1.7.8
mhagger@alum.mit.edu· Dec 13, 2011, 20:06 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 6/6] do_for_each_extra_ref(): simplify signature

From: Michael Haggerty <mhagger@alum.mit.edu>

Now that do_for_each_extra_ref() is only called from one place, it can be less general.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)
Show changes to refs.c +5 −9
diff --git a/refs.c b/refs.c
index f52d8b5..4b2ba3f 100644
--- a/refs.c
+++ b/refs.c
@@ -706,19 +706,15 @@ fallback:
 	return -1;
 }
 
-static int do_for_each_extra_ref(const char *base, each_ref_fn fn,
-				 int trim, void *cb_data)
+static int do_for_each_extra_ref(each_ref_fn fn, void *cb_data)
 {
 	struct extra_ref *extra;
 
 	current_ref = NULL;
 	for (extra = extra_refs; extra; extra = extra->next) {
-		if (!prefixcmp(extra->name, base)) {
-			int retval = fn(extra->name + trim, extra->sha1,
-					0, cb_data);
-			if (retval)
-				return retval;
-		}
+		int retval = fn(extra->name, extra->sha1, 0, cb_data);
+		if (retval)
+			return retval;
 	}
 	return 0;
 }
@@ -794,7 +790,7 @@ int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
 
 int for_each_ref(each_ref_fn fn, void *cb_data)
 {
-	int retval = do_for_each_extra_ref("", fn, 0, cb_data);
+	int retval = do_for_each_extra_ref(fn, cb_data);
 	if (retval)
 		return retval;
 	return do_for_each_ref(NULL, "", fn, 0, 0, cb_data);

-- 
1.7.8

← back to recent threads