Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+92) |
Add extra context for added and deleted empty files.
Review Request #5802 — Created May 12, 2014 and submitted
Information | |
---|---|
anselina | |
RBTools | |
master | |
5785 | |
fb2d6fd... | |
Reviewers | |
rbtools | |
Review Board could not handle added or deleted empty files in SVN, Mercurial,
and Perforce diffs since the diff output fromsvn diff
/hg diff
did not
provide enough context on empty files. This change adds extra processing in
RBTools to generate a diff with extra information on any added or deleted empty
files, if the Review Board server has theempty_files
capability for the
SCM tool in question.The original SVN diff output of an added/deleted empty file was:
Index: foo ===================================================================
After this change, the SVN diff of an added empty file becomes:
Index: foo\t(added) =================================================================== --- foo\t(<base_revision>) +++ foo\t(<tip_revision>)
For Mercurial, the diff of an added empty file is now:
diff -r <base_revision> -r <tip_revision> foo --- /dev/null\tThu Jan 01 00:00:00 1970 +0000 +++ b/foo\t<date>
For Perforce, the diff of an added empty file is:
==== //depot/foo#0 ==A== //depot/foo ====
Ran
rbt post
on a RB server with theempty_files
capabilities with different
SVN, Mercurial, and Perforce diffs, and verified that the modified diffs were
as expected:
- Diff with 1 added empty file
- Diff with 1 added empty file, 1 added non-empty file
- Diff with 1 deleted empty file
- Diff with 1 deleted empty file, 1 deleted non-empty file
- Diff with 1 added empty file, 1 deleted empty file
- Diff with 1 added empty file, 1 added non-empty file, 1 deleted empty file,
1 deleted non-empty file, 1 modified fileRan
rbt post
on a RB server without theempty_files
capabilities, and
verified that the generated diffs did not include any extra information on
added/deleted empty files. These added/deleted empty files were not included in
the review requests.Ran unit tests. More testing was done in /r/5785 as well.
Description | From | Last Updated |
---|---|---|
It would be nice to use re.compile() outside the for loop, so it won't recompile the regex each iteration. |
|
|
It would be nice to use re.compile() outside the for loop, so it won't recompile the regex each iteration. |
|
|
'files' will always be a list, so you can just use the list comprehension version and skip the if/else. If … |
|
|
Should this be _handle_empty_files? |
|
|
Put spaces around the +s |
|
|
This could be if dl == [] and not is_move and changetype_short not in ('A', 'D'): |
|
|
Col: 5 E129 visually indented line with same indent as next logical line |
![]() |
|
I dare say that parsing diffs should never be the answer. When you emailed me, I neglected to tell you … |
IN indygreg | |
I would use hg locate to obtain the set of all files in each changeset. Take the set difference to … |
IN indygreg | |
You should be using :: instead of : for the revision set. : is revision order (order changesets were added … |
IN indygreg | |
I would return a set from _get_files_in_changeset() and use the overloaded operators on set. e.g. added_empty_files = tip_empty_files - base_files |
IN indygreg | |
This logic is kind of confusing. How about this: is_empty_and_changed = (self._supports_empty_files() and changetype_short in ('A', 'D')) if dl == … |
|
Change Summary:
Add support for Mercurial.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+167) |
-
-
rbtools/clients/mercurial.py (Diff revision 3) It would be nice to use
re.compile()
outside the for loop, so it won't recompile the regex each iteration. -
rbtools/clients/mercurial.py (Diff revision 3) It would be nice to use
re.compile()
outside the for loop, so it won't recompile the regex each iteration. -
rbtools/clients/mercurial.py (Diff revision 3) 'files' will always be a list, so you can just use the list comprehension version and skip the if/else. If files is empty, it's the same as returning
[]
-
-
Change Summary:
Fix review issues, and add support for Perforce.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+169 -1) |

-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: rbtools/clients/svn.py rbtools/clients/mercurial.py rbtools/clients/perforce.py Ignored Files:
-
rbtools/clients/perforce.py (Diff revision 4) Col: 5 E129 visually indented line with same indent as next logical line

-
This is a review from Review Bot. Tool: Pyflakes Processed Files: rbtools/clients/svn.py rbtools/clients/mercurial.py rbtools/clients/perforce.py Ignored Files:
-
-
rbtools/clients/perforce.py (Diff revision 4) This could be
if dl == [] and not is_move and changetype_short not in ('A', 'D'):
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+167 -1) |

-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: rbtools/clients/svn.py rbtools/clients/mercurial.py rbtools/clients/perforce.py Ignored Files:

-
This is a review from Review Bot. Tool: Pyflakes Processed Files: rbtools/clients/svn.py rbtools/clients/mercurial.py rbtools/clients/perforce.py Ignored Files:
-
I only looked at the Mercurial changes.
I would rewrite things in terms of
hg locate
. I believe the implementation will be much cleaner.I apologize for not informing you of the existence of filesets via email. It simply slipped my mind. I didn't intend to make you look bad. If another problem I was trying to solve hadn't required me to look at the help page for filesets and see it offers a size() query, I likely wouldn't have remembered at all!
-
rbtools/clients/mercurial.py (Diff revision 5) For performance reasons, it's unfortunate we have to invoke
hg
more than once to obtain this data.That being said, this will only be an issue on very large repositories (like Firefox's). We already have enough performance problems with rbt on large repositories. I wouldn't worry too much about it.
-
rbtools/clients/mercurial.py (Diff revision 5) I dare say that parsing diffs should never be the answer.
When you emailed me, I neglected to tell you about a Mercurial feature called filesets. (See http://www.selenic.com/hg/help/filesets). Essentially, any command that takes file arguments can take these fileset queries to dynamically query Mercurial.
In the case of file adds, we can look for 0-size files in an arbitrary changeset:
$ hg locate -r <rev> 'set:size(0)'
So, you can take the output of
hg log --template '{join(file_adds, "\n")}' -r A::B
and intersect that set with the output ofhg locate
from above to find added empty files. This should perform much better than generating a diff (which is relatively slow for Mercurial). Alternatively, you can take the output ofhg locate
for the base and tip revisions and find added entries manually. This may be better than parsinghg log
output. -
rbtools/clients/mercurial.py (Diff revision 5) I would use
hg locate
to obtain the set of all files in each changeset. Take the set difference to obtain deleted files. Limit the query of the base revision with 'set:size(0)' to only show files that were empty initially. -
rbtools/clients/mercurial.py (Diff revision 5) You should be using :: instead of : for the revision set. : is revision order (order changesets were added to Mercurial). :: is DAG order. : may include changesets from unrelated branches/heads.
Change Summary:
Handle empty files in Mercurial using
hg locate
instead.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+160 -1) |

-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: rbtools/clients/svn.py rbtools/clients/mercurial.py rbtools/clients/perforce.py Ignored Files:

-
This is a review from Review Bot. Tool: Pyflakes Processed Files: rbtools/clients/svn.py rbtools/clients/mercurial.py rbtools/clients/perforce.py Ignored Files:

-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: rbtools/clients/svn.py rbtools/clients/mercurial.py rbtools/clients/perforce.py Ignored Files:

-
This is a review from Review Bot. Tool: Pyflakes Processed Files: rbtools/clients/svn.py rbtools/clients/mercurial.py rbtools/clients/perforce.py Ignored Files:
-
The Mercurial logic looks sound.
-
rbtools/clients/mercurial.py (Diff revision 6) I would return a set from _get_files_in_changeset() and use the overloaded operators on set.
e.g.
added_empty_files = tip_empty_files - base_files
Change Summary:
Return a set in
_get_files_in_changeset()
instead.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+160 -1) |
Testing Done: |
|
---|
-
What happens if you use rbt post with this code on a review board server that doesn't have /r/5785? Does it still parse the diffs OK?
Change Summary:
Check if the RB server has the empty_files capability before handling added/deleted empty files.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 8 (+190 -1) |

-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: rbtools/clients/svn.py rbtools/clients/mercurial.py rbtools/clients/perforce.py Ignored Files:

-
This is a review from Review Bot. Tool: Pyflakes Processed Files: rbtools/clients/svn.py rbtools/clients/mercurial.py rbtools/clients/perforce.py Ignored Files:
-
-
rbtools/clients/perforce.py (Diff revision 8) This logic is kind of confusing. How about this:
is_empty_and_changed = (self._supports_empty_files() and changetype_short in ('A', 'D')) if dl == [] and (is_move or is_empty_and_changed): dl.insert(0, '==== %s#%s ==%s== %s ====\n' % (depot_file, base_revision, changetype_short, new_local_path)) dl.append('\n') else: if ignore_unmodified: return [] else: print 'Warning: %s in your changeset is unmodified' % \ local_path
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+195 -6) |

-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: rbtools/clients/svn.py rbtools/clients/mercurial.py rbtools/clients/perforce.py Ignored Files:

-
This is a review from Review Bot. Tool: Pyflakes Processed Files: rbtools/clients/svn.py rbtools/clients/mercurial.py rbtools/clients/perforce.py Ignored Files: