[RFC 00/11] Foreign VCS helper program for CVS repositories

[RFC 00/11] Foreign VCS helper program for CVS repositories

[Johan Herland]

This series is a first draft towards implementing a foreign VCS helper
program for CVS.

It is based on the 'db/foreign-scm' and 'jh/notes' topics in 'pu'. As such,
this patch series should apply cleanly to current 'pu'.

The first 4 patches supply the necessary parts of Daniel Barkalow's
foreign-scm topic that has not yet been merged to 'pu' (i.e. everything
remaining in Daniel's original patch series, except the p4-specific stuff).

The next 2 patches add some functionality to git fast-import, for
facilitating the import of 'notes' objects (this is a _much_ faster way to
generate/import notes than invoking "git notes edit -m MSG" for each note).

Next, there are 3 patches tweaking and expanding the git-vcs API (with
corresponding implementations in the foreign transport code) to adjust for
the CVS helper's needs.

The final 2 patches add the git-vcs-cvs helper program that implements the
fetch/import of objects from a local or remote CVS repository, along with
selftests for verifying its correctness in some simple cases. This is a
first RFC/draft of this helper program, and will hopefully grow along with
the foreign-scm feature itself until it provides a compelling alternative
to "git cvsimport" and "git cvsexportcommit".


Daniel Barkalow (4):
  Add specification of git-vcs-* helper programs
  Use a function to determine whether a remote is valid
  Allow programs to not depend on remotes having urls
  Add a transport implementation using git-vcs-* helpers

Johan Herland (7):
  Refactor path name parsing into new function: get_path_str()
  Add support for mark references as path names
  Preliminary clarifications to git-vcs documentation
  Teach foreign transport code to perform the "capabilities" command
  Introduce a 'marks <filename>' feature to the foreign transport code
  First draft of CVS importer using the foreign-scm machinery
  Add simple test cases of git-vcs-cvs functionality

 Documentation/git-fast-import.txt |    9 +-
 Documentation/git-vcs-cvs.txt     |   85 ++++
 Documentation/git-vcs.txt         |   88 ++++
 Makefile                          |   47 ++
 builtin-fetch.c                   |   19 +-
 builtin-ls-remote.c               |    4 +-
 builtin-push.c                    |   54 ++-
 configure.ac                      |    3 +
 fast-import.c                     |   71 ++--
 git-vcs-cvs.py                    |  697 +++++++++++++++++++++++++++++
 git_vcs_cvs/.gitignore            |    2 +
 git_vcs_cvs/Makefile              |   27 ++
 git_vcs_cvs/changeset.py          |  114 +++++
 git_vcs_cvs/commit_states.py      |   52 +++
 git_vcs_cvs/cvs.py                |  884 +++++++++++++++++++++++++++++++++++++
 git_vcs_cvs/cvs_revision_map.py   |  367 +++++++++++++++
 git_vcs_cvs/cvs_symbol_cache.py   |  283 ++++++++++++
 git_vcs_cvs/git.py                |  588 ++++++++++++++++++++++++
 git_vcs_cvs/setup.py              |   12 +
 git_vcs_cvs/util.py               |  147 ++++++
 remote.c                          |   13 +-
 t/t9800-foreign-vcs-cvs-basic.sh  |  518 ++++++++++++++++++++++
 t/t9801-foreign-vcs-cvs-fetch.sh  |  291 ++++++++++++
 t/test-lib.sh                     |    1 +
 transport-foreign.c               |  271 ++++++++++++
 transport.c                       |    4 +
 transport.h                       |    1 +
 27 files changed, 4588 insertions(+), 64 deletions(-)
 create mode 100644 Documentation/git-vcs-cvs.txt
 create mode 100644 Documentation/git-vcs.txt
 create mode 100755 git-vcs-cvs.py
 create mode 100644 git_vcs_cvs/.gitignore
 create mode 100644 git_vcs_cvs/Makefile
 create mode 100644 git_vcs_cvs/__init__.py
 create mode 100644 git_vcs_cvs/changeset.py
 create mode 100644 git_vcs_cvs/commit_states.py
 create mode 100644 git_vcs_cvs/cvs.py
 create mode 100644 git_vcs_cvs/cvs_revision_map.py
 create mode 100644 git_vcs_cvs/cvs_symbol_cache.py
 create mode 100644 git_vcs_cvs/git.py
 create mode 100644 git_vcs_cvs/setup.py
 create mode 100644 git_vcs_cvs/util.py
 create mode 100755 t/t9800-foreign-vcs-cvs-basic.sh
 create mode 100755 t/t9801-foreign-vcs-cvs-fetch.sh
 create mode 100644 transport-foreign.c


Have fun! :)

...Johan

[RFC 01/11] Add specification of git-vcs-* helper programs

[Johan Herland]

From: Daniel Barkalow <barkalow@iabervon.org>

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/git-vcs.txt |   77 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 77 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/git-vcs.txt

