Re: [RFCv2 05/16] remote.h: Change get_remote_heads return to void

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [RFCv2 05/16] remote.h: Change get_remote_heads return to void

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:05:06

Stefan Beller [off-list ref] writes:
No function uses the return value of get_remote_heads, so we don't want
to confuse readers by it.

Signed-off-by: Stefan Beller <redacted>
---
This is somewhat a sad change, as the returned value is designed to
be useful if caller wants to continue appending to the list.

Now such a caller has to tangle the list (the variable it gave the
function as the fourth argument) itself to find its tail.

Does it really "confuse" readers enough that it hurts to have a
return value?
quoted hunk
 connect.c | 10 ++++------
 remote.h  |  8 ++++----
 2 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/connect.c b/connect.c
index 4295ba1..a2c777e 100644
--- a/connect.c
+++ b/connect.c
@@ -108,10 +108,10 @@ static void annotate_refs_with_symref_info(struct ref *ref)
 /*
  * Read all the refs from the other end
  */
-struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
-			      struct ref **list, unsigned int flags,
-			      struct sha1_array *extra_have,
-			      struct sha1_array *shallow_points)
+void get_remote_heads(int in, char *src_buf, size_t src_len,
+		      struct ref **list, unsigned int flags,
+		      struct sha1_array *extra_have,
+		      struct sha1_array *shallow_points)
 {
 	struct ref **orig_list = list;
 	int got_at_least_one_head = 0;
@@ -172,8 +172,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	}
 
 	annotate_refs_with_symref_info(*orig_list);
-
-	return list;
 }
 
 static const char *parse_feature_value(struct string_list *feature_list, const char *feature, int *lenp)
diff --git a/remote.h b/remote.h
index 02d66ce..d5242b0 100644
--- a/remote.h
+++ b/remote.h
@@ -144,10 +144,10 @@ int check_ref_type(const struct ref *ref, int flags);
 void free_refs(struct ref *ref);
 
 struct sha1_array;
-extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
-				     struct ref **list, unsigned int flags,
-				     struct sha1_array *extra_have,
-				     struct sha1_array *shallow);
+extern void get_remote_heads(int in, char *src_buf, size_t src_len,
+			     struct ref **list, unsigned int flags,
+			     struct sha1_array *extra_have,
+			     struct sha1_array *shallow);
 
 int resolve_remote_symref(struct ref *ref, struct ref *list);
 int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1);

Re: [RFCv2 05/16] remote.h: Change get_remote_heads return to void

From: Stefan Beller <hidden>
Date: 2016-06-15 23:05:06

On Tue, Jun 2, 2015 at 2:17 PM, Junio C Hamano [off-list ref] wrote:
Stefan Beller [off-list ref] writes:
quoted
No function uses the return value of get_remote_heads, so we don't want
to confuse readers by it.

Signed-off-by: Stefan Beller <redacted>
---
This is somewhat a sad change, as the returned value is designed to
be useful if caller wants to continue appending to the list.
But there are no callers since like 2005. ;)
(I did not exactly track it down, but even the last caller went away
rather fast)
Does it really "confuse" readers enough that it hurts to have a
return value?
Probably no.
But I think this is just carrying around cruft we could avoid?
Now such a caller has to tangle the list (the variable it gave the
function as the fourth argument) itself to find its tail.
So you're saying if someone in the future really wants to append
to that list, they don't find out to just return it again but rather
do an O(n) operation?
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help