* [RFC/PATCH 0/4] GSOC remote-svn @ 2012-07-11 13:38 Florian Achleitner 2012-07-11 13:38 ` [PATCH 1/4] vcs-svn: add fast_export_note to create notes Florian Achleitner 2012-07-15 14:26 ` [PATCH] Fix overwritten remote ref on with fast-import Florian Achleitner 0 siblings, 2 replies; 20+ messages in thread From: Florian Achleitner @ 2012-07-11 13:38 UTC (permalink / raw) To: git, David Michael Barr, Jonathan Nieder; +Cc: florian.achleitner.2.6.31 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/4] vcs-svn: add fast_export_note to create notes 2012-07-11 13:38 [RFC/PATCH 0/4] GSOC remote-svn Florian Achleitner @ 2012-07-11 13:38 ` Florian Achleitner 2012-07-11 13:38 ` [PATCH 2/4] Allow reading svn dumps from files via file:// urls Florian Achleitner 2012-07-15 14:26 ` [PATCH] Fix overwritten remote ref on with fast-import Florian Achleitner 1 sibling, 1 reply; 20+ messages in thread From: Florian Achleitner @ 2012-07-11 13:38 UTC (permalink / raw) To: git, David Michael Barr, Jonathan Nieder Cc: florian.achleitner.2.6.31, Dmitry Ivankov 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(+) 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/4] Allow reading svn dumps from files via file:// urls. 2012-07-11 13:38 ` [PATCH 1/4] vcs-svn: add fast_export_note to create notes Florian Achleitner @ 2012-07-11 13:38 ` Florian Achleitner 2012-07-11 13:38 ` [PATCH 3/4] Create a note for every imported commit containing svn metadata Florian Achleitner 2012-07-11 14:29 ` [PATCH 2/4] Allow reading svn dumps from files via file:// urls Dmitry Ivankov 0 siblings, 2 replies; 20+ messages in thread From: Florian Achleitner @ 2012-07-11 13:38 UTC (permalink / raw) To: git, David Michael Barr, Jonathan Nieder; +Cc: florian.achleitner.2.6.31 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(-) 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/4] Create a note for every imported commit containing svn metadata. 2012-07-11 13:38 ` [PATCH 2/4] Allow reading svn dumps from files via file:// urls Florian Achleitner @ 2012-07-11 13:38 ` Florian Achleitner 2012-07-11 13:38 ` [PATCH 4/4] When debug==1, start fast-import with "--stats" instead of "--quiet" Florian Achleitner 2012-07-11 14:29 ` [PATCH 2/4] Allow reading svn dumps from files via file:// urls Dmitry Ivankov 1 sibling, 1 reply; 20+ messages in thread From: Florian Achleitner @ 2012-07-11 13:38 UTC (permalink / raw) To: git, David Michael Barr, Jonathan Nieder; +Cc: florian.achleitner.2.6.31 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(-) 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/4] When debug==1, start fast-import with "--stats" instead of "--quiet". 2012-07-11 13:38 ` [PATCH 3/4] Create a note for every imported commit containing svn metadata Florian Achleitner @ 2012-07-11 13:38 ` Florian Achleitner 0 siblings, 0 replies; 20+ messages in thread From: Florian Achleitner @ 2012-07-11 13:38 UTC (permalink / raw) To: git, David Michael Barr, Jonathan Nieder; +Cc: florian.achleitner.2.6.31 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(-) 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] Allow reading svn dumps from files via file:// urls. 2012-07-11 13:38 ` [PATCH 2/4] Allow reading svn dumps from files via file:// urls Florian Achleitner 2012-07-11 13:38 ` [PATCH 3/4] Create a note for every imported commit containing svn metadata Florian Achleitner @ 2012-07-11 14:29 ` Dmitry Ivankov 2012-07-11 17:00 ` Junio C Hamano 1 sibling, 1 reply; 20+ messages in thread From: Dmitry Ivankov @ 2012-07-11 14:29 UTC (permalink / raw) To: git 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. Maybe something like dumpfile:// instead? [patch skipped] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] Allow reading svn dumps from files via file:// urls. 2012-07-11 14:29 ` [PATCH 2/4] Allow reading svn dumps from files via file:// urls Dmitry Ivankov @ 2012-07-11 17:00 ` Junio C Hamano 2012-07-11 17:49 ` Stephen Bash 0 siblings, 1 reply; 20+ messages in thread From: Junio C Hamano @ 2012-07-11 17:00 UTC (permalink / raw) To: Dmitry Ivankov; +Cc: git 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] Allow reading svn dumps from files via file:// urls. 2012-07-11 17:00 ` Junio C Hamano @ 2012-07-11 17:49 ` Stephen Bash 0 siblings, 0 replies; 20+ messages in thread From: Stephen Bash @ 2012-07-11 17:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Dmitry Ivankov ----- Original Message ----- > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] Fix overwritten remote ref on with fast-import. 2012-07-11 13:38 [RFC/PATCH 0/4] GSOC remote-svn Florian Achleitner 2012-07-11 13:38 ` [PATCH 1/4] vcs-svn: add fast_export_note to create notes Florian Achleitner @ 2012-07-15 14:26 ` Florian Achleitner 2012-07-16 0:30 ` Jonathan Nieder 1 sibling, 1 reply; 20+ messages in thread From: Florian Achleitner @ 2012-07-15 14:26 UTC (permalink / raw) To: git, David Michael Barr Cc: florian.achleitner.2.6.31, Jeff King, Jonathan Nieder, Michael Haggerty, Sverre Rabbelier 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(-) 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix overwritten remote ref on with fast-import. 2012-07-15 14:26 ` [PATCH] Fix overwritten remote ref on with fast-import Florian Achleitner @ 2012-07-16 0:30 ` Jonathan Nieder 2012-07-16 4:33 ` Junio C Hamano 2012-07-16 22:33 ` Florian Achleitner 0 siblings, 2 replies; 20+ messages in thread From: Jonathan Nieder @ 2012-07-16 0:30 UTC (permalink / raw) To: Florian Achleitner Cc: git, David Michael Barr, Jeff King, Michael Haggerty, Sverre Rabbelier 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! 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix overwritten remote ref on with fast-import. 2012-07-16 0:30 ` Jonathan Nieder @ 2012-07-16 4:33 ` Junio C Hamano 2012-07-16 22:33 ` Florian Achleitner 1 sibling, 0 replies; 20+ messages in thread From: Junio C Hamano @ 2012-07-16 4:33 UTC (permalink / raw) To: Jonathan Nieder Cc: Florian Achleitner, git, David Michael Barr, Jeff King, Michael Haggerty, Sverre Rabbelier Jonathan Nieder <jrnieder@gmail.com> writes: > 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. > 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix overwritten remote ref on with fast-import. 2012-07-16 0:30 ` Jonathan Nieder 2012-07-16 4:33 ` Junio C Hamano @ 2012-07-16 22:33 ` Florian Achleitner 2012-07-17 3:27 ` Jonathan Nieder 1 sibling, 1 reply; 20+ messages in thread From: Florian Achleitner @ 2012-07-16 22:33 UTC (permalink / raw) To: Jonathan Nieder Cc: Florian Achleitner, git, David Michael Barr, Jeff King, Michael Haggerty, Sverre Rabbelier, Junio C Hamano On Sunday 15 July 2012 19:30:25 Jonathan Nieder wrote: > 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. > > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix overwritten remote ref on with fast-import. 2012-07-16 22:33 ` Florian Achleitner @ 2012-07-17 3:27 ` Jonathan Nieder 2012-07-17 9:54 ` Florian Achleitner 2012-07-17 9:56 ` [PATCH] Add explanatory comment for transport-helpers refs mapping Florian Achleitner 0 siblings, 2 replies; 20+ messages in thread From: Jonathan Nieder @ 2012-07-17 3:27 UTC (permalink / raw) To: Florian Achleitner Cc: git, David Michael Barr, Jeff King, Michael Haggerty, Sverre Rabbelier, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix overwritten remote ref on with fast-import. 2012-07-17 3:27 ` Jonathan Nieder @ 2012-07-17 9:54 ` Florian Achleitner 2012-07-17 13:48 ` Jonathan Nieder 2012-07-17 9:56 ` [PATCH] Add explanatory comment for transport-helpers refs mapping Florian Achleitner 1 sibling, 1 reply; 20+ messages in thread From: Florian Achleitner @ 2012-07-17 9:54 UTC (permalink / raw) To: Jonathan Nieder Cc: Florian Achleitner, git, David Michael Barr, Jeff King, Michael Haggerty, Sverre Rabbelier, Junio C Hamano On Monday 16 July 2012 22:27:25 Jonathan Nieder wrote: > 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?) > > [...] > > > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix overwritten remote ref on with fast-import. 2012-07-17 9:54 ` Florian Achleitner @ 2012-07-17 13:48 ` Jonathan Nieder 2012-07-17 20:52 ` Florian Achleitner 0 siblings, 1 reply; 20+ messages in thread From: Jonathan Nieder @ 2012-07-17 13:48 UTC (permalink / raw) To: Florian Achleitner Cc: git, David Michael Barr, Jeff King, Michael Haggerty, Sverre Rabbelier, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix overwritten remote ref on with fast-import. 2012-07-17 13:48 ` Jonathan Nieder @ 2012-07-17 20:52 ` Florian Achleitner 2012-07-17 21:02 ` Jonathan Nieder 0 siblings, 1 reply; 20+ messages in thread From: Florian Achleitner @ 2012-07-17 20:52 UTC (permalink / raw) To: Jonathan Nieder Cc: Florian Achleitner, git, David Michael Barr, Jeff King, Michael Haggerty, Sverre Rabbelier, Junio C Hamano On Tuesday 17 July 2012 08:48:20 Jonathan Nieder wrote: > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix overwritten remote ref on with fast-import. 2012-07-17 20:52 ` Florian Achleitner @ 2012-07-17 21:02 ` Jonathan Nieder 2012-07-17 22:25 ` Florian Achleitner 0 siblings, 1 reply; 20+ messages in thread From: Jonathan Nieder @ 2012-07-17 21:02 UTC (permalink / raw) To: Florian Achleitner Cc: git, David Michael Barr, Jeff King, Michael Haggerty, Sverre Rabbelier, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix overwritten remote ref on with fast-import. 2012-07-17 21:02 ` Jonathan Nieder @ 2012-07-17 22:25 ` Florian Achleitner 0 siblings, 0 replies; 20+ messages in thread From: Florian Achleitner @ 2012-07-17 22:25 UTC (permalink / raw) To: Jonathan Nieder Cc: Florian Achleitner, git, David Michael Barr, Jeff King, Michael Haggerty, Sverre Rabbelier, Junio C Hamano On Tuesday 17 July 2012 16:02:12 Jonathan Nieder wrote: > 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. > > 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. > > > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] Add explanatory comment for transport-helpers refs mapping. 2012-07-17 3:27 ` Jonathan Nieder 2012-07-17 9:54 ` Florian Achleitner @ 2012-07-17 9:56 ` Florian Achleitner 2012-07-17 16:04 ` Jonathan Nieder 1 sibling, 1 reply; 20+ messages in thread From: Florian Achleitner @ 2012-07-17 9:56 UTC (permalink / raw) To: Jonathan Nieder Cc: Florian Achleitner, git, David Michael Barr, Jeff King, Michael Haggerty, Sverre Rabbelier, Junio C Hamano 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(+) 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] Add explanatory comment for transport-helpers refs mapping. 2012-07-17 9:56 ` [PATCH] Add explanatory comment for transport-helpers refs mapping Florian Achleitner @ 2012-07-17 16:04 ` Jonathan Nieder 0 siblings, 0 replies; 20+ messages in thread From: Jonathan Nieder @ 2012-07-17 16:04 UTC (permalink / raw) To: Florian Achleitner Cc: git, David Michael Barr, Jeff King, Michael Haggerty, Sverre Rabbelier, Junio C Hamano Hi, Florian Achleitner wrote: > --- 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2012-07-17 22:25 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-11 13:38 [RFC/PATCH 0/4] GSOC remote-svn Florian Achleitner 2012-07-11 13:38 ` [PATCH 1/4] vcs-svn: add fast_export_note to create notes Florian Achleitner 2012-07-11 13:38 ` [PATCH 2/4] Allow reading svn dumps from files via file:// urls Florian Achleitner 2012-07-11 13:38 ` [PATCH 3/4] Create a note for every imported commit containing svn metadata Florian Achleitner 2012-07-11 13:38 ` [PATCH 4/4] When debug==1, start fast-import with "--stats" instead of "--quiet" Florian Achleitner 2012-07-11 14:29 ` [PATCH 2/4] Allow reading svn dumps from files via file:// urls Dmitry Ivankov 2012-07-11 17:00 ` Junio C Hamano 2012-07-11 17:49 ` Stephen Bash 2012-07-15 14:26 ` [PATCH] Fix overwritten remote ref on with fast-import Florian Achleitner 2012-07-16 0:30 ` Jonathan Nieder 2012-07-16 4:33 ` Junio C Hamano 2012-07-16 22:33 ` Florian Achleitner 2012-07-17 3:27 ` Jonathan Nieder 2012-07-17 9:54 ` Florian Achleitner 2012-07-17 13:48 ` Jonathan Nieder 2012-07-17 20:52 ` Florian Achleitner 2012-07-17 21:02 ` Jonathan Nieder 2012-07-17 22:25 ` Florian Achleitner 2012-07-17 9:56 ` [PATCH] Add explanatory comment for transport-helpers refs mapping Florian Achleitner 2012-07-17 16:04 ` Jonathan Nieder
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.