diff --git a/Documentation/git-vcs.txt b/Documentation/git-vcs.txt
new file mode 100644
index 0000000..402c927
--- /dev/null
+++ b/Documentation/git-vcs.txt
@@ -0,0 +1,77 @@
+git-vcs(1)
+============
+
+NAME
+----
+git-vcs - Helper programs for interoperation with foreign systems
+
+SYNOPSIS
+--------
+'git vcs-<system>' <remote>
+
+DESCRIPTION
+-----------
+
+These programs are normally not used directly by end users, but are
+invoked by various git programs that interact with remote repositories
+when the repository they would operate on uses a foreign system.
+
+Each 'git vcs-<system>' is a helper for interoperating with a
+particular version control system. Different helpers have different
+capabilities (limited both by the particular helper and by the
+capabilities of the system they connect to), and they report what
+capabilities they support.
+
+These programs can store refs in refs/<system>/*, and arbitrary
+information in info/<system>.
+
+COMMANDS
+--------
+
+Commands are given by the caller on the helper's standard input, one per line.
+
+'capabilities'::
+	Outputs a single line with a list of feature names separated
+	by spaces. Each of these indicates a supported feature of the
+	helper, and the caller will only attempt operations that are
+	supported.
+
+'list'::
+	Outputs the names of refs, one per line. These may be
+	followed, after a single space, by "changed" or "unchanged",
+	indicating whether the foreign repository has changed from the
+	state in the ref. If the helper doesn't know, it doesn't have
+	to provide a value. (In particular, it shouldn't do expensive
+	operations, such as importing the content, to see whether it
+	matches.) Other information, not yet supported, may be output
+	as well, separated by single spaces.
+
+'import' ref::
+	Imports the given ref by outputting it in git-fast-import
+	format.
+
+'export' commit ref::
+	Sends the given commit to the foreign system, with the
+	location given by ref, and reimports it by outputting it in
+	git-fast-import format as the foreign system rendered it.
++
+All parents of commit must either have been created with 'import' or
+have been passed to 'export' previously. Depending on the features,
+there may be other restrictions on what may be exported.
+
+FEATURES
+--------
+
+'export'::
+	Helper supports exporting commits, at least exporting
+	non-merge commits whose parents are not the parents of any
+	other commit exported to the same branch or make in the other
+	system on the same branch.
+
+'export-branch'::
+	Helper supports creating new branches by exporting commits to
+	them.
+
+'export-merges'::
+	Helper supports exporting two-parent merges, where both
+	parents have already been exported successfully.
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC 02/11] Use a function to determine whether a remote is valid

[Johan Herland]

From: Daniel Barkalow <barkalow@iabervon.org>

Currently, it only checks url, but it will allow other things in the future.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
Signed-off-by: Johan Herland <johan@herland.net>
---
 remote.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/remote.c b/remote.c
index 6be9083..c5ac881 100644
--- a/remote.c
+++ b/remote.c
@@ -48,6 +48,11 @@ static int rewrite_nr;
 #define BUF_SIZE (2048)
 static char buffer[BUF_SIZE];
 
+static int valid_remote(const struct remote *remote)
+{
+	return !!remote->url;
+}
+
 static const char *alias_url(const char *url)
 {
 	int i, j;
@@ -672,14 +677,14 @@ struct remote *remote_get(const char *name)
 
 	ret = make_remote(name, 0);
 	if (valid_remote_nick(name)) {
-		if (!ret->url)
+		if (!valid_remote(ret))
 			read_remotes_file(ret);
-		if (!ret->url)
+		if (!valid_remote(ret))
 			read_branches_file(ret);
 	}
-	if (name_given && !ret->url)
+	if (name_given && !valid_remote(ret))
 		add_url_alias(ret, name);
-	if (!ret->url)
+	if (!valid_remote(ret))
 		return NULL;
 	ret->fetch = parse_fetch_refspec(ret->fetch_refspec_nr, ret->fetch_refspec);
 	ret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC 03/11] Allow programs to not depend on remotes having urls

[Johan Herland]

From: Daniel Barkalow <barkalow@iabervon.org>

For fetch and ls-remote, which use the first url of a remote, have
transport_get() determine this by passing a remote and passing NULL
for the url. For push, which uses every url of a remote, use each url
in turn if there are any, and use NULL if there are none.

This will allow the transport code to do something different if the
location is not specified with a url.

Also, have the message for a fetch say "foreign" if there is no url.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin-fetch.c     |   19 +++++++++++------
 builtin-ls-remote.c |    4 +-
 builtin-push.c      |   54 +++++++++++++++++++++++++++++++++-----------------
 transport.c         |    3 ++
 4 files changed, 52 insertions(+), 28 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 817dd6b..3f32db6 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -346,12 +346,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 			what = rm->name;
 		}
 
-		url_len = strlen(url);
-		for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
-			;
-		url_len = i + 1;
-		if (4 < i && !strncmp(".git", url + i - 3, 4))
-			url_len = i - 3;
+		if (url) {
+			url_len = strlen(url);
+			for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
+				;
+			url_len = i + 1;
+			if (4 < i && !strncmp(".git", url + i - 3, 4))
+				url_len = i - 3;
+		} else {
+			url = "foriegn";
+			url_len = strlen(url);
+		}
 
 		note_len = 0;
 		if (*what) {
@@ -663,7 +668,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	if (!remote)
 		die("Where do you want to fetch from today?");
 
-	transport = transport_get(remote, remote->url[0]);
+	transport = transport_get(remote, NULL);
 	if (verbosity >= 2)
 		transport->verbose = 1;
 	if (verbosity < 0)
diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
index 78a88f7..4c6fc58 100644
--- a/builtin-ls-remote.c
+++ b/builtin-ls-remote.c
@@ -87,9 +87,9 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		}
 	}
 	remote = nongit ? NULL : remote_get(dest);
-	if (remote && !remote->url_nr)
+	if (!nongit && !remote)
 		die("remote %s has no configured URL", dest);
-	transport = transport_get(remote, remote ? remote->url[0] : dest);
+	transport = transport_get(remote, remote ? NULL : dest);
 	if (uploadpack != NULL)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
 
diff --git a/builtin-push.c b/builtin-push.c
index 1d92e22..ae63521 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -88,6 +88,26 @@ static void setup_default_push_refspecs(void)
 	}
 }
 
+static int push_with_options(struct transport *transport, int flags)
+{
+	int err;
+	if (receivepack)
+		transport_set_option(transport,
+				     TRANS_OPT_RECEIVEPACK, receivepack);
+	if (thin)
+		transport_set_option(transport, TRANS_OPT_THIN, "yes");
+
+	if (flags & TRANSPORT_PUSH_VERBOSE)
+		fprintf(stderr, "Pushing to %s\n", transport->url);
+	err = transport_push(transport, refspec_nr, refspec, flags);
+	err |= transport_disconnect(transport);
+
+	if (!err)
+		return 0;
+
+	return 1;
+}
+
 static int do_push(const char *repo, int flags)
 {
 	int i, errs;
@@ -136,26 +156,22 @@ static int do_push(const char *repo, int flags)
 		url = remote->url;
 		url_nr = remote->url_nr;
 	}
-	for (i = 0; i < url_nr; i++) {
+	if (url_nr) {
+		for (i = 0; i < url_nr; i++) {
+			struct transport *transport =
+				transport_get(remote, url[i]);
+			if (push_with_options(transport, flags)) {
+				error("failed to push some refs to '%s'", url[i]);
+				errs++;
+			}
+		}
+	} else {
 		struct transport *transport =
-			transport_get(remote, url[i]);
-		int err;
-		if (receivepack)
-			transport_set_option(transport,
-					     TRANS_OPT_RECEIVEPACK, receivepack);
-		if (thin)
-			transport_set_option(transport, TRANS_OPT_THIN, "yes");
-
-		if (flags & TRANSPORT_PUSH_VERBOSE)
-			fprintf(stderr, "Pushing to %s\n", url[i]);
-		err = transport_push(transport, refspec_nr, refspec, flags);
-		err |= transport_disconnect(transport);
-
-		if (!err)
-			continue;
-
-		error("failed to push some refs to '%s'", url[i]);
-		errs++;
+			transport_get(remote, NULL);
+		if (push_with_options(transport, flags)) {
+			error("failed to push some refs to foreign system");
+			errs++;
+		}
 	}
 	return !!errs;
 }
diff --git a/transport.c b/transport.c
index 13fbca4..2864f40 100644
--- a/transport.c
+++ b/transport.c
@@ -793,6 +793,9 @@ struct transport *transport_get(struct remote *remote, const char *url)
 	struct transport *ret = xcalloc(1, sizeof(*ret));
 
 	ret->remote = remote;
+
+	if (!url && remote && remote->url)
+		url = remote->url[0];
 	ret->url = url;
 
 	if (remote && remote->foreign_vcs) {
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC 04/11] Add a transport implementation using git-vcs-* helpers

[Johan Herland]

From: Daniel Barkalow <barkalow@iabervon.org>

This is somewhat careless about pushes (that is, it attempts to make
pushes that the helper can't necessarily handle), but actually works for
fetches and simple pushes.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
Signed-off-by: Johan Herland <johan@herland.net>
---
 Makefile            |    1 +
 remote.c            |    2 +-
 transport-foreign.c |  200 +++++++++++++++++++++++++++++++++++++++++++++++++++
 transport.c         |    1 +
 transport.h         |    1 +
 5 files changed, 204 insertions(+), 1 deletions(-)
 create mode 100644 transport-foreign.c

diff --git a/Makefile b/Makefile
index f396e52..6b39d56 100644
--- a/Makefile
+++ b/Makefile
@@ -554,6 +554,7 @@ LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += trace.o
 LIB_OBJS += transport.o
+LIB_OBJS += transport-foreign.o
 LIB_OBJS += transport-shim.o
 LIB_OBJS += tree-diff.o
 LIB_OBJS += tree.o
diff --git a/remote.c b/remote.c
index c5ac881..8d1560e 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;
+	return remote->url || remote->foreign_vcs;
 }
 
 static const char *alias_url(const char *url)
diff --git a/transport-foreign.c b/transport-foreign.c
new file mode 100644
index 0000000..29aad77
--- /dev/null
+++ b/transport-foreign.c
@@ -0,0 +1,200 @@
+#include "cache.h"
+#include "transport.h"
+
+#include "run-command.h"
+#include "commit.h"
+#include "diff.h"
+#include "revision.h"
+
+struct foreign_data
+{
+	struct child_process *importer;
+};
+
+static struct child_process *get_importer(struct transport *transport)
+{
+	struct child_process *importer = transport->data;
+	if (!importer) {
+		struct strbuf buf;
+		importer = xcalloc(1, sizeof(*importer));
+		importer->in = -1;
+		importer->out = -1;
+		importer->err = 0;
+		importer->argv = xcalloc(3, sizeof(*importer->argv));
+		strbuf_init(&buf, 80);
+		strbuf_addf(&buf, "vcs-%s", transport->remote->foreign_vcs);
+		importer->argv[0] = buf.buf;
+		importer->argv[1] = transport->remote->name;
+		importer->git_cmd = 1;
+		start_command(importer);
+		transport->data = importer;
+	}
+	return importer;
+}
+
+static int disconnect_foreign(struct transport *transport)
+{
+	struct child_process *importer = transport->data;
+	if (importer) {
+		write(importer->in, "\n", 1);
+		close(importer->in);
+		finish_command(importer);
+		free(importer);
+		transport->data = NULL;
+	}
+	return 0;
+}
+
+static int fetch_refs_via_foreign(struct transport *transport,
+				  int nr_heads, struct ref **to_fetch)
+{
+	struct child_process *importer;
+	struct child_process fastimport;
+	struct ref *posn;
+	int i, count;
+
+	count = 0;
+	for (i = 0; i < nr_heads; i++) {
+		posn = to_fetch[i];
+		if (posn->status & REF_STATUS_UPTODATE)
+			continue;
+		count++;
+	}
+	if (count) {
+		importer = get_importer(transport);
+
+		memset(&fastimport, 0, sizeof(fastimport));
+		fastimport.in = importer->out;
+		fastimport.argv = xcalloc(3, sizeof(*fastimport.argv));
+		fastimport.argv[0] = "fast-import";
+		fastimport.argv[1] = "--quiet";
+		fastimport.git_cmd = 1;
+		start_command(&fastimport);
+
+		for (i = 0; i < nr_heads; i++) {
+			posn = to_fetch[i];
+			if (posn->status & REF_STATUS_UPTODATE)
+				continue;
+			write(importer->in, "import ", 7);
+			write(importer->in, posn->name, strlen(posn->name));
+			write(importer->in, "\n", 1);
+		}
+		disconnect_foreign(transport);
+		finish_command(&fastimport);
+	}
+	for (i = 0; i < nr_heads; i++) {
+		posn = to_fetch[i];
+		if (posn->status & REF_STATUS_UPTODATE)
+			continue;
+		read_ref(posn->name, posn->old_sha1);
+	}
+	return 0;
+}
+
+static struct ref *get_refs_via_foreign(struct transport *transport, int for_push)
+{
+	struct child_process *importer;
+	struct ref *ret = NULL;
+	struct ref **end = &ret;
+	struct strbuf buf;
+	FILE *file;
+
+	importer = get_importer(transport);
+	write(importer->in, "list\n", 5);
+
+	strbuf_init(&buf, 0);
+	file = fdopen(importer->out, "r");
+	while (1) {
+		char *eon;
+		if (strbuf_getline(&buf, file, '\n') == EOF)
+			break;
+
+		if (!*buf.buf)
+			break;
+
+		eon = strchr(buf.buf, ' ');
+		if (eon)
+			*eon = '\0';
+		*end = alloc_ref(buf.buf);
+		if (eon) {
+			if (strstr(eon + 1, "unchanged")) {
+				(*end)->status |= REF_STATUS_UPTODATE;
+				if (read_ref((*end)->name, (*end)->old_sha1))
+					die("Unchanged?");
+				fprintf(stderr, "Old: %p %s\n", *end, sha1_to_hex((*end)->old_sha1));
+			}
+		}
+		end = &((*end)->next);
+		strbuf_reset(&buf);
+	}
+
+	strbuf_release(&buf);
+	return ret;
+}
+
+static int foreign_push(struct transport *transport, struct ref *remote_refs, int flags) {
+	struct ref *ref, *has;
+	struct child_process *importer;
+	struct rev_info revs;
+	struct commit *commit;
+	struct child_process fastimport;
+
+	importer = get_importer(transport);
+
+	memset(&fastimport, 0, sizeof(fastimport));
+	fastimport.in = importer->out;
+	fastimport.argv = xcalloc(3, sizeof(*fastimport.argv));
+	fastimport.argv[0] = "fast-import";
+	fastimport.argv[1] = "--quiet";
+	fastimport.git_cmd = 1;
+	start_command(&fastimport);
+	for (ref = remote_refs; ref; ref = ref->next) {
+		if (!ref->peer_ref) {
+			ref->status = REF_STATUS_NONE;
+			continue;
+		}
+		init_revisions(&revs, NULL);
+		revs.reverse = 1;
+		for (has = remote_refs; has; has = has->next) {
+			commit = lookup_commit(has->old_sha1);
+			commit->object.flags |= UNINTERESTING;
+			add_pending_object(&revs, &commit->object, has->name);
+		}
+		commit = lookup_commit(ref->peer_ref->new_sha1);
+		add_pending_object(&revs, &commit->object, ref->name);
+
+		if (prepare_revision_walk(&revs))
+			die("Something wrong");
+
+		ref->status = REF_STATUS_UPTODATE;
+		while ((commit = get_revision(&revs))) {
+			ref->status = REF_STATUS_EXPECTING_REPORT;
+			fprintf(stderr, "export %s %s\n", sha1_to_hex(commit->object.sha1), ref->name);
+			write(importer->in, "export ", 7);
+			write(importer->in, sha1_to_hex(commit->object.sha1), 40);
+			write(importer->in, " ", 1);
+			write(importer->in, ref->name, strlen(ref->name));
+			write(importer->in, "\n", 1);
+		}
+	}
+
+	disconnect_foreign(transport);
+	finish_command(&fastimport);
+
+	for (ref = remote_refs; ref; ref = ref->next) {
+		read_ref(ref->name, ref->new_sha1);
+		if (ref->status == REF_STATUS_EXPECTING_REPORT)
+			ref->status = REF_STATUS_OK;
+	}
+
+	return 0;
+}
+
+void transport_foreign_init(struct transport *transport)
+{
+	transport->get_refs_list = get_refs_via_foreign;
+	transport->fetch = fetch_refs_via_foreign;
+	transport->push_refs = foreign_push;
+	transport->disconnect = disconnect_foreign;
+	transport->url = transport->remote->foreign_vcs;
+}
diff --git a/transport.c b/transport.c
index 2864f40..4e984ca 100644
--- a/transport.c
+++ b/transport.c
@@ -799,6 +799,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 	ret->url = url;
 
 	if (remote && remote->foreign_vcs) {
+		transport_foreign_init(ret);
 	} else if (!prefixcmp(url, "rsync:")) {
 		ret->get_refs_list = get_refs_via_rsync;
 		ret->fetch = fetch_objs_via_rsync;
diff --git a/transport.h b/transport.h
index 229a7b4..2e1bd6c 100644
--- a/transport.h
+++ b/transport.h
@@ -116,6 +116,7 @@ int transport_disconnect(struct transport *transport);
 char *transport_anonymize_url(const char *url);
 
 /* Transport methods defined outside transport.c */
+void transport_foreign_init(struct transport *transport);
 void transport_shim_init(struct transport *transport, const char *name);
 
 #endif
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC 05/11] Refactor path name parsing into new function: get_path_str()

[Johan Herland]

This is in preparation for adding mark reference capability to path names.

Signed-off-by: Johan Herland <johan@herland.net>
---
 fast-import.c |   60 ++++++++++++++++++++++++++++++--------------------------
 1 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 7ef9865..8a7cdc1 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1853,6 +1853,24 @@ static void load_branch(struct branch *b)
 	}
 }
 
