threads / patch / 30993

patch, 4 partsvcs-svn: add fast_export_note to create notes

Subject: [PATCH 1/4] vcs-svn: add fast_export_note to create notes

## tl;dr

20 messages between Jul 11, 2012 and Jul 17, 2012. Diffs are folded; open one to read it.

replies: 19people: 5as markdown or json

Florian Achleitner· Jul 11, 2012, 13:38 UTC · lore

[RFC/PATCH 0/4] GSOC remote-svn

Hi!

This series adds creating notes to vcs-svn, plus some testing aids. I picked one patch from Dmitry's existing work.

Next steps are storing the fetched revisions and notes in the right place in refs/remote/ and adding incremental import using the notes.

Im currently stuck on some unexpected behaviour when fetching via remote-svn. The refs/remote/svn/master points to the same commit as refs/heads/master and not to the fetched one. It gets overwritten after fast-import terminates. I'm digging through the internals of fetching..

Florian:

[PATCH 1/4] vcs-svn: add fast_export_note to create notes [PATCH 2/4] Allow reading svn dumps from files via file:// urls. [PATCH 3/4] Create a note for every imported commit containing svn [PATCH 4/4] When debug==1, start fast-import with "--stats" instead

Florian Achleitner· Jul 11, 2012, 13:38 UTC · re: Florian Achleitner · lore
From: Dmitry Ivankov <divanorama@gmail.com>

fast_export lacked a method to writes notes to fast-import stream. Add two new functions fast_export_note which is similar to fast_export_modify. And also add fast_export_buf_to_data to be able to write inline blobs that don't come from a line_buffer or from delta application.

To be used like this: fast_export_begin_commit("refs/notes/somenotes", ...)

fast_export_note("refs/heads/master", "inline") fast_export_buf_to_data(&data) or maybe fast_export_note("refs/heads/master", sha1)

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 vcs-svn/fast_export.c |   13 +++++++++++++
 vcs-svn/fast_export.h |    2 ++
 2 files changed, 15 insertions(+)
Show changes to 2 files +15 −0

vcs-svn/fast_export.c, vcs-svn/fast_export.h

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index b823b85..ec7a1c1 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -73,11 +73,17 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref)
 	putchar('\n');
 }
 
+void fast_export_note(const char *committish, const char *dataref)
+{
+	printf("N %s %s\n", dataref, committish);
+}
+
 static char gitsvnline[MAX_GITSVN_LINE_LEN];
 void fast_export_begin_commit(uint32_t revision, const char *author,
 			const struct strbuf *log,
 			const char *uuid, const char *url,
 			unsigned long timestamp)
+
 {
 	static const struct strbuf empty = STRBUF_INIT;
 	if (!log)
@@ -227,6 +233,13 @@ static long apply_delta(off_t len, struct line_buffer *input,
 	return ret;
 }
 
+void fast_export_buf_to_data(const struct strbuf *data)
+{
+	printf("data %"PRIuMAX"\n", (uintmax_t)data->len);
+	fwrite(data->buf, data->len, 1, stdout);
+	fputc('\n', stdout);
+}
+
 void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input)
 {
 	assert(len >= 0);
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index aa629f5..62bac44 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -10,11 +10,13 @@ void fast_export_reset(void);
 
 void fast_export_delete(const char *path);
 void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
+void fast_export_note(const char *committish, const char *dataref);
 void fast_export_begin_commit(uint32_t revision, const char *author,
 			const struct strbuf *log, const char *uuid,
 			const char *url, unsigned long timestamp);
 void fast_export_end_commit(uint32_t revision);
 void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input);
+void fast_export_buf_to_data(const struct strbuf *data);
 void fast_export_blob_delta(uint32_t mode,
 			uint32_t old_mode, const char *old_data,
 			off_t len, struct line_buffer *input);
-- 
1.7.9.5
Florian Achleitner· Jul 11, 2012, 13:38 UTC · re: Florian Achleitner · lore

[PATCH 2/4] Allow reading svn dumps from files via file:// urls.

Especially for testing and development it's useful to bypass svnrdump and replay the svndump from a file without connecting to an svn server.

Add support for file:// urls in the remote url. e.g. svn::file:///path/to/dump When the remote helper finds an url starting with file:// it tries to open that file instead of invoking svnrdump.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 contrib/svn-fe/remote-svn.c |   53 +++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 17 deletions(-)
Show changes to contrib/svn-fe/remote-svn.c +36 −17
diff --git a/contrib/svn-fe/remote-svn.c b/contrib/svn-fe/remote-svn.c
index 5ec7fbb..a248166 100644
--- a/contrib/svn-fe/remote-svn.c
+++ b/contrib/svn-fe/remote-svn.c
@@ -26,6 +26,7 @@ static inline void printd(const char* fmt, ...)
 
 static struct remote* remote;
 static const char* url;
+static int dump_from_file;
 const char* private_refs = "refs/remote-svn/";		/* + remote->name. */
 const char* remote_ref = "refs/heads/master";
 
