Re: [PATCH 2/2] match_refs: search ref list tail internally

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

Re: [PATCH 2/2] match_refs: search ref list tail internally

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:52

Clemens Buchacher [off-list ref] writes:
Avoid code duplication by moving list tail search to match_refs().

This does not change the semantics, not even for http-push. The NULL
test for remote_tail was redundant.
The existing program (and the result after the patch) open-codes too much
inside the huge main() function, and it is extremely painful to follow
what is going on.

But it is not that "the NULL test was redundant" that I'd be worried
about.  In that unreadable main() function:

 - get_dav_remote_heads() is called, which asks the other end what the
   remote refs are; the result is queued in the linked list whose head is
   remote_refs and tail is remote_tail;

 - the resulting remote_tail is given to match_refs() to further grow the
   linked list.  The function will queue new "struct ref" instances
   (e.g. by making a call to match_explicit_refs()) at the end of the
   list.

   The caller used to pass "here is the end of the list" variable to it,
   but now you compute (perhaps redundantly) the end of the ref list by
   tangling from dst to its tail, yourself, and match_refs() links new
   elements at the tail of the list correctly.

   But what happens to the calling program's remote_tail variable after
   match_refs() returns?  The code used to guarantee that it always point
   at the real end of the list, but that guarantee is now gone.

It so happens that http-push.c never looked at remote_tail to do further
processing on the list after match_refs() returned, and that is why your
patch does not break http-push.c.  Any third-party patch to http-push.c
that relied on the old guarantee will textually merge cleanly but will
subtly break with this change.

Other parts of this patch removes the local "remote_tail" variables, and
it is very clear that they do not have this problem; any third-parth patch
will break if they used remote_tail after match_refs() returned, so this
change is a safe one for them.

I wonder what interaction this change will have with the http-push
clean-up Ray Chuan has been working on...

Re: [PATCH 2/2] match_refs: search ref list tail internally

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:46:52

On Thu, May 28, 2009 at 12:06:32AM -0700, Junio C Hamano wrote:
It so happens that http-push.c never looked at remote_tail to do further
processing on the list after match_refs() returned, and that is why your
patch does not break http-push.c.

Any third-party patch to http-push.c
that relied on the old guarantee will textually merge cleanly but will
subtly break with this change.

Other parts of this patch removes the local "remote_tail" variables, and
it is very clear that they do not have this problem; any third-parth patch
will break if they used remote_tail after match_refs() returned, so this
change is a safe one for them.
Yes, I tried to get rid of remote_tail entirely, but I could not come up
with a solution immediately. We could insert to the front of the list
instead. I'll try that.
I wonder what interaction this change will have with the http-push
clean-up Ray Chuan has been working on...
For me, t5540 currently segfaults on pu. I'm looking into it.

[PATCH v2] match_refs: search ref list tail internally

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:46:53

Avoid code duplication by moving list tail search to match_refs().

This does not change the semantics, except for http-push, which now inserts
to the front of the ref list in order to get rid of the global remote_tail.

Signed-off-by: Clemens Buchacher <redacted>
---

On Thu, May 28, 2009 at 12:06:32AM -0700, Junio C Hamano wrote:
Clemens Buchacher [off-list ref] writes:
quoted
Avoid code duplication by moving list tail search to match_refs().

This does not change the semantics, not even for http-push. The NULL
test for remote_tail was redundant.
[...]
Other parts of this patch removes the local "remote_tail" variables, and
it is very clear that they do not have this problem; any third-parth patch
will break if they used remote_tail after match_refs() returned, so this
change is a safe one for them.
Alright, so here's the version which removes remote_tail.
I wonder what interaction this change will have with the http-push
clean-up Ray Chuan has been working on...
I did test on rctay's http-fix-push-fetching branch in
git://github.com/rctay/git.git with the most recent fixes.

I also squashed in your style fixes from the version currently in pu.
Thanks.

Clemens

 builtin-remote.c    |    7 ++-----
 builtin-send-pack.c |    9 ++-------
 http-push.c         |   11 ++++-------
 remote.c            |   17 +++++++++++++----
 remote.h            |    2 +-
 transport.c         |    6 +-----
 6 files changed, 23 insertions(+), 29 deletions(-)
diff --git a/builtin-remote.c b/builtin-remote.c
index fda9a54..9248e0a 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -294,17 +294,14 @@ static int get_push_ref_states(const struct ref *remote_refs,
 	struct ref_states *states)
 {
 	struct remote *remote = states->remote;
-	struct ref *ref, *local_refs, *push_map, **push_tail;
+	struct ref *ref, *local_refs, *push_map;
 	if (remote->mirror)
 		return 0;
 
 	local_refs = get_local_heads();
 	push_map = copy_ref_list(remote_refs);
 
-	push_tail = &push_map;
-	while (*push_tail)
-		push_tail = &((*push_tail)->next);
-	match_refs(local_refs, push_map, &push_tail, remote->push_refspec_nr,
+	match_refs(local_refs, &push_map, remote->push_refspec_nr,
 		   remote->push_refspec, MATCH_REFS_NONE);
 
 	states->push.strdup_strings = 1;
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index be3b092..c375a3d 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -473,7 +473,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int fd[2];
 	struct child_process *conn;
 	struct extra_have_objects extra_have;
-	struct ref *remote_refs, **remote_tail, *local_refs;
+	struct ref *remote_refs, *local_refs;
 	int ret;
 	int send_all = 0;
 	const char *receivepack = "git-receive-pack";
@@ -567,13 +567,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 		flags |= MATCH_REFS_MIRROR;
 
 	/* match them up */
-	remote_tail = &remote_refs;
-	while (*remote_tail)
-		remote_tail = &((*remote_tail)->next);
-	if (match_refs(local_refs, remote_refs, &remote_tail,
-		       nr_refspecs, refspecs, flags)) {
+	if (match_refs(local_refs, &remote_refs, nr_refspecs, refspecs, flags))
 		return -1;
-	}
 
 	ret = send_pack(&args, fd, conn, remote_refs, &extra_have);
 
diff --git a/http-push.c b/http-push.c
index 10f64e3..c44cedd 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1850,7 +1850,7 @@ static int update_remote(unsigned char *sha1, struct remote_lock *lock)
 	return 1;
 }
 
-static struct ref *remote_refs, **remote_tail;
+static struct ref *remote_refs;
 
 static void one_remote_ref(char *refname)
 {
@@ -1880,13 +1880,12 @@ static void one_remote_ref(char *refname)
 		}
 	}
 
