git-ubuntu

Merge ~racb/git-ubuntu:fix-rich-history-unescaped-patch into git-ubuntu:master

Proposed by Robie Basak on 2020-05-07

Status:	Merged
Merged at revision:	0376feae802c37ba6e71b7eca899230c32bc7904
Proposed branch:	~racb/git-ubuntu:fix-rich-history-unescaped-patch
Merge into:	git-ubuntu:master
Diff against target:	262 lines (+81/-58) 4 files modified doc/STYLE.md (+0/-9) gitubuntu/rich_history.py (+17/-15) gitubuntu/rich_history_test.py (+64/-14) gitubuntu/test_fixtures.py (+0/-20)
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Server Team CI bot	continuous-integration		Approve on 2020-05-11
Bryce Harrington		2020-05-07	Approve on 2020-05-09
Review via email: mp+383597@code.staging.launchpad.net

Commit message

Make Jenkins happy

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2020-05-07:

PASSED: Continuous integration, rev:e30452d73045c9a44dae172dae0029d79bbd58e8
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/512/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/512//rebuild

review: Approve (continuous-integration)

Revision history for this message

Bryce Harrington (bryce) wrote on 2020-05-09:

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

Revision history for this message

Robie Basak (racb) wrote on 2020-05-11:

Thanks. Agreed with the docstring change. I have updated the patchset and pushed. Now awaiting CI result before landing.

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2020-05-11:

PASSED: Continuous integration, rev:0376feae802c37ba6e71b7eca899230c32bc7904
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/513/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/513//rebuild