[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

[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.

To reproduce the issue that this patch fixes, you need a test git repository
availlable over http+webdav, let's say at http://myhost/myrepo.git/
Then, you do this:
$ git clone http://myhost/myrepo.git/
$ cd myrepo
$ git push http
Fetching remote heads...
  refs/
  refs/heads/
  refs/tags/
No refs in common and none specified; doing nothing.
$ git push http
Fetching remote heads...
  refs/
  refs/heads/
  refs/tags/
No refs in common and none specified; doing nothing.
$

Finally, you look at the web server logs, and will find one LOCK query and no
UNLOCK query, of course the second one will be in 423 return code instead of
200:
1.2.3.4 - gb [19/Jan/2008:14:24:56 +0100] "LOCK /myrepo.git/info/refs HTTP/1.1" 200 465
(...)
1.2.3.4 - gb [19/Jan/2008:14:25:10 +0100] "LOCK /myrepo.git/info/refs HTTP/1.1" 423 363

With this patch, there would have be two UNLOCKs in addition of the LOCKs

From the user point of view:
- If you realize that you should have typed e.g. "git push http master"
instead of "git push http", you will have to wait for 10 minutes for the lock
to expire by its own.
- Furthermore, if somebody else is dumb enough to type "git push http" while
you need to push "master" branch, then you'll need too to wait for 10 minutes
too.

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 eef7674..2c4e91d 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2264,11 +2264,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;
@@ -2399,10 +2402,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.4.rc3.52.g9a5bd-dirty

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

[Johannes Schindelin]

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

Hi,

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

> diff --git a/http-push.c b/http-push.c
> index eef7674..2c4e91d 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -2264,11 +2264,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;
> @@ -2399,10 +2402,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);

This late in the rc cycle, together with my unfamiliarity of the code and 
the code paths in http.c and http-push.c would make me feel _much_ better 
if you could insert the "if (info_ref_lock)" before the returns, instead 
of replacing the returns with "goto cleanup"s...

Thanks,
Dscho

Allowing override of the default "origin" nickname

[Mark Levedahl]

git's current support for remote nicknames other than
"origin" is restricted to tracking branches where
branch.<name>.remote is defined. This does not work on
detached heads, and thus does not work for managed
submodules as those are kept on detached heads. When working
with submodules, the remote must be called "origin."

As my project is distributed across multiple domains with
many firewalls and airgaps such that no single server is
available to all, we really need to use nicknames to refer
to different servers, and we need that to work well with
submodules.

So, this patch series:
1) defines a new "remotes.default" config variable per
repository to be the default remote used if no
branch.<name>.remote is found.

2) teaches clone to set remotes.default according to
the user's command (via -o).

3) teaches remote rm to unset remotes.default if deleting
that remote.

4) teaches git-submodule to propoagate the parent's default
branch to submoules during "init", IFF those modules are
defined using relative urls. (Modules using absolute urls
are likely from a different server, so this inheritence is
not likely the right thing in that case.)


This is working well for me, allowing

        git clone -o myserver <url> project
        cd project
        git submodule init
        git submoule update

to work as expected, with all submodules pointing to
"myserver" rather than "origin" and updating correctly despite
being on detached heads.

Mark Levedahl

[PATCH] Teach remote machinery about remotes.default config variable

[Mark Levedahl]

This introduces a new configuration variable, remotes.default, that
defines the name of the default remote to be used. Traditionally, this
is "origin", and could be overridden for a given branch. This change
introduces a way to redefine the default as desired and have that honored
regardless of the currently checked out head (e.g., remotes.default is
used when on a detached head or any other non-tracking branch).

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
 Documentation/config.txt |    6 ++++++
 git-parse-remote.sh      |    5 +++--
 remote.c                 |   11 ++++++++++-
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1b6d6d6..01ce295 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -800,6 +800,12 @@ remote.<name>.tagopt::
 	Setting this value to --no-tags disables automatic tag following when fetching
 	from remote <name>
 
