[PATCH] http-push: making HTTP push more robust and more user-friendly

[PATCH] http-push: making HTTP push more robust and more user-friendly

[Grégoire Barbier]

Fail when info/refs exists and is already locked (avoiding strange behaviour
and errors, and maybe avoiding some repository corruption).

Warn if the URL does not end with '/' (since 302 is not yet handled)

More explicit error message when the URL or password is not set correctly
(instead of "no DAV locking support").

DAV locking time of 1 minute instead of 10 minutes (avoid waiting 10 minutes
for a orphan lock to expire before anyone can do a push on the repo).

Signed-off-by: Grégoire Barbier <gb@gbarbier.org>
---
 http-push.c |   17 ++++++++++++++++-
 http.c      |   25 +++++++++++++++++++++++++
 http.h      |    1 +
 3 files changed, 42 insertions(+), 1 deletions(-)

diff --git a/http-push.c b/http-push.c
index 55d0c94..c005903 100644
--- a/http-push.c
+++ b/http-push.c
@@ -57,7 +57,7 @@ enum XML_Status {
 #define PROPFIND_ALL_REQUEST "<?xml version=\"1.0\" encoding=\"utf-8\" ?>\n<D:propfind xmlns:D=\"DAV:\">\n<D:allprop/>\n</D:propfind>"
 #define LOCK_REQUEST "<?xml version=\"1.0\" encoding=\"utf-8\" ?>\n<D:lockinfo xmlns:D=\"DAV:\">\n<D:lockscope><D:exclusive/></D:lockscope>\n<D:locktype><D:write/></D:locktype>\n<D:owner>\n<D:href>mailto:%s</D:href>\n</D:owner>\n</D:lockinfo>"
 
-#define LOCK_TIME 600
+#define LOCK_TIME 60
 #define LOCK_REFRESH 30
 
 /* bits #0-15 in revision.h */
@@ -2224,6 +2224,16 @@ int main(int argc, char **argv)
 
 	no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
 
+	/* Verify connexion string (agains bad URLs or password errors) */
+	if (remote->url && remote->url[strlen(remote->url)-1] != '/') {
+		fprintf(stderr, "Warning: remote URL does not end with a '/' which often leads to problems\n");
+	}
+	if (!http_test_connection(remote->url)) {
+		fprintf(stderr, "Error: cannot access to remote URL (maybe malformed URL, network error or bad credentials)\n");
+		rc = 1;
+		goto cleanup;
+	}
+
 	/* Verify DAV compliance/lock support */
 	if (!locking_available()) {
 		fprintf(stderr, "Error: no DAV locking support on remote repo %s\n", remote->url);
@@ -2239,6 +2249,11 @@ int main(int argc, char **argv)
 		info_ref_lock = lock_remote("info/refs", LOCK_TIME);
 		if (info_ref_lock)
 			remote->can_update_info_refs = 1;
+		else {
+			fprintf(stderr, "Error: cannot lock existing info/refs\n");
+			rc = 1;
+			goto cleanup;
+		}
 	}
 	if (remote->has_info_packs)
 		fetch_indices();
diff --git a/http.c b/http.c
index d2c11ae..8b04ae9 100644
--- a/http.c
+++ b/http.c
@@ -634,3 +634,28 @@ int http_fetch_ref(const char *base, const char *ref, unsigned char *sha1)
 	free(url);
 	return ret;
 }
+
+int http_test_connection(const char *url)
+{
+	struct strbuf buffer = STRBUF_INIT;
+	struct active_request_slot *slot;
+	struct slot_results results;
+	int ret = 0;
+
+	slot = get_active_slot();
+	slot->results = &results;
+	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
+	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
+	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
+	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
+	if (start_active_slot(slot)) {
+		run_active_slot(slot);
+		if (results.curl_result == CURLE_OK)
+			ret = -1;
+		else
+			error("Cannot access to URL %s, return code %d", url, results.curl_result);
+	} else
+		error("Unable to start request");
+	strbuf_release(&buffer);
+	return ret;
+}
diff --git a/http.h b/http.h
index aeba930..b353007 100644
--- a/http.h
+++ b/http.h
@@ -77,6 +77,7 @@ extern void step_active_slots(void);
 
 extern void http_init(void);
 extern void http_cleanup(void);
+extern int http_test_connection(const char *url);
 
 extern int data_received;
 extern int active_requests;
