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

[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

[PATCH] http-push: fail when info/refs exists and is already locked

[Grégoire Barbier]

Rationale:

Failing instead of silently not updating remote refs makes the things cleare
for the user when trying to push on a repository while another person do (or
while a dandling locks are waiting for a 10 minutes timeout).

When silently not updating remote refs, the user does not even know that git
has pushed the objects but leaved the refs as they were before (e.g. a new
bunch of commits on branch "master" is uploaded, however the branch by itsel
still points on the previous head commit).
---
 http-push.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/http-push.c b/http-push.c
index 2c4e91d..e1984d3 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2243,6 +2243,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();
-- 
1.5.4.rc3.52.g9a5bd-dirty

[PATCH] http-push: more explicit error message with bad URL or password

[Grégoire Barbier]

Previously, when URL or password where not set correctly (or when some network
errors occur), the error message was "no DAV locking support".
---
 http-push.c |    6 ++++++
 http.c      |   25 +++++++++++++++++++++++++
 http.h      |    1 +
 3 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/http-push.c b/http-push.c
index e1984d3..c984d84 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2228,6 +2228,12 @@ int main(int argc, char **argv)
 
 	no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
 
+	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);
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.4.rc3.52.g9a5bd-dirty

[PATCH] http-push and http-fetch: handle URLs without leading /

[Grégoire Barbier]

Since HTTP/302 is not handled in the git code calling curl, URLs without
leading / used to lead to frozen git-fetch or git-push with no error message.

Furthermore, http-push freeze forces the user to interrupt it (^C) and
therefore to leave a dandling webdav lock that makes the remote repository
un-pushable for 10 minutes.

The patch does not make curl calls handle HTTP/302 but instead adds a / at
the end of URLs that does not have it yet.
---
 builtin-http-fetch.c |   10 ++++++++++
 http-push.c          |   11 +++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/builtin-http-fetch.c b/builtin-http-fetch.c
index 4a50dbd..3c1ed08 100644
--- a/builtin-http-fetch.c
+++ b/builtin-http-fetch.c
@@ -9,6 +9,7 @@ int cmd_http_fetch(int argc, const char **argv, const char *prefix)
 	const char **write_ref = NULL;
 	char **commit_id;
 	const char *url;
+	char *rewritten_url = NULL;
 	int arg = 1;
 	int rc = 0;
 	int get_tree = 0;
@@ -51,6 +52,12 @@ int cmd_http_fetch(int argc, const char **argv, const char *prefix)
 		commits = 1;
 	}
 	url = argv[arg];
+        if (url && url[strlen(url)-1] != '/') {
+                rewritten_url = malloc(strlen(url)+2);
+                strcpy(rewritten_url, url);
+                strcat(rewritten_url, "/");
+                url = rewritten_url;
+        }
 
 	walker = get_http_walker(url);
 	walker->get_tree = get_tree;
@@ -73,5 +80,8 @@ int cmd_http_fetch(int argc, const char **argv, const char *prefix)
 
 	walker_free(walker);
 
+	if (rewritten_url)
+		free(rewritten_url);
+
 	return rc;
 }
diff --git a/http-push.c b/http-push.c
index c984d84..2c27105 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2161,6 +2161,7 @@ int main(int argc, char **argv)
 	int i;
 	int new_refs;
 	struct ref *ref;
+	char *rewritten_url = NULL;
 
 	setup_git_directory();
 
@@ -2228,6 +2229,14 @@ int main(int argc, char **argv)
 
 	no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
 
+	if (remote->url && remote->url[strlen(remote->url)-1] != '/') {
+		rewritten_url = malloc(strlen(remote->url)+2);
+		strcpy(rewritten_url, remote->url);
+		strcat(rewritten_url, "/");
+		remote->url = rewritten_url;
+		++remote->path_len;
+	}
+
 	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;
@@ -2415,6 +2424,8 @@ int main(int argc, char **argv)
 	}
 
  cleanup:
+	if (rewritten_url)
+		free(rewritten_url);
 	if (info_ref_lock)
 		unlock_remote(info_ref_lock);
 	free(remote);
-- 
1.5.4.rc3.52.g9a5bd-dirty

[PATCH] added #define DEFAULT_MAX_REQUESTS for USE_CURL_MULTI mode

[Grégoire Barbier]

From: Gregoire Barbier <gb@gbarbier.org>

I'm not sure of which value is the good one, but 4 seems good since it's
not very high which would lead to resouce consumption problems.
---
 http.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/http.c b/http.c