+remotes.default::
+	The name of the remote used by default for fetch / pull. If unset,
+	origin is assumed. This value is used whenever the current branch
+	has no corresponding branch.<name>.remote, such as when working on
+	a detached head.
+
 remotes.<group>::
 	The list of remotes which are fetched by "git remote update
 	<group>".  See linkgit:git-remote[1].
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 695a409..1b235e0 100755
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -56,8 +56,9 @@ get_remote_url () {
 
 get_default_remote () {
 	curr_branch=$(git symbolic-ref -q HEAD | sed -e 's|^refs/heads/||')
-	origin=$(git config --get "branch.$curr_branch.remote")
-	echo ${origin:-origin}
+	git config --get "branch.$curr_branch.remote" ||
+	git config --get "remotes.default" ||
+	echo origin
 }
 
 get_remote_default_refs_for_push () {
diff --git a/remote.c b/remote.c
index 0e00680..4937237 100644
--- a/remote.c
+++ b/remote.c
@@ -10,6 +10,7 @@ static int allocated_branches;
 
 static struct branch *current_branch;
 static const char *default_remote_name;
+static const char *remotes_default_name;
 
 #define BUF_SIZE (2048)
 static char buffer[BUF_SIZE];
@@ -233,6 +234,11 @@ static int handle_config(const char *key, const char *value)
 			add_merge(branch, xstrdup(value));
 		return 0;
 	}
+	if (!strcmp(key, "remotes.default")) {
+		if (value)
+			remotes_default_name = xstrdup(value);
+		return 0;
+	}
 	if (prefixcmp(key,  "remote."))
 		return 0;
 	name = key + 7;
@@ -291,7 +297,6 @@ static void read_config(void)
 	int flag;
 	if (default_remote_name) // did this already
 		return;
-	default_remote_name = xstrdup("origin");
 	current_branch = NULL;
 	head_ref = resolve_ref("HEAD", sha1, 0, &flag);
 	if (head_ref && (flag & REF_ISSYMREF) &&
@@ -300,6 +305,10 @@ static void read_config(void)
 			make_branch(head_ref + strlen("refs/heads/"), 0);
 	}
 	git_config(handle_config);
+	if (!default_remote_name) {
+			default_remote_name = remotes_default_name ?
+				remotes_default_name : xstrdup("origin");
+	}
 }
 
 struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)
-- 
1.5.4.rc2.99.g3ef7-dirty

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> This introduces a new configuration variable, remotes.default, that
> defines the name of the default remote to be used.

Does this mean "default" is now a new reserved word that cannot
be used as "git remote update default"?

> ... Traditionally, this
> is "origin", and could be overridden for a given branch. This change
> introduces a way to redefine the default as desired and have that honored
> regardless of the currently checked out head (e.g., remotes.default is
> used when on a detached head or any other non-tracking branch).

I'd 100% agree that being able to use anything not just
hardcoded 'origin' is much better than not being able to.  I do
not have much against that goal.

However, it is a bit hard to judge how much of inconvenience it
really is in your real life that the current behaviour does not
allow you to.

In your cover letter, you said:

>> As my project is distributed across multiple domains with
>> many firewalls and airgaps such that no single server is
>> available to all, we really need to use nicknames to refer
>> to different servers,...

If you need to access different repositories on different
machines from your submodules, you would of course need to
access different domains from your submodule repositories.  But
that does not mean each of them cannot be named 'origin'.  That
name is local to each of the submodule (and the toplevel) and
can point at different domains over different transfer channels.

> diff --git a/git-parse-remote.sh b/git-parse-remote.sh
> index 695a409..1b235e0 100755
> --- a/git-parse-remote.sh
> +++ b/git-parse-remote.sh
> @@ -56,8 +56,9 @@ get_remote_url () {
>  
>  get_default_remote () {
>  	curr_branch=$(git symbolic-ref -q HEAD | sed -e 's|^refs/heads/||')
> -	origin=$(git config --get "branch.$curr_branch.remote")
> -	echo ${origin:-origin}
> +	git config --get "branch.$curr_branch.remote" ||
> +	git config --get "remotes.default" ||
> +	echo origin

This sequence cascaded with || is much nicer than the original,
even if it did not change the behaviour.

> @@ -300,6 +305,10 @@ static void read_config(void)
>  			make_branch(head_ref + strlen("refs/heads/"), 0);
>  	}
>  	git_config(handle_config);
> +	if (!default_remote_name) {
> +			default_remote_name = remotes_default_name ?
> +				remotes_default_name : xstrdup("origin");
> +	}

Is this a bit too deep indentation?

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Mark Levedahl]

On Jan 11, 2008 3:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Does this mean "default" is now a new reserved word that cannot
> be used as "git remote update default"?

oops...git-remote already has a (partially undocumented) use for
remotes.* as well as remote.*, so I need another variable name,
probably core.origin to avoid either defining new namespace or
polluting one reserved for arbitrary end-user use. Will resend patches
later tonight.

>
> However, it is a bit hard to judge how much of inconvenience it
> really is in your real life that the current behaviour does not
> allow you to.

I believe I addressed this in the thread with Dscho.

> >       git_config(handle_config);
> > +     if (!default_remote_name) {
> > +                     default_remote_name = remotes_default_name ?
> > +                             remotes_default_name : xstrdup("origin");
> > +     }
>
> Is this a bit too deep indentation?
>

will fix.


Mark

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Junio C Hamano]

"Mark Levedahl" <mlevedahl@gmail.com> writes:

>> However, it is a bit hard to judge how much of inconvenience it
>> really is in your real life that the current behaviour does not
>> allow you to.
>
> I believe I addressed this in the thread with Dscho.

Thanks.

I have to admit that I happen to agree with Dscho.  I do not see
this helping to solve communication issues very much.

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Mark Levedahl]

Junio C Hamano wrote:
> "Mark Levedahl" <mlevedahl@gmail.com> writes:
>   
> Thanks.
>
> I have to admit that I happen to agree with Dscho.  I do not see
> this helping to solve communication issues very much.
>   
Junio,

