Grégoire Barbier [off-list ref] 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?
Hi Junio, Hi Johannes,
I recently sent three patches about http-push:
$gmane/70406 [off-list ref]
$gmane/70407 [off-list ref]
$gmane/70405 [off-list ref]
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 [off-list ref] writes:
quoted
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
Hi,
On Sat, 19 Jan 2008, Grégoire Barbier wrote:
Johannes Schindelin a écrit :
quoted
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 :
quoted
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