Hi!
[...]
>> diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
[...]
>> - if (svndump_init(NULL, url, ref, backflow_fd))
>> + if (svndump_init(NULL, url, ref, backflow_fd, progress))
>
> You're modifying the svndump_init API for every new option that's
> added. This'll clearly break down when you have many options -- how
> about wrapping it up in an options structure and then passing that?
Well, there has to be a function to init that structure then. And the
structure will become a part of API.
So don't know if it's worthy.
>> diff --git a/contrib/svn-fe/svn-fe.txt b/contrib/svn-fe/svn-fe.txt
>> index a7ad368..f1a459e 100644
>> --- a/contrib/svn-fe/svn-fe.txt
>> +++ b/contrib/svn-fe/svn-fe.txt
>> @@ -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.
>
> Hm, this will make it a little too silent for the end-user. What do
> you feel about printing something minimalistic like a '.' for each
> imported revision? Atleast it won't look like it's hung.
For a medium 8k commit repo it is 100 lines - still too much.
A single line for the first revision seem harmless and will indicate
that the remote connection succeeded
(helps to see that it's not a connection timeout, probably caused by
dns lookup or a firewall).
> Also, how does this interact with the 'progress' option of fast-import protocol?
git fast-import --quiet prints any progress line produced by a helper.
transport-helper.c tries to set the option
but doesn't fail if it is not accepted or if helper doesn't support
options at all.
For now the helper doesn't use this protocol option.
A better solution could be to use progress.o api to display progress.
Or an ad-hoc hack with adaptive progress step, say report a progress
on each "power of two"-th revision.
As a starting point for tests let there bust a simple switch-off.
[...]
>> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
>> index cfb0f2b..a8b8603 100644
>> --- a/vcs-svn/fast_export.c
>> +++ b/vcs-svn/fast_export.c
>> @@ -19,6 +19,7 @@ static uint32_t first_commit_done;
>> static struct line_buffer postimage = LINE_BUFFER_INIT;
>> static struct line_buffer report_buffer = LINE_BUFFER_INIT;
>> static struct strbuf ref_name = STRBUF_INIT;
>> +static int print_progress;
[...]
>> -void fast_export_init(int fd, const char *dst_ref)
>> +void fast_export_init(int fd, const char *dst_ref, int progress)
>> {
>> first_commit_done = 0;
>> + print_progress = progress;
>
> The only reason you're modifying the API of fast_export_init is so
> that it can set a global static variable?
Looked once more at how these new variables are used. We can move
progress lines generation to svndump.c. And also move ref_name from
_init and being global static to a parameter in fast_export_begin_commit(),
and to be more sane s/revision/target_mark/ s/revision - 1/from_mark/ and
add a from_mark parameter, move first_commit_done logic to svndump.o.
This way fast_export.o can operate on single commits and maybe it'll be
easier to use it to apply svn branches layout in svn-fe in one run, though I'm
not sure I'll use svn-fe to manage svn branches.
> Also, this change seems
> more absurd because progress reporting isn't directly related to
> fast_export_init. How about having a dedicated function for option
> parsing that sets all the global statics?
>
> I'm sorry I haven't been more involved with this project. Still, I
> hope this review helps.
This is very helpful, thanks.
>
> -- Ram
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html