[PATCH 0/3] Eliminate one user of extra_refs

[PATCH 0/3] Eliminate one user of extra_refs

[mhagger]

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

Receive pack currently uses "extra refs" to keep track of ".have"
references, which in turn are used to tell the source the SHA1s of
references that are already known to the repository via alternates.

But the code already creates an array holding the alternate SHA1s.  So
just read the SHA1s out of this array rather then round-tripping them
through the extra_refs mechanism.

This is one step towards hopefully abolishing extra_refs altogether.
I still have to examine the other user.

Michael Haggerty (3):
  receive-pack: move more work into write_head_info()
  show_ref(): remove unused "flag" and "cb_data" arguments
  write_head_info(): handle "extra refs" locally

 builtin/receive-pack.c |   51 ++++++++++++++++++++---------------------------
 1 files changed, 22 insertions(+), 29 deletions(-)

-- 
1.7.8.2

[PATCH 1/3] receive-pack: move more work into write_head_info()

[mhagger]

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

Move some more code from the calling site into write_head_info(), and
inline add_alternate_refs() there.  (Some more simplification is
coming, and it is easier if all this code is in the same place.)

Move some helper functions to avoid the need for forward declarations.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/receive-pack.c |   42 ++++++++++++++++++------------------------
 1 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d2dcb7e..08df17e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -144,12 +144,30 @@ static int show_ref_cb(const char *path, const unsigned char *sha1, int flag, vo
 	return show_ref(path, sha1, flag, cb_data);
 }
 
+static void add_one_alternate_sha1(const unsigned char sha1[20], void *unused)
+{
+	add_extra_ref(".have", sha1, 0);
+}
+
+static void collect_one_alternate_ref(const struct ref *ref, void *data)
+{
+	struct sha1_array *sa = data;
+	sha1_array_append(sa, ref->old_sha1);
+}
+
 static void write_head_info(void)
 {
+	struct sha1_array sa = SHA1_ARRAY_INIT;
+	for_each_alternate_ref(collect_one_alternate_ref, &sa);
+	sha1_array_for_each_unique(&sa, add_one_alternate_sha1, NULL);
+	sha1_array_clear(&sa);
 	for_each_ref(show_ref_cb, NULL);
 	if (!sent_capabilities)
 		show_ref("capabilities^{}", null_sha1, 0, NULL);
+	clear_extra_refs();
 
+	/* EOF */
+	packet_flush(1);
 }
 
 struct command {
@@ -869,25 +887,6 @@ static int delete_only(struct command *commands)
 	return 1;
 }
 
-static void add_one_alternate_sha1(const unsigned char sha1[20], void *unused)
-{
-	add_extra_ref(".have", sha1, 0);
-}
-
-static void collect_one_alternate_ref(const struct ref *ref, void *data)
-{
-	struct sha1_array *sa = data;
-	sha1_array_append(sa, ref->old_sha1);
-}
-
-static void add_alternate_refs(void)
-{
-	struct sha1_array sa = SHA1_ARRAY_INIT;
-	for_each_alternate_ref(collect_one_alternate_ref, &sa);
-	sha1_array_for_each_unique(&sa, add_one_alternate_sha1, NULL);
-	sha1_array_clear(&sa);
-}
-
 int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 {
 	int advertise_refs = 0;
@@ -937,12 +936,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		unpack_limit = receive_unpack_limit;
 
 	if (advertise_refs || !stateless_rpc) {
-		add_alternate_refs();
 		write_head_info();
-		clear_extra_refs();
-
-		/* EOF */
-		packet_flush(1);
 	}
 	if (advertise_refs)
 		return 0;
-- 
1.7.8.2

[PATCH 2/3] show_ref(): remove unused "flag" and "cb_data" arguments

[mhagger]

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

The function is not used as a callback, so it doesn't need these
arguments.  Also change its return type to void.

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

I suppose, though I didn't verify, that the old function signature was
a vestige of its earlier having been used as a callback function.  But
it doesn't really matter; the point is that the extra arguments are
currently not needed.

 builtin/receive-pack.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 08df17e..65d129c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -115,7 +115,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-static int show_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)
+static void show_ref(const char *path, const unsigned char *sha1)
 {
 	if (sent_capabilities)
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
@@ -125,10 +125,9 @@ static int show_ref(const char *path, const unsigned char *sha1, int flag, void
 			     " report-status delete-refs side-band-64k",
 			     prefer_ofs_delta ? " ofs-delta" : "");
 	sent_capabilities = 1;
-	return 0;
 }
 
-static int show_ref_cb(const char *path, const unsigned char *sha1, int flag, void *cb_data)
+static int show_ref_cb(const char *path, const unsigned char *sha1, int flag, void *unused)
 {
 	path = strip_namespace(path);
 	/*
@@ -141,7 +140,8 @@ static int show_ref_cb(const char *path, const unsigned char *sha1, int flag, vo
 	 */
 	if (!path)
 		path = ".have";
-	return show_ref(path, sha1, flag, cb_data);
+	show_ref(path, sha1);
+	return 0;
 }
 
 static void add_one_alternate_sha1(const unsigned char sha1[20], void *unused)
