diff --git a/reviewboard/diffviewer/filediff_creator.py b/reviewboard/diffviewer/filediff_creator.py new file mode 100644 index 0000000000000000000000000000000000000000..aa2e6902b8052ddd4aef302e04d00ca57442cf69 --- /dev/null +++ b/reviewboard/diffviewer/filediff_creator.py @@ -0,0 +1,425 @@ +"""Utilities for creating FileDiffs.""" + +from __future__ import unicode_literals + +import os + +from django.utils.encoding import smart_unicode +from django.utils.translation import ugettext as _ + +from reviewboard.diffviewer.errors import EmptyDiffError +from reviewboard.scmtools.core import FileNotFoundError, PRE_CREATION, UNKNOWN + + +# Extensions used for intelligent sorting of header files +# before implementation files. +_HEADER_EXTENSIONS = ['h', 'H', 'hh', 'hpp', 'hxx', 'h++'] +_IMPL_EXTENSIONS = ['c', 'C', 'cc', 'cpp', 'cxx', 'c++', 'm', 'mm', 'M'] + + +def create_filediffs(diff_file_contents, parent_diff_file_contents, + repository, basedir, base_commit_id, diffset, + request=None, check_existence=True, get_file_exists=None, + diffcommit=None, validate_only=False): + """Create FileDiffs from the given data. + + Args: + diff_file_contents (bytes): + The contents of the diff file. + + parent_diff_file_contents (bytes): + The contents of the parent diff file. + + repository (reviewboard.scmtools.models.Repository): + The repository the diff is being posted against. + + basedir (unicode): + The base directory to prepend to all file paths in the diff. + + base_commit_id (unicode): + The ID of the commit that the diff is based upon. This is + needed by some SCMs or hosting services to properly look up + files, if the diffs represent blob IDs instead of commit IDs + and the service doesn't support those lookups. + + diffset (reviewboard.diffviewer.models.diffset.DiffSet): + The DiffSet to attach the created FileDiffs to. + + request (django.http.HttpRequest, optional): + The current HTTP request. + + check_existence (bool, optional): + Whether or not existence checks should be performed against + the upstream repository. + + This argument defaults to ``True``. + + get_file_exists (callable, optional): + A callable that is used to determine if a file exists. + + This must be provided if ``check_existence`` is ``True``. + + diffcommit (reviewboard.diffviewer.models.diffcommit.DiffCommit, + optional): + The Diffcommit to attach the created FileDiffs to. + + validate_only (bool, optional): + Whether to just validate and not save. If ``True``, then this + won't populate the database at all and will return ``None`` + upon success. This defaults to ``False``. + + Returns: + list of reviewboard.diffviewer.models.filediff.FileDiff: + The created FileDiffs. + + If ``validate_only`` is ``True``, the returned list will be empty. + """ + from reviewboard.diffviewer.diffutils import convert_to_unicode + from reviewboard.diffviewer.models import FileDiff + + files, parser, parent_commit_id, parent_files = _prepare_file_list( + diff_file_contents=diff_file_contents, + parent_diff_file_contents=parent_diff_file_contents, + repository=repository, + request=request, + basedir=basedir, + check_existence=check_existence, + get_file_exists=get_file_exists, + base_commit_id=base_commit_id) + + encoding_list = repository.get_encoding_list() + filediffs = [] + + for f in files: + parent_file = None + orig_rev = None + parent_content = b'' + + if f.origFile in parent_files: + parent_file = parent_files[f.origFile] + parent_content = parent_file.data + orig_rev = parent_file.origInfo + + # If there is a parent file there is not necessarily an original + # revision for the parent file in the case of a renamed file in + # git. + if not orig_rev: + if parent_commit_id and f.origInfo != PRE_CREATION: + orig_rev = parent_commit_id + else: + orig_rev = f.origInfo + + orig_file = convert_to_unicode(f.origFile, encoding_list)[1] + dest_file = convert_to_unicode(f.newFile, encoding_list)[1] + + if f.deleted: + status = FileDiff.DELETED + elif f.moved: + status = FileDiff.MOVED + elif f.copied: + status = FileDiff.COPIED + else: + status = FileDiff.MODIFIED + + filediff = FileDiff( + diffset=diffset, + commit=diffcommit, + source_file=parser.normalize_diff_filename(orig_file), + dest_file=parser.normalize_diff_filename(dest_file), + source_revision=smart_unicode(orig_rev), + dest_detail=f.newInfo, + binary=f.binary, + status=status) + + filediff.extra_data = { + 'is_symlink': f.is_symlink, + } + + if (parent_file and + (parent_file.moved or parent_file.copied) and + parent_file.insert_count == 0 and + parent_file.delete_count == 0): + filediff.extra_data['parent_moved'] = True + + if not validate_only: + # This state all requires making modifications to the database. + # We only want to do this if we're saving. + filediff.diff = f.data + filediff.parent_diff = parent_content + + filediff.set_line_counts(raw_insert_count=f.insert_count, + raw_delete_count=f.delete_count) + + filediffs.append(filediff) + + if filediffs: + FileDiff.objects.bulk_create(filediffs) + num_filediffs = len(filediffs) + + if diffset.file_count is None: + diffset.reinit_file_count() + else: + diffset.file_count += num_filediffs + diffset.save(update_fields=('file_count',)) + + if diffcommit is not None: + diffcommit.file_count = len(filediffs) + diffcommit.save(update_fields=('file_count',)) + + return filediffs + + +def _prepare_file_list(diff_file_contents, parent_diff_file_contents, + repository, request, basedir, check_existence, + get_file_exists=None, base_commit_id=None): + """Extract the list of files from the diff. + + Args: + diff_file_contents (bytes): + The contents of the diff. + + parent_diff_file_contents (bytes): + The contents of the parent diff, if any. + + repository (reviewboard.scmtools.models.Repository): + The repository against which the diff was created. + + request (django.http.HttpRequest): + The current HTTP request. + + basedir (unicode): + The base directory to prepend to all file paths in the diff. + + check_existence (bool): + Whether or not existence checks should be performed against + the upstream repository. + + get_file_exists (callable, optional): + A callable to use to determine if a file exists in the repository. + + This argument must be provided if ``check_existence`` is ``True``. + + base_commit_id (unicode, optional): + The ID of the commit that the diff is based upon. This is + needed by some SCMs or hosting services to properly look up + files, if the diffs represent blob IDs instead of commit IDs + and the service doesn't support those lookups. + + Returns: + tuple: + A tuple of the following: + + * The files in the diff. (:py:class:`list` of + :py:class:`ParsedDiffFile`) + * The diff parser. + (:py:class:`reviewboard.diffviewer.parser.DiffParser`) + * The parent commit ID or ``None`` if not applicable. + (:py:class:`unicode`) + * A dictionary of files in the parent diff. (:py:class:`dict`) + + Raises: + reviewboard.diffviewer.errors.EmptyDiffError: + The diff contains no files. + + ValueError: + ``check_existence`` was ``True`` but ``get_file_exists`` was not + provided. + """ + if check_existence and get_file_exists is None: + raise ValueError('Must provide get_file_exists when check_existence ' + 'is True') + + tool = repository.get_scmtool() + parser = tool.get_parser(diff_file_contents) + files = list(_process_files( + parser=parser, + basedir=basedir, + repository=repository, + base_commit_id=base_commit_id, + request=request, + check_existence=(check_existence and + not parent_diff_file_contents), + get_file_exists=get_file_exists)) + + if len(files) == 0: + raise EmptyDiffError(_('The diff is empty.')) + + # Sort the files so that header files come before implementation + # files. + files.sort(cmp=_compare_files, key=lambda f: f.origFile) + + parent_files = {} + + # This is used only for tools like Mercurial that use atomic changeset + # IDs to identify all file versions. but not individual file version + # IDs. + parent_commit_id = None + + if parent_diff_file_contents: + diff_filenames = {f.origFile for f in files} + parent_parser = tool.get_parser(parent_diff_file_contents) + + # If the user supplied a base diff, we need to parse it and later + # apply each of the files that are in main diff. + parent_files = { + f.newFile: f + for f in _process_files( + get_file_exists=get_file_exists, + parser=parent_parser, + basedir=basedir, + repository=repository, + base_commit_id=base_commit_id, + request=request, + check_existence=check_existence, + limit_to=diff_filenames) + } + + # This will return a non-None value only for tools that use commit + # IDs to identify file versions as opposed to file revision IDs. + parent_commit_id = parent_parser.get_orig_commit_id() + + return files, parser, parent_commit_id, parent_files + + +def _process_files(parser, basedir, repository, base_commit_id, + request, get_file_exists=None, check_existence=False, + limit_to=None): + """Collect metadata about files in the parser. + + Args: + parser (reviewboard.diffviewer.parser.DiffParser): + A DiffParser instance for the diff. + + basedir (unicode): + The base directory to prepend to all file paths in the diff. + + repository (reviewboard.scmtools.models.Repository): + The repository that the diff was created against. + + base_commit_id (unicode): + The ID of the commit that the diff is based upon. This is + needed by some SCMs or hosting services to properly look up + files, if the diffs represent blob IDs instead of commit IDs + and the service doesn't support those lookups. + + request (django.http.HttpRequest): + The current HTTP request. + + check_existence (bool, optional): + Whether or not existence checks should be performed against + the upstream repository. + + get_file_exists (callable, optional): + A callable to use to determine if a given file exists in the + repository. + + If ``check_existence`` is ``True`` this argument must be + provided. + + limit_to (list of unicode, optional): + A list of filenames to limit the results to. + + Yields: + reviewboard.diffviewer.parser.ParsedDiffFile: + The files present in the diff. + + Raises: + ValueError: + ``check_existence`` was ``True`` but ``get_file_exists`` was not + provided. + """ + if check_existence and get_file_exists is None: + raise ValueError('Must provide get_file_exists when check_existence ' + 'is True') + + tool = repository.get_scmtool() + + for f in parser.parse(): + source_filename, source_revision = tool.parse_diff_revision( + f.origFile, + f.origInfo, + moved=f.moved, + copied=f.copied) + + dest_filename = _normalize_filename(f.newFile, basedir) + source_filename = _normalize_filename(source_filename, + basedir) + + if limit_to is not None and dest_filename not in limit_to: + # This file isn't actually needed for the diff, so save + # ourselves a remote file existence check and some storage. + continue + + # FIXME: this would be a good place to find permissions errors + if (source_revision != PRE_CREATION and + source_revision != UNKNOWN and + not f.binary and + not f.deleted and + not f.moved and + not f.copied and + (check_existence and + not get_file_exists(source_filename, + source_revision, + base_commit_id=base_commit_id, + request=request))): + raise FileNotFoundError(source_filename, source_revision, + base_commit_id) + + f.origFile = source_filename + f.origInfo = source_revision + f.newFile = dest_filename + + yield f + + +def _compare_files(filename1, filename2): + """Compare two filenames, giving precedence to header files. + + Args: + filename1 (unicode): + The first filename to compare. + + filename2 (unicode): + The second filename to compare. + + Returns: + int: + An ordering (-1, 0, or 1, representing less than, equal, + or greater than, respectively) of the filenames. + + """ + if filename1.find('.') != -1 and filename2.find('.') != -1: + basename1, ext1 = filename1.rsplit('.', 1) + basename2, ext2 = filename2.rsplit('.', 1) + + if basename1 == basename2: + if (ext1 in _HEADER_EXTENSIONS and ext2 in _IMPL_EXTENSIONS): + return -1 + elif (ext1 in _IMPL_EXTENSIONS and ext2 in _HEADER_EXTENSIONS): + return 1 + + # Python 3 equivalent to cmp in Python 2. + # + # See: + # https://docs.python.org/3.0/whatsnew/3.0.html#ordering-comparisons. + return (filename1 > filename2) - (filename1 < filename2) + + +def _normalize_filename(filename, basedir): + """Normalize a filename to be relative to the repository root. + + Args: + filename (unicode): + The filename to normalize. + + basedir (unicode): + The base directory to prepend to all file paths in the diff. + + Returns: + unicode: + The filename relative to the repository root. + """ + if filename.startswith('/'): + return filename + + return os.path.join(basedir, filename).replace('\\', '/') diff --git a/reviewboard/diffviewer/managers.py b/reviewboard/diffviewer/managers.py index bb3cbf640c4e8f997b8c6c9410963c3e731854c1..56f91d69c9d5a928ee48be2e2d415203d23a2a7a 100644 --- a/reviewboard/diffviewer/managers.py +++ b/reviewboard/diffviewer/managers.py @@ -1,23 +1,23 @@ +"""Managers for reviewboard.diffviewer.models.""" + from __future__ import unicode_literals import bz2 import gc import hashlib -import os import warnings from django.conf import settings from django.db import models, reset_queries, connection from django.db.models import Count, Q from django.db.utils import IntegrityError -from django.utils.encoding import smart_unicode from django.utils.six.moves import range from django.utils.translation import ugettext as _ from djblets.siteconfig.models import SiteConfiguration from reviewboard.diffviewer.differ import DiffCompatVersion -from reviewboard.diffviewer.errors import DiffTooBigError, EmptyDiffError -from reviewboard.scmtools.core import PRE_CREATION, UNKNOWN, FileNotFoundError +from reviewboard.diffviewer.errors import DiffTooBigError +from reviewboard.diffviewer.filediff_creator import create_filediffs class FileDiffManager(models.Manager): @@ -380,33 +380,46 @@ class RawFileDiffDataManager(models.Manager): return hasher.hexdigest() -class DiffSetManager(models.Manager): - """A custom manager for DiffSet objects. +class BaseDiffManager(models.Manager): + """A base manager class for creating models out of uploaded diffs""" - This includes utilities for creating diffsets based on the data from form - uploads, webapi requests, and upstream repositories. - """ + def create_from_data(self, *args, **kwargs): + """Create a model instance from data. - # Extensions used for intelligent sorting of header files - # before implementation files. - HEADER_EXTENSIONS = ["h", "H", "hh", "hpp", "hxx", "h++"] - IMPL_EXTENSIONS = ["c", "C", "cc", "cpp", "cxx", "c++", "m", "mm", "M"] + Subclasses must implement this method. + + See :py:meth:`create_from_upload` for the fields that will be passed to + this method. + + Returns: + django.db.models.Model: + The model instance. + """ + raise NotImplementedError def create_from_upload(self, repository, diff_file, parent_diff_file=None, - diffset_history=None, basedir=None, request=None, - base_commit_id=None, validate_only=False, **kwargs): - """Create a DiffSet from a form upload. + request=None, validate_only=False, **kwargs): + """Create a model instance from a form upload. This parses a diff and optional parent diff covering one or more files, - validates, and constructs :py:class:`DiffSets - ` and - :py:class:`FileDiffs ` - representing the diff. + validates, and constructs either: + + * a :py:class:`~reviewboard.diffviewer.models.diffset.DiffSet` or + * a :py:class:`~reviewboard.diffviewer.models.diffcommit.DiffCommit` + + and the child :py:class:`FileDiffs + ` representing the + diff. This can optionally validate the diff without saving anything to the database. In this case, no value will be returned. Instead, callers should take any result as success. + This function also accepts a number of keyword arguments when creating + a :py:class:`~reviewboard.diffviewer.model.DiffCommit`. In that case, + the following fields are required (except for the committer fields when + the underlying SCM does not distinguish betwen author and committer): + Args: repository (reviewboard.scmtools.models.Repository): The repository the diff applies to. @@ -414,32 +427,27 @@ class DiffSetManager(models.Manager): diff_file (django.core.files.uploadedfile.UploadedFile): The diff file uploaded in the form. - parent_diff_file (django.core.files.uploadedfile.UploadedFile, optional): + parent_diff_file (django.core.files.uploadedfile.UploadedFile, + optional): The parent diff file uploaded in the form. - diffset_history (reviewboard.diffviewer.models.diffset_history. - DiffSetHistory, optional): - The history object to associate the DiffSet with. This is - not required if using ``validate_only=True``. - - basedir (unicode, optional): - The base directory to prepend to all file paths in the diff. - request (django.http.HttpRequest, optional): The current HTTP request, if any. This will result in better logging. - base_commit_id (unicode, optional): - The ID of the commit that the diff is based upon. This is - needed by some SCMs or hosting services to properly look up - files, if the diffs represent blob IDs instead of commit IDs - and the service doesn't support those lookups. - validate_only (bool, optional): Whether to just validate and not save. If ``True``, then this won't populate the database at all and will return ``None`` upon success. This defaults to ``False``. + **kwargs (dict): + Additional keyword arguments to pass to + :py:meth:`create_from_data`. + + See :py:meth:`~DiffSetManager.create_from_data` and + :py:meth:`DiffCommitManager.create_from_data` for acceptable + keyword arguments. + Returns: reviewboard.diffviewer.models.diffset.DiffSet: The resulting DiffSet stored in the database, if processing @@ -470,11 +478,12 @@ class DiffSetManager(models.Manager): to Git. """ if 'save' in kwargs: - warnings.warn('The save parameter to ' - 'DiffSet.objects.create_from_upload is deprecated. ' - 'Please set validate_only instead.', - DeprecationWarning) - validate_only = not kwargs['save'] + warnings.warn( + 'The save parameter to %s.objects.create_from_upload is ' + 'deprecated. Please set validate_only instead.' + % type(self.model).__name__, + DeprecationWarning) + validate_only = not kwargs.pop('save') siteconfig = SiteConfiguration.objects.get_current() max_diff_size = siteconfig.get('diffviewer_max_diff_size') @@ -482,12 +491,12 @@ class DiffSetManager(models.Manager): if max_diff_size > 0: if diff_file.size > max_diff_size: raise DiffTooBigError( - _('The supplied diff file is too large'), + _('The supplied diff file is too large.'), max_diff_size=max_diff_size) if parent_diff_file and parent_diff_file.size > max_diff_size: raise DiffTooBigError( - _('The supplied parent diff file is too large'), + _('The supplied parent diff file is too large.'), max_diff_size=max_diff_size) if parent_diff_file: @@ -503,18 +512,163 @@ class DiffSetManager(models.Manager): diff_file_contents=diff_file.read(), parent_diff_file_name=parent_diff_file_name, parent_diff_file_contents=parent_diff_file_contents, - diffset_history=diffset_history, - basedir=basedir, request=request, + validate_only=validate_only, + **kwargs) + + +class DiffCommitManager(BaseDiffManager): + """A custom manager for DiffCommit objects. + + This includes utilities for creating diffsets based on the data from form + uploads, webapi requests, and upstream repositories. + """ + + def create_from_data(self, + repository, + diff_file_name, + diff_file_contents, + parent_diff_file_name, + parent_diff_file_contents, + diffset, + commit_id, + parent_id, + commit_message, + author_name, + author_email, + author_date, + request=None, + committer_name=None, + committer_email=None, + committer_date=None, + base_commit_id=None, + check_existence=True, + validate_only=False): + """Create a DiffCommit from raw diff data. + + Args: + repository (reviewboard.scmtools.models.Repository): + The repository the diff was posted against. + + diff_file_name (unicode): + The name of the diff file. + + diff_file_contents (bytes): + The contents of the diff file. + + parent_diff_file_name (unicode): + The name of the parent diff file. + + parent_diff_file_contents (bytes): + The contents of the parent diff file. + + diffset (reviewboard.diffviewer.models.diffset.DiffSet): + The DiffSet to attach the created DiffCommit to. + + commit_id (unicode): + The unique identifier of the commit. + + parent_id (unicode): + The unique identifier of the parent commit. + + commit_message (unicode): + The commit message. + + author_name (unicode): + The author's name. + + author_email (unicode): + The author's e-mail address. + + author_date (datetime.datetime): + The date and time that the commit was authored. + + request (django.http.HttpRequest, optional): + The HTTP request from the client. + + committer_name (unicode, optional): + The committer's name. + + committer_email (unicode, optional) + The committer's e-mail address. + + committer_date (datetime.datetime, optional): + The date and time that the commit was committed. + + base_commit_id (unicode, optional): + The ID of the commit that the diff is based upon. This is + needed by some SCMs or hosting services to properly look up + files, if the diffs represent blob IDs instead of commit IDs + and the service doesn't support those lookups. + + check_existence (bool, optional): + Whether or not existence checks should be performed against + the upstream repository. + + validate_only (bool, optional): + Whether to just validate and not save. If ``True``, then this + won't populate the database at all and will return ``None`` + upon success. This defaults to ``False``. + + Returns: + reviewboard.diffviewer.models.diffcommit.DiffCommit: + The created model instance. + """ + diffcommit = self.model( + filename=diff_file_name, + diffset=diffset, + commit_id=commit_id, + parent_id=parent_id, + author_name=author_name, + author_email=author_email, + author_date=author_date, + commit_message=commit_message, + committer_name=committer_name, + committer_email=committer_email, + committer_date=committer_date) + + if not validate_only: + diffcommit.save() + + create_filediffs( + get_file_exists=repository.get_file_exists, + diff_file_contents=diff_file_contents, + parent_diff_file_contents=parent_diff_file_contents, + repository=repository, + request=request, + basedir='', base_commit_id=base_commit_id, - validate_only=validate_only) + diffset=diffset, + diffcommit=diffcommit, + validate_only=validate_only, + check_existence=check_existence) + + if validate_only: + return None + + return diffcommit - def create_from_data(self, repository, diff_file_name, diff_file_contents, + +class DiffSetManager(BaseDiffManager): + """A custom manager for DiffSet objects. + + This includes utilities for creating diffsets based on the data from form + uploads, webapi requests, and upstream repositories. + """ + + def create_from_data(self, + repository, + diff_file_name, + diff_file_contents, parent_diff_file_name=None, parent_diff_file_contents=None, - diffset_history=None, basedir=None, request=None, - base_commit_id=None, check_existence=True, - validate_only=False, **kwargs): + diffset_history=None, + basedir=None, + request=None, + base_commit_id=None, + check_existence=True, + validate_only=False, + **kwargs): """Create a DiffSet from raw diff data. This parses a diff and optional parent diff covering one or more files, @@ -570,6 +724,9 @@ class DiffSetManager(models.Manager): won't populate the database at all and will return ``None`` upon success. This defaults to ``False``. + **kwargs (dict): + Additional keyword arguments. + Returns: reviewboard.diffviewer.models.diffset.DiffSet: The resulting DiffSet stored in the database, if processing @@ -595,9 +752,6 @@ class DiffSetManager(models.Manager): could not be used to look up the file. This is applicable only to Git. """ - from reviewboard.diffviewer.diffutils import convert_to_unicode - from reviewboard.diffviewer.models import FileDiff - if 'save' in kwargs: warnings.warn('The save parameter to ' 'DiffSet.objects.create_from_data is deprecated. ' @@ -605,52 +759,9 @@ class DiffSetManager(models.Manager): DeprecationWarning) validate_only = not kwargs['save'] - tool = repository.get_scmtool() - parser = tool.get_parser(diff_file_contents) - - files = list(self._process_files( - parser, - basedir, - repository, - base_commit_id, - request, - check_existence=check_existence and not parent_diff_file_contents)) - - # Parse the diff - if len(files) == 0: - raise EmptyDiffError(_("The diff file is empty")) - - # Sort the files so that header files come before implementation. - files.sort(cmp=self._compare_files, key=lambda f: f.origFile) - - # Parse the parent diff - parent_files = {} - - # This is used only for tools like Mercurial that use atomic changeset - # IDs to identify all file versions but not individual file version - # IDs. - parent_commit_id = None - - if parent_diff_file_contents: - diff_filenames = set([f.origFile for f in files]) - - parent_parser = tool.get_parser(parent_diff_file_contents) - - # If the user supplied a base diff, we need to parse it and - # later apply each of the files that are in the main diff - for f in self._process_files(parent_parser, basedir, - repository, base_commit_id, request, - check_existence=check_existence, - limit_to=diff_filenames): - parent_files[f.newFile] = f - - # This will return a non-None value only for tools that use - # commit IDs to identify file versions as opposed to file revision - # IDs. - parent_commit_id = parent_parser.get_orig_commit_id() - diffset = self.model( - name=diff_file_name, revision=0, + name=diff_file_name, + revision=0, basedir=basedir, history=diffset_history, repository=repository, @@ -660,142 +771,52 @@ class DiffSetManager(models.Manager): if not validate_only: diffset.save() - encoding_list = repository.get_encoding_list() - filediffs = [] - - for f in files: - parent_file = None - orig_rev = None - parent_content = b'' - - if f.origFile in parent_files: - parent_file = parent_files[f.origFile] - parent_content = parent_file.data - orig_rev = parent_file.origInfo - - # If there is a parent file there is not necessarily an original - # revision for the parent file in the case of a renamed file in - # git. - if not orig_rev: - if parent_commit_id and f.origInfo != PRE_CREATION: - orig_rev = parent_commit_id - else: - orig_rev = f.origInfo - - enc, orig_file = convert_to_unicode(f.origFile, encoding_list) - enc, dest_file = convert_to_unicode(f.newFile, encoding_list) - - if f.deleted: - status = FileDiff.DELETED - elif f.moved: - status = FileDiff.MOVED - elif f.copied: - status = FileDiff.COPIED - else: - status = FileDiff.MODIFIED - - filediff = FileDiff( - diffset=diffset, - source_file=parser.normalize_diff_filename(orig_file), - dest_file=parser.normalize_diff_filename(dest_file), - source_revision=smart_unicode(orig_rev), - dest_detail=f.newInfo, - binary=f.binary, - status=status) - - filediff.extra_data = { - 'is_symlink': f.is_symlink, - } - - if (parent_file and - (parent_file.moved or parent_file.copied) and - parent_file.insert_count == 0 and - parent_file.delete_count == 0): - filediff.extra_data['parent_moved'] = True - - if not validate_only: - # This state all requires making modifications to the database. - # We only want to do this if we're saving. - filediff.diff = f.data - filediff.parent_diff = parent_content - - filediff.set_line_counts(raw_insert_count=f.insert_count, - raw_delete_count=f.delete_count) - - filediffs.append(filediff) + create_filediffs( + get_file_exists=repository.get_file_exists, + diff_file_contents=diff_file_contents, + parent_diff_file_contents=parent_diff_file_contents, + repository=repository, + request=request, + basedir=basedir, + base_commit_id=base_commit_id, + diffset=diffset, + check_existence=check_existence, + validate_only=validate_only + ) if validate_only: return None - if filediffs: - FileDiff.objects.bulk_create(filediffs) - return diffset - def _normalize_filename(self, filename, basedir): - """Normalize a file name to be relative to the repository root.""" - if filename.startswith('/'): - return filename - - return os.path.join(basedir, filename).replace('\\', '/') - - def _process_files(self, parser, basedir, repository, base_commit_id, - request, check_existence=False, limit_to=None): - tool = repository.get_scmtool() - - for f in parser.parse(): - source_filename, source_revision = tool.parse_diff_revision( - f.origFile, - f.origInfo, - moved=f.moved, - copied=f.copied) - - dest_filename = self._normalize_filename(f.newFile, basedir) - source_filename = self._normalize_filename(source_filename, - basedir) - - if limit_to is not None and dest_filename not in limit_to: - # This file isn't actually needed for the diff, so save - # ourselves a remote file existence check and some storage. - continue - - # FIXME: this would be a good place to find permissions errors - if (source_revision != PRE_CREATION and - source_revision != UNKNOWN and - not f.binary and - not f.deleted and - not f.moved and - not f.copied and - (check_existence and - not repository.get_file_exists(source_filename, - source_revision, - base_commit_id=base_commit_id, - request=request))): - raise FileNotFoundError(source_filename, source_revision, - base_commit_id) - - f.origFile = source_filename - f.origInfo = source_revision - f.newFile = dest_filename - - yield f - - def _compare_files(self, filename1, filename2): - """ - Compares two files, giving precedence to header files over source - files. This allows the resulting list of files to be more - intelligently sorted. + def create_empty(self, repository, diffset_history=None, **kwargs): + """Create a DiffSet with no attached FileDiffs. + + An empty DiffSet must be created before + :py:class:`DiffCommits ` can be added to it. When the DiffCommits are created, + the :py:class:`FileDiffs ` will be attached to the DiffSet. + + Args: + repository (reviewboard.scmtools.models.Repository): + The repository to create the DiffSet in. + + diffset_history (reviewboard.diffviewer.models.diffset_history. + DiffSetHistory, optional): + An optional DiffSetHistory instance to attach the DiffSet to. + + **kwargs (dict): + Additional keyword arguments to pass to :py:meth:`create`. + + Returns: + reviewboard.diffviewer.models.diffset.DiffSet: + The created DiffSet. """ - if filename1.find('.') != -1 and filename2.find('.') != -1: - basename1, ext1 = filename1.rsplit('.', 1) - basename2, ext2 = filename2.rsplit('.', 1) - - if basename1 == basename2: - if (ext1 in self.HEADER_EXTENSIONS and - ext2 in self.IMPL_EXTENSIONS): - return -1 - elif (ext1 in self.IMPL_EXTENSIONS and - ext2 in self.HEADER_EXTENSIONS): - return 1 - - return cmp(filename1, filename2) + kwargs.setdefault('revision', 0) + return super(DiffSetManager, self).create( + name='diff', + history=diffset_history, + repository=repository, + diffcompat=DiffCompatVersion.DEFAULT, + **kwargs) diff --git a/reviewboard/diffviewer/models/diffcommit.py b/reviewboard/diffviewer/models/diffcommit.py index cbe7ede93da970412528e9a16fd736d4f51db19c..fdd9f40dddd6f821be405a9309ff01f7d49b6cac 100644 --- a/reviewboard/diffviewer/models/diffcommit.py +++ b/reviewboard/diffviewer/models/diffcommit.py @@ -10,6 +10,7 @@ from django.utils.functional import cached_property from django.utils.translation import ugettext_lazy as _ from djblets.db.fields import JSONField +from reviewboard.diffviewer.managers import DiffCommitManager from reviewboard.diffviewer.models.diffset import DiffSet from reviewboard.diffviewer.models.mixins import FileDiffCollectionMixin from reviewboard.diffviewer.validators import (COMMIT_ID_LENGTH, @@ -107,6 +108,8 @@ class DiffCommit(FileDiffCollectionMixin, models.Model): extra_data = JSONField(null=True) + objects = DiffCommitManager() + @property def author(self): """The author's name and e-mail address. diff --git a/reviewboard/diffviewer/tests/test_diffcommit_manager.py b/reviewboard/diffviewer/tests/test_diffcommit_manager.py new file mode 100644 index 0000000000000000000000000000000000000000..cb1bca194f204bc3782c2b3d2b537a4ba17af71a --- /dev/null +++ b/reviewboard/diffviewer/tests/test_diffcommit_manager.py @@ -0,0 +1,74 @@ +"""Tests for reviewboard.diffviewer.managers.DiffCommitManager.""" + +from __future__ import unicode_literals + +from dateutil.parser import parse as parse_date +from kgb import SpyAgency + +from reviewboard.diffviewer.models import DiffCommit, DiffSet +from reviewboard.testing.testcase import TestCase + + +class DiffCommitManagerTests(SpyAgency, TestCase): + """Unit tests for DiffCommitManager.""" + + fixtures = ['test_scmtools'] + + def test_create_from_data(self): + """Testing DiffCommitManager.create_from_data""" + diff = ( + b'diff --git a/README b/README\n' + b'index d6613f5..5b50866 100644\n' + b'--- README\n' + b'+++ README\n' + b'@ -1,1 +1,1 @@\n' + b'-blah..\n' + b'+blah blah\n' + ) + + repository = self.create_repository(tool_name='Test') + self.spy_on(repository.get_file_exists, + call_fake=lambda *args, **kwargs: True) + + diffset = DiffSet.objects.create_empty( + repository=repository, + basedir='', + revision=1) + + raw_date = '2000-01-01 00:00:00-0600' + parsed_date = parse_date(raw_date) + commit = DiffCommit.objects.create_from_data( + repository=repository, + diff_file_name='diff', + diff_file_contents=diff, + parent_diff_file_name=None, + parent_diff_file_contents=b'', + request=None, + commit_id='r1', + parent_id='r0', + author_name='Author', + author_email='author@example.com', + author_date=parsed_date, + committer_name='Committer', + committer_email='committer@example.com', + committer_date=parsed_date, + commit_message='Description', + diffset=diffset) + + self.assertEqual(commit.files.count(), 1) + self.assertEqual(diffset.files.count(), commit.files.count()) + self.assertEqual(diffset.commit_count, 1) + + # We have to compare regular equality and equality after applying + # ``strftime`` because two datetimes with different timezone info + # may be equal + self.assertEqual(parsed_date, commit.author_date) + self.assertEqual(parsed_date, commit.committer_date) + + self.assertEqual( + raw_date, + commit.author_date.strftime(DiffCommit.ISO_DATE_FORMAT)) + + self.assertEqual( + raw_date, + commit.committer_date.strftime(DiffCommit.ISO_DATE_FORMAT)) diff --git a/reviewboard/diffviewer/tests/test_diffset_manager.py b/reviewboard/diffviewer/tests/test_diffset_manager.py index 3cae5508110703c692698b12ecc686d94893acc3..8cb81c5fec7f7db34a5ccbcf391428bf424f33fd 100644 --- a/reviewboard/diffviewer/tests/test_diffset_manager.py +++ b/reviewboard/diffviewer/tests/test_diffset_manager.py @@ -2,7 +2,7 @@ from __future__ import unicode_literals from kgb import SpyAgency -from reviewboard.diffviewer.models import DiffSet, FileDiff +from reviewboard.diffviewer.models import DiffSet, DiffSetHistory, FileDiff from reviewboard.testing import TestCase @@ -86,3 +86,40 @@ class DiffSetManagerTests(SpyAgency, TestCase): self.assertIsNone(diffset) self.assertEqual(DiffSet.objects.count(), 0) self.assertEqual(FileDiff.objects.count(), 0) + + def test_create_empty(self): + """Testing DiffSetManager.create_empty""" + repository = self.create_repository(tool_name='Test') + history = DiffSetHistory.objects.create() + + diffset = DiffSet.objects.create_empty( + repository=repository, + diffset_history=history) + + self.assertEqual(diffset.files.count(), 0) + self.assertEqual(diffset.revision, 1) + + def test_create_empty_with_revision(self): + """Testing DiffSetManager.create_empty with revision""" + repository = self.create_repository(tool_name='Test') + history = DiffSetHistory.objects.create() + + diffset = DiffSet.objects.create_empty( + repository=repository, + diffset_history=history, + revision=10) + + self.assertEqual(diffset.files.count(), 0) + self.assertEqual(diffset.history, history) + self.assertEqual(diffset.revision, 10) + + def test_create_empty_without_history(self): + """Testing DiffSetManager.create_empty without diffset_history""" + repository = self.create_repository(tool_name='Test') + + diffset = DiffSet.objects.create_empty( + repository=repository) + + self.assertEqual(diffset.files.count(), 0) + self.assertIsNone(diffset.history) + self.assertEqual(diffset.revision, 0) diff --git a/reviewboard/diffviewer/tests/test_filediff_creator.py b/reviewboard/diffviewer/tests/test_filediff_creator.py new file mode 100644 index 0000000000000000000000000000000000000000..8cb2fc5df2f9cf2e1e6f63769209baa3047ae611 --- /dev/null +++ b/reviewboard/diffviewer/tests/test_filediff_creator.py @@ -0,0 +1,99 @@ +"""Tests for reviewboard.diffviewer.filediff_creator.""" + +from __future__ import unicode_literals + +from django.utils.timezone import now + +from reviewboard.diffviewer.filediff_creator import create_filediffs +from reviewboard.diffviewer.models import DiffCommit, DiffSet +from reviewboard.testing import TestCase + + +class FileDiffCreatorTests(TestCase): + """Tests for reviewboard.diffviewer.filediff_creator.""" + + fixtures = ['test_scmtools'] + + def test_create_filediffs_file_count(self): + """Testing create_filediffs() with a DiffSet""" + repository = self.create_repository() + diffset = self.create_diffset(repository=repository) + + self.assertEqual(diffset.file_count, 0) + + create_filediffs( + self.DEFAULT_GIT_FILEDIFF_DATA, + None, + repository=repository, + basedir='/', + base_commit_id='0' * 40, + diffset=diffset, + check_existence=False) + + diffset = DiffSet.objects.get(pk=diffset.pk) + + self.assertEqual(diffset.file_count, 1) + + def test_create_filediffs_commit_file_count(self): + """Testing create_filediffs() with a DiffSet and a DiffCommit""" + repository = self.create_repository() + diffset = DiffSet.objects.create_empty(repository=repository) + commits = [ + DiffCommit.objects.create( + diffset=diffset, + filename='diff', + author_name='Author Name', + author_email='author@example.com', + commit_message='Message', + author_date=now(), + commit_id='a' * 40, + parent_id='0' * 40), + DiffCommit.objects.create( + diffset=diffset, + filename='diff', + author_name='Author Name', + author_email='author@example.com', + commit_message='Message', + author_date=now(), + commit_id='b' * 40, + parent_id='a' * 40), + ] + + self.assertEqual(diffset.file_count, 0) + self.assertEqual(commits[0].file_count, 0) + + create_filediffs( + self.DEFAULT_GIT_FILEDIFF_DATA, + None, + repository=repository, + basedir='/', + base_commit_id='0' * 40, + diffset=diffset, + diffcommit=commits[0], + check_existence=False) + + diffset = DiffSet.objects.get(pk=diffset.pk) + commits[0] = DiffCommit.objects.get(pk=commits[0].pk) + + self.assertEqual(len(diffset.files.all()), 1) + self.assertEqual(diffset.file_count, 1) + self.assertEqual(len(commits[0].files.all()), 1) + self.assertEqual(commits[0].file_count, 1) + + create_filediffs( + self.DEFAULT_GIT_FILEDIFF_DATA, + None, + repository=repository, + basedir='/', + base_commit_id='0' * 40, + diffset=diffset, + diffcommit=commits[1], + check_existence=False) + + diffset = DiffSet.objects.get(pk=diffset.pk) + commits[1] = DiffCommit.objects.get(pk=commits[1].pk) + + self.assertEqual(len(diffset.files.all()), 2) + self.assertEqual(diffset.file_count, 2) + self.assertEqual(len(commits[1].files.all()), 1) + self.assertEqual(commits[1].file_count, 1) diff --git a/reviewboard/testing/testcase.py b/reviewboard/testing/testcase.py index 0599b5b7104c60b6146026368b02dfb5d831ca7f..6885146444d06d93f45cf9136cc1614e989d789d 100644 --- a/reviewboard/testing/testcase.py +++ b/reviewboard/testing/testcase.py @@ -475,7 +475,7 @@ class TestCase(FixturesCompilerMixin, DjbletsTestCase): def create_filediff(self, diffset, source_file='/test-file', dest_file='/test-file', source_revision='123', dest_detail='124', status=FileDiff.MODIFIED, - diff=DEFAULT_FILEDIFF_DATA, save=True): + diff=DEFAULT_FILEDIFF_DATA, commit=None, save=True): """Create a FileDiff for testing. The FileDiff is tied to the given DiffSet. It's populated with @@ -506,6 +506,10 @@ class TestCase(FixturesCompilerMixin, DjbletsTestCase): diff (bytes, optional): The diff contents. + commit (reviewboard.diffviewer.models.diffcommit.DiffCommit, + optional): + The commit to attach the FileDiff to. + save (bool, optional): Whether to automatically save the resulting object. @@ -520,7 +524,8 @@ class TestCase(FixturesCompilerMixin, DjbletsTestCase): source_revision=source_revision, dest_detail=dest_detail, status=status, - diff=diff) + diff=diff, + commit=commit) if save: filediff.save()