Re: [PATCH 8/8] vcs-svn: allow to disable 'progress' lines
- From
Dmitry Ivankov <divanorama@gmail.com>
- Date
- Jul 6, 2011, 10:14 UTC
- Message-ID
- <CA+gfSn-LEW36+bbxs1ydnhd3iQu-+wvrsSTuTszTYKm+jmuhOw@mail.gmail.com>
- In-Reply-To
- <CALkWK0nWSVrrtzwE2ePucK=XTnSHWuojxhoayRSUbZmk1We9Fg@mail.gmail.com>
Hi!
[...]
>> diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
[...]
Show 6 quoted lines
>> - 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.
Show 15 quoted lines
>> 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.
[...]
Show 9 quoted lines
>> 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;
[...]
Show 8 quoted lines
>> -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.
Show 7 quoted lines
> 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 >