-- 
1.5.3.6

[PATCH] http-push: fix webdav lock leak.

[Grégoire Barbier]

Releasing webdav lock even if push fails because of bad (or no) reference
on command line.

Signed-off-by: Grégoire Barbier <gb@gbarbier.org>
---
 http-push.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/http-push.c b/http-push.c
index c005903..cbbf432 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2275,11 +2275,14 @@ int main(int argc, char **argv)
 	if (!remote_tail)
 		remote_tail = &remote_refs;
 	if (match_refs(local_refs, remote_refs, &remote_tail,
-		       nr_refspec, (const char **) refspec, push_all))
-		return -1;
+		       nr_refspec, (const char **) refspec, push_all)) {
+		rc = -1;
+		goto cleanup;
+	}
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n");
-		return 0;
+		rc = 0;
+		goto cleanup;
 	}
 
 	new_refs = 0;
@@ -2410,10 +2413,10 @@ int main(int argc, char **argv)
 			fprintf(stderr, "Unable to update server info\n");
 		}
 	}
-	if (info_ref_lock)
-		unlock_remote(info_ref_lock);
 
  cleanup:
+	if (info_ref_lock)
+		unlock_remote(info_ref_lock);
 	free(remote);
 
 	curl_slist_free_all(no_pragma_header);
-- 
1.5.3.6

[PATCH] http-push: disable http-push without USE_CURL_MULTI

[Grégoire Barbier]

Make http-push always fail when not compiled with USE_CURL_MULTI, since
otherwise it corrupts the remote repository (and then fails anyway).

Signed-off-by: Grégoire Barbier <gb@gbarbier.org>
---
 http-push.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/http-push.c b/http-push.c
index cbbf432..96c8e75 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2212,6 +2212,10 @@ int main(int argc, char **argv)
 		break;
 	}
 
+#ifndef USE_CURL_MULTI
+	die("git-push is not available for http/https repository when not compiled with USE_CURL_MULTI");
+#endif
+
 	if (!remote->url)
 		usage(http_push_usage);
 
-- 
1.5.3.6

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Junio C Hamano]

Grégoire Barbier <gb@gbarbier.org> writes:

> Fail when info/refs exists and is already locked (avoiding strange behaviour
> and errors, and maybe avoiding some repository corruption).
>
> Warn if the URL does not end with '/' (since 302 is not yet handled)
>
> More explicit error message when the URL or password is not set correctly
> (instead of "no DAV locking support").
>
> DAV locking time of 1 minute instead of 10 minutes (avoid waiting 10 minutes
> for a orphan lock to expire before anyone can do a push on the repo).

I do not remember these discussed on the list, and would like to
see people who do use http-push to comment on these.  Especially
because there is no correct timeout that is good for everybody,
the last item might be contentious.

The second one to add a couple of "goto cleanup" looked
correct.  Acks, people?

Also http-push being unusable without CURL_MULTI was also a news
to me.  Is this something that came up on #git perhaps?

This change means people need curl 7.10 or newer (post May 2003,
that is).  I do not think it is too new a version to require,
but then it makes me wonder if it makes much sense for us to
keep supporting non CURL_MULTI build these days.  Perhaps we
should schedule such a move to drop non MULTI build in the
future?

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Johannes Schindelin]

Hi,

On Sun, 13 Jan 2008, Junio C Hamano wrote:

> The second one to add a couple of "goto cleanup" looked correct.  Acks, 
> people?

I haven't used http-push in ages, but there was a bug report with msysgit.  
Hopefully that issue gets fixed by this patch.

Ciao,
Dscho

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sun, 13 Jan 2008, Junio C Hamano wrote:
>
>> The second one to add a couple of "goto cleanup" looked correct.  Acks, 
>> people?
>
> I haven't used http-push in ages, but there was a bug report with msysgit.  
> Hopefully that issue gets fixed by this patch.

Could you work with the reporter to see if this fixes the issue
for him?

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Johannes Schindelin]

Hi,

On Mon, 14 Jan 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sun, 13 Jan 2008, Junio C Hamano wrote:
> >
> >> The second one to add a couple of "goto cleanup" looked correct.  
> >> Acks, people?
> >
> > I haven't used http-push in ages, but there was a bug report with 
> > msysgit.  Hopefully that issue gets fixed by this patch.
> 
> Could you work with the reporter to see if this fixes the issue for him?

