# [PATCH 0/3] http-push fixes 8 messages from 2008-02-14 to 2008-02-16. Participants: Johannes Schindelin, Junio C Hamano, Mike Hommey. Thread: https://gitlist.dev/t/12108 ## Johannes Schindelin, 2008-02-14 23:25 Subject: [PATCH 0/3] http-push fixes Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802142319340.30505%40racer.site ``` 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, 2008-02-14 23:25 Subject: [PATCH 1/3] http-push: avoid invalid memory accesses Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802142325120.30505%40racer.site In-Reply-To: ``` 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 --- 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 ``` ## Johannes Schindelin, 2008-02-14 23:25 Subject: [PATCH 2/3] http-push: do not get confused by submodules Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802142325400.30505%40racer.site In-Reply-To: ``` 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 --- 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 ``` ## Johannes Schindelin, 2008-02-14 23:26 Subject: [PATCH 3/3] http-push: avoid a needless goto Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802142326040.30505%40racer.site In-Reply-To: ``` 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 --- 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 ``` ## Johannes Schindelin, 2008-02-14 23:32 Subject: [PATCH 2/3, v2] http-push: do not get confused by submodules Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802142331100.30505%40racer.site In-Reply-To: ``` 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 --- 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 ``` ## Junio C Hamano, 2008-02-16 07:23 Subject: Re: [PATCH 1/3] http-push: avoid invalid memory accesses Message-ID: <7vabm1cqkd.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vabm1cqkd.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin 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 > --- > 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... ``` ## Mike Hommey, 2008-02-16 08:13 Subject: Re: [PATCH 1/3] http-push: avoid invalid memory accesses Message-ID: <20080216081317.GA5350@glandium.org> URL: https://gitlist.dev/e/20080216081317.GA5350%40glandium.org In-Reply-To: <7vabm1cqkd.fsf@gitster.siamese.dyndns.org> ``` On Fri, Feb 15, 2008 at 11:23:46PM -0800, Junio C Hamano wrote: > Johannes Schindelin 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 > > --- > > 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, 2008-02-16 12:12 Subject: Re: [PATCH 1/3] http-push: avoid invalid memory accesses Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802161211320.30505%40racer.site In-Reply-To: <7vabm1cqkd.fsf@gitster.siamese.dyndns.org> ``` Hi, On Fri, 15 Feb 2008, Junio C Hamano wrote: > Johannes Schindelin 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 > > --- > > 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 ```