Re: Improving svn commit progress notification

On 20 June 2014 18:38, Julian Foad <julianfoad_at_btopenworld.com> wrote:
>
> Ivan Zhakov wrote:
> >> [[[
> >> $ svn ci wc -m "log msg"
> >> Sending wc\foo
> >> Transmitting file data ............done
> >> Committing transaction...
> >> Committed revision 5.
> >> ]]]
> >>
> >> Also consider the out-of-date case:
> >
> > Committed in r1603388 and r1604179. I'm going to implement showing
> > percentage of deltas transmitted so far later.
>
> I like this, basically.
>
> A few small concerns...
>
> Index: subversion/include/svn_wc.h
> + /** Finalizing commit.
> + * @since New in 1.9. */
> + svn_wc_notify_commit_finalizing
> } svn_wc_notify_action_t;
>
> This is a public API. We should document what other fields the notification receiver will receive: in this case, it's the "url" field.
>
> I know we haven't done that in other cases, but we should.
>
It's already documented that every svn_wc_notify_t structure instance
must have PATH field initialized and URL field should be also
initialized if operation performed on URL. I agree that it would nice
to document svn_wc_notify_t fields for different action types, I just
didn't find good way to document other structure values in enum
declaration. Do you have any ideas?

> Index: subversion/svn/notify.c
> + case svn_wc_notify_commit_finalizing:
> + if (nb->sent_first_txdelta)
> + {
> + SVN_ERR(svn_cmdline_printf(pool, _("done\n")));
>
> I understand we don't want to print "done" if we did not print "Sending.......",
>
> + SVN_ERR(svn_cmdline_printf(pool, _("Committing transaction...")));
>
> but I think it is inconsistent to not print "Committing transaction..." for a commit that only
> performed tree changes. Part of your rationale for the change was to let the user know
> that all of the relevant changes have been transmitted and now we are waiting for the
> server to finalize the txn. This rationale applies equally well for a commit that did not include text deltas.
>
> Certain operations such as "svn propset p v URL" can only perform one change (one prop change in this case).
> With a one-change operation, we could consider that this extra line is ugly -- it's just noise really -- one line
> of notification is enough. If that was your reasoning, we should implement that.
>
Good point. I forgot about commits with tree only changes.

The problem that currently Subversion command line doesn't print
progress notifications for URL operations. For example:
[[
$ svn mkdir http://example.org/svn/foo http://example.org/svn/bar

Committed revision 1.
]]

So if Subversion will "Committing transaction" progress for tree only
commits output will be like this:
[[
$ svn mkdir http://example.org/svn/foo http://example.org/svn/bar
Committing transaction...
Committed revision 1.
]]

Which is probably fine.

Another option to always print detailed progress notifiction for every
kind of operation that changes repository. I mean:
[[
$ svn mkdir http://example.org/svn/foo http://example.org/svn/bar
Adding http://example.org/svn/foo
Adding http://example.org/svn/bar
Committing transaction...
Committed revision 1.
]]

And in more interesting svn move case:
[[[
$ svn move http://example.org/svn/foo http://example.org/svn/bar
Adding http://example.org/svn/foo
Deleting http://example.org/svn/bar
Committing transaction...
Committed revision 1.
]]]

The current output is just:
[[[
$ svn move http://example.org/svn/foo http://example.org/svn/bar
Committed revision 1.
]]]

Which is inconsistent with WC to WC moves:
[[[
$ svn move foo bar
A foo
D bar
]]]

What do you think?

> + }
>
> (nit: Wonky indentation on that closing brace.)
>
Fixed in r1604334. Thanks!

-- 
Ivan Zhakov