threads / patch / 12108

patch, 3 partshttp-push fixes

Subject: [PATCH 0/3] http-push fixes

## tl;dr

8 messages between Feb 14, 2008 and Feb 16, 2008. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Johannes Schindelin· Feb 14, 2008, 23:25 UTC · lore
Hi,

I have to admit that impatience got the better part of me. So I did not wait for those promised http cleanups, but dived into the current code and debugged a (failing) http-push for several hours.

Let me tell this: http-push's source code is not nice. The code is hard to follow, and it is repetitive. I would really welcome a major cleanup, but unfortunately, I do not have half the time I would need for this.

So here go three patches, the first two of them fixing real issues (a segfault, and an "out of memory", which is not really one), and the last showing just what kind of cruft needs removing in http-push.c.

I'm particularly bad with cover-letters (waiting for format-patch --cover-letter to be applied, hint, hint), so this is my version of one:

 http-push.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

Ciao, Dscho

Johannes Schindelin· Feb 14, 2008, 23:25 UTC · re: Johannes Schindelin · lore

[PATCH 1/3] http-push: avoid invalid memory accesses

Before objects are sent, the respective ref is locked. However, without this patch, the lock is lifted before the last object for that ref was sent. As a consequence, the lock data was accessed after the lock structure was free()d.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 http-push.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Show changes to http-push.c +4 −1
diff --git a/http-push.c b/http-push.c
index b2b410d..7a6c669 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2398,7 +2398,10 @@ int main(int argc, char **argv)
 		fill_active_slots();
 		add_fill_function(NULL, fill_active_slot);
 #endif
-		finish_all_active_slots();
+		do {
+			finish_all_active_slots();
+			fill_active_slots();
+		} while (request_queue_head && !aborted);
 
 		/* Update the remote branch if all went well */
 		if (aborted || !update_remote(ref->new_sha1, ref_lock)) {

-- 
1.5.4.1.1353.g0d5dd
Junio C Hamano· Feb 16, 2008, 07:23 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/3] http-push: avoid invalid memory accesses

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 21 quoted lines
> Before objects are sent, the respective ref is locked.  However,
> without this patch, the lock is lifted before the last object for
> that ref was sent.  As a consequence, the lock data was accessed
> after the lock structure was free()d.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  http-push.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/http-push.c b/http-push.c
> index b2b410d..7a6c669 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -2398,7 +2398,10 @@ int main(int argc, char **argv)
>  		fill_active_slots();
>  		add_fill_function(NULL, fill_active_slot);
>  #endif
> -		finish_all_active_slots();
> +		do {
> +			finish_all_active_slots();
#ifdef CURL_MULTI
> +			fill_active_slots();
#endif
Show 6 quoted lines
> +		} while (request_queue_head && !aborted);
>  
>  		/* Update the remote branch if all went well */
>  		if (aborted || !update_remote(ref->new_sha1, ref_lock)) {
> -- 
> 1.5.4.1.1353.g0d5dd

I wonder if we should define a no-op function fill_active_slots() for non MULTI case...

Mike Hommey· Feb 16, 2008, 08:13 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] http-push: avoid invalid memory accesses

On Fri, Feb 15, 2008 at 11:23:46PM -0800, Junio C Hamano wrote:
Show 35 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Before objects are sent, the respective ref is locked.  However,
> > without this patch, the lock is lifted before the last object for
> > that ref was sent.  As a consequence, the lock data was accessed
> > after the lock structure was free()d.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  http-push.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/http-push.c b/http-push.c
> > index b2b410d..7a6c669 100644
> > --- a/http-push.c
> > +++ b/http-push.c
> > @@ -2398,7 +2398,10 @@ int main(int argc, char **argv)
> >  		fill_active_slots();
> >  		add_fill_function(NULL, fill_active_slot);
> >  #endif
> > -		finish_all_active_slots();
> > +		do {
> > +			finish_all_active_slots();
> #ifdef CURL_MULTI
> > +			fill_active_slots();
> #endif
> > +		} while (request_queue_head && !aborted);
> >  
> >  		/* Update the remote branch if all went well */
> >  		if (aborted || !update_remote(ref->new_sha1, ref_lock)) {
> > -- 
> > 1.5.4.1.1353.g0d5dd
> 
> I wonder if we should define a no-op function fill_active_slots()
> for non MULTI case...

Or a #define... but all this slot thing is probably going to change anyway, when I'll be done with it (but this is not planned for tomorrow).

Mike
Johannes Schindelin· Feb 16, 2008, 12:12 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] http-push: avoid invalid memory accesses

