[PATCH 0/7] Prepare git-remote-helpers series for hg support

This series does not add hg support, but it does do all the
refactoring that is needed to make it possible. With this series
applied actually adding full-fledged hg support is a matter of adding
the git-remote-hg file.
The reason the git-remote-hg file is not included in this series is
that while I have finished support for 'git clone hg::/path/to/hg' as
well as 'git fetch hg::/path/to/hg' and 'git fetch hgremote',
currently it does a full export on each run (that is, incremental
fetch is not supported).
Nevertheless, I think it is vital that this is included in the series
before we do anything serious with it. I hope to have the
git-remote-hg bit done tomorrow before I get on my airplane, but no
promises there.

Junio, I can try to re-roll the entire series after I finish
git-remote-hg, or I can just send it in on top of this series.

Based on abdcdc5c [More fixes to the git-remote-cvs...].

Johannes Schindelin (1):
      Finally make remote helper support useful

Sverre Rabbelier (5):
      .gitignore: add git-remote-cvs
      Add notify and warn to util.py
      Allow both url and foreign vcs in remote config
      When updating refs after a fetch, resolve the symref if set
      Factor ref updating out of fetch_with_import

 .gitignore                 |    1 +
 builtin-clone.c            |    7 +++++++
 builtin-fetch.c            |    4 ++++
 git_remote_helpers/util.py |    8 ++++++++
 remote.c                   |    2 +-
 transport-helper.c         |   22 +++++++++++++++-------
 transport.c                |   10 ++++++++++
 transport.h                |    7 +++++--
 8 files changed, 51 insertions(+), 10 deletions(-)

Signed-off-by: Sverre Rabbelier <redacted>
---
 .gitignore |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index 46c26cd..b8afdf4 100644
--- a/.gitignore
+++ b/.gitignore

@@ -105,6 +105,7 @@ git-reflog
 git-relink
 git-remote
 git-remote-curl
+git-remote-cvs
 git-repack
 git-repo-config
 git-request-pull

-- 
1.6.5.2.291.gf76a3

From: Johannes Schindelin <redacted>

The common case for remote helpers will be to import some repository which
can be specified by a single URL.  Rather than supporting those who say
that Git is really complicated, let's be nice to users so that they will
be able to say:

	git clone hg::https://soc.googlecode.com/hg/ soc

Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Sverre Rabbelier <redacted>
--

	Dscho wrote this a while back and it really does make the
	series so much more useful.

 transport.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/transport.c b/transport.c
index ef33404..a682c75 100644
--- a/transport.c
+++ b/transport.c

@@ -813,6 +813,16 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		url = remote->url[0];
 	ret->url = url;
 
+	/* maybe it is a foreign URL? */
+	if (url) {
+		const char *p = url;
+
+		while (isalnum(*p))
+			p++;
+		if (!prefixcmp(p, "::"))
+			remote->foreign_vcs = xstrndup(url, p - url);
+	}
+
 	if (remote && remote->foreign_vcs) {
 		transport_helper_init(ret, remote->foreign_vcs);
 		return ret;

-- 
1.6.5.2.291.gf76a3

Also allow the new update_refs to actually update the refs set, this
way the remote helper can set the value of previously unknown refs.

Signed-off-by: Sverre Rabbelier <redacted>
---

	This is the most important patch of the series, without it it
	is truly impossible to 'git clone hg::/path/to/hg/repo' since
	when we try to guess remote_head, we use
	find_ref_by_name(refs, "HEAD"), which will not find anything
	unless we set refs to the updated refs _after_ doing the
	import. This is a result of the fact that we do not know the
	value of the remote HEAD until after the import is complete.

 builtin-clone.c    |    7 +++++++
 builtin-fetch.c    |    4 ++++
 transport-helper.c |   22 +++++++++++++++-------
 transport.h        |    7 +++++--
 4 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index f281756..768668d 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c

@@ -512,6 +512,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		if (refs) {
 			struct ref *ref_cpy = copy_ref_list(refs);
 			transport_fetch_refs(transport, ref_cpy);
+			if (transport->update_refs)
+			{
+				ref_cpy = copy_ref_list(refs);
+				transport->update_refs(transport, ref_cpy);
+				refs = ref_cpy;
+				mapped_refs = wanted_peer_refs(refs, refspec);
+			}
 		}
 	}
 

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 63a4ff0..3aaac47 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c

@@ -479,7 +479,11 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
 {
 	int ret = quickfetch(ref_map);
 	if (ret)
+	{
 		ret = transport_fetch_refs(transport, ref_map);
+		if (transport->update_refs)
+			transport->update_refs(transport, ref_map);
+	}
 	if (!ret)
 		ret |= store_updated_refs(transport->url,
 				transport->remote->name,

diff --git a/transport-helper.c b/transport-helper.c
index a361366..9a98fae 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -154,13 +154,6 @@ static int fetch_with_import(struct transport *transport,
 	finish_command(&fastimport);
 	free((char *) fastimport.argv[2]);
 	free((char *) fastimport.argv[3]);
-
-	for (i = 0; i < nr_heads; i++) {
-		posn = to_fetch[i];
-		if (posn->status & REF_STATUS_UPTODATE)
-			continue;
-		read_ref(posn->symref ? posn->symref : posn->name, posn->old_sha1);
-	}
 	return 0;
 }
 

@@ -255,6 +248,20 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 	return ret;
 }
 
+static int update_refs(struct transport *transport, struct ref *refs)
+{
+	struct ref *ref = refs;
+
+	while (ref) {
+		if (ref->status & REF_STATUS_UPTODATE)
+			continue;
+		read_ref(ref->symref ? ref->symref : ref->name, ref->old_sha1);
+		ref = ref->next;
+	}
+
+	return 0;
+}
+
 int transport_helper_init(struct transport *transport, const char *name)
 {
 	struct helper_data *data = xcalloc(sizeof(*data), 1);

@@ -264,5 +271,6 @@ int transport_helper_init(struct transport *transport, const char *name)
 	transport->get_refs_list = get_refs_list;
 	transport->fetch = fetch;
 	transport->disconnect = release_helper;
+	transport->update_refs = update_refs;
 	return 0;
 }

diff --git a/transport.h b/transport.h
index 40dfe64..6d31df3 100644
--- a/transport.h
+++ b/transport.h

@@ -32,12 +32,15 @@ struct transport {
 	/**
 	 * Fetch the objects for the given refs. Note that this gets
 	 * an array, and should ignore the list structure.
-	 *
+	 **/
+	int (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);
+
+	/**
 	 * If the transport did not get hashes for refs in
 	 * get_refs_list(), it should set the old_sha1 fields in the
 	 * provided refs now.
 	 **/
-	int (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);
+	int (*update_refs)(struct transport *transport, struct ref *refs);
 
 	/**
 	 * Push the objects and refs. Send the necessary objects, and

-- 
1.6.5.2.291.gf76a3

This makes it possible to return in response to 'list':
	@refs/hg/origin/tip refs/heads/tip
And have HEAD resolve properly after the import completes.

Signed-off-by: Sverre Rabbelier <redacted>
---

	Without this it is very hard to support 'git clone' on a
	non-git repository, which I think is a very important thing
	to have.

 transport-helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 639f08c..a361366 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -159,7 +159,7 @@ static int fetch_with_import(struct transport *transport,
 		posn = to_fetch[i];
 		if (posn->status & REF_STATUS_UPTODATE)
 			continue;
-		read_ref(posn->name, posn->old_sha1);
+		read_ref(posn->symref ? posn->symref : posn->name, posn->old_sha1);
 	}
 	return 0;
 }

-- 
1.6.5.2.291.gf76a3

The current logic prevents both 'remote.url' and 'remote.vcs' from
being set, while that might be useful.

Signed-off-by: Sverre Rabbelier <redacted>
---

	This way I can do:

	remote.hgremote.url = /path/to/hg/repo
	remote.hgremote.vcs = hg

	As well as:

	remote.hgremote.url = hg::/path/to/hg/repo

 remote.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/remote.c b/remote.c
index f0441c4..29365de 100644
--- a/remote.c
+++ b/remote.c

@@ -50,7 +50,7 @@ static char buffer[BUF_SIZE];
 
 static int valid_remote(const struct remote *remote)
 {
-	return !remote->url != !remote->foreign_vcs;
+	return (!!remote->url) || (!!remote->foreign_vcs);
 }
 
 static const char *alias_url(const char *url)

-- 
1.6.5.2.291.gf76a3

Signed-off-by: Sverre Rabbelier <redacted>
---

	These turned out to be quite useful for my git-remote-hg, if
	desired this patch can move to later in the series, but I
	figured that it might be useful to someone else already.

 git_remote_helpers/util.py |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/git_remote_helpers/util.py b/git_remote_helpers/util.py
index 7d6adb4..d3ca487 100644
--- a/git_remote_helpers/util.py
+++ b/git_remote_helpers/util.py

@@ -15,6 +15,10 @@ import subprocess
 # Whether or not to show debug messages
 DEBUG = False
 
+def notify(msg, *args):
+	"""Print a message to stderr."""
+	print >> sys.stderr, msg % args
+
 def debug (msg, *args):
     """Print a debug message to stderr when DEBUG is enabled."""
     if DEBUG:

@@ -24,6 +28,10 @@ def error (msg, *args):
     """Print an error message to stderr."""
     print >> sys.stderr, "ERROR:", msg % args
 
+def warn(msg, *args):
+	"""Print a warning message to stderr."""
+	print >> sys.stderr, "warning:", msg % args
+
 def die (msg, *args):
     """Print as error message to stderr and exit the program."""
     error(msg, *args)

-- 
1.6.5.2.291.gf76a3

Sverre Rabbelier [off-list ref] wrote:

Also allow the new update_refs to actually update the refs set, this
way the remote helper can set the value of previously unknown refs.

...

Please define a transport_update_refs wrapper function to implement
this method invocation on the transport instance.  Callers should
not be reaching into the struct transport directly.

I certainly have to wonder... if this is done in both fetch and
clone, why isn't it just part of fetch_refs?

Please set this before the disconnect method (move it up one line).

-- 
Shawn.

Heya,

On Thu, Oct 29, 2009 at 07:22, Shawn O. Pearce [off-list ref] wrote:

Please define a transport_update_refs wrapper function to implement
this method invocation on the transport instance.  Callers should
not be reaching into the struct transport directly.

With pleasure, should the transport_update_refs wrapper simply 'return
1' without doing anything if transport->update_refs is not set?

I certainly have to wonder... if this is done in both fetch and
clone, why isn't it just part of fetch_refs?

Because clone does not use fetch_refs, or am I missing something?

Please set this before the disconnect method (move it up one line).

Will do.

-- 
Cheers,

Sverre Rabbelier

Sverre Rabbelier [off-list ref] wrote:

On Thu, Oct 29, 2009 at 07:22, Shawn O. Pearce [off-list ref] wrote:

quoted

Please define a transport_update_refs wrapper function to implement
this method invocation on the transport instance. ?Callers should
not be reaching into the struct transport directly.

With pleasure, should the transport_update_refs wrapper simply 'return
1' without doing anything if transport->update_refs is not set?

If the function isn't defined, it should behave as though it were
defined and successfully completed.
 

quoted

I certainly have to wonder... if this is done in both fetch and
clone, why isn't it just part of fetch_refs?

Because clone does not use fetch_refs, or am I missing something?

Hmmph.  Weird.  Its been a while since I last looked at this code,
maybe I misunderstood it.
 
-- 
Shawn.

Heya,

On Thu, Oct 29, 2009 at 08:56, Shawn O. Pearce [off-list ref] wrote:

quoted

quoted

I certainly have to wonder... if this is done in both fetch and
clone, why isn't it just part of fetch_refs?

Because clone does not use fetch_refs, or am I missing something?

Hmmph.  Weird.  Its been a while since I last looked at this code,
maybe I misunderstood it.

Unless you mean transport_fetch_refs? It can't be done in
transport_fetch_refs because the foreign helper transport needs to
update all refs, including those that wanted_peer_refs decided not to
fetch. Otherwise the 'HEAD' ref will not be updated, and it is left at
all zeros. It is not as obvious that that is a problem in this patch,
but when Junio merges with Nico's [5bdc32d3e "make 'git clone' ask the
remote only for objects it cares about"] the code looks like this:

		if (refs) {
			struct ref *ref_cpy;
			mapped_refs = wanted_peer_refs(refs, refspec);
			ref_cpy = copy_ref_list(mapped_refs);
			transport_fetch_refs(transport, ref_cpy);
			if (transport->update_refs)
			{
				ref_cpy = copy_ref_list(refs);
				transport->update_refs(transport, ref_cpy);
				refs = ref_cpy;
				mapped_refs = wanted_peer_refs(refs, refspec);
			}
		}

Does it make more sense now? transport_fetch_refs gets only a limited
view of the refs, so it cannot pass all the refs to
transport_update_refs as needed.

-- 
Cheers,

Sverre Rabbelier

On Thu, 29 Oct 2009, Sverre Rabbelier wrote:

Heya,

On Thu, Oct 29, 2009 at 08:56, Shawn O. Pearce [off-list ref] wrote:

quoted

quoted

quoted

I certainly have to wonder... if this is done in both fetch and
clone, why isn't it just part of fetch_refs?

Because clone does not use fetch_refs, or am I missing something?

Hmmph.  Weird.  Its been a while since I last looked at this code,
maybe I misunderstood it.

Unless you mean transport_fetch_refs? It can't be done in
transport_fetch_refs because the foreign helper transport needs to
update all refs, including those that wanted_peer_refs decided not to
fetch. Otherwise the 'HEAD' ref will not be updated, and it is left at
all zeros. It is not as obvious that that is a problem in this patch,
but when Junio merges with Nico's [5bdc32d3e "make 'git clone' ask the
remote only for objects it cares about"] the code looks like this:

		if (refs) {
			struct ref *ref_cpy;
			mapped_refs = wanted_peer_refs(refs, refspec);
			ref_cpy = copy_ref_list(mapped_refs);
			transport_fetch_refs(transport, ref_cpy);
			if (transport->update_refs)
			{
				ref_cpy = copy_ref_list(refs);
				transport->update_refs(transport, ref_cpy);
				refs = ref_cpy;
				mapped_refs = wanted_peer_refs(refs, refspec);
			}
		}

Does it make more sense now? transport_fetch_refs gets only a limited
view of the refs, so it cannot pass all the refs to
transport_update_refs as needed.

I think there are a bunch of bugs that you're working around:

(a) if HEAD is determined to be a symref, and we care about HEAD, we care 
about whatever HEAD is a symref to; wanted_peer_refs() shouldn't be 
filtering such things out.

(b) we don't want to make a whole bunch of copies of the ref list to avoid 
updating the mutable ref list that we will look for updated values in; the 
merge of my series with Nico's should replace my copy_ref_list with his 
wanted_peer_refs, not include the copy afterwards. Correcting this should 
lead to the value that matters in the list that will be used having been 
updated directly by transport_fetch_refs().

	-Daniel
*This .sig left intentionally blank*

Heya,

On Thu, Oct 29, 2009 at 10:22, Daniel Barkalow [off-list ref] wrote:

(a) if HEAD is determined to be a symref, and we care about HEAD, we care
about whatever HEAD is a symref to; wanted_peer_refs() shouldn't be
filtering such things out.

It seems that wanted_peer_refs filters out HEAD no matter what though?

(b) we don't want to make a whole bunch of copies of the ref list to avoid
updating the mutable ref list that we will look for updated values in; the
merge of my series with Nico's should replace my copy_ref_list with his
wanted_peer_refs, not include the copy afterwards. Correcting this should
lead to the value that matters in the list that will be used having been
updated directly by transport_fetch_refs().

This I can and will fix. Patch-bomb incoming.

-- 
Cheers,

Sverre Rabbelier

On Thu, 29 Oct 2009, Sverre Rabbelier wrote:

Heya,

On Thu, Oct 29, 2009 at 10:22, Daniel Barkalow [off-list ref] wrote:

quoted

(a) if HEAD is determined to be a symref, and we care about HEAD, we care
about whatever HEAD is a symref to; wanted_peer_refs() shouldn't be
filtering such things out.

It seems that wanted_peer_refs filters out HEAD no matter what though?

It probably shouldn't. Actually, I bet Nico's clone will get extremely 
confused if the remote's HEAD isn't the same as any of its branches. I'll 
try to take a look tonight.

quoted

(b) we don't want to make a whole bunch of copies of the ref list to avoid
updating the mutable ref list that we will look for updated values in; the
merge of my series with Nico's should replace my copy_ref_list with his
wanted_peer_refs, not include the copy afterwards. Correcting this should
lead to the value that matters in the list that will be used having been
updated directly by transport_fetch_refs().

This I can and will fix. Patch-bomb incoming.

I'll look over it later, thanks.

	-Daniel
*This .sig left intentionally blank*

Heya,

On Thu, Oct 29, 2009 at 11:43, Daniel Barkalow [off-list ref] wrote:

It probably shouldn't. Actually, I bet Nico's clone will get extremely
confused if the remote's HEAD isn't the same as any of its branches. I'll
try to take a look tonight.

I'll look over it later, thanks.

Thanks!

-- 
Cheers,

Sverre Rabbelier