[PATCH 0/3] http-push fixes

[PATCH 0/3] http-push fixes

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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();
+			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

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

[Junio C Hamano]

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...

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

[Mike Hommey]

On Fri, Feb 15, 2008 at 11:23:46PM -0800, Junio C Hamano wrote:
> 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

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

[Johannes Schindelin]

Hi,

On Fri, 15 Feb 2008, Junio C Hamano wrote:

> 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

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

[Johannes Schindelin]

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(-)

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

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

[Johannes Schindelin]

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(-)

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

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

[Johannes Schindelin]

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(-)

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