Hi,
On Fri, 15 Feb 2008, Junio C Hamano wrote:
Show 35 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Before objects are sent, the respective ref is locked.  However,
> > without this patch, the lock is lifted before the last object for
> > that ref was sent.  As a consequence, the lock data was accessed
> > after the lock structure was free()d.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  http-push.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/http-push.c b/http-push.c
> > index b2b410d..7a6c669 100644
> > --- a/http-push.c
> > +++ b/http-push.c
> > @@ -2398,7 +2398,10 @@ int main(int argc, char **argv)
> >  		fill_active_slots();
> >  		add_fill_function(NULL, fill_active_slot);
> >  #endif
> > -		finish_all_active_slots();
> > +		do {
> > +			finish_all_active_slots();
> #ifdef CURL_MULTI
> > +			fill_active_slots();
> #endif
> > +		} while (request_queue_head && !aborted);
> >  
> >  		/* Update the remote branch if all went well */
> >  		if (aborted || !update_remote(ref->new_sha1, ref_lock)) {
> > -- 
> > 1.5.4.1.1353.g0d5dd
> 
> I wonder if we should define a no-op function fill_active_slots()
> for non MULTI case...
Darn, darn, darn.  Fixing the MULTI case was on my TODO list.

Ciao, Dscho

Johannes Schindelin· Feb 14, 2008, 23:25 UTC · re: Johannes Schindelin · lore

[PATCH 2/3] http-push: do not get confused by submodules

When encountering submodules in a tree, http-push should not try sending the respective object. Instead, it should ignore it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 http-push.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to http-push.c +1 −1
diff --git a/http-push.c b/http-push.c
index 7a6c669..93bd87d 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1637,7 +1637,7 @@ static struct object_list **process_tree(struct tree *tree,
 	while (tree_entry(&desc, &entry)) {
 		if (S_ISDIR(entry.mode))
 			p = process_tree(lookup_tree(entry.sha1), p, &me, name);
-		else
+		else if (!S_ISGITLINK(entry.mode))
 			p = process_blob(lookup_blob(entry.sha1), p, &me, name);
 	}
 	free(tree->buffer);

-- 
1.5.4.1.1353.g0d5dd
Johannes Schindelin· Feb 14, 2008, 23:32 UTC · re: Johannes Schindelin · lore

[PATCH 2/3, v2] http-push: do not get confused by submodules

When encountering submodules in a tree, http-push should not try sending the respective object. Instead, it should ignore it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	I just remembered that Linus did something similar, but more 
	elegant, in 481f0ee60eef2c34b891e5d04b7e6e5a955eedf4.  This
	patch imitates that commit.
 http-push.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)
Show changes to http-push.c +11 −4
diff --git a/http-push.c b/http-push.c
index 7a6c669..4d200bc 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1634,12 +1634,19 @@ static struct object_list **process_tree(struct tree *tree,
 
 	init_tree_desc(&desc, tree->buffer, tree->size);
 
-	while (tree_entry(&desc, &entry)) {
-		if (S_ISDIR(entry.mode))
+	while (tree_entry(&desc, &entry))
+		switch (object_type(entry.mode)) {
+		case OBJ_TREE:
 			p = process_tree(lookup_tree(entry.sha1), p, &me, name);
-		else
+			break;
+		case OBJ_BLOB:
 			p = process_blob(lookup_blob(entry.sha1), p, &me, name);
-	}
+			break;
+		default:
+			/* Subproject commit - not in this repository */
+			break;
+		}
+
 	free(tree->buffer);
 	tree->buffer = NULL;
 	return p;

-- 
1.5.4.1.1353.g0d5dd
Johannes Schindelin· Feb 14, 2008, 23:26 UTC · re: Johannes Schindelin · lore

[PATCH 3/3] http-push: avoid a needless goto

There was a goto, and while it is not half as harmful as some people believe, it was unnecessary here. So remove it for readability.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 http-push.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)
Show changes to http-push.c +1 −4
diff --git a/http-push.c b/http-push.c
index 93bd87d..4862db6 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2404,12 +2404,9 @@ int main(int argc, char **argv)
 		} while (request_queue_head && !aborted);
 
 		/* Update the remote branch if all went well */
-		if (aborted || !update_remote(ref->new_sha1, ref_lock)) {
+		if (aborted || !update_remote(ref->new_sha1, ref_lock))
 			rc = 1;
-			goto unlock;
-		}
 
-	unlock:
 		if (!rc)
 			fprintf(stderr, "    done\n");
 		unlock_remote(ref_lock);

-- 
1.5.4.1.1353.g0d5dd

← back to recent threads