I wanted to try to reproduce first, but I had definitely not enough time 
for git today.

Will try to find some time tomorrow,
Dscho

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Grégoire Barbier]

Hi Junio, Hi Johannes,

I recently sent three patches about http-push:
>  $gmane/70406 <1200250979-19604-1-git-send-email-gb@gbarbier.org>
>  $gmane/70407 <1200250979-19604-2-git-send-email-gb@gbarbier.org>
>  $gmane/70405 <1200250979-19604-3-git-send-email-gb@gbarbier.org>

I saw that Junio has already applied one of them (the one that disable 
http-push without USE_CURL_MULTI).

I wont talk about the second one "fix webdav lock leak" in the present 
mail but in another one, since Johannes has found severe bugs in it. I 
prefer to make them separate subjects.

As for the third patch ("making HTTP push more robust and more 
user-friendly"), I recall the commit message here:

>  Grégoire Barbier <gb@gbarbier.org> writes:
> > Fail when info/refs exists and is already locked (avoiding strange
> > behaviour and errors, and maybe avoiding some repository
> > corruption).
> >
> > Warn if the URL does not end with '/' (since 302 is not yet
> > handled)
> >
> > More explicit error message when the URL or password is not set
> > correctly (instead of "no DAV locking support").
> >
> > DAV locking time of 1 minute instead of 10 minutes (avoid waiting
> > 10 minutes for a orphan lock to expire before anyone can do a push
> > on the repo).

I agree that it should be improved seriously in several ways. I will 
submit the patch again with following improvements.

1) I will split the patch into several ones, to enable Junio to apply it 
partially.

Junio C Hamano a écrit :
>  there is no correct timeout that is good for everybody, the last item
>  might be contentious.

2) I won't change the timeout to avoid possible side effects for other 
things I don't know about since I'm rather new to git.

Johannes Schindelin a écrit :
>  This patch makes http-push Warn if URL does not end if "/", but it
>  would be even better to just handle it... we know exactly that HTTP
>  URLs _must_ end in a slash.

3) Rather than warning if the URL does not end with a slash, I will add 
the slash, so that this will work, even without having to handle 
HTTP/302 in curl calls. BTW I will do the same for http-fetch either.

Johannes Schindelin a écrit :
>  It gives a better warning if the URL cannot be accessed, alright. But
>  I hate the fact that it introduces yet another function which does a
>  bunch of curl_easy_setopt()s only to start an active slot and check
>  for errors.
>
>  Currently, I am not familiar enough with http-push.c to suggest a
>  proper alternative, but I suspect that the return values of the
>  _existing_ calls to curl should know precisely why the requests
>  failed, and _this_ should be reported.

Mike Hommey a écrit :
 > FWIW, I have a work in progress refactoring the http code, avoiding a
 > great amount of curl_easy_setopt()s and simplifying the whole thing.
 > It's been sitting on my hard drive during my (quite long) vacation. I
 > will probably start working again on this soonish.

4) I agree with Johannes. However I am not familiar enough with curl to 
write the proper alternative. I create the new function by copy/paste of 
an existing one. I'm not 100% sure that it  has no resource leaks or 
other bugs, but it's called only once at http-push start, and thus is 
likely not to do heavy damage...

As a rationale: I've tried to make several developers use git over http, 
including push, and they made all the same beginner mistakes on the 
command line, all leading to that stupid error message about locking not 
available, and I think that making a clearer error message is an 
important improvement to make not-so-skilled developers using git when 
neither ssh nor git protocols are available.

Therefore I think that applying my patch, even if it's far from being 
perfect, is the lesser of two evils.

Then, for instance during 1.5.5 development cycle, I would be happy to 
help Mike if I can, to clean my new code that he is likelly not to have 
cleaned up on his hard disk during his vacation...
For instance I may look at his patches and take them in example to clean 
up my code.


Apart from the discussion on the source code, I would like to reply to 
Junio about the patch disabling http-push without USE_CURL_MULTI:
Junio C Hamano a écrit :
>  Also http-push being unusable without CURL_MULTI was also a news to
>  me.  Is this something that came up on #git perhaps?
>
>  This change means people need curl 7.10 or newer (post May 2003, that
>  is).  I do not think it is too new a version to require, but then it
>  makes me wonder if it makes much sense for us to keep supporting non
>  CURL_MULTI build these days.  Perhaps we should schedule such a move
>  to drop non MULTI build in the future?

