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

["Grégoire Barbier" <gb@gbarbier.org>]

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