+static const char *get_path_str(
+	struct strbuf *sb,
+	const char *p,
+	char endc,
+	const char **endp)
+{
+	strbuf_reset(sb);
+	*endp = p;
+	if (!unquote_c_style(sb, p, endp)) {
+		/* successfully unquoted C-style quoted name */
+		p = sb->buf;
+	} else {
+		/* regular path ending at endc */
+		*endp = strchr(p, endc);
+	}
+	return p;
+}
+
 static void file_change_m(struct branch *b)
 {
 	const char *p = command_buf.buf + 2;
@@ -1896,12 +1914,9 @@ static void file_change_m(struct branch *b)
 	if (*p++ != ' ')
 		die("Missing space after SHA1: %s", command_buf.buf);
 
-	strbuf_reset(&uq);
-	if (!unquote_c_style(&uq, p, &endp)) {
-		if (*endp)
-			die("Garbage after path in: %s", command_buf.buf);
-		p = uq.buf;
-	}
+	p = get_path_str(&uq, p, 0, &endp);
+	if (*endp)
+		die("Garbage after path in: %s", command_buf.buf);
 
 	if (S_ISGITLINK(mode)) {
 		if (inline_data)
@@ -1948,12 +1963,9 @@ static void file_change_d(struct branch *b)
 	static struct strbuf uq = STRBUF_INIT;
 	const char *endp;
 
-	strbuf_reset(&uq);
-	if (!unquote_c_style(&uq, p, &endp)) {
-		if (*endp)
-			die("Garbage after path in: %s", command_buf.buf);
-		p = uq.buf;
-	}
+	p = get_path_str(&uq, p, 0, &endp);
+	if (*endp)
+		die("Garbage after path in: %s", command_buf.buf);
 	tree_content_remove(&b->branch_tree, p, NULL);
 }
 
@@ -1966,29 +1978,21 @@ static void file_change_cr(struct branch *b, int rename)
 	struct tree_entry leaf;
 
 	s = command_buf.buf + 2;
-	strbuf_reset(&s_uq);
-	if (!unquote_c_style(&s_uq, s, &endp)) {
-		if (*endp != ' ')
-			die("Missing space after source: %s", command_buf.buf);
-	} else {
-		endp = strchr(s, ' ');
-		if (!endp)
-			die("Missing space after source: %s", command_buf.buf);
+	s = get_path_str(&s_uq, s, ' ', &endp);
+	if (*endp != ' ')
+		die("Missing space after source: %s", command_buf.buf);
+	if (s != s_uq.buf) {
 		strbuf_add(&s_uq, s, endp - s);
+		s = s_uq.buf;
 	}
-	s = s_uq.buf;
 
 	endp++;
 	if (!*endp)
 		die("Missing dest: %s", command_buf.buf);
 
-	d = endp;
-	strbuf_reset(&d_uq);
-	if (!unquote_c_style(&d_uq, d, &endp)) {
-		if (*endp)
-			die("Garbage after dest in: %s", command_buf.buf);
-		d = d_uq.buf;
-	}
+	d = get_path_str(&d_uq, endp, 0, &endp);
+	if (*endp)
+		die("Garbage after dest in: %s", command_buf.buf);
 
 	memset(&leaf, 0, sizeof(leaf));
 	if (rename)
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC 06/11] Add support for mark references as path names

[Johan Herland]

When using a mark reference as a path name, the mark reference will be
expanded to the 40-byte hex version of the object name associated with the
mark. This is useful e.g. when importing notes objects (where the filenames
in a notes tree are the object names of the annotated objects).

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/git-fast-import.txt |    9 +++++++--
 fast-import.c                     |   11 +++++++++--
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index c2f483a..bbc8b78 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -487,12 +487,17 @@ in octal.  Git only supports the following modes:
 
 In both formats `<path>` is the complete path of the file to be added
 (if not already existing) or modified (if already existing).
+`<path>` may also be a mark reference (`:<idnum>`) set by a prior
+command, which will expand to a full 40-byte SHA-1 of the Git object
+associated with the mark. This is useful e.g. when importing commit
+notes (the filenames in a notes commit are the object names of the
+annotated commits).
 
 A `<path>` string must use UNIX-style directory separators (forward
 slash `/`), may contain any byte other than `LF`, and must not
-start with double quote (`"`).
+start with double quote (`"`) or colon (`:`).
 
-If an `LF` or double quote must be encoded into `<path>` shell-style
+If an `LF`, double quote or colon must be encoded into `<path>` shell-style
 quoting should be used, e.g. `"path/with\n and \" in it"`.
 
 The value of `<path>` must be in canonical form. That is it must not:
diff --git a/fast-import.c b/fast-import.c
index 8a7cdc1..3edfcf0 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -85,13 +85,13 @@ Format of STDIN stream:
      # common escapes of 'c' (e..g \n, \t, \\, \") or \nnn where nnn
      # is the signed byte value in octal.  Note that the only
      # characters which must actually be escaped to protect the
-     # stream formatting is: \, " and LF.  Otherwise these values
+     # stream formatting is: \, ", : and LF.  Otherwise these values
      # are UTF8.
      #
   ref_str     ::= ref;
   sha1exp_str ::= sha1exp;
   tag_str     ::= tag;
-  path_str    ::= path    | '"' quoted(path)    '"' ;
+  path_str    ::= path | '"' quoted(path) '"' | idnum;
   mode        ::= '100644' | '644'
                 | '100755' | '755'
                 | '120000'
@@ -1864,6 +1864,13 @@ static const char *get_path_str(
 	if (!unquote_c_style(sb, p, endp)) {
 		/* successfully unquoted C-style quoted name */
 		p = sb->buf;
+	} else if (*p == ':') {
+		/* resolve mark reference */
+		char *x;
+		struct object_entry *pe = find_mark(strtoumax(p + 1, &x, 10));
+		*endp = x;
+		strbuf_add(sb, sha1_to_hex(pe->sha1), 40);
+		p = sb->buf;
 	} else {
 		/* regular path ending at endc */
 		*endp = strchr(p, endc);
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC 07/11] Preliminary clarifications to git-vcs documentation

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/git-vcs.txt |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-vcs.txt b/Documentation/git-vcs.txt
index 402c927..febe415 100644
--- a/Documentation/git-vcs.txt
+++ b/Documentation/git-vcs.txt
@@ -22,13 +22,15 @@ capabilities (limited both by the particular helper and by the
 capabilities of the system they connect to), and they report what
 capabilities they support.
 
-These programs can store refs in refs/<system>/*, and arbitrary
-information in info/<system>.
+These programs can store refs in refs/<system>/*, note refs in
+refs/notes/<system>/*, and arbitrary information in info/<system>/*.
 
 COMMANDS
 --------
 
 Commands are given by the caller on the helper's standard input, one per line.
+The output of each command must be produced on the helper's standard output.
+The helper's standard error stream can be used for status/progress messages.
 
 'capabilities'::
 	Outputs a single line with a list of feature names separated
@@ -45,6 +47,7 @@ Commands are given by the caller on the helper's standard input, one per line.
 	operations, such as importing the content, to see whether it
 	matches.) Other information, not yet supported, may be output
 	as well, separated by single spaces.
+	The output list shall be terminated with a blank line.
 
 'import' ref::
 	Imports the given ref by outputting it in git-fast-import
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC 08/11] Teach foreign transport code to perform the "capabilities" command

[Johan Herland]

The features reported by the vcs helper are stored in the foreign_data struct
which is expanded for this purpose.

In the process, we also change the capabilities command slightly to expect
one feature per line (instead of all features on a single line). This enables
us to add features in the future that take one or more arguments. To terminate
the list of features, a blank line is output at the end.

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/git-vcs.txt |    8 ++--
 transport-foreign.c       |   83 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 67 insertions(+), 24 deletions(-)

diff --git a/Documentation/git-vcs.txt b/Documentation/git-vcs.txt
index febe415..85656d4 100644
--- a/Documentation/git-vcs.txt
+++ b/Documentation/git-vcs.txt
@@ -33,10 +33,10 @@ The output of each command must be produced on the helper's standard output.
 The helper's standard error stream can be used for status/progress messages.
 
 'capabilities'::
-	Outputs a single line with a list of feature names separated
-	by spaces. Each of these indicates a supported feature of the
-	helper, and the caller will only attempt operations that are
-	supported.
+	Outputs a list of feature names, one per line. Each of these
+	indicates a supported feature of the helper, and the caller
+	will only attempt operations that are supported.
+	The output list shall be terminated with a blank line.
 
 'list'::
 	Outputs the names of refs, one per line. These may be
diff --git a/transport-foreign.c b/transport-foreign.c
index 29aad77..be0a587 100644
--- a/transport-foreign.c
+++ b/transport-foreign.c
@@ -8,38 +8,81 @@
 
 struct foreign_data
 {
-	struct child_process *importer;
+	struct child_process importer;
+
+	/* capabilities */
+	unsigned export:1;
+	unsigned export_branch:1;
+	unsigned export_merges:1;
 };
 
+static void get_foreign_capabilities(struct foreign_data *fdata)
+{
+	struct strbuf buf;
+	FILE *file;
+
+	write(fdata->importer.in, "capabilities\n", 13);
+
+	strbuf_init(&buf, 0);
+	file = fdopen(fdata->importer.out, "r");
+	while (1) {
+		char *eon;
+		if (strbuf_getline(&buf, file, '\n') == EOF)
+			break;
+
+		if (!*buf.buf)
+			break;
+
+		eon = strchr(buf.buf, ' ');
+		if (eon)
+			*eon = '\0';
+
+		/* parse features */
+		if (!strcmp(buf.buf, "export"))
+			fdata->export = 1;
+		else if (!strcmp(buf.buf, "export-branch"))
+			fdata->export_branch = 1;
+		else if (!strcmp(buf.buf, "export-merges"))
+			fdata->export_merges = 1;
+		else
+			die("Invalid feature '%s'", buf.buf);
+
+		strbuf_reset(&buf);
+	}
+
+	strbuf_release(&buf);
+}
+
 static struct child_process *get_importer(struct transport *transport)
 {
-	struct child_process *importer = transport->data;
-	if (!importer) {
+	struct foreign_data *fdata = (struct foreign_data *) transport->data;
+	if (!fdata) {
 		struct strbuf buf;
-		importer = xcalloc(1, sizeof(*importer));
-		importer->in = -1;
-		importer->out = -1;
-		importer->err = 0;
-		importer->argv = xcalloc(3, sizeof(*importer->argv));
+		fdata = xcalloc(1, sizeof(*fdata));
+		fdata->importer.in = -1;
+		fdata->importer.out = -1;
+		fdata->importer.err = 0;
+		fdata->importer.argv = xcalloc(3, sizeof(*fdata->importer.argv));
 		strbuf_init(&buf, 80);
 		strbuf_addf(&buf, "vcs-%s", transport->remote->foreign_vcs);
-		importer->argv[0] = buf.buf;
-		importer->argv[1] = transport->remote->name;
-		importer->git_cmd = 1;
-		start_command(importer);
-		transport->data = importer;
+		fdata->importer.argv[0] = buf.buf;
+		fdata->importer.argv[1] = transport->remote->name;
+		fdata->importer.git_cmd = 1;
+		start_command(&fdata->importer);
+		get_foreign_capabilities(fdata);
+		transport->data = fdata;
 	}
-	return importer;
+	return &fdata->importer;
 }
 
 static int disconnect_foreign(struct transport *transport)
 {
-	struct child_process *importer = transport->data;
-	if (importer) {
-		write(importer->in, "\n", 1);
-		close(importer->in);
-		finish_command(importer);
-		free(importer);
+	struct foreign_data *fdata = (struct foreign_data *) transport->data;
+	if (fdata) {
+		write(fdata->importer.in, "\n", 1);
+		close(fdata->importer.in);
+		finish_command(&fdata->importer);
+		free(fdata);
 		transport->data = NULL;
 	}
 	return 0;
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC 09/11] Introduce a 'marks <filename>' feature to the foreign transport code

[Johan Herland]

The 'marks' feature is reported by the vcs helper when it requires the
fast-import marks database to loaded/saved by the git-fast-import process
that is executed by the foreign transport machinery. The feature is
advertised along with exactly one argument: the location of the file
containing the marks database.

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/git-vcs.txt |    8 ++++++++
 transport-foreign.c       |   32 ++++++++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-vcs.txt b/Documentation/git-vcs.txt
index 85656d4..75815ba 100644
--- a/Documentation/git-vcs.txt
+++ b/Documentation/git-vcs.txt
@@ -65,6 +65,14 @@ there may be other restrictions on what may be exported.
 FEATURES
 --------
 
+'marks' filename::
+	Helper requires the marks from a git-fast-import run to be loaded
+	from, and saved to, the given filename. When this "feature" is
+	advertised, each git-fast-import run must load and save the
+	internal marks database (see the --import-marks and --export-marks
+	option to git-fast-import for more details) located at the given
+	filename.
+
 'export'::
 	Helper supports exporting commits, at least exporting
 	non-merge commits whose parents are not the parents of any
diff --git a/transport-foreign.c b/transport-foreign.c
index be0a587..ae9cb9a 100644
--- a/transport-foreign.c
+++ b/transport-foreign.c
@@ -11,6 +11,7 @@ struct foreign_data
 	struct child_process importer;
 
 	/* capabilities */
+	char *marks_filename;
 	unsigned export:1;
 	unsigned export_branch:1;
 	unsigned export_merges:1;
@@ -38,7 +39,12 @@ static void get_foreign_capabilities(struct foreign_data *fdata)
 			*eon = '\0';
 
 		/* parse features */
-		if (!strcmp(buf.buf, "export"))
+		if (!strcmp(buf.buf, "marks")) {
+			if (!eon)
+				die("Feature 'marks' requires an argument");
+			fdata->marks_filename = xstrdup(eon + 1);
+		}
+		else if (!strcmp(buf.buf, "export"))
 			fdata->export = 1;
 		else if (!strcmp(buf.buf, "export-branch"))
 			fdata->export_branch = 1;
@@ -82,6 +88,7 @@ static int disconnect_foreign(struct transport *transport)
 		write(fdata->importer.in, "\n", 1);
 		close(fdata->importer.in);
 		finish_command(&fdata->importer);
+		free(fdata->marks_filename);
 		free(fdata);
 		transport->data = NULL;
 	}
@@ -91,10 +98,13 @@ static int disconnect_foreign(struct transport *transport)
 static int fetch_refs_via_foreign(struct transport *transport,
 				  int nr_heads, struct ref **to_fetch)
 {
+	struct foreign_data *fdata;
 	struct child_process *importer;
 	struct child_process fastimport;
 	struct ref *posn;
+	struct strbuf export_marks, import_marks;
 	int i, count;
+	FILE *f;
 
 	count = 0;
 	for (i = 0; i < nr_heads; i++) {
@@ -105,12 +115,28 @@ static int fetch_refs_via_foreign(struct transport *transport,
 	}
 	if (count) {
 		importer = get_importer(transport);
+		fdata = (struct foreign_data *) transport->data;
+		strbuf_init(&export_marks, 0);
+		strbuf_init(&import_marks, 0);
 
 		memset(&fastimport, 0, sizeof(fastimport));
 		fastimport.in = importer->out;
-		fastimport.argv = xcalloc(3, sizeof(*fastimport.argv));
+		fastimport.argv = xcalloc(5, sizeof(*fastimport.argv));
 		fastimport.argv[0] = "fast-import";
 		fastimport.argv[1] = "--quiet";
+		if (fdata->marks_filename) {
+			strbuf_addf(&export_marks, "--export-marks=%s",
+				fdata->marks_filename);
+			fastimport.argv[2] = export_marks.buf;
+
+			f = fopen(fdata->marks_filename, "r");
+			if (f) {
+				strbuf_addf(&import_marks, "--import-marks=%s",
+					fdata->marks_filename);
+				fastimport.argv[3] = import_marks.buf;
+				fclose(f);
+			}
+		}
 		fastimport.git_cmd = 1;
 		start_command(&fastimport);
 
@@ -124,6 +150,8 @@ static int fetch_refs_via_foreign(struct transport *transport,
 		}
 		disconnect_foreign(transport);
 		finish_command(&fastimport);
+		strbuf_release(&export_marks);
+		strbuf_release(&import_marks);
 	}
 	for (i = 0; i < nr_heads; i++) {
 		posn = to_fetch[i];
-- 
1.6.4.rc3.138.ga6b98.dirty

[RFC 11/11] Add simple test cases of git-vcs-cvs functionality

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t9800-foreign-vcs-cvs-basic.sh |  518 ++++++++++++++++++++++++++++++++++++++
 t/t9801-foreign-vcs-cvs-fetch.sh |  291 +++++++++++++++++++++
 t/test-lib.sh                    |    1 +
 3 files changed, 810 insertions(+), 0 deletions(-)
 create mode 100755 t/t9800-foreign-vcs-cvs-basic.sh
 create mode 100755 t/t9801-foreign-vcs-cvs-fetch.sh

diff --git a/t/t9800-foreign-vcs-cvs-basic.sh b/t/t9800-foreign-vcs-cvs-basic.sh
new file mode 100755
index 0000000..4e29e62
--- /dev/null
+++ b/t/t9800-foreign-vcs-cvs-basic.sh
@@ -0,0 +1,518 @@
+#!/bin/sh
+
+test_description='git vcs-cvs basic tests'
+. ./test-lib.sh
+
+if ! test_have_prereq PYTHON; then
+	say 'skipping CVS foreign-vcs helper tests, python not available'
+	test_done
+fi
+
+CVS_EXEC=cvs
+CVS_OPTS="-f -q"
+CVS="$CVS_EXEC $CVS_OPTS"
+
+CVSROOT=$(pwd)/cvsroot
+export CVSROOT
+unset CVS_SERVER
+
+CVSMODULE=cvsmodule
+GITREMOTE=cvsremote
+
+if ! type $CVS_EXEC >/dev/null 2>&1
+then
+	say 'skipping vcs-cvs tests, $CVS_EXEC not found'
+	test_done
+fi
+
+test_expect_success 'setup cvsroot' '$CVS init'
+
+test_expect_success '#1: setup a cvs module' '
+
+	mkdir "$CVSROOT/$CVSMODULE" &&
+	$CVS co -d module-cvs $CVSMODULE &&
+	(
+		cd module-cvs &&
+		cat <<EOF >o_fortuna &&
+O Fortuna
+velut luna
+statu variabilis,
+
+semper crescis
+aut decrescis;
+vita detestabilis
+
+nunc obdurat
+et tunc curat
+ludo mentis aciem,
+
+egestatem,
+potestatem
+dissolvit ut glaciem.
+EOF
+		$CVS add o_fortuna &&
+		cat <<EOF >message &&
+add "O Fortuna" lyrics
+
+These public domain lyrics make an excellent sample text.
+EOF
+		$CVS commit -f -F message o_fortuna
+	)
+'
+
+test_expect_success 'set up CVS repo as a foreign remote' '
+
+	git config "user.name" "Test User"
+	git config "user.email" "test@example.com"
+	git config "remote.$GITREMOTE.vcs" cvs
+	git config "remote.$GITREMOTE.cvsRoot" "$CVSROOT"
+	git config "remote.$GITREMOTE.cvsModule" "$CVSMODULE"
+	git config "remote.$GITREMOTE.fetch" \
+		"+refs/cvs/$GITREMOTE/*:refs/remotes/$GITREMOTE/*"
+
+'
+
+test_expect_success '#1: git-vcs-cvs "capabilities" command' '
+
+	echo "capabilities" | git vcs-cvs "$GITREMOTE" > actual &&
+	cat <<EOF >expect &&
+marks .git/info/cvs/$GITREMOTE/marks
+
+EOF
+	test_cmp expect actual
+
+'
+
+test_expect_success '#1: git-vcs-cvs "list" command' '
+
+	echo "list" | git vcs-cvs "$GITREMOTE" > actual &&
+	cat <<EOF >expect &&
+refs/cvs/$GITREMOTE/HEAD changed
+
+EOF
+	test_cmp expect actual
+
+'
+
+test_expect_success '#1: git-vcs-cvs "import" command' '
+
+	echo "import refs/cvs/$GITREMOTE/HEAD" | git vcs-cvs "$GITREMOTE" > actual &&
+	cat <<EOF >expect &&
+# Importing CVS revision o_fortuna:1.1
+blob
+mark :1
+data 180
+O Fortuna
+velut luna
+statu variabilis,
+
+semper crescis
+aut decrescis;
+vita detestabilis
+
+nunc obdurat
+et tunc curat
+ludo mentis aciem,
+
+egestatem,
+potestatem
+dissolvit ut glaciem.
+
+commit refs/cvs/$GITREMOTE/HEAD
+mark :2
+data 82
+add "O Fortuna" lyrics
+
+These public domain lyrics make an excellent sample text.
+
+M 644 :1 o_fortuna
+
+# Importing note for object 2
+blob
+mark :3
+data 14
+o_fortuna:1.1
+
+commit refs/notes/cvs/$GITREMOTE
+mark :4
+data 43
+Annotate commits imported by "git vcs-cvs"
+
+M 644 :3 :2
+
+blob
+mark :5
+data 32
+1 o_fortuna:1.1
+2 o_fortuna:1.1
+
+blob
+mark :6
+data 16
+blob 1
+commit 2
+
+commit refs/cvs/$GITREMOTE/_metadata
+mark :7
+data 39
+Updated metadata used by "git vcs-cvs"
+
+M 644 :5 CVS/marks
+M 644 :6 o_fortuna/1.1
+
+EOF
+	grep -v "^committer " actual > actual.filtered &&
+	test_cmp expect actual.filtered
+
+'
+
+test_expect_success '#1: Passing git-vcs-cvs output to git-fast-import' '
+
+	git fast-import --quiet \
+		--export-marks=".git/info/cvs/$GITREMOTE/marks" \
+		< actual &&
+	git gc
+
+'
+
+test_expect_success '#1: Verifying correctness of import' '
+
+	echo "verify HEAD" | git vcs-cvs "$GITREMOTE"
+
+'
+
+test_expect_success '#2: update cvs module' '
+
+	(
+		cd module-cvs &&
+		cat <<EOF >o_fortuna &&
+O Fortune,
+like the moon
+you are changeable,
+
+ever waxing
+and waning;
+hateful life
+
+first oppresses
+and then soothes
+as fancy takes it;
+
+poverty
+and power
+it melts them like ice.
+EOF
+		cat <<EOF >message &&
+translate to English
+
+My Latin is terrible.
+EOF
+		$CVS commit -f -F message o_fortuna
+	)
+'
+
+test_expect_success '#2: git-vcs-cvs "capabilities" command' '
+
+	echo "capabilities" | git vcs-cvs "$GITREMOTE" > actual &&
+	cat <<EOF >expect &&
+marks .git/info/cvs/$GITREMOTE/marks
+
+EOF
+	test_cmp expect actual
+
+'
+
+test_expect_success '#2: git-vcs-cvs "list" command' '
+
+	echo "list" | git vcs-cvs "$GITREMOTE" > actual &&
+	cat <<EOF >expect &&
+refs/cvs/$GITREMOTE/HEAD changed
+
+EOF
+	test_cmp expect actual
+
+'
+
+test_expect_success '#2: git-vcs-cvs "import" command' '
+
+	echo "import refs/cvs/$GITREMOTE/HEAD" | git vcs-cvs "$GITREMOTE" > actual &&
+	cat <<EOF >expect &&
+# Importing CVS revision o_fortuna:1.2
+blob
+mark :8
+data 179
+O Fortune,
+like the moon
+you are changeable,
+
+ever waxing
+and waning;
+hateful life
+
+first oppresses
+and then soothes
+as fancy takes it;
+
+poverty
+and power
+it melts them like ice.
+
+commit refs/cvs/$GITREMOTE/HEAD
+mark :9
+data 44
+translate to English
+
+My Latin is terrible.
+
+from refs/cvs/$GITREMOTE/HEAD^0
+M 644 :8 o_fortuna
+
+# Importing note for object 9
+blob
+mark :10
+data 14
+o_fortuna:1.2
+
+commit refs/notes/cvs/$GITREMOTE
+mark :11
+data 43
+Annotate commits imported by "git vcs-cvs"
+
+from refs/notes/cvs/$GITREMOTE^0
+M 644 :10 :9
+
+blob
+mark :12
+data 32
+8 o_fortuna:1.2
+9 o_fortuna:1.2
+
+blob
+mark :13
+data 94
+
+blob
+mark :14
+data 16
+blob 8
+commit 9
+
+commit refs/cvs/$GITREMOTE/_metadata
+mark :15
+data 39
+Updated metadata used by "git vcs-cvs"
+
+from refs/cvs/$GITREMOTE/_metadata^0
+M 644 :12 CVS/marks
+M 644 :13 o_fortuna/1.1
+M 644 :14 o_fortuna/1.2
+
+EOF
+	grep -v -e "^committer " -e "\b[0-9a-f]\{40\}\b" actual > actual.filtered &&
+	test_cmp expect actual.filtered
+
+'
+
+test_expect_success '#2: Passing git-vcs-cvs output to git-fast-import' '
+
+	git fast-import --quiet \
+		--import-marks=".git/info/cvs/$GITREMOTE/marks" \
+		--export-marks=".git/info/cvs/$GITREMOTE/marks" \
+		< actual &&
+	git gc
+
+'
+
+test_expect_success '#2: Verifying correctness of import' '
+
+	echo "verify HEAD" | git vcs-cvs "$GITREMOTE"
+
+'
+
+test_expect_success '#3: update cvs module' '
+
+	(
+		cd module-cvs &&
+		echo 1 >tick &&
+		$CVS add tick &&
+		$CVS commit -f -m 1 tick
+	)
+
+'
+
+test_expect_success '#3: git-vcs-cvs "capabilities" command' '
+
+	echo "capabilities" | git vcs-cvs "$GITREMOTE" > actual &&
+	cat <<EOF >expect &&
+marks .git/info/cvs/$GITREMOTE/marks
+
+EOF
+	test_cmp expect actual
+
+'
+
+test_expect_success '#3: git-vcs-cvs "list" command' '
+
+	echo "list" | git vcs-cvs "$GITREMOTE" > actual &&
+	cat <<EOF >expect &&
+refs/cvs/$GITREMOTE/HEAD changed
+
+EOF
+	test_cmp expect actual
+
+'
+
+test_expect_success '#3: git-vcs-cvs "import" command' '
+
+	echo "import refs/cvs/$GITREMOTE/HEAD" | git vcs-cvs "$GITREMOTE" > actual &&
+	cat <<EOF >expect &&
+# Importing CVS revision tick:1.1
+blob
+mark :16
+data 2
+1
+
+commit refs/cvs/$GITREMOTE/HEAD
+mark :17
+data 2
+1
+
+from refs/cvs/$GITREMOTE/HEAD^0
+M 644 :16 tick
+
+# Importing note for object 17
+blob
+mark :18
+data 23
+o_fortuna:1.2
+tick:1.1
+
+commit refs/notes/cvs/$GITREMOTE
+mark :19
+data 43
+Annotate commits imported by "git vcs-cvs"
+
+from refs/notes/cvs/$GITREMOTE^0
+M 644 :18 :17
+
+blob
+mark :20
+data 41
+16 tick:1.1
+17 tick:1.1
+17 o_fortuna:1.2
+
+blob
+mark :21
+data 104
+commit 17
+
+blob
+mark :22
+data 18
+blob 16
+commit 17
+
+commit refs/cvs/$GITREMOTE/_metadata
+mark :23
+data 39
+Updated metadata used by "git vcs-cvs"
+
+from refs/cvs/$GITREMOTE/_metadata^0
+M 644 :20 CVS/marks
+M 644 :21 o_fortuna/1.2
+M 644 :22 tick/1.1
+
+EOF
+	grep -v -e "^committer " -e "\b[0-9a-f]\{40\}\b" actual > actual.filtered &&
+	test_cmp expect actual.filtered
+
+'
+
+test_expect_success '#3: Passing git-vcs-cvs output to git-fast-import' '
+
+	git fast-import --quiet \
+		--import-marks=".git/info/cvs/$GITREMOTE/marks" \
+		--export-marks=".git/info/cvs/$GITREMOTE/marks" \
+		< actual &&
+	git gc
+
+'
+
+test_expect_success '#3: Verifying correctness of import' '
+
+	echo "verify HEAD" | git vcs-cvs "$GITREMOTE"
+
+'
+
+test_expect_success '#4: git-vcs-cvs "capabilities" command' '
+
+	echo "capabilities" | git vcs-cvs "$GITREMOTE" > actual &&
+	cat <<EOF >expect &&
+marks .git/info/cvs/$GITREMOTE/marks
+
+EOF
+	test_cmp expect actual
+
+'
+
+test_expect_success '#4: git-vcs-cvs "list" command' '
+
+	echo "list" | git vcs-cvs "$GITREMOTE" > actual &&
+	cat <<EOF >expect &&
+refs/cvs/$GITREMOTE/HEAD unchanged
+
+EOF
+	test_cmp expect actual
+
+'
+
+test_expect_success '#4: git-vcs-cvs "import" command' '
+
+	echo "import refs/cvs/$GITREMOTE/HEAD" | git vcs-cvs "$GITREMOTE" > actual &&
+	cat <<EOF >expect &&
+blob
+mark :24
+data 0
+
+blob
+mark :25
+data 142
+
+blob
+mark :26
+data 94
+
+commit refs/cvs/$GITREMOTE/_metadata
+mark :27
+data 39
+Updated metadata used by "git vcs-cvs"
+
+from refs/cvs/$GITREMOTE/_metadata^0
+M 644 :24 CVS/marks
+M 644 :25 o_fortuna/1.2
+M 644 :26 tick/1.1
+
+EOF
+	grep -v -e "^committer " -e "\b[0-9a-f]\{40\}\b" actual > actual.filtered &&
+	test_cmp expect actual.filtered
+
+'
+
+test_expect_success '#4: Passing git-vcs-cvs output to git-fast-import' '
+
+	git fast-import --quiet \
+		--import-marks=".git/info/cvs/$GITREMOTE/marks" \
+		--export-marks=".git/info/cvs/$GITREMOTE/marks" \
+		< actual &&
+	git gc
+
+'
+
+test_expect_success '#4: Verifying correctness of import' '
+
+	echo "verify HEAD" | git vcs-cvs "$GITREMOTE"
+
+'
+
+test_done
diff --git a/t/t9801-foreign-vcs-cvs-fetch.sh b/t/t9801-foreign-vcs-cvs-fetch.sh
new file mode 100755
index 0000000..62a2325
--- /dev/null
+++ b/t/t9801-foreign-vcs-cvs-fetch.sh
@@ -0,0 +1,291 @@
+#!/bin/sh
+
+test_description='git vcs-cvs basic tests'
+. ./test-lib.sh
+
+if ! test_have_prereq PYTHON; then
+	say 'skipping CVS foreign-vcs helper tests, python not available'
+	test_done
+fi
+
+CVS_EXEC=cvs
+CVS_OPTS="-f -q"
+CVS="$CVS_EXEC $CVS_OPTS"
+
+CVSROOT=$(pwd)/cvsroot
+export CVSROOT
+unset CVS_SERVER
+
+CVSMODULE=cvsmodule
+GITREMOTE=cvsremote
+
+if ! type $CVS_EXEC >/dev/null 2>&1
+then
+	say 'skipping vcs-cvs tests, $CVS_EXEC not found'
+	test_done
+fi
+
+verify () {
+	git log --reverse --format="--- %T%n%s%n%n%b" "$GITREMOTE/$1" >actual &&
+	test_cmp "expect.$1" actual &&
+	echo "verify $1" | git vcs-cvs "$GITREMOTE"
+}
+
+test_expect_success 'setup CVS repo' '$CVS init'
+
+test_expect_success 'create CVS module with initial commit' '
+
+	mkdir "$CVSROOT/$CVSMODULE" &&
+	$CVS co -d module-cvs $CVSMODULE &&
+	(
+		cd module-cvs &&
+		cat <<EOF >o_fortuna &&
+O Fortuna
+velut luna
+statu variabilis,
+
+semper crescis
+aut decrescis;
+vita detestabilis
+
+nunc obdurat
+et tunc curat
+ludo mentis aciem,
+
+egestatem,
+potestatem
+dissolvit ut glaciem.
+EOF
+		$CVS add o_fortuna &&
+		cat <<EOF >message &&
+add "O Fortuna" lyrics
+
+These public domain lyrics make an excellent sample text.
+EOF
+		$CVS commit -f -F message o_fortuna
+	)
+'
+
+test_expect_success 'set up CVS repo/module as a foreign remote' '
+
+	git config "user.name" "Test User"
+	git config "user.email" "test@example.com"
+	git config "remote.$GITREMOTE.vcs" cvs
+	git config "remote.$GITREMOTE.cvsRoot" "$CVSROOT"
+	git config "remote.$GITREMOTE.cvsModule" "$CVSMODULE"
+	git config "remote.$GITREMOTE.fetch" \
+		"+refs/cvs/$GITREMOTE/*:refs/remotes/$GITREMOTE/*"
+
+'
+
+test_expect_success 'initial fetch from CVS remote' '
+
+	cat <<EOF >expect.HEAD &&
+--- 0e06d780dedab23e683c686fb041daa9a84c936c
+add "O Fortuna" lyrics
+
+These public domain lyrics make an excellent sample text.
+
+EOF
+	git fetch "$GITREMOTE" &&
+	verify HEAD
+
+'
+
+test_expect_success 'CVS commit' '
+
+	(
+		cd module-cvs &&
+		cat <<EOF >o_fortuna &&
+O Fortune,
+like the moon
+you are changeable,
+
+ever waxing
+and waning;
+hateful life
+
+first oppresses
+and then soothes
+as fancy takes it;
+
+poverty
+and power
+it melts them like ice.
+EOF
+		cat <<EOF >message &&
+translate to English
+
+My Latin is terrible.
+EOF
+		$CVS commit -f -F message o_fortuna
+	) &&
+	cat <<EOF >>expect.HEAD &&
+--- daa87269a5e00388135ad9542dc16ab6754466e5
+translate to English
+
+My Latin is terrible.
+
+EOF
+	git fetch "$GITREMOTE" &&
+	verify HEAD
+
+'
+
+test_expect_success 'CVS commit with new file' '
+
+	(
+		cd module-cvs &&
+		echo 1 >tick &&
+		$CVS add tick &&
+		$CVS commit -f -m 1 tick
+	) &&
+	cat <<EOF >>expect.HEAD &&
+--- 486935b4fccecea9b64cbed3a797ebbcbe2b7461
+1
+
+
+EOF
+	git fetch "$GITREMOTE" &&
+	verify HEAD
+
+'
+
+test_expect_success 'fetch without CVS changes' '
+
+	git fetch "$GITREMOTE" &&
+	verify HEAD
+
+'
+
+test_expect_success 'add 2 CVS commits' '
+
+	(
+		cd module-cvs &&
+		echo 2 >tick &&
+		$CVS commit -f -m 2 tick &&
+		echo 3 >tick &&
+		$CVS commit -f -m 3 tick
+	) &&
+	cat <<EOF >>expect.HEAD &&
+--- 83437ab3e57bf0a42915de5310e3419792b5a36f
+2
+
+
+--- 60fc50406a82dc6bd32dc6e5f7bd23e4c3cdf7ef
+3
+
+
+EOF
+	git fetch "$GITREMOTE" &&
+	verify HEAD
+
+'
+
+test_expect_success 'CVS commit with removed file' '
+
+	(
+		cd module-cvs &&
+		$CVS remove -f tick &&
+		$CVS commit -f -m "remove file" tick
+	) &&
+	cat <<EOF >>expect.HEAD &&
+--- daa87269a5e00388135ad9542dc16ab6754466e5
+remove file
+
+
+EOF
+	git fetch "$GITREMOTE" &&
+	verify HEAD
+
+'
+
+test_expect_success 'CVS commit with several new files' '
+
+	(
+		cd module-cvs &&
+		echo spam >spam &&
+		echo sausage >sausage &&
+		echo eggs >eggs &&
+		$CVS add spam sausage eggs &&
+		$CVS commit -f -m "spam, sausage, and eggs" spam sausage eggs
+	) &&
+	cat <<EOF >>expect.HEAD &&
+--- 3190dfce44a6d5e9916b4870dbf8f37d1ca4ddaf
+spam, sausage, and eggs
+
+
+EOF
+	git fetch "$GITREMOTE" &&
+	verify HEAD
+
+'
+
+test_expect_success 'new CVS branch' '
+
+	(
+		cd module-cvs &&
+		$CVS tag -b foo
+	) &&
+	cp expect.HEAD expect.foo &&
+	git fetch "$GITREMOTE" &&
+	verify HEAD &&
+	verify foo
+
+'
+
+test_expect_success 'CVS commit on branch' '
+
+	(
+		cd module-cvs &&
+		$CVS up -r foo &&
+		echo "spam spam spam" >spam &&
+		$CVS commit -f -m "commit on branch foo" spam
+	) &&
+	cat <<EOF >>expect.foo &&
+--- 1aba123e5c83898ce3a8b976cc6064d60246aef4
+commit on branch foo
+
+
+EOF
+	git fetch "$GITREMOTE" &&
+	verify HEAD &&
+	verify foo
+
+'
+
+test_expect_success 'create CVS tag' '
+
+	(
+		cd module-cvs &&
+		$CVS tag bar
+	) &&
+	cp expect.foo expect.bar &&
+	git fetch "$GITREMOTE" &&
+	verify HEAD &&
+	verify foo &&
+	verify bar
+
+'
+
+test_expect_success 'another CVS commit on branch' '
+
+	(
+		cd module-cvs &&
+		echo "spam spam spam spam spam spam" >> spam &&
+		$CVS commit -f -m "another commit on branch foo" spam
+	) &&
+	cat <<EOF >>expect.foo &&
+--- 15a2635e76e8e5a5a8746021643de317452f2340
+another commit on branch foo
+
+
+EOF
+	git fetch "$GITREMOTE" &&
+	verify HEAD &&
+	verify foo &&
+	verify bar
+
+'
+
+test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5fdc5d9..8eb8b95 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -706,6 +706,7 @@ case $(uname -s) in
 esac
 
 test -z "$NO_PERL" && test_set_prereq PERL
+test -z "$NO_PYTHON" && test_set_prereq PYTHON
 
 # test whether the filesystem supports symbolic links
 ln -s x y 2>/dev/null && test -h y 2>/dev/null && test_set_prereq SYMLINKS
-- 
1.6.4.rc3.138.ga6b98.dirty

Re: [RFC 06/11] Add support for mark references as path names

[Shawn O. Pearce]

Johan Herland <johan@herland.net> wrote:
> When using a mark reference as a path name, the mark reference will be
> expanded to the 40-byte hex version of the object name associated with the
> mark. This is useful e.g. when importing notes objects (where the filenames
> in a notes tree are the object names of the annotated objects).
> 
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  Documentation/git-fast-import.txt |    9 +++++++--
>  fast-import.c                     |   11 +++++++++--
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index c2f483a..bbc8b78 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -487,12 +487,17 @@ in octal.  Git only supports the following modes:
>  
>  In both formats `<path>` is the complete path of the file to be added
>  (if not already existing) or modified (if already existing).
> +`<path>` may also be a mark reference (`:<idnum>`) set by a prior
> +command, which will expand to a full 40-byte SHA-1 of the Git object
> +associated with the mark. This is useful e.g. when importing commit
> +notes (the filenames in a notes commit are the object names of the
> +annotated commits).
>  
>  A `<path>` string must use UNIX-style directory separators (forward
>  slash `/`), may contain any byte other than `LF`, and must not
> -start with double quote (`"`).
> +start with double quote (`"`) or colon (`:`).

I'm worried about changing the path rules here.  Previously writing
a path as :1 was legal and produced a file named ":1" in the top
level directory of the repository.  Now it will create a file that
matches a mark.

I think you need to find another back door, something that the
language wouldn't have considered as valid previously.
  
-- 
Shawn.

Re: [RFC 00/11] Foreign VCS helper program for CVS repositories

[Daniel Barkalow]

On Mon, 27 Jul 2009, Johan Herland wrote:

> This series is a first draft towards implementing a foreign VCS helper
> program for CVS.
> 
> It is based on the 'db/foreign-scm' and 'jh/notes' topics in 'pu'. As such,
> this patch series should apply cleanly to current 'pu'.
> 
> The first 4 patches supply the necessary parts of Daniel Barkalow's
> foreign-scm topic that has not yet been merged to 'pu' (i.e. everything
> remaining in Daniel's original patch series, except the p4-specific stuff).

I've got some changes to this series in the works; I just made a few 
changes to share the transport-side code for interacting with the helper 
with the transport-side code for interacting with native-object helpers. 
Mainly, this means starting each line in the "list" response with "?" to 
indicate that you don't know the hash, since the protocol is trying to be 
compatible with cases where you do know.

> The next 2 patches add some functionality to git fast-import, for
> facilitating the import of 'notes' objects (this is a _much_ faster way to
> generate/import notes than invoking "git notes edit -m MSG" for each note).
> 
> Next, there are 3 patches tweaking and expanding the git-vcs API (with
> corresponding implementations in the foreign transport code) to adjust for
> the CVS helper's needs.

These make sense to me. I assume you're planning to support exporting in 
some ways, but haven't got it working yet? I largely ignored the 
capabilities stuff because I didn't have two systems with differing 
requirements to actually use it.

	-Daniel
*This .sig left intentionally blank*

Re: [RFC 00/11] Foreign VCS helper program for CVS repositories

[Johan Herland]

On Monday 27 July 2009, Daniel Barkalow wrote:
> On Mon, 27 Jul 2009, Johan Herland wrote:
> > This series is a first draft towards implementing a foreign VCS
> > helper program for CVS.
> >
> > It is based on the 'db/foreign-scm' and 'jh/notes' topics in 'pu'.
> > As such, this patch series should apply cleanly to current 'pu'.
> >
> > The first 4 patches supply the necessary parts of Daniel Barkalow's
> > foreign-scm topic that has not yet been merged to 'pu' (i.e.
> > everything remaining in Daniel's original patch series, except the
> > p4-specific stuff).
>
> I've got some changes to this series in the works; I just made a few
> changes to share the transport-side code for interacting with the
> helper with the transport-side code for interacting with
> native-object helpers. Mainly, this means starting each line in the
> "list" response with "?" to indicate that you don't know the hash,
> since the protocol is trying to be compatible with cases where you do
> know.

No problem. I'm actually considering whether I should rewrite the CVS 
helper to a full-fledged git-shim (btw, thanks for the shim work; it 
looks really promising). I would still probably have to use 
git-fast-import, but I would have more control over the fast-import 
process (e.g. by closing the fast-import process myself, I could 
simplify the code maintaining the marks database).

With the git-shim feature already having found an excellent use case 
(the HTTP fetcher), how do you see the future for the foreign-scm 
topic? I like the idea of rewriting foreign-scm on top of git-shim, but 
is there anything substantial _left_ in foreign-scm after such a 
rewrite?

> > Next, there are 3 patches tweaking and expanding the git-vcs API
> > (with corresponding implementations in the foreign transport code)
> > to adjust for the CVS helper's needs.
>
> These make sense to me. I assume you're planning to support exporting
> in some ways, but haven't got it working yet?

Indeed. The current version is only a first draft that fetches 
successfully from simple toy repositories (but probably not many 
real-world CVS repos). I hope to support both fetching and pushing to 
real-world CVS repos at some point.


Have fun! :)

...Johan

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

Re: [RFC 06/11] Add support for mark references as path names

[Johan Herland]

On Monday 27 July 2009, Shawn O. Pearce wrote:
> Johan Herland <johan@herland.net> wrote:
> > When using a mark reference as a path name, the mark reference will
> > be expanded to the 40-byte hex version of the object name
> > associated with the mark. This is useful e.g. when importing notes
> > objects (where the filenames in a notes tree are the object names
> > of the annotated objects).
> >
> > [...cut...]
>
> I'm worried about changing the path rules here.  Previously writing
> a path as :1 was legal and produced a file named ":1" in the top
> level directory of the repository.  Now it will create a file that
> matches a mark.
>
> I think you need to find another back door, something that the
> language wouldn't have considered as valid previously.

Ok. One (moderately hideous) option is to introduce new commands m/d/r/c 
that works just like M/D/R/C, except that they also expand mark 
references (leaving M/D/R/C with the old behaviour). I don't know how 
you'd feel about that...

However, I'm also pondering Dscho's idea of organizing note trees into 
hierarchies (to limit #entries in tree objects), and (although I have 
yet to try to implement this) it feels like this transformation should 
be done in the notes.c code, which means that it must happen on 
the "git side" of fast-import (at least the transformation should not 
happen on the "external side" of fast-import). This ultimately means 
that we cannot handle notes as "regular" trees and commits (which is 
what I'm trying to do with the help of this patch), and suggests that 
instead of using 'commit' with 'M' for adding notes, we should probably 
have a dedicated 'note' fast-import command that collects note objects, 
and does the tree transformation and subsequent commit upon 
checkpoint/exit.


Have fun! :)

...Johan

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

Re: [RFC 06/11] Add support for mark references as path names

[Shawn O. Pearce]

Johan Herland <johan@herland.net> wrote:
> On Monday 27 July 2009, Shawn O. Pearce wrote:
> > I think you need to find another back door, something that the
> > language wouldn't have considered as valid previously.
> 
> Ok. One (moderately hideous) option is to introduce new commands m/d/r/c 
> that works just like M/D/R/C, except that they also expand mark 
> references (leaving M/D/R/C with the old behaviour). I don't know how 
> you'd feel about that...

Bleh.  Because I think this has more truth to it:
 
> However, I'm also pondering Dscho's idea of organizing note trees into 
> hierarchies (to limit #entries in tree objects), and (although I have 
> yet to try to implement this) it feels like this transformation should 
> be done in the notes.c code, which means that it must happen on 
> the "git side" of fast-import (at least the transformation should not 
> happen on the "external side" of fast-import). This ultimately means 
> that we cannot handle notes as "regular" trees and commits (which is 
> what I'm trying to do with the help of this patch), and suggests that 
> instead of using 'commit' with 'M' for adding notes, we should probably 
> have a dedicated 'note' fast-import command that collects note objects, 
> and does the tree transformation and subsequent commit upon 
> checkpoint/exit.

Exactly.  We probably want to do this instead.

Though you might want the "note" command to be a subcommand of
commit, like M/D/R/C are, because notes are wrapped up inside of
a commit and you can modify multiple notes in a single commit.

-- 
Shawn.

Re: [RFC 03/11] Allow programs to not depend on remotes having urls

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
> index 78a88f7..4c6fc58 100644
> --- a/builtin-ls-remote.c
> +++ b/builtin-ls-remote.c
> @@ -87,9 +87,9 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>  	remote = nongit ? NULL : remote_get(dest);
> -	if (remote && !remote->url_nr)
> +	if (!nongit && !remote)
>  		die("remote %s has no configured URL", dest);

It appears to me that remote_get() calls make_remote() that never returns
NULL, thus this check will never trigger.  At least, the die() message
needs to be updated, since remote_get() may return NULL for some reason
other than "no configured URL", but the condition the message says is not
what this code checks.

It is Ok if the new world order is that the URL does not have to be known
before transport_get() is called, but if that is the case, it should be
made clear by removing this check from here (and possibly having a new
check elsewhere where it really matters, e.g. somewhere in transport
code).

> -	transport = transport_get(remote, remote ? remote->url[0] : dest);
> +	transport = transport_get(remote, remote ? NULL : dest);
>  	if (uploadpack != NULL)
>  		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);

Re: [RFC 00/11] Foreign VCS helper program for CVS repositories

[Daniel Barkalow]

On Mon, 27 Jul 2009, Johan Herland wrote:

> On Monday 27 July 2009, Daniel Barkalow wrote:
> > On Mon, 27 Jul 2009, Johan Herland wrote:
> > > This series is a first draft towards implementing a foreign VCS
> > > helper program for CVS.
> > >
> > > It is based on the 'db/foreign-scm' and 'jh/notes' topics in 'pu'.
> > > As such, this patch series should apply cleanly to current 'pu'.
> > >
> > > The first 4 patches supply the necessary parts of Daniel Barkalow's
> > > foreign-scm topic that has not yet been merged to 'pu' (i.e.
> > > everything remaining in Daniel's original patch series, except the
> > > p4-specific stuff).
> >
> > I've got some changes to this series in the works; I just made a few
> > changes to share the transport-side code for interacting with the
> > helper with the transport-side code for interacting with
> > native-object helpers. Mainly, this means starting each line in the
> > "list" response with "?" to indicate that you don't know the hash,
> > since the protocol is trying to be compatible with cases where you do
> > know.
> 
> No problem. I'm actually considering whether I should rewrite the CVS 
> helper to a full-fledged git-shim (btw, thanks for the shim work; it 
> looks really promising). I would still probably have to use 
> git-fast-import, but I would have more control over the fast-import 
> process (e.g. by closing the fast-import process myself, I could 
> simplify the code maintaining the marks database).
> 
> With the git-shim feature already having found an excellent use case 
> (the HTTP fetcher), how do you see the future for the foreign-scm 
> topic? I like the idea of rewriting foreign-scm on top of git-shim, but 
> is there anything substantial _left_ in foreign-scm after such a 
> rewrite?

Things that are still there:

 - vcs helpers make active use of the ability to state that there is a 
   remote ref by a particular name, without having any idea what hash it 
   currently holds (until they're told to import it).

 - vcs helper is specified directly by the user's config, possibly to 
   invoke a helper distributed separately; shim helpers are determined by 
   transport_get() when it identifies that the url can be handled by that 
   shim.

 - vcs helpers don't necessarily use URLs, or anything that works in the 
   same way, to identify the remote repo.

 - vcs export wants to deal with the remote server pretty much always 
   rewriting exported commits.

 - For export, the vcs helper is going to want to get the commits ordered 
   in some way that makes each export a single-commit export operation, so 
   that each helper doesn't have to deal with ordering the commits 
   suitably; this is particularly significant because the transport-side 
   code should make sure that the history is something that can be 
   represented in the foreign vcs. The native shim helper wants just the 
   endpoint.

> > > Next, there are 3 patches tweaking and expanding the git-vcs API
> > > (with corresponding implementations in the foreign transport code)
> > > to adjust for the CVS helper's needs.
> >
> > These make sense to me. I assume you're planning to support exporting
> > in some ways, but haven't got it working yet?
> 
> Indeed. The current version is only a first draft that fetches 
> successfully from simple toy repositories (but probably not many 
> real-world CVS repos). I hope to support both fetching and pushing to 
> real-world CVS repos at some point.

Great.
	-Daniel
*This .sig left intentionally blank*

Re: [RFC 03/11] Allow programs to not depend on remotes having urls

[Daniel Barkalow]

On Mon, 27 Jul 2009, Junio C Hamano wrote:

> Johan Herland <johan@herland.net> writes:
> 
> > diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
> > index 78a88f7..4c6fc58 100644
> > --- a/builtin-ls-remote.c
> > +++ b/builtin-ls-remote.c
> > @@ -87,9 +87,9 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
> >  		}
> >  	}
> >  	remote = nongit ? NULL : remote_get(dest);
> > -	if (remote && !remote->url_nr)
> > +	if (!nongit && !remote)
> >  		die("remote %s has no configured URL", dest);
> 
> It appears to me that remote_get() calls make_remote() that never returns
> NULL, thus this check will never trigger.  At least, the die() message
> needs to be updated, since remote_get() may return NULL for some reason
> other than "no configured URL", but the condition the message says is not
> what this code checks.

The previous patch in the series causes remote_get() to return NULL if 
the remote configuration is invalid. You're right, however, that the 
message should be something more directly accurate.

> It is Ok if the new world order is that the URL does not have to be known
> before transport_get() is called, but if that is the case, it should be
> made clear by removing this check from here (and possibly having a new
> check elsewhere where it really matters, e.g. somewhere in transport
> code).

This spot is trying to make sure that the remote was valid, according to 
remote.c

> > -	transport = transport_get(remote, remote ? remote->url[0] : dest);
> > +	transport = transport_get(remote, remote ? NULL : dest);
> >  	if (uploadpack != NULL)
> >  		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[RFC 06/11 v2] fast-import: Add support for importing commit notes

[Johan Herland]

Introduce a 'notemodify' subcommand of the 'commit' command. This subcommand
is similar to 'filemodify', except that no mode is supplied (all notes have
mode 0644), and the path is set to the hex SHA1 of the given "comittish".

This enables fast import of note objects along with their associated commits,
since the notes can now be named using the mark references of their
corresponding commits.

The patch also includes a test case of the added functionality.

Signed-off-by: Johan Herland <johan@herland.net>
---

On Monday 27 July 2009, Shawn O. Pearce wrote:
> Though you might want the "note" command to be a subcommand of
> commit, like M/D/R/C are, because notes are wrapped up inside of
> a commit and you can modify multiple notes in a single commit.

Thanks for the suggestion. Here's a replacement patch introducing a new
'notemodify' ("N") command.

Note that this patch also makes the preceding patch superfluous:

  [RFC 05/11] Refactor path name parsing into new function: get_path_str()

If you think that patch is useful in its own right, I can keep it in the
series, although it has no connection to the rest of the series at this
point.

Finally, this patch should probably migrate into the jh/notes topic...


Have fun! :)

...Johan


 Documentation/git-fast-import.txt |   45 +++++++++--
 fast-import.c                     |   88 +++++++++++++++++++-
 t/t9300-fast-import.sh            |  166 +++++++++++++++++++++++++++++++++++++
 3 files changed, 289 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index c2f483a..288032c 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -316,7 +316,7 @@ change to the project.
 	data
 	('from' SP <committish> LF)?
 	('merge' SP <committish> LF)?
-	(filemodify | filedelete | filecopy | filerename | filedeleteall)*
+	(filemodify | filedelete | filecopy | filerename | filedeleteall | notemodify)*
 	LF?
 ....
 
@@ -339,14 +339,13 @@ commit message use a 0 length data.  Commit messages are free-form
 and are not interpreted by Git.  Currently they must be encoded in
 UTF-8, as fast-import does not permit other encodings to be specified.
 
-Zero or more `filemodify`, `filedelete`, `filecopy`, `filerename`
-and `filedeleteall` commands
+Zero or more `filemodify`, `filedelete`, `filecopy`, `filerename`,
+`filedeleteall` and `notemodify` commands
 may be included to update the contents of the branch prior to
 creating the commit.  These commands may be supplied in any order.
 However it is recommended that a `filedeleteall` command precede
-all `filemodify`, `filecopy` and `filerename` commands in the same
-commit, as `filedeleteall`
-wipes the branch clean (see below).
+all `filemodify`, `filecopy`, `filerename` and `notemodify` commands in
+the same commit, as `filedeleteall` wipes the branch clean (see below).
 
 The `LF` after the command is optional (it used to be required).
 
@@ -595,6 +594,40 @@ more memory per active branch (less than 1 MiB for even most large
 projects); so frontends that can easily obtain only the affected
 paths for a commit are encouraged to do so.
 
+`notemodify`
+^^^^^^^^^^^^
+Included in a `commit` command to add a new note (annotating a given
+commit) or change the content of an existing note.  This command has
+two different means of specifying the content of the note.
+
+External data format::
+	The data content for the note was already supplied by a prior
+	`blob` command.  The frontend just needs to connect it to the
+	commit that is to be annotated.
++
+....
+	'N' SP <dataref> SP <committish> LF
+....
++
+Here `<dataref>` can be either a mark reference (`:<idnum>`)
+set by a prior `blob` command, or a full 40-byte SHA-1 of an
+existing Git blob object.
+
+Inline data format::
+	The data content for the note has not been supplied yet.
+	The frontend wants to supply it as part of this modify
+	command.
++
+....
+	'N' SP 'inline' SP <committish> LF
+	data
+....
++
+See below for a detailed description of the `data` command.
+
+In both formats `<committish>` is any of the commit specification
+expressions also accepted by `from` (see above).
+
 `mark`
 ~~~~~~
 Arranges for fast-import to save a reference to the current object, allowing
diff --git a/fast-import.c b/fast-import.c
index 8a7cdc1..a366c61 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -22,8 +22,8 @@ Format of STDIN stream:
     ('author' sp name sp '<' email '>' sp when lf)?
     'committer' sp name sp '<' email '>' sp when lf
     commit_msg
-    ('from' sp (ref_str | hexsha1 | sha1exp_str | idnum) lf)?
-    ('merge' sp (ref_str | hexsha1 | sha1exp_str | idnum) lf)*
+    ('from' sp committish lf)?
+    ('merge' sp committish lf)*
     file_change*
     lf?;
   commit_msg ::= data;
@@ -41,15 +41,18 @@ Format of STDIN stream:
   file_obm ::= 'M' sp mode sp (hexsha1 | idnum) sp path_str lf;
   file_inm ::= 'M' sp mode sp 'inline' sp path_str lf
     data;
+  note_obm ::= 'N' sp (hexsha1 | idnum) sp committish lf;
+  note_inm ::= 'N' sp 'inline' sp committish lf
+    data;
 
   new_tag ::= 'tag' sp tag_str lf
-    'from' sp (ref_str | hexsha1 | sha1exp_str | idnum) lf
+    'from' sp committish lf
     ('tagger' sp name sp '<' email '>' sp when lf)?
     tag_msg;
   tag_msg ::= data;
 
   reset_branch ::= 'reset' sp ref_str lf
-    ('from' sp (ref_str | hexsha1 | sha1exp_str | idnum) lf)?
+    ('from' sp committish lf)?
     lf?;
 
   checkpoint ::= 'checkpoint' lf
@@ -88,6 +91,7 @@ Format of STDIN stream:
      # stream formatting is: \, " and LF.  Otherwise these values
      # are UTF8.
      #
+  committish  ::= (ref_str | hexsha1 | sha1exp_str | idnum);
   ref_str     ::= ref;
   sha1exp_str ::= sha1exp;
   tag_str     ::= tag;
@@ -2007,6 +2011,80 @@ static void file_change_cr(struct branch *b, int rename)
 		leaf.tree);
 }
 
+static void note_change_n(struct branch *b)
+{
+	const char *p = command_buf.buf + 2;
+	static struct strbuf uq = STRBUF_INIT;
+	struct object_entry *oe = oe;
+	struct branch *s;
+	unsigned char sha1[20], commit_sha1[20];
+	uint16_t inline_data = 0;
+
+	/* <dataref> or 'inline' */
+	if (*p == ':') {
+		char *x;
+		oe = find_mark(strtoumax(p + 1, &x, 10));
+		hashcpy(sha1, oe->sha1);
+		p = x;
+	} else if (!prefixcmp(p, "inline")) {
+		inline_data = 1;
+		p += 6;
+	} else {
+		if (get_sha1_hex(p, sha1))
+			die("Invalid SHA1: %s", command_buf.buf);
+		oe = find_object(sha1);
+		p += 40;
+	}
+	if (*p++ != ' ')
+		die("Missing space after SHA1: %s", command_buf.buf);
+
+	/* <committish> */
+	s = lookup_branch(p);
+	if (s) {
+		hashcpy(commit_sha1, s->sha1);
+	} else if (*p == ':') {
+		uintmax_t commit_mark = strtoumax(p + 1, NULL, 10);
+		struct object_entry *commit_oe = find_mark(commit_mark);
+		if (commit_oe->type != OBJ_COMMIT)
+			die("Mark :%" PRIuMAX " not a commit", commit_mark);
+		hashcpy(commit_sha1, commit_oe->sha1);
+	} else if (!get_sha1(p, commit_sha1)) {
+		unsigned long size;
+		char *buf = read_object_with_reference(commit_sha1,
+			commit_type, &size, commit_sha1);
+		if (!buf || size < 46)
+			die("Not a valid commit: %s", p);
+		free(buf);
+	} else
+		die("Invalid ref name or SHA1 expression: %s", p);
+
+	if (inline_data) {
+		static struct strbuf buf = STRBUF_INIT;
+
+		if (p != uq.buf) {
+			strbuf_addstr(&uq, p);
+			p = uq.buf;
+		}
+		read_next_command();
+		parse_data(&buf);
+		store_object(OBJ_BLOB, &buf, &last_blob, sha1, 0);
+	} else if (oe) {
+		if (oe->type != OBJ_BLOB)
+			die("Not a blob (actually a %s): %s",
+				typename(oe->type), command_buf.buf);
+	} else {
+		enum object_type type = sha1_object_info(sha1, NULL);
+		if (type < 0)
+			die("Blob not found: %s", command_buf.buf);
+		if (type != OBJ_BLOB)
+			die("Not a blob (actually a %s): %s",
+			    typename(type), command_buf.buf);
+	}
+
+	tree_content_set(&b->branch_tree, sha1_to_hex(commit_sha1), sha1,
+		S_IFREG | 0644, NULL);
+}
+
 static void file_change_deleteall(struct branch *b)
 {
 	release_tree_content_recursive(b->branch_tree.tree);
@@ -2176,6 +2254,8 @@ static void parse_new_commit(void)
 			file_change_cr(b, 1);
 		else if (!prefixcmp(command_buf.buf, "C "))
 			file_change_cr(b, 0);
+		else if (!prefixcmp(command_buf.buf, "N "))
+			note_change_n(b);
 		else if (!strcmp("deleteall", command_buf.buf))
 			file_change_deleteall(b);
 		else {
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 821be7c..b49815d 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1088,4 +1088,170 @@ INPUT_END
 test_expect_success 'P: fail on blob mark in gitlink' '
     test_must_fail git fast-import <input'
 
+###
+### series Q (notes)
+###
+
+note1_data="Note for the first commit"
+note2_data="Note for the second commit"
+note3_data="Note for the third commit"
+
+test_tick
+cat >input <<INPUT_END
+blob
+mark :2
+data <<EOF
+$file2_data
+EOF
+
+commit refs/heads/notes-test
+mark :3
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+first (:3)
+COMMIT
+
+M 644 :2 file2
+
+blob
+mark :4
+data $file4_len
+$file4_data
+commit refs/heads/notes-test
+mark :5
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+second (:5)
+COMMIT
+
+M 644 :4 file4
+
+commit refs/heads/notes-test
+mark :6
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+third (:6)
+COMMIT
+
+M 644 inline file5
+data <<EOF
+$file5_data
+EOF
+
+M 755 inline file6
+data <<EOF
+$file6_data
+EOF
+
+blob
+mark :7
+data <<EOF
+$note1_data
+EOF
+
+blob
+mark :8
+data <<EOF
+$note2_data
+EOF
+
+commit refs/notes/foobar
+mark :9
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+notes (:9)
+COMMIT
+
+N :7 :3
+N :8 :5
+N inline :6
+data <<EOF
+$note3_data
+EOF
+
+INPUT_END
+test_expect_success \
+	'Q: commit notes' \
+	'git fast-import <input &&
+	 git whatchanged notes-test'
+test_expect_success \
+	'Q: verify pack' \
+	'for p in .git/objects/pack/*.pack;do git verify-pack $p||exit;done'
+
+commit1=$(git rev-parse notes-test~2)
+commit2=$(git rev-parse notes-test^)
+commit3=$(git rev-parse notes-test)
+
+cat >expect <<EOF
+author $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+
+first (:3)
+EOF
+test_expect_success \
+	'Q: verify first commit' \
+	'git cat-file commit notes-test~2 | sed 1d >actual &&
+	test_cmp expect actual'
+
+cat >expect <<EOF
+parent $commit1
+author $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+
+second (:5)
+EOF
+test_expect_success \
+	'Q: verify second commit' \
+	'git cat-file commit notes-test^ | sed 1d >actual &&
+	test_cmp expect actual'
+
+cat >expect <<EOF
+parent $commit2
+author $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+
+third (:6)
+EOF
+test_expect_success \
+	'Q: verify third commit' \
+	'git cat-file commit notes-test | sed 1d >actual &&
+	test_cmp expect actual'
+
+cat >expect <<EOF
+author $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+
+notes (:9)
+EOF
+test_expect_success \
+	'Q: verify notes commit' \
+	'git cat-file commit refs/notes/foobar | sed 1d >actual &&
+	test_cmp expect actual'
+
+cat >expect.unsorted <<EOF
+100644 blob $commit1
+100644 blob $commit2
+100644 blob $commit3
+EOF
+cat expect.unsorted | sort >expect
+test_expect_success \
+	'Q: verify notes tree' \
+	'git cat-file -p refs/notes/foobar^{tree} | sed "s/ [0-9a-f]*	/ /" >actual &&
+	 test_cmp expect actual'
+
+echo "$note1_data" >expect
+test_expect_success \
+	'Q: verify note for first commit' \
+	'git cat-file blob refs/notes/foobar:$commit1 >actual && test_cmp expect actual'
+
+echo "$note2_data" >expect
+test_expect_success \
+	'Q: verify note for second commit' \
+	'git cat-file blob refs/notes/foobar:$commit2 >actual && test_cmp expect actual'
+
+echo "$note3_data" >expect
+test_expect_success \
+	'Q: verify note for third commit' \
+	'git cat-file blob refs/notes/foobar:$commit3 >actual && test_cmp expect actual'
+
 test_done
-- 
1.6.4.rc3.138.ga6b98.dirty

Re: [RFC 06/11 v2] fast-import: Add support for importing commit notes

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> +External data format::
> +	The data content for the note was already supplied by a prior
> +	`blob` command.  The frontend just needs to connect it to the
> +	commit that is to be annotated.
> ++
> +....
> +	'N' SP <dataref> SP <committish> LF
> +....
> ++
> +Here `<dataref>` can be either a mark reference (`:<idnum>`)
> +set by a prior `blob` command, or a full 40-byte SHA-1 of an
> +existing Git blob object.
> +
> +Inline data format::
> +	The data content for the note has not been supplied yet.
> +	The frontend wants to supply it as part of this modify
> +	command.
> ++
> +....
> +	'N' SP 'inline' SP <committish> LF
> +	data
> +....
> ++
> +See below for a detailed description of the `data` command.
> +
> +In both formats `<committish>` is any of the commit specification
> +expressions also accepted by `from` (see above).

Doesn't this make fast-import language incapable of add notes to anything
other than commits?  As far as I remember, there is no such limitation in
the underlying data structure on git notes, even though the git-notes
sample Porcelain might have such a restriction.

We recently hit a similar unintended limitation that we regret in the
fast-import language, didn't we?

Although personally I do not think it is a big deal if we cannot tag or
add notes to trees, I am pointing it out in case other people care.

Re: [RFC 06/11 v2] fast-import: Add support for importing commit notes

[Johan Herland]

On Wednesday 29 July 2009, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > +External data format::
> > +	The data content for the note was already supplied by a prior
> > +	`blob` command.  The frontend just needs to connect it to the
> > +	commit that is to be annotated.
> > ++
> > +....
> > +	'N' SP <dataref> SP <committish> LF
> > +....
> > ++
> > +Here `<dataref>` can be either a mark reference (`:<idnum>`)
> > +set by a prior `blob` command, or a full 40-byte SHA-1 of an
> > +existing Git blob object.
> > +
> > +Inline data format::
> > +	The data content for the note has not been supplied yet.
> > +	The frontend wants to supply it as part of this modify
> > +	command.
> > ++
> > +....
> > +	'N' SP 'inline' SP <committish> LF
> > +	data
> > +....
> > ++
> > +See below for a detailed description of the `data` command.
> > +
> > +In both formats `<committish>` is any of the commit specification
> > +expressions also accepted by `from` (see above).
>
> Doesn't this make fast-import language incapable of add notes to anything
> other than commits?  As far as I remember, there is no such limitation in
> the underlying data structure on git notes, even though the git-notes
> sample Porcelain might have such a restriction.

It does (probably because the default notes tree is "refs/notes/commits").

> We recently hit a similar unintended limitation that we regret in the
> fast-import language, didn't we?

I don't know. Must have slipped past my mailbox.

> Although personally I do not think it is a big deal if we cannot tag or
> add notes to trees, I am pointing it out in case other people care.

I copied the semantics from the 'tag' command, for no particular reason 
(except following the git-notes procelain). Expanding 'notemodify' (and 
'tag') to cover all types of objects is fine by me, unless there are good 
arguments otherwise. Shawn?


Have fun!

...Johan

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

Re: [RFC 03/11] Allow programs to not depend on remotes having urls

[Alex Riesen]

On Mon, Jul 27, 2009 at 03:04, Johan Herland<johan@herland.net> wrote:
> diff --git a/builtin-fetch.c b/builtin-fetch.c
> index 817dd6b..3f32db6 100644
> --- a/builtin-fetch.c
> +++ b/builtin-fetch.c
> @@ -346,12 +346,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
> +               if (url) {
> +                       url_len = strlen(url);
> +                       for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
> +                               ;
> +                       url_len = i + 1;
> +                       if (4 < i && !strncmp(".git", url + i - 3, 4))
> +                               url_len = i - 3;
> +               } else {
> +                       url = "foriegn";

Typo. Should be "foreign".

Re: [RFC 06/11 v2] fast-import: Add support for importing commit notes

[Shawn O. Pearce]

Johan Herland <johan@herland.net> wrote:
> On Wednesday 29 July 2009, Junio C Hamano wrote:
> > Johan Herland <johan@herland.net> writes:
> > > +
> > > +In both formats `<committish>` is any of the commit specification
> > > +expressions also accepted by `from` (see above).
> >
> > Doesn't this make fast-import language incapable of add notes to anything
> > other than commits?  As far as I remember, there is no such limitation in
> > the underlying data structure on git notes, even though the git-notes
> > sample Porcelain might have such a restriction.
> 
> It does (probably because the default notes tree is "refs/notes/commits").

Yea, it does have that limitation right now.  That limitation could
be relaxed in the code by just allowing the <committish> to be any
object and simply don't check its type.

But I've already stated with regards to the notes that I think we
should only allow noting commits and annotated tags, where we have
a timestamp we can use to split the notes in the note tree by time,
so that we can index recent notes much more quickly and can answer
`git log -20` much more efficiently.

I just don't see a lot of value in noting a blob or a tree, there
is too little context information on such things for it to really
be all that useful.

> > We recently hit a similar unintended limitation that we regret in the
> > fast-import language, didn't we?
> 
> I don't know. Must have slipped past my mailbox.

I remember something being raised, but I can't remember exactly
what it was either.

It might have had to do with the effects of rename commands, e.g. a
file rename takes place immediately when issued, and some frontends
wanted it to take place only after the commit was completed.
 
> > Although personally I do not think it is a big deal if we cannot tag or
> > add notes to trees, I am pointing it out in case other people care.
> 
> I copied the semantics from the 'tag' command, for no particular reason 
> (except following the git-notes procelain). Expanding 'notemodify' (and 
> 'tag') to cover all types of objects is fine by me, unless there are good 
> arguments otherwise. Shawn?

tag, there might be arguments for tagging trees, e.g. so you can
export the linux kernel repository with `git fast-export` and reload
it with fast-import.  But that's unrelated to this change.

See above about notes.

-- 
Shawn.

Re: [RFC 06/11 v2] fast-import: Add support for importing commit notes

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

>> > We recently hit a similar unintended limitation that we regret in the
>> > fast-import language, didn't we?
>> 
>> I don't know. Must have slipped past my mailbox.
>
> I remember something being raised, but I can't remember exactly
> what it was either.

Exporting a tag that points at a non commit was what I had in mind.

> tag, there might be arguments for tagging trees, e.g. so you can
> export the linux kernel repository with `git fast-export` and reload
> it with fast-import.  But that's unrelated to this change.
>
> See above about notes.

I think we are in agreement then.  Good.

Re: [RFC 06/11 v2] fast-import: Add support for importing commit notes

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Introduce a 'notemodify' subcommand of the 'commit' command. This subcommand
> is similar to 'filemodify', except that no mode is supplied (all notes have
> mode 0644), and the path is set to the hex SHA1 of the given "comittish".
>
> This enables fast import of note objects along with their associated commits,
> since the notes can now be named using the mark references of their
> corresponding commits.
>
> The patch also includes a test case of the added functionality.
>
> Signed-off-by: Johan Herland <johan@herland.net>

Thanks.

Replacing this will cascade to the rest of the series, right?

Re: [RFC 06/11 v2] fast-import: Add support for importing commit notes

[Johan Herland]

On Wednesday 29 July 2009, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > Introduce a 'notemodify' subcommand of the 'commit' command. This
> > subcommand is similar to 'filemodify', except that no mode is supplied
> > (all notes have mode 0644), and the path is set to the hex SHA1 of the
> > given "comittish".
> >
> > This enables fast import of note objects along with their associated
> > commits, since the notes can now be named using the mark references of
> > their corresponding commits.
> >
> > The patch also includes a test case of the added functionality.
> >
> > Signed-off-by: Johan Herland <johan@herland.net>
>
> Thanks.
>
> Replacing this will cascade to the rest of the series, right?

Not sure what you mean by "cascade to the rest of the series". If you just 
replace 06/11_v1 with 06/11_v2, things will stop working (since 10/11 and 
11/11 needs a corresponding update of their fast-import commands).

On the other hand, I have moved this patch into the jh/notes series (which I 
updated/resent last night).

I will update/resend this patch series tomorrow, with suggested fixes, and 
rebased onto the updated jh/notes series.


...Johan

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

Re: [RFC 03/11] Allow programs to not depend on remotes having urls

[Johan Herland]

On Wednesday 29 July 2009, Alex Riesen wrote:
> On Mon, Jul 27, 2009 at 03:04, Johan Herland<johan@herland.net> wrote:
> > diff --git a/builtin-fetch.c b/builtin-fetch.c
> > index 817dd6b..3f32db6 100644
> > --- a/builtin-fetch.c
> > +++ b/builtin-fetch.c
> > @@ -346,12 +346,17 @@ static int store_updated_refs(const char
> > *raw_url, const char *remote_name, +               if (url) {
> > +                       url_len = strlen(url);
> > +                       for (i = url_len - 1; url[i] == '/' && 0 <= i;
> > i--) +                               ;
> > +                       url_len = i + 1;
> > +                       if (4 < i && !strncmp(".git", url + i - 3, 4))
> > +                               url_len = i - 3;
> > +               } else {
> > +                       url = "foriegn";
>
> Typo. Should be "foreign".

Thanks. Will be fixed in the next iteration of this topic.

...Johan

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

Re: [RFC 06/11 v2] fast-import: Add support for importing commit notes

[Johan Herland]

On Wednesday 29 July 2009, Junio C Hamano wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> >> > We recently hit a similar unintended limitation that we regret in
> >> > the fast-import language, didn't we?
> >>
> >> I don't know. Must have slipped past my mailbox.
> >
> > I remember something being raised, but I can't remember exactly
> > what it was either.
>
> Exporting a tag that points at a non commit was what I had in mind.
>
> > tag, there might be arguments for tagging trees, e.g. so you can
> > export the linux kernel repository with `git fast-export` and reload
> > it with fast-import.  But that's unrelated to this change.
> >
> > See above about notes.
>
> I think we are in agreement then.  Good.

We do? on what? That 'notemodify' should work on <committish> + <tag 
object>? Or that 'notemodify' should work on <any object>?

...Johan

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

Re: [RFC 06/11 v2] fast-import: Add support for importing commit notes

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Wednesday 29 July 2009, Junio C Hamano wrote:
>> "Shawn O. Pearce" <spearce@spearce.org> writes:
> ...
>> > See above about notes.
>>
>> I think we are in agreement then.  Good.
>
> We do? on what?

We might mind if fast-import does not handle tags to non-commits, but we
would want to restrict notes to apply only to commits for other reasons
anyway, and it is perfectly fine if your patch handled only notes that are
attached to commits and not arbitrary objects.