I don't know if USE_CURL_MULTI works well for other git binaries than 
http-push (although I've used it successfully two or three times with 
clone and fetch).

If yes, I think that the release notes, or whatever information channel 
you can have with the various distribution maintainers, should advice to 
compile with USE_CURL_MULTI. Or we can make it the default compilation 
option in a future release (> 1.5.4 I think).

If USE_CURL_MULTI is not safe for other binaries than http-push, I think 
I should manage to make a new patch, let's say for git-1.5.5, that would 
change the makefile to use CURL_MULTI by default on http-push (for 
example without -DNEVER_USE_CURL_MULTI) and leave alone other binaries 
as they are (CURL_MULTI disabled without -DUSE_CURL_MULTI).

I want to insist that the present patch for 1.5.4 (which you've already 
applied to git.git), does not introduce by itself a dependence or a 
regression, it only disables unwarned users to call a function that does 
not work, but pretends to work and by the way corrupts the remote 
repository.

I thank you very much for the time you spent reviewing my patches and 
more generally for the work you do. I'll try to improve the way I submit 
patches to make them take you less time to review.

-- 
Grégoire Barbier - gb à gbarbier.org - +33 6 21 35 73 49

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1561 bytes --]

Hi,

On Sat, 19 Jan 2008, Grégoire Barbier wrote:

> Johannes Schindelin a écrit :
> >  It gives a better warning if the URL cannot be accessed, alright. But
> >  I hate the fact that it introduces yet another function which does a
> >  bunch of curl_easy_setopt()s only to start an active slot and check
> >  for errors.
> > 
> >  Currently, I am not familiar enough with http-push.c to suggest a
> >  proper alternative, but I suspect that the return values of the
> >  _existing_ calls to curl should know precisely why the requests
> >  failed, and _this_ should be reported.
> 
> Mike Hommey a écrit :
> > FWIW, I have a work in progress refactoring the http code, avoiding a 
> > great amount of curl_easy_setopt()s and simplifying the whole thing. 
> > It's been sitting on my hard drive during my (quite long) vacation. I 
> > will probably start working again on this soonish.
> 
> 4) I agree with Johannes. However I am not familiar enough with curl to 
> write the proper alternative. I create the new function by copy/paste of 
> an existing one. I'm not 100% sure that it has no resource leaks or 
> other bugs, but it's called only once at http-push start, and thus is 
> likely not to do heavy damage...

I agree that it is too late in the rc cycle (actually, I cannot wait for 
the end of it...) to do heavy refactoring, and this function is small 
enough that it should not hurt the refactoring effort, especially given 
that you want to work on that end anyway.

So please strike this one of my objections.

Thanks for all your work,
Dscho

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Grégoire Barbier]

Junio C Hamano a écrit :
> Also http-push being unusable without CURL_MULTI was also a news
> to me.  Is this something that came up on #git perhaps?
> 
> This change means people need curl 7.10 or newer (post May 2003,
> that is).  I do not think it is too new a version to require,
> but then it makes me wonder if it makes much sense for us to
> keep supporting non CURL_MULTI build these days.  Perhaps we
> should schedule such a move to drop non MULTI build in the
> future?

In fact, it's not curl 7.10 but curl 7.16 (those guys working on curl 
speak hexa).
See commit 9cf04301b182c4c57d62ea63554d109db613f9d3

However... http-push is anyway broken without USE_CURL_MULTI.

-- 
Grégoire Barbier - gb à gbarbier.org - +33 6 21 35 73 49

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Junio C Hamano]

Thanks for correction.  I need to update Release Notes...

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Grégoire Barbier]

Junio C Hamano a écrit :
> Thanks for correction.  I need to update Release Notes...

Curl 7.16 has been released in october 2006 
(http://curl.haxx.se/changes.html), rather than 2003 like for 7.10.

The consequences is that a lot of not so old distributions may be 
concerned. I only checked Fedora, which does not provide curl > 7.15 
before Fedora 7 (issued late may 2007).

(BTW you may guess well that I'm using a Fedora Core 6 for my git 
patches...)

-- 
Grégoire Barbier - gb à gbarbier.org - +33 6 21 35 73 49

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Junio C Hamano]

Grégoire Barbier <devel@gbarbier.org> writes:

