subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Foad <julianf...@btopenworld.com>
Subject Re: Improving svn commit progress notification
Date Fri, 20 Jun 2014 16:38:23 GMT
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.

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.

+      }

(nit: Wonky indentation on that closing brace.)

- Julian

Mime
View raw message