@@ -56,6 +57,7 @@ enum cmd_result cmd_import(struct strbuf* line)
 	const char* revs = "-r0:HEAD";
 	int code, report_fd;
 	char* back_pipe_env;
+	int dumpin_fd;
 	struct child_process svndump_proc = {
 			.argv = NULL,		/* comes later .. */
 			/* we want a pipe to the child's stdout, but stdin, stderr inherited.
@@ -90,27 +92,35 @@ enum cmd_result cmd_import(struct strbuf* line)
 
 	printd("Opened fast-import back-pipe %s for reading.", back_pipe_env);
 
-	svndump_proc.argv = xcalloc(5, sizeof(char*));
-	svndump_proc.argv[0] = "svnrdump";
-	svndump_proc.argv[1] = "dump";
-	svndump_proc.argv[2] = url;
-	svndump_proc.argv[3] = revs;
-
-	code = start_command(&svndump_proc);
-	if(code)
-		die("Unable to start %s, code %d", svndump_proc.argv[0], code);
-
+	if(dump_from_file) {
+		dumpin_fd = open(url, O_RDONLY);
+		if(dumpin_fd < 0) {
+			die_errno("Couldn't open svn dump file %s.", url);
+		}
+	}
+	else {
+		svndump_proc.argv = xcalloc(5, sizeof(char*));
+		svndump_proc.argv[0] = "svnrdump";
+		svndump_proc.argv[1] = "dump";
+		svndump_proc.argv[2] = url;
+		svndump_proc.argv[3] = revs;
+
+		code = start_command(&svndump_proc);
+		if(code)
+			die("Unable to start %s, code %d", svndump_proc.argv[0], code);
+		dumpin_fd = svndump_proc.out;
+	}
 
 
-	svndump_init_fd(svndump_proc.out, report_fd);
+	svndump_init_fd(dumpin_fd, report_fd);
 	svndump_read(url);
 	svndump_deinit();
 	svndump_reset();
 
-	close(svndump_proc.out);
+	close(dumpin_fd);
 	close(report_fd);
-
-	code = finish_command(&svndump_proc);
+	if(!dump_from_file)
+		code = finish_command(&svndump_proc);
 	if(code)
 		warning("Something went wrong with termination of %s, code %d", svndump_proc.argv[0], code);
 	free(svndump_proc.argv);
@@ -166,14 +176,23 @@ int main(int argc, const char **argv)
 
 	remote = remote_get(argv[1]);
 	if (argc == 3) {
-		end_url_with_slash(&buf, argv[2]);
+		url = argv[2];
 	} else if (argc == 2) {
-		end_url_with_slash(&buf, remote->url[0]);
+		url = remote->url[0];
 	} else {
 		warning("Excess arguments!");
 	}
 
-	url = strbuf_detach(&buf, NULL);
+	if (!prefixcmp(url, "file://")) {
+		dump_from_file = 1;
+		url = url_decode(url + sizeof("file://")-1);
+		printd("remote-svn uses a file as dump input.");
+	}
+	else {
+		dump_from_file = 0;
+		end_url_with_slash(&buf, url);
+		url = strbuf_detach(&buf, NULL);
+	}
 
 	printd("remote-svn starting with url %s", url);
 
-- 
1.7.9.5
Florian Achleitner· Jul 11, 2012, 13:38 UTC · re: Florian Achleitner · lore

[PATCH 3/4] Create a note for every imported commit containing svn metadata.

To provide metadata from svn dumps for further processing, e.g.
branch detection, attach a note to each imported commit that
stores additional information.
The notes are currently hard-coded in refs/notes/svn/revs.
Currently the following lines from the svn dump are directly
accumulated in the note. This can be refined on purpose, of course.
- "Revision-number"
- "Node-path"
- "Node-kind"
- "Node-action"
- "Node-copyfrom-path"
- "Node-copyfrom-rev"
Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 vcs-svn/fast_export.c |   13 +++++++++++++
 vcs-svn/fast_export.h |    2 ++
 vcs-svn/svndump.c     |   21 +++++++++++++++++++--
 3 files changed, 34 insertions(+), 2 deletions(-)
Show changes to 3 files +34 −2

vcs-svn/fast_export.c, vcs-svn/fast_export.h, vcs-svn/svndump.c

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index ec7a1c1..7774ab0 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -12,6 +12,7 @@
 #include "svndiff.h"
 #include "sliding_window.h"
 #include "line_buffer.h"
+#include "cache.h"
 
 #define MAX_GITSVN_LINE_LEN 4096
 
@@ -73,6 +74,18 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref)
 	putchar('\n');
 }
 
+void fast_export_begin_note(uint32_t revision, const char *author,
+		const char *log, unsigned long timestamp)
+{
+	timestamp = 1341914616;
+	size_t loglen = strlen(log);
+	printf("commit refs/notes/svn/revs\n");
+	printf("committer %s <%s@%s> %ld +0000\n", author, author, "local", timestamp);
+	printf("data %"PRIuMAX"\n", loglen);
+	fwrite(log, loglen, 1, stdout);
+	fputc('\n', stdout);
+}
+
 void fast_export_note(const char *committish, const char *dataref)
 {
 	printf("N %s %s\n", dataref, committish);
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 62bac44..febab6e 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -11,6 +11,8 @@ void fast_export_reset(void);
 void fast_export_delete(const char *path);
 void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
 void fast_export_note(const char *committish, const char *dataref);
+void fast_export_begin_note(uint32_t revision, const char *author,
+		const char *log, unsigned long timestamp);
 void fast_export_begin_commit(uint32_t revision, const char *author,
 			const struct strbuf *log, const char *uuid,
 			const char *url, unsigned long timestamp);
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index eb76bf8..61f1449 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -49,7 +49,7 @@ static struct {
 static struct {
 	uint32_t revision;
 	unsigned long timestamp;
-	struct strbuf log, author;
+	struct strbuf log, author, note;
 } rev_ctx;
 
 static struct {
@@ -78,6 +78,7 @@ static void reset_rev_ctx(uint32_t revision)
 	rev_ctx.timestamp = 0;
 	strbuf_reset(&rev_ctx.log);
 	strbuf_reset(&rev_ctx.author);
+	strbuf_reset(&rev_ctx.note);
 }
 
 static void reset_dump_ctx(const char *url)
@@ -311,8 +312,15 @@ static void begin_revision(void)
 
 static void end_revision(void)
 {
-	if (rev_ctx.revision)
+	struct strbuf mark = STRBUF_INIT;
+	if (rev_ctx.revision) {
 		fast_export_end_commit(rev_ctx.revision);
+		fast_export_begin_note(rev_ctx.revision, "remote-svn",
+				"Note created by remote-svn.", rev_ctx.timestamp);
+		strbuf_addf(&mark, ":%"PRIu32, rev_ctx.revision);
+		fast_export_note(mark.buf, "inline");
+		fast_export_buf_to_data(&rev_ctx.note);
+	}
 }
 
 void svndump_read(const char *url)
@@ -359,6 +367,7 @@ void svndump_read(const char *url)
 				end_revision();
 			active_ctx = REV_CTX;
 			reset_rev_ctx(atoi(val));
+			strbuf_addf(&rev_ctx.note, "%s\n", t);
 			break;
 		case sizeof("Node-path"):
 			if (prefixcmp(t, "Node-"))
@@ -370,10 +379,12 @@ void svndump_read(const char *url)
 					begin_revision();
 				active_ctx = NODE_CTX;
 				reset_node_ctx(val);
+				strbuf_addf(&rev_ctx.note, "%s\n", t);
 				break;
 			}
 			if (constcmp(t + strlen("Node-"), "kind"))
 				continue;
+			strbuf_addf(&rev_ctx.note, "%s\n", t);
 			if (!strcmp(val, "dir"))
 				node_ctx.type = REPO_MODE_DIR;
 			else if (!strcmp(val, "file"))
@@ -384,6 +395,7 @@ void svndump_read(const char *url)
 		case sizeof("Node-action"):
 			if (constcmp(t, "Node-action"))
 				continue;
+			strbuf_addf(&rev_ctx.note, "%s\n", t);
 			if (!strcmp(val, "delete")) {
 				node_ctx.action = NODEACT_DELETE;
 			} else if (!strcmp(val, "add")) {
@@ -402,11 +414,13 @@ void svndump_read(const char *url)
 				continue;
 			strbuf_reset(&node_ctx.src);
 			strbuf_addstr(&node_ctx.src, val);
+			strbuf_addf(&rev_ctx.note, "%s\n", t);
 			break;
 		case sizeof("Node-copyfrom-rev"):
 			if (constcmp(t, "Node-copyfrom-rev"))
 				continue;
 			node_ctx.srcRev = atoi(val);
+			strbuf_addf(&rev_ctx.note, "%s\n", t);
 			break;
 		case sizeof("Text-content-length"):
 			if (!constcmp(t, "Text-content-length")) {
@@ -472,6 +486,7 @@ static void init(int report_fd)
 	strbuf_init(&dump_ctx.url, 4096);
 	strbuf_init(&rev_ctx.log, 4096);
 	strbuf_init(&rev_ctx.author, 4096);
+	strbuf_init(&rev_ctx.note, 4096);
 	strbuf_init(&node_ctx.src, 4096);
 	strbuf_init(&node_ctx.dst, 4096);
 	reset_dump_ctx(NULL);
@@ -503,6 +518,8 @@ void svndump_deinit(void)
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	strbuf_release(&rev_ctx.log);
+	strbuf_release(&rev_ctx.author);
+	strbuf_release(&rev_ctx.note);
 	strbuf_release(&node_ctx.src);
 	strbuf_release(&node_ctx.dst);
 	if (buffer_deinit(&input))
-- 
1.7.9.5
Florian Achleitner· Jul 11, 2012, 13:38 UTC · re: Florian Achleitner · lore

[PATCH 4/4] When debug==1, start fast-import with "--stats" instead of "--quiet".

fast-import prints statistics that could be interesting to the developer of remote helpers.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 transport-helper.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to transport-helper.c +1 −1
diff --git a/transport-helper.c b/transport-helper.c
index 616db91..d6daad5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -392,7 +392,7 @@ static int get_importer(struct transport *transport, struct child_process *fasti
 	memset(fastimport, 0, sizeof(*fastimport));
 	fastimport->in = helper->out;
 	argv_array_push(argv, "fast-import");
-	argv_array_push(argv, "--quiet");
+	argv_array_push(argv, debug ? "--stats" : "--quiet");
 	argv_array_pushf(argv, "--cat-blob-pipe=%s", data->report_fifo);
 	fastimport->argv = argv->argv;
 	fastimport->git_cmd = 1;
-- 
1.7.9.5
Dmitry Ivankov· Jul 11, 2012, 14:29 UTC · re: Florian Achleitner · lore

Re: [PATCH 2/4] Allow reading svn dumps from files via file:// urls.

Florian Achleitner <florian.achleitner.2.6.31 <at> gmail.com> writes:
Show 9 quoted lines
> 
> Especially for testing and development it's useful
> to bypass svnrdump and replay the svndump from a file
> without connecting to an svn server.
> 
> Add support for file:// urls in the remote url.
> e.g. svn::file:///path/to/dump
> When the remote helper finds an url starting with
> file:// it tries to open that file instead of invoking svnrdump.

file:// is a bad choice because file:// style repo urls are valid for svn and it's for local repos rather than dumpfiles. Maybe something like dumpfile:// instead?

[patch skipped]
Junio C Hamano· Jul 11, 2012, 17:00 UTC · re: Dmitry Ivankov · lore

Re: [PATCH 2/4] Allow reading svn dumps from files via file:// urls.

Dmitry Ivankov <divanorama@gmail.com> writes:
Show 14 quoted lines
> Florian Achleitner <florian.achleitner.2.6.31 <at> gmail.com> writes:
>
>> 
>> Especially for testing and development it's useful
>> to bypass svnrdump and replay the svndump from a file
>> without connecting to an svn server.
>> 
>> Add support for file:// urls in the remote url.
>> e.g. svn::file:///path/to/dump
>> When the remote helper finds an url starting with
>> file:// it tries to open that file instead of invoking svnrdump.
>
> file:// is a bad choice because file:// style repo urls are valid for svn and 
> it's for local repos rather than dumpfiles.
Thanks; I had the same reaction when I saw it.
> Maybe something like dumpfile:// instead?

If dumpfile:// pseudo URL is an established convention in the Subversion land, that sounds like a sensible direction, but if that is not the case, it may be cleaner if you can find some other way to convey the information to the backend out-of-band, instead of overloading it in the URL used to access the repository.

Stephen Bash· Jul 11, 2012, 17:49 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/4] Allow reading svn dumps from files via file:// urls.

----- Original Message -----
Show 32 quoted lines
> From: "Junio C Hamano" <gitster@pobox.com>
> To: "Dmitry Ivankov" <divanorama@gmail.com>
> Cc: git@vger.kernel.org
> Sent: Wednesday, July 11, 2012 1:00:29 PM
> Subject: Re: [PATCH 2/4] Allow reading svn dumps from files via file:// urls.
> 
> Dmitry Ivankov <divanorama@gmail.com> writes:
> 
> > Florian Achleitner <florian.achleitner.2.6.31 <at> gmail.com>
> > writes:
> >
> > > Especially for testing and development it's useful to bypass
> > > svnrdump and replay the svndump from a file without connecting to
> > > an svn server.
> > > 
> > > Add support for file:// urls in the remote url.  e.g.
> > > svn::file:///path/to/dump When the remote helper finds an url
> > > starting with file:// it tries to open that file instead of
> > > invoking svnrdump.
> >
> > file:// is a bad choice because file:// style repo urls are valid
> > for svn and it's for local repos rather than dumpfiles.
> 
> Thanks; I had the same reaction when I saw it.
> 
> > Maybe something like dumpfile:// instead?
> 
> If dumpfile:// pseudo URL is an established convention in the
> Subversion land, that sounds like a sensible direction, but if that is
> not the case, it may be cleaner if you can find some other way to
> convey the information to the backend out-of-band, instead of
> overloading it in the URL used to access the repository.
Others may have a different opinion, but in my experience dump files are always handled via stdin/stdout in Subversion land.  For example:

http://svnbook.red-bean.com/en/1.7/svn.ref.svnadmin.c.dump.html http://svnbook.red-bean.com/en/1.7/svn.ref.svnadmin.c.load.html http://svnbook.red-bean.com/en/1.7/svn.reposadmin.maint.html#svn.reposadmin.maint.filtering

I'm not sure that helps in this scenario, but that was the convention I grew used to.
Stephen
Florian Achleitner· Jul 15, 2012, 14:26 UTC · re: Florian Achleitner · lore

[PATCH] Fix overwritten remote ref on with fast-import.

After importing new commits on top of refs/remotes/* the ref was overwritten with the local refs/heads/master, because the name of the remote reference to fetch, i.e. refs/heads/master, was used to retrieve old_sha1 for it's local counterpart. Therefore, old_sha1 pointed to the local head which was not contained in the remote branch and couldn't be updated (printing a warning ..).

There are some points that are still not completely clear to me:
- I found, that the remote ref I need is stored in ref->peer_ref. There
  is one little comment on peer_ref saying /* when renaming */. That doesn't say much
  to me. Is peer_ref the correct solution?
- fast-import's commit command does already add a commit to a branch. The
  remote ref was already correct, but got overwritten by store_updated_refs
  after fast import terminated. (I figured that out using strace).
  So the update is somewhat redundant. But probably only in this special case.(?)
Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 transport-helper.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to transport-helper.c +3 −1
diff --git a/transport-helper.c b/transport-helper.c
index d6daad5..a0f05ce 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -485,8 +485,10 @@ static int fetch_with_import(struct transport *transport,
 			continue;
 		if (data->refspecs)
 			private = apply_refspecs(data->refspecs, data->refspec_nr, posn->name);
+		else if (posn->peer_ref)
+			private = xstrdup(posn->peer_ref->name);
 		else
-			private = xstrdup(posn->name);
+			private = NULL;
 		if (private) {
 			read_ref(private, posn->old_sha1);
 			free(private);
-- 
1.7.9.5
Jonathan Nieder· Jul 16, 2012, 00:30 UTC · re: Florian Achleitner · lore

Re: [PATCH] Fix overwritten remote ref on with fast-import.

Hi Florian,
Florian Achleitner wrote:
Show 6 quoted lines
> After importing new commits on top of refs/remotes/* the
> ref was overwritten with the local refs/heads/master, because the name
> of the remote reference to fetch, i.e. refs/heads/master, was used to
> retrieve old_sha1 for it's local counterpart. Therefore, old_sha1 pointed
> to the local head which was not contained in the remote branch and couldn't
> be updated (printing a warning ..).

I assume you are talking about the status quo here. It's easy to forget that others have not already applied your patch, but using the present tense would make reading easier. Think of the patch description as a special kind of bug report.

Unfortunately, as a bug report, the above is lacking some detail. Do I understand correctly that some remote helper is failing when git invokes its 'import' command? What are the symptoms? If it prints a warning, what is the exact warning?

Does that remote helper advertise the 'refspec' capability? If so, what refspec does it use? If not, why not?

It might seem silly to ask for these things when you're providing a fix along with the report! However, if someone else runs into the same symptoms, they need to be able to find your patch quickly; if your patch has a bad side-effect then we need to know why not to revert it; and if someone new starts working on the same area of code, they need to know what bugs to avoid reintroducing.

Curious, Jonathan

Junio C Hamano· Jul 16, 2012, 04:33 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Fix overwritten remote ref on with fast-import.

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 26 quoted lines
> Hi Florian,
>
> Florian Achleitner wrote:
>
>> After importing new commits on top of refs/remotes/* the
>> ref was overwritten with the local refs/heads/master, because the name
>> of the remote reference to fetch, i.e. refs/heads/master, was used to
>> retrieve old_sha1 for it's local counterpart. Therefore, old_sha1 pointed
>> to the local head which was not contained in the remote branch and couldn't
>> be updated (printing a warning ..).
>
> I assume you are talking about the status quo here.  It's easy to
> forget that others have not already applied your patch, but using the
> present tense would make reading easier.  Think of the patch
> description as a special kind of bug report.
>
> Unfortunately, as a bug report, the above is lacking some detail.  Do
> I understand correctly that some remote helper is failing when git
> invokes its 'import' command?  What are the symptoms?  If it prints a
> warning, what is the exact warning?
>
> Does that remote helper advertise the 'refspec' capability?  If so,
> what refspec does it use?  If not, why not?
>
> It might seem silly to ask for these things when you're providing a
> fix along with the report!

I share the puzzlement you expressed above, and it is unclear if this is a fix or covering a user error with butchering a code that is working as intended, making two wrongs happen to collide into producing a right.

At least it needs a better description of the problem it tries to fix, preferrably in the form of a new test for the transport helper.

Show 5 quoted lines
> However, if someone else runs into the
> same symptoms, they need to be able to find your patch quickly; if
> your patch has a bad side-effect then we need to know why not to
> revert it; and if someone new starts working on the same area of code,
> they need to know what bugs to avoid reintroducing.
That, too.
Thanks.
Florian Achleitner· Jul 16, 2012, 22:33 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Fix overwritten remote ref on with fast-import.

On Sunday 15 July 2012 19:30:25 Jonathan Nieder wrote:
Show 20 quoted lines
> Hi Florian,
> 
> Florian Achleitner wrote:
> > After importing new commits on top of refs/remotes/* the
> > ref was overwritten with the local refs/heads/master, because the name
> > of the remote reference to fetch, i.e. refs/heads/master, was used to
> > retrieve old_sha1 for it's local counterpart. Therefore, old_sha1 pointed
> > to the local head which was not contained in the remote branch and
> > couldn't
> > be updated (printing a warning ..).
> 
> I assume you are talking about the status quo here.  It's easy to
> forget that others have not already applied your patch, but using the
> present tense would make reading easier.  Think of the patch
> description as a special kind of bug report.
> 
> Unfortunately, as a bug report, the above is lacking some detail.  Do
> I understand correctly that some remote helper is failing when git
> invokes its 'import' command?  What are the symptoms?  If it prints a
> warning, what is the exact warning?

I got that problem when I wanted to make remote-svn fetch to refs/remotes/ instead of the formerly hardcoded (in fast-export.c) refs/heads/master. I didn't yet have the refspec capability (now I have it), and that seems to be a significant part of the problem. The scenario is as follows: The fast import stream contains 'commit refs/remotes/svnfile/master' and fast- import adds all the commits on top of it and updates the ref correctly. But the string affected by the patch, 'private', contains the remote name of the branch because it is duped from ref->name, namely refs/heads/master. As a consequence, subsequent processing leads to:

fatal: bad object 0000000000000000000000000000000000000000 error: svn::file:///anypath did not send all necessary objects

..because it expects something to have arrived on refs/heads/master.

If ref/heads/master already exists, it works, but the resulting refs are wrong. refs/remotes/svnfile/master points to the same commit as ref/heads/master does, which is the one created locally. There is no ref to the remote commits.

It follows that, on re-fetching the same remote it fails and fast-import refuses to update the ref: warning: Not updating refs/remotes/svnfile/master (new tip 5479b212afab5ef541c142bf75357405b7888e4d does not contain 4e49b15fcc9797bb90e36ec90c14de3d5437a94d)

That's because the refs/remotes/svnfile/master points to the wrong local-only commit.

After exploring the whole fetch process by inserting dozens of printfs, I concluded that it's wrong to retrieve the sha1 to update by passing the branch name on the remote side (in private) to read_ref, which gives the sha1 of a local branch, but that the correct ref is stored in ->peer_ref. I wasn't really sure what peer-ref is meant to be. That's what lead to the patch, but..

> 
> Does that remote helper advertise the 'refspec' capability?  If so,
> what refspec does it use?  If not, why not?
When it does advertise refspec like:
Debug: Remote helper: <- refspec refs/heads/master:refs/remotes/svnfile/master
it all works. Unfortunatly I didn't understand that a day ago.

But I'm still not completely sure about what the line I wanted to patch is for. Doc about git-remote-helpers says: "If no refspec capability is advertised, there is an implied refspec *:*." Hmm..so if the helper doesn't advertise 'refspec' the remote refs/heads/master is always fetched to the local refs/heads/master? If yes, it makes sense now! A little comment in the sources would help a lot.

Show 7 quoted lines
> 
> It might seem silly to ask for these things when you're providing a
> fix along with the report!  However, if someone else runs into the
> same symptoms, they need to be able to find your patch quickly; if
> your patch has a bad side-effect then we need to know why not to
> revert it; and if someone new starts working on the same area of code,
> they need to know what bugs to avoid reintroducing.
I think we can throw that patch away.
> 
> Curious,
> Jonathan
Jonathan Nieder· Jul 17, 2012, 03:27 UTC · re: Florian Achleitner · lore

Re: [PATCH] Fix overwritten remote ref on with fast-import.

Florian Achleitner wrote:
> When it does advertise refspec like:
> Debug: Remote helper: <- refspec refs/heads/master:refs/remotes/svnfile/master
> it all works. Unfortunatly I didn't understand that a day ago.

Hm, that still doesn't look right. The RHS of the refspec is supposed to be a _private_ namespace for the remote helper, and refs/remotes/ is not private.

Would something like
	refspec refs/heads/*:refs/svn/<name of remote>/*
work?
[...]
> If yes, it makes sense now! A little comment in the sources would help a lot.

Now that you know what the reader of this code needs to know, a patch would be very welcome.

"git blame" or "git log -S" can be useful to find what the authors were thinking when they wrote that line, or to find wording to steal for a comment. :)

Hope that helps, Jonathan

Florian Achleitner· Jul 17, 2012, 09:54 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Fix overwritten remote ref on with fast-import.

On Monday 16 July 2012 22:27:25 Jonathan Nieder wrote:
Show 15 quoted lines
> Florian Achleitner wrote:
> > When it does advertise refspec like:
> > Debug: Remote helper: <- refspec
> > refs/heads/master:refs/remotes/svnfile/master it all works. Unfortunatly
> > I didn't understand that a day ago.
> 
> Hm, that still doesn't look right.  The RHS of the refspec is supposed to
> be a _private_ namespace for the remote helper, and refs/remotes/ is
> not private.
> 
> Would something like
> 
> 	refspec refs/heads/*:refs/svn/<name of remote>/*
> 
> work?

remote-svn now uses get_fetch_map to retrieve the local refs. So it respects the fetch refspec in the config.

If I change the default fetch refspec in .git/config to 'fetch = +refs/heads/*:refs/svn/svnfile/*' it works.

If the remote-helper only advertises the private refspec, but the config and remote->fetch is unchanged, it works also as long as the import target is consistent (of course).

So we do not want helpers to import to remotes/ but to their private svn/ in this case? (ignoring the config?)

Show 11 quoted lines
> 
> [...]
> 
> > If yes, it makes sense now! A little comment in the sources would help a
> > lot.
> Now that you know what the reader of this code needs to know, a patch
> would be very welcome.
> 
> "git blame" or "git log -S" can be useful to find what the authors
> were thinking when they wrote that line, or to find wording to steal
> for a comment. :)
coming ..
> 
> Hope that helps,
> Jonathan
Florian
Jonathan Nieder· Jul 17, 2012, 13:48 UTC · re: Florian Achleitner · lore

Re: [PATCH] Fix overwritten remote ref on with fast-import.

Florian Achleitner wrote:
> On Monday 16 July 2012 22:27:25 Jonathan Nieder wrote:
>> Hm, that still doesn't look right.  The RHS of the refspec is supposed to
>> be a _private_ namespace for the remote helper, and refs/remotes/ is
>> not private.
[...]
> remote-svn now uses get_fetch_map to retrieve the local refs. So it respects 
> the fetch refspec in the config.
No no no no no.  That's transport-helper's job.

The RHS of the remote helper's refspec really is supposed to be _private_. Improvements to the documentation to clarify this would be welcome.

Thanks, Jonathan

Florian Achleitner· Jul 17, 2012, 20:52 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Fix overwritten remote ref on with fast-import.

On Tuesday 17 July 2012 08:48:20 Jonathan Nieder wrote:
Show 16 quoted lines
> Florian Achleitner wrote:
> > On Monday 16 July 2012 22:27:25 Jonathan Nieder wrote:
> >> Hm, that still doesn't look right.  The RHS of the refspec is supposed to
> >> be a _private_ namespace for the remote helper, and refs/remotes/ is
> >> not private.
> 
> [...]
> 
> > remote-svn now uses get_fetch_map to retrieve the local refs. So it
> > respects the fetch refspec in the config.
> 
> No no no no no.  That's transport-helper's job.
> 
> The RHS of the remote helper's refspec really is supposed to be
> _private_.  Improvements to the documentation to clarify this would be
> welcome.

So we want the transport-helper to touch only private refs, i.e. some subdir of refs/, ok. On the other hand I thought we expect git-fetch to update the RHS of the passed refspec (or the default one ). How?

Btw, whats FETCH_HEAD for?
> 
> Thanks,
> Jonathan

Thanks, Florian

Jonathan Nieder· Jul 17, 2012, 21:02 UTC · re: Florian Achleitner · lore

Re: [PATCH] Fix overwritten remote ref on with fast-import.

Hi,
Florian Achleitner wrote:
> So we want the transport-helper to touch only private refs, i.e. some subdir 
> of refs/, ok.
> On the other hand I thought we expect git-fetch to update the RHS of the 
> passed refspec (or the default one ). How?

Now I am getting confused by terminology. By "the transport-helper" do you mean the remote helper (e.g., git-remote-svn) or transport-helper.c?

By the "default" refspec do you mean the one specified in .git/config or some default when none is specified there? "git fetch" updates refs according to the specified fetch refspec in builtin/fetch.c::store_updated_refs().

> Btw, whats FETCH_HEAD for?
"grep FETCH_HEAD Documentation/*.txt" gives some hints.  Most notably:
	git-fetch(1)
	------------
	The ref names and their object names of fetched refs are stored
	in ".git/FETCH_HEAD".  This information is left for a later merge
	operation done by 'git merge'.
	gittutorial(7)
	--------------
	Alice can peek at what Bob did without merging first, using the "fetch"
	command; this allows Alice to inspect what Bob did, using a special
	symbol "FETCH_HEAD", in order to determine if he has anything worth
	pulling, like this:
	------------------------------------------------
	alice$ git fetch /home/bob/myrepo master
	alice$ git log -p HEAD..FETCH_HEAD
	------------------------------------------------

Hope that helps, Jonathan

Florian Achleitner· Jul 17, 2012, 22:25 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Fix overwritten remote ref on with fast-import.

On Tuesday 17 July 2012 16:02:12 Jonathan Nieder wrote:
Show 11 quoted lines
> Hi,
> 
> Florian Achleitner wrote:
> > So we want the transport-helper to touch only private refs, i.e. some
> > subdir of refs/, ok.
> > On the other hand I thought we expect git-fetch to update the RHS of the
> > passed refspec (or the default one ). How?
> 
> Now I am getting confused by terminology.  By "the transport-helper"
> do you mean the remote helper (e.g., git-remote-svn) or
> transport-helper.c?
I was confused too. It should say remote-helper. 
Show 5 quoted lines
> 
> By the "default" refspec do you mean the one specified in .git/config
> or some default when none is specified there?  "git fetch" updates
> refs according to the specified fetch refspec in
> builtin/fetch.c::store_updated_refs().

.. and I didn't realize that the two different refspecs involved here can be different and shall be different because they get post-processed accordingly. I thought the remote-helper has to import according to the fetch refspec.

Show 25 quoted lines
> 
> > Btw, whats FETCH_HEAD for?
> 
> "grep FETCH_HEAD Documentation/*.txt" gives some hints.  Most notably:
> 
> 	git-fetch(1)
> 	------------
> 	The ref names and their object names of fetched refs are stored
> 	in ".git/FETCH_HEAD".  This information is left for a later merge
> 	operation done by 'git merge'.
> 
> 	gittutorial(7)
> 	--------------
> 	Alice can peek at what Bob did without merging first, using the "fetch"
> 	command; this allows Alice to inspect what Bob did, using a special
> 	symbol "FETCH_HEAD", in order to determine if he has anything worth
> 	pulling, like this:
> 
> 	------------------------------------------------
> 	alice$ git fetch /home/bob/myrepo master
> 	alice$ git log -p HEAD..FETCH_HEAD
> 	------------------------------------------------
> 
> Hope that helps,
> Jonathan

Thanks, Florian

Florian Achleitner· Jul 17, 2012, 09:56 UTC · re: Jonathan Nieder · lore

[PATCH] Add explanatory comment for transport-helpers refs mapping.

transport-helpers can advertise the 'refspec' capability, if not a default refspec *:* is assumed. This could be helpful information for the reader.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 transport-helper.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
Show changes to transport-helper.c +10 −0
diff --git a/transport-helper.c b/transport-helper.c
index d6daad5..4a763a7 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -484,8 +484,18 @@ static int fetch_with_import(struct transport *transport,
 		if (posn->status & REF_STATUS_UPTODATE)
 			continue;
 		if (data->refspecs)
+			/*
+			 * If the remote-helper advertised the refpec capability, we
+			 * retrieve the local, private ref from it. The imported data is
+			 * expected there. (see Documentation/git-remote-helpers.*).
+			 */
 			private = apply_refspecs(data->refspecs, data->refspec_nr, posn-
>name);
 		else
+			/*
+			 * else, the default refspec *:* is implied. The remote-helper has
+			 * to import the remote heads directly to the local heads.
+			 * remote-helpers using 'import' should have the refspec capability.
+			 */
 			private = xstrdup(posn->name);
 		if (private) {
 			read_ref(private, posn->old_sha1);
-- 
1.7.9.5
Jonathan Nieder· Jul 17, 2012, 16:04 UTC · re: Florian Achleitner · lore

Re: [PATCH] Add explanatory comment for transport-helpers refs mapping.

Hi,
Florian Achleitner wrote:
Show 20 quoted lines
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -484,8 +484,18 @@ static int fetch_with_import(struct transport *transport,
>  		if (posn->status & REF_STATUS_UPTODATE)
>  			continue;
>  		if (data->refspecs)
> +			/*
> +			 * If the remote-helper advertised the refpec capability, we
> +			 * retrieve the local, private ref from it. The imported data is
> +			 * expected there. (see Documentation/git-remote-helpers.*).
> +			 */
>  			private = apply_refspecs(data->refspecs, data->refspec_nr, posn-
>>name);
>  		else
> +			/*
> +			 * else, the default refspec *:* is implied. The remote-helper has
> +			 * to import the remote heads directly to the local heads.
> +			 * remote-helpers using 'import' should have the refspec capability.
> +			 */
>  			private = xstrdup(posn->name);

What is _exactly_ the information the reader would want to know here? Looking at this code:

		char *private;
		posn = to_fetch[i];
		if (posn->status & REF_STATUS_UPTODATE)
			continue;
		if (!data->refspecs)
			private = xstrdup(...);
		else
			private = apply_refspecs(...);
		if (!private)
			continue;
		read_ref(private, posn->old_sha1);
		free(private);

Despite the misleading "old_sha1" name, this loop runs after fast-import has finished, and the values being read into to_fetch[]::old_sha1 are object names representing the result.

Callers such as builtin/fetch.c then use these values to write feedback to the terminal, to populate FETCH_HEAD, and to determine what new value peer_ref should get.

Shouldn't the comment say something about these SHA-1s not actually being old? Something like:

	/*
	 * If the remote helper advertised the "refspec" capability,
	 * it will have the written result of the import to the refs
	 * named on the right hand side of the first refspec matching
	 * each ref we were fetching.
	 *
	 * (If no "refspec" capability was specified, for historical
	 * reasons we default to *:*.)
	 *
	 * Store the result in to_fetch[i].old_sha1.  Callers such
	 * as "git fetch" can use the value to write feedback to the
	 * terminal, populate FETCH_HEAD, and determine what new value
	 * should be written to peer_ref if the update is a
	 * fast-forward or this is a forced update.
	 */
	for (i = 0; ...

Hmm? Jonathan

← back to recent threads