> Junio C Hamano a écrit :
>> Thanks for correction.  I need to update Release Notes...
>
> Curl 7.16 has been released in october 2006
> (http://curl.haxx.se/changes.html), rather than 2003 like for 7.10.
>
> The consequences is that a lot of not so old distributions may be
> concerned. I only checked Fedora, which does not provide curl > 7.15
> before Fedora 7 (issued late may 2007).
>
> (BTW you may guess well that I'm using a Fedora Core 6 for my git
> patches...)

Now, that means the patch is not quite good for 1.5.4, and if we
want to keep http-push alive (I do not very much care about it
myself, though), and make it usable, we would need to fix it for
non MULTI case.

Hmmmmm.

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1474 bytes --]

Hi,

On Mon, 21 Jan 2008, Junio C Hamano wrote:

> Grégoire Barbier <devel@gbarbier.org> writes:
> 
> > Junio C Hamano a écrit :
> >> Thanks for correction.  I need to update Release Notes...
> >
> > Curl 7.16 has been released in october 2006 
> > (http://curl.haxx.se/changes.html), rather than 2003 like for 7.10.
> >
> > The consequences is that a lot of not so old distributions may be 
> > concerned. I only checked Fedora, which does not provide curl > 7.15 
> > before Fedora 7 (issued late may 2007).
> >
> > (BTW you may guess well that I'm using a Fedora Core 6 for my git 
> > patches...)
> 
> Now, that means the patch is not quite good for 1.5.4, and if we want to 
> keep http-push alive (I do not very much care about it myself, though), 
> and make it usable, we would need to fix it for non MULTI case.

IMHO it is safer to disable it for curl < 7.0xa -- even if it affects a 
number of distros -- than to give the illusion that it works, when it does 
not.

As for fixing it in the non-MULTI case, I have a hunch that Mike's 
cleanups will help that, but that this is a 1.5.5 feature.

So, I would like to read in the ReleaseNotes something like this:

-- snip --
Support for pushing via HTTP was broken with curl versions prior to 7.16, 
so we disabled it for now.  However, it is likely that a major cleanup of 
the http transport code -- scheduled after the release of git 1.5.4 -- 
will be supported with more curl versions.
-- snap --

Ciao,
Dscho

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> IMHO it is safer to disable it for curl < 7.0xa -- even if it affects a 
> number of distros -- than to give the illusion that it works, when it does 
> not.
>
> As for fixing it in the non-MULTI case, I have a hunch that Mike's 
> cleanups will help that, but that this is a 1.5.5 feature.
>
> So, I would like to read in the ReleaseNotes something like this:
>
> -- snip --
> Support for pushing via HTTP was broken with curl versions prior to 7.16, 
> so we disabled it for now.  However, it is likely that a major cleanup of 
> the http transport code -- scheduled after the release of git 1.5.4 -- 
> will be supported with more curl versions.
> -- snap --

That's tempting but I suspect that it might be a wrong approach.

I think two important questions are:

 * Do we know that the current code is broken for everybody, or
   just broken for the majority of people who do nontrivial
   things?

 * Is the code in 1.5.3.8 any better?  IOW, did we make it worse
   during 1.5.4 cycle?

The feature was added by one person who needed it, and it was
included because the need was satisfid with an implementation,
so at some point in the past, it must have worked for _somebody_
(I am hoping that this is not a regression during 1.5.4 cycle).

Imagine that you are like that somebody who have been happily
using http-push.  Or imagine that you are starting to use git
and are tempted to use http-push.  With the above wording, I
strongly suspect that you would say "Crap --- 1.5.4 does not let
me run http-push, so I'll stay at 1.5.3.8 until 1.5.X lets me
use it again".

Which is _not_ a solution, if 1.5.3.8 has an http-push that is
broken the same way.  You will be choosing a version with the
same brokenness with respect to http-push, and are missing fixes
we made to http-push during 1.5.4 cycle, let alone fixes and
enhancements to other programs that comes with 1.5.4.

So while I strongly agree that we should warn the users about
existing breakages, I think it is better to just revert the code
to limit its use to USE_CURL_MULTI, if that is the case.

Do we even know what exactly is broken?

On the other hand, if the "transport.c" rewrite broke it and the
current one for 1.5.4 is fundamentally much worse than what we
used to have in 1.5.3.8, would it be possible as an interim
measure to revert http-push changes (but keep changes to other
programs that already are converetd to use transport.c) so that
we can ship the same code as 1.5.3.8 only for http-push?
Perhaps copy in selected old sources in a subdirectory to build
and link a standalone http-push program?

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Mike Hommey]

