Bug 174339 – Do not duplicate files when deleting directories with svn 1.9

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

When directories are moved or deleted, files will be duplicated in svn-create-patch.  Prevent this duplication.

[Radar WebKit Bug Importer]

<rdar://problem/33226781>

[Jonathan Bedard]

Note that this change must be made before The WebKit2 -> WebKit and WebKit -> WebKitLegacy change is made.

[Jonathan Bedard]

Created attachment 315058 [details]
Patch

[Sam Weinig]

Comment on attachment 315058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315058&action=review

> Tools/ChangeLog:3
> +        Need a short description (OOPS!).

Please add a descriptive title.

> Tools/ChangeLog:10
> +        (diffOptionsForFile): No longer pass -N option, since this does not work in SVN 1.9.4.

Should this be predicated on the svn version?  I know we support older versions of subversion, since we have checks like isSVNVersion16OrNewer().

[Jonathan Bedard]

(In reply to Sam Weinig from comment #4)
> Comment on attachment 315058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315058&action=review
> 
> > Tools/ChangeLog:3
> > +        Need a short description (OOPS!).
> 
> Please add a descriptive title.
> 
> > Tools/ChangeLog:10
> > +        (diffOptionsForFile): No longer pass -N option, since this does not work in SVN 1.9.4.
> 
> Should this be predicated on the svn version?  I know we support older
> versions of subversion, since we have checks like isSVNVersion16OrNewer().

This will still work with older versions of svn.  What this patch does is run a diff on the highest level directory deleted instead of each child.  So for example, given the following:

D folder1
D folder1/subfolder
D folder1/file1
D folder1/subfolder/file2

we would only run 'svn diff' on folder1.

[Jonathan Bedard]

Created attachment 315143 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 315058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315058&action=review

Tentative r=me if this has been tested with older svn versions, and you change the $flag variable name to describe why a path is being skipped.

However, I'd like to understand why --depth=XYZ won't work to replace -N for svn v1.9.x.

>> Tools/ChangeLog:10
>> +        (diffOptionsForFile): No longer pass -N option, since this does not work in SVN 1.9.4.
> 
> Should this be predicated on the svn version?  I know we support older versions of subversion, since we have checks like isSVNVersion16OrNewer().

Technically, the -N switch doesn't work because it was purposely deprecated.  (This makes it sound like it was broken by accident.)

> Tools/Scripts/svn-create-patch:278
> +        # svn diff -N doesn't work on svn 1.9, so only return top-level deletions.
> +        if ($modificationType eq "deletion") {
> +            push @deletedFiles, $path;
> +            next;
> +        }

Has this change been tested with older versions of svn like v1.7.x or v1.8.x?

Looking at "svn help diff", I see that -N is deprecated in svn v1.9.x:

  -N [--non-recursive]     : obsolete; try --depth=files or --depth=immediates
  --depth ARG              : limit operation by depth ARG ('empty', 'files', 'immediates', or 'infinity')

Can the --depth=XYZ switch be used instead of -N on svn v1.9.x and later?

(I could probably answer this if I was fully paying attention when you were talking to Matt yesterday.  Sorry.)

It seems like --depth=files might do what you want by listing all deleted files under the deleted directory, though.

> Tools/Scripts/svn-create-patch:297
> +        my $flag = 0;

Need a more descriptive name than "flag" for this variable to describe why the path is being skipped.  Maybe something like these (whichever makes the most sense):

$duplicatePathFound
$parentDirectoryFound
$parentPathFound

[David Kilzer (:ddkilzer)]

Comment on attachment 315143 [details]
Patch

Tentative r=me, but all comments from Attachment 315058 [details] still apply.

[Jonathan Bedard]

(In reply to David Kilzer (:ddkilzer) from comment #7)
> Comment on attachment 315058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315058&action=review
> 
> Tentative r=me if this has been tested with older svn versions, and you
> change the $flag variable name to describe why a path is being skipped.
> 
> However, I'd like to understand why --depth=XYZ won't work to replace -N for
> svn v1.9.x.
> 
> >> Tools/ChangeLog:10
> >> +        (diffOptionsForFile): No longer pass -N option, since this does not work in SVN 1.9.4.
> > 
> > Should this be predicated on the svn version?  I know we support older versions of subversion, since we have checks like isSVNVersion16OrNewer().
> 
> Technically, the -N switch doesn't work because it was purposely deprecated.
> (This makes it sound like it was broken by accident.)
> 
> > Tools/Scripts/svn-create-patch:278
> > +        # svn diff -N doesn't work on svn 1.9, so only return top-level deletions.
> > +        if ($modificationType eq "deletion") {
> > +            push @deletedFiles, $path;
> > +            next;
> > +        }
> 
> Has this change been tested with older versions of svn like v1.7.x or v1.8.x?

I will double-check it against svn 1.7 and 1.8.  I don't expect this will cause any problems because it is essentially just using svn's recursion instead of us implementing our own.
 
> Looking at "svn help diff", I see that -N is deprecated in svn v1.9.x:
> 
>   -N [--non-recursive]     : obsolete; try --depth=files or
> --depth=immediates
>   --depth ARG              : limit operation by depth ARG ('empty', 'files',
> 'immediates', or 'infinity')
> 
> Can the --depth=XYZ switch be used instead of -N on svn v1.9.x and later?
> 
> (I could probably answer this if I was fully paying attention when you were
> talking to Matt yesterday.  Sorry.)
> 
> It seems like --depth=files might do what you want by listing all deleted
> files under the deleted directory, though.

I'm pretty sure -N was broken by accident.  The reason I say this is that --depth doesn't work either.  In fact, '--depth empty,' '--depth infinity' and '--depth immediates' all behave the way I would expect '--depth infinity' to behave.

> 
> > Tools/Scripts/svn-create-patch:297
> > +        my $flag = 0;
> 
> Need a more descriptive name than "flag" for this variable to describe why
> the path is being skipped.  Maybe something like these (whichever makes the
> most sense):
> 
> $duplicatePathFound
> $parentDirectoryFound
> $parentPathFound

I'll update flag to a more reasonable name.

[David Kilzer (:ddkilzer)]

(In reply to Jonathan Bedard from comment #9)
> (In reply to David Kilzer (:ddkilzer) from comment #7)
> > Comment on attachment 315058 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=315058&action=review
> > 
> > Tentative r=me if this has been tested with older svn versions, and you
> > change the $flag variable name to describe why a path is being skipped.
> > 
> > However, I'd like to understand why --depth=XYZ won't work to replace -N for
> > svn v1.9.x.
> > 
> > >> Tools/ChangeLog:10
> > >> +        (diffOptionsForFile): No longer pass -N option, since this does not work in SVN 1.9.4.
> > > 
> > > Should this be predicated on the svn version?  I know we support older versions of subversion, since we have checks like isSVNVersion16OrNewer().
> > 
> > Technically, the -N switch doesn't work because it was purposely deprecated.
> > (This makes it sound like it was broken by accident.)
> > 
> > > Tools/Scripts/svn-create-patch:278
> > > +        # svn diff -N doesn't work on svn 1.9, so only return top-level deletions.
> > > +        if ($modificationType eq "deletion") {
> > > +            push @deletedFiles, $path;
> > > +            next;
> > > +        }
> > 
> > Has this change been tested with older versions of svn like v1.7.x or v1.8.x?
> 
> I will double-check it against svn 1.7 and 1.8.  I don't expect this will
> cause any problems because it is essentially just using svn's recursion
> instead of us implementing our own.
>  
> > Looking at "svn help diff", I see that -N is deprecated in svn v1.9.x:
> > 
> >   -N [--non-recursive]     : obsolete; try --depth=files or
> > --depth=immediates
> >   --depth ARG              : limit operation by depth ARG ('empty', 'files',
> > 'immediates', or 'infinity')
> > 
> > Can the --depth=XYZ switch be used instead of -N on svn v1.9.x and later?
> > 
> > (I could probably answer this if I was fully paying attention when you were
> > talking to Matt yesterday.  Sorry.)
> > 
> > It seems like --depth=files might do what you want by listing all deleted
> > files under the deleted directory, though.
> 
> I'm pretty sure -N was broken by accident.  The reason I say this is that
> --depth doesn't work either.  In fact, '--depth empty,' '--depth infinity'
> and '--depth immediates' all behave the way I would expect '--depth
> infinity' to behave.

This is really picking nits, but does svn care if you use '--depth=files' vs. '--depth files'?

[Jonathan Bedard]

(In reply to David Kilzer (:ddkilzer) from comment #10)
> ...
> 
> This is really picking nits, but does svn care if you use '--depth=files'
> vs. '--depth files'?

No, it doesn't care which format.

And actually, when testing both formats, I learned that SVN is respecting those flags, at least partially.  But it isn't sufficient for our use cases.  Using this example:

D containing/folder1
D containing/folder1/subfolder
D containing/folder1/file1
D containing/folder1/subfolder/file2

'svn diff --diff-cmd diff -x -uap --depth=empty containing' will return nothing, as expected.  However, 'svn diff --diff-cmd diff -x -uap --depth=empty containing/folder1/subfolder' will contain the diff of the removed file2.

[Jonathan Bedard]

Created attachment 315174 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 315174 [details]
Patch

Clearing flags on attachment: 315174

Committed r219376: <http://trac.webkit.org/changeset/219376>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.