When directories are moved or deleted, files will be duplicated in svn-create-patch. Prevent this duplication.
<rdar://problem/33226781>
Note that this change must be made before The WebKit2 -> WebKit and WebKit -> WebKitLegacy change is made.
Created attachment 315058 [details] Patch
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().
(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.
Created attachment 315143 [details] Patch
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
Comment on attachment 315143 [details] Patch Tentative r=me, but all comments from Attachment 315058 [details] still apply.
(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.
(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'?
(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.
Created attachment 315174 [details] Patch
Comment on attachment 315174 [details] Patch Clearing flags on attachment: 315174 Committed r219376: <http://trac.webkit.org/changeset/219376>
All reviewed patches have been landed. Closing bug.