On Mon, Jan 21, 2008 at 12:18:14PM -0800, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > IMHO it is safer to disable it for curl < 7.0xa -- even if it affects a 
> > number of distros -- than to give the illusion that it works, when it does 
> > not.
> >
> > As for fixing it in the non-MULTI case, I have a hunch that Mike's 
> > cleanups will help that, but that this is a 1.5.5 feature.
> >
> > So, I would like to read in the ReleaseNotes something like this:
> >
> > -- snip --
> > Support for pushing via HTTP was broken with curl versions prior to 7.16, 
> > so we disabled it for now.  However, it is likely that a major cleanup of 
> > the http transport code -- scheduled after the release of git 1.5.4 -- 
> > will be supported with more curl versions.
> > -- snap --
> 
> That's tempting but I suspect that it might be a wrong approach.
> 
> I think two important questions are:
> 
>  * Do we know that the current code is broken for everybody, or
>    just broken for the majority of people who do nontrivial
>    things?

IIRC, http-push simply doesn't work without CURL_MULTI. 

>  * Is the code in 1.5.3.8 any better?  IOW, did we make it worse
>    during 1.5.4 cycle?

Changes in http-push.c since 1.5.3.8 mostly involve cleanup. It
didn't change anything about CURL_MULTI or lack thereof.

Mike

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Johannes Schindelin]

Hi,

On Mon, 21 Jan 2008, Mike Hommey wrote:

> On Mon, Jan 21, 2008 at 12:18:14PM -0800, Junio C Hamano wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > IMHO it is safer to disable it for curl < 7.0xa -- even if it affects a 
> > > number of distros -- than to give the illusion that it works, when it does 
> > > not.
> > >
> > > As for fixing it in the non-MULTI case, I have a hunch that Mike's 
> > > cleanups will help that, but that this is a 1.5.5 feature.
> > >
> > > So, I would like to read in the ReleaseNotes something like this:
> > >
> > > -- snip --
> > > Support for pushing via HTTP was broken with curl versions prior to 7.16, 
> > > so we disabled it for now.  However, it is likely that a major cleanup of 
> > > the http transport code -- scheduled after the release of git 1.5.4 -- 
> > > will be supported with more curl versions.
> > > -- snap --
> > 
> > That's tempting but I suspect that it might be a wrong approach.
> > 
> > I think two important questions are:
> > 
> >  * Do we know that the current code is broken for everybody, or
> >    just broken for the majority of people who do nontrivial
> >    things?
> 
> IIRC, http-push simply doesn't work without CURL_MULTI. 

I have to agree.  When I last tried without CURL_MULTI (IIRC it was just 
once, when I had an ancient curl available), it would just not work, and I 
gave up/in and installed a newer curl, thus enabling CURL_MULTI.

> >  * Is the code in 1.5.3.8 any better?  IOW, did we make it worse
> >    during 1.5.4 cycle?
> 
> Changes in http-push.c since 1.5.3.8 mostly involve cleanup. It
> didn't change anything about CURL_MULTI or lack thereof.

I meant to look into http-push and curl_multi, ever since Daniel asked me 
(or for that matter, other people knowing about the issues) do do it.

Alas, I forgot about it.

So I am half-convinced that http-push w/o CURL_MULTI was broken since long 
ago (pre 1.5.3).

I'll try tomorrow, since I have a (kinda) working http-push setup 
available then.

Ciao,
Dscho

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> So I am half-convinced that http-push w/o CURL_MULTI was broken since long 
> ago (pre 1.5.3).

Sigh, but Ok.  Then let's do this.

-- >8 --
Clarify that http-push being temporarily disabled with older cURL

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/RelNotes-1.5.4.txt |   14 ++++++++++++--
 Documentation/git-http-push.txt  |    3 +++
 http.h                           |    8 ++++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/Documentation/RelNotes-1.5.4.txt b/Documentation/RelNotes-1.5.4.txt
index 9c864c9..b3cc5e0 100644
--- a/Documentation/RelNotes-1.5.4.txt
+++ b/Documentation/RelNotes-1.5.4.txt
@@ -10,8 +10,18 @@ Removal
  * As git-commit and git-status have been rewritten, "git runstatus"
    helper script lost all its users and has been removed.
 
