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

Hi Junio, Hi Johannes,

I recently sent three patches about http-push:

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:

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 :

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 :

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 :

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 :

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.