@@ -163,7 +163,7 @@ static void write_head_info(void)
 	sha1_array_clear(&sa);
 	for_each_ref(show_ref_cb, NULL);
 	if (!sent_capabilities)
-		show_ref("capabilities^{}", null_sha1, 0, NULL);
+		show_ref("capabilities^{}", null_sha1);
 	clear_extra_refs();
 
 	/* EOF */
-- 
1.7.8.2

Re: [PATCH 2/3] show_ref(): remove unused "flag" and "cb_data" arguments

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> I suppose, though I didn't verify, that the old function signature was
> a vestige of its earlier having been used as a callback function.  But
> it doesn't really matter; the point is that the extra arguments are
> currently not needed.

Yeah, since 8a65ff7 (Generalize the "show each ref" code in receice-pack,
2005-07-02) the function has always been fed to for_each_ref(), but when
6b01ecf (ref namespaces: Support remote repositories via upload-pack and
receive-pack, 2011-07-08) introduced show_ref_cb() as the callback that
uses show_ref() as a helper, it forgot to do the clean-up in this patch.

[PATCH 3/3] write_head_info(): handle "extra refs" locally

[mhagger]

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

The old code basically did:

     generate array of SHA1s for alternate refs
     for each unique SHA1 in array:
         add_extra_ref(".have", sha1)
     for each ref (including real refs and extra refs):
         show_ref(refname, sha1)

But there is no need to stuff the alternate refs in extra_refs; we can
call show_ref() directly when iterating over the array, then handle
real refs separately.  So change the code to:

     generate array of SHA1s for alternate refs
     for each unique SHA1 in array:
         show_ref(".have", sha1)
     for each ref (this now only includes real refs):
         show_ref(refname, sha1)

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/receive-pack.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 65d129c..8c9e91e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -144,9 +144,9 @@ static int show_ref_cb(const char *path, const unsigned char *sha1, int flag, vo
 	return 0;
 }
 
-static void add_one_alternate_sha1(const unsigned char sha1[20], void *unused)
+static void show_one_alternate_sha1(const unsigned char sha1[20], void *unused)
 {
-	add_extra_ref(".have", sha1, 0);
+	show_ref(".have", sha1);
 }
 
 static void collect_one_alternate_ref(const struct ref *ref, void *data)
@@ -159,12 +159,11 @@ static void write_head_info(void)
 {
 	struct sha1_array sa = SHA1_ARRAY_INIT;
 	for_each_alternate_ref(collect_one_alternate_ref, &sa);
-	sha1_array_for_each_unique(&sa, add_one_alternate_sha1, NULL);
+	sha1_array_for_each_unique(&sa, show_one_alternate_sha1, NULL);
 	sha1_array_clear(&sa);
 	for_each_ref(show_ref_cb, NULL);
 	if (!sent_capabilities)
 		show_ref("capabilities^{}", null_sha1);
-	clear_extra_refs();
 
 	/* EOF */
 	packet_flush(1);
-- 
1.7.8.2

Re: [PATCH 3/3] write_head_info(): handle "extra refs" locally

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> The old code basically did:
>
>      generate array of SHA1s for alternate refs
>      for each unique SHA1 in array:
>          add_extra_ref(".have", sha1)
>      for each ref (including real refs and extra refs):
>          show_ref(refname, sha1)
>
> But there is no need to stuff the alternate refs in extra_refs; we can
> call show_ref() directly when iterating over the array, then handle
> real refs separately.  So change the code to:
>
>      generate array of SHA1s for alternate refs
>      for each unique SHA1 in array:
>          show_ref(".have", sha1)
>      for each ref (this now only includes real refs):
>          show_ref(refname, sha1)

This updated logic should be equivalent to the old one as long as nobody
else called add_extra_ref() before we come to write_head_info() function,
which should hold true.

The entire series looks good. Thanks.

Re: [PATCH 0/3] Eliminate one user of extra_refs

[Jeff King]

On Fri, Jan 06, 2012 at 03:12:30PM +0100, mhagger@alum.mit.edu wrote:

> Receive pack currently uses "extra refs" to keep track of ".have"
> references, which in turn are used to tell the source the SHA1s of
> references that are already known to the repository via alternates.
> 
> But the code already creates an array holding the alternate SHA1s.  So
> just read the SHA1s out of this array rather then round-tripping them
> through the extra_refs mechanism.
> 
> This is one step towards hopefully abolishing extra_refs altogether.
> I still have to examine the other user.

Thanks, this is a nice simplification. The patches look good to me, and
they produce the same output for a simple test (I happened to be fiddling
with receive-pack and alternates yesterday, so I had a nice test case
right at hand :) ).

>   receive-pack: move more work into write_head_info()

BTW, I have a patch to make sending ".have" refs configurable[1] (it
adds a receive.advertiseAlternates config variable), which this patch
conflicts with. I don't think that is your problem, but I thought I
would mention it since you are working in the area. Is that something we
want in git?

-Peff

[1] We are using it at GitHub because our alternates repos are so huge
    that the overhead of advertising the refs outweighs the minor
    benefits you get from avoiding object transfer.