threads / patch / 29305

patch, 3 partsshow_ref(): remove unused "flag" and "cb_data" arguments

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

## tl;dr

7 messages between Jan 6, 2012 and Jan 7, 2012. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

mhagger@alum.mit.edu· Jan 6, 2012, 14:12 UTC · lore

[PATCH 0/3] Eliminate one user of extra_refs

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
mhagger@alum.mit.edu· Jan 6, 2012, 14:12 UTC · re: mhagger@alum.mit.edu · lore

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

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(-)
Show changes to builtin/receive-pack.c +18 −24
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
mhagger@alum.mit.edu· Jan 6, 2012, 14:12 UTC · re: mhagger@alum.mit.edu · lore
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(-)
Show changes to builtin/receive-pack.c +5 −5
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
Junio C Hamano· Jan 7, 2012, 05:08 UTC · re: mhagger@alum.mit.edu · lore

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

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.

mhagger@alum.mit.edu· Jan 6, 2012, 14:12 UTC · re: mhagger@alum.mit.edu · lore

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

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(-)
Show changes to builtin/receive-pack.c +3 −4
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
Junio C Hamano· Jan 6, 2012, 19:45 UTC · re: mhagger@alum.mit.edu · lore

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

mhagger@alum.mit.edu writes:
Show 19 quoted lines
> 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.
Jeff King· Jan 6, 2012, 14:53 UTC · re: mhagger@alum.mit.edu · lore

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

On Fri, Jan 06, 2012 at 03:12:30PM +0100, mhagger@alum.mit.edu wrote:
Show 10 quoted lines
> 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.

← back to recent threads