My use really is a different use-case than is typical. Origin is a great 
concept for the common case of projects with a single upstream 
repository. Except for cloning, you don't have to know or care the name 
of the upstream as you move from project to project, it is just always 
"origin" and you use the same remote nickname in each.

This breaks down in a project like mine where there are multiple servers 
and the differences are important. Content and usage vary server to 
server, not just connectivity. At this point, hiding the server names is 
counterproductive. Basically, use of origin is data hiding, and data 
hiding is not good when you actually need the data.

Across the git project, I believe everyone basically understands origin 
as git.kernel.org/..., and origin is not ambiguous. There is just one 
server. For my project, there are multiple servers and a number of us 
pull from and push to multiple servers with no intent that any one 
server has everything (This multiplicity is necessary for several 
reasons, and we have various guards in place restrict the content of 
different servers). Thus, there really is no usefully defined *origin*. 
There just isn't. This is where the disagreements lie.

The argument against my approach of explicitly naming the server rests 
upon the premise that hiding a half-dozen servers, all different and 
with those differences being important, under the single universal name 
"origin", makes things easier. It doesn't when different servers are 
different. Yes, it is possible to figure out what "origin" means at a 
given client, and thus understand how to address a given server from 
that client. That is the essence of the problem. It is clear to address 
server1 as "server1", and server3 as "server3." It is not helpful to 
sometimes refer to server1 as origin, sometimes as server3, and thus 
need to know the definition of origin to know how to name the server. 
For the "normal" git use-case the specific definition of origin is 
unimportant when you use it and so provides a useful abstraction. That I 
must know what origin means in order to know what to do indicates the 
abstraction is counter-productive.

Until we started using sub-modules, we used git clone --origin 
<nickname> and per our standard usage never even had "origin" defined. 
We just agreed on a common set of nicknames for our servers and used 
those. Not everyone had all the remotes defined, but nickname "foo" 
meant the same thing everywhere it was defined. That worked very well 
for us.

So, all I am doing here is trying to extend a basic multi-server 
capability git already has for a monolithic project into projects using 
sub-modules. This will let us resume working the way we did before and 
stop overloading a single nickname (origin) with multiple meanings.

Mark

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> My use really is a different use-case than is typical....
>
> This breaks down in a project like mine where there are multiple
> servers and the differences are important. Content and usage vary
> server to server, not just connectivity. At this point, hiding the
> server names is counterproductive. Basically, use of origin is data
> hiding, and data hiding is not good when you actually need the data.

If you need explicit name, you do not have to use "origin".

You can spell URL explicitly to name which exact repository you
mean to reach over which datapath (one physical host may have
different name depending on the network interface you reach it
via).  You can always say

	$ git pull git://that.exact.machine/repo that-branch

if you want to avoid ambiguity.

And that is not atypical at all.  Scan the kernel mailing list,
looking for "please pull" requests.  You will never see 'origin'
or any short nickname.  The names used in communication should
be unambiguous in the context of the communication.  If you know
'origin' mean different things to different people, do not use
that in public communication.

It's that simple.  Isn't it?

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Mark Levedahl]

Junio C Hamano wrote:
> Mark Levedahl <mlevedahl@gmail.com> writes:
>
>
>   
> It's that simple.  Isn't it?
>
>   
Yes, until you hit submodules whose state you are managing from a super 
project. Then it gets hard because the machinery brings origin into play.

Mark

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> Junio C Hamano wrote:
>> Mark Levedahl <mlevedahl@gmail.com> writes:
>>
>>   It's that simple.  Isn't it?
>>
> Yes, until you hit submodules whose state you are managing from a
> super project. Then it gets hard because the machinery brings origin
> into play.

Sorry, I may be missing something.

Even if you have a submodule, you can go there and that will be
a valid freestanding repository.  You can always be explicit,
bypassing any behaviour that defaults to 'origin' to avoid
ambiguity.

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Mark Levedahl]

Junio C Hamano wrote:
> Sorry, I may be missing something.
>
> Even if you have a submodule, you can go there and that will be
> a valid freestanding repository.  You can always be explicit,
> bypassing any behaviour that defaults to 'origin' to avoid
> ambiguity.
>   
"git-submodule update" *requires* that origin is defined in all 
sub-modules. There is no way to avoid this behavior.

Mark

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> Junio C Hamano wrote:
>> Sorry, I may be missing something.
>>
>> Even if you have a submodule, you can go there and that will be
>> a valid freestanding repository.  You can always be explicit,
>> bypassing any behaviour that defaults to 'origin' to avoid
>> ambiguity.
>>
> "git-submodule update" *requires* that origin is defined in all
> sub-modules. There is no way to avoid this behavior.

Ahh.

Does that suggest the new configuration thing is only about the
"submodule update" command, not "remotes.default" that affects
how the non-submodule merge and fetch works?

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Mark Levedahl]

