Re: [PATCH 8/8] vcs-svn: allow to disable 'progress' lines

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi again,

Dmitry Ivankov writes:
>>> -       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.

Well, you'll be wrapping up url, ref, backflow_fd, progress, and all
other future command-line parameters in one variable.  Think about it.
 You can use the parse-options API to fill in default arguments
anyway.

>>> 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).

Ah, right. I agree with you.

>>  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.

Yeah, that makes sense -- would love to see that in future.

>>> -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.

Makes sense.
Thanks.

-- Ram