Code review comment for ~racb/git-ubuntu:fix-rich-history-unescaped-patch

Revision history for this message
Bryce Harrington (bryce) wrote :

8f84439e71276f94c56837872b97d3d9432bc33d
  - Test refactoring, to make it use a single repo rather than source
    and dest ones.
  - Drops the repo_fixture
  - LGTM
  √ Testsuite result: passed

1c52d81028861d18a43d09d18e926bbeb969fd2d
  - Adds xfail test for bug about patch in message

  - I would suggest some verbage changes to the test description:
    """Commit messages with patch-like contents should round trip

    If a commit message in rich history contains text in a patch-like
    format (such as a diff of some file), it should not prevent correct
    round-tripping. "git format-patch" followed by "git am" fails this
    test, for example.
    ...
    """
  - + :param repo: our standard repo fixture.
    Should be ":param Repo repo: our..."
  √ Testsuite result: passed

c2555935fcb8fc6285d60bcf5daa0599d7f6b0ec
  - Changes from using git am to git cherry-pick, the main implication
    is needing to hand it commit id's rather than patch mbox's.
  - This feels like it will be far more robust
  - One xfail test case is re-enabled
  √ Testsuite result: passed
  - LGTM

e30452d73045c9a44dae172dae0029d79bbd58e8
  - LGTM

This MP is well organized, first introducing an xfail test case that catches the bug, then the fix, and separating out a preliminary refactoring to support the test, and a cleanup after. I verified the xfail case does indeed fail initially, and is fixed in subsequent commits.

The fix itself seems a lot more robust to me, because it shifts to rely on git cherry-pick rather than git am and in my experience the former is a lot stricter and thus more reliable for our use case. cherry-pick leverages git's internal functionality, while git am's nature has it limited by text file i/o and the patch file format.

I have a couple suggestions on improvements for test case pydocs, but nothing major and no need for re-review.

review: Approve

« Back to merge proposal