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

Johannes Schindelin [off-list ref] 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?

On Mon, Jan 21, 2008 at 12:18:14PM -0800, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

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

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

Johannes Schindelin [off-list ref] writes:

quoted

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*

Hi,

On Mon, 21 Jan 2008, Mike Hommey wrote:

On Mon, Jan 21, 2008 at 12:18:14PM -0800, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

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.

quoted

 * 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