- * Curl library older than 7.10 is not supported by "git http-push",
-   as it does not work without CURLM.
+
+Temporarily Disabled
+--------------------
+
+ * "git http-push" is known not to work well with cURL library older
+   than 7.16, and we had reports of repository corruption.  It is
+   disabled on such platforms for now.  Unfortunately, 1.5.3.8 shares
+   the same issue.  In other words, this does not mean you will be
+   fine if you stick to an older git release.  For now, please do not
+   use http-push from older git with cURL older than 7.16 if you
+   value your data. A proper fix will hopefully materialize in
+   later versions.
 
 
 Deprecation notices
diff --git a/Documentation/git-http-push.txt b/Documentation/git-http-push.txt
index cca77f1..0b82722 100644
--- a/Documentation/git-http-push.txt
+++ b/Documentation/git-http-push.txt
@@ -15,6 +15,9 @@ DESCRIPTION
 Sends missing objects to remote repository, and updates the
 remote branch.
 
+*NOTE*: This command is temporarily disabled if your cURL
+library is older than 7.16, as the combination has been reported
+not to work and sometimes corrupts repository.
 
 OPTIONS
 -------
diff --git a/http.h b/http.h
index aeba930..9bab2c8 100644
--- a/http.h
+++ b/http.h
@@ -8,6 +8,14 @@
 
 #include "strbuf.h"
 
+/*
+ * We detect based on the cURL version if multi-transfer is
+ * usable in this implementation and define this symbol accordingly.
+ * This is not something Makefile should set nor users should pass
+ * via CFLAGS.
+ */
+#undef USE_CURL_MULTI
+
 #if LIBCURL_VERSION_NUM >= 0x071000
 #define USE_CURL_MULTI
 #define DEFAULT_MAX_REQUESTS 5
-- 
1.5.4.rc4.11.g7422b

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Johannes Schindelin]

Hi,

On Mon, 21 Jan 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > So I am half-convinced that http-push w/o CURL_MULTI was broken since 
> > long ago (pre 1.5.3).
> 
> Sigh, but Ok.  Then let's do this.

This sigh hurts my heart.  So I will try to find out what is broken.  
Tomorrow.  If the fix(es) is/are small enough, I hope that they will go 
into 1.5.4.  If it/they is/are not, I will let you know why.

Ciao,
Dscho

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Mon, 21 Jan 2008, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > So I am half-convinced that http-push w/o CURL_MULTI was broken since 
>> > long ago (pre 1.5.3).
>> 
>> Sigh, but Ok.  Then let's do this.
>
> This sigh hurts my heart.

Heh, it's not like you broke it, and if I sounded like I was
shooting at the messenger I apologize.

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Johannes Schindelin]

Hi,

On Mon, 21 Jan 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Mon, 21 Jan 2008, Junio C Hamano wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> 
> >> > So I am half-convinced that http-push w/o CURL_MULTI was broken 
> >> > since long ago (pre 1.5.3).
> >> 
> >> Sigh, but Ok.  Then let's do this.
> >
> > This sigh hurts my heart.
> 
> Heh, it's not like you broke it, and if I sounded like I was shooting at 
> the messenger I apologize.

No, no, that's alright.  I am the messenger alright, but it seems that I 
am in a relatively rare position to do something about the sad state of 
http-push.

Probably with the help of Gregoire and Mike ;-)

Ciao,
Dscho

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Daniel Barkalow]

On Mon, 21 Jan 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > IMHO it is safer to disable it for curl < 7.0xa -- even if it affects a 
> > number of distros -- than to give the illusion that it works, when it does 
> > not.
> >
> > As for fixing it in the non-MULTI case, I have a hunch that Mike's 
> > cleanups will help that, but that this is a 1.5.5 feature.
> >
> > So, I would like to read in the ReleaseNotes something like this:
> >
> > -- snip --
> > Support for pushing via HTTP was broken with curl versions prior to 7.16, 
> > so we disabled it for now.  However, it is likely that a major cleanup of 
> > the http transport code -- scheduled after the release of git 1.5.4 -- 
> > will be supported with more curl versions.
> > -- snap --
> 
> That's tempting but I suspect that it might be a wrong approach.
> 
> I think two important questions are:
> 
>  * Do we know that the current code is broken for everybody, or
>    just broken for the majority of people who do nontrivial
>    things?
> 
>  * Is the code in 1.5.3.8 any better?  IOW, did we make it worse
>    during 1.5.4 cycle?

