{"thread":{"id":"12108","subject":"[PATCH 0/3] http-push fixes","startedAt":"2008-02-14T23:25:02Z","lastAt":"2008-02-16T12:12:03Z","messageCount":8,"participants":["Johannes Schindelin","Junio C Hamano","Mike Hommey"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"68780","messageId":"alpine.LSU.1.00.0802142319340.30505@racer.site","threadId":"12108","inReplyTo":null,"subject":"[PATCH 0/3] http-push fixes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-14T23:25:02Z","receivedAt":"2008-02-14T23:25:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nI have to admit that impatience got the better part of me. So I did not \nwait for those promised http cleanups, but dived into the current code and \ndebugged a (failing) http-push for several hours.\n\nLet me tell this: http-push's source code is not nice. The code is hard \nto follow, and it is repetitive. I would really welcome a major cleanup, \nbut unfortunately, I do not have half the time I would need for this.\n\nSo here go three patches, the first two of them fixing real issues (a \nsegfault, and an \"out of memory\", which is not really one), and the last \nshowing just what kind of cruft needs removing in http-push.c.\n\nI'm particularly bad with cover-letters (waiting for format-patch \n--cover-letter to be applied, hint, hint), so this is my version of one:\n\n http-push.c | 12 ++++++------\n 1 files changed, 6 insertions(+), 6 deletions(-)\n\nCiao,\nDscho\n"},{"id":"68781","messageId":"alpine.LSU.1.00.0802142325120.30505@racer.site","threadId":"12108","inReplyTo":"alpine.LSU.1.00.0802142319340.30505@racer.site","subject":"[PATCH 1/3] http-push: avoid invalid memory accesses","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-14T23:25:33Z","receivedAt":"2008-02-14T23:25:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nBefore objects are sent, the respective ref is locked. However,\nwithout this patch, the lock is lifted before the last object for\nthat ref was sent. As a consequence, the lock data was accessed\nafter the lock structure was free()d.\n\nSigned-off-by: Johannes Schindelin \n---\n http-push.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex b2b410d..7a6c669 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -2398,7 +2398,10 @@ int main(int argc, char **argv)\n \t\tfill_active_slots();\n \t\tadd_fill_function(NULL, fill_active_slot);\n #endif\n-\t\tfinish_all_active_slots();\n+\t\tdo {\n+\t\t\tfinish_all_active_slots();\n+\t\t\tfill_active_slots();\n+\t\t} while (request_queue_head && !aborted);\n \n \t\t/* Update the remote branch if all went well */\n \t\tif (aborted || !update_remote(ref->new_sha1, ref_lock)) {\n-- \n1.5.4.1.1353.g0d5dd\n"},{"id":"68782","messageId":"alpine.LSU.1.00.0802142325400.30505@racer.site","threadId":"12108","inReplyTo":"alpine.LSU.1.00.0802142319340.30505@racer.site","subject":"[PATCH 2/3] http-push: do not get confused by submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-14T23:25:54Z","receivedAt":"2008-02-14T23:25:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nWhen encountering submodules in a tree, http-push should not try sending\nthe respective object. Instead, it should ignore it.\n\nSigned-off-by: Johannes Schindelin \n---\n http-push.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 7a6c669..93bd87d 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1637,7 +1637,7 @@ static struct object_list **process_tree(struct tree *tree,\n \twhile (tree_entry(&desc, &entry)) {\n \t\tif (S_ISDIR(entry.mode))\n \t\t\tp = process_tree(lookup_tree(entry.sha1), p, &me, name);\n-\t\telse\n+\t\telse if (!S_ISGITLINK(entry.mode))\n \t\t\tp = process_blob(lookup_blob(entry.sha1), p, &me, name);\n \t}\n \tfree(tree->buffer);\n-- \n1.5.4.1.1353.g0d5dd\n"},{"id":"68783","messageId":"alpine.LSU.1.00.0802142326040.30505@racer.site","threadId":"12108","inReplyTo":"alpine.LSU.1.00.0802142319340.30505@racer.site","subject":"[PATCH 3/3] http-push: avoid a needless goto","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-14T23:26:12Z","receivedAt":"2008-02-14T23:26:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThere was a goto, and while it is not half as harmful as some people\nbelieve, it was unnecessary here. So remove it for readability.\n\nSigned-off-by: Johannes Schindelin \n---\n http-push.c | 5 +----\n 1 files changed, 1 insertions(+), 4 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 93bd87d..4862db6 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -2404,12 +2404,9 @@ int main(int argc, char **argv)\n \t\t} while (request_queue_head && !aborted);\n \n \t\t/* Update the remote branch if all went well */\n-\t\tif (aborted || !update_remote(ref->new_sha1, ref_lock)) {\n+\t\tif (aborted || !update_remote(ref->new_sha1, ref_lock))\n \t\t\trc = 1;\n-\t\t\tgoto unlock;\n-\t\t}\n \n-\tunlock:\n \t\tif (!rc)\n \t\t\tfprintf(stderr, \" done\\n\");\n \t\tunlock_remote(ref_lock);\n-- \n1.5.4.1.1353.g0d5dd\n"},{"id":"68784","messageId":"alpine.LSU.1.00.0802142331100.30505@racer.site","threadId":"12108","inReplyTo":"alpine.LSU.1.00.0802142325400.30505@racer.site","subject":"[PATCH 2/3, v2] http-push: do not get confused by submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-14T23:32:32Z","receivedAt":"2008-02-14T23:32:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nWhen encountering submodules in a tree, http-push should not try sending\nthe respective object. Instead, it should ignore it.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tI just remembered that Linus did something similar, but more \n\telegant, in 481f0ee60eef2c34b891e5d04b7e6e5a955eedf4. This\n\tpatch imitates that commit.\n\n http-push.c | 15 +++++++++++----\n 1 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 7a6c669..4d200bc 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1634,12 +1634,19 @@ static struct object_list **process_tree(struct tree *tree,\n \n \tinit_tree_desc(&desc, tree->buffer, tree->size);\n \n-\twhile (tree_entry(&desc, &entry)) {\n-\t\tif (S_ISDIR(entry.mode))\n+\twhile (tree_entry(&desc, &entry))\n+\t\tswitch (object_type(entry.mode)) {\n+\t\tcase OBJ_TREE:\n \t\t\tp = process_tree(lookup_tree(entry.sha1), p, &me, name);\n-\t\telse\n+\t\t\tbreak;\n+\t\tcase OBJ_BLOB:\n \t\t\tp = process_blob(lookup_blob(entry.sha1), p, &me, name);\n-\t}\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\t/* Subproject commit - not in this repository */\n+\t\t\tbreak;\n+\t\t}\n+\n \tfree(tree->buffer);\n \ttree->buffer = NULL;\n \treturn p;\n-- \n1.5.4.1.1353.g0d5dd\n"},{"id":"68881","messageId":"7vabm1cqkd.fsf@gitster.siamese.dyndns.org","threadId":"12108","inReplyTo":"alpine.LSU.1.00.0802142325120.30505@racer.site","subject":"Re: [PATCH 1/3] http-push: avoid invalid memory accesses","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-16T07:23:46Z","receivedAt":"2008-02-16T07:23:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Before objects are sent, the respective ref is locked. However,\n> without this patch, the lock is lifted before the last object for\n> that ref was sent. As a consequence, the lock data was accessed\n> after the lock structure was free()d.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> http-push.c | 5 ++++-\n> 1 files changed, 4 insertions(+), 1 deletions(-)\n>\n> diff --git a/http-push.c b/http-push.c\n> index b2b410d..7a6c669 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -2398,7 +2398,10 @@ int main(int argc, char **argv)\n> \t\tfill_active_slots();\n> \t\tadd_fill_function(NULL, fill_active_slot);\n> #endif\n> -\t\tfinish_all_active_slots();\n> +\t\tdo {\n> +\t\t\tfinish_all_active_slots();\n#ifdef CURL_MULTI\n> +\t\t\tfill_active_slots();\n#endif\n> +\t\t} while (request_queue_head && !aborted);\n> \n> \t\t/* Update the remote branch if all went well */\n> \t\tif (aborted || !update_remote(ref->new_sha1, ref_lock)) {\n> -- \n> 1.5.4.1.1353.g0d5dd\n\nI wonder if we should define a no-op function fill_active_slots()\nfor non MULTI case...\n"},{"id":"68885","messageId":"20080216081317.GA5350@glandium.org","threadId":"12108","inReplyTo":"7vabm1cqkd.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/3] http-push: avoid invalid memory accesses","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2008-02-16T08:13:17Z","receivedAt":"2008-02-16T08:13:17Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Feb 15, 2008 at 11:23:46PM -0800, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n> > Before objects are sent, the respective ref is locked. However,\n> > without this patch, the lock is lifted before the last object for\n> > that ref was sent. As a consequence, the lock data was accessed\n> > after the lock structure was free()d.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > http-push.c | 5 ++++-\n> > 1 files changed, 4 insertions(+), 1 deletions(-)\n> >\n> > diff --git a/http-push.c b/http-push.c\n> > index b2b410d..7a6c669 100644\n> > --- a/http-push.c\n> > +++ b/http-push.c\n> > @@ -2398,7 +2398,10 @@ int main(int argc, char **argv)\n> > \t\tfill_active_slots();\n> > \t\tadd_fill_function(NULL, fill_active_slot);\n> > #endif\n> > -\t\tfinish_all_active_slots();\n> > +\t\tdo {\n> > +\t\t\tfinish_all_active_slots();\n> #ifdef CURL_MULTI\n> > +\t\t\tfill_active_slots();\n> #endif\n> > +\t\t} while (request_queue_head && !aborted);\n> > \n> > \t\t/* Update the remote branch if all went well */\n> > \t\tif (aborted || !update_remote(ref->new_sha1, ref_lock)) {\n> > -- \n> > 1.5.4.1.1353.g0d5dd\n> \n> I wonder if we should define a no-op function fill_active_slots()\n> for non MULTI case...\n\nOr a #define... but all this slot thing is probably going to change\nanyway, when I'll be done with it (but this is not planned for tomorrow).\n\nMike\n"},{"id":"68898","messageId":"alpine.LSU.1.00.0802161211320.30505@racer.site","threadId":"12108","inReplyTo":"7vabm1cqkd.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/3] http-push: avoid invalid memory accesses","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-16T12:12:03Z","receivedAt":"2008-02-16T12:12:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 15 Feb 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Before objects are sent, the respective ref is locked. However,\n> > without this patch, the lock is lifted before the last object for\n> > that ref was sent. As a consequence, the lock data was accessed\n> > after the lock structure was free()d.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > http-push.c | 5 ++++-\n> > 1 files changed, 4 insertions(+), 1 deletions(-)\n> >\n> > diff --git a/http-push.c b/http-push.c\n> > index b2b410d..7a6c669 100644\n> > --- a/http-push.c\n> > +++ b/http-push.c\n> > @@ -2398,7 +2398,10 @@ int main(int argc, char **argv)\n> > \t\tfill_active_slots();\n> > \t\tadd_fill_function(NULL, fill_active_slot);\n> > #endif\n> > -\t\tfinish_all_active_slots();\n> > +\t\tdo {\n> > +\t\t\tfinish_all_active_slots();\n> #ifdef CURL_MULTI\n> > +\t\t\tfill_active_slots();\n> #endif\n> > +\t\t} while (request_queue_head && !aborted);\n> > \n> > \t\t/* Update the remote branch if all went well */\n> > \t\tif (aborted || !update_remote(ref->new_sha1, ref_lock)) {\n> > -- \n> > 1.5.4.1.1353.g0d5dd\n> \n> I wonder if we should define a no-op function fill_active_slots()\n> for non MULTI case...\n\nDarn, darn, darn. Fixing the MULTI case was on my TODO list.\n\nCiao,\nDscho\n"}]}