index 8b04ae9..7b1bcb8 100644
--- a/http.c
+++ b/http.c
@@ -4,6 +4,7 @@ int data_received;
 int active_requests = 0;
 
 #ifdef USE_CURL_MULTI
+#define DEFAULT_MAX_REQUESTS 4
 static int max_requests = -1;
 static CURLM *curlm;
 #endif
-- 
1.5.4.rc3.52.g9a5bd-dirty

Re: [PATCH] added #define DEFAULT_MAX_REQUESTS for USE_CURL_MULTI mode

[Junio C Hamano]

Grégoire Barbier <gb@gbarbier.org> writes:

> From: Gregoire Barbier <gb@gbarbier.org>
>
> I'm not sure of which value is the good one, but 4 seems good since it's
> not very high which would lead to resouce consumption problems.
> ---
>  http.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/http.c b/http.c
> index 8b04ae9..7b1bcb8 100644
> --- a/http.c
> +++ b/http.c
> @@ -4,6 +4,7 @@ int data_received;
>  int active_requests = 0;
>  
>  #ifdef USE_CURL_MULTI
> +#define DEFAULT_MAX_REQUESTS 4
>  static int max_requests = -1;
>  static CURLM *curlm;
>  #endif

Why is this needed?

How does this interact with an existing #define in http.h that
defines it to 5?

Re: [PATCH] added #define DEFAULT_MAX_REQUESTS for USE_CURL_MULTI mode

[Grégoire Barbier]

Junio C Hamano a écrit :
> Grégoire Barbier <gb@gbarbier.org> writes:
>   
>> From: Gregoire Barbier <gb@gbarbier.org>
>>
>> I'm not sure of which value is the good one, but 4 seems good since it's
>> not very high which would lead to resouce consumption problems.
>> ---
>>  http.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/http.c b/http.c
>> index 8b04ae9..7b1bcb8 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -4,6 +4,7 @@ int data_received;
>>  int active_requests = 0;
>>  
>>  #ifdef USE_CURL_MULTI
>> +#define DEFAULT_MAX_REQUESTS 4
>>  static int max_requests = -1;
>>  static CURLM *curlm;
>>  #endif
>>     
>
> Why is this needed?
>
> How does this interact with an existing #define in http.h that
> defines it to 5?
>   

Ok, please forget my patch and forgive my stupidity, I've juste realized 
why I needed to define -DUSE_CURL_MULTI by hand in the Makefile, I'm so 
stupid...

I will answers you other mail and Johannes' later.

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

Re: [PATCH] added #define DEFAULT_MAX_REQUESTS for USE_CURL_MULTI mode

[Junio C Hamano]

Grégoire Barbier <gb@gbarbier.org> writes:

> Ok, please forget my patch and forgive my stupidity, I've juste
> realized why I needed to define -DUSE_CURL_MULTI by hand in the
> Makefile, I'm so stupid...

No, it is not your stupidity.

The http.h and http-*.c files assume that USE_CURL_MULTI is
internal and do not want the user to define that symbol from
outside.  It should be better documented and mistakes should be
prevented.

Perhaps we need something like like this.


diff --git a/http.h b/http.h
index aeba930..046b17f 100644
--- a/http.h
+++ b/http.h
@@ -8,6 +8,12 @@
 
 #include "strbuf.h"
 
+/*
+ * We detect based on the cURL version if multi-transfer is
+ * usable in this implementation and define this symbol accordingly.
+ */
+#undef USE_CURL_MULTI
+
 #if LIBCURL_VERSION_NUM >= 0x071000
 #define USE_CURL_MULTI
 #define DEFAULT_MAX_REQUESTS 5

Re: [PATCH] http-push and http-fetch: handle URLs without leading /

[Mike Hommey]

On Sat, Jan 19, 2008 at 04:22:50PM +0100, Grégoire Barbier wrote:
> Since HTTP/302 is not handled in the git code calling curl, URLs without
> leading / used to lead to frozen git-fetch or git-push with no error message.
> 
> Furthermore, http-push freeze forces the user to interrupt it (^C) and
> therefore to leave a dandling webdav lock that makes the remote repository
> un-pushable for 10 minutes.
> 
> The patch does not make curl calls handle HTTP/302 but instead adds a / at
> the end of URLs that does not have it yet.

Actually, it would be much better to do just that, i.e. handle HTTP 302
return codes. I suspect there may be other cases leading to similar dead
locks with other HTTP codes. But that might just be easier to deal with
once my refactoring will be done ;)

Mike

Re: [PATCH] http-push and http-fetch: handle URLs without leading /

