Ivan Zhakov wrote:
> On 20 June 2014 18:38, Julian Foad <julianfoad_at_btopenworld.com> wrote:
>> 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?
Oh, I didn't notice we documented that some common fields will always be set. In that case, to make the reader aware that we have explicitly documented this case, we could add a note that "Just the common fields are set."
>> 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/foohttp://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/foohttp://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/foohttp://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/foohttp://example.org/svn/bar
> Adding http://example.org/svn/foo
> Deleting http://example.org/svn/bar
> Committing transaction...
> Committed revision 1.
> ]]]
That would make this kind of commit consistent with "svn commit", which is logical...
> The current output is just:
> [[[
> $ svn move http://example.org/svn/foohttp://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?
... and I like logical.
Why would we *not* want the notifications to be consistent between "svn commit" and other "svn" subcommands that also make a commit? I see no reason. A consistent style of notifications would be better than the current ad-hoc variations.
Can the implementation use the same code-path for all kinds of commit?
- Julian
Received on 2014-06-23 10:59:24 CEST