Junio C Hamano wrote:
> Ahh.
>
> Does that suggest the new configuration thing is only about the
> "submodule update" command, not "remotes.default" that affects
> how the non-submodule merge and fetch works?
>
>   
Yes - this patch set was inspired by the single question of "how do I 
avoid needing to define origin as opposed to a server-specific nickname 
now that I am using sub-modules?"

Mark

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> Junio C Hamano wrote:
>> Ahh.
>>
>> Does that suggest the new configuration thing is only about the
>> "submodule update" command, not "remotes.default" that affects
>> how the non-submodule merge and fetch works?
>>
>>
> Yes - this patch set was inspired by the single question of "how do I
> avoid needing to define origin as opposed to a server-specific
> nickname now that I am using sub-modules?"

If it is truly only about "submodule update" then the change
seems too intrusive, especially "remotes.default" variable that
affects the way how fetch and merge works in situations that do
not involve submodules.

If it is not limited to "submodule update" but equally valid fix
to non-submodule situations, the changes to the other parts may
very well be justifiable, but that would mean your "Yes" is a
lie and instead should be "No, but these situations are helped
by these changes because...".

In any case, let's step back a bit.

Earlier you said in a response to Dscho that your servers are
named consistently across repositories.  servername.foo.bar has
nickname servername everywhere.

If your top-level repository needs to access a specific server
"frotz.foo.bar" for updates, then you would have bootstrapped
the whole thing with:

	$ git clone git://frotz.foo.bar/toplevel.git

and in that particular instance of the repository, the source
repository on frotz.foo.bar would have been known as 'origin',
right?  I would not object if you also gave another nickname
'frotz' to the same repository for consistency across
developers.

If that is the case, I am wondering why your subprojects are not
pointing at the corresponding repository on that same
'frotz.foo.bar' machine as 'origin'.  I suspect the reason is
that .gitmodules do not say 'frotz.foo.bar' but name some other
machine.

And in-tree .gitmodules can name only one URL, as it is project
global and shared by everybody.  There is no escaping it.

At least as things were designed, "git submodule init" takes URL
recorded in .gitmodules as a hint, but this is for the user to
override in .git/config in the top-level.  Maybe the UI to allow
this overriding is not easy enough to use, and your submodules
ended up pointing at wrong (from the machine's point of view)
URL as 'origin'.  And perhaps that is the root cause of this
issue?

I am looking at the discussion on the list archive when we
discussed the initial design of .gitmodules:

    http://thread.gmane.org/gmane.comp.version-control.git/47466/focus=47502
    http://thread.gmane.org/gmane.comp.version-control.git/47466/focus=47548
    http://thread.gmane.org/gmane.comp.version-control.git/47466/focus=47621

I do not think we are there yet, and suspect that the current
"git submodule init" does not give the user a chance to say "the
URL recorded in the in-tree .gitmodules corresponds to this URL
in this repository for administrative or network connectivity or
whatever reasons".

Maybe that is the real issue that we should be tackling.  I
dunno.

Although I _think_ being able to use nickname other than
hardcoded 'origin' for fetch/merge is a good change, if my above
suspicion is correct, that change alone would not make the life
easier to people who _use_ submodules, as the need for them to
set up extra nicknames (like 'frotz') and configure the
submodule repositories to use that specific nickname instead of
'origin' would not change.

For communication purposes, I would agree with Dscho that the
name 'origin' that names different things for different people
is wrong and using specific name 'frotz' would solve
communication issues.  But when using the repository and doing
actual work, wouldn't it be _much_ better if you can
consistently go to a repository on a random machine and always
can say 'origin' to mean the other repository this repository
usually gets new objects from (and sends its new objects to)?

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Mark Levedahl]

Junio C Hamano wrote:
> If it is truly only about "submodule update" then the change
> seems too intrusive, especially "remotes.default" variable that
> affects the way how fetch and merge works in situations that do
> not involve submodules.
> If it is not limited to "submodule update" but equally valid fix
> to non-submodule situations, the changes to the other parts may
> very well be justifiable, but that would mean your "Yes" is a
> lie and instead should be "No, but these situations are helped
> by these changes because...".
>
>   
First, I resent the patch series last night, it now uses core.origin to 
avoid touching remotes.* namespace.

The changes *do* fix a nit when on a non-tracking branch. With this, 
fetch / merge / pull will now honor that the user said (via git clone -o 
frotz) "my upstream is nicknamed frotz" and not try to use origin when 
origin was never defined.

So, while fixing this minor aggravation wasn't my motivation, I view 
this as a nice side-benefit :^).

The driving issues:
1) I deal with too many servers for "origin" to be a useful nick name, 
and we have an agreed set of nickname / server pairings across my project.
2) Therefore, we always do git clone -o frotz  frotz.foo.bar/path_to_git.
3) Because of 2, for top-level, "origin" is not defined, tracking 
branches set up via git branch --track point to the correct remote, and 
we basically understand branch names as <nickname>/branch. In other 
words, we *are* aware of what server we are using.
4) git-submodule update breaks the above:
- a) it invokes git clone frotz.foo.bar/path_to_git thus defining 
"origin" as the nickname for frotz.foo.bar.
  b) it invokes bare git-fetch on a detached head, so the upstream *has* 
