subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ivan Zhakov <i...@visualsvn.com>
Subject Re: Improving svn commit progress notification
Date Sat, 21 Jun 2014 11:16:27 GMT
On 20 June 2014 18:38, Julian Foad <julianfoad@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

Mime
View raw message