plumbing/object: add WalkCommitHistoryPost func

[mvdan]

No description provided.

[mvdan]

As for the test - tried modifying TestWalkerNext to use this func as a start, but surprisingly got the wrong number of commits - 10 instead of 8. I presume this is because my implementation doesn't check for duplicates, while the other does. Should it?

Also don't know how to test this func properly. Ideally, both funcs would have a test that verifies the order is correct. In practice, this test suite is a bit obscure and some pointers on how to do that with this mock git repository/history would be of great help.

[mvdan]

@smola @mcuadros

[codecov]

Codecov Report

Merging #305 into master will decrease coverage by 0.01%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
- Coverage   77.36%   77.34%   -0.02%     
==========================================
  Files         117      117              
  Lines        7995     8011      +16     
==========================================
+ Hits         6185     6196      +11     
- Misses       1152     1156       +4     
- Partials      658      659       +1

Impacted Files	Coverage Δ
plumbing/object/commit_walker.go	87.23% <87.5%> (+0.13%)	⬆️
plumbing/object/commit.go	72.93% <0%> (-2.26%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfbd64f...33db30d. Read the comment docs.

[mvdan]

A seen map got rid of the len error, so did that.

Perhaps the way to have the tests for the two funcs actually test the order would be to avoid SortCommits(commits) and instead check for the expected hashes in the correct order. Are any of those 8 commits merge commits?

[smola]

@mvdan for testing, you can call to WalkCommitHistoryPost passing a function that adds commit hashes to a slice, then you just check the slice.

[mvdan]

@smola that's what the other test already does (minus the sort). But my question is whether those set of commits contain any merge commits. If they don't, the order obtained from the two functions would be exactly the same and the tests would not be as useful.

[mvdan]

ping @smola

[smola]

@mvdan Sorry for the delay!

Yes. The basic fixture contains 2 merges. There is a copy of it at GitHub: https://github.com/git-fixtures/basic/commits/master where you can check the details.

[mvdan]

Awesome thanks - will update the PR with proper tests.

[mvdan]

Done, PTAL.

[smola]

Thanks @mvdan!