to be origin.

> If your top-level repository needs to access a specific server
> "frotz.foo.bar" for updates, then you would have bootstrapped
> the whole thing with:
>
> 	$ git clone git://frotz.foo.bar/toplevel.git
>
> and in that particular instance of the repository, the source
> repository on frotz.foo.bar would have been known as 'origin',
> right? 
Nope, we did it with git clone -o frotz git://frotz.foo.bar/toplevel.git
We *never* define origin, frozt.foo.bar is *always* frotz.

>  I would not object if you also gave another nickname
> 'frotz' to the same repository for consistency across
> developers.
>   
good. We are making (some) progress. :^)
> If that is the case, I am wondering why your subprojects are not
> pointing at the corresponding repository on that same
> 'frotz.foo.bar' machine as 'origin'.  I suspect the reason is
> that .gitmodules do not say 'frotz.foo.bar' but name some other
> machine.
>   
Actually,
1) We don't use origin because we avoid having to wonder "Is 
frotz.foo.bar named "origin" or "frotz" on this client, and thus how do 
I get data from frotz?
2) I submitted the change allowing submodules to be recorded into 
.gitmodules with a relative url (e.g., ./path_from_parent_to_submodule) 
rather than an absolute, so we record the relative path only.
3) Thus, git submodule has set up the submodules to point at the parent 
project's default remote. However, in the parent the server is nicknamed 
"frotz", but now in the submodule the server is nicknamed "origin"  Oops.

With my patches, parent and submodule both refer to frotz.foo.bar as frotz.

> And in-tree .gitmodules can name only one URL, as it is project
> global and shared by everybody.  There is no escaping it.
> At least as things were designed, "git submodule init" takes URL
> recorded in .gitmodules as a hint, but this is for the user to
> override in .git/config in the top-level.  Maybe the UI to allow
> this overriding is not easy enough to use, and your submodules
> ended up pointing at wrong (from the machine's point of view)
> URL as 'origin'.  And perhaps that is the root cause of this
> issue?
>
>   
Again, the relative-url patch was to address this so that a project that 
is mirrored to another server remains valid on the new server without 
modifying the .gitmodules in-tree. (Yes, I know you *can* modify 
information in a given clones .git/config, but I'm trying to avoid such 
manual per clone/checkout modifications where it can reasonably be done.).

Basically, I think an important (but not complete) test of the design is 
that

    git clone -o frotz git://frotz.foo.bar/myproject.git
    cd myproject
    git submodule init
    git submodule update

work, with origin = frotz throughout the submodules, and with the whole 
project correctly checked out even if the entire project was rehosted 
onto a different server. With relative urls and my latest patch series 
last night, this all works, and of course upstream can still be "origin" 
if that is what is desired.

While our overall project exists on many servers, mirroring is an 
incorrect term. Rather, only certain branches of  various parts exist 
everywhere, many other branches are specific to a given server, so we 
really name branches using servername/branchname. It is this aspect of 
the project that causes us to be aware of the server in use, and thus 
makes use of "origin" as a generic upstream not useful.

> I am looking at the discussion on the list archive when we
> discussed the initial design of .gitmodules:
>
>     http://thread.gmane.org/gmane.comp.version-control.git/47466/focus=47502
>     http://thread.gmane.org/gmane.comp.version-control.git/47466/focus=47548
>     http://thread.gmane.org/gmane.comp.version-control.git/47466/focus=47621
>
> I do not think we are there yet, and suspect that the current
> "git submodule init" does not give the user a chance to say "the
> URL recorded in the in-tree .gitmodules corresponds to this URL
> in this repository for administrative or network connectivity or
> whatever reasons".
>   
> Maybe that is the real issue that we should be tackling.  I
> dunno.
>
> Although I _think_ being able to use nickname other than
> hardcoded 'origin' for fetch/merge is a good change, if my above
> suspicion is correct, that change alone would not make the life
> easier to people who _use_ submodules, as the need for them to
> set up extra nicknames (like 'frotz') and configure the
> submodule repositories to use that specific nickname instead of
> 'origin' would not change.
>
>   

git-submodule right now supports two different layouts (urls relative to 
the parent, and absolute urls such that each sub-module is on an 
independent server). The management approaches to these are going to be 
different.

I also suspect there are two basic use cases here: accumulation of a 
number of independently managed projects vs. splitting a single major 
project into a number of smaller pieces to allow some decoupling, but 
still managing the set as a composite whole.

There may be some direct correlation of use-case and submodule layout, 
don't know. My project uses relative-urls, and I am managing a large 
project that has been split into a number of components. So, my 
suggestions are focused entirely upon this design and use-case, and I 
don't expect I am addressing the others at all. (As usual, this requires 
someone who needs the other model(s) to step up and drive).

