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

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

[Sverre Rabbelier]

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(-)

[PATCH 2/7] .gitignore: add git-remote-cvs

[Sverre Rabbelier]

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---
 .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

[PATCH 3/7] Add notify and warn to util.py

[Sverre Rabbelier]

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

	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

[PATCH 4/7] Allow both url and foreign vcs in remote config

[Sverre Rabbelier]

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

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

	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

[PATCH 5/7] Finally make remote helper support useful

[Sverre Rabbelier]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
--

	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

[PATCH 6/7] When updating refs after a fetch, resolve the symref if set

[Sverre Rabbelier]

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 <srabbelier@gmail.com>
---

	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

[PATCH 7/7] Factor ref updating out of fetch_with_import

[Sverre Rabbelier]

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 <srabbelier@gmail.com>
---

	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

Re: [PATCH 1/7] Refactor git_remote_cvs to a more generic git_remote_helpers

[Johan Herland]

On Thursday 29 October 2009, Sverre Rabbelier wrote:
> This in an effort to allow future remote helpers written in python to
> re-use the non-cvs-specific code.
> 
> Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
> CC: Johan Herland <johan@herland.net>
> ---
> 
> 	As discussed with Johan Herland, refactored git_remote_cvs into a
> 	more reusable git_remote_helpers module.

It's hard to review this patch for a couple of reasons:

1. It's over 200K large, which causes it to be blocked by the Git mailing 
list (100K limit, I believe).

2. The patch renames some files, but instead of simply stating the rename, 
the patch lists the entire file twice (deletion + creation)

Fortunately, you can easily solve both problems by rerolling the patch with 
the -M flag to git-format-patch.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 7/7] Factor ref updating out of fetch_with_import

[Shawn O. Pearce]

Sverre Rabbelier <srabbelier@gmail.com> 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.
...
> 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);

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.

> 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);

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

> diff --git a/transport-helper.c b/transport-helper.c
> index a361366..9a98fae 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -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;

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

-- 
Shawn.

Re: [PATCH 1/7] Refactor git_remote_cvs to a more generic git_remote_helpers

[Sverre Rabbelier]

Heya,

On Thu, Oct 29, 2009 at 05:05, Johan Herland <johan@herland.net> wrote:
> Fortunately, you can easily solve both problems by rerolling the patch with
> the -M flag to git-format-patch.

Whow, I feel really silly now, thanks for pointing that out, proper patch sent!

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 7/7] Factor ref updating out of fetch_with_import

[Sverre Rabbelier]

Heya,

On Thu, Oct 29, 2009 at 07:22, Shawn O. Pearce <spearce@spearce.org> 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

Re: [PATCH 7/7] Factor ref updating out of fetch_with_import

[Shawn O. Pearce]

Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Thu, Oct 29, 2009 at 07:22, Shawn O. Pearce <spearce@spearce.org> 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?

If the function isn't defined, it should behave as though it were
defined and successfully completed.
 
> > 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.

Re: [PATCH 7/7] Factor ref updating out of fetch_with_import

[Sverre Rabbelier]

Heya,

On Thu, Oct 29, 2009 at 08:56, Shawn O. Pearce <spearce@spearce.org> wrote:
>> > 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

Re: [PATCH 7/7] Factor ref updating out of fetch_with_import

[Daniel Barkalow]

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

On Thu, 29 Oct 2009, Sverre Rabbelier wrote:

> Heya,
> 
> On Thu, Oct 29, 2009 at 08:56, Shawn O. Pearce <spearce@spearce.org> wrote:
> >> > 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*

Re: [PATCH 7/7] Factor ref updating out of fetch_with_import

[Sverre Rabbelier]

Heya,

On Thu, Oct 29, 2009 at 10:22, Daniel Barkalow <barkalow@iabervon.org> 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

Re: [PATCH 7/7] Factor ref updating out of fetch_with_import

[Daniel Barkalow]

On Thu, 29 Oct 2009, Sverre Rabbelier wrote:

> Heya,
> 
> On Thu, Oct 29, 2009 at 10:22, Daniel Barkalow <barkalow@iabervon.org> 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?

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.

> > (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*

Re: [PATCH 7/7] Factor ref updating out of fetch_with_import

[Sverre Rabbelier]

Heya,

On Thu, Oct 29, 2009 at 11:43, Daniel Barkalow <barkalow@iabervon.org> 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