This is a second iteration of [1].
The most significant changes since [1] are:
1) svndump_init parameters are wrapped into a struct svndump_init_args
This way it is a bit easier to play with various parameters.
Thanks to Ram for insisting on this.
2) added [6/11] "vcs-svn: move commit parameters logic to svndump.c"
Now fast_export is touched only in one commit of this series and there
are other nice implications mentioned in the commit message.
3) added [10/11] and [11/11] - two more options to be used in a
upcoming iteration of remote-svn-alpha series
The patch base is svn-fe branch at git://repo.or.cz/git/jrn.git
[1] http://thread.gmane.org/gmane.comp.version-control.git/176578
Dmitry Ivankov (11):
svn-fe: add man target to Makefile
test-svn-fe: use parse-options
svn-fe: add EXTLIBS needed for parse-options
svn-fe: add usage and unpositional arguments versions
vcs-svn: move url parameter from _read to _init
vcs-svn: move commit parameters logic to svndump.c
vcs-svn,svn-fe: allow to specify dump destination ref
vcs-svn,svn-fe: convert REPORT_FILENO to an option
vcs-svn,svn-fe: allow to disable 'progress' lines
vcs-svn,svn-fe: add --incremental option
vcs-svn,svn-fe: add an option to write svnrev notes
contrib/svn-fe/Makefile | 18 +++---
contrib/svn-fe/svn-fe.c | 46 ++++++++++++-
contrib/svn-fe/svn-fe.txt | 37 +++++++++--
t/t9010-svn-fe.sh | 157 +++++++++++++++++++++++++++++++++++++++-----
test-svn-fe.c | 59 +++++++++++++----
vcs-svn/fast_export.c | 44 ++++---------
vcs-svn/fast_export.h | 8 ++-
vcs-svn/svndump.c | 82 +++++++++++++++++++----
vcs-svn/svndump.h | 11 +++-
9 files changed, 361 insertions(+), 101 deletions(-)
--
1.7.3.4
There was custom options parsing. As more options arise it will
be easier to add and document new options with parse-options api.
Signed-off-by: Dmitry Ivankov <redacted>
---
test-svn-fe.c | 42 ++++++++++++++++++++++++++++--------------
1 files changed, 28 insertions(+), 14 deletions(-)
Currently parse-options.o pull quite a big bunch of dependencies
that are neither pulled in by svn-fe Makefile nor included in libgit.a.
Use a temporary hack: put hardcoded EXTLIBS, this may not work in all
setups because /Makefile logic is not repeated.
For example, one may need -lcrypto instead of -lssl or no crypto library
if BLK_SHA1 is set, also an additional -lz or -lpcre could be required.
Signed-off-by: Dmitry Ivankov <redacted>
---
contrib/svn-fe/Makefile | 12 +++++-------
1 files changed, 5 insertions(+), 7 deletions(-)
@@ -8,11 +8,12 @@ CFLAGS = -g -O2 -WallLDFLAGS=ALL_CFLAGS=$(CFLAGS)ALL_LDFLAGS=$(LDFLAGS)-EXTLIBS=+EXTLIBS=-lssl-lpthreadGIT_LIB=../../libgit.aVCSSVN_LIB=../../vcs-svn/lib.a-LIBS=$(VCSSVN_LIB)$(GIT_LIB)$(EXTLIBS)+XDIFF_LIB=../../xdiff/lib.a+LIBS=$(VCSSVN_LIB)$(GIT_LIB)$(XDIFF_LIB)$(EXTLIBS)QUIET_SUBDIR0=+$(MAKE)-C# space to separate -C and subdirQUIET_SUBDIR1=
There already is a svn-fe.1 target. But 'man' being a standard
target is easier to discover or type. It can also be reused if
more manpages arise here.
Signed-off-by: Dmitry Ivankov <redacted>
---
contrib/svn-fe/Makefile | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
There will be more command line options for svn-fe so support
unpositional version for flexibility. Also clarify the meaning
of url parameter.
$ svn-fe --git-svn-id-url=url
does the same thing as
$ svn-fe url
i.e., url is used to generate git-svn-id: lines, if url is set.
Signed-off-by: Dmitry Ivankov <redacted>
---
contrib/svn-fe/Makefile | 2 +-
contrib/svn-fe/svn-fe.c | 30 +++++++++++++++++++++++++++---
contrib/svn-fe/svn-fe.txt | 17 +++++++++++++----
3 files changed, 41 insertions(+), 8 deletions(-)
@@ -3,14 +3,38 @@*Youmayfreelyuse,modify,distribute,andrelicenseit.*/-#include<stdlib.h>+#include"git-compat-util.h"+#include"parse-options.h"#include"svndump.h"-intmain(intargc,char**argv)+staticconstchar*constsvn_fe_usage[]={+"svn-fe [options] [git-svn-id-url] < dump | fast-import-backend",+NULL+};++staticconstchar*url;++staticstructoptionsvn_fe_options[]={+OPT_STRING(0,"git-svn-id-url",&url,"url",+"append git-svn metadata line to commit messages"),+OPT_END()+};++intmain(intargc,constchar**argv){+argc=parse_options(argc,argv,NULL,svn_fe_options,+svn_fe_usage,0);+if(argc==1){+if(url)+usage_msg_opt("git-svn-id-url is set twice: as a "+"--parameter and as a [parameter]",+svn_fe_usage,svn_fe_options);+url=argv[0];+}elseif(argc)+usage_with_options(svn_fe_usage,svn_fe_options);if(svndump_init(NULL))return1;-svndump_read((argc>1)?argv[1]:NULL);+svndump_read(url);svndump_deinit();svndump_reset();return0;
@@ -25,6 +25,14 @@ command. Note: this tool is very young. The details of its commandline interface may change in backward incompatible ways.+OPTIONS+-------++--git-svn-id-url=<url>::+ Url to be used in git-svn-id: lines in git-svn+ metadata lines format. See NOTES for more detailed+ description.+ INPUT FORMAT ------------ Subversion's repository dump format is documented in full in
@@ -50,9 +58,10 @@ user <user@UUID> as committer, where 'user' is the value of the `svn:author` property and 'UUID' the repository's identifier.-To support incremental imports, 'svn-fe' puts a `git-svn-id` line at-the end of each commit log message if passed an url on the command-line. This line has the form `git-svn-id: URL@REVNO UUID`.+'svn-fe' can be used in preparing a repository for 'git-svn' as follows.+If `git-svn-id-url` is specified, 'svn-fe' will put `git-svn-id` line at+the end of each commit log message.+This line has the form `git-svn-id: URL@REVNO UUID`. The resulting repository will generally require further processing to put each project in its own repository and to separate the history
svn-fe needs to read fast-import's responses to "ls" and "cat-blob".
These come through a file descriptor number 3.
Sometimes it is easier to setup variable fd than a fixed one. It is
the case with pipe() call and even more fd=3 can be already taken.
On Windows file descriptors are not by default inherited by a child
process, nor there is an option to setup descriptors other than
standard stdin, stdout, stderr at a process creation time.
Add an option for this file descriptor number in vcs-svn/ and svn-fe,
add a simple test for it.
To be used like following:
$ svn-fe --read-blob-fd=7 ... 7<somewhere
Signed-off-by: Dmitry Ivankov <redacted>
---
contrib/svn-fe/svn-fe.c | 3 ++
contrib/svn-fe/svn-fe.txt | 8 ++++-
t/t9010-svn-fe.sh | 58 +++++++++++++++++++++++++++++++++++++++++---
test-svn-fe.c | 3 ++
vcs-svn/svndump.c | 4 +--
vcs-svn/svndump.h | 1 +
6 files changed, 68 insertions(+), 9 deletions(-)
@@ -19,12 +19,15 @@ static struct option svn_fe_options[] = {"append git-svn metadata line to commit messages"),OPT_STRING(0,"ref",&args.ref,"dst_ref","write to dst_ref instead of refs/heads/master"),+OPT_INTEGER(0,"read-blob-fd",&args.backflow_fd,+"read blobs and trees from this fd instead of 3"),OPT_END()};intmain(intargc,constchar**argv){args.ref="refs/heads/master";+args.backflow_fd=3;argc=parse_options(argc,argv,NULL,svn_fe_options,svn_fe_usage,0);if(argc==1){
@@ -35,6 +35,10 @@ OPTIONS --ref=<dst_ref>:: Ref to be written by the generated stream. Default is refs/heads/master.+--read-blob-fd=<fd>::+ Integer number of file descriptor from which+ responses to 'ls' and 'cat-blob' requests will come.+ Default is fd=3. INPUT FORMAT ------------
@@ -1120,10 +1121,59 @@ test_expect_success SVNREPO,PIPE 't9135/svn.dump' ' test_expect_successPIPE'import to notmaster ref''reinit_git&&-try_dump_ext"--ref=refs/heads/notmaster"emptyprop.dump&&+try_dump_ext"--ref=refs/heads/notmaster"3emptyprop.dump&&gitrev-parse--verifynotmaster&&test_must_failgitrev-parse--verifymaster'+test_expect_successPIPE'use different backflow fd''+reinit_git&&+echohi>hi&&+{+properties\+svn:authorauthor@example.com\+svn:date"1999-02-01T00:01:002.000000Z"\+svn:log"add directory with some files in it"&&+echoPROPS-END+}>props&&+{+echoProp-content-length:$(wc-c<props)&&+echoContent-length:$(wc-c<props)&&+echo&&+catprops+}>revprops&&+{+cat<<-EOF&&+SVN-fs-dump-format-version:3++Revision-number:1+EOF+catrevprops&&+cat<<-EOF&&+Node-path:directory+Node-kind:dir+Node-action:add+Node-path:directory/somefile+Node-kind:file+Node-action:add+EOF+text_no_propshi&&++echo"Revision-number: 2"&&+catrevprops&&+cat<<-\EOF+Node-path:otherfile+Node-kind:file+Node-action:add+Node-copyfrom-rev:1+Node-copyfrom-path:directory/somefile+EOF+}>directory.dump&&+try_dump_ext"--read-blob-fd=7"7directory.dump&&++gitcheckoutHEADotherfile&&+test_cmphiotherfile+'+ test_done
@@ -22,6 +22,8 @@ static struct option test_svnfe_options[] = {OPT_SET_INT('d',NULL,&d,"test apply_delta",1),OPT_STRING(0,"ref",&args.ref,"dst_ref","write to dst_ref instead of refs/heads/master"),+OPT_INTEGER(0,"read-blob-fd",&args.backflow_fd,+"read blobs and trees from this fd instead of 3"),OPT_END()};
This option is to make svn-fe write commits on top of the existing ref
instead of overwriting it. More precise, the first commit's parent is
set to be :(first_revision_in_current_dump - 1) mark.
Prerequisite is to (re)use import marks (from previous imports). It is
safe to use this option on a svn dump that starts with r0/r1. The svn
dump itself should be incremental too.
Signed-off-by: Dmitry Ivankov <redacted>
---
contrib/svn-fe/svn-fe.c | 3 +++
contrib/svn-fe/svn-fe.txt | 5 +++++
t/t9010-svn-fe.sh | 34 +++++++++++++++++++++++++++++-----
test-svn-fe.c | 3 +++
vcs-svn/svndump.c | 13 +++++++++----
vcs-svn/svndump.h | 2 +-
6 files changed, 50 insertions(+), 10 deletions(-)
@@ -18,6 +18,9 @@ static struct option svn_fe_options[] = {{OPTION_BIT,0,"progress",&args.progress,NULL,"don't write a progress line after each commit",PARSE_OPT_NOARG|PARSE_OPT_NEGHELP,NULL,1},+OPT_BIT(0,"incremental",&args.incremental,+"resume export, requires marks and incremental dump",+1),OPT_STRING(0,"git-svn-id-url",&args.url,"url","append git-svn metadata line to commit messages"),OPT_STRING(0,"ref",&args.ref,"dst_ref",
@@ -42,6 +42,11 @@ OPTIONS --[no-]progress:: Write 'progress' lines to fast-import stream. These can be displayed by fast-import.+--incremental::+ If the first revision in dump has number greater than+ 1, make :(revision - 1) it's parent. For this to work+ fast-import must be supplied with import-marks file+ and the dump must be incremental. INPUT FORMAT ------------
@@ -22,6 +22,9 @@ static struct option test_svnfe_options[] = {OPT_SET_INT('d',NULL,&d,"test apply_delta",1),OPT_STRING(0,"ref",&args.ref,"dst_ref","write to dst_ref instead of refs/heads/master"),+OPT_BIT(0,"incremental",&args.incremental,+"resume export, requires marks and incremental dump",+1),OPT_INTEGER(0,"read-blob-fd",&args.backflow_fd,"read blobs and trees from this fd instead of 3"),OPT_END()
svn-fe produces fast-import stream for a fixed refs/heads/master ref.
It is usually desired to write to a different ref. In a remote helper
it would be a ref in private namespace. If svn-fe is used by someone
directly it'll be more safe to remind where the commits can go. And
in both cases it may be needed to import from two repos and hence to
different refs.
Add a destination ref parameter to vcs-svn/, a corresponding parameter
to svn-fe and a simple test for it.
$ svn-fe --ref=refs/heads/master ...
is an explicit way to stay with the default destination.
Signed-off-by: Dmitry Ivankov <redacted>
---
contrib/svn-fe/svn-fe.c | 3 ++
contrib/svn-fe/svn-fe.txt | 3 ++
t/t9010-svn-fe.sh | 49 +++++++++++++++++++++++++++++---------------
test-svn-fe.c | 5 +++-
vcs-svn/svndump.c | 11 ++++++---
vcs-svn/svndump.h | 1 +
6 files changed, 50 insertions(+), 22 deletions(-)
@@ -17,11 +17,14 @@ static struct svndump_args args;staticstructoptionsvn_fe_options[]={OPT_STRING(0,"git-svn-id-url",&args.url,"url","append git-svn metadata line to commit messages"),+OPT_STRING(0,"ref",&args.ref,"dst_ref",+"write to dst_ref instead of refs/heads/master"),OPT_END()};intmain(intargc,constchar**argv){+args.ref="refs/heads/master";argc=parse_options(argc,argv,NULL,svn_fe_options,svn_fe_usage,0);if(argc==1){
@@ -32,6 +32,9 @@ OPTIONS Url to be used in git-svn-id: lines in git-svn metadata lines format. See NOTES for more detailed description.+--ref=<dst_ref>::+ Ref to be written by the generated stream.+ Default is refs/heads/master. INPUT FORMAT ------------
@@ -20,6 +20,8 @@ static int d;staticstructoptiontest_svnfe_options[]={OPT_SET_INT('d',NULL,&d,"test apply_delta",1),+OPT_STRING(0,"ref",&args.ref,"dst_ref",+"write to dst_ref instead of refs/heads/master"),OPT_END()};
svndump_read takes a url parameter that is used in git-svn-id: lines
generation. Internally it is stored in dump_ctx which is initialized
in svndump_init with reset_dump_ctx and then is reinitialized again
in svndump_read.
Move url parameter to svndump_init so that reset_dump_ctx is done
once per dump and in the same place as other resets. Wrap all _init
parameters to a struct svndump_args. More parameters will arise and
all will go to this struct to setup the module for dumping. Having
a struct reduces a chance to confuse one parameter with another a
bit, if they are filled via named assignments or common defines.
Signed-off-by: Dmitry Ivankov <redacted>
---
contrib/svn-fe/svn-fe.c | 12 ++++++------
test-svn-fe.c | 7 +++++--
vcs-svn/svndump.c | 12 ++++++------
vcs-svn/svndump.h | 8 ++++++--
4 files changed, 23 insertions(+), 16 deletions(-)
@@ -12,10 +12,10 @@ static const char * const svn_fe_usage[] = {NULL};-staticconstchar*url;+staticstructsvndump_argsargs;staticstructoptionsvn_fe_options[]={-OPT_STRING(0,"git-svn-id-url",&url,"url",+OPT_STRING(0,"git-svn-id-url",&args.url,"url","append git-svn metadata line to commit messages"),OPT_END()};
@@ -25,16 +25,16 @@ int main(int argc, const char **argv)argc=parse_options(argc,argv,NULL,svn_fe_options,svn_fe_usage,0);if(argc==1){-if(url)+if(args.url)usage_msg_opt("git-svn-id-url is set twice: as a ""--parameter and as a [parameter]",svn_fe_usage,svn_fe_options);-url=argv[0];+args.url=argv[0];}elseif(argc)usage_with_options(svn_fe_usage,svn_fe_options);-if(svndump_init(NULL))+if(svndump_init(&args))return1;-svndump_read(url);+svndump_read();svndump_deinit();svndump_reset();return0;
vcs-svn/ writes a progress line after each processed revision. It
is too noisy for big imports. That's a stress for a terminal and
any other output can be lost or scrolled away among these lines.
If svn-fe is invoked by a remote helper the import stream with
progress lines in it will go directly to the git fast-import which
always prints every progress line met in the stream.
For now just add a switch to turn progress lines off:
$ svn-fe --no-progress ...
Signed-off-by: Dmitry Ivankov <redacted>
---
contrib/svn-fe/svn-fe.c | 3 +++
contrib/svn-fe/svn-fe.txt | 3 +++
test-svn-fe.c | 1 +
vcs-svn/svndump.c | 6 +++++-
vcs-svn/svndump.h | 1 +
5 files changed, 13 insertions(+), 1 deletions(-)
@@ -15,6 +15,9 @@ static const char * const svn_fe_usage[] = {staticstructsvndump_argsargs;staticstructoptionsvn_fe_options[]={+{OPTION_BIT,0,"progress",&args.progress,+NULL,"don't write a progress line after each commit",+PARSE_OPT_NOARG|PARSE_OPT_NEGHELP,NULL,1},OPT_STRING(0,"git-svn-id-url",&args.url,"url","append git-svn metadata line to commit messages"),OPT_STRING(0,"ref",&args.ref,"dst_ref",
@@ -39,6 +39,9 @@ OPTIONS Integer number of file descriptor from which responses to 'ls' and 'cat-blob' requests will come. Default is fd=3.+--[no-]progress::+ Write 'progress' lines to fast-import stream. These+ can be displayed by fast-import. INPUT FORMAT ------------
fast_export.c had logic to set up commit ref, author name, email,
parent commit, import mark and git-svn-id: line based on both it's
own state (current import batch history) and the arguments passed.
Lift the decision on these parameters to the caller. This way it is
easier to customize them. Move progress lines generation to the caller
for the same reason.
Now fast_export doesn't have any internal state except the files set
up in fast_export_init, so it doesn't rely on being passed commits
sequentially and to one and the same branch. It operates just on a
current commit. Which makes it possible to generate an incremental
stream (if stream's first commit parent is set up properly by the
caller) or maybe to generate a stream for multiple svn branches.
Also progress lines generation is lifted up to svndump.o. So further
progress indication enhancements won't need to change fast_export.o
api.
Signed-off-by: Dmitry Ivankov <redacted>
---
vcs-svn/fast_export.c | 44 ++++++++++++++------------------------------
vcs-svn/fast_export.h | 8 +++++---
vcs-svn/svndump.c | 30 ++++++++++++++++++++++++++----
3 files changed, 45 insertions(+), 37 deletions(-)
There are already a few options to determine svn revision from which
a git commit imported with svn-fe came from. One is to make svn-fe
write a git-svn-id: line to commit messages. Another one is to calc
distance to the root commit. The former includes a "url" and is for
git-svn compatibility, the latter is obviously slow and a bit fragile.
$ svn-fe --notes_ref=notes_tree --ref=branch...
will write annotations for branch commits to the notes_tree, each
annotation is a simple "rN" string. Then these annotations can be
viewed manually or used in incremental import to detect the last
imported revision or to (re)create the import marks for further
imports.
Signed-off-by: Dmitry Ivankov <redacted>
---
contrib/svn-fe/svn-fe.c | 2 ++
contrib/svn-fe/svn-fe.txt | 3 +++
t/t9010-svn-fe.sh | 32 ++++++++++++++++++++++++++++++++
test-svn-fe.c | 2 ++
vcs-svn/svndump.c | 28 ++++++++++++++++++++++++----
vcs-svn/svndump.h | 2 +-
6 files changed, 64 insertions(+), 5 deletions(-)
@@ -25,6 +25,8 @@ static struct option svn_fe_options[] = {"append git-svn metadata line to commit messages"),OPT_STRING(0,"ref",&args.ref,"dst_ref","write to dst_ref instead of refs/heads/master"),+OPT_STRING(0,"notes-ref",&args.notes_ref,"notes",+"write \"rN\" notes to the <notes> tree"),OPT_INTEGER(0,"read-blob-fd",&args.backflow_fd,"read blobs and trees from this fd instead of 3"),OPT_END()
@@ -35,6 +35,9 @@ OPTIONS --ref=<dst_ref>:: Ref to be written by the generated stream. Default is refs/heads/master.+--notes-ref=<notes_ref>::+ Write "rN" notes to the notes_ref tree for each+ imported commit. --read-blob-fd=<fd>:: Integer number of file descriptor from which responses to 'ls' and 'cat-blob' requests will come.
@@ -27,6 +27,8 @@ static struct option test_svnfe_options[] = {1),OPT_INTEGER(0,"read-blob-fd",&args.backflow_fd,"read blobs and trees from this fd instead of 3"),+OPT_STRING(0,"notes-ref",&args.notes_ref,"notes",+"write \"rN\" notes to the <notes> tree"),OPT_END()};
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:38
Hi Dmitry,
Dmitry Ivankov wrote:
There already is a svn-fe.1 target. But 'man' being a standard
target is easier to discover or type. It can also be reused if
more manpages arise here.
This will make life a little easier when wanting to test-build
git-remote-svn.1 along with svn-fe.1 (once git-remote-svn exists).
Thanks, queued.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:38
Hi,
Sorry for the slow response.
Dmitry Ivankov wrote:
There was custom options parsing. As more options arise it will
be easier to add and document new options with parse-options api.
In particular this gives us a "test-svn-fe -h" command --- sounds
good. Might make sense to combine this with the patch that
parsoptifies contrib/svn-fe/svn-fe.c.
With this API, we're allowed to print multiple usage strings. Might as
well take advantage of that for clarity:
static const char * const test_svnfe_usage[] = {
"test-svn-fe <dumpfile>",
"test-svn-fe -d <preimage> <delta> <len>",
NULL
};
+static int d;
+
The variable name is not so memorable. Maybe something like
"apply_delta" would do.
Might make sense to take the opportunity to add a mnemonic long
option name while at it:
OPT_SET_INT('d', "apply-delta", ...
[...]
quoted hunk
@@ -37,10 +47,16 @@ static int apply_delta(int argc, char *argv[]) return 0; }-int main(int argc, char *argv[])+int main(int argc, const char *argv[]) {- if (argc == 2) {- if (svndump_init(argv[1]))+ argc = parse_options(argc, argv, NULL, test_svnfe_options,+ test_svnfe_usage, 0);++ if (d)+ return apply_delta(argc, argv);++ if (argc == 1) {
Probably easier to read with the simple and exceptional case first.
if (apply_delta_instead)
return apply_delta(argc, argv);
if (argc != 1)
usage_with_options(...);
if (svndump_init(argv[0]))
return 1;
...
quoted hunk
+ if (svndump_init(argv[0]))
return 1;
svndump_read(NULL);
svndump_deinit();
Except for the minor nits noted above (in particular, hopefully this
can be squashed with the corresponding svn-fe patch),
Reviewed-by: Jonathan Nieder <redacted>
---
test-svn-fe.c | 29 +++++++++++++++--------------
1 files changed, 15 insertions(+), 14 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:38
Dmitry Ivankov wrote:
Currently parse-options.o pull quite a big bunch of dependencies
that are neither pulled in by svn-fe Makefile nor included in libgit.a.
Use a temporary hack: put hardcoded EXTLIBS
this may not work in all
setups because /Makefile logic is not repeated.
For example, one may need -lcrypto instead of -lssl or no crypto library
if BLK_SHA1 is set, also an additional -lz or -lpcre could be required.
Better to pull in too many libs and let the operator remove them from
the Makefile than too few and make her guess. Though of course
neither should be needed. :)
With the following applied on top locally, it works for me.
-- >8 --
Subject: squash! svn-fe: add EXTLIBS needed for parse-options
-lcrypto is needed for SHA-1 routines unless NO_OPENSSL or BLK_SHA1
is set, -lpcre is for grep if USE_LIBPCRE is set, and -lz is needed
throughout.
In the future, none of these should be needed, after a little
rearranging to ensure that parse-options.o has no references to
translation units that need to access the object db.
Signed-off-by: Jonathan Nieder <redacted>
---
contrib/svn-fe/Makefile | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
+static struct option svn_fe_options[] = {
+ OPT_STRING(0, "git-svn-id-url", &url, "url",
+ "append git-svn metadata line to commit messages"),
Hmm. How about this?
"add git-svn-id line to log messages, imitating git-svn"
[...]
+ argc = parse_options(argc, argv, NULL, svn_fe_options,
+ svn_fe_usage, 0);
+ if (argc == 1) {
+ if (url)
+ usage_msg_opt("git-svn-id-url is set twice: as a "
+ "--parameter and as a [parameter]",
+ svn_fe_usage, svn_fe_options);
+ url = argv[0];
+ } else if (argc)
+ usage_with_options(svn_fe_usage, svn_fe_options);
IMHO would be more readable with the simplest and exceptional case
first:
if (argc > 1)
usage_with_options(...);
This way, a person reading can be comforted with the knowledge that
argc <= 1 from then on.
if (argc == 1) {
if (url)
...
}
To sum up, the patch looks good, and the only improvements I can think
of are tiny nits. :)
With whatever changes mentioned above seem suitable,
Reviewed-by: Jonathan Nieder <redacted>
Thanks.
---
contrib/svn-fe/Makefile | 4 ++--
contrib/svn-fe/svn-fe.c | 8 +++++---
2 files changed, 7 insertions(+), 5 deletions(-)
@@ -16,7 +16,7 @@ static const char *url;staticstructoptionsvn_fe_options[]={OPT_STRING(0,"git-svn-id-url",&url,"url",-"append git-svn metadata line to commit messages"),+"add git-svn-id line to log messages, imitating git-svn"),OPT_END()};
@@ -24,14 +24,16 @@ int main(int argc, const char **argv){argc=parse_options(argc,argv,NULL,svn_fe_options,svn_fe_usage,0);+if(argc>1)+usage_with_options(svn_fe_usage,svn_fe_options);+if(argc==1){if(url)usage_msg_opt("git-svn-id-url is set twice: as a ""--parameter and as a [parameter]",svn_fe_usage,svn_fe_options);url=argv[0];-}elseif(argc)-usage_with_options(svn_fe_usage,svn_fe_options);+}if(svndump_init(NULL))return1;svndump_read(url);
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:38
Dmitry Ivankov wrote:
svndump_read takes a url parameter that is used in git-svn-id: lines
[...]
Move url parameter to svndump_init so that reset_dump_ctx is done
once per dump and in the same place as other resets. Wrap all _init
[...]
I'm getting lost seeing the forest for the trees, so let me try to
summarize.
Before:
if (svndump_init(url))
die("svndump_init failed");
svndump_read(dumpfile);
After:
struct svndump_args opts;
memset(&opts, 0, sizeof(opts));
opts.url = url;
opts.filename = dumpfile;
if (svndump_init(&opts))
die("svndump_init failed");
svndump_read();
Using an options struct instead of a list of arguments means each
option is optional and has a descriptive name mentioned at the call
site, and means it is easy to add new arguments in the future.
The patch still keeps the init/read distinction even though we don't
need it anywhere (i.e., all call sites look the same) to minimize its
invasiveness.
Do I understand correctly?
If so, it sounds like a good idea, and I have only minor nitpicks:
- It's tempting to call the struct svndump_options, by analogy
with struct merge_options from merge-recursive.h.
- Now that we're making the name of the "url" argument part of the
public API, maybe we should emphasize that the url is only for
show and git will never try to contact it. Maybe something like
"metadata_url"? (Sorry, I'm not so great at coming up with names.)
- Likewise, the "filename" argument could be made more
self-explanatory. Maybe "dumpfile"?
- Now that the filename argument is passed at init time instead of
read time, there is some uncertainty about when the file is going
to be opened. A comment could help, or merging the two functions
could help. :)
Thanks, and hope that helps.
Jonathan
---
contrib/svn-fe/svn-fe.c | 4 ++--
test-svn-fe.c | 4 ++--
vcs-svn/svndump.c | 10 ++++++----
vcs-svn/svndump.h | 7 ++++---
4 files changed, 14 insertions(+), 11 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:38
Dmitry Ivankov wrote:
fast_export.c had logic to set up commit ref, author name, email,
parent commit, import mark and git-svn-id: line based on both it's
own state (current import batch history) and the arguments passed.
Lift the decision on these parameters to the caller.
Again I find myself getting lost. I think this is another internal
API change, with the intent being to make the fast_export lib more
intuitive by making it focus on communicating with fast-import and the
delta applier instead of taking care of so much svn-fe-specific logic.
In other words, the idea would be to avoid a few layering violations.
Is that right?
If so:
This change leaves fast_export_end_commit empty. Why not remove
it? (Later patches that want to insert code there could reintroduce
the function.)
[...]
This doesn't compile for me (missing "ref" argument).
}
static void end_revision(void)
{
- if (rev_ctx.revision)
+ if (rev_ctx.revision) {
fast_export_end_commit(rev_ctx.revision);
+ printf("progress Imported commit %"PRIu32".\n\n", rev_ctx.revision);
Until now, svndump.c did not have to know about the fast-import
format (e.g., the existence of a "progress" command). Is that
worth changing?
Quick sketch with suggestions. What do you think?
-- >8 --
Subject: squash! vcs-svn: move commit parameters logic to svndump.c
The previous commit doesn't build because we forgot to pass the new
ref name argument to the fast_export API. While fixing that, simplify
fast_export_begin_commit to be more intuitive by using a set of
parameters closer to what gets written to fast-import.
The actual impact of this patch would be to run a little slower, since
we needlessly copy the author name into a temporary buffer for an
email address. That is a small per-commit rather than per-path cost
so the loss in speed might be worth the gain in readability.
Signed-off-by: Jonathan Nieder <redacted>
---
vcs-svn/fast_export.c | 35 ++++++++++++++++-------------------
vcs-svn/fast_export.h | 10 ++++------
vcs-svn/svndump.c | 41 ++++++++++++++++++++++++-----------------
3 files changed, 44 insertions(+), 42 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:38
Dmitry Ivankov wrote:
svn-fe produces fast-import stream for a fixed refs/heads/master ref.
It is usually desired to write to a different ref. In a remote helper
it would be a ref in private namespace. If svn-fe is used by someone
directly it'll be more safe to remind where the commits can go. And
in both cases it may be needed to import from two repos and hence to
different refs.
Add a destination ref parameter to vcs-svn/, a corresponding parameter
to svn-fe and a simple test for it.
$ svn-fe --ref=refs/heads/master ...
is an explicit way to stay with the default destination.
This improvement is very welcome!
Another reason to avoid fetching straight to "master" is that changing
HEAD from under the user's feet can be confusing. See [*] for some
explanation.
@@ -17,11 +17,14 @@ static struct svndump_args args;staticstructoptionsvn_fe_options[]={OPT_STRING(0,"git-svn-id-url",&args.url,"url","append git-svn metadata line to commit messages"),+OPT_STRING(0,"ref",&args.ref,"dst_ref",+"write to dst_ref instead of refs/heads/master"),
A small nit: such an underscored identifier would be typical for a
variable name in code but less so for a user-visible syntactic
placeholder. One possibility for avoiding that (inspired "git
update-ref") is:
OPT_STRING(0, "ref", &args.ref, "refname",
"write to <refname> instead of refs/heads/master"),
which shows up in "svn-fe -h" output as
--ref <refname> write to <refname> instead of refs/heads/master
Would it make sense to avoid having to pre-initialize ref by
interpreting NULL as refs/heads/master?
My secret goal in asking that is to find some way to avoid the
git-specific idiom of the refs/heads/master ref in fast-import
frontends of the future, by coming up with some improvement to the
syntax some day (like "commit default").
@@ -32,6 +32,9 @@ OPTIONS Url to be used in git-svn-id: lines in git-svn metadata lines format. See NOTES for more detailed description.+--ref=<dst_ref>::+ Ref to be written by the generated stream.+ Default is refs/heads/master.
Style: usually there is a blank line between items in definition list
markup. Options in manpages (unlike commands, files, and
configuration items) are usually described in the imperative mood, as
a command you give to the program. Like so:
--ref=<refname>::
Make the ref <refname> point to the tip of the history
imported so far, instead of writing to
refs/heads/master. This can be useful when importing
into a non-bare repository and the "master" branch is
checked out.
[...]
Is this new function needed?. We could let the $args argument include
$input, like so:
try_dump an-interesting-dump.dump
try_dump "--in-a-special-way another-interesting-dump.dump"
Another alternative would be to treat arguments starting with a minus
sign specially, so the call sites could look natural:
try_dump --in-a-special-way another-interesting-dump.dump
though that seems more fragile (e.g., if I want to test "svn-fe --ref
detached-argument").
No need for the minus sign and &&, since this is unindented and just
supplying test data rather than part of a chain of test assertions
that can fail. Putting a \ before the EOF can be a friendly touch to
save reviewers the trouble of looking for shell metacharacters in the
here document's body.
[...]
Nice and simple. Maybe it would be logical to put it earlier in the
file (for three reasons: on one hand to make the file easier to read
straight through; on the other hand to make other patches adding tests
less likely to conflict with this one; and to keep the slow test that
uses svnadmin at the end of the test script, so results from simpler
ones like this come sooner).
quoted hunk
--- a/test-svn-fe.c+++ b/test-svn-fe.c
[...]
quoted hunk
@@ -20,6 +20,8 @@ static int d; static struct option test_svnfe_options[] = { OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),+ OPT_STRING(0, "ref", &args.ref, "dst_ref",+ "write to dst_ref instead of refs/heads/master"),
This means the svndump module keeps its own copy of the refname
string, saving the caller the trouble of keeping it alive. Probably
sensible, especially because there could be code between
svndump_init() and svndump_read() that actually uses it (for example,
if the refname is taken from an envvar that getenv() clobbers).
Hm. fast_export_begin_commit acts on a ref in a sense. That could
mean the argument should be the first or last one, depending on
whether we are imitating fprintf() or fwrite().
For reference, here's what I tested locally (as a full patch instead
of incremental because patches it's based on changed enough that the
original did not apply cleanly).
Thanks.
[*] https://git.wiki.kernel.org/index.php/GitFaq#How_would_I_use_.22git_push.22_to_sync_out_of_a_host_that_I_cannot_pull_from.3F
-- >8 --
From: Dmitry Ivankov <redacted>
Date: Wed, 13 Jul 2011 18:21:09 +0600
Subject: vcs-svn,svn-fe: allow to specify dump destination ref
svn-fe produces fast-import stream for a fixed refs/heads/master ref.
It is usually desired to write to a different ref. In a remote helper
it would be a ref in private namespace. If svn-fe is used by someone
directly it'll be more safe to remind where the commits can go. And
in both cases it may be needed to import from two repos and hence to
different refs.
Add a destination ref parameter to vcs-svn/, a corresponding parameter
to svn-fe and a simple test for it.
$ svn-fe --ref=refs/heads/master ...
is an explicit way to stay with the default destination.
[jn: use NULL for default, clarify documentation, reorder
reset_dump_ctx arguments, last fast_export module specify default,
pass by strbuf so signature changes (API break), test simplifications]
Signed-off-by: Dmitry Ivankov <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
contrib/svn-fe/svn-fe.c | 2 ++
contrib/svn-fe/svn-fe.txt | 7 +++++++
t/t9010-svn-fe.sh | 42 +++++++++++++++++++++++++-----------------
test-svn-fe.c | 6 +++---
vcs-svn/fast_export.c | 9 +++++----
vcs-svn/fast_export.h | 5 ++++-
vcs-svn/svndump.c | 14 ++++++++++----
vcs-svn/svndump.h | 1 +
8 files changed, 57 insertions(+), 29 deletions(-)
@@ -17,6 +17,8 @@ static struct svndump_options args;staticstructoptionsvn_fe_options[]={OPT_STRING(0,"git-svn-id-url",&args.metadata_url,"url","add git-svn-id line to log messages, imitating git-svn"),+OPT_STRING(0,"ref",&args.ref,"refname",+"write to <refname> instead of refs/heads/master"),OPT_END()};
@@ -33,6 +33,13 @@ OPTIONS metadata lines format. See NOTES for more detailed description.+--ref=<refname>::+ Make the ref <refname> point to the tip of the history+ imported so far, instead of writing to+ `refs/heads/master`. This can be useful when importing+ into a non-bare repository and the "master" branch is+ checked out.+ INPUT FORMAT ------------ Subversion's repository dump format is documented in full in
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:39
Dmitry Ivankov wrote:
svn-fe needs to read fast-import's responses to "ls" and "cat-blob".
These come through a file descriptor number 3.
Sometimes it is easier to setup variable fd than a fixed one. It is
the case with pipe() call and even more fd=3 can be already taken.
On Windows file descriptors are not by default inherited by a child
process, nor there is an option to setup descriptors other than
standard stdin, stdout, stderr at a process creation time.
Add an option for this file descriptor number in vcs-svn/ and svn-fe,
add a simple test for it.
To be used like following:
$ svn-fe --read-blob-fd=7 ... 7<somewhere
Thanks. The above description covers the basics but I think it is out
of order. Maybe it would make sense to say:
. first, that Windows lacks fork() and has facilities to redirect
stdin, stdout, and stderr and to inherit others in a child process
but nothing more (by the way, does anyone on list know if this is
true?)
. second, that this patch should help to work around that by allowing
the caller to tell what file descriptor number the reading end of
the "feature cat-blob" pipe inherited
. third, that being able to specify the fd number is more convenient
anyway
. lastly, that the option is plumbed into both test-svn-fe and
contrib's svn-fe tool, and what usage looks like
That way, the motivation comes first.
It is also possible to motivate it by that third point instead
(hard-coded fds as part of a command's interface do not scale and are
just weird), so I'd be tempted to leave out the Windows stuff I am
uncertain about if I were writing it. :)
@@ -19,12 +19,15 @@ static struct option svn_fe_options[] = {"append git-svn metadata line to commit messages"),OPT_STRING(0,"ref",&args.ref,"dst_ref","write to dst_ref instead of refs/heads/master"),+OPT_INTEGER(0,"read-blob-fd",&args.backflow_fd,+"read blobs and trees from this fd instead of 3"),
From the operator's point of view, I think this is just the other end
of the pipe that fast-import --cat-blob-fd writes to. Maybe
"read fast-import replies from file descriptor <n> (default: 3)"
[...]
@@ -35,6 +35,10 @@ OPTIONS --ref=<dst_ref>:: Ref to be written by the generated stream. Default is refs/heads/master.+--read-blob-fd=<fd>::+ Integer number of file descriptor from which+ responses to 'ls' and 'cat-blob' requests will come.+ Default is fd=3.
If try_dump_ext from the previous patch gets removed, it would ripple
through here, too. Demonstration of one possible approach below.
[...]
+test_expect_success PIPE 'use different backflow fd' '
+ reinit_git &&
+ echo hi >hi &&
+ {
+ properties \
+ svn:author author@example.com \
+ svn:date "1999-02-01T00:01:002.000000Z" \
+ svn:log "add directory with some files in it" &&
Is this dump copy/pasted from another test? Would it be possible to
simplify or share the dumpfile?
Some but not all of the suggestions above implemented below (this is
just an example; if something looks crazy, please feel free to drop or
fix it, of course).
Sorry to take so long to look this over. In broad strokes your
patches carry out very good changes.
-- >8 --
From: Dmitry Ivankov <redacted>
svn-fe needs to read fast-import's responses to "ls" and "cat-blob".
These come through a file descriptor number 3.
Sometimes it is easier to setup variable fd than a fixed one. It is
the case with pipe() call and even more fd=3 can be already taken.
On Windows file descriptors are not by default inherited by a child
process, nor there is an option to setup descriptors other than
standard stdin, stdout, stderr at a process creation time.
Add an option for this file descriptor number in vcs-svn/ and svn-fe,
add a simple test for it.
To be used like following:
$ svn-fe --read-blob-fd=7 ... 7<somewhere
[jn: various style tweaks]
Signed-off-by: Dmitry Ivankov <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
contrib/svn-fe/svn-fe.c | 4 ++-
contrib/svn-fe/svn-fe.txt | 8 +++++-
t/t9010-svn-fe.sh | 54 +++++++++++++++++++++++++++++++++++++++++++-
test-svn-fe.c | 6 ++--
vcs-svn/fast_export.c | 2 +
vcs-svn/svndump.c | 4 +--
vcs-svn/svndump.h | 3 ++
7 files changed, 70 insertions(+), 11 deletions(-)
@@ -33,6 +33,10 @@ OPTIONS metadata lines format. See NOTES for more detailed description.+--read-blob-fd=<fd>::+ Read responses to 'ls' and 'cat-blob' requests from+ this file descriptor. The default is 3.+ INPUT FORMAT ------------ Subversion's repository dump format is documented in full in
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:39
Dmitry Ivankov wrote:
This option is to make svn-fe write commits on top of the existing ref
instead of overwriting it. More precise, the first commit's parent is
set to be :(first_revision_in_current_dump - 1) mark.
Prerequisite is to (re)use import marks (from previous imports). It is
safe to use this option on a svn dump that starts with r0/r1. The svn
dump itself should be incremental too.
In other words, this allows running svn-fe to resume a partial import or
to resume an import after the remote repository has added more history.
Hoorah!
[...]
It should be possible to avoid this extra argument by making "reinit_git"
remove the marks file and using something like
git fast-import --import-marks-if-exists=marks --export-marks=marks
quoted hunk
--- a/test-svn-fe.c+++ b/test-svn-fe.c
@@ -22,6 +22,9 @@ static struct option test_svnfe_options[] = {OPT_SET_INT('d',NULL,&d,"test apply_delta",1),OPT_STRING(0,"ref",&args.ref,"dst_ref","write to dst_ref instead of refs/heads/master"),+OPT_BIT(0,"incremental",&args.incremental,+"resume export, requires marks and incremental dump",+1),
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:39
Hi,
Dmitry Ivankov wrote:
There are already a few options to determine svn revision from which
a git commit imported with svn-fe came from. One is to make svn-fe
write a git-svn-id: line to commit messages. Another one is to calc
distance to the root commit. The former includes a "url" and is for
git-svn compatibility, the latter is obviously slow and a bit fragile.
$ svn-fe --notes_ref=notes_tree --ref=branch...
will write annotations for branch commits to the notes_tree, each
annotation is a simple "rN" string. Then these annotations can be
viewed manually or used in incremental import to detect the last
imported revision or to (re)create the import marks for further
imports.
Wouldn't another way be to look at the mark numbers?
I am not sure I like this. svn-fe is supposed to be a generally
useful tool, and this patch hard-codes the particular note format rN.
If it is needed, maybe it would be possible to do something like
--notes-ref=refs/notes/svn-rev --note='project foo, r%N'
As a bonus, that would allow including more information using
different flag characters in the note in the future.
Sorry for a way too slow response. This patch is probably the most
unbaked one, so I'll start here.
On Tue, Jul 26, 2011 at 3:39 AM, Jonathan Nieder [off-list ref] wrote:
Hi,
Dmitry Ivankov wrote:
quoted
There are already a few options to determine svn revision from which
a git commit imported with svn-fe came from. One is to make svn-fe
write a git-svn-id: line to commit messages. Another one is to calc
distance to the root commit. The former includes a "url" and is for
git-svn compatibility, the latter is obviously slow and a bit fragile.
$ svn-fe --notes_ref=notes_tree --ref=branch...
will write annotations for branch commits to the notes_tree, each
annotation is a simple "rN" string. Then these annotations can be
viewed manually or used in incremental import to detect the last
imported revision or to (re)create the import marks for further
imports.
Wouldn't another way be to look at the mark numbers?
If marks file is absent (after clone for example), we'll need to look
at notes anyway.
If it is present and has a mark for latest revision we can hope it is
valid and not
regenerate it.
If for some reason notes are lost and marks are present, it's possible
to recreate
notes from marks though, at least "rN" ones.
I am not sure I like this. svn-fe is supposed to be a generally
useful tool, and this patch hard-codes the particular note format rN.
If it is needed, maybe it would be possible to do something like
--notes-ref=refs/notes/svn-rev --note='project foo, r%N'
As a bonus, that would allow including more information using
different flag characters in the note in the future.
Format string looks nice. While the whole notes thing may need
more thinking.
The main reason I wrote these in svn-fe is that it's more atomic
to write note just after writing a commit. Also "checkpoint" will
create notes for current status (and will write marks too, but weren't
we going to consider notes as a primary data copy? it can be cloned
for example).
One more consideration is that copy-from information most likely
will be written by svn-fe (nothing else knows it anyway) to some
notes, so we'll need some notes writing in svn-fe.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:40
Dmitry Ivankov wrote:
On Tue, Jul 26, 2011 at 3:39 AM, Jonathan Nieder [off-list ref] wrote:
quoted
--notes-ref=refs/notes/svn-rev --note='project foo, r%N'
As a bonus, that would allow including more information using
different flag characters in the note in the future.
Format string looks nice.
Yes, it sounds pleasant to work with to me. Open questions:
- how fine-grained should the notes commits be? E.g., should they
be written one at a time, in batches of 10, or something else?
- is there a way to make some commits get no note at all? Is this
template-based approach the right way to go? (I guess yes, it is.)
- what if someone wants multiple notes refs (e.g., revision numbers
and revprops in separate notes refs)? Will this support that? If
not, is it extensible enough to sensibly support that in the
future?
[...]
One more consideration is that copy-from information most likely
will be written by svn-fe (nothing else knows it anyway) to some
notes, so we'll need some notes writing in svn-fe.
Yep, copyfrom info has to get downstream somehow. Which means
a. in log messages
b. in notes
c. in comments or "progress" lines in the stream, or
or d. in a second output stream, with file descriptor number specified
by the caller.
I was leaning towards (a) or (c), but (b) certainly also seems
sensible.