For *my* uses (relative urls, single logical project):

1) There are times when the parent's branch.<name>.remote should be 
flowed down to all subprojects for git submodule update, of course this 
would require that the remote be defined for all.
2) Thus, there needs to be a way to define a new remote globally for the 
project, and have it be correctly interpreted by each submodule (e.g., a 
repeat of the relative-url dereferencing now done by submodule init, but 
applied later to all submodules to define a new remote). Yes, this could 
be accomplished by going into each submodule independently and issuing 
appropriate commands, but administration would be much easier given a 
top-level command that could recurse and "do the right thing" per 
sub-project.

I *suspect* that origin is a much more useful concept for the alternate 
construct (absolute urls, loose alliance of separately managed 
projects), but as I said that is not my problem so please ask folks who 
have that model to define what works for them.
> For communication purposes, I would agree with Dscho that the
> name 'origin' that names different things for different people
> is wrong and using specific name 'frotz' would solve
> communication issues.  But when using the repository and doing
> actual work, wouldn't it be _much_ better if you can
> consistently go to a repository on a random machine and always
> can say 'origin' to mean the other repository this repository
> usually gets new objects from (and sends its new objects to)?
>
>
>   
(Acutally, I thought I was the one arguing that using origin when it 
means different things to different folks is not good. That's the root 
of my problems. :^) )

Anyway, I have not found any use of "origin" on my project really 
useful. We have to be and *are* aware of the server/branchname in use, 
not just the branch. Partly this is because different subgroups have 
different natural gathering points (we tend to exchange data via ad hoc 
"mob" branches on whatever server is most accessible to the particular 
group), and partly because some information simply cannot be allowed on 
some servers, but basically the more accessible a server is, the less 
information that server can have. I believe "origin" is really useful 
only when it has just one meaning, or when all values are effectively 
identical (e.g., you have several mirrors for load balancing, etc, but 
all are identical modulo mirroring delays).

OTOH, a reasonable change to the semantics of "origin" might be to have:
1) core.origin name the remote that is the "normal" upstream.
2) Reserve and allow use of the name "origin" to mean $core.origin, 
e.g., in shell scripts replace all references to remote "origin" with 
$(git config core.origin). Of course, if core.origin = origin, then no 
user visible change occurs.

In this way, git would  not record the same remote's branches in two 
ways (as origin/master and as frotz/master), but rather dereference 
origin -> frotz and then get frotz/master. Dunno, no matter how you 
slice it, having more than one way to refer to the same remote is going 
to be confusing, and that's why we don't use origin.


Mark

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> Basically, I think an important (but not complete) test of the design
> is that
>
>    git clone -o frotz git://frotz.foo.bar/myproject.git
>    cd myproject
>    git submodule init
>    git submodule update
>
> work, with origin = frotz throughout the submodules, and with the
> whole project correctly checked out even if the entire project was
> rehosted onto a different server.

I like that.  This is a very good argument, especially because
it clarifies very well that the issue is not about "'submodule
init' misbehaves" but "fetch/pull/merge does not play well with
clone -o".

The only remaining (minor) doubt I have (not in the sense that
"I object to it!", but in the sense that "I wish there could be
a better alternative, but I do not think of one offhand") is
polluting the core.* namespace with this configuration variable.

Looking at Documentation/config.txt, I realize that we already
have made a mistake of allowing core.gitproxy, but other than
that single mistake, everything in core.* is still about things
that apply to the use of git even when the repository does not
talk with any other repository.  If we deprecate and rename away
that one mistake, we can again make core.* to mean things that
are _really_ core, but using core.origin for "the default remote
is not called 'origin' but 'frotz' here" is a step backwards
from that ideal.

But that's a minor naming issue.

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Johannes Schindelin]

Hi,

On Sat, 12 Jan 2008, Junio C Hamano wrote:

> Mark Levedahl <mlevedahl@gmail.com> writes:
> 
> > Basically, I think an important (but not complete) test of the design
> > is that
> >
> >    git clone -o frotz git://frotz.foo.bar/myproject.git
> >    cd myproject
> >    git submodule init
> >    git submodule update
> >
> > work, with origin = frotz throughout the submodules, and with the
> > whole project correctly checked out even if the entire project was
> > rehosted onto a different server.
> 
> I like that.  This is a very good argument, especially because it 
> clarifies very well that the issue is not about "'submodule init' 
> misbehaves" but "fetch/pull/merge does not play well with clone -o".

FWIW I disagree.

I never understood why people want to complicate things by being able to 
name default _keys_ differently.  Why not letting "origin" being the 
default being pulled from, and be done with it?

Besides, I _really_ do not understand why we have such a discussion in rc 
phase.  There are _many_ more interesting discussions now that _also_ do 
not belong into a freeze phase.

Ciao,
Dscho

Re: [PATCH] Teach remote machinery about remotes.default config variable

[Junio C Hamano]

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

> I never understood why people want to complicate things by being able to 
> name default _keys_ differently.  Why not letting "origin" being the 
> default being pulled from, and be done with it?

That happens to match my personal feeling.

HOWEVER.

We treat 'origin' in a special way when you do this:

	$ git clone somewhere new.git
        $ cd new.git
        $ git checkout HEAD^0
        $ git pull

And we already have "clone -o" and claim to support that option.
I think that it is very reasonable from the consistency point of
view to make sure that the following sequence treats 'frotz' the
same special way the above treats 'origin' specially:

	$ git clone -o frotz somewhere new.git
        $ cd new.git
        $ git checkout HEAD^0
        $ git pull

A purist alternative is to deprecate "git clone -o" and
eventually remove it.

Note that I was agreeing only with this specific aspect of the
argument.  I am not at all interested in getting involved in
refining or re-defining the existing submodule semantics this
late in the cycle before 1.5.4.  But I can very well see that
fixing this specific inconsistency can be separated out from the
rest of Mark's series and viewed as a set of trivially correct
fixes.

> Besides, I _really_ do not understand why we have such a
> discussion in rc phase.  There are _many_ more interesting
> discussions now that _also_ do not belong into a freeze phase.

Currently the ones I looked at and consider possible 1.5.4
material are http-push fixes from Grégoire Barbier and
parse_commit_buffer() tightening from Martin Koegler.

Recently I looked at the following patches and topics but I do
not think any of them belongs to 1.5.4.  None of them is obvious
and trivially correct fix to regressions or serious existing
bugs:

 * compress/decompress abstraction (Marco)

 * crlf (Steffen Prohaska and Dmitry Potapov)

 * whitespace error: "cr at eol is ok" (me)

 * various conflicting submodule changes(Ping Yin, Mark
   Levedahl, Imran M Yousuf)

 * unconfigured ident safety (Stephen Sinclair)

 * gitweb feed from commit to commitdiff (Florian La Rouche --
   Jakub seems to be on top of this so I am not worried about it
   too much).

 * color.ui (Matthias Kestenholz)

 * test scripts to use valgrind (Jeff King, but there was another
   one in the past -- can their efforts compared and coordinated
   better?).

 * various lstat(2) reduction changes (me).

 * pathname safety on insane filesystems (Linus, Robin
   Rosenberg, me).

(yes, some of the above list do not even have any code).

I am hoping that authors will resend the ones they really care
about after 1.5.4, as I do not want to take patches early.

Thanks.

What's not in 'master' but should be

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Currently the ones I looked at and consider possible 1.5.4
> material are http-push fixes from Grégoire Barbier and
> parse_commit_buffer() tightening from Martin Koegler.

It seems that for the past few days, people were having too much
fun bashing how broken MacOS X is, and the real work has stalled
in the meantime.  Well, not really stalled but they certainly
made the patches and discussions harder to find in the list
archive.

But that's Ok.  You cannot win every battle.

Now the lack of unsetenv can be autodetected, and coloring
breakage of --color-words has been fixed.  We have also managed
to catch a real breakage in fast-import, but somebody seems to
have managed to bash OS X even in that thread ;-)

But there are still unapplied patches that deserve attention.
The one that I am most worried about is Grégoire Barbier's
http-push changes:

  $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>

They look sensible on paper. I do not, however, use http-push
myself, and I'd really like an independent success (or failure)
reports on them.  I can also threaten to apply them and see if
it breaks for anybody, which I may end up doing.

Martin Koegler's parse_commit_buffer() tightening is much easier:

  $gname/70478 <12003456313661-git-send-email-mkoegler@auto.tuwien.ac.at>

It needs a proper commit message; the patch itself is good.  I
could write one myself but I'd rather want description from the
real contributor.

  gmane = http://news.gmane.org/gmane.comp.version-control.git

Re: What's not in 'master' but should be

[Johannes Schindelin]

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

Hi,

On Fri, 18 Jan 2008, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Currently the ones I looked at and consider possible 1.5.4 material 
> > are http-push fixes from Grégoire Barbier and parse_commit_buffer() 
> > tightening from Martin Koegler.
> 
> It seems that for the past few days, people were having too much fun 
> bashing how broken MacOS X is, and the real work has stalled in the 
> meantime.  Well, not really stalled but they certainly made the patches 
> and discussions harder to find in the list archive.
> 
> [...]
>
> But there are still unapplied patches that deserve attention. The one 
> that I am most worried about is Grégoire Barbier's http-push changes:
> 
>   $gmane/70406 <1200250979-19604-1-git-send-email-gb@gbarbier.org>

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.

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.

>   $gmane/70407 <1200250979-19604-2-git-send-email-gb@gbarbier.org>

I first could not reproduce the breakage described in the commit message 
(bad or no ref given on command line).

After playing around for a while, all of a sudden, I got a segmentation 
fault:

Waiting for 
http://dscho@127.0.0.1/test.git/objects/56/5e84516c1c6dca168be1715b45aeae70b24d13_36e8d912-4841-455a-bbd9-69e54d00db99
Segmentation fault (core dumped)