I believe that the move to transport.c didn't change anything except 
cleaning up linking conflicts and moving the dispatch by URL method code. 
I suppose something could have gotten messed up in dealing with the 
linking conflicts, but I don't think it actually did.

I think that the bad combination is requests getting aborted and 
USE_CURL_MULTI and early curl versions. I think that requests getting 
aborted is not normal, anyway, but not something easy for users to debug 
if it happens, and possible to have happen to anybody.

I don't really know much about http-push, and don't have a testing setup 
for it, so I can't really say if it works without USE_CURL_MULTI or how 
hard it would be to make it work.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Mon, 21 Jan 2008, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > IMHO it is safer to disable it for curl < 7.0xa -- even if it affects a 
>> > number of distros -- than to give the illusion that it works, when it does 
>> > not.
>> >
>> > As for fixing it in the non-MULTI case, I have a hunch that Mike's 
>> > cleanups will help that, but that this is a 1.5.5 feature.
>> >
>> > So, I would like to read in the ReleaseNotes something like this:
>> >
>> > -- snip --
>> > Support for pushing via HTTP was broken with curl versions prior to 7.16, 
>> > so we disabled it for now.  However, it is likely that a major cleanup of 
>> > the http transport code -- scheduled after the release of git 1.5.4 -- 
>> > will be supported with more curl versions.
>> > -- snap --
>> 
>> That's tempting but I suspect that it might be a wrong approach.
>> 
>> I think two important questions are:
>> 
>>  * Do we know that the current code is broken for everybody, or
>>    just broken for the majority of people who do nontrivial
>>    things?
>> 
>>  * Is the code in 1.5.3.8 any better?  IOW, did we make it worse
>>    during 1.5.4 cycle?
>
> I believe that the move to transport.c didn't change anything except 
> cleaning up linking conflicts and moving the dispatch by URL method code. 
> I suppose something could have gotten messed up in dealing with the 
> linking conflicts, but I don't think it actually did.

Ok, so copying 1.5.3.8 http-push to include in 1.5.4 would not
make it work, it sounds like.  Then I guess Dscho's notice (and
the same notice with disabling http-push without MULTI in
1.5.3.9) would be the sane thing we should do in the short term.

Re: [PATCH] http-push: making HTTP push more robust and more user-friendly

[Grégoire Barbier]

Junio C Hamano a écrit > * Do we know that the current code is broken 
for everybody, or just
> broken for the majority of people who do nontrivial things?

http-push without USE_CURL_MULTI is broken for everybody who uses it

> * Is the code in 1.5.3.8 any better?  IOW, did we make it worse 
> during 1.5.4 cycle?

I think it is better, because it hurts less.

after:
- http-push with curl >= 7.16 works
- http-push with curl < 7.16 does not work

before
- http-push with curl >= 7.16 works
- http-push with curl < 7.16 does not work and in addition corrups repos

In addition, the sooner we disable the repo corrupting code, the less we
will have dangerous code in the wide

Junio C Hamano a écrit :
> Then I guess Dscho's notice (and the same notice with disabling 
> http-push without MULTI in 1.5.3.9) would be the sane thing we should
> do in the short term.

This is my opinion.

Junio C Hamano a écrit :
 > The feature was added by one person who needed it, and it was
 > included because the need was satisfid with an implementation,
 > so at some point in the past, it must have worked for _somebody_
 > (I am hoping that this is not a regression during 1.5.4 cycle).
 >
 > Imagine that you are like that somebody who have been happily
 > using http-push.  Or imagine that you are starting to use git
 > and are tempted to use http-push.  With the above wording, I
 > strongly suspect that you would say "Crap --- 1.5.4 does not let
 > me run http-push, so I'll stay at 1.5.3.8 until 1.5.X lets me
 > use it again".

My experience is that 1.5.3.6 is broken too (but I did not ever try 
1.5.3.8). Therefore I don't think it's an 1.5.4 regression.

In fact, a few weeks ago, I was that guy discovering git and trying to 
use http-push, and said "Crap. That thing is broken." And this why I'm 
bothering you all since a while.

-- 
Grégoire Barbier - gb à gbarbier.org - +33 6 21 35 73 49