[Johannes Schindelin]

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

Hi,

On Sat, 19 Jan 2008, Mike Hommey wrote:

> On Sat, Jan 19, 2008 at 04:22:50PM +0100, Grégoire Barbier wrote:
> > Since HTTP/302 is not handled in the git code calling curl, URLs 
> > without leading / used to lead to frozen git-fetch or git-push with no 
> > error message.
> > 
> > Furthermore, http-push freeze forces the user to interrupt it (^C) and 
> > therefore to leave a dandling webdav lock that makes the remote 
> > repository un-pushable for 10 minutes.
> > 
> > The patch does not make curl calls handle HTTP/302 but instead adds a 
> > / at the end of URLs that does not have it yet.
> 
> Actually, it would be much better to do just that, i.e. handle HTTP 302 
> return codes. I suspect there may be other cases leading to similar dead 
> locks with other HTTP codes. But that might just be easier to deal with 
> once my refactoring will be done ;)

Independently, it seems a good idea to not try a URL we _know_ will fail 
(read: redirect).

Ciao,
Dscho

Re: [PATCH] http-push and http-fetch: handle URLs without leading /

[Johannes Schindelin]

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

Hi,

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

> Since HTTP/302 is not handled in the git code calling curl, URLs without
> leading / used to lead to frozen git-fetch or git-push with no error message.

JFYI these lines are a little bit too long; I would like to see them 
unwrapped with a 4-space indent on a 80-column display.

I am not sure how easily Junio can fix them.


> @@ -51,6 +52,12 @@ int cmd_http_fetch(int argc, const char **argv, const char *prefix)
>  		commits = 1;
>  	}
>  	url = argv[arg];
> +        if (url && url[strlen(url)-1] != '/') {
> +                rewritten_url = malloc(strlen(url)+2);
> +                strcpy(rewritten_url, url);
> +                strcat(rewritten_url, "/");
> +                url = rewritten_url;
> +        }
>  
>  	walker = get_http_walker(url);
>  	walker->get_tree = get_tree;

Please use strbuf, like so:

	struct strbuf rewritten_url = STRBUF_INIT;

	...
	url = argv[arg];
        if (url && url[strlen(url)-1] != '/') {
		strbuf_addstr(&rewritten_url, url);
		strbuf_addch(&rewritten_url, '/');
		url = rewritten_url.buf;
	}
	...
	strbuf_release(&rewritten_url);
	
BTW it seems you indented using spaces, but we like the indentation as 
tabs in git.git.

Other than that, I like your patch!

Thanks,
Dscho

Re: [PATCH] http-push: more explicit error message with bad URL or password

[Junio C Hamano]

Grégoire Barbier <gb@gbarbier.org> writes:

> Previously, when URL or password where not set correctly (or
> when some network errors occur), the error message was "no DAV
> locking support".

The standard "Sign-off?" comment aside,...

I think something like this would be much less invasive and more
to the point.

-- >8 --
http-push: clarify the reason of error from the initial PROPFIND request

The first thing http-push does is a PROPFIND to see if the other
end supports locking.  The failure message we give is always
reported as "no DAV locking support at the remote repository",
regardless of the reason why we ended up not finding the locking
support on the other end.

This moves the code to report "no DAV locking support" down the
codepath so that the message is issued only when we successfully
get a response to PROPFIND and the other end say it does not
support locking.  Other failures, such as connectivity glitches
and credential mismatches, have their own error message issued
and we will not issue "no DAV locking" error (we do not even
know if the remote end supports it).

---

 http-push.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/http-push.c b/http-push.c
index eef7674..9f92cc1 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1563,9 +1563,17 @@ static int locking_available(void)
 				lock_flags = 0;
 			}
 			XML_ParserFree(parser);
+			if (!lock_flags)
+				error("Error: no DAV locking support on %s",
+				      remote->url);
+
+		} else {
+			error("Cannot access URL %s, return code %d",
+			      remote->url, results.curl_result);
+			lock_flags = 0;
 		}
 	} else {
-		fprintf(stderr, "Unable to start PROPFIND request\n");
+		error("Unable to start PROPFIND request on %s", remote->url);
 	}
 
 	strbuf_release(&out_buffer.buf);
@@ -2230,7 +2238,6 @@ int main(int argc, char **argv)
 
 	/* Verify DAV compliance/lock support */
 	if (!locking_available()) {
-		fprintf(stderr, "Error: no DAV locking support on remote repo %s\n", remote->url);
 		rc = 1;
 		goto cleanup;
 	}

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

[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

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