Unfortunately, this is with _and_ without this patch.

In gdb, it looks like this:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1213430096 (LWP 31418)]
check_locks () at http-push.c:637
637                     if (!lock->refreshing && time_remaining < 
LOCK_REFRESH) {
(gdb) p lock
$1 = (struct remote_lock *) 0x20
(gdb) bt
#0  check_locks () at http-push.c:637
#1  0x08053f8a in process_response (callback_data=0x80c4550)
    at http-push.c:683
#2  0x0804dbf4 in process_curl_messages () at http.c:539
#3  0x0804dc46 in step_active_slots () at http.c:453
#4  0x0804dccb in run_active_slot (slot=0x80c2388) at http.c:474
#5  0x0804deaa in http_cleanup () at http.c:291
#6  0x0805268f in main (argc=3, argv=Cannot access memory at address 0x4
) at http-push.c:2428

So it seems that there is more to fix.

>   $gmane/70405 <1200250979-19604-3-git-send-email-gb@gbarbier.org>

This makes sense.  I only tried to compile http-push once without 
CURL_MULTI, and gave up (I think I even sent out a patch disabling 
CURL_MULTI for curl versions lacking a certain symbol).

Ciao,
Dscho

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

[Grégoire Barbier]

Johannes Schindelin a écrit :
> > $gmane/70407 <1200250979-19604-2-git-send-email-gb@gbarbier.org>
>
>  I first could not reproduce the breakage described in the commit
>  message (bad or no ref given on command line).

It's rather easy anyway:

First, you need a test git repository availlable over http+webdav, let's 
say at http://myhost/myrepo.git/
Then, you do this:
$ git clone http://myhost/myrepo.git/
$ cd myrepo
$ git push http
Fetching remote heads...
  refs/
  refs/heads/
  refs/tags/
No refs in common and none specified; doing nothing.
$ git push http
Fetching remote heads...
  refs/
  refs/heads/
  refs/tags/
No refs in common and none specified; doing nothing.
$

Finally, you look at the web server logs, and will find one LOCK query 
and no UNLOCK query, of course the second one will be in 423 return code 
instead of 200:
1.2.3.4 - gb [19/Jan/2008:14:24:56 +0100] "LOCK /myrepo.git/info/refs 
HTTP/1.1" 200 465
(...)
1.2.3.4 - gb [19/Jan/2008:14:25:10 +0100] "LOCK /myrepo.git/info/refs 
HTTP/1.1" 423 363

With my patch, there would have be two UNLOCKs in addition of the LOCKs

 From the user point of view:
- If you realize that you should have typed e.g. "git push http master" 
instead of "git push http", you will have to wait for 10 minutes for the 
lock to expire by its own.
- Furthermore, if somebody else is dumb enough to type "git push http" 
while you need to push "master" branch, then you'll need too to wait for 
10 minutes too.

>  After playing around for a while, all of a sudden, I got a
>  segmentation fault:
>
>  Waiting for
> 
http://dscho@127.0.0.1/test.git/objects/56/5e84516c1c6dca168be1715b45aeae70b24d13_36e8d912-4841-455a-bbd9-69e54d00db99
>  Segmentation fault (core dumped)
>
>  Unfortunately, this is with _and_ without this patch.
>
>  In gdb, it looks like this:
(...)
>  The segmentation fault occurs due to check_locks() accessing the
>  remote that was just free()d, due to the new change.
>
>  But now I cannot even reproduce the segmentation fault, it seems.
>  Strange.  Very strange.
>
>  Grégoire, it would help tremendously if you could come up with a test
>  case.  The description you gave did not lead to something
>  reproducible here.

I don't know what's wrong but I can't manage to reproduce the segfault, 
I'm using the master branch on git.git plus my patches, and with CFLAGS 
containing -DUSE_CURL_MULTI, nothing more nothing less.
Is the test case I described above is enough for you to make another test?
What kind of additional information would you need ?

I will resubmit this patch today with a more detailled commit message 
including the way to reproduce the issue.
BTW you'll be interested to look at one of the "patches" I will repost 
today, since it's related to this one (the patch subject is "http-push: 
fail when info/refs exists and is already locked").

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

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

[Johannes Schindelin]

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

Hi,

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

> Johannes Schindelin a écrit :
>
> >  After playing around for a while, all of a sudden, I got a
> >  segmentation fault:
> > 
> >  Waiting for
> > 
> > http://dscho@127.0.0.1/test.git/objects/56/5e84516c1c6dca168be1715b45aeae70b24d13_36e8d912-4841-455a-bbd9-69e54d00db99
> > Segmentation fault (core dumped)
> > 
> >  Unfortunately, this is with _and_ without this patch.

Looking at it again in more depth, it seems that this failure is indeed 
independent of your patch.

But I would still feel better if the fixes were kept minimal for now 
(codepath-wise, not only code-wise).

Ciao,
Dscho