[PATCH] Fix for temporarily accepting ssl certificate not working in javahl

[Doros Agathangelou]

Hi All

I discovered a small bug in javahl and I believe I have a one line patch
that fixes it.

When connecting to a server via https and a self signed certificate is used
on the server, we get a callback
on UserPasswordCallback.askTrustSSLServer() method that asks the user
whether to reject the certificate, accept it temporarily or accept it
permanently.

If we choose temporarily, whatever operation we were attempting fails with
a certificate not trusted error. The bug can be seen in a live environment
by using the subclipse plugin under Eclipse and trying to load a repository
using https:// from a server with a self signed certificate. When the
accept certificate dialog comes up, hit accept temporarily and you can see
that the operation fails because the certificate is not trusted.

As for the fix:
The following excerpt from Prompter.cpp shows the cred->accepted_failures =
failures; line. This is the line that 'accepts' any identified errors.

This line can be seen under the AcceptPermanently section but it is missing
under the AcceptTemporary section. Adding the line to the AcceptTemporary
section fixes this problem. The difference between the two blocks of code
then becomes whether the credentials may be saved or not (i.e. the
cred->may_save differs)


 switch (authn.ask_trust_ssl_server(::Java::String(env, question),
may_save))
{
case
org_apache_subversion_javahl_callback_UserPasswordCallback_AcceptTemporary:
  cred->may_save = FALSE;
 cred->accepted_failures = failures;   // ** NEW LINE I ADDED **
  *cred_p = cred;
  break;
case
org_apache_subversion_javahl_callback_UserPasswordCallback_AcceptPermanently:
  cred->may_save = TRUE;
  cred->accepted_failures = failures;
  *cred_p = cred;
  break;
default:
  *cred_p = NULL;
}
  return SVN_NO_ERROR;


[[[
   Fix for temporarily accepting ssl certificate not working in javahl

   * subversion/bindings/javahl/native/Prompter.cpp
   (accept certificate temporarily): set the accepted failures to the
identified failures in the temporarily accepted section
]]]



Best Regards
Doros


Prompter.cpp.patch
Description: Binary data

[PATCH] Resolve issue #4647 on trunk (v3)

[Stefan]

On 10/13/2016 11:38 AM, Stefan wrote:
> On 10/13/2016 11:08 AM, Stefan wrote:
>> On 10/10/2016 11:39 PM, Stefan wrote:
>>> On 10/10/2016 6:12 PM, Ivan Zhakov wrote:
 On 10 October 2016 at 17:53, Stefan  wrote:
> On 8/28/2016 11:32 PM, Bert Huijben wrote:
>>> -Original Message-
>>> From: Daniel Shahaf [mailto:d...@daniel.shahaf.name]
>>> Sent: zondag 28 augustus 2016 20:23
>>> To: Stefan 
>>> Cc: dev@subversion.apache.org
>>> Subject: Re: [PATCH] Fix a conflict resolution issue related to binary 
>>> files (patch
>>> v4)
>>>
>>> Stefan wrote on Sun, Aug 28, 2016 at 13:31:39 +0200:
 The regression test was tested against 1.9.4, 1.9.x and trunk r1743999.

 I also tried to run the test against 1.8.16 but there it fails (didn't
 investigate in detail).
 Trunk r1758069 caused some build issues on my machine. Therefore I
 couldn't validate/check the patch against the latest trunk (maybe it's
 just some local issue with my build machine rather than some actual
 problem on trunk - didn't look into that yet).
>>> For future reference, you might have tried building trunk@HEAD after
>>> locally reverting r1758069; i.e.:
>>>
>>> svn up
>>> svn merge -c -r1758069
>>> 
>>> make check
>>>
>>> Stefan wrote on Sun, Aug 28, 2016 at 18:33:55 +0200:
 Got approved by Bert.

>>> (Thanks for stating so on the thread.)
>>>
 Separated the repro test from the actual fix in order to have the
 possibility to selectively only backport the regression test to the 1.8
 branch.
>>> Good call, but the fix and the "remove XFail markers" (r1758129 and
>>> r1758130) should have been done in a single revision: they _are_
>>> a single logical change.  That would also avoid breaking 'make check'
>>> (at r1758129 'make check' exits non-zero because of the XPASS).
>> I do this the same way sometimes, when I want to use the separate 
>> revision for backporting... But usually I commit things close enough 
>> that nobody notices the bot results ;-)
>> (While the initial XFail addition is still running, you can commit the 
>> two follow ups, and the buildbots collapses all the changes to a single 
>> build)
>>
>> I just committed the followup patch posted in another thread to unbreak 
>> the bots for the night...
>>
>>   Bert
> Attached is a patch which should resolve the test case you added, Bert.
> Anybody feels like approving it? Or is there something I should
> improve/change?
>
> [[[
>
> Add support for the svn_client_conflict_option_working_text resolution for
> binary file conflicts.
>
> * subversion/libsvn_client/conflicts.c
>   (): Add svn_client_conflict_option_working_text to 
> binary_conflict_options
>
> * subversion/tests/cmdline/resolve_tests.py
>   (automatic_binary_conflict_resolution): Remove XFail marker
>
> ]]]
>
 It seems this patch breaks interactive conflict resolve:
 With trunk I get the following to 'svn resolve' on binary file:
 [[[
 Merge conflict discovered in binary file 'A_COPY\theta'.
 Select: (p) postpone,
 (r) accept binary file as it appears in the working copy,
 (tf) accept incoming version of binary file: h

   (p)  - skip this conflict and leave it unresolved  [postpone]
   (tf) - accept incoming version of binary file  [theirs-full]
   (r)  - accept binary file as it appears in the working copy  [working]
   (q)  - postpone all remaining conflicts
 ]]]

 But with patch I get the following:
 [[[
 Merge conflict discovered in binary file 'A_COPY\theta'.
 Select: (p) postpone,
 (r) accept binary file as it appears in the working copy,
 (tf) accept incoming version of binary file: h

   (p)  - skip this conflict and leave it unresolved  [postpone]
   (tf) - accept incoming version of binary file  [theirs-full]
   (mf) - accept binary file as it appears in the working copy  [mine-full]
   (r)  - accept binary file as it appears in the working copy  [working]
   (q)  - postpone all remaining conflicts
 ]]]

 I think it's confusing and we should not offer the same option twice.

>>> Completely agreed. The display of the option in the UI shouldn't be like
>>> that. Certainly an oversight on my side. Will revise the patch and come
>>> up with a different/better approach tomorrow.
>>>
>>> Regards,
>>> Stefan
>> Trying to put together a revised patch without the issue. The attached
>> patch fixes the 4647 test but breaks two other tests:
>>
>> basic#41
>> update#38
>>
>> Still looking into what I'm doing wrong, but any pointers would be much
>> appreciated.
> Looks like update#38 is 

Re: [PATCH v3] Conflict option labels

[Stefan]

On 10/13/2016 5:26 PM, Patrick Steinhardt wrote:
> sion re-adds the result pool to
> `svn_client_conflict_option_get_lazel`.

> diff --git a/subversion/include/svn_client.h
> b/subversion/include/svn_client.h
> index 9bbe62b..f456c92 100644
> --- a/subversion/include/svn_client.h
> +++ b/subversion/include/svn_client.h
> @@ -4718,6 +4718,20 @@ svn_client_conflict_option_id_t
>  svn_client_conflict_option_get_id(svn_client_conflict_option_t *option);
>  
> [...]
>  
>add_resolution_option(*options, conflict,
>  svn_client_conflict_option_merged_text,
> +_("Mark as resolved"),
>  _("accept binary file as it appears in the working copy"),
>  resolve_text_conflict);
Not sure whether "Mark as resolved" means much to the user. Maybe
"Accept/Use current version" would be easier to get? Or maybe
"Accept/Use current" to keep the label consistent with the style of the
others.
> [...]
>  
>add_resolution_option(*options, conflict,
>  svn_client_conflict_option_working_text,
> +_("Reject incoming"),
>  _("reject all incoming changes for this file"),
>  resolve_text_conflict);
IMO "Reject incoming" is the inversed way to describe what's being done.
Better would be: "Accept current"?
>  
> [...]
>  
>add_resolution_option(*options, conflict,
>  svn_client_conflict_option_working_text_where_conflicted,
> +_("Reject conflicts"),
>  _("reject changes which conflict and accept the rest"),
>  resolve_text_conflict);
=> "Accept incoming non-conflicting changes only."?
>  
>add_resolution_option(*options, conflict,
>  svn_client_conflict_option_merged_text,
> +_("Mark as resolved"),
>  _("accept the file as it appears in the working copy"),
>  resolve_text_conflict);
Same as above: "Accept/Use current"
>[...]
>  
>add_resolution_option(*options, conflict,
>  svn_client_conflict_option_working_text,
> +_("Mark as resolved"),
>  _("accept working copy version of entire property value"),
>  resolve_prop_conflict);
Same as above.
>  
>add_resolution_option(*options, conflict,
>  svn_client_conflict_option_incoming_text_where_conflicted,
> -N_("accept changes only where they conflict"),
> +_("Accept incoming for conflicts"),
> +_("accept incoming changes only where they conflict"),
>  resolve_prop_conflict);
The wording is a bit misleading IMO. It might be interpreted as
non-conflicting incoming changes being rejected rather than these being
merged implicitly (or am I wrong?).

Maybe better wording would be:
Label: Resolve conflicts with incoming changes
Desc: Accept incoming and resolve conflicts using the incoming changes.

>  
>add_resolution_option(*options, conflict,
>  svn_client_conflict_option_working_text_where_conflicted,
> +_("Reject conflicts"),
>  _("reject changes which conflict and accept the rest"),
>  resolve_prop_conflict);
Reject conflicts doesn't quite reflect what's being done. Maybe:

"Resolve conflicts with local changes."
"Accept incoming and resolve conflicts using the local changes."

> [...]
>add_resolution_option(options, conflict,
> 
> svn_client_conflict_option_accept_current_wc_state,
> +_("Mark as resolved"),
>  _("accept current working copy state"),
>  do_resolve_func);
See above.
>  
> @@ -7264,6 +7285,7 @@
> configure_option_update_move_destination(svn_client_conflict_t *conflict,
>add_resolution_option(
>  options, conflict,
>  svn_client_conflict_option_update_move_destination,
> +_("Update move destination"),
>  _("apply incoming changes to move destination"),
>  resolve_update_moved_away_node);
>  }
The label doesn't quite reflect what's being done (aka: how it's
updated). Maybe: "Apply changes to move destination"