-	*remote_tail = ref;
-	remote_tail = &ref->next;
+	ref->next = remote_refs;
+	remote_refs = ref;
 }
 
 static void get_dav_remote_heads(void)
 {
-	remote_tail = &remote_refs;
 	remote_ls("refs/", (PROCESS_FILES | PROCESS_DIRS | RECURSIVE), process_ls_ref, NULL);
 }
 
@@ -2341,9 +2340,7 @@ int main(int argc, char **argv)
 	}
 
 	/* match them up */
-	if (!remote_tail)
-		remote_tail = &remote_refs;
-	if (match_refs(local_refs, remote_refs, &remote_tail,
+	if (match_refs(local_refs, &remote_refs,
 		       nr_refspec, (const char **) refspec, push_all)) {
 		rc = -1;
 		goto cleanup;
diff --git a/remote.c b/remote.c
index 2c3e905..08a5964 100644
--- a/remote.c
+++ b/remote.c
@@ -1085,12 +1085,20 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,
 		return NULL;
 }
 
+static struct ref **tail_ref(struct ref **head)
+{
+	struct ref **tail = head;
+	while (*tail)
+		tail = &((*tail)->next);
+	return tail;
+}
+
 /*
  * Note. This is used only by "push"; refspec matching rules for
  * push and fetch are subtly different, so do not try to reuse it
  * without thinking.
  */
-int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
+int match_refs(struct ref *src, struct ref **dst,
 	       int nr_refspec, const char **refspec, int flags)
 {
 	struct refspec *rs;
@@ -1098,13 +1106,14 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 	int send_mirror = flags & MATCH_REFS_MIRROR;
 	int errs;
 	static const char *default_refspec[] = { ":", 0 };
+	struct ref **dst_tail = tail_ref(dst);
 
 	if (!nr_refspec) {
 		nr_refspec = 1;
 		refspec = default_refspec;
 	}
 	rs = parse_push_refspec(nr_refspec, (const char **) refspec);
-	errs = match_explicit_refs(src, dst, dst_tail, rs, nr_refspec);
+	errs = match_explicit_refs(src, *dst, &dst_tail, rs, nr_refspec);
 
 	/* pick the remainder */
 	for ( ; src; src = src->next) {
@@ -1134,7 +1143,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 						     dst_side, &dst_name))
 				die("Didn't think it matches any more");
 		}
-		dst_peer = find_ref_by_name(dst, dst_name);
+		dst_peer = find_ref_by_name(*dst, dst_name);
 		if (dst_peer) {
 			if (dst_peer->peer_ref)
 				/* We're already sending something to this ref. */
@@ -1150,7 +1159,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 				goto free_name;
 
 			/* Create a new one and link it */
-			dst_peer = make_linked_ref(dst_name, dst_tail);
+			dst_peer = make_linked_ref(dst_name, &dst_tail);
 			hashcpy(dst_peer->new_sha1, src->new_sha1);
 		}
 		dst_peer->peer_ref = copy_ref(src);
diff --git a/remote.h b/remote.h
index 99706a8..257a555 100644
--- a/remote.h
+++ b/remote.h
@@ -85,7 +85,7 @@ void ref_remove_duplicates(struct ref *ref_map);
 int valid_fetch_refspec(const char *refspec);
 struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
 
-int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
+int match_refs(struct ref *src, struct ref **dst,
 	       int nr_refspec, const char **refspec, int all);
 
 /*
diff --git a/transport.c b/transport.c
index 17891d5..d8a2392 100644
--- a/transport.c
+++ b/transport.c
@@ -1003,7 +1003,6 @@ int transport_push(struct transport *transport,
 	if (transport->push_refs) {
 		struct ref *remote_refs =
 			transport->get_refs_list(transport, 1);
-		struct ref **remote_tail;
 		struct ref *local_refs = get_local_heads();
 		int match_flags = MATCH_REFS_NONE;
 		int verbose = flags & TRANSPORT_PUSH_VERBOSE;
@@ -1014,10 +1013,7 @@ int transport_push(struct transport *transport,
 		if (flags & TRANSPORT_PUSH_MIRROR)
 			match_flags |= MATCH_REFS_MIRROR;
 
-		remote_tail = &remote_refs;
-		while (*remote_tail)
-			remote_tail = &((*remote_tail)->next);
-		if (match_refs(local_refs, remote_refs, &remote_tail,
+		if (match_refs(local_refs, &remote_refs,
 			       refspec_nr, refspec, match_flags)) {
 			return -1;
 		}
-- 
1.6.3.1.147.g637c3
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help