-
-
/trunk/reviewboard/diffviewer/forms.py (Diff revision 1) I was going to say that this seems wrong to me, since it means we're clobbering the parent revision each time, but then I realized it's only for atomic commits. Can you make a note saying that, and also indicating above where you first declare parent_rev that it's only used for atomic commits?
-
/trunk/reviewboard/diffviewer/forms.py (Diff revision 1) This should check tool.uses_atomic_revisions instead. Also, blank line before this conditional, and since this is a multi-line conditional, use parens instead of \
-
/trunk/reviewboard/scmtools/core.py (Diff revision 1) As mentioned above, we shouldn't have this. Instead, hg should make sure to set uses_atomic_revisions in the constructor (which it currently doesn't do).
Get parent diffs working with Mercurial.
Review Request #841 — Created April 28, 2009 and submitted
Information | |
---|---|
cfc | |
Review Board SVN (deprecated) | |
Reviewers | |
reviewboard | |
The existing parent diff code apparently assumes that each version of a file has an ID independent of the ID for the whole revision. This doesn't work for Mercurial because Mercurial only has revision IDs, not file version IDs. This fix alters the behaviour of parent diffs with Mercurial to use the source ID of the parent diff for all files, not just files that are in the parent diff. I've added a method to SCMTool to ensure that the behaviour with git (or any other tool) is not affected.
Tested various diffs between revisions that are not in the master repository, including ones with changed files, added files and removed files.
CF
Change Summary:
Modified to use SCMTool.uses_atomic_revisions instead of get_uses_file_revision_ids(), and removed the latter. Added some comments around the parent_rev variable.
Diff: |
Revision 2 (+17 -2) |
---|
CF
Change Summary:
Updated as discussed; SCMTool now has a diff_uses_changeset_ids that is true for Mercurial and false for everything else. I also found and fixed a bug that caused some diffs to fail if a file had changed between the parent and new revisions but not between the base and parent revisions. In this case the original revision was seen as /dev/null instead of the base changeset ID. To fix this I added a member called origCset to parser.File; the Mercurial tool sets this to the original changeset of the diff even for files that are new (whereas origInfo is set to PRE_CREATION). This is used instead of origInfo when choosing the source revision for parent diffs.
Diff: |
Revision 3 (+23 -2) |
---|
-
-
-
/trunk/reviewboard/diffviewer/forms.py (Diff revision 3) Won't we hit this case with parent diffs? Presumably the file would be in the list, and we'd end up using parent_file.origInfo, which I imagine would be incorrect.
-
/trunk/reviewboard/diffviewer/forms.py (Diff revision 3) f.origInfo should be aligned with tool.diff_uses_changeset_ids. Can you do one check per line? So parent_cset would be on its own line.
-
/trunk/reviewboard/diffviewer/parser.py (Diff revision 3) I'd actually prefer that we spell it out. "origChangesetId" in both file and info. I know we're not super consistent there, but "Cset" isn't a very common abbreviation. Should also use this in forms.py for parent_cset. parent_changeset_id.
-
/trunk/reviewboard/scmtools/hg.py (Diff revision 3) Blank line after blocks, so before "self.uses_atomic_revisions"
CF
Change Summary:
Made suggested fixes. Note I've updated my repo since posting the first three diffs, so if you look at the diff between 3 and 4 you'll see some changes that are not part of this fix.
Diff: |
Revision 4 (+25 -2) |
---|