> @@ -7298,6 +7320,7 @@ configure_option_update_raise_moved_away_children(
>add_resolution_option(
>  options, conflict,
>  svn_client_conflict_option_update_any_moved_away_children,
> +_("Update any moved-away children"),
>  _("prepare for updating moved-away children, if any"),
>  resolve_update_raise_moved_away);
>  }
Same as above.
> [...]
>  
>return SVN_NO_ERROR;
> @@ -7425,7 +7448,8 @@
> configure_option_incoming_added_file_text_merge(svn_client_conflict_t
> *conflict,
>add_resolution_option(
>  options, conflict,
>  svn_client_conflict_option_incoming_added_file_text_merge,
> -description, resolve_merge_incoming_added_file_text_merge);
> +_("Merge the files"), description,
> +resolve_merge_incoming_added_file_text_merge);
>  }
Maybe better: Merge incoming file with local.
>  
>return SVN_NO_ERROR;
> @@ -7481,6 +7505,7 @@
> configure_option_incoming_added_file_replace_and_merge(
>

[PATCH v3] Conflict option labels

[Patrick Steinhardt]

Hi,

the third version re-adds the result pool to
`svn_client_conflict_option_get_lazel`.

[[
Move conflict resolution options' labels out of the client

* subversion/include/svn_client.h:
  (svn_client_conflict_option_get_label): New function.
* subversion/libsvn_client/conflicts.c:
  (svn_client_conflict_option_t): Add label.
  (add_resolution_option): Add label argument.
  (svn_client_conflict_option_get_label): New function.
  (svn_client_conflict_text_get_reslution_options,
   svn_client_conflict_prop_get_resolution_options,
   configure_option_accept_current_wc_state,
   configure_option_move_destination,
   configure_option_update_raise_moved_away_children,
   configure_option_incoming_add_ignore,
   configure_option_incoming_added_file_text_merge,
   configure_option_incoming_added_file_replace_and_merge,
   configure_option_incoming_added_dir_merge,
   configure_option_incoming_added_dir_replace,
   configure_option_incoming_added_dir_replace_and_merge,
   configure_option_incoming_delete_ignore,
   configure_option_incoming_delete_accept,
   configure_option_incoming_move_file_merge,
   configure_option_incoming_dir_merge,
   svn_client_conflict_tree_get_resolution_options): Set
   resolution option labels.
* subversion/svn/conflict-callbacks.c:
  (resolver_option_t): Remove short_desc and long_desc.
  (client_option_t): New struct for client options.
  (builtin_resolver_options): Remove short_desc and long_desc.
  (extra_resolver_options,
   extra_resolver_options_text,
   extra_resolver_options_prop,
   extra_resolver_options_tree): Convert to client_option_t.
  (find_option): Accept options as apr_array_header_t.
  (find_option_by_builtin): New function to create provided
  options from builtin library options.
  (find_option_by_id): Replaced by find_option_by_builtin.
  (prompt_string,
   help_string,
   prompt_user,
   build_text_conflict_options,
   build_prop_conflict_options,
   build_prop_text_conflict_options,
   handle_one_prop_conflict.
   build_tree_conflict_options,
   handle_tree_conflict): Accept options as apr_array_header_t.
]]

Regards
Patick
diff --git a/subversion/include/svn_client.h b/subversion/include/svn_client.h
index 9bbe62b..f456c92 100644
--- a/subversion/include/svn_client.h
+++ b/subversion/include/svn_client.h
@@ -4718,6 +4718,20 @@ svn_client_conflict_option_id_t
 svn_client_conflict_option_get_id(svn_client_conflict_option_t *option);
 
 /**
+ * Return a textual human-readable label of @a option, allocated in
+ * @a result_pool. The label is encoded in UTF-8 and usually
+ * contains up to three words.
+ *
+ * Additionally, the label may be localized to the language used
+ * by the current locale.
+ *
+ * @since New in 1.10.
+ */
+const char *
+svn_client_conflict_option_get_label(svn_client_conflict_option_t *option,
+ apr_pool_t *result_pool);
+
+/**
  * Return a textual human-readable description of @a option, allocated in
  * @a result_pool. The description is encoded in UTF-8 and may contain
  * multiple lines separated by @c APR_EOL_STR.
diff --git a/subversion/libsvn_client/conflicts.c 
b/subversion/libsvn_client/conflicts.c
index d06ccb2..7f6ba54 100644
--- a/subversion/libsvn_client/conflicts.c
+++ b/subversion/libsvn_client/conflicts.c
@@ -122,6 +122,7 @@ typedef svn_error_t *(*conflict_option_resolve_func_t)(
 struct svn_client_conflict_option_t
 {
   svn_client_conflict_option_id_t id;
+  const char *label;
   const char *description;
 
   svn_client_conflict_t *conflict;
@@ -7063,6 +7064,7 @@ static svn_client_conflict_option_t *
 add_resolution_option(apr_array_header_t *options,
   svn_client_conflict_t *conflict,
   svn_client_conflict_option_id_t id,
+  const char *label,
   const char *description,
   conflict_option_resolve_func_t resolve_func)
 {
@@ -7071,6 +7073,7 @@ add_resolution_option(apr_array_header_t *options,
 option = apr_pcalloc(options->pool, sizeof(*option));
 option->pool = options->pool;
 option->id = id;
+option->label = apr_pstrdup(option->pool, label);
 option->description = apr_pstrdup(option->pool, description);
 option->conflict = conflict;
 option->do_resolve_func = resolve_func;
@@ -7096,6 +7099,7 @@ 
svn_client_conflict_text_get_resolution_options(apr_array_header_t **options,
 
   add_resolution_option(*options, conflict,
   svn_client_conflict_option_postpone,
+  _("Postpone"),
   _("skip this conflict and leave it unresolved"),
   resolve_postpone);
 
@@ -7105,16 +7109,19 @@ 
svn_client_conflict_text_get_resolution_options(apr_array_header_t **options,
   /* Resolver options for a binary file conflict. */
   add_resolution_option(*options, conflict,
 svn_client_conflict_option_base_text,
+_("Accept base"),
 _("discard local and incoming changes for this binary file"),
 resolve_text_conflict);
 
  

Re: [PATCH v2] Conflict option labels

[Ivan Zhakov]

On 13 October 2016 at 15:46, Patrick Steinhardt
 wrote:
> Hi,
>
> here's the second version of the conflict option label patch.
> Changes:
>
> - reworded some labels
> - now using apr_array to pass around options
> - renamed and simplified svn_client_resolver_option_label
>
> The functionality has been lightly tested by creating conflict
> scenarios.
>

Quick review:
> + * Return a textual human-readable label of @a option, allocated in
> + * @a result_pool. The label is encoded in UTF-8 and usually
> + * contains up to three words.
> + *
> + * Additionally, the label may be localized to the language used
> + * by the current locale.
> + *
> + * @since New in 1.10.
> + */
> +const char *
> +svn_client_conflict_option_get_label(svn_client_conflict_option_t *option);
The docstring mentions RESULT_POOL, but there is no such argument. I
think it would be better to RESULT_POOL for this function. This would
help to avoid slightly incorrect pool usage like in
find_option_by_builtin():
[[[
  client_opt = apr_pcalloc(result_pool, sizeof(*client_opt));
  client_opt->choice = id;
  client_opt->code = opt->code;
  client_opt->label = svn_client_conflict_option_get_label(
  builtin_option);
^ the label is not copied to RESULT_POOL.

  SVN_ERR(svn_client_conflict_option_describe(_opt->long_desc,
  builtin_option,
  result_pool,
  scratch_pool));
]]]

-- 
Ivan Zhakov

Re: Backport mixup last night

[Johan Corveleyn]

On Thu, Oct 13, 2016 at 4:17 PM, Daniel Shahaf  wrote:
> Johan Corveleyn wrote on Thu, Oct 13, 2016 at 11:37:25 +0200:
>> [...] the cronjob of backport.pl was running simultaneously on
>> svn-qavm2 (old machine) and svn-qavm3 (new machine we're migrating to,
>> see [1]), creating a race.
>>
>> For now, I've just renamed the backport.pl script on qavm2, so it
>> can't be run from cron anymore.
>
> Thanks for diagnosing and fixing the problem.  For belt and braces we
> might do «mkdir ~svnsvn/src/svn/trunk/tools/dist/backport.pl» on the old
> machine to ensure that an errant 'svn up' wouldn't reintroduce the
> script run from cron.
>
> (I'd do this myself but I'm separated from my ssh keys right now)

Ah, good idea. Done!

-- 
Johan

Re: [PATCH v2] Conflict option labels

[Patrick Steinhardt]

On Thu, Oct 13, 2016 at 04:52:12PM +0200, Stefan Sperling wrote:
> On Thu, Oct 13, 2016 at 04:46:55PM +0200, Patrick Steinhardt wrote:
> > Is there by any chance a tool which helps generating this tedious
> > list?
> 
> There are some but I cannot tell which is best and how well these are working.
> 
> https://svn.apache.org/repos/asf/subversion/trunk/contrib/client-side/vim/
> https://svn.apache.org/repos/asf/subversion/trunk/tools/dev/mklog.py

Oh, that's cool. Thanks for the pointers.

Regards
Patrick


signature.asc
Description: PGP signature

Re: Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)

[Daniel Shahaf]

Johan Corveleyn wrote on Thu, Oct 13, 2016 at 13:14:30 +0200:
> On Thu, Oct 13, 2016 at 1:05 PM, Branko Čibej  wrote:
> > On 13.10.2016 13:01, Branko Čibej wrote:
> >> On 13.10.2016 11:39, Ivan Zhakov wrote:
> >>> On 13 October 2016 at 10:59,   wrote:
>  Author: jcorvel
>  Date: Thu Oct 13 08:59:07 2016
>  New Revision: 1764631
> 
>  URL: http://svn.apache.org/viewvc?rev=1764631=rev
>  Log:
>  Resurrect the '1.9.x-r1721488' branch, to give backport.pl another
>  chance to execute the correct backport commands, after backport mess.
> 
> >>> I'm wondering if we really need backport.pl running as cronjob to
> >>> merge backports automatically to the stable branches? It's not a first
> >>> time when this automatic job performs invalid merges. And as far I
> >>> understand we still spend some time to babysit this tool, fix bugs
> >>> etc. What is wrong with old proven process by merging revisions
> >>> manually?
> >> It doesn't happen all that often that backport.pl makes a mistake. I bet
> >> manual merging would be just as error-prone.
> >>
> >> Backporting is a well-defined process. The best possible way to document
> >> a process is to automate it. Errors will happen but that's no reason to
> >> revert to PEBKAC; bugs can be fixed.
> >
> >
> > In fact, now that I've read Johan's mail ... it /was/ a PEBKAC and
> > nothing was wrong with backport.pl.
> 
> Indeed, what we experienced last night was not a script bug (unless if
> you state that it should have protected against that race condition
> :-)).

I think the issue is different.  backport.pl was never expected to run
concurrently with itself.  However, tonight's bug would also have
occurred if a human made a commit that met all the following conditions:

- Committed between 04:00:02 and (roughly) 04:00:12 UTC,
- on a night when there are 2+ approved groups,
- removes a group (other than the last) from the "Approved changes"
  section.

That's a pretty rare combination: we have few commits at around 4am UTC,
and we have few cases of vetoing an approved group.  That's why this bug
has never been encountered in 5+ years of using the cron job (and even
tonight, it was encountered due to the migration, not due to a nocturnal
committer).

Patch attached.

Cheers,

Daniel

[[[
backport.py: Fix a race condition involving concurrent commits to STATUS
(see r1764633).

The fix is to run 'update' at the top of the outermost loop, rather than
immediately before calling 'svn merge'.

* tools/dist/backport/merger.py
  (merge): Don't run revert+update; instead, expect the caller to have done so.

* tools/dist/merge-approved-backports.py:
Run 'update' and re-parse STATUS before each merge.

* tools/dist/detect-conflicting-backports.py:
Track API change of merge().
]]]

[[[
Index: backport/merger.py
===
--- backport/merger.py  (revision 1762016)
+++ backport/merger.py  (working copy)
@@ -195,11 +195,8 @@ def merge(entry, expected_stderr=None, *, commit=F
 mergeargs.extend(['--', sf.trunk_url()])
   logmsg += entry.raw
 
-  # TODO(interactive mode): exclude STATUS from reverts
-  # TODO(interactive mode): save local mods to disk, as backport.pl does
-  run_revert()
+  no_local_mods('.')
 
-  run_svn_quiet(['update'])
   # TODO: use select() to restore interweaving of stdout/stderr
   _, stdout, stderr = run_svn_quiet(['merge'] + mergeargs, expected_stderr)
   sys.stdout.write(stdout)
Index: detect-conflicting-backports.py
===
--- detect-conflicting-backports.py (revision 1762016)
+++ detect-conflicting-backports.py (working copy)
@@ -77,6 +77,7 @@ ERRORS = collections.defaultdict(list)
 for entry_para in sf.entries_paras():
 entry = entry_para.entry()
 # SVN_ERR_WC_FOUND_CONFLICT = 155015
+backport.merger.run_svn_quiet(['update']) # TODO: what to do if this pulls 
in a STATUS mod?
 backport.merger.merge(entry, 'svn: E155015' if entry.depends else None)
 
 _, output, _ = backport.merger.run_svn(['status'])
Index: merge-approved-backports.py
===
--- merge-approved-backports.py (revision 1762016)
+++ merge-approved-backports.py (working copy)
@@ -40,11 +40,14 @@ if sys.argv[1:]:
   sys.exit(0)
 
 backport.merger.no_local_mods('./STATUS')
-sf = backport.status.StatusFile(open('./STATUS', encoding="UTF-8"))
 
-# Duplicate sf.paragraphs, since merge() will be removing elements from it.
-entries_paras = list(sf.entries_paras())
-for entry_para in entries_paras:
-  if entry_para.approved():
-entry = entry_para.entry()
-backport.merger.merge(entry, commit=True)
+while True:
+backport.merger.run_svn_quiet(['update'])
+sf = backport.status.StatusFile(open('./STATUS', encoding="UTF-8"))
+for entry_para in sf.entries_paras():
+if entry_para.approved():
+

Re: [PATCH v2] Conflict option labels

[Stefan Sperling]

On Thu, Oct 13, 2016 at 04:46:55PM +0200, Patrick Steinhardt wrote:
> Is there by any chance a tool which helps generating this tedious
> list?

There are some but I cannot tell which is best and how well these are working.

https://svn.apache.org/repos/asf/subversion/trunk/contrib/client-side/vim/
https://svn.apache.org/repos/asf/subversion/trunk/tools/dev/mklog.py

Re: [PATCH v2] Conflict option labels

[Patrick Steinhardt]

On Thu, Oct 13, 2016 at 04:40:59PM +0200, Stefan Sperling wrote:
> On Thu, Oct 13, 2016 at 03:46:32PM +0200, Patrick Steinhardt wrote:
> > * subversion/include/svn_client.h:
> >   - new function `svn_client_conflict_option_get_label`
> > * subversion/libsvn_client/conflicts.c:
> >   - svn_client_conflict_option_t: add label
> >   - add_resolution_option: add label argument
> >   - implement function `svn_client_conflict_option_get_label`
> >   - (svn_client_conflict_text_get_reslution_options,
> >  svn_client_conflict_prop_get_resolution_options,
> >  configure_option_accept_current_wc_state,
> >  configure_option_move_destination,
> >  configure_option_update_raise_moved_away_children,
> >  configure_option_incoming_add_ignore,
> >  configure_option_incoming_added_file_text_merge,
> >  configure_option_incoming_added_file_replace_and_merge,
> >  configure_option_incoming_added_dir_merge,
> >  configure_option_incoming_added_dir_replace,
> >  configure_option_incoming_added_dir_replace_and_merge,
> >  configure_option_incoming_delete_ignore,
> >  configure_option_incoming_delete_accept,
> >  configure_option_incoming_move_file_merge,
> >  configure_option_incoming_dir_merge,
> >  svn_client_conflict_tree_get_resolution_options): set
> >  resolution option labels
> 
> This log message format is not entirely conforming to our guidelines.
> 
> Can you review existing log messages for examples and adjust your
> patch submission accordingly?
> 
> The above would usually be formatted like this:
> 
>  * subversion/libsvn_client/conflicts.c
>(svn_client_conflict_option_t): Add label.
>(add_resolution_option): Add label argument.
>(svn_client_conflict_option_get_label): New function.
>(svn_client_conflict_text_get_reslution_options,
> svn_client_conflict_prop_get_resolution_options,
> configure_option_accept_current_wc_state,
> configure_option_move_destination,
> configure_option_update_raise_moved_away_children,
> configure_option_incoming_add_ignore,
> configure_option_incoming_added_file_text_merge,
> configure_option_incoming_added_file_replace_and_merge,
> configure_option_incoming_added_dir_merge,
> configure_option_incoming_added_dir_replace,
> configure_option_incoming_added_dir_replace_and_merge,
> configure_option_incoming_delete_ignore,
> configure_option_incoming_delete_accept,
> configure_option_incoming_move_file_merge,
> configure_option_incoming_dir_merge,
> svn_client_conflict_tree_get_resolution_options): Set resolution option 
> labels.

Is there by any chance a tool which helps generating this tedious
list?

Regards
-- 
Patrick Steinhardt, Entwickler

elego Software Solutions GmbH, http://www.elego.de
Gebäude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany

Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
Handelsregister: Amtsgericht Charlottenburg HRB 77719
Geschäftsführer: Olaf Wagner


signature.asc
Description: PGP signature

Re: [PATCH v2] Conflict option labels

[Stefan Sperling]

On Thu, Oct 13, 2016 at 03:46:32PM +0200, Patrick Steinhardt wrote:
> * subversion/include/svn_client.h:
>   - new function `svn_client_conflict_option_get_label`
> * subversion/libsvn_client/conflicts.c:
>   - svn_client_conflict_option_t: add label
>   - add_resolution_option: add label argument
>   - implement function `svn_client_conflict_option_get_label`
>   - (svn_client_conflict_text_get_reslution_options,
>  svn_client_conflict_prop_get_resolution_options,
>  configure_option_accept_current_wc_state,
>  configure_option_move_destination,
>  configure_option_update_raise_moved_away_children,
>  configure_option_incoming_add_ignore,
>  configure_option_incoming_added_file_text_merge,
>  configure_option_incoming_added_file_replace_and_merge,
>  configure_option_incoming_added_dir_merge,
>  configure_option_incoming_added_dir_replace,
>  configure_option_incoming_added_dir_replace_and_merge,
>  configure_option_incoming_delete_ignore,
>  configure_option_incoming_delete_accept,
>  configure_option_incoming_move_file_merge,
>  configure_option_incoming_dir_merge,
>  svn_client_conflict_tree_get_resolution_options): set
>  resolution option labels

This log message format is not entirely conforming to our guidelines.

Can you review existing log messages for examples and adjust your
patch submission accordingly?

The above would usually be formatted like this:

 * subversion/libsvn_client/conflicts.c
   (svn_client_conflict_option_t): Add label.
   (add_resolution_option): Add label argument.
   (svn_client_conflict_option_get_label): New function.
   (svn_client_conflict_text_get_reslution_options,
svn_client_conflict_prop_get_resolution_options,
configure_option_accept_current_wc_state,
configure_option_move_destination,
configure_option_update_raise_moved_away_children,
configure_option_incoming_add_ignore,
configure_option_incoming_added_file_text_merge,
configure_option_incoming_added_file_replace_and_merge,
configure_option_incoming_added_dir_merge,
configure_option_incoming_added_dir_replace,
configure_option_incoming_added_dir_replace_and_merge,
configure_option_incoming_delete_ignore,
configure_option_incoming_delete_accept,
configure_option_incoming_move_file_merge,
configure_option_incoming_dir_merge,
svn_client_conflict_tree_get_resolution_options): Set resolution option 
labels.

Re: svn commit: r1764676 - /subversion/trunk/subversion/libsvn_fs_fs/pack.c

[Ivan Zhakov]

On 13 October 2016 at 15:49,   wrote:
> Author: stefan2
> Date: Thu Oct 13 13:49:47 2016
> New Revision: 1764676
>
> URL: http://svn.apache.org/viewvc?rev=1764676=rev
> Log:
> Make the FSFS pack no longer depend on a working file trunc() operation.
>
> * subversion/libsvn_fs_fs/pack.c
>   (reset_pack_context): Instead of emptying the temporary files, close,
> auto-delete and re-create them.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/pack.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/pack.c
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/pack.c?rev=1764676=1764675=1764676=diff
> ==
> --- subversion/trunk/subversion/libsvn_fs_fs/pack.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/pack.c Thu Oct 13 13:49:47 2016
> @@ -341,20 +341,38 @@ static svn_error_t *
>  reset_pack_context(pack_context_t *context,
> apr_pool_t *pool)
>  {
> +  const char *temp_dir;
> +
>apr_array_clear(context->changes);
> -  SVN_ERR(svn_io_file_trunc(context->changes_file, 0, pool));
> +  SVN_ERR(svn_io_file_close(context->changes_file, pool));
>apr_array_clear(context->file_props);
> -  SVN_ERR(svn_io_file_trunc(context->file_props_file, 0, pool));
> +  SVN_ERR(svn_io_file_close(context->file_props_file, pool));
>apr_array_clear(context->dir_props);
> -  SVN_ERR(svn_io_file_trunc(context->dir_props_file, 0, pool));
> +  SVN_ERR(svn_io_file_close(context->dir_props_file, pool));
>
>apr_array_clear(context->rev_offsets);
>apr_array_clear(context->path_order);
>apr_array_clear(context->references);
>apr_array_clear(context->reps);
> -  SVN_ERR(svn_io_file_trunc(context->reps_file, 0, pool));
> +  SVN_ERR(svn_io_file_close(context->reps_file, pool));
>
>svn_pool_clear(context->info_pool);
> +
> +  /* The new temporary files must live at least as long as any other info
> +   * object in CONTEXT. */
> +  SVN_ERR(svn_io_temp_dir(_dir, pool));
> +  SVN_ERR(svn_io_open_unique_file3(>changes_file, NULL, temp_dir,
> +   svn_io_file_del_on_close,
> +   context->info_pool, pool));
> +  SVN_ERR(svn_io_open_unique_file3(>file_props_file, NULL, temp_dir,
> +   svn_io_file_del_on_close,
> +   context->info_pool, pool));
> +  SVN_ERR(svn_io_open_unique_file3(>dir_props_file, NULL, temp_dir,
> +   svn_io_file_del_on_close,
> +   context->info_pool, pool));
> +  SVN_ERR(svn_io_open_unique_file3(>reps_file, NULL, temp_dir,
> +   svn_io_file_del_on_close,
> +   context->info_pool, pool));
Probably we should update code in initialize_pack_context() to use
CONTEX->INFO_POOL when opening these files initially?

-- 
Ivan Zhakov

Re: Backport mixup last night

[Daniel Shahaf]

Johan Corveleyn wrote on Thu, Oct 13, 2016 at 11:37:25 +0200:
> [...] the cronjob of backport.pl was running simultaneously on
> svn-qavm2 (old machine) and svn-qavm3 (new machine we're migrating to,
> see [1]), creating a race.
> 
> For now, I've just renamed the backport.pl script on qavm2, so it
> can't be run from cron anymore.

Thanks for diagnosing and fixing the problem.  For belt and braces we
might do «mkdir ~svnsvn/src/svn/trunk/tools/dist/backport.pl» on the old
machine to ensure that an errant 'svn up' wouldn't reintroduce the
script run from cron.

(I'd do this myself but I'm separated from my ssh keys right now)

Re: [PATCH v2] Conflict option labels

[Ivan Zhakov]

On 13 October 2016 at 15:52, Patrick Steinhardt  wrote:
> On Thu, Oct 13, 2016 at 03:46:32PM +0200, Patrick Steinhardt wrote:
>> Hi,
>>
[..]
> [snip]
>
> By the way, one problem that still exists is consistency. Right
> now, we have a mixture of labels where the first character is
> uppercased and labels where the first character is lowercased.
> With GUI clients in mind I personally lend towards using
> uppercased labels, but I'd need to adjust remaining labels to
> provide them (maybe in a separate patch, this one is big enough
> already as-is).
>
> Any opinions?
>
I'm for uppercased labels.


-- 
Ivan Zhakov

Re: [PATCH v2] Conflict option labels

[Patrick Steinhardt]

On Thu, Oct 13, 2016 at 03:46:32PM +0200, Patrick Steinhardt wrote:
> Hi,
> 
> here's the second version of the conflict option label patch.
> Changes:
> 
> - reworded some labels
> - now using apr_array to pass around options
> - renamed and simplified svn_client_resolver_option_label
> 
> The functionality has been lightly tested by creating conflict
> scenarios.
> 
> [[
> Move conflict resolution options' labels out of the client
> 
> * subversion/include/svn_client.h:
>   - new function `svn_client_conflict_option_get_label`
> * subversion/libsvn_client/conflicts.c:
>   - svn_client_conflict_option_t: add label
>   - add_resolution_option: add label argument
>   - implement function `svn_client_conflict_option_get_label`
>   - (svn_client_conflict_text_get_reslution_options,
>  svn_client_conflict_prop_get_resolution_options,
>  configure_option_accept_current_wc_state,
>  configure_option_move_destination,
>  configure_option_update_raise_moved_away_children,
>  configure_option_incoming_add_ignore,
>  configure_option_incoming_added_file_text_merge,
>  configure_option_incoming_added_file_replace_and_merge,
>  configure_option_incoming_added_dir_merge,
>  configure_option_incoming_added_dir_replace,
>  configure_option_incoming_added_dir_replace_and_merge,
>  configure_option_incoming_delete_ignore,
>  configure_option_incoming_delete_accept,
>  configure_option_incoming_move_file_merge,
>  configure_option_incoming_dir_merge,
>  svn_client_conflict_tree_get_resolution_options): set
>  resolution option labels
> * subversion/svn/conflict-callbacks.c:
>   - resolver_option_t: remove short_desc and long_desc
>   - client_option_t: new struct for client options
>   - builtin_resolver_options: remove short_desc and long_desc
>   - (extra_resolver_options,
>  extra_resolver_options_text,
>  extra_resolver_options_prop,
>  extra_resolver_options_tree): convert to client_option_t
>   - find_option: accept options as apr_array_header_t
>   - find_option_by_builtin: function to create provided options
>  from builtin library options
>   - find_option_by_id: replaced by find_option_by_builtin
>   - (prompt_string,
>  help_string,
>  prompt_user,
>  build_text_conflict_options,
>  build_prop_conflict_options,
>  build_prop_text_conflict_options,
>  handle_one_prop_conflict.
>  build_tree_conflict_options,
>  handle_tree_conflict): accept options as apr_array_header_t
> ]]

[snip]

By the way, one problem that still exists is consistency. Right
now, we have a mixture of labels where the first character is
uppercased and labels where the first character is lowercased.
With GUI clients in mind I personally lend towards using
uppercased labels, but I'd need to adjust remaining labels to
provide them (maybe in a separate patch, this one is big enough
already as-is).

Any opinions?

Regards
Patrick


signature.asc
Description: PGP signature

[PATCH v2] Conflict option labels

[Patrick Steinhardt]

Hi,

here's the second version of the conflict option label patch.
Changes:

- reworded some labels
- now using apr_array to pass around options
- renamed and simplified svn_client_resolver_option_label

The functionality has been lightly tested by creating conflict
scenarios.

[[
Move conflict resolution options' labels out of the client

* subversion/include/svn_client.h:
  - new function `svn_client_conflict_option_get_label`
* subversion/libsvn_client/conflicts.c:
  - svn_client_conflict_option_t: add label
  - add_resolution_option: add label argument
  - implement function `svn_client_conflict_option_get_label`
  - (svn_client_conflict_text_get_reslution_options,
 svn_client_conflict_prop_get_resolution_options,
 configure_option_accept_current_wc_state,
 configure_option_move_destination,
 configure_option_update_raise_moved_away_children,
 configure_option_incoming_add_ignore,
 configure_option_incoming_added_file_text_merge,
 configure_option_incoming_added_file_replace_and_merge,
 configure_option_incoming_added_dir_merge,
 configure_option_incoming_added_dir_replace,
 configure_option_incoming_added_dir_replace_and_merge,
 configure_option_incoming_delete_ignore,
 configure_option_incoming_delete_accept,
 configure_option_incoming_move_file_merge,
 configure_option_incoming_dir_merge,
 svn_client_conflict_tree_get_resolution_options): set
 resolution option labels
* subversion/svn/conflict-callbacks.c:
  - resolver_option_t: remove short_desc and long_desc
  - client_option_t: new struct for client options
  - builtin_resolver_options: remove short_desc and long_desc
  - (extra_resolver_options,
 extra_resolver_options_text,
 extra_resolver_options_prop,
 extra_resolver_options_tree): convert to client_option_t
  - find_option: accept options as apr_array_header_t
  - find_option_by_builtin: function to create provided options
 from builtin library options
  - find_option_by_id: replaced by find_option_by_builtin
  - (prompt_string,
 help_string,
 prompt_user,
 build_text_conflict_options,
 build_prop_conflict_options,
 build_prop_text_conflict_options,
 handle_one_prop_conflict.
 build_tree_conflict_options,
 handle_tree_conflict): accept options as apr_array_header_t
]]

Regards
-- 
Patrick Steinhardt, Entwickler

elego Software Solutions GmbH, http://www.elego.de
Gebäude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany

Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
Handelsregister: Amtsgericht Charlottenburg HRB 77719
Geschäftsführer: Olaf Wagner
diff --git a/subversion/include/svn_client.h b/subversion/include/svn_client.h
index 9bbe62b..8dc121c 100644
--- a/subversion/include/svn_client.h
+++ b/subversion/include/svn_client.h
@@ -4718,6 +4718,19 @@ svn_client_conflict_option_id_t
 svn_client_conflict_option_get_id(svn_client_conflict_option_t *option);
 
 /**
+ * Return a textual human-readable label of @a option, allocated in
+ * @a result_pool. The label is encoded in UTF-8 and usually
+ * contains up to three words.
+ *
+ * Additionally, the label may be localized to the language used
+ * by the current locale.
+ *
+ * @since New in 1.10.
+ */
+const char *
+svn_client_conflict_option_get_label(svn_client_conflict_option_t *option);
+
+/**
  * Return a textual human-readable description of @a option, allocated in
  * @a result_pool. The description is encoded in UTF-8 and may contain
  * multiple lines separated by @c APR_EOL_STR.
diff --git a/subversion/libsvn_client/conflicts.c 
b/subversion/libsvn_client/conflicts.c
index d06ccb2..2e97cbd 100644
--- a/subversion/libsvn_client/conflicts.c
+++ b/subversion/libsvn_client/conflicts.c
@@ -122,6 +122,7 @@ typedef svn_error_t *(*conflict_option_resolve_func_t)(
 struct svn_client_conflict_option_t
 {
   svn_client_conflict_option_id_t id;
+  const char *label;
   const char *description;
 
   svn_client_conflict_t *conflict;
@@ -7063,6 +7064,7 @@ static svn_client_conflict_option_t *
 add_resolution_option(apr_array_header_t *options,
   svn_client_conflict_t *conflict,
   svn_client_conflict_option_id_t id,
+  const char *label,
   const char *description,
   conflict_option_resolve_func_t resolve_func)
 {
@@ -7071,6 +7073,7 @@ add_resolution_option(apr_array_header_t *options,
 option = apr_pcalloc(options->pool, sizeof(*option));
 option->pool = options->pool;
 option->id = id;
+option->label = apr_pstrdup(option->pool, label);
 option->description = apr_pstrdup(option->pool, description);
 option->conflict = conflict;
 option->do_resolve_func = resolve_func;
@@ -7096,6 +7099,7 @@ 
svn_client_conflict_text_get_resolution_options(apr_array_header_t **options,
 
   add_resolution_option(*options, conflict,
   svn_client_conflict_option_postpone,
+  _("Postpone"),
   _("skip 

Re: [RFC] Subversion command line UI for interactive conflict resolution

[Ivan Zhakov]

On 13 October 2016 at 15:23, Stefan Sperling  wrote:
> On Thu, Oct 13, 2016 at 03:01:39PM +0200, Ivan Zhakov wrote:
>> I suggest to change behavior to something like the following:
>> [[[
>> $ svn resolve
>> Searching tree conflict details for
>> 'D:\ivan\svn\test-wc\add-versus-add\foo' in repository:
>> Checking r5... done
>> Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo':
>> File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved
>> to '^/branches/b1/bar' by ivan in r5.
>> A file which differs from the corresponding file on the merge source
>> branch was found in the working copy.
>>
>> Resolution options:
>>   (p)  - postpone
>>   (r)  - mark as resolved
>>   (m)  - move and merge
>>   (h)  - help
>>   (q)  - postpone all remaining conflicts
>>
>> Select:
>> ]]]
>>
>> When user types 'h' the some prompt will be shown, but with more
>> detailed description:
>> [[[
>> Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo':
>> File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved
>> to '^/branches/b1/bar' by ivan in r5.
>> A file which differs from the corresponding file on the merge source
>> branch was found in the working copy.
>>
>> Resolution options:
>>   (p)  - postpone
>>  skip this conflict and leave it unresolved [postpone]
>>   (r)  - mark as resolved
>>  accept current working copy state [working]
>>   (m)  - move and merge
>>  move 'foo' to 'bar' and merge
>>   (h)  - help
>>   (q)  - postpone all remaining conflicts
>>
>> Select:
>> ]]]
>
> +1
>
> It might also be nice to show a detailed conflict description only
> if the user asks for help:
>
>  File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved
>  to '^/branches/b1/bar' by ivan in r5.
>  A file which differs from the corresponding file on the merge source
>  branch was found in the working copy.
>
> By default we could show an abbreviated version of this description.
Good idea. But we need short_description API to implement this :)


-- 
Ivan Zhakov

Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS

[Branko Čibej]

On 13.10.2016 14:56, Ivan Zhakov wrote:
> On 13 October 2016 at 14:00, Branko Čibej  wrote:
>> On 13.10.2016 13:28, Johan Corveleyn wrote:
>>> On Thu, Oct 13, 2016 at 1:22 PM, Ivan Zhakov  wrote:
 On 12 October 2016 at 22:30, Johan Corveleyn  wrote:
> On Wed, Oct 12, 2016 at 10:13 PM,  wrote:
>> Author: ivan
>> Date: Wed Oct 12 20:13:24 2016
>> New Revision: 1764536
>>
>> URL: http://svn.apache.org/viewvc?rev=1764536=rev
>> Log:
>> * STATUS: Veto r1759116 group.
>>
>> Modified:
>> subversion/branches/1.9.x/STATUS
> ...
>> @@ -135,6 +119,24 @@ Candidate changes:
>>  Veto-blocked changes:
>>  =
>>
>> + * r1759116, r1759117, r1759122, r1759123, r1759124
>> +   Fix FSFS format 7 packing for revisions packs with lots of changes.
>> +   Justification:
>> + Problem occurred in at least two user repositories.  Without the 
>> fix,
>> + format 7 repositories with an exceptionally large number of 
>> changes in
>> + a pack cannot be packed - which renders using f7 pointless for 
>> those
>> + users.
>> +   Branch:
>> + ^/subversion/branches/1.9.x-fsfs-pack-fixes
>> +   Notes:
>> + r1759116 adds a workaround for trunc() corrupting apr_file_t 
>> internal state.
>> + r1759117-23 provide the actual fixes.
>> + r1759124 adds a regression test with the necessary internal API 
>> changes.
>> +   Votes:
>> + +1: stefan2
>> + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR,
>> +   not by unreliable workaround with unknown consequences)
>> +
 [...]
> So that leaves you claiming that it's an "unreliable workaround with
> unknown consequences". Why? Please clarify. We are talking about a
> bugfix which fixes an important problem reported by users. Do you have
> any suggestions for improvement? Perhaps the fix should be technically
> discussed on the mailing list? Or should we just leave this important
> problem unfixed?
 I think it's almost always better to solve the problem in its origin.
 Any workaround is unreliable, so we should always try to fix the
 problem at the root before trying the workarounds. Note that we are
 talking about a relatively obvious bug in other Apache's project, not
 about something irrecoverable.

 As far as I know, this problem has never been reported to the APR's
 dev list. And this is the first action that we should have done.

> Do you have any suggestions for improvement?
 I have plans to send a patch to APR in order to solve this problem. So
 we don't need to release this workaround.
>>> Okay, I understand this should be fixed in APR.
>>>
>>> But should we block a workaround in our code for a bug that causes
>>> problems in the wild for existing svn 1.9 installations? Those users
>>> deserve a fix in a 1.9.x patch release, IMHO. If you can arrange this
>>> by fixing it in APR, creating a patch release of APR, and then
>>> creating a 1.9.x svn release with that fix (in a reasonable
>>> timeframe), then fine. But otherwise I think a svn-internal-workaround
>>> is warranted.
>>
>> There's no chance of doing this in a reasonable time-frame because we
>> require APR-1.3 and no fix we can possibly come up with will be released
>> in a 1.3 patch.
>>
>> We have any number of workarounds for APR quirks in the code. I can't
>> think of a single valid reason to veto another one, especially since
>> we'd /still/ have to have the workaround in our code for people who use
>> older APR versions (likely versions prior to 1.5 or even 1.6, depending
>> on what the fix ends up looking like).
>>
>> "Any workaround is unreliable" is just nonsense, IMHO.
>>
> It seems that you are pulling my phrase out of context. Do you suggest
> to rely on workarounds instead of fixing problems at they roots? Note
> that this particular discussion is about our workaround, not about
> something officially suggested by APR devs.


We're talking about your vetoing a backport because it contains a
workaround for a bug in APR. Obviously we want to fix APR, but there's
no reason to stop doing what we've already done in a number of places in
our code. Your argument for the veto is that workarounds are unreliable
... well, they can be, and maybe this one is, but the general statement
is irrelevant to the discussion.


-- Brane

Re: [RFC] Subversion command line UI for interactive conflict resolution

[Evgeny Kotkov]

Ivan Zhakov  writes:

> I suggest to change behavior to something like the following:
> [[[
> $ svn resolve
> Searching tree conflict details for
> 'D:\ivan\svn\test-wc\add-versus-add\foo' in repository:
> Checking r5... done
> Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo':
> File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved
> to '^/branches/b1/bar' by ivan in r5.
> A file which differs from the corresponding file on the merge source
> branch was found in the working copy.
>
> Resolution options:
>   (p)  - postpone
>   (r)  - mark as resolved
>   (m)  - move and merge
>   (h)  - help
>   (q)  - postpone all remaining conflicts

I'd love to see the layout like this:

 Resolution options:
   (p) Postpone
   (r) Mark as resolved
   (m) Move and merge
   (h) Help
   (q) Postpone all remaining conflicts

Which changes to this, if the user selects "Help":

 Resolution options:
   (p) Postpone (skip this conflict and leave it unresolved) [postpone]
   (r) Mark as resolved (accept current working copy state) [working]
   (m) Move and merge (move '^/subversion/trunk/subversion/libsvn_client/foo'
   to 'subversion\libsvn_client\bar' and merge)
   (h) Help
   (q) Postpone all remaining conflicts


Regards,
Evgeny Kotkov

Re: [RFC] Subversion command line UI for interactive conflict resolution

[Stefan Sperling]

On Thu, Oct 13, 2016 at 03:01:39PM +0200, Ivan Zhakov wrote:
> I suggest to change behavior to something like the following:
> [[[
> $ svn resolve
> Searching tree conflict details for
> 'D:\ivan\svn\test-wc\add-versus-add\foo' in repository:
> Checking r5... done
> Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo':
> File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved
> to '^/branches/b1/bar' by ivan in r5.
> A file which differs from the corresponding file on the merge source
> branch was found in the working copy.
> 
> Resolution options:
>   (p)  - postpone
>   (r)  - mark as resolved
>   (m)  - move and merge
>   (h)  - help
>   (q)  - postpone all remaining conflicts
> 
> Select:
> ]]]
> 
> When user types 'h' the some prompt will be shown, but with more
> detailed description:
> [[[
> Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo':
> File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved
> to '^/branches/b1/bar' by ivan in r5.
> A file which differs from the corresponding file on the merge source
> branch was found in the working copy.
> 
> Resolution options:
>   (p)  - postpone
>  skip this conflict and leave it unresolved [postpone]
>   (r)  - mark as resolved
>  accept current working copy state [working]
>   (m)  - move and merge
>  move 'foo' to 'bar' and merge
>   (h)  - help
>   (q)  - postpone all remaining conflicts
> 
> Select:
> ]]]

+1

It might also be nice to show a detailed conflict description only
if the user asks for help:

 File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved
 to '^/branches/b1/bar' by ivan in r5.
 A file which differs from the corresponding file on the merge source
 branch was found in the working copy.

By default we could show an abbreviated version of this description.

[RFC] Subversion command line UI for interactive conflict resolution

[Ivan Zhakov]

Hi,

I'm thinking new conflict resolution should look like in Subversion
command line client. Currently 'svn resolve' works like the following:
[[[
$ svn resolve
Searching tree conflict details for
'D:\ivan\svn\test-wc\add-versus-add\foo' in repository:
Checking r5... done
Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo':
File merged from
'^/trunk/foo@2'
to
'^/branches/b1/foo@16'
was moved to '^/branches/b1/bar' by ivan in r5.
A file which differs from the corresponding file on the merge source
branch was found in the working copy.
Select: (p) postpone, (r) accept current working copy state,
(m) move 'foo' to 'bar' and merge, (h) help, (q) quit resolution:
]]]

Then when user types 'h' it will see expanded conflict resolutions
options with one option on each line:
[[[
File merged from
'^/trunk/foo@2'
to
'^/branches/b1/foo@16'
was moved to '^/branches/b1/bar' by ivan in r5.
A file which differs from the corresponding file on the merge source
branch was found in the working copy.

  (p)  - skip this conflict and leave it unresolved  [postpone]
  (r)  - accept current working copy state  [working]
  (m)  - move 'foo' to 'bar' and merge
  (h)  - show this help (also '?')
  (q)  - postpone all remaining conflicts
Words in square brackets are the corresponding --accept option arguments.

Select: (p) postpone, (r) accept current working copy state,
(m) move 'foo' to 'bar' and merge, (h) help, (q) quit resolution:
]]]

I suggest to change behavior to something like the following:
[[[
$ svn resolve
Searching tree conflict details for
'D:\ivan\svn\test-wc\add-versus-add\foo' in repository:
Checking r5... done
Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo':
File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved
to '^/branches/b1/bar' by ivan in r5.
A file which differs from the corresponding file on the merge source
branch was found in the working copy.

Resolution options:
  (p)  - postpone
  (r)  - mark as resolved
  (m)  - move and merge
  (h)  - help
  (q)  - postpone all remaining conflicts

Select:
]]]

When user types 'h' the some prompt will be shown, but with more
detailed description:
[[[
Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo':
File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved
to '^/branches/b1/bar' by ivan in r5.
A file which differs from the corresponding file on the merge source
branch was found in the working copy.

Resolution options:
  (p)  - postpone
 skip this conflict and leave it unresolved [postpone]
  (r)  - mark as resolved
 accept current working copy state [working]
  (m)  - move and merge
 move 'foo' to 'bar' and merge
  (h)  - help
  (q)  - postpone all remaining conflicts

Select:
]]]

Alternative layout:
[[[
Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo':
File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved
to '^/branches/b1/bar' by ivan in r5.
A file which differs from the corresponding file on the merge source
branch was found in the working copy.

Resolution options:
  (p)  - Postpone: skip this conflict and leave it unresolved. [postpone]
  (r)  - Mark as resolved: accept current working copy state. [working]
  (m)  - Move and merge: move 'foo' to 'bar' and merge.
  (h)  - Help
  (q)  - Postpone all remaining conflicts

Select:
]]]


-- 
Ivan Zhakov

Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS

[Ivan Zhakov]

On 13 October 2016 at 14:00, Branko Čibej  wrote:
> On 13.10.2016 13:28, Johan Corveleyn wrote:
>> On Thu, Oct 13, 2016 at 1:22 PM, Ivan Zhakov  wrote:
>>> On 12 October 2016 at 22:30, Johan Corveleyn  wrote:
 On Wed, Oct 12, 2016 at 10:13 PM,  wrote:
> Author: ivan
> Date: Wed Oct 12 20:13:24 2016
> New Revision: 1764536
>
> URL: http://svn.apache.org/viewvc?rev=1764536=rev
> Log:
> * STATUS: Veto r1759116 group.
>
> Modified:
> subversion/branches/1.9.x/STATUS
 ...
> @@ -135,6 +119,24 @@ Candidate changes:
>  Veto-blocked changes:
>  =
>
> + * r1759116, r1759117, r1759122, r1759123, r1759124
> +   Fix FSFS format 7 packing for revisions packs with lots of changes.
> +   Justification:
> + Problem occurred in at least two user repositories.  Without the 
> fix,
> + format 7 repositories with an exceptionally large number of changes 
> in
> + a pack cannot be packed - which renders using f7 pointless for those
> + users.
> +   Branch:
> + ^/subversion/branches/1.9.x-fsfs-pack-fixes
> +   Notes:
> + r1759116 adds a workaround for trunc() corrupting apr_file_t 
> internal state.
> + r1759117-23 provide the actual fixes.
> + r1759124 adds a regression test with the necessary internal API 
> changes.
> +   Votes:
> + +1: stefan2
> + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR,
> +   not by unreliable workaround with unknown consequences)
> +
>>> [...]
 So that leaves you claiming that it's an "unreliable workaround with
 unknown consequences". Why? Please clarify. We are talking about a
 bugfix which fixes an important problem reported by users. Do you have
 any suggestions for improvement? Perhaps the fix should be technically
 discussed on the mailing list? Or should we just leave this important
 problem unfixed?
>>> I think it's almost always better to solve the problem in its origin.
>>> Any workaround is unreliable, so we should always try to fix the
>>> problem at the root before trying the workarounds. Note that we are
>>> talking about a relatively obvious bug in other Apache's project, not
>>> about something irrecoverable.
>>>
>>> As far as I know, this problem has never been reported to the APR's
>>> dev list. And this is the first action that we should have done.
>>>
 Do you have any suggestions for improvement?
>>> I have plans to send a patch to APR in order to solve this problem. So
>>> we don't need to release this workaround.
>> Okay, I understand this should be fixed in APR.
>>
>> But should we block a workaround in our code for a bug that causes
>> problems in the wild for existing svn 1.9 installations? Those users
>> deserve a fix in a 1.9.x patch release, IMHO. If you can arrange this
>> by fixing it in APR, creating a patch release of APR, and then
>> creating a 1.9.x svn release with that fix (in a reasonable
>> timeframe), then fine. But otherwise I think a svn-internal-workaround
>> is warranted.
>
>
> There's no chance of doing this in a reasonable time-frame because we
> require APR-1.3 and no fix we can possibly come up with will be released
> in a 1.3 patch.
>
> We have any number of workarounds for APR quirks in the code. I can't
> think of a single valid reason to veto another one, especially since
> we'd /still/ have to have the workaround in our code for people who use
> older APR versions (likely versions prior to 1.5 or even 1.6, depending
> on what the fix ends up looking like).
>
> "Any workaround is unreliable" is just nonsense, IMHO.
>
It seems that you are pulling my phrase out of context. Do you suggest
to rely on workarounds instead of fixing problems at they roots? Note
that this particular discussion is about our workaround, not about
something officially suggested by APR devs.

-- 
Ivan Zhakov

Re: [PATCH] Reject checkouts to existing directory

[Stefan]

On 10/12/2016 5:00 PM, 'Stefan Sperling' wrote:
> I'm actually surprised that we made this change in 1.7.
>
> If 'svn checkout' sees an existing directory the most likely situation is
> that the user has made a mistake. Leaving behind a working copy full of
> tree conflicts is hardly useful in this situation. I would expect this to
> happen only with a flag such as --force but the default behaviour being
> not touch an existing directory.

Was looking through the mailing list but couldn't find any older
discussion leading to this change of behavior in 1.7.

From my point of view, it would be fine to change the behavior, if
checking out into an empty directory would still work without the
force-parameter.
Rational: New users of svn (especially Windows users) usually create a
folder first and then want to co into that directory (don't ask me why
it's different for the Linux user base). If an obvious "svn co [URL]
C:\mywc" fails without specifying the --force parameter it would
introduce a new unnecessary hurdle for new users, IMO.

Regards,
Stefan




smime.p7s
Description: S/MIME Cryptographic Signature

RE: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS

[Bert Huijben]

> -Original Message-
> From: Branko Čibej [mailto:br...@apache.org]
> Sent: donderdag 13 oktober 2016 14:00
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS


> There's no chance of doing this in a reasonable time-frame because we
> require APR-1.3 and no fix we can possibly come up with will be released
> in a 1.3 patch.

As long as nobody even reported the bug on APR on any apr list/group/anything, 
there won't be a fix... ever...


Please, can we at least prioritize reporting a problem in apr and preferable 
fixing it there over doing workarounds that we have to maintain forever.


After apr is patched I would like to see the workaround code conditionalized on 
being compiled against older versions of apr.


Note I haven't looked at the patch in detail, but it looks like it changes a 
quite generic function by introducing new race conditions in certain error 
cases. Not something I would like to see in our code forever

Bert

Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS

[Branko Čibej]

On 13.10.2016 13:28, Johan Corveleyn wrote:
> On Thu, Oct 13, 2016 at 1:22 PM, Ivan Zhakov  wrote:
>> On 12 October 2016 at 22:30, Johan Corveleyn  wrote:
>>> On Wed, Oct 12, 2016 at 10:13 PM,  wrote:
 Author: ivan
 Date: Wed Oct 12 20:13:24 2016
 New Revision: 1764536

 URL: http://svn.apache.org/viewvc?rev=1764536=rev
 Log:
 * STATUS: Veto r1759116 group.

 Modified:
 subversion/branches/1.9.x/STATUS
>>> ...
 @@ -135,6 +119,24 @@ Candidate changes:
  Veto-blocked changes:
  =

 + * r1759116, r1759117, r1759122, r1759123, r1759124
 +   Fix FSFS format 7 packing for revisions packs with lots of changes.
 +   Justification:
 + Problem occurred in at least two user repositories.  Without the fix,
 + format 7 repositories with an exceptionally large number of changes 
 in
 + a pack cannot be packed - which renders using f7 pointless for those
 + users.
 +   Branch:
 + ^/subversion/branches/1.9.x-fsfs-pack-fixes
 +   Notes:
 + r1759116 adds a workaround for trunc() corrupting apr_file_t 
 internal state.
 + r1759117-23 provide the actual fixes.
 + r1759124 adds a regression test with the necessary internal API 
 changes.
 +   Votes:
 + +1: stefan2
 + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR,
 +   not by unreliable workaround with unknown consequences)
 +
>> [...]
>>> So that leaves you claiming that it's an "unreliable workaround with
>>> unknown consequences". Why? Please clarify. We are talking about a
>>> bugfix which fixes an important problem reported by users. Do you have
>>> any suggestions for improvement? Perhaps the fix should be technically
>>> discussed on the mailing list? Or should we just leave this important
>>> problem unfixed?
>> I think it's almost always better to solve the problem in its origin.
>> Any workaround is unreliable, so we should always try to fix the
>> problem at the root before trying the workarounds. Note that we are
>> talking about a relatively obvious bug in other Apache's project, not
>> about something irrecoverable.
>>
>> As far as I know, this problem has never been reported to the APR's
>> dev list. And this is the first action that we should have done.
>>
>>> Do you have any suggestions for improvement?
>> I have plans to send a patch to APR in order to solve this problem. So
>> we don't need to release this workaround.
> Okay, I understand this should be fixed in APR.
>
> But should we block a workaround in our code for a bug that causes
> problems in the wild for existing svn 1.9 installations? Those users
> deserve a fix in a 1.9.x patch release, IMHO. If you can arrange this
> by fixing it in APR, creating a patch release of APR, and then
> creating a 1.9.x svn release with that fix (in a reasonable
> timeframe), then fine. But otherwise I think a svn-internal-workaround
> is warranted.


There's no chance of doing this in a reasonable time-frame because we
require APR-1.3 and no fix we can possibly come up with will be released
in a 1.3 patch.

We have any number of workarounds for APR quirks in the code. I can't
think of a single valid reason to veto another one, especially since
we'd /still/ have to have the workaround in our code for people who use
older APR versions (likely versions prior to 1.5 or even 1.6, depending
on what the fix ends up looking like).

"Any workaround is unreliable" is just nonsense, IMHO.


-- Brane

Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS

[Johan Corveleyn]

On Thu, Oct 13, 2016 at 1:22 PM, Ivan Zhakov  wrote:
> On 12 October 2016 at 22:30, Johan Corveleyn  wrote:
>> On Wed, Oct 12, 2016 at 10:13 PM,  wrote:
>>>
>>> Author: ivan
>>> Date: Wed Oct 12 20:13:24 2016
>>> New Revision: 1764536
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1764536=rev
>>> Log:
>>> * STATUS: Veto r1759116 group.
>>>
>>> Modified:
>>> subversion/branches/1.9.x/STATUS
>> ...
>>> @@ -135,6 +119,24 @@ Candidate changes:
>>>  Veto-blocked changes:
>>>  =
>>>
>>> + * r1759116, r1759117, r1759122, r1759123, r1759124
>>> +   Fix FSFS format 7 packing for revisions packs with lots of changes.
>>> +   Justification:
>>> + Problem occurred in at least two user repositories.  Without the fix,
>>> + format 7 repositories with an exceptionally large number of changes in
>>> + a pack cannot be packed - which renders using f7 pointless for those
>>> + users.
>>> +   Branch:
>>> + ^/subversion/branches/1.9.x-fsfs-pack-fixes
>>> +   Notes:
>>> + r1759116 adds a workaround for trunc() corrupting apr_file_t internal 
>>> state.
>>> + r1759117-23 provide the actual fixes.
>>> + r1759124 adds a regression test with the necessary internal API 
>>> changes.
>>> +   Votes:
>>> + +1: stefan2
>>> + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR,
>>> +   not by unreliable workaround with unknown consequences)
>>> +
>>
> [...]
>> So that leaves you claiming that it's an "unreliable workaround with
>> unknown consequences". Why? Please clarify. We are talking about a
>> bugfix which fixes an important problem reported by users. Do you have
>> any suggestions for improvement? Perhaps the fix should be technically
>> discussed on the mailing list? Or should we just leave this important
>> problem unfixed?
>
> I think it's almost always better to solve the problem in its origin.
> Any workaround is unreliable, so we should always try to fix the
> problem at the root before trying the workarounds. Note that we are
> talking about a relatively obvious bug in other Apache's project, not
> about something irrecoverable.
>
> As far as I know, this problem has never been reported to the APR's
> dev list. And this is the first action that we should have done.
>
>> Do you have any suggestions for improvement?
> I have plans to send a patch to APR in order to solve this problem. So
> we don't need to release this workaround.

Okay, I understand this should be fixed in APR.

But should we block a workaround in our code for a bug that causes
problems in the wild for existing svn 1.9 installations? Those users
deserve a fix in a 1.9.x patch release, IMHO. If you can arrange this
by fixing it in APR, creating a patch release of APR, and then
creating a 1.9.x svn release with that fix (in a reasonable
timeframe), then fine. But otherwise I think a svn-internal-workaround
is warranted.

-- 
Johan

Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS

[Ivan Zhakov]

On 12 October 2016 at 22:30, Johan Corveleyn  wrote:
> On Wed, Oct 12, 2016 at 10:13 PM,  wrote:
>>
>> Author: ivan
>> Date: Wed Oct 12 20:13:24 2016
>> New Revision: 1764536
>>
>> URL: http://svn.apache.org/viewvc?rev=1764536=rev
>> Log:
>> * STATUS: Veto r1759116 group.
>>
>> Modified:
>> subversion/branches/1.9.x/STATUS
> ...
>> @@ -135,6 +119,24 @@ Candidate changes:
>>  Veto-blocked changes:
>>  =
>>
>> + * r1759116, r1759117, r1759122, r1759123, r1759124
>> +   Fix FSFS format 7 packing for revisions packs with lots of changes.
>> +   Justification:
>> + Problem occurred in at least two user repositories.  Without the fix,
>> + format 7 repositories with an exceptionally large number of changes in
>> + a pack cannot be packed - which renders using f7 pointless for those
>> + users.
>> +   Branch:
>> + ^/subversion/branches/1.9.x-fsfs-pack-fixes
>> +   Notes:
>> + r1759116 adds a workaround for trunc() corrupting apr_file_t internal 
>> state.
>> + r1759117-23 provide the actual fixes.
>> + r1759124 adds a regression test with the necessary internal API 
>> changes.
>> +   Votes:
>> + +1: stefan2
>> + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR,
>> +   not by unreliable workaround with unknown consequences)
>> +
>
[...]
> So that leaves you claiming that it's an "unreliable workaround with
> unknown consequences". Why? Please clarify. We are talking about a
> bugfix which fixes an important problem reported by users. Do you have
> any suggestions for improvement? Perhaps the fix should be technically
> discussed on the mailing list? Or should we just leave this important
> problem unfixed?

I think it's almost always better to solve the problem in its origin.
Any workaround is unreliable, so we should always try to fix the
problem at the root before trying the workarounds. Note that we are
talking about a relatively obvious bug in other Apache's project, not
about something irrecoverable.

As far as I know, this problem has never been reported to the APR's
dev list. And this is the first action that we should have done.

> Do you have any suggestions for improvement?
I have plans to send a patch to APR in order to solve this problem. So
we don't need to release this workaround.

-- 
Ivan Zhakov

[PATCH] Improve error message on copying to unversioned dir

[Patrick Steinhardt]

Hi,

attached a patch to a wrong and confusing error message.

[[
libsvn_client: improve error when copying to unversioned dir

When trying to copy a file to an unversioned (but existing)
directory '/foo/bar', subversion will happily claim that "Path
'/foo/bar' is not a directory", which is obviously wrong. Fix the
issue by instead noticing the user that '/foo/bar' is actually
not a _versioned_ directory.

- subversion/libsvn_client/copy.c:
  * (verify_wc_dsts): improve error messages
- subversion/tests/cmdline/copy_tests.py:
  * (copy_into_missing_dir): adjust expected error message
- subversion/tests/cmdline/input_validation_tests.py:
  * (invalid_copy_target): adjust expected error message
]]

Regards
-- 
Patrick Steinhardt, Entwickler

elego Software Solutions GmbH, http://www.elego.de
Gebäude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany

Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
Handelsregister: Amtsgericht Charlottenburg HRB 77719
Geschäftsführer: Olaf Wagner
diff --git a/subversion/libsvn_client/copy.c b/subversion/libsvn_client/copy.c
index 857d340..1fded02 100644
--- a/subversion/libsvn_client/copy.c
+++ b/subversion/libsvn_client/copy.c
@@ -1065,7 +1065,7 @@ verify_wc_dsts(const apr_array_header_t *copy_pairs,
   else if (dst_parent_kind != svn_node_dir)
 {
   return svn_error_createf(SVN_ERR_WC_PATH_NOT_FOUND, NULL,
-   _("Path '%s' is not a directory"),
+   _("Path '%s' is not a versioned directory"),
svn_dirent_local_style(
  pair->dst_parent_abspath, scratch_pool));
 }
@@ -1075,7 +1075,7 @@ verify_wc_dsts(const apr_array_header_t *copy_pairs,
 
   if (dst_parent_kind != svn_node_dir)
 return svn_error_createf(SVN_ERR_WC_MISSING, NULL,
- _("Path '%s' is not a directory"),
+ _("Path '%s' is not a versioned directory"),
  svn_dirent_local_style(
  pair->dst_parent_abspath, scratch_pool));
 }
diff --git a/subversion/tests/cmdline/copy_tests.py b/subversion/tests/cmdline/copy_tests.py
index 3bb8599..055d88b 100755
--- a/subversion/tests/cmdline/copy_tests.py
+++ b/subversion/tests/cmdline/copy_tests.py
@@ -3864,7 +3864,7 @@ def copy_into_missing_dir(sbox):
   # svn: Error processing command 'modify-entry' in '.'
   # svn: Error modifying entry for 'A'
   # svn: Entry 'A' is already under version control
-  svntest.actions.run_and_verify_svn(None, ".*: Path '.*' is not a directory",
+  svntest.actions.run_and_verify_svn(None, ".*: Path '.*' is not a versioned directory",
  'cp', iota_path, A_path)
 
   # 'cleanup' should not error.
diff --git a/subversion/tests/cmdline/input_validation_tests.py b/subversion/tests/cmdline/input_validation_tests.py
index e1d74a6..48ee5bb 100755
--- a/subversion/tests/cmdline/input_validation_tests.py
+++ b/subversion/tests/cmdline/input_validation_tests.py
@@ -104,7 +104,7 @@ def invalid_copy_target(sbox):
   sbox.build(read_only=True)
   mu_path = os.path.join('A', 'mu')
   C_path = os.path.join('A', 'C')
-  run_and_verify_svn_in_wc(sbox, "svn: E155(007|010): Path '.*' is not a directory",
+  run_and_verify_svn_in_wc(sbox, "svn: E155(007|010): Path '.*' is not a versioned directory",
'copy', mu_path, C_path, "iota")
 
 def invalid_delete_targets(sbox):


signature.asc
Description: PGP signature

Re: Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)

[Johan Corveleyn]

On Thu, Oct 13, 2016 at 1:05 PM, Branko Čibej  wrote:
> On 13.10.2016 13:01, Branko Čibej wrote:
>> On 13.10.2016 11:39, Ivan Zhakov wrote:
>>> On 13 October 2016 at 10:59,   wrote:
 Author: jcorvel
 Date: Thu Oct 13 08:59:07 2016
 New Revision: 1764631

 URL: http://svn.apache.org/viewvc?rev=1764631=rev
 Log:
 Resurrect the '1.9.x-r1721488' branch, to give backport.pl another
 chance to execute the correct backport commands, after backport mess.

>>> I'm wondering if we really need backport.pl running as cronjob to
>>> merge backports automatically to the stable branches? It's not a first
>>> time when this automatic job performs invalid merges. And as far I
>>> understand we still spend some time to babysit this tool, fix bugs
>>> etc. What is wrong with old proven process by merging revisions
>>> manually?
>> It doesn't happen all that often that backport.pl makes a mistake. I bet
>> manual merging would be just as error-prone.
>>
>> Backporting is a well-defined process. The best possible way to document
>> a process is to automate it. Errors will happen but that's no reason to
>> revert to PEBKAC; bugs can be fixed.
>
>
> In fact, now that I've read Johan's mail ... it /was/ a PEBKAC and
> nothing was wrong with backport.pl.

Indeed, what we experienced last night was not a script bug (unless if
you state that it should have protected against that race condition
:-)). It was the first time the script was run from qavm3, so I was
paying extra attention to potential problems.

Anyway, it's not like we will migrate to a new machine every month, so
it's quite an exceptional situation ...

-- 
Johan

Re: Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)

[Branko Čibej]

On 13.10.2016 13:01, Branko Čibej wrote:
> On 13.10.2016 11:39, Ivan Zhakov wrote:
>> On 13 October 2016 at 10:59,   wrote:
>>> Author: jcorvel
>>> Date: Thu Oct 13 08:59:07 2016
>>> New Revision: 1764631
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1764631=rev
>>> Log:
>>> Resurrect the '1.9.x-r1721488' branch, to give backport.pl another
>>> chance to execute the correct backport commands, after backport mess.
>>>
>> I'm wondering if we really need backport.pl running as cronjob to
>> merge backports automatically to the stable branches? It's not a first
>> time when this automatic job performs invalid merges. And as far I
>> understand we still spend some time to babysit this tool, fix bugs
>> etc. What is wrong with old proven process by merging revisions
>> manually?
> It doesn't happen all that often that backport.pl makes a mistake. I bet
> manual merging would be just as error-prone.
>
> Backporting is a well-defined process. The best possible way to document
> a process is to automate it. Errors will happen but that's no reason to
> revert to PEBKAC; bugs can be fixed.


In fact, now that I've read Johan's mail ... it /was/ a PEBKAC and
nothing was wrong with backport.pl.

-- Brane

Re: Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)

[Branko Čibej]

On 13.10.2016 11:39, Ivan Zhakov wrote:
> On 13 October 2016 at 10:59,   wrote:
>> Author: jcorvel
>> Date: Thu Oct 13 08:59:07 2016
>> New Revision: 1764631
>>
>> URL: http://svn.apache.org/viewvc?rev=1764631=rev
>> Log:
>> Resurrect the '1.9.x-r1721488' branch, to give backport.pl another
>> chance to execute the correct backport commands, after backport mess.
>>
> I'm wondering if we really need backport.pl running as cronjob to
> merge backports automatically to the stable branches? It's not a first
> time when this automatic job performs invalid merges. And as far I
> understand we still spend some time to babysit this tool, fix bugs
> etc. What is wrong with old proven process by merging revisions
> manually?

It doesn't happen all that often that backport.pl makes a mistake. I bet
manual merging would be just as error-prone.

Backporting is a well-defined process. The best possible way to document
a process is to automate it. Errors will happen but that's no reason to
revert to PEBKAC; bugs can be fixed.

-- Brane

Re: Status review for 1.8.17 and 1.9.5

[Stefan Fuhrmann]

On 12.10.2016 22:22, Evgeny Kotkov wrote:

Stefan Fuhrmann  writes:


There are a number of backport proposals that only need one more +1.
Since the Berlin hackathon is going on and we feel like preparing the
next patch release(s) soon-ish, please review and vote.

I plan to roll 1.9.5 after the hackathon.

+1.

   Any reason why we might also
want to prepare 1.8.17?  I don't think that we had any security or critical
bugfixes in the 1.8.x branch.

Nothing really critical, IIRC. But it has been 6 months
by now with no bugfix release. So, it would be nice
to demonstrate that 1.8.x is still being supported ...

-- Stefan^2.

Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)

[Ivan Zhakov]

On 13 October 2016 at 10:59,   wrote:
> Author: jcorvel
> Date: Thu Oct 13 08:59:07 2016
> New Revision: 1764631
>
> URL: http://svn.apache.org/viewvc?rev=1764631=rev
> Log:
> Resurrect the '1.9.x-r1721488' branch, to give backport.pl another
> chance to execute the correct backport commands, after backport mess.
>
I'm wondering if we really need backport.pl running as cronjob to
merge backports automatically to the stable branches? It's not a first
time when this automatic job performs invalid merges. And as far I
understand we still spend some time to babysit this tool, fix bugs
etc. What is wrong with old proven process by merging revisions
manually?

-- 
Ivan Zhakov

Re: [PATCH] Resolve issue #4647 on trunk (v2)

[Stefan]

On 10/13/2016 11:08 AM, Stefan wrote:
> On 10/10/2016 11:39 PM, Stefan wrote:
>> On 10/10/2016 6:12 PM, Ivan Zhakov wrote:
>>> On 10 October 2016 at 17:53, Stefan  wrote:
 On 8/28/2016 11:32 PM, Bert Huijben wrote:
>> -Original Message-
>> From: Daniel Shahaf [mailto:d...@daniel.shahaf.name]
>> Sent: zondag 28 augustus 2016 20:23
>> To: Stefan 
>> Cc: dev@subversion.apache.org
>> Subject: Re: [PATCH] Fix a conflict resolution issue related to binary 
>> files (patch
>> v4)
>>
>> Stefan wrote on Sun, Aug 28, 2016 at 13:31:39 +0200:
>>> The regression test was tested against 1.9.4, 1.9.x and trunk r1743999.
>>>
>>> I also tried to run the test against 1.8.16 but there it fails (didn't
>>> investigate in detail).
>>> Trunk r1758069 caused some build issues on my machine. Therefore I
>>> couldn't validate/check the patch against the latest trunk (maybe it's
>>> just some local issue with my build machine rather than some actual
>>> problem on trunk - didn't look into that yet).
>> For future reference, you might have tried building trunk@HEAD after
>> locally reverting r1758069; i.e.:
>>
>> svn up
>> svn merge -c -r1758069
>> 
>> make check
>>
>> Stefan wrote on Sun, Aug 28, 2016 at 18:33:55 +0200:
>>> Got approved by Bert.
>>>
>> (Thanks for stating so on the thread.)
>>
>>> Separated the repro test from the actual fix in order to have the
>>> possibility to selectively only backport the regression test to the 1.8
>>> branch.
>> Good call, but the fix and the "remove XFail markers" (r1758129 and
>> r1758130) should have been done in a single revision: they _are_
>> a single logical change.  That would also avoid breaking 'make check'
>> (at r1758129 'make check' exits non-zero because of the XPASS).
> I do this the same way sometimes, when I want to use the separate 
> revision for backporting... But usually I commit things close enough that 
> nobody notices the bot results ;-)
> (While the initial XFail addition is still running, you can commit the 
> two follow ups, and the buildbots collapses all the changes to a single 
> build)
>
> I just committed the followup patch posted in another thread to unbreak 
> the bots for the night...
>
>   Bert
 Attached is a patch which should resolve the test case you added, Bert.
 Anybody feels like approving it? Or is there something I should
 improve/change?

 [[[

 Add support for the svn_client_conflict_option_working_text resolution for
 binary file conflicts.

 * subversion/libsvn_client/conflicts.c
   (): Add svn_client_conflict_option_working_text to 
 binary_conflict_options

 * subversion/tests/cmdline/resolve_tests.py
   (automatic_binary_conflict_resolution): Remove XFail marker

 ]]]

>>> It seems this patch breaks interactive conflict resolve:
>>> With trunk I get the following to 'svn resolve' on binary file:
>>> [[[
>>> Merge conflict discovered in binary file 'A_COPY\theta'.
>>> Select: (p) postpone,
>>> (r) accept binary file as it appears in the working copy,
>>> (tf) accept incoming version of binary file: h
>>>
>>>   (p)  - skip this conflict and leave it unresolved  [postpone]
>>>   (tf) - accept incoming version of binary file  [theirs-full]
>>>   (r)  - accept binary file as it appears in the working copy  [working]
>>>   (q)  - postpone all remaining conflicts
>>> ]]]
>>>
>>> But with patch I get the following:
>>> [[[
>>> Merge conflict discovered in binary file 'A_COPY\theta'.
>>> Select: (p) postpone,
>>> (r) accept binary file as it appears in the working copy,
>>> (tf) accept incoming version of binary file: h
>>>
>>>   (p)  - skip this conflict and leave it unresolved  [postpone]
>>>   (tf) - accept incoming version of binary file  [theirs-full]
>>>   (mf) - accept binary file as it appears in the working copy  [mine-full]
>>>   (r)  - accept binary file as it appears in the working copy  [working]
>>>   (q)  - postpone all remaining conflicts
>>> ]]]
>>>
>>> I think it's confusing and we should not offer the same option twice.
>>>
>> Completely agreed. The display of the option in the UI shouldn't be like
>> that. Certainly an oversight on my side. Will revise the patch and come
>> up with a different/better approach tomorrow.
>>
>> Regards,
>> Stefan
> Trying to put together a revised patch without the issue. The attached
> patch fixes the 4647 test but breaks two other tests:
>
> basic#41
> update#38
>
> Still looking into what I'm doing wrong, but any pointers would be much
> appreciated.

Looks like update#38 is actually fixed. Leaving basic#41 broken:
FAIL:  basic_tests.py 41: automatic conflict resolution

Attached is the full test output.

Regards,
Stefan

same: 

Backport mixup last night

[Johan Corveleyn]

Just a quick report about what happened and what I've done (things are
okay now).

There was an svn-role (automatic backport) mixup last night: some
revisions were still correct, but the last 6 automated backport
commits were wrong (mixed STATUS changes with wrong actual changes
etc). Cause: the cronjob of backport.pl was running simultaneously on
svn-qavm2 (old machine) and svn-qavm3 (new machine we're migrating to,
see [1]), creating a race.

For now, I've just renamed the backport.pl script on qavm2, so it
can't be run from cron anymore. I've reverted the incorrect backport
commits, and manually ran the backport.pl script on qavm3 to redo the
correct backports. That seems to have worked out OK, so all is fine
now ... carry on (or: keep calm and resolve conflicts :-).

[1] https://issues.apache.org/jira/browse/INFRA-12289

-- 
Johan

[PATCH] Resolve issue #4647 on trunk (v2)

[Stefan]

On 10/10/2016 11:39 PM, Stefan wrote:
> On 10/10/2016 6:12 PM, Ivan Zhakov wrote:
>> On 10 October 2016 at 17:53, Stefan  wrote:
>>> On 8/28/2016 11:32 PM, Bert Huijben wrote:
> -Original Message-
> From: Daniel Shahaf [mailto:d...@daniel.shahaf.name]
> Sent: zondag 28 augustus 2016 20:23
> To: Stefan 
> Cc: dev@subversion.apache.org
> Subject: Re: [PATCH] Fix a conflict resolution issue related to binary 
> files (patch
> v4)
>
> Stefan wrote on Sun, Aug 28, 2016 at 13:31:39 +0200:
>> The regression test was tested against 1.9.4, 1.9.x and trunk r1743999.
>>
>> I also tried to run the test against 1.8.16 but there it fails (didn't
>> investigate in detail).
>> Trunk r1758069 caused some build issues on my machine. Therefore I
>> couldn't validate/check the patch against the latest trunk (maybe it's
>> just some local issue with my build machine rather than some actual
>> problem on trunk - didn't look into that yet).
> For future reference, you might have tried building trunk@HEAD after
> locally reverting r1758069; i.e.:
>
> svn up
> svn merge -c -r1758069
> 
> make check
>
> Stefan wrote on Sun, Aug 28, 2016 at 18:33:55 +0200:
>> Got approved by Bert.
>>
> (Thanks for stating so on the thread.)
>
>> Separated the repro test from the actual fix in order to have the
>> possibility to selectively only backport the regression test to the 1.8
>> branch.
> Good call, but the fix and the "remove XFail markers" (r1758129 and
> r1758130) should have been done in a single revision: they _are_
> a single logical change.  That would also avoid breaking 'make check'
> (at r1758129 'make check' exits non-zero because of the XPASS).
 I do this the same way sometimes, when I want to use the separate revision 
 for backporting... But usually I commit things close enough that nobody 
 notices the bot results ;-)
 (While the initial XFail addition is still running, you can commit the two 
 follow ups, and the buildbots collapses all the changes to a single build)

 I just committed the followup patch posted in another thread to unbreak 
 the bots for the night...

   Bert
>>> Attached is a patch which should resolve the test case you added, Bert.
>>> Anybody feels like approving it? Or is there something I should
>>> improve/change?
>>>
>>> [[[
>>>
>>> Add support for the svn_client_conflict_option_working_text resolution for
>>> binary file conflicts.
>>>
>>> * subversion/libsvn_client/conflicts.c
>>>   (): Add svn_client_conflict_option_working_text to binary_conflict_options
>>>
>>> * subversion/tests/cmdline/resolve_tests.py
>>>   (automatic_binary_conflict_resolution): Remove XFail marker
>>>
>>> ]]]
>>>
>> It seems this patch breaks interactive conflict resolve:
>> With trunk I get the following to 'svn resolve' on binary file:
>> [[[
>> Merge conflict discovered in binary file 'A_COPY\theta'.
>> Select: (p) postpone,
>> (r) accept binary file as it appears in the working copy,
>> (tf) accept incoming version of binary file: h
>>
>>   (p)  - skip this conflict and leave it unresolved  [postpone]
>>   (tf) - accept incoming version of binary file  [theirs-full]
>>   (r)  - accept binary file as it appears in the working copy  [working]
>>   (q)  - postpone all remaining conflicts
>> ]]]
>>
>> But with patch I get the following:
>> [[[
>> Merge conflict discovered in binary file 'A_COPY\theta'.
>> Select: (p) postpone,
>> (r) accept binary file as it appears in the working copy,
>> (tf) accept incoming version of binary file: h
>>
>>   (p)  - skip this conflict and leave it unresolved  [postpone]
>>   (tf) - accept incoming version of binary file  [theirs-full]
>>   (mf) - accept binary file as it appears in the working copy  [mine-full]
>>   (r)  - accept binary file as it appears in the working copy  [working]
>>   (q)  - postpone all remaining conflicts
>> ]]]
>>
>> I think it's confusing and we should not offer the same option twice.
>>
> Completely agreed. The display of the option in the UI shouldn't be like
> that. Certainly an oversight on my side. Will revise the patch and come
> up with a different/better approach tomorrow.
>
> Regards,
> Stefan

Trying to put together a revised patch without the issue. The attached
patch fixes the 4647 test but breaks two other tests:

basic#41
update#38

Still looking into what I'm doing wrong, but any pointers would be much
appreciated.

Regards,
Stefan

Index: subversion/libsvn_client/conflicts.c
===
--- subversion/libsvn_client/conflicts.c(revision 1764448)
+++ subversion/libsvn_client/conflicts.c(working copy)
@@ -7121,7 +7121,7 @@
 resolve_text_conflict);