I think there is a bug in "rbt post" in generating the diff from perforce 
for a submitted change list that contains a delete. The most recent version 
of the file in the submitted change list output from perforce is the delete.

I think the issue begins in _compute_range_changes in perforce.py

What I believe happens is the code in perforce.py runs "p4 filelog 
"//...@<changeno>,<changeno>" to get the list of changes from the change 
list I supplied.

It iterates through the list of changes, parsing out the depot files, 
action and locations.

_accumulate_range_change is called for each file. If "old_action" is edit, 
then it decrements initialRev. This is therefore not done for a delete, so 
the value of initialRev remains as the last (deleted) version of the file:-

    def _accumulate_range_change(self, file_entry, change):
        """Compute the effects of a given change on a given file"""
        old_action = file_entry['action']
        current_action = change['action']

        if old_action == 'none':
            # This is the first entry for this file.
            new_action = current_action
            file_entry['depotFile'] = file_entry['initialDepotFile']

            # If the first action was an edit, then the initial revision
            # (that we'll use to generate the diff) is n-1
            if current_action == 'edit':
                file_entry['initialRev'] -= 1

It then iterates through the built list of files and if action is delete, 
does this:-

            elif action == 'delete':
                    old_file, new_file = self._extract_delete_files(
                        initial_depot_file, initial_rev)
                except ValueError:
                    logging.warning('Skipping file %s: %s', depot_file, e)

                diff_lines += self._do_diff(
                    old_file, new_file, initial_depot_file, initial_rev,
                    depot_file, 'D', ignore_unmodified=True)

In this case the value passed to extract_delete_files is the deleted 
revision of the file (I.E. the version output from p4 filelog).

    def _extract_delete_files(self, depot_file, revision):
        """Extract the 'old' and 'new' files for a delete operation.

        Returns a tuple of (old filename, new filename). This can raise a
        ValueError if extraction fails.
        # Get the old version out of perforce
        old_filename = make_tempfile()
        self._write_file('%s#%s' % (depot_file, revision), old_filename)

        # Make an empty tempfile for the new file
        new_filename = make_tempfile()

        return old_filename, new_filename

The _write_file method then causes it to execute this command:-

p4 print -o /tmp/tmp-9Bncr -q  //depot/..../DeletedFile#2 (where #2 is the 
deleted revision of the file)

In this case if the temp file did not already exist, perforce does not 
create it. But it was already created by make_tempfile (and was empty). So 
the result is we still have an empty file. The new file is also created 
with make_tempfile and hence is empty.

This means that when the diff is run, it ends up comparing two empty files. 
It then of course says there is no change and so the file that is deleted 
ends up NOT getting included in the diff that rbt uploads. I think this is 
an error and in the old_filename code above the depot revision needs to be 
decreased by 1. This will cause "p4 print" to output the old version of the 
file (prior to the delete). Then when the diff runs, it compares the old 
(pre delete) version of the file with an empty file and then correctly logs 
this is a delete in the diff it creates.

I am not sure if the best place to fix this is in _extract_delete_files (to 
reduce the depot revision by one) or to change _accumulate_range_change to 
decrement the initialVersion for both edit and delete. 

Could you please confirm I am correct on this. If so, shall I raise a bug 
report in the bug tracker? The behaviour might differ for a shelved change 
but it certainly happens for a submitted change.


Get the Review Board Power Pack at http://www.reviewboard.org/powerpack/
Sign up for Review Board hosting at RBCommons: https://rbcommons.com/
Happy user? Let us know at http://www.reviewboard.org/users/
You received this message because you are subscribed to the Google Groups 
"reviewboard" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to