1.8.x backport r1619380 - diff locally copied dir with props

[Julian Foad <ju...@btopenworld.com>]

This issue is proposed for backport:

 * r1619380, r1619393 
   Fix diff of a locally copied directory with props: it showed all props
   as added instead of a diff against the copy-from props.
   Justification:
     Behaviour regression introduced in 1.8.0.
   Notes:
     r1619380 is the fix; r1619393 a test for it.
     The test on trunk@1619393 is tweaked to account for a trunk bug in the 
     display of diff headers; the backport branch provides the correct
     version for 1.8.x.
   Branch:
     ^/subversion/branches/1.8.x-r1619380
   Votes:
     +1: rhuijben, stefan2
     -0: julianfoad (assertion failure on incomplete dir -- see email)


But the code it introduces contains an assertion that can fail, as Stefan Sperling reported recently[1], on an 'incomplete' directory such as can result from an aborted update.

> assertion "kind == svn_node_dir && (status == 
svn_wc__db_status_normal || status == svn_wc__db_status_added || (status == svn_wc__db_status_deleted && diff_pristine))" failed: file 
"subversion/libsvn_wc/diff_editor.c", line 1057, function 
"svn_wc__diff_local_only_dir"

To reproduce the conditions, svn_wc__db_read_info() needs to return status=incomplete for a directory that exists in the WC at one end of the requested diff's revision range, but doesn't exist in the repo at the other end of the revision range. (That's the case these "local-only" functions are handling.)

Do we want to:

  * backport first, then investigate and fix this case at leisure (on trunk)
  * hold off the backport until this is investigated and fixed?

?

For the command-line client, I think fixing the currently wrong output outweighs the possible crash in obscure 'incomplete' cases, but for GUIs that priority is probably reversed.

As for a fix, I was the one who inserted that code, but I did it by copying it from the similar ..._local_only_file function just above and didn't know I needed to adjust it. I don't know what should be done with it now, so I need someone else to step in. In any case, whatever is done here, please consider keeping ..._local_only_file() in sync with it.

- Julian


[1] <http://colabti.org/irclogger/irclogger_log/svn-dev?date=2015-02-13#l109>

Re: 1.8.x backport r1619380 - diff locally copied dir with props

[Philip Martin <ph...@wandisco.com>]

Julian Foad <ju...@btopenworld.com> writes:

> Do we want to:
>
>   * backport first, then investigate and fix this case at leisure (on trunk)
>   * hold off the backport until this is investigated and fixed?

Hold off.  Avoid introducing a new bug into the stable code and keep the
existing bug.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*

RE: 1.8.x backport r1619380 - diff locally copied dir with props

[Bert Huijben <be...@qqmail.nl>]

> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: maandag 16 februari 2015 17:41
> To: dev@subversion.apache.org
> Cc: Bert Huijben; Stefan Sperling; Stefan Fuhrmann
> Subject: 1.8.x backport r1619380 - diff locally copied dir with props
> 
> This issue is proposed for backport:
> 
>  * r1619380, r1619393
>    Fix diff of a locally copied directory with props: it showed all props
>    as added instead of a diff against the copy-from props.
>    Justification:
>      Behaviour regression introduced in 1.8.0.
>    Notes:
>      r1619380 is the fix; r1619393 a test for it.
>      The test on trunk@1619393 is tweaked to account for a trunk bug in
the
>      display of diff headers; the backport branch provides the correct
>      version for 1.8.x.
>    Branch:
>      ^/subversion/branches/1.8.x-r1619380
>    Votes:
>      +1: rhuijben, stefan2
>      -0: julianfoad (assertion failure on incomplete dir -- see email)
> 
> 
> But the code it introduces contains an assertion that can fail, as Stefan
Sperling
> reported recently[1], on an 'incomplete' directory such as can result from
an
> aborted update.
> 
> > assertion "kind == svn_node_dir && (status ==
> svn_wc__db_status_normal || status == svn_wc__db_status_added || (status
> == svn_wc__db_status_deleted && diff_pristine))" failed: file
> "subversion/libsvn_wc/diff_editor.c", line 1057, function
> "svn_wc__diff_local_only_dir"
> 
> To reproduce the conditions, svn_wc__db_read_info() needs to return
> status=incomplete for a directory that exists in the WC at one end of the
> requested diff's revision range, but doesn't exist in the repo at the
other end of
> the revision range. (That's the case these "local-only" functions are
handling.)
> 
> Do we want to:
> 
>   * backport first, then investigate and fix this case at leisure (on
trunk)
>   * hold off the backport until this is investigated and fixed?
> 
> ?
> 
> For the command-line client, I think fixing the currently wrong output
> outweighs the possible crash in obscure 'incomplete' cases, but for GUIs
that
> priority is probably reversed.

The Windows GUIs don't care about assert() as those are compiled away when
compiling in release mode on Windows. (See old mails from Stefan Kung and
me)

assert() is typically used as maintainer mode helper, while SVN_ERR_ASSERT()
is used for assertions that also happen at runtime in production code.

All assertions in diff_editor.c are just checks for preconditions that are
later also checked via if statements. 


The incomplete state itself says that you can't trust the list of children
nor the list of properties on the affected nodes... So you can't trust the
diff of those either. Producing a proper error